On the deprecation of checkInherents

[girazoki]

We have recently started to adapt our codebase (both in Tanssi and Moonbeam) to asynchronous backing and we noticed that the CheckInherents trait is being deprecated, and in fact, it has disappeared from most of the cumulus runtimes in the register_validate_block macro:

polkadot-sdk/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

Line 1624 in cad9479

cumulus_pallet_parachain_system::register_validate_block! {

However both moonbeam and tanssi have additional inherents, e.g., to verify block authorship, to inject randomness or to inject additional state proofs.

What we realized when removing this checkInherents is that a malicious author could decide not to inject one of the inherents (e.g., the inherent that validates block authorship) and the relay would still accept it, while the rest of the network would reject it because they would fail importing the block.

Our question goes in the direction of: why is checkInherents being deprecated? I know parachain-system now has the ConsensusHook trait that can be used to verify a set of things in the set_validation_data inherent but what happens with those inherents that required an additional argument to be injected? (e.g., an additional state proof)

[ggwpez]

cc @paritytech/parachains-core

[girazoki]

I think the best solution we have is to set a boolean storage that verifies that the inherent has run. on_initialize we kill this storage and on_finalize we verify. SImilar to how parachain-system performs this check

However this needs to be documented properly for the substrate framework as without this a collator can intentionally not include one of the inherents and the relay will never complain.

[librelois]

Actually, the only thing we need is to verify that all mandatory inherents are included, and this check is generated by the macro construct_runtime! here: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/procedural/src/construct_runtime/expand/inherent.rs#L161

I think that the macro register_validate_block! should auto include this check whatever the configuration, but for the moment this check (all mandatory inherent included) is added "manually" via CheckInherents.

[ggwpez]

Actually, the only thing we need is to verify that all mandatory inherents are included

Yes indeed. I originally thought that this issue is only about parachains, but we need to lockdown the inherent order and requirements indeed.
Please see #2154. It should be ready for review, but currently has low prio since it gathered no approvals.

[librelois]

Please see #2154.

I've just looked at this PR and it doesn't seem to include a check that the mandatory inherents are present. As a block builder, being forced to order inherents isn't enough; I can "forget" one anyway.

[ggwpez]

If the ProvideInherent::is_inherent_required returns true then it should be checked to always be present, otherwise it is optional.
But i think we often dont use that, and instead panic in on_finalize, which is ugly.

[librelois]

i think we often dont use that, and instead panic in on_finalize, which is ugly.

And undocumented, on moonbeam and tanssi we have a mandatory inherent that dosn't use this ugly trick, so we rely only on is_inherent_required.
More broadly speaking, it doesn't make sense to have a "Mandatory" dispatch class, and not to include the basic checks to make ensure that Mandatory things are included.

[ggwpez]

More broadly speaking, it doesn't make sense to have a "Mandatory" dispatch class, and not to include the basic checks to make ensure that Mandatory things are included.

"Mandatory" is actually a bad name, since it is used to describe different things:

1. An extrinsic that must execute in every block (required)
2. An extrinsic that must not fail (infallible)
3. Both 1. and 2. at the same time

Executive interprets it in the sense of 2., which is confusing. The Mandatory dispatch class is more of an Infallible, but does not make sense outside of inherents.
We thought about changing the DispatchClass more to a WeightClass, since that is what it is currently describing. But i dont think there was much traction.

And undocumented, on moonbeam and tanssi we have a mandatory inherent that dosn't use this ugly trick, so we rely only on is_inherent_required.

But IIUC then with my MR and when using is_inherent_required, it should check the existence and exact order or all non-optional inherents.

[bkchr]

I think the best solution we have is to set a boolean storage that verifies that the inherent has run. on_initialize we kill this storage and on_finalize we verify. SImilar to how parachain-system performs this check

This is actually the only correct solution for this issue. Inherent checking is only an optional "task" that is currently executed by all Substrate nodes, but actually should only be done by validators. While for parachains it would also mean that you should check it at validate_block.

The proper way forward should be that all inherents are required. If certain inherents are optional, they should be changed to make the data they carry an optional and not just omit them.

[librelois]

Executive interprets it in the sense of 2., which is confusing. The Mandatory dispatch class is more of an Infallible

That's not quite true: the DispatchClass is also used to determine whether or not to insert the extrinsic, depending on how much space is left in the block. Even if an extrinsic is Infallible, it may well not be included in the block if there's no room (if on_initialize takes up the whole palce, for example).
However, the current behavior of DispatchClass Mandatory always inserts the extrinsic, whatever space is left in the block, so it's not just limited to Infallible.

But IIUC then with my MR and when using is_inherent_required, it should check the existence and exact order or all non-optional inherents.

The problem is that is_inherent_required is not called by default, although it should be for all inherents.

This is actually the only correct solution for this issue.

It has to be documented, because we didn't know about it, fortunately we realized it through extensive testing, but we almost pushed a major flaw into production.

The proper way forward should be that all inherents are required. If certain inherents are optional, they should be changed to make the data they carry an optional and not just omit them.

This would solve the problem and simplify the framework, so I'm all for it.