PeerIdentify must not add a remote address if we don't belong to the same chain

[dmitry-markin]

We add known addresses of the peer to PeerStore (ex Peerset) as a result of PeerIdentify here: https://github.com/paritytech/substrate/blob/d38d176b844aab1338ce79eb71cd6df86c97d4a0/client/network/src/service.rs#L1416

We should do this only if we are on the same chain / have common protocols.

[chungquantin]

Hi is this issue is still in the plan? I plan to work on this.

[dmitry-markin]

Hi is this issue is still in the plan? I plan to work on this.

This would be much appreciated! To elaborate on the original issue, ideally we should not add a peer to PeerStore nor add it's addresses to Discovery behavior unless we have common protocols.

[dmitry-markin]

There might be some tricky bits, like we probably have some generic protocols in common with all chains.

[dmitry-markin]

Thinking about it a little more, may be it's enough to make use of identify protocol_version, i.e. /substrate/1.0 in https://github.com/paritytech/polkadot-sdk/blob/master/substrate/client/network/src/peer_info.rs#L124, and base it on the genesis hash.

And, also, we might have troubles when it comes to actual deployment because DHT tables of different chains are now mixed (this applies at least to parachains), and when we try to unlink them we might need to do it in a "gentle" way.

CC @lexnv

[chungquantin]

@dmitry-markin

There might be some tricky bits, like we probably have some generic protocols in common with all chains.

When you mention generic protocols in common with all chains, do you mean a set of defined common protocols or the common protocols found from a set of discovered peers. If it is the later, I wonder how would we identify if the protocol is common for all chains.

[dmitry-markin]

I mean just checking if we have some protocols in common is not a good indicator of whether we belong to the same chain, as we have some "generic" protocols not based on the genesis hash nor chain's ProtocolId.

For example, here is the list of protocols for rococo chain:

/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/block-announces/1
/rococo/block-announces/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/grandpa/1
/paritytech/grandpa/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/beefy/2
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/transactions/1
/rococo/transactions/1
/ipfs/ping/1.0.0
/ipfs/id/1.0.0
/ipfs/id/push/1.0.0
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/kad
/rococo/kad
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/state/2
/rococo/state/2
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/beefy/justifications/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/light/2
/rococo/light/2
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/req_collation/2
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/req_pov/1
/polkadot/req_pov/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/req_collation/1
/polkadot/req_collation/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/send_dispute/1
/polkadot/send_dispute/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/req_statement/1
/polkadot/req_statement/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/req_available_data/1
/polkadot/req_available_data/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/sync/2
/rococo/sync/2
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/sync/warp
/rococo/sync/warp
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/req_chunk/1
/polkadot/req_chunk/1
/6408de7737c59c238890533af25896a2c20608d8b380bb01029acb392781063e/req_attested_candidate/2"]

The following protocols will be present on all chains:

/paritytech/grandpa/1
/ipfs/ping/1.0.0
/ipfs/id/1.0.0
/ipfs/id/push/1.0.0

Grandpa legacy protocol not being based on ProtocolId (paritytech instead of rococo) is a bug, but even without it we have identify and ping protocols names being generic.

We could, in theory, blacklist/hardcode some protocols for the purpose of detecting whether we are on the same chain with a remote peer, but generally I think this is a bad idea because we'd need to track protocol names and maintain this list.

A better approach would be to use the Identify protocol name ("protocol_version" in Identify messages) to check the compatibility. We'd need to base Identify protocol_version on the genesis hash for this. Actually deploying this can be tricky for two reasons:

1. We'd need to update the Identify protocol name and make sure all the clients are updated, before we actually enforce filtering based on this name. This could take months.
2. Even when all ("99%") the nodes upgrade to the new Identify protocol name, we can have issues with DHT when suddenly it's split by protocol names. This applies to parachains only, as they have a common DHT protocol name /sup/kad and a shared DHT as a consequence. We need to address this anyway, and the phasing out of /sup/kad legacy DHT name should be handled gracefully to not break the DHT for parachains (we had issues with the node participation in DHT when we just tried to disable /sup/kad protocol name). Here are more details: Remove support for legacy ProtocolId-based protocol names #504 (comment).

So, I must admit the task is turning out to be more tricky than it appeared to be at the first glance.