search

paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.network/
1.85k stars 670 forks source link

Reconsider if we should connect/accept reserved nodes if they are banned by the reputation system #3368

Open dmitry-markin opened 8 months ago

dmitry-markin commented 8 months ago

Right now, we don't disconnect reserved nodes when they are banned by the reputation system: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/network/src/protocol_controller.rs#L604-L611

On the other hand, we don't connect to a reserved node if it's banned and happens to be disconnected: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/network/src/protocol_controller.rs#L781

Also, we don't accept connections from banned reserved nodes: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/network/src/protocol_controller.rs#L668-L669

Reconsider this behavior and, may be, make it consistent.

One thing to keep in mind is that there are two types of reserved nodes:

  1. Reserved nodes of the "default" sync protocol, manually set by the node operator.
  2. Reserved nodes of the protocols that track sync protocol, used just to track sync peer set.

While being logical to always keep connected to the nodes of type 1, it's not that clear with the nodes of type 2. May be, the entire approach of tracking the sync peer set via reserved nodes should be reworked.

altonen commented 8 months ago

Since GRANDPA and transactions follow the sync peerset using SyncEventStream, if a peer was banned over GRANDPA and stayed connected as a reserved peer, it would still be removed from the sync peerset and once the peer is registered as disconnected in SyncingEngine, it would also get removed from the reserved sets of GRANDPA and transctions via SyncEvent::PeerDisconnected

dmitry-markin commented 8 months ago

Since GRANDPA and transactions follow the sync peerset using SyncEventStream, if a peer was banned over GRANDPA and stayed connected as a reserved peer, it would still be removed from the sync peerset and once the peer is registered as disconnected in SyncingEngine, it would also get removed from the reserved sets of GRANDPA and transctions via SyncEvent::PeerDisconnected

Good point. This makes the existing approach somewhat reasonable: in the end, we only disconnect peers that were not marked as reserved by the node operator.

It looks like it's safe to accept/connect to banned reserved nodes, as this will only affect the nodes manually set as reserved.