Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEX - sharing peers with othes [continue #261] #284

Merged
merged 14 commits into from
Dec 4, 2024
Merged

Conversation

ikatson
Copy link
Owner

@ikatson ikatson commented Nov 28, 2024

#261 isn't being worked on, so I tried to finish it here

  • Do not compute live peers within every peer all the time - store them in a shared place

CC @izderadicka

@izderadicka
Copy link
Contributor

@ikatson Sorry, did not have much time recently - I see that you rewrite the logic as per discussion in #261

Logic is clear (though I'm still not convinced that previous logic was wrong). I like the idea having shared view of live peers - so it'll have also some timestamp, where it was last updated and if it will be older that X secs then refreshed? I guess it should be under RWLock to enable sharing and updates?

@ikatson
Copy link
Owner Author

ikatson commented Nov 30, 2024

@izderadicka I didn't mean the previous logic was wrong, just that it was hard to read.

Yeah I don't want to merge it until the live peer computation is optimized: there may be tens of thousands of known peers, but only a limited number of live ones. It doesn't make sense to iterate tens of thousands to get a few live peers.

Yeah, RwLock is one option. Can also use a broadcast channel or smth. RwLock probably simpler

@ikatson
Copy link
Owner Author

ikatson commented Dec 4, 2024

@izderadicka did that change, was harder than I thought, needed to refactor a few things here and there.

Seems to work perfectly.

@ikatson ikatson merged commit a1de63e into main Dec 4, 2024
5 checks passed
@izderadicka
Copy link
Contributor

@ikatson Thanks for finishing this one, unfortunately I did not have time now to follow.
Indeed it looks like a lot of additional refactoring was involved finally - something I'm not sure will be able to do, not closely familiar with the code.

The solution for keeping shared state in live_outgoing_peers in PeerStates looks very efficient to me, though from my perspective it's something I probably would not be able to come up with, as would be difficult for me to understand where exactly to add or remove addresses to/from it - final solution seem to be obvious, thanks do additional refactorings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants