Make polkadot behave correctly if node changes peer-id at restart

[alexggh]

During the investigation for #3314 we discovered there is a number of validators that change their PeerId at every restart, that seems not work properly right now.

Work-arrounds

1. Don't change PeerId at every restart, the peer-id is saved on disk next to the databse in the network folder, so you can simply reuse that by always providing --base-path argument and making sure the network folder is present where you restart the node.
2. If you restarted already, key rotations would fix the issue.

[WIP] Things that need fixing

In general all the assumptions where we think we have just one PeerID per AuthorithyId are breaking this on a quick scan by me and @bkchr:

With kusama size nodes won't be able to connect to nodes if they aren't in the reserved set.

polkadot-sdk/polkadot/node/network/protocol/src/peer_set.rs

Lines 95 to 96 in 809d466

	in_peers: super::MIN_GOSSIP_PEERS as u32 / 2 - 1,
	out_peers: super::MIN_GOSSIP_PEERS as u32 / 2 - 1,

Some places where the assumption that just one peerID per authority exist.

polkadot-sdk/polkadot/node/network/gossip-support/src/lib.rs

Line 408 in 809d466

.find_map(|addr| parse_addr(addr).ok().map(|(p, _)| p));

polkadot-sdk/polkadot/node/network/statement-distribution/src/v2/mod.rs

Line 451 in a756baf

"Ignoring new peer with duplicate authority ID as a bearer of that identity"

Another thing that I observed that is probably off while testing on versi is that ConnectToResolvedValidators gets called sometime with the new address and sometimes with old-address.

[bkchr]

The first link is not really a problem. Just more the explanation why these people could not connect to all the other validators.

polkadot-sdk/polkadot/node/network/gossip-support/src/lib.rs

Lines 327 to 352 in 809d466

	async fn issue_connection_request<Sender>(
	&mut self,
	sender: &mut Sender,
	authorities: Vec<AuthorityDiscoveryId>,
	) where
	Sender: overseer::GossipSupportSenderTrait,
	{
	let num = authorities.len();
	let mut validator_addrs = Vec::with_capacity(authorities.len());
	let mut failures = 0;
	let mut resolved = HashMap::with_capacity(authorities.len());
	for authority in authorities {
	if let Some(addrs) =
	self.authority_discovery.get_addresses_by_authority_id(authority.clone()).await
	{
	validator_addrs.push(addrs.clone());
	resolved.insert(authority, addrs);
	} else {
	failures += 1;
	gum::debug!(
	target: LOG_TARGET,
	"Couldn't resolve addresses of authority: {:?}",
	authority
	);
	}
	}

This function needs to be called multiple times per session. Currently we don't call this once and if a validator address was not known at this point, we will never resolve it in this session.

[alexggh]

The first link is not really a problem

Messed up the links, fixed it in the description.

[alexggh]

Looked a bit more closer at Kademlia DHT and the biggest problem for this use-case seem to be is that both records will continue living in the DHT on different nodes, so on any given node any of the following might be true:

1. When querying the DHT a node discovers only the old record.
2. When querying the DHT a node discovers only the new record.
3. When querying the DHT a node discovers both the new record and the old record, but because they are sequentially advertised to authority-discovery only the last one will be saved. We can probably mitigate this, by modifying authorithy-discovery to save both of them and expire the old-record at a later data.

Both 1) & 3) will happen until the old record TTL is met which currently is 36 hours, after that the new record will be the only one in the network.

Why does this happen ?

The way Kademlia DHT works is that it uses the PeerId as a key for determining on which nodes records need to be replicated the receiving nodes will do replication as well every 60 minutes. The replication factor is 20.
So, regularly both the old records and new records will continue living in the network on different nodes until the old one expires.

What can we do ?

At least as far as I can tell this behaviour is entrenched in the Kademlia DHT protocol, we can probably reduce the window the old entry lives by playing with record_ttl(36h), record_replication_interval(1), record_publication_interval(24h), but there will still be a window where this happens.

So, I'm inclined to think changing the network key(PeerID) while a node is in the active set should not be considered a safe operation, to perform, hence what I'm proposing is:

• Modify the behaviour of polkadot executable to not auto-generated the network key(PeerID) if none is provided and exit with error (Kudos to @s0me0ne-unkn0wn for giving me the idea)
• Add an explicit flag for auto-generating the network key at startup with proper documentation that this is not a safe operation to do while the node is in the active set and what will happen.

@bkchr @dmitry-markin @eskimor Thoughts ?

[bkchr]

Here are my notes from last week that I wanted to put into an issue actually:

I think one problem exists in authority discovery:

polkadot-sdk/substrate/client/authority-discovery/src/worker/addr_cache.rs

Lines 77 to 86 in a6713c5

	let old_addresses = self.authority_id_to_addresses.insert(authority_id.clone(), addresses);
	let old_peer_ids = addresses_to_peer_ids(&old_addresses.unwrap_or_default());
	// Add the new peer ids
	peer_ids.difference(&old_peer_ids).for_each(|new_peer_id| {
	self.peer_id_to_authority_ids
	.entry(*new_peer_id)
	.or_default()
	.insert(authority_id.clone());
	});

