Do not run unneeded subsystems on collator and its alongside node

[s0me0ne-unkn0wn]

Currently, collators and their alongside nodes spin up a full-scale overseer running a bunch of subsystems that are not needed if the node is not a validator. That was considered to be harmless; however, we've got problems with unused subsystems getting stalled for a reason not currently known, resulting in the overseer exiting and bringing down the whole node.

This PR aims to only run needed subsystems on such nodes, replacing the rest with DummySubsystem.

It also enables collator-optimized availability recovery subsystem implementation.

Partially solves #1730.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5031914

[ordian]

I quite like this! At the cost of some minor code duplication, we get rid of some hacky conditional logic and gain more flexibility in instantiating different type of overseer for collators.

Noticed a couple of things that can be cleaned up further, but other than that LGTM.

[skunert]

Nice! Do you see anything speaking against reusing this also for the minimal collator node? In the end we have the nearly identical overseer here again:

polkadot-sdk/cumulus/client/relay-chain-minimal-node/src/collator_overseer.rs

Line 85 in 604b2ee

fn build_overseer(

[s0me0ne-unkn0wn]

Nice! Do you see anything speaking against reusing this also for the minimal collator node?

Yeah, that's a good point, and that's what I tried first. It seemed to be quite logical but came out as usual. The current "full scale" overseer is implemented through the OverseerBuilder which is generic over RuntimeClient, and over DefaultSubsystemClient<RuntimeClient> as well. They are two different levels of API implementation and work fine like that. But in the minimal node, the overseer is generic over BlockChainRpcClient, which provides both levels. So to generalize that code, we need to seriously rework BlockChainRpcClient (make it implement ProvideRuntimeApi for parachain host API, and thus we'd be able to stick it into DefaultSubsystemClient). After evaluating the amount of work to achieve that, I decided to postpone it for now. Might be a follow-up, though.

[ordian]

looks like cumulus pov recovery test is failing, @skunert do you know why wouldn't it work with minimal overseer?

[s0me0ne-unkn0wn]

@ordian looks like I forgot to include something required 🤔

2024-01-27 00:00:56.387 DEBUG tokio-runtime-worker parachain::availability-recovery: [pretty-bee-8350] Requesting availability chunks for a candidate common_params.candidate_hash=0xd9a47aaa51662f5b56f371f2d1616950d44bd8ec76d45e8e2b31c648ebd04ea8 desired_requests_count=5 error_count=136315 total_received=136315 threshold=5 already_requesting_count=0
2024-01-27 00:00:56.387 TRACE tokio-runtime-worker parachain::availability-recovery: [pretty-bee-8350] Failure requesting chunk candidate_hash=0xd9a47aaa51662f5b56f371f2d1616950d44bd8ec76d45e8e2b31c648ebd04ea8 err=NetworkError(UnknownProtocol) validator_index=ValidatorIndex(7) traceID=289296499718077548945207771063688194384

[s0me0ne-unkn0wn]

Fixed it

[ordian]

Nice, thanks!

[skunert]

Yeah, that's a good point, and that's what I tried first. It seemed to be quite logical but came out as usual. The current "full scale" overseer is implemented through the OverseerBuilder which is generic over RuntimeClient, and over DefaultSubsystemClient as well. They are two different levels of API implementation and work fine like that.

Hmm I have not taken a deep dive on that code, but on first sight it looks like DefaultSubsystemClient only implements the single trait RuntimeApiSubsystemClient. So why does the overseer building codepath even need to know about it. Can we not modify that code to just take some type that implements RuntimeApiSubsystemClient (and iirc AuxStore and ChainApiBackend). This is satisfied by both BlockChainRpcClient and DefaultSubsystemClient.

But anyway, I don't insist that this needs to be investigated right now. Code looks good 👍

[s0me0ne-unkn0wn]

@skunert makes sense! I'll try to follow up on that.