Skip to content

fix(provider): cap storage_v2 unwind history by MDBX tip#23335

Merged
joshieDo merged 13 commits into
mainfrom
joshie/unwind-reader-barrier
Apr 2, 2026
Merged

fix(provider): cap storage_v2 unwind history by MDBX tip#23335
joshieDo merged 13 commits into
mainfrom
joshie/unwind-reader-barrier

Conversation

@joshieDo
Copy link
Copy Markdown
Collaborator

@joshieDo joshieDo commented Apr 1, 2026

Alternative to #23291 and #23305.

For storage_v2 unwind, this keeps the MDBX-first ordering but waits after the MDBX commit and before RocksDB so a reader opened before the unwind cannot observe unwound MDBX state and then later route through pre-unwind Rocks history.

It then caps RocksDB history lookups by the reader's MDBX-visible tip instead of adding a second wait/fence. Example: if a reader's MDBX snapshot only sees block N, a later Rocks snapshot ignores history entries above N and falls back to the MDBX-visible prefix instead of routing into a newer changeset.

Also drops the validation RO provider before unwind_provider_rw() so the unwind wait cannot block on that provider itself.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 1, 2026

⚠️ Changelog not found.

A changelog entry is required before merging. We've generated a suggested changelog based on your changes:

Preview
---
reth-db-api: minor
reth-db: minor
reth-libmdbx: minor
reth-provider: minor
reth-stages-api: patch
---

Added `oldest_reader_txnid` and `last_txnid` methods to the `Database` trait and a new `ReaderTxnTracker` trait that waits for pre-commit MDBX readers to drain before applying RocksDB unwind batches in `storage_v2` mode. Added a `visible_tip` parameter to `account_history_info` and `storage_history_info` so RocksDB history lookups ignore entries above the companion MDBX snapshot's tip. Fixed a read provider lifetime issue in the pipeline unwind path by dropping the provider before the unwind target validation completes.

Add changelog to commit this to your branch.

Comment thread crates/storage/db-api/src/database.rs Outdated
Comment on lines +31 to +36
/// This is the committed txnid of the snapshot the reader is pinned to, not a unique per-reader
/// identifier, so multiple readers can report the same txnid.
///
/// Used to check whether stale readers from a previous write transaction have completed.
/// Returns `None` if no readers are active or the backend does not support this query.
fn oldest_reader_txnid(&self) -> Option<u64> {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is especially important to understand

Comment on lines +215 to +216
/// Database handle used to inspect active MDBX readers during unwind commits.
reader_txn_tracker: Option<Arc<dyn ReaderTxnTracker>>,
Copy link
Copy Markdown
Collaborator Author

@joshieDo joshieDo Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is essentially Database/DatabaseEnv, but didn't want to make it such an hard dependency. wdyt

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah having this generic over DB would be a mess

@joshieDo joshieDo marked this pull request as ready for review April 1, 2026 16:44
@joshieDo joshieDo requested a review from mediocregopher as a code owner April 1, 2026 18:55
@joshieDo joshieDo changed the title fix(provider): add storage_v2 unwind reader barrier fix(provider): cap storage_v2 unwind history by MDBX tip Apr 1, 2026
Comment on lines +215 to +216
/// Database handle used to inspect active MDBX readers during unwind commits.
reader_txn_tracker: Option<Arc<dyn ReaderTxnTracker>>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah having this generic over DB would be a mess

Comment on lines +1552 to 1557

// Only the prefix up to `visible_tip` should influence routing for this reader. Any
// later Rocks history entries are not yet visible in the companion MDBX snapshot.
let visible_count = chunk.rank(visible_tip);
let (rank, found_block) = compute_history_rank(&chunk, block_number);

Copy link
Copy Markdown
Member

@klkvr klkvr Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm couldn't we simplify this by assuming that visible_tip > block_number?

i was thinking that given that this always holds we basically have 2 cases

  • found_block > visible_tip which means that rocksdb is ahead AND we can safely treat this as found_block = None because the earliest block the key was changed in is after our mdbx view
  • found_block <= visible_tip which can be handled as is

@github-project-automation github-project-automation Bot moved this from Backlog to In Progress in Reth Tracker Apr 2, 2026
@joshieDo joshieDo added this pull request to the merge queue Apr 2, 2026
Merged via the queue into main with commit 4a6f9cd Apr 2, 2026
35 checks passed
@joshieDo joshieDo deleted the joshie/unwind-reader-barrier branch April 2, 2026 11:32
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Reth Tracker Apr 2, 2026
github-merge-queue Bot pushed a commit to tempoxyz/tempo that referenced this pull request Apr 3, 2026
Automated nightly update of reth dependencies from `paradigmxyz/reth`
main branch.

## Upstream reth changes


[`f8efc76...c82d435`](paradigmxyz/reth@f8efc76...c82d435)

🔗 Amp thread:
https://ampcode.com/threads/T-019d517e-981b-75fd-b3cf-8ad2455f38f6
**RPC**
- Remove `0x` prefix from `admin_peers` id and return keccak256 node ID
in `admin_nodeInfo.id` to match go-ethereum format
([#23318](paradigmxyz/reth#23318),
[#23319](paradigmxyz/reth#23319))
- Apply count filter only after `after` is consumed in pagination
([#23338](paradigmxyz/reth#23338))
- Pre-allocate vectors in `eth_feeHistory`
([#23334](paradigmxyz/reth#23334))
- Add `bal` RPC methods
([#23330](paradigmxyz/reth#23330))

**Engine**
- Include backpressure in reported `persistence_wait`, make wait-time
mimic CL slot time
([#23308](paradigmxyz/reth#23308))

**Perf**
- Reduce cacheline ping-pong in workers availability
([#23321](paradigmxyz/reth#23321))

**Net**
- Avoid itertools `Format` panic in tracing log
([#23331](paradigmxyz/reth#23331))

**DB**
- Cap `storage_v2` unwind history by MDBX tip
([#23335](paradigmxyz/reth#23335))

**Bench**
- Add warmup step for big blocks mode
([#23323](paradigmxyz/reth#23323))
- Make payload handling ethereum-only
([#23324](paradigmxyz/reth#23324))
- Use repository instead of GitHub Actions cache to track hourly runs
([#23306](paradigmxyz/reth#23306))
- Upload nightly regression results to ClickHouse
([#23344](paradigmxyz/reth#23344))

**Chore**
- Migrate `allow(clippy::` to `expect(clippy::`
([#23340](paradigmxyz/reth#23340))
- Remove unused return value from `dispatch_with_chunking`
([#23341](paradigmxyz/reth#23341))
- Update nixpkgs to 25.11 and refresh flake inputs
([#23342](paradigmxyz/reth#23342))

## Migrations

🔗 Amp thread:
https://ampcode.com/threads/T-019d517e-baf0-727b-a57e-0488240af973
- **Reth dependency bump**: All `reth-*` git dependencies updated from
rev `f8efc76` to `c82d435`
- **New `GetBlockAccessList` trait impl**: `TempoEthApi` now implements
`GetBlockAccessList` (imported from `reth_rpc_eth_api::helpers::bal`),
likely a new required trait added upstream in the reth update

[GitHub
Workflow](https://github.com/tempoxyz/tempo/actions/runs/23933027282)

---------

Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants