-
Notifications
You must be signed in to change notification settings - Fork 969
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
Drop old peers post v20 upgrade #4120
Drop old peers post v20 upgrade #4120
Conversation
5cf2048
to
7dcdc3f
Compare
@@ -212,7 +213,7 @@ class Peer : public std::enable_shared_from_this<Peer>, | |||
|
|||
std::string mRemoteVersion; | |||
uint32_t mRemoteOverlayMinVersion; | |||
uint32_t mRemoteOverlayVersion; | |||
std::optional<uint32_t> mRemoteOverlayVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this optional instead of 0, so that we don't drop pending peers when we don't know their version yet (this can happen if we've sent HELLO, but haven't received a response)
7dcdc3f
to
9addd76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good -- I suggested a simpler version for the way we loop over peers
src/overlay/OverlayManagerImpl.cpp
Outdated
{ | ||
maybeDrop<decltype(OverlayManagerImpl::PeersList::mPending)>( | ||
mPending, predicate, version, reason); | ||
maybeDrop<decltype(OverlayManagerImpl::PeersList::mAuthenticated)>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to do all this templating stuff: you can just call getPendingPeers
and getRandomAuthenticatedPeers
(that also gives you a list), if you don't want to randomize for the later, you can refactor it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
9addd76
to
de714e6
Compare
r+ de714e6 |
No description provided.