Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Check for State Modification from Extrinsic Errors on Syncing Nodes #325

Closed
shawntabrizi opened this issue Apr 6, 2021 · 8 comments
Closed

Check for State Modification from Extrinsic Errors on Syncing Nodes #325

shawntabrizi opened this issue Apr 6, 2021 · 8 comments
Labels
A2-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@shawntabrizi
Copy link
Member

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
Copy link
Contributor

hirschenberger commented Apr 15, 2021 •
edited
Loading

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
Copy link
Member

bkchr commented Apr 15, 2021

_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
Copy link
Contributor

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

@bkchr
Copy link
Member

bkchr commented Apr 15, 2021

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

@hirschenberger
Copy link
Contributor

hirschenberger commented Apr 16, 2021 •
edited
Loading

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
Copy link
Member

bkchr commented Apr 17, 2021

It only panics on import.

And the code I linked is called by the executive.

@stale
Copy link

stale bot commented Jul 7, 2021

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.

@stale stale bot added the A5-stale label Jul 7, 2021
@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added A2-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. T1-FRAME This PR/Issue is related to core FRAME, the framework. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. and removed A5-stale labels Aug 25, 2023
@ggwpez
Copy link
Member

ggwpez commented Nov 30, 2023

Stale since we revert storage changes in the error case.

@ggwpez ggwpez closed this as completed Nov 30, 2023
@github-project-automation github-project-automation bot moved this from Backlog to Done in Runtime / FRAME Nov 30, 2023
claravanstaden pushed a commit to Snowfork/polkadot-sdk that referenced this issue Dec 8, 2023
@Rizziepit
Benchmarks for basic channel (paritytech#325)
6267f6d 
* Add Verifier::initialize_storage function for genesis + benchmarking

* MessageDispatch::successful_dispatch_event helper for benchmark verification

* Benchmarks for basic channel

* Fix dot-app bench initialization

* Set module balances in spec.json
github-merge-queue bot pushed a commit that referenced this issue Feb 20, 2025
@lexnv
net/litep2p: Bring the latest compatibility fixes via v0.9.1 (#7640)
42e9de7 
This PR updates litep2p to version 0.9.1. The yamux config is entirely
removed to mirror the libp2p yamux upstream version.
While at it, I had to bump indexmap and URL as well. 


## [0.9.1] - 2025-01-19

This release enhances compatibility between litep2p and libp2p by using
the latest Yamux upstream version. Additionally, it includes various
improvements and fixes to boost the stability and performance of the
WebSocket stream and the multistream-select protocol.

### Changed

- yamux: Switch to upstream implementation while keeping the controller
API ([#320](paritytech/litep2p#320))
- req-resp: Replace SubstreamSet with FuturesStream
([#321](paritytech/litep2p#321))
- cargo: Bring up to date multiple dependencies
([#324](paritytech/litep2p#324))
- build(deps): bump hickory-proto from 0.24.1 to 0.24.3
([#323](paritytech/litep2p#323))
- build(deps): bump openssl from 0.10.66 to 0.10.70
([#322](paritytech/litep2p#322))

### Fixed

- websocket/stream: Fix unexpected EOF on `Poll::Pending` state
poisoning ([#327](paritytech/litep2p#327))
- websocket/stream: Avoid memory allocations on flushing
([#325](paritytech/litep2p#325))
- multistream-select: Enforce `io::error` instead of empty protocols
([#318](paritytech/litep2p#318))
- multistream: Do not wait for negotiation in poll_close
([#319](paritytech/litep2p#319))

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <[email protected]>
github-actions bot pushed a commit that referenced this issue Feb 20, 2025
@lexnv @github-actions
net/litep2p: Bring the latest compatibility fixes via v0.9.1 (#7640)
76e9c54 
This PR updates litep2p to version 0.9.1. The yamux config is entirely
removed to mirror the libp2p yamux upstream version.
While at it, I had to bump indexmap and URL as well.

## [0.9.1] - 2025-01-19

This release enhances compatibility between litep2p and libp2p by using
the latest Yamux upstream version. Additionally, it includes various
improvements and fixes to boost the stability and performance of the
WebSocket stream and the multistream-select protocol.

### Changed

- yamux: Switch to upstream implementation while keeping the controller
API ([#320](paritytech/litep2p#320))
- req-resp: Replace SubstreamSet with FuturesStream
([#321](paritytech/litep2p#321))
- cargo: Bring up to date multiple dependencies
([#324](paritytech/litep2p#324))
- build(deps): bump hickory-proto from 0.24.1 to 0.24.3
([#323](paritytech/litep2p#323))
- build(deps): bump openssl from 0.10.66 to 0.10.70
([#322](paritytech/litep2p#322))

### Fixed

- websocket/stream: Fix unexpected EOF on `Poll::Pending` state
poisoning ([#327](paritytech/litep2p#327))
- websocket/stream: Avoid memory allocations on flushing
([#325](paritytech/litep2p#325))
- multistream-select: Enforce `io::error` instead of empty protocols
([#318](paritytech/litep2p#318))
- multistream: Do not wait for negotiation in poll_close
([#319](paritytech/litep2p#319))

cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <[email protected]>
(cherry picked from commit 42e9de7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A2-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

No branches or pull requests
6 participants
@hirschenberger @shawntabrizi @juangirini @bkchr @ggwpez @the-right-joyce