Enforce validator disabling in the runtime

[tdimitrov]

Desired outcome

With the disabling strategy in place (#784 (comment)) the actual disabling needs to be enforced by the runtime. This means that the runtime should ignore the following data from disabled validators:

• Availability bitfields -

  polkadot-sdk/polkadot/primitives/src/v5/mod.rs

  Line 1426 in 1e2a2f0

  pub bitfields: UncheckedSignedAvailabilityBitfields,

• Validity votes:

  polkadot-sdk/polkadot/primitives/src/v5/mod.rs

  Line 703 in 1e2a2f0

  pub validity_votes: Vec<ValidityAttestation>,

• Dispute statements:

  polkadot-sdk/polkadot/primitives/src/v5/mod.rs

  Line 1364 in 1e2a2f0

  pub statements: Vec<(DisputeStatement, ValidatorIndex, ValidatorSignature)>,

When a validator is disabled

Each candidate has got a relay parent. If a validator is disabled at this relay parent, we consider the validator disabled in the context of the candidate.
So if a candidate is included at height A and B and validator X is disabled at height A but re-enabled at height B we will ignore its votes in the first case and count them in the latter.

How data filtering will work

All votes/statements will be filtered in

polkadot-sdk/polkadot/runtime/parachains/src/paras_inherent/mod.rs

Line 326 in 1e2a2f0

fn process_inherent_data(

Related to #784 (comment)

[tdimitrov]

@eskimor @ordian @Overkillus this is the way I envision the disabling in the runtime. Please share your feedback.

[eskimor]

Yes, but make sure to not include dead weight in the block. This means conceptually on block production you have to filter on block import we have to return an error if data from disabled validators are present.

To avoid divergence of code I would suggest to do the same filtering both on import and creation, but check the length in import before and after - if they differ: return an error.

This way it should be possible to maintain almost identical code paths (which prevents bugs from them getting out of sync). They would look like this (Rust inspired pseude code):

let len_pre_filter = statements.len();
filter_disabled(&mut statements);
let len_after_filter = statements.len();
if importing && len_pre_filter != len_after_filter {
  return Error(...)
}

[eskimor]

For using the relay parent of the candidate as basis for disabled state. I know we said to do this to have consistency on the node side. In the runtime this seems annoying though .. .would make more sense to use the current block state here. Might make sense to discuss this one more time.

[eskimor]

Actually did we even say this? What we could do is the following: A node is considered enabled on the node side, if we have at least one leaf in our view where it is enabled. Meaning, if there is at least one leaf where the validator is enabled we will not prematurely drop statements for example.

Different story in runtime and provisioner: Here we can (and should) drop statements based on the leaf we are currently operating on.

Potential problem: We don't have consensus here: Depending on our view we might filter votes, while other nodes do not. If then later on we learn about a better fork, which does not have the validator disabled then we dropped votes, which we should actually have kept.

Possible solution: No premature filtering at all: Subsystems accept votes from disabled notes, then for subsystems which operate with peer views, things stay easy: We only forward messages to nodes that have a leaf in their view where the validator is enabled.

For disputes, this does not work .... so I see why we decided on using the relay parent. Thoughts from your end?

[eskimor]

What could work: Indeed no premature/early filtering, but the above and we don't participate in disputes if all negative votes are all from disabled nodes as in our currently active leaves. On leaf updates which cause disabled status by above definition to change to enabled again, we would need to make up for that missed participation.

This sounds like it could cause consensus issues, but in fact it should be fine: Once a single not disabled node participated, disabled status does not matter anymore. We don't need perfect consensus here.

[tdimitrov]

Yes, but make sure to not include dead weight in the block.

I assume you talk about this filtering. If yes - I don't understand your idea. Why not filter the data just before the weights are calculated and ignore the disabled votes altogether? Maybe it is related to the two code paths of process_inherent_data.

For using the relay parent of the candidate as basis for disabled state.

Afair we didn't agree on anything. I'm proposing this because:

1. The relay parent is finalized and all the instances of the runtime will have the same view in terms of which validators are disabled and which are not. This is not that important.
2. We will avoid the situation where a candidate gets disabled in the middle of a fork and we need to reevaluate our decisions.

I am more worried about 2. What I mean:

BLOCK N -> Fork N+1 -> Fork N+2 -> Fork N+3 ...
(finalzied)

Let's assume we make a decision that in N+1 a dispute has concluded and in N+2 some validartor (which have alreadyu voted) gets disabled. We need to reevaluate the decision about the dispute? Or do something else?

Not considering if a validator is disabled in the current leaf has got its drawbacks - potential offenders (or buggy validators) can do wild things in the fork before they get disabled. In the general case it is just one block but in a dispute storm this window might get quite big.

[tdimitrov]

Why not filter the data just before the weights are calculated and ignore the disabled votes altogether?

Yeah, this won't work.

EDIT: I wasn't aware that process_inherent_data was used to prepare inherent data and to validate it on block executioin. Now your idea makes sense to me.

[Overkillus]

Based on the new updated validator disabling strategy Availability Bitfields and Dispute Statements are not touched within the runtime so #1863 fully closes the issue.

Just need to merge into feature branch and test holistically.