cumulus: add SignedSchedulingInfo PVF verification#12097
Conversation
For simplicity, reasoning and efficiency.
+ Refactor/cleanup + some docs.
parents Remove per-parachain tracking of allowed relay parents in backing implicit view. Allowed relay parents are now computed per relay parent (globally) based on scheduling lookahead, rather than per parachain. Changes: - Remove `collating_for` parameter and para_id tracking from View - Update all callers to remove para_id arguments - Refactor tests with helper functions to reduce ~150 lines of duplication - Add comprehensive documentation to tests explaining expected behavior - Clarify `paths_via_relay_parent` returns full paths from oldest block to leaf This simplification aligns with the reality that all parachains share the same allowed relay parent windows at a given relay chain block.
+ Fix a typo.
# Description An intermediate PR that just adds a trait that can be used to configure whether V3 collations should be built, and how to verify scheduling signatures. ## Integration N/A ## Review Notes Addresses #12097 (comment) mostly. --------- Signed-off-by: Iulian Barbu <iulian.barbu@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Lets also do a rebase on top of #10742 please. |
iulianbarbu
left a comment
There was a problem hiding this comment.
lgtm! good work
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| /// Returns `true` only when every step succeeds; all error paths return `false` (fail-closed) | ||
| /// so the PVF rejects the candidate without panicking on adversarial input. | ||
| /// | ||
| /// Steps: | ||
| /// 1. `signed_info.payload.internal_scheduling_parent` must equal the hash of the supplied | ||
| /// header. The caller (`check_scheduling`) has already verified the header hashes to the | ||
| /// derived internal scheduling parent, so this binds the signature to that same anchor. | ||
| /// 2. Read the relay slot from the BABE pre-digest of the header. | ||
| /// 3. Convert it to a parachain slot via `relay_slot * RELAY_CHAIN_SLOT_DURATION_MILLIS / | ||
| /// para_slot_duration`, using checked arithmetic. | ||
| /// 4. Pick the eligible author as `authorities[para_slot % authorities.len()]` from the cached | ||
| /// Aura authority set in this pallet. | ||
| /// 5. Decode the 64-byte signature blob as `<T::AuthorityId as RuntimeAppPublic>::Signature` | ||
| /// and verify it against the SCALE-encoded `SchedulingInfoPayload`. |
There was a problem hiding this comment.
nit: can we shorthen this?
| // 1. Decode relay slot from the BABE pre-digest of the internal_scheduling_parent header. | ||
| // The eligible parachain author is determined by *this* slot, anchoring the signature to | ||
| // a specific block (the one being submitted/resubmitted) rather than to a moving relay | ||
| // tip. `check_scheduling` proves this header is the actual relay block at | ||
| // internal_scheduling_parent — it can't be substituted. |
There was a problem hiding this comment.
nit:
| // 1. Decode relay slot from the BABE pre-digest of the internal_scheduling_parent header. | |
| // The eligible parachain author is determined by *this* slot, anchoring the signature to | |
| // a specific block (the one being submitted/resubmitted) rather than to a moving relay | |
| // tip. `check_scheduling` proves this header is the actual relay block at | |
| // internal_scheduling_parent — it can't be substituted. | |
| // 1. Relay slot at internal scheduling parent gives the para slot that determines the valid | |
| // author. |
| // 2. Convert relay slot to parachain slot. Both slot durations are in milliseconds; the | ||
| // relay slot duration is the global Polkadot/Kusama/Westend/Rococo value re-exported by | ||
| // polkadot-primitives, and the para slot duration is read from pallet-aura. Fail closed | ||
| // on overflow rather than saturating, so an out-of-range relay slot can't quietly | ||
| // produce a wrong author index. |
There was a problem hiding this comment.
| // 2. Convert relay slot to parachain slot. Both slot durations are in milliseconds; the | |
| // relay slot duration is the global Polkadot/Kusama/Westend/Rococo value re-exported by | |
| // polkadot-primitives, and the para slot duration is read from pallet-aura. Fail closed | |
| // on overflow rather than saturating, so an out-of-range relay slot can't quietly | |
| // produce a wrong author index. | |
| // 2. Determine the para slot. |
| use sp_keyring::{Ed25519Keyring, Sr25519Keyring}; | ||
| use sp_runtime::generic::Digest; | ||
|
|
||
| const PARA_SLOT_DURATION_MS: u64 = 6_000; |
There was a problem hiding this comment.
nit: worth testing with 24s slots too.
| // parachain state, which requires externalities — only available | ||
| // inside this scope. Run it once per PoV (on the first block) using | ||
| // the same authority set the seal was verified against. | ||
| if block_index == 0 { |
There was a problem hiding this comment.
I realise some parachain authorities could be changed after executing each block, so a more accurate check would be after executing all blocks. I suggested initially to have this check here, sorry.
Actually, during block building, the older authorities (from the parent), are relevant. Same applies during execution (the right to build the block is decided at the parent). Please ignore 😅
What I'd to at most is to run the signed scheduling info verification outside of the for loop - meaning we run_with_externalities_and_recorder against the parent_header, and not have this if block_index == 0 check at every iteration.
|
|
||
| // 5. Validate relay_parent position. relay_parent must NOT be inside the header | ||
| // chain — it either equals internal_scheduling_parent (initial submission) or is | ||
| // an ancestor of it (resubmission), but never between scheduling_parent and |
There was a problem hiding this comment.
Being an ancestor of the ISP is not checked here. Honest collators check it. Validators should check it too, via the ancestor_relay_parent_info calls, but not sure at which point during the backing/inclusion pipeline. TO check this.
| - audience: Runtime Dev | ||
| description: |- | ||
| Verifies the `SignedSchedulingInfo` payload inside the PVF when a candidate is a V3 | ||
| resubmission (`relay_parent != internal_scheduling_parent`). The verifier confirms |
There was a problem hiding this comment.
we verify now for initial submissions too, if the info is present
| - Concerns like parachain session or para slot duration changes must be handled by | ||
| the resubmission engine, not the on-chain verifier | ||
| (see paritytech/polkadot-sdk#12036 (comment) 4479412418). |
There was a problem hiding this comment.
- lets remove this (it is not necessarily a useful tip, more like an implementation detail that hasn't been tought through fully, but either way, not super relevant for the verifier at this point).
- I'd rather mention that enabling resubmissions should be done only after the relay-chain is enabled to support them (the CandidateReceiptV3 node feature is set on-chain & V4 collator-protocol will be used between collators and validators, after both validators & collators upgrade their node binaries to ones that support V4 collator-protocol).
|
|
||
| /// Map a slot to an author index in an authority set of size `authorities_count`, | ||
| /// or `None` if the set is empty. | ||
| pub fn slot_author_index(slot: Slot, authorities_count: usize) -> Option<u32> { |
There was a problem hiding this comment.
shouldn't we use the authorities_len() here?
Description
Verifies the scheduling info payload in PVF, if formed by an author that claims its para slot at internal scheduling parent.
Closes #12152
Integration
Runtime developers that want to enable resubmissions for their parachains must configure the newly added associated type on parachain-system's
Config:VerifySchedulingSignature.Review Notes
Concerns like parachain session or para slot duration changes must be handled by the resubmission engine, as described here: #12036 (comment).