Approve multiple candidates with a single signature

[alexggh]

Initial implementation for the plan discussed here: paritytech/polkadot-sdk#701 on top of: #6782

Overall idea

When approval-voting checks a candidate and is ready to advertise the approval, defer it in a per-relay chain block until we either have MAX_APPROVAL_COALESCE_COUNT candidates to sign or a candidate has stayed MAX_APPROVALS_COALESCE_TICKS in the queue, in both cases we sign what candidates we have available.

This should allow us to reduce the number of approvals messages we have to create/send/verify. The parameters are configurable, so we should find some values that balance:

• Security of the network: Delaying broadcasting of an approval shouldn't but the finality at risk and to make sure that never happens we won't delay sending a vote if we are past 2/3 from the no-show time.
• Scalability of the network: MAX_APPROVAL_COALESCE_COUNT = 1 & MAX_APPROVALS_COALESCE_TICKS =0, is what we have now and we know from the measurements we did on versi, it bottlenecks approval-distribution/approval-voting when increase significantly the number of validators and parachains
• Block storage: In case of disputes we have to import this votes on chain and that increase the necessary storage with MAX_APPROVAL_COALESCE_COUNT * CandidateHash per vote. Given that disputes are not the normal way of the network functioning and we will limit MAX_APPROVAL_COALESCE_COUNT in the single digits numbers, this should be good enough. Alternatively, we could try to create a better way to store this on-chain through indirection, if that's needed.

TODO:

• Get feedback, that this is moving in the right direction. @ordian @sandreim @eskimor @burdges, let me know what you think.
• More and more testing.
• Test in versi.
• Make MAX_APPROVAL_COALESCE_COUNT & MAX_APPROVAL_COALESCE_WAIT_MILLIS a parachain host configuration.
• Make sure the backwards compatibility works correctly
• Make sure this direction is compatible with other streams of work: Slash approval voters on approving invalid blocks - dynamically polkadot-sdk#635 & Time Disputes polkadot-sdk#742

[rphmeier]

This should ensure that candidate_hash is included in candidate_hashes.

We could also save a bit of space by having the ValidDisputeStatementKind::ApprovalCheckingV2 include only the other candidate hashes and the index in the vec where the candidate hash in question should be included. Maybe overcomplicated, but payload_data returns a Vec<u8> unconditionally which is a problem. i.e. we can't represent a failure path with the current interface.

As it stands, we'd allow any set of candidate hashes to be treated as a valid dispute vote for any candidate.

Pseudocode

ValidDisputeStatementKind::ApprovalCheckingV2(candidate_hashes, index) => {
    let mut final_candidates = candidate_hashes.clone();
    let index = min(candidate_hashes.len(), index);
    final_candidates.insert(index, candidate_hash);
    signing_payload(final_candidates, session);
}

[alexggh]

Agree, this interface wasn't yet cleaned, it was just in a state that worked on the happy-path, I was thinking about making candidate_hash an Option<CandidateHash> and when is Some we just added to the payload.

What do you think about that ?

[alexggh]

Opted instead for throwing an error if the provided candidate_hash is not in the vec of candidate_hashes, I think that keeps the API easy to use and prevents accidental misusage.

See here: 2271c71#diff-e876407804ef19c762a4fe91db962a8eea5185a3aee42ed76b60fb8b549beb03R1288, let me know what you think.

[rphmeier]

It seems fine, so long as we throw an error in dispute handling as normally done.

I do feel this part of the code could be much better optimized.

[rphmeier]

IMO this shouldn't be in the runtime configuration but is rather a strategy set in the node-side.

The runtime should be concerned with validity, not necessarily how the nodes get there in the fastest possible way.

[alexggh]

Are you referring just to the max_approval_coalesce_wait_millis part or are you think that both configurations parameters shouldn't be there ?

[rphmeier]

The count is a hard parameter defining a boundary between validity and invalidity, which could reasonably live in the runtime or else just be an implementation constant. Though it should probably be very high if defined in the runtime.

The wait time is just tactical from a validator and more advanced strategies would be precluded by hardcoding here.

[alexggh]

Ok, I see, one reason I had for keeping at least having max_approval_coalesce_count in the runtime is that it gives us a good knob to enable v2 approvals on all validator, instead of having to basically do 2 separate releases of node to enable this logic.

But I guess just for that use-case I could go with the option were I make both max_approval_coalesce_count and max_approval_coalesce_wait_ticks implementation constants and just have a param in the runtime enable_approvals_v2. Would that make more sense ?

[rphmeier]

I haven't reviewed the changeset in-depth yet, but typically there are two approaches to enabling new features:

1. Do a first node upgrade that adds support for the change, followed by a second after giving enough time for nodes to upgrade to the first that activates the behavior.
2. Do a single node upgrade that adds support for the change, which is activated by a runtime switch such as a version increase.

(2) is usually better as it goes through chain governance, which it sounds like you are going for - great. (see the #5022 changeset for an example, where we use the async_backing_params runtime API to activate the feature).

I could go with the option were I make both max_approval_coalesce_count and max_approval_coalesce_wait_ticks implementation constants and just have a param in the runtime enable_approvals_v2. Would that make more sense

Yep, makes sense to me, as long as the following statement is something you agree with: we don't mind coalesced bundles being arbitrarily large as long as they are valid, and don't expect that valid bundles will be of a size too large to handle within the disputes pallet. Setting a hard maximum seems valuable to protect per-block disputes load from causing overweight blocks.

[alexggh]

Setting a hard maximum seems valuable to protect per-block disputes load from causing overweight blocks.

Yes, we definitely need a maximum here to protect per-block disputes creating overweight blocks.

[sandreim]

This should inc_by(candidate_indices.count_ones() )

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3432194

[alindima]

nit:

Suggested change

	RequestResult::ApprovalVotingParams(relay_parent, params) =>
	ApprovalVotingParams(relay_parent, params) =>

[alindima]

can't this be cached per-session? to have fewer entries in this map.
maybe even add it to the session_info_provider