validator-discovery: don't remove multiaddr of requested PeerIds
#4036
Conversation
ordian
added
A0-please_review
|
This would disconnect the peers before they are re-added. |
ordian
added
the
D3-trivial 🧸
|
Ok, I'll rework it to be a companion to paritytech/substrate#9964 and do what you suggested. |
ordian
changed the title
ordian
changed the title
PeerIds
ordian
added
B7-runtimenoteworthy
and removed
B0-silent
ordian
added
B0-silent
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
|
|
||
| let addr_to_remove: Vec<PeerId> = | ||
| state.previously_requested.difference(&new_peer_ids).cloned().collect(); | ||
| let multiaddr_to_add: HashSet<_> = newly_requested |
There was a problem hiding this comment.
This looks a bit hacky and inefficient. Also we will fail to add new addresses of a peer this way.
I think the cleanest solution would be to use set_reserved_peers with the full set and afterwards call the now changed remove_from_peers_set function to get rid of any dead peers.
There was a problem hiding this comment.
If you're thinking set_reserved_peers is more efficient, it's not: https://github.com/paritytech/substrate/blob/0e220090f61233db76bb2f705849e1773b62498c/client/network/src/service.rs#L1102-L1131.
Also we will fail to add new addresses of a peer this way.
See e0bc56a
There was a problem hiding this comment.
honestly, we should revisit that API at some point. The usage is proving to be pretty awkward. It should either be more low-level or more high-level. This mid-level, where we do the same stuff at the calling and at the called side makes no sense.
There was a problem hiding this comment.
e352c2d is using set_reserved_peers
ordian
added
A4-companion
|
Waiting for commit status. |
ghost
merged commit 
…4036) * validator-discovery: remove from peer set before inserting * bump spec versions * rework into a companion * fmt * fix * fix * one more time * one more try * one more try * Revert "one more try" This reverts commit ab6568d. * one more try * one more try * Revert "one more try" This reverts commit 8d7369f. * fix a warning * fix another warn * correct log * fix compilation * ffs * less cloning * Apply suggestions from code review Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> * add comments and a small refactoring * use set_reserved_peers * cargo update -p sp-io * rename added to num_peers * update Substrate Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com> Co-authored-by: parity-processbot <>
An alternative to #4034.
Should help with improving validation peerset connectivity on kusama (#3877).
companion to paritytech/substrate#9964.