paras: count upgrade delay from inclusion

[slumber]

Resolves #4601

[ordian]

Okay, so after this issue you will have the full validation_upgrade_delay instead of validation_upgrade_delay - 10 or 20. Sounds like we spent more time discussing here than the issue is worthwhile :D (sorry) But this also doesn't sound like a big problem or solves that much (as we theoretically can still run into this problem).

Yeah exactly! It's not a big issue.

This seems like a partial solution to a very contrived scenario, so not sure how helpful this change is.

I also have trouble following the example #4601 (comment)

Because the candidate with relay-parent 1939 is invalid nodes will dispute it, however, they won't be able since the new code is not available.

Wouldn't nodes disputing candidate with relay parent 1939 fetch PVF from that block or some recent block and not inclusion block?

polkadot/node/core/dispute-coordinator/src/participation/mod.rs

Line 271 in 68b7db2

recent_head,

[slumber]

Wouldn't nodes disputing candidate with relay parent 1939 fetch PVF from that block or some recent block and not inclusion block?

The problem is it won't be available. The code gets stored on candidate's inclusion, however the delay for its enactment is counted from candidate's relay parent. So in total we're off by chain_availability_period + (with async backing) allowed_ancestry_len blocks. In numbers, that would mean that we're off by approx 10 + 5 blocks + another 1-2 for prechecking.

This seems like a partial solution to a very contrived scenario, so not sure how helpful this change is.

It is partial in a sense that validation_upgrade_delay is not deterministic value, or rather, it doesn't restrict chain from reverting past this number, but if we want to rely on it (i.e. assuming it is), we have to count from inclusion, otherwise (from the above paragraph) it's broken.

[rphmeier]

As mentioned in the original issue, this is strictly more correct now that code upgrades are handled with on-chain signals as opposed to coordinated block numbers. The GoAheadSignal infrastructure has been in place for over a year now, so all chains definitely make use of it.

[rphmeier]

Nit: I really don't like the term "inclusion parent", since the 'parent' part isn't important for this case. included_at or included_in makes more sense.

[slumber]

bot merge

[paritytech-processbot]

Error: Statuses failed for 18f8932

[slumber]

bot merge

[paritytech-processbot]

Waiting for commit status.

[paritytech-processbot]

Merge cancelled due to error. Error: Statuses failed for 2654579

[slumber]

CI failure seems to be unrelated

[slumber]

bot merge

[paritytech-processbot]

Waiting for commit status.