paras_inherent: reject only candidates with concluded disputes

[ordian]

Triggers Error::<T>::CandidateConcludedInvalid (formerly Error::<T>::CandidateCouldBeInvalid) only if the dispute already has an AGAINST_SUPERMAJORITY. In other cases the dispute (and reverting) can still be handled later when needed.

[eskimor]

I don't understand the point of this check in the first place. Candidates can only be disputed once included, so how can a candidate be disputed before it is even backed? I mean obviously because of a fake dispute, but in a legit way?

[eskimor]

Answering my own question - in theory via forks. Candidate is about to be backed on two forks, one is faster - where it gets backed and included and disputed, while the backing did not even happen on the other fork - especially with chain reversions/reorgs and such that could actually happen. The check is still not needed, I think, because if it gets backed, it will get included, then checked by approval voters and disputed again - correct?

Just testing my understanding here.... It could help with avoiding potential endless loops, any other motivation?

[eskimor]

Avoiding endless loops is a good one, just curious whether I am missing something else.

[eskimor]

Ok, I should think more before I type: I actually think this can not happen at all. The disputes are recorded in chain state. So for a candidate to be disputed and considered for being backed it must have a relay parent that either is the block that contained the dispute or some later block. Even in the case of a chain reversion, the presence of the dispute, rules out the existence of such a candidate. As the dispute must have happened in one of the parents (even if transplanted), but if that's the case then the relay parent of the candidate cannot be the current head, as the dispute can not have happened for a candidate with a relay parent that has not existed yet - that's just not possible.

Candidate x with relay_parent: A

Relay Parent A: |Relay parent containing dispute for candidate x| 

How?!

[eskimor]

Talking to myself again: It could happen in the future, with contextual execution, then the relay parent of a candidate can be older. 💡 Which makes me realize, we should be really careful with contextual execution...

[rphmeier]

I don't understand the point of this check in the first place. Candidates can only be disputed once included, so how can a candidate be disputed before it is even backed? I mean obviously because of a fake dispute, but in a legit way?

It's the entire point of remote disputes. Typical flow:

B1[C1 backed] <- B2[C1 included] <- B3[C1 disputed]
      |
      | (B2 reverted)
      |
       \>  B2'[C1 disputed and dropped]

An adversary with a few block authors could withhold a small chain and wait for the reversion to successfully back the bad block unless this check is respected:

B1[C1 backed] <- B2[C1 included] <- B3[C1 disputed]
      |
      | (B2 reverted)
      |
      |
B1'[withheld by adversary until dispute]  <- B2''[C1 disputed] <- B3''[C1 backed]

This would land B3'' in an impossible state where C1 has already been disputed and cannot be disputed again.

[eskimor]

Thanks @rphmeier for those really useful illustrations! Ok, I understand that we need to drop an already backed candidate in case we find it disputed, but isn't the above code just dropping candidates that enter backing state in this very block? Like, in my understanding the enter function gets called with inherents of the currently imported block.

If that is the case then B3''[C1 backed] with the meaning that C1 gets backed in this block cannot exist as C1 would need to have B2'' as relay parent which already contained the dispute of C1, therefore C1 cannot have B2'' as relay parent. In fact the relay parent of C1 must be B0 (common ancestor of both B1' and B1). (B1'[witheld by adversary until dispute] must had C1 already backed, the adversary cannot just transplant a candidate to a later block as then the relay parent would not match)

My reasoning is based on the assumption that a candidate must be based on the relay parent right before the block where it is backed, which reflects my current understanding of things 🤔 .

[eskimor]

Which is indeed ensured:

polkadot/runtime/parachains/src/inclusion.rs

Line 454 in 7bf12d6

candidate.descriptor().relay_parent == parent_hash,

[rphmeier]

@eskimor Good point on the relay parent. The current code would still be affected if B1'[C1 disputed, C1 backed] if we assume that B1 and B1' have the same parent B0

[eskimor]

Uuh, because disputes are imported before candidates - so disputes of the current block will be already available in the state, regardless of being added in the same block. 💡 Got it, thanks!