Get rid of Peerset compatibility layer

[dmitry-markin]

Eliminate Peerset completely, giving handles to PeerStore & ProtocolController to the components that require them instead. Resolves #14170.

This PR also fixes a bug in Protocol with peer counting: now it made clear that self.peers contains only peers on the default (sync) protocol, and the entries are not removed when peers are disconnected on non-default protocols.

Added B1-note_worthy because Peerset is completely replaced by PeerStore & ProtocolController.

polkadot companion: paritytech/polkadot#7355
cumulus companion: paritytech/cumulus#2739

[alindima]

@dmitry-markin I integrated this in polkadot for testing purposes. Here's the branch I used, since I needed to rebase it: https://github.com/paritytech/substrate/tree/alindima-get-rid-of-peerset-stub

I tested it in Versi and ran into a weird issue, where the node would print out many errors like:
Protocol controllers receiver stream has returned None

and then it looks like the node restarted. Here are the full logs: https://grafana.parity-mgmt.parity.io/goto/2NEzJ7j4g?orgId=1.

Are these errors expected under normal usage?

[dmitry-markin]

@dmitry-markin I integrated this in polkadot for testing purposes. Here's the branch I used, since I needed to rebase it: https://github.com/paritytech/substrate/tree/alindima-get-rid-of-peerset-stub

I tested it in Versi and ran into a weird issue, where the node would print out many errors like: Protocol controllers receiver stream has returned None

and then it looks like the node restarted. Here are the full logs: https://grafana.parity-mgmt.parity.io/goto/2NEzJ7j4g?orgId=1.

Are these errors expected under normal usage?

Thanks for testing this! There are currently issues with the slot allocation and connection management mechanism that need to be addressed first, so this PR is blocked. I'll look into the errors reported once I get back to it.

[dmitry-markin]

Protocol controllers receiver stream has returned None

@alindima Is it possible that the node was externally restarted? These errors might happen if the node components were already shutting down, but I found no indication in the logs for why would the node shut down.

[alindima]

@alindima Is it possible that the node was externally restarted? These errors might happen if the node components were already shutting down, but I found no indication in the logs for why would the node shut down.

This could be it, I started noticing random restarts in other versi nodes as well. I'll ask around to make sure that others noticed this as well

[dmitry-markin]

@alindima Could you try burning in again? Here is the burn-in PR for polkadot debug docker image: paritytech/polkadot#7533.

[dmitry-markin]

I've converted the PR into draft, so we remember to fix the flaky test before merging. The test in question is syncs_header_only_forks and it gets stuck once in a while on CI. As discovered by @altonen, it gets stuck here. For some reason it doesn't get stuck if the test runtime is converted to single-threaded.

I've created an issue paritytech/polkadot-sdk#497 to fix this later.

[dmitry-markin]

Versi burn-in showed no impact on block height, peer count, CPU usage. So, IMO, this PR is ready for merging. CC @altonen

[dmitry-markin]

@altonen can we merge this?

[altonen]

@altonen can we merge this?

yes we can merge it

[dmitry-markin]

bot merge

[paritytech-processbot]

Error: Github API says paritytech/polkadot#7355 is not mergeable

[dmitry-markin]

bot merge

[paritytech-processbot]

Error: Github API says paritytech/cumulus#2739 is not mergeable

[dmitry-markin]

bot merge