litep2p/peerset: Do not disconnect all peers on SetReservedPeers command
#6016
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`calculate_slot_adjustment` Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
reserved Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
lexnv
added
T0-node
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
dmitry-markin
approved these changes
Oct 21, 2024
substrate/client/network/src/litep2p/shim/notification/peerset.rs
Outdated
Show resolved
Hide resolved
substrate/client/network/src/litep2p/shim/notification/peerset.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Dmitry Markin <[email protected]>
Co-authored-by: Dmitry Markin <[email protected]>
Co-authored-by: Dmitry Markin <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
michalkucharczyk
approved these changes
Nov 5, 2024
substrate/client/network/src/litep2p/shim/notification/peerset.rs
Outdated
Show resolved
Hide resolved
Comment on lines
1002
to
1005
| match direction { | ||
| Direction::Inbound(_) => self.num_in >= self.max_in, | ||
| Direction::Outbound(_) => self.num_out >= self.max_out, | ||
| }; |
There was a problem hiding this comment.
nit: some of the blocks seems to be duplicated - maybe moving them to small functions would be better.
The same with increasing out/inbound count.
Not sure however if this would increase readability.
There was a problem hiding this comment.
This piece of code was replicated three times. Could be moved into one function that takes a disconnect_state and a keep_state.
| match peerset.next().await { | ||
| Some(PeersetNotificationCommand::CloseSubstream { peers }) => { | ||
| // This ensures we don't disconnect peers when receiving `SetReservedPeers`. | ||
| assert_eq!(peers.len(), 2); |
There was a problem hiding this comment.
would it make sense to check if common_perr is not in the peers?
| event => panic!("invalid event: {event:?}"), | ||
| } | ||
|
|
||
| // verify all three peers are counted as outbound peers and they don't count towards |
There was a problem hiding this comment.
dumb question: is outbound peer term used as an equivalent of reserved peer?
There was a problem hiding this comment.
I think that was a mistake, thanks for pointing that out 🙏
bkchr
approved these changes
Nov 5, 2024
Comment on lines
1002
to
1005
| match direction { | ||
| Direction::Inbound(_) => self.num_in >= self.max_in, | ||
| Direction::Outbound(_) => self.num_out >= self.max_out, | ||
| }; |
There was a problem hiding this comment.
This piece of code was replicated three times. Could be moved into one function that takes a disconnect_state and a keep_state.
Co-authored-by: Michal Kucharczyk <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
lexnv
added a commit
that referenced
this pull request
Nov 15, 2024
…mmand (#6016) Previously, when receiving the `SetReservedPeers { reserved }` all peers not in the `reserved` set were removed. This is incorrect, the intention of `SetReservedPeers` is to change the active set of reserved peers and disconnect previously reserved peers not in the new set. While at it, have added a few other improvements to make the peerset more robust: - `SetReservedPeers`: does not disconnect all peers - `SetReservedPeers`: if a reserved peer is no longer reserved, the peerset tries to move the peers to the regular set if the slots allow this move. This ensures the (now regular) peer counts towards slot allocation. - every 1 seconds: If we don't have enough connect peers, add the reserved peers to the list that the peerstore ignores. Reserved peers are already connected and the peerstore might return otherwise a reserved peer ### Next Steps - [x] More testing cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Dmitry Markin <[email protected]> Co-authored-by: Michal Kucharczyk <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when receiving the
SetReservedPeers { reserved }all peers not in thereservedset were removed.This is incorrect, the intention of
SetReservedPeersis to change the active set of reserved peers and disconnect previously reserved peers not in the new set.While at it, have added a few other improvements to make the peerset more robust:
SetReservedPeers: does not disconnect all peersSetReservedPeers: if a reserved peer is no longer reserved, the peerset tries to move the peers to the regular set if the slots allow this move. This ensures the (now regular) peer counts towards slot allocation.Next Steps
cc @paritytech/networking