-
Notifications
You must be signed in to change notification settings - Fork 668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Vision: Execute a hook after inherent but before transactions #312
Comments
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 |
I can see cumulus is already doing something similar |
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:
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 Then, I think |
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 |
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 |
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 |
Gui made a PR not so long ago which made all inherents before transactions: I will work on the feature as outlined by @pepyakin this weekend |
Since both Furthermore, one of them (or both) should add up weight for the @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? |
@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. |
We do have high-ish priority need for this to remove a big footgun for parachains. We can just tell parachain teams never invoke |
agree, and on_post_inherent should have and alias called on_initialize, so we're totally backwards compat. |
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 |
@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. |
One thing that I asked myself in the recent weeks is if we also need some kind of "two phase" |
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. |
|
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 inon_finalize
is also not idea due to risk of exceed block time limit.The text was updated successfully, but these errors were encountered: