Check for State Modification from Extrinsic Errors on Syncing Nodes

[shawntabrizi]

A rule of all extrinsics should be that if it results in an error, it does not modify state. This is the "Check First, Write Last" rule.

We try out best to do this in all the various runtime unit tests, but it is very likely the we or other Substrate builders have missed some.

I suggest we add a feature flag that any syncing node can add to their chain which will process the blocks per extrinsic, check the state root before, run the extrinsic, see if it returns error, and then check the state root after.

If this occurs, then we may have a problem in the runtime code, and it should emit a WARNING or ERROR log which we can monitor.

[hirschenberger]

Should this be implemented in the executive pallet or in client?
Is this the position to hook into? https://github.com/paritytech/substrate/blob/master/client/service/src/client/client.rs#L888
Unfortunately I can't find the implementation of the execute_block_with_context function anywhere 🤔

[bkchr]

_with_context is just some postfix. Essentially this is the execute_block function.

But this would need to be done in "deep" in the runtime. This means we would require some special wasm that is used by the syncing nodes.

[hirschenberger]

"deep in the runtime" sounds as if you want give me hint that this may be beyond my substrate knowledge yet? 😁

[bkchr]

https://github.com/paritytech/substrate/blob/master/primitives/runtime/src/generic/checked_extrinsic.rs#L80

[hirschenberger]

Thanks for the entrypoint. So it seems that when an extrinsic fails in executive, it simply panics. Is it correct, that this will be propagated to the dispatch call you marked?
So would it be enough to check sp_io::storage::root(); before and after the dispatch and check if they are equal in the error case?

[bkchr]

It only panics on import.

And the code I linked is called by the executive.

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.