feat(storage): add EitherReader for routing history queries to MDBX or RocksDB#21063
feat(storage): add EitherReader for routing history queries to MDBX or RocksDB#21063
Conversation
fff917c to
d8a600c
Compare
49154c0 to
f8a98e7
Compare
f8a98e7 to
7dcc5e3
Compare
a795126 to
8e8e6b7
Compare
feb3c69 to
585bcc1
Compare
gakonst
left a comment
There was a problem hiding this comment.
Code Review: PR #21063 (Base of RocksDB stack)
CI Status
- ❌ clippy failure
- ❌ udeps failure
- ❌ test / ethereum / edge failure
Changes Summary
Adds EitherReader abstraction to route history lookups to MDBX or RocksDB based on storage settings. Good design pattern.
Review Notes
✅ Good
- Clean abstraction with
EitherReaderenum variants with_rocksdb_txhelper nicely encapsulates cfg-gated code- Proper rank/select logic for history lookups
needs_prev_shard_checkreused correctly
🟡 Suggestions
-
Duplicated rank/select logic: The rank adjustment code is duplicated in both
account_history_infoandstorage_history_info:let mut rank = chunk.rank(block_number); if rank.checked_sub(1).and_then(|r| chunk.select(r)) == Some(block_number) { rank -= 1; } let found_block = chunk.select(rank);
Consider extracting to a helper function (I see #21064 adds
compute_history_rank- good). -
Test pattern match update: Line 1032 updates to
EitherReader::Database(_)but line 1027 still hasEitherReader::StaticFile(_, _)- inconsistent pattern. Should beStaticFile(..)for consistency with other changes. -
PhantomData inconsistency:
StaticFilevariant keepsPhantomData<&'a ()>but comment says it's for lifetime when rocksdb feature is disabled. TheDatabasevariant no longer has it - verify this compiles correctly with all feature combinations.
Blocking Issues
Fix CI failures before this can be merged - likely clippy warnings and unused deps.
Review by @tempo_ai
This wires RocksDB into the history lookup paths: - Adds account_history_info and storage_history_info methods to EitherReader - Updates HistoricalStateProviderRef to use EitherReader for lookups - Adds RocksDBProviderFactory trait bounds to provider impls - Uses the rank/select pattern for efficient binary search in shards
**Problem** - EitherReader had unnecessary PhantomData markers - RocksDB transaction setup was duplicated in historical.rs with cfg-gated blocks - Addressed joshieDo's feedback about RocksDB logic leaking into historical provider **Solution** - Remove PhantomData from EitherReader enum variants (lifetime already captured by RocksDB reference) - Add with_rocksdb_tx helper method to RocksDBProviderFactory trait - Refactor historical.rs to use trait method instead of duplicated cfg-gated blocks **Changes** - Remove PhantomData from EitherReader enum and all constructors/match arms - Add with_rocksdb_tx to RocksDBProviderFactory trait with default implementation - Refactor account_history_lookup and storage_history_lookup to use with_rocksdb_tx helper - Make RocksTxRefArg type alias public for trait method **Expected Impact** - Cleaner EitherReader API without unnecessary PhantomData - RocksDB transaction setup encapsulated in trait method - Reduced cfg-gated block duplication in historical.rs - No behavioral changes, all existing tests pass (96/97)
When rocksdb feature is disabled, the RocksDB variant is compiled out, leaving the lifetime 'a unused and causing E0392 error. Add PhantomData<&'a ()> to StaticFile variant to ensure the lifetime is always used regardless of feature flags.
…lication - Add compute_history_rank() function for shared rank/select logic - Simplify EitherReader::storage_history_info and account_history_info - Simplify RocksTx::history_info by using the shared helper - Fix clippy doc_markdown warning for RocksDB Reduces duplicated rank/select code across 3 locations while preserving comments.
Previously, when RocksDB tables were empty but MDBX had checkpoints > 0, the consistency check would return Some(0), triggering an assertion failure because unwinding to block 0 is considered destructive. This is the expected state when RocksDB is enabled for the first time alongside existing MDBX data. The fix treats empty RocksDB tables as a first-run/migration scenario, logging a warning instead of requesting an unwind. The pipeline will naturally populate the tables during sync.
- Use edge feature instead of separate rocksdb feature for enabling RocksDB - Remove 'expected on first run' warning logs from invariants.rs - Keep Debug impl in historical.rs (required by lint)
RocksDB already propagates through existing edge feature chain: reth-ethereum-cli/edge → reth-cli-commands/edge → reth-db-common/edge → reth-provider/rocksdb
bbbe927 to
280f62c
Compare
2e064e0 to
bea16c0
Compare
Co-authored-by: joshieDo <93316087+joshieDo@users.noreply.github.com>
Co-authored-by: joshieDo <93316087+joshieDo@users.noreply.github.com>
| |key| key.key == address, | ||
| self.lowest_available_blocks.account_history_block_number, | ||
| ) | ||
| self.provider.with_rocksdb_tx(|rocks_tx_ref| { |
There was a problem hiding this comment.
but i think we have to turn the with_rocksdb_tx/batch and the batch we pass to the eihereader an option, otherwise we'll always be creating it even for legacy nodes
Summary
Adds
EitherReaderabstraction to route historical state queries (account history, storage history) to either MDBX or RocksDB based on storage settings. This is the foundation for using RocksDB as an alternative backend for history indices.Changes
EitherReader Abstraction
EitherReaderenum withDatabaseandRocksDBvariants ineither_writer.rsaccount_history_info()andstorage_history_info()methods that handle shard lookupscompute_history_rank()andneeds_prev_shard_check()helpers to reduce duplication between backendsHistorical State Provider Integration
EitherReaderintoHistoricalStateProviderReffor account and storage lookupswith_rocksdb_tx()helper toRocksDBProviderFactorytrait for scoped RocksDB transactionsRocksDB Provider Enhancements
history_info()method toRocksTxfor efficient shard lookups using raw iteratorswrite_blocks_dataPR Stack