Vision: Execute a hook after inherent but before transactions

[xlc]

We have a lot of per block logics that we want to execute them in the beginning of the block. However we don't really want to use on_initialize because it is executed before inherent are been processed. This means data from inherent are not available, such as latest timestamp and relaychain block number. Having those logic in on_finalize is also not idea due to risk of exceed block time limit.

[xlc]

Maybe we can create a custom mandatory inherent to achieve this goal?

[bkchr]

Maybe we can create a custom mandatory inherent to achieve this goal?

We don't need this actually, however I'm not sure we want this feature at all.

WDYT @shawntabrizi

[xlc]

I can see cumulus is already doing something similar

https://github.com/paritytech/cumulus/blob/f119e39033d3ff92e5e5695bad585808870ddc65/pallets/parachain-system/src/lib.rs#L325-L336

[pepyakin]

It's not the first time we have this discussion. There are actually several places like that in that module.

IMO, Ideally we had three block lifecycle callbacks:

1. on_pre_inherent. Currently is known as on_initialize.
2. on_post_inherent. This is called after all inherents are submitted and before the other extrinsics are to be submitted or on_finalize called.
3. on_finalize

I think this system is more friendly to problems we encountered. One of such problems is that order of inherents may matter. In that case we would need to impose some restrictions in the handlers of such inherents: the handler of the inherent A cannot run before handler B. This problem can be completely sidestepped, if we only saved the required data in the storage and do the actual handling in on_post_inherent where all the data is available. This eliminates questions about the order of submission.

Then, I think on_post_inherent is more suitable for general initialization logic (such that found in on_initialize currently) compared to on_pre_inherent. For example, consider the upcoming change to the upgrade mechanism in cumulus. In a nutshell, there will be a signal from the relay-chain that can disable upgrades for a parachain. That is, if the para issues an upgrade that parablock will become invalid. In order to communicate the relay-chain's will, the collator should submit a relay-chain storage proof via an inherent. That means that during on_initialize we do not know if the relay-chain allows for the upgrade or not. Thus for safety we reject all requests at upgrading happened before the appropriate inherent is posted to err on the safe side.

[kianenigma]

This will also help with the fact that we do (or used to paritytech/substrate#8953) initialize_block on all runtime api calls, and if we move most of the heavy work to on_post_inherent, we don't really need to skip on_initialize since it will contain much less work.

[rphmeier]

I think at the moment in FRAME it's legal for inherents to interleave with transactions. The block builder does not create such blocks but we'd be imposing a hard limitation on FRAME that did not exist before. I think this is OK as it gives more flexibility.

Furthermore, there's the matter of weights. We can either have on_pre_inherent determine the weight for on_post_inherent or have on_post_inherent return its own weight. Given that this is all before user-submitted transactions, I lean towards the latter.

[rphmeier]

Given that this changes the block authorship APIs we might couple this with other changes for making transaction inclusion more efficient i.e. speeding up parachains

[shawntabrizi]

I think at the moment in FRAME it's legal for inherents to interleave with transactions

Gui made a PR not so long ago which made all inherents before transactions:

paritytech/substrate#8173

I will work on the feature as outlined by @pepyakin this weekend

[pepyakin]

Furthermore, there's the matter of weights. We can either have on_pre_inherent determine the weight for on_post_inherent or have on_post_inherent return its own weight. Given that this is all before user-submitted transactions, I lean towards the latter.

Since both on_pre_inherent and on_post_inherent are really on the mandatory path of execution, I do not think they should return weight for any of them. Both of them should return weight for their execution.

Furthermore, one of them (or both) should add up weight for the on_finalize. I think maybe it's OK to give a guidance for developers that only one of these on_{pre,post}_inherent should return weight for on_finalize for a specific use-case.

@shawntabrizi it's your call. However, I am a bit uncomfortable of rushing this change right away: there is no immediate need and this is a rather big departure from the status quo. Maybe try to play in mind with this concept to try to uncover its downsides?

[shawntabrizi]

@pepyakin I actually do not see any departure from status quo here. From my perspective, we are adding a new hook, which if not used, everything will behave as before.

[xlc]

We do have high-ish priority need for this to remove a big footgun for parachains. We can just tell parachain teams never invoke system.setCode in on_initialize directly or indirectly, but the fact accessing timestamp & relaychain number in on_initialize will get you old values is always a big footgun and many people are not aware of this. It could easily trigger some economic security issue if timestamp value is accessed in on_initialize after chain stalled for significant period of time.

[kianenigma]

@pepyakin I actually do not see any departure from status quo here. From my perspective, we are adding a new hook, which if not used, everything will behave as before.

agree, and on_post_inherent should have and alias called on_initialize, so we're totally backwards compat.

[Polkadot-Forum]

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/open-discussion-thread-fall-2022-roadmap-roundup/542/7

[kianenigma]

@xlc if you want this, it is just a matter of taking the above PR, and merging master in it AFAIK.

I will mark it as mentor to attract more folks.

[bkchr]

One thing that I asked myself in the recent weeks is if we also need some kind of "two phase" on_finalize. My idea here being that in the first phase you are wrapping up and maybe still calling into other pallets, while the later phase is then to only do some clean ups or whatever. It may happens that you write data to some storage item that you then need to remove at the end of the block. However, will writing this, I think that these kind of use cases could be solved with the ephemeral storage items.

[riusricardo]

While many of you are already aware of a recent issue in a Polkadot governance proposal. I still want to mention it here to show why we need to fix the logic.
This operation of pushing code and the header upgrade to on_initialize is unsafe, because the inherents can overwrite it.
https://polkadot.polkassembly.io/motion/393

[ggwpez]

after_inherents is being added here paritytech/substrate#14414, although currently not exposed to pallets or runtime.