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

Introduce proper mechanism for tracking other protocols peer sets #7006

Open
dmitry-markin opened this issue Dec 27, 2024 · 4 comments
Open
Labels
D2-substantial Can be fixed by an experienced coder with a working knowledge of the codebase. T0-node This PR/Issue is related to the topic “node”.

Comments

@dmitry-markin
Copy link
Contributor

Currently, the reserved nodes mechanism is used to make additional protocols track the sync protocol peer set.

This is done by subscribing to SyncEvent::PeerConnected(PeerId) and SyncEvent::PeerDisconnected(PeerId) sync events and adding/removing such peers to the reserved set of the protocol tracking the sync protocol peer set.

The reserved peers mechanism was intended for operators to control the peers a local node is connecting to, and its use to sync peer sets of the syncing protocol and other protocols is a hack. This leads to race conditions like #6573 (comment) when the sync event is missed and requires other hacks like network starter (attempted to get removed in #6400).

Instead of tracking sync peer set via sync events a proper mechanism should be introduced. Possible implications should be carefully considered before implementing it: for example, sync events allow filtering the set of peers by the protocol logic before marking them as reserved, and a purelyPeerStore/PeerSet based mechanism of tracking peers won't allow this.

@dmitry-markin dmitry-markin added T0-node This PR/Issue is related to the topic “node”. D2-substantial Can be fixed by an experienced coder with a working knowledge of the codebase. labels Dec 27, 2024
@bkchr
Copy link
Member

bkchr commented Dec 27, 2024

This all still comes from the old days where the sync protocol was the default, builtin protocol. Reserved nodes today is already ignored by other protocols like the parachain protocol completely. If possible we should make all protocols independent of each other. Grandpa and transaction syncing need to checked if they can be independent, but from the top of my head I can not find a reason why they should not be able to be independent.

Then we should have reserved-nodes options per protocol via cli.

@dmitry-markin
Copy link
Contributor Author

Do you mean there is no need to track sync peers by i.e. grandpa? And grandpa can just source peers form kademlia/peerstore and connect to them?

@dmitry-markin
Copy link
Contributor Author

Wouldn't reserved-nodes per every protocol via cli be too complicated for operators?

@bkchr
Copy link
Member

bkchr commented Dec 27, 2024

Do you mean there is no need to track sync peers by i.e. grandpa? And grandpa can just source peers form kademlia/peerstore and connect to them?

Yes, I think so. I mean we only need to ensure that these peers are somewhere around the tip of the chain etc. Which may brings us back to just keep the list of peers of the sync protocol, because this is already ensuring that peers are valid etc. But clearly something too look into.

Wouldn't reserved-nodes per every protocol via cli be too complicated for operators?

I doubt that anyone right now is using this. Probably mainly useful for debugging stuff as we are doing it. But yeah, should be discussed. As I already said, right now not all protocols are following this list of reserved nodes, so it is a little bit "ridiculous" already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D2-substantial Can be fixed by an experienced coder with a working knowledge of the codebase. T0-node This PR/Issue is related to the topic “node”.
Projects
Status: No status
Development

No branches or pull requests

2 participants