paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.com/
1.89k stars 696 forks source link

Runtime: unify and harden UMP signal checks in check_core_index #6179

Closed sandreim closed 1 week ago

sandreim commented 3 weeks ago

Currently in para_inherent we do some filtering of candidates which includes checking if the core index is correct, but we do not check the count of UMP signals or format. If these are invalid we assume the parachain cannot use elastic scaling and we check the core index in the descriptor.

We need to move the UMP checks from verify_backed_candidate to the filtering stage in para_inherent so old nodes that don't check the number of UMP messages offchain filter candidates and don't panic during block production if a parachain sends a malformed signal.

I am not an expert on that code but it appears to me that we can move the verify_backed_candidate as last step in filtering stage just to ensure that filtering bugs on the node don't make the author panic.

alindima commented 3 weeks ago

We're already doing verify_backed_candidate during candidate filtering: https://github.com/paritytech/polkadot-sdk/blob/b4732add46910370443d092a3f479986060f6df5/polkadot/runtime/parachains/src/paras_inherent/mod.rs#L1412 so we won't panic during block production.

alindima commented 3 weeks ago

I opened another issue for refactoring the runtime code so that we only do candidate validation during filtering and remove this code from inclusion: https://github.com/paritytech/polkadot-sdk/issues/6186

It makes sense however to harden the core index check as you suggest here: https://github.com/paritytech/polkadot-sdk/pull/6178#discussion_r1812133305

sandreim commented 3 weeks ago

I opened another issue for refactoring the runtime code so that we only do candidate validation during filtering and remove this code from inclusion: #6186

It makes sense however to harden the core index check as you suggest here: #6178 (comment)

yup, sounds good to me.

As you say, we already call verify_backed_candidate in filtering, so we just need to keep filtering UMPSignal in check_upward_messages, but without the signal count checks.