Add OnPostInherent Hook (try 2)
#10128
Conversation
shawntabrizi
added
B7-runtimenoteworthy
D9-needsaudit 👮
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
| let block_id = &self.block_id; | ||
|
|
||
| self.api | ||
| .on_post_inherent_with_context(&block_id, ExecutionContext::BlockConstruction)?; |
There was a problem hiding this comment.
what will happen if runtime have not yet implemented this API?
There was a problem hiding this comment.
Block Production and Block Execution would come to different results.
There was a problem hiding this comment.
I think we need to think bit more about how to upgrade.
New runtime + old client needs to panic, else I think it will cause forking
Old runtime + new client should have the same behavior as old runtime + old client else it is forking
So the upgrade order should be upgrade client, and then runtime. And all the old client shouldn't be able to produce blocks for new runtime. Syncing maybe is fine?
There was a problem hiding this comment.
syncing should be fine since there will be no implementations of on_post_inherent before this PR, so everything will be a noop.
I agree, this is clearly a client breaking change, and should be handled like any hard change, but also only if the feature is being used.
There is not much danger to release this in the wild as long as the feature is not used.
There was a problem hiding this comment.
if the client call the on_post_inherent api but the api doesn't exist in the runtime, I don't think it will be no-op. I think it will panic, no ?
there should be a way to only execute on_post_inherent if the api is provided by the runtime (by using version or stuff like this https://paritytech.github.io/substrate/master/sp_api/macro.decl_runtime_apis.html#runtime-api-trait-versioning), this way the client support all kind of runtime.
Thought the client will have to upgrade before the runtime, an old client will produce invalid block.
There was a problem hiding this comment.
The more concrete solution to me would be:
- bump the version of BlockBuilder
- in the client we check the version of BlockBuilder and call into on_post_inherent only if version is superior or equal to the new version
When release a version we need to say that client must upgrade before runtime. or at least before runtime implement on_post_inherent with something not no-op.
|
We need to bump version of block builder |
|
yeah for me the solution is:
When release a version we need to say that client must upgrade before runtime. or at least before runtime implement |
|
For anyone awaiting this feature, our current work-around is to create a dummy empty-payload inherent just to kick off the computation you want to run. |
ggwpez
added
the
A5-stale
|
stale |
louismerlin
removed
the
D9-needsaudit 👮
Fixes paritytech/polkadot-sdk#312
Fixes #5757
This PR introduces a new
OnPostInherenthook within the block builder/execution pipeline.This allows runtime developers to execute code after all inherent extrinsics have been executed, but before any signed extrinsics.
We introduce this hook only for the
#[pallet]macro and not for thedecl_module!macro. (it wouldnt be so hard to enable this if we wanted... but seems we should encourage people to start giving updecl_module!for new features.)This should be completely backwards compatible with existing pallets which do not take advantage of this hook.