Skip to content

Conversation

@haardikk21
Copy link
Contributor

This PR fixes a bug affecting eth_call and eth_simulateV1 that caused incorrect state overrides to happen when the same address had distinct storage slots modified intra-block but across flashblock boundaries

e.g. consider a contract with two storage slots - A and B. Initially, say A = 1 and B = 1

Flashblock 1:

  • updates A = 2

Flashblock 2:

  • updates B = 2

The way the old logic worked, due to a misunderstanding of how Reth applies state diff overrides, setting an AccountOverride for B = 2 in the pending state would actually cause us to lose the account override for A = 2

Therefore, an eth_call or eth_simulateV1 that depended on reading the value of A would end up returning an incorrect value

This was never caught in our tests for two reasons:

  • the Counter contract test had only a single storage slot that could be modified, so we never tested behaviour across multiple storage slots
  • the Counter contract's storage was also initialized to 0 at the beginning, so we couldn't differentiate between an "initial result" vs. a "this got reset" result

This PR does two things:

  1. Gracefully "merges" previously existing pending state diffs and new state diff upon executing transactions to override any slots that have changed for the n'th time, and adding/keeping any slots that have only changed once
  2. Replaces the Counter contract tests with a DoubleCounter. It has two storage slots - count1 and count2 that can both be incremented and tested separately. It also initializes those values to 1 instead of default 0 - so a 0 result at any point is an error caused due to a state reset somewhere.

This is partially related to #151. @steph-rs is continuing to work on an even more comprehensive test setup with ERC-20 contract tests.

@haardikk21 haardikk21 changed the title fix: merge state diffs cumulatively, improve eth_call tests fix: merge state diffs when overriding accounts, improve eth_call tests Oct 30, 2025
@niran
Copy link
Contributor

niran commented Oct 30, 2025

I think we should consider abandoning StateOverride for this use case. If my understanding is correct, this block is basically an alternative implementation of what State.take_bundle() does out of the box. A BundleState can be applied to a another State with StateBuilder.with_bundle_prestate(). BundleState is basically a specialized version of StateOverride for state modified by locally executed transactions. We should only need StateOverride for things like eth_call where the state changes have been derived by a third party.

@haardikk21
Copy link
Contributor Author

I think we should consider abandoning StateOverride for this use case. If my understanding is correct, this block is basically an alternative implementation of what State.take_bundle() does out of the box. A BundleState can be applied to a another State with StateBuilder.with_bundle_prestate(). BundleState is basically a specialized version of StateOverride for state modified by locally executed transactions. We should only need StateOverride for things like eth_call where the state changes have been derived by a third party.

Interesting, i'm gonna need to explore that a bit. Since this is affecting downstream clients, I would like to get this merged and released first and I will look into the BundleState situation afterwards and see if it can help make this whole setup simpler.

@danyalprout danyalprout merged commit 7f0a07b into main Oct 30, 2025
8 checks passed
@danyalprout danyalprout deleted the merge-state-diffs branch October 30, 2025 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants