fix(storage): pin RocksDB snapshot for cross-store read consistency#23305
Closed
joshieDo wants to merge 9 commits into
Closed
fix(storage): pin RocksDB snapshot for cross-store read consistency#23305joshieDo wants to merge 9 commits into
joshieDo wants to merge 9 commits into
Conversation
…tency HistoricalStateProvider takes its MDBX read tx at creation time (MVCC snapshot) but previously took a fresh RocksDB snapshot on every account_history_lookup/storage_history_lookup call. During aggressive reorgs, RemoveBlocksAbove commits MDBX then RocksDB sequentially — a reader with an older MDBX tx could see unwound RocksDB indices, causing InPlainState fallback to return state at the wrong tip. Fix: capture both MDBX read tx and RocksDB snapshot together in ProviderFactory::provider() with a retry loop that verifies no write commit landed between the two (via last_txnid check). The pinned snapshot is threaded through DatabaseProvider → HistoricalStateProvider and used for all history lookups instead of per-query snapshots. This only affects storage_v2 where history indices live in RocksDB. With storage_v1, history indices are in MDBX and share the read tx's MVCC snapshot automatically. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019d4393-7938-71ab-a3fd-0132444583bd
Contributor
|
Amp-Thread-ID: https://ampcode.com/threads/T-019d4435-3bfe-7739-b47f-fbcf6473a16a Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019d4435-3bfe-7739-b47f-fbcf6473a16a Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019d4435-3bfe-7739-b47f-fbcf6473a16a Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019d4435-3bfe-7739-b47f-fbcf6473a16a Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019d4435-3bfe-7739-b47f-fbcf6473a16a Co-authored-by: Amp <amp@ampcode.com>
joshieDo
commented
Mar 31, 2026
Comment on lines
+295
to
+297
| if let Some(snapshot) = pinned_rocksdb_snapshot { | ||
| provider = provider.with_pinned_rocksdb_snapshot(snapshot); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
option since storage.v1 doesnt need/use it
Amp-Thread-ID: https://ampcode.com/threads/T-019d4435-3bfe-7739-b47f-fbcf6473a16a Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019d4435-3bfe-7739-b47f-fbcf6473a16a Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019d4435-3bfe-7739-b47f-fbcf6473a16a Co-authored-by: Amp <amp@ampcode.com>
joshieDo
added
C-bug
Rjected
reviewed
Mar 31, 2026
Member
Rjected
left a comment
Rjected
left a comment
There was a problem hiding this comment.
just one comment otherwise i think this does fix the stated bug
Comment on lines
+245
to
+247
| /// The helper retries if an MDBX write commit lands between opening the read transaction and | ||
| /// taking the `RocksDB` snapshot. This keeps both stores aligned for historical reads that | ||
| /// span MDBX state and `RocksDB` history indices. |
Member
There was a problem hiding this comment.
I think we need to note that this:
- allows rocksdb to be ahead of mdbx, ie mdbx at block 7 and rocksdb up to block 10, and
- we need docs that outline what behavior is safe, for example the engine is safe because it creates txs sequentially, and we only ever perform SaveBlocks after we have closed a tx and added a block to the tree. I am not sure a single historical provider should outlive both a
remove_blocksmdbx commit and a subsequentsave_blocks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #23291.
Fixes block gas mismatch during aggressive reorgs with
storage_v2.HistoricalStateProvidertakes a fresh RocksDB snapshot per query, but its MDBX read tx is from creation time. IfRemoveBlocksAbovecommits between the two, the reader sees unwound RocksDB indices with pre-unwind MDBX state —InPlainStatefallback returns wrong values.Fix: pin the RocksDB snapshot at
DatabaseProvidercreation alongside the MDBX read tx, with alast_txnidretry loop to ensure consistency. Thread it through toHistoricalStateProviderfor all history lookups.