and

polkadot-sdk/substrate/client/authority-discovery/src/worker.rs

Lines 571 to 572 in a6713c5

	.flatten()
	.take(MAX_ADDRESSES_PER_AUTHORITY)

.
If the validator exists with multiple peerids in the DHT (because of the restart), we maybe get multiple DHT events with different peerids. We should keep different peerids around. Maybe store when we received them the last time. We also should not take the maximum number of addresses solely based on the addresses, we should have addresses per peer_id and then have a max number of peer_ids and addresses.

polkadot-sdk/substrate/client/network/src/discovery.rs

Lines 784 to 790 in a6713c5

	// Let's directly finish the query, as we are only interested in a
	// quorum of 1.
	if let Some(kad) = self.kademlia.as_mut() {
	if let Some(mut query) = kad.query_mut(&id) {
	query.finish();
	}
	}

Another issue is this properly. Here we should not end directly with quorum of 1. Instead we should probably fetch all the values.

This assumes the same as you are writing. However, I would like to try to optimize the current implementation given the points above. We can also go further and extend the record with a timestamp to find out which one is newer. We also don't verify the data in the DHT storage. The verification is only done in the authority discovery. This means that the DHT may could still contain invalid values.

• Modify the behaviour of polkadot executable to not auto-generated the network key(PeerID) if none is provided and exit with error

Could be done as well, but should only be done for validators. Generally we probably need to improve docs around this stuff. There also exists generate-node-key to generate a key. Maybe we should just require that --node-key is passed when --validator is set.

[alexggh]

Another issue is this properly. Here we should not end directly with quorum of 1. Instead we should probably fetch all the values.

Ok, I see what you mean, so basically if we let the query continue we could probably end up discovering both more often than not and have them both stored in authorithy-discovery. Let me check how that behaves.

[alexggh]

Posted a draft PR with all the plumbing that would need to happen to make this a bit better than the way it currently is: #3786, let me know what you guys think, don't be nitpicky it is in a very rough form :D.

[dmitry-markin]

So, I'm inclined to think changing the network key(PeerID) while a node is in the active set should not be considered a safe operation, to perform, hence what I'm proposing is:

I don't understand why people regenerate the PeerIDs of validators in the first place. Obviously, a validator should have the actual database, so the contents of --base-path should be preserved across restarts. Why they remove the network key file then?

Generally I'm for not-regenerating PeerIDs of the validators without a sound reason (is there one?)

Modify the behaviour of polkadot executable to not auto-generated the network key(PeerID) if none is provided and exit with error (Kudos to @s0me0ne-unkn0wn for giving me the idea)

May be make this default for validators only? This would change the interface and force operators to update their procedures, but we can print a nice error message if somebody starts a validator without network key file present.

Add an explicit flag for auto-generating the network key at startup with proper documentation that this is not a safe operation to do while the node is in the active set and what will happen.

IMO, this should be an explicit cli command like generate-node-key that has to be invoked separately, otherwise the operators would just plug this --auto-generate-node-key option and keep everything as before.

[lexnv]

Likewise, we could fix this in the CLI and require validators to use --node-key with some improved documentation.

We can also go further and extend the record with a timestamp to find out which one is newer. We also don't verify the data in the DHT storage.

I think this sounds like a reasonable solution to me. We'd need to extend the DHT::ValueFound event with the kademlia record expiry time:

polkadot-sdk/substrate/client/network/src/discovery.rs

Lines 475 to 478 in e88d1cb

	/// The DHT yielded results for the record request.
	///
	/// Returning the result grouped in (key, value) pairs as well as the request duration.
	ValueFound(Vec<(RecordKey, Vec<u8>)>, Duration),

Then, in the authority-discovery, we could store only the most recent records, since we know that older records might contain stale addresses. While at it, we could also have a look at the MAX_ADDRESSES_PER_AUTHORITY and adjust it as Basti suggested 🙏

[alexggh]

I think this sounds like a reasonable solution to me. We'd need to extend the DHT::ValueFound event with the kademlia record expiry time:

I've been down that road, it won't work because expiry is actually a function of the distance from the key to the node answering the request: https://github.com/libp2p/rust-libp2p/blob/master/protocols/kad/src/behaviour.rs#L1816. Expiry doesn't tell how old the record is because if you get it from nodes farther from the key they would have a close expiry they should not store the record for too much time.

[alexggh]

I don't understand why people regenerate the PeerIDs of validators in the first place. Obviously, a validator should have the actual database, so the contents of --base-path should be preserved across restarts. Why they remove the network key file then?

In this case a large validator set >50, were mount individually the keystore and db in their pod, so they missed persisting the network:

.local/share/polkadot/chains/ksmcc3/db
.local/share/polkadot/chains/ksmcc3/keystore

[alexggh]

IMO, this should be an explicit cli command like generate-node-key that has to be invoked separately, otherwise the operators would just plug this --auto-generate-node-key option and keep everything as before.

I was thinking to name it more like --i-know-i-m-getting-0-rewards-but-auto-generate-node-key :D

[alexggh]

PR to not allow auto generate of network key unless it is explicitly required: #3852.

[alexggh]

Problem mitigated with: 6720279