CandidateDescriptor: disable collator signature and collator id usage

[sandreim]

Collator id and collator signature do not serve any useful purpose. Removing the signature check from runtime but keeping the checks in the node until the runtime is upgraded.

TODO:

• PRDoc
• Add node feature for core index commitment enablement

[burdges]

Aren't they in the block header anyways?

[sandreim]

Aren't they in the block header anyways?

Yeah AFAIK, but the relay chain doesn't make any assumptions about what the header contains. In any case we just don't need them

[alexggh]

Looks alright, I have a small concern, but other than that it should be fine.

[alexggh]

Correlated with this this:

polkadot-sdk/polkadot/node/core/candidate-validation/src/lib.rs

Line 1218 in ac98bc3

if let Err(()) = candidate.check_collator_signature() {

, isn't it possible we actually stall finality if backers collude to include a candidate with the wrong signature ?

[sandreim]

With this change, the runtime will no longer care about what is being put in collator signature.

[alexggh]

Right, but wouldn't that mean that the block author will accept blocks with invalid signature, that will then fail the candidate-validation because we are still checking the signature there ?

Anyways, I thought a bit more about this and it is a non-issue because, that will just trigger a dispute which will punish the malicious backers, so this change does not introduce any attack-vector.

[sandreim]

This is actually a good point. It is a bit subtle but yes, validators strictly need to still check the signatures even if the runtime doesn't. The problem I see with this is that malicious collators can put garbage in the signature, candidates get backed by validators upgraded to not check the signature in the future. During approval checking, these candidates will be disputed by older validators that check the signature.

Then who gets punished depends on which kind of super majority we have. This sounds like a new attack vector to me.

[sandreim]

We'll likely need another node feature 🤦🏼 just to give people more time to upgrade, but at least 2/3, then we need to communicate the deadline when it becomes dangerous (due to prev comment) to not have upgraded yet.

[sandreim]

One way to solve this is to still keep the signature checks only for node but only check them if we don't have a core index (signaled by Ump commitment and zero padding in descriptor).

[eskimor]

One way to solve this is to still keep the signature checks only for node but only check them if we don't have a core index (signaled by Ump commitment and zero padding in descriptor).

I had a similar idea, but I don't see how this fixes anything. You still have the problem of different validators arriving at different validity.

The node feature is actually the way to go. We still need to wait for enough validators to upgrade, but then we have perfect consensus on what strategy to use in an instant. Any too old validators might get slashed, but we actually have requirements on validators to keep their node current, so that is fine.

So basic plan is to implement everything at once:

1. Ignore signature
2. If signature zero, re-interpret data.
3. Enable all of that with a node feature.

Collators will not be affected. Collators can still provide a signature, it will simply be ignored. If they set it to 0, we re-interpret fields.

[sandreim]

Agreed. I'll add the node feature in this PR. It will signal the nodes to not check the signature if there is a core_index.

[eskimor]

Once the feature is enabled no signature needs to be checked at all anymore. But we should also directly start re-interpreting fields in case the signature is 0. Otherwise we would need two node features: One to disable checking and then another one to check the core index.

[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/6803290