Skip to content

Conversation

@yongkangc
Copy link
Member

@yongkangc yongkangc commented Dec 21, 2025

Summary

Complete RocksDB integration for history tables by wiring everything together. Closes #20388

This is PR 3/3 of a stacked PR series that splits #20412 for easier review.

Depends on: #20543#20542

Review focus

  1. Correctness: Do RocksDB history lookups match MDBX semantics
  2. Batch handling: Is the shared batch + last-shard cache correct?
  3. Deduplication: Unwind uses dedup for RocksDB's "last write wins" semantics

Stack

@github-project-automation github-project-automation bot moved this to Backlog in Reth Tracker Dec 21, 2025
@yongkangc yongkangc changed the title feat(storage): wire RocksDB into history lookups and HistoryWriter feat(storage): wire RocksDB into history lookups and HistoryWriter [3/3] Dec 21, 2025
@yongkangc yongkangc changed the title feat(storage): wire RocksDB into history lookups and HistoryWriter [3/3] feat(storage): wire RocksDB into DB Provider And Historical Provider Dec 21, 2025
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch from 3fe37d1 to 75a5a5d Compare December 21, 2025 00:20
@yongkangc yongkangc force-pushed the yk/pr2-rocksdb-infrastructure branch from e45acf9 to 40f84f2 Compare December 21, 2025 02:12
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch from 75a5a5d to 28e5af4 Compare December 21, 2025 02:12
@yongkangc yongkangc changed the title feat(storage): wire RocksDB into DB Provider And Historical Provider feat(storage): wire RocksDB into DB Provider And Historical Provider [3/3] Dec 21, 2025
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch from de43672 to 9e5131e Compare December 22, 2025 03:01
@yongkangc yongkangc force-pushed the yk/pr2-rocksdb-infrastructure branch from 7b937ec to 9aefeba Compare December 22, 2025 03:05
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch 2 times, most recently from 27ea434 to 6f04c58 Compare December 22, 2025 04:51
@yongkangc yongkangc force-pushed the yk/pr2-rocksdb-infrastructure branch from e885749 to 6b3de83 Compare December 22, 2025 04:55
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch from 6f04c58 to 48852bd Compare December 22, 2025 04:55
@yongkangc yongkangc force-pushed the yk/pr2-rocksdb-infrastructure branch 2 times, most recently from 46f23c6 to 5a518d5 Compare December 22, 2025 06:06
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch from 48852bd to 1a553ac Compare December 22, 2025 07:47
@yongkangc yongkangc self-assigned this Dec 22, 2025
@yongkangc yongkangc marked this pull request as ready for review December 22, 2025 08:12
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch 2 times, most recently from 9161ae0 to 7549845 Compare December 22, 2025 08:37
///
/// Uses the rank/select logic to efficiently find the first block >= target
/// where the account was modified.
pub fn account_history_info(
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the wiring up part, which is different from pr2 where its just rocks db

Comment on lines 169 to 184
/// Single `RocksDB` batch for history index operations.
/// All history index writes accumulate here and commit together at provider commit time.
#[cfg(all(unix, feature = "rocksdb"))]
pending_rocks_batch: parking_lot::Mutex<crate::providers::rocksdb::RocksDBBatch>,
/// In-memory cache of history-index "last shards" (the `u64::MAX` shard that receives appends)
/// for the shared `RocksDB` batch.
///
/// History index appends are read-modify-write: read last shard, merge new block numbers,
/// then write it back. With MDBX, the write transaction can read its own writes, so
/// subsequent appends see earlier updates. `RocksDBBatch` is write-only, so without this
/// cache each append would read only the persisted state and clobber earlier batch writes.
#[cfg(all(unix, feature = "rocksdb"))]
pending_history_index_last_shards: parking_lot::Mutex<PendingHistoryIndexLastShards>,
/// Pending `RocksDB` batches to be committed at provider commit time (from `EitherWriter`).
#[cfg(all(unix, feature = "rocksdb"))]
pending_rocksdb_batches: parking_lot::Mutex<Vec<rocksdb::WriteBatchWithTransaction<true>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels like rocksdb is leaking too much here

Copy link
Collaborator

Choose a reason for hiding this comment

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

and it's not clear to me why we even need mutexes for this

Comment on lines 1620 to 1622
// MDBX path
let mut cursor = self.tx.cursor_read::<tables::TransactionHashNumbers>()?;
Ok(cursor.seek_exact(tx_hash)?.map(|(_, v)| v))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// MDBX path
let mut cursor = self.tx.cursor_read::<tables::TransactionHashNumbers>()?;
Ok(cursor.seek_exact(tx_hash)?.map(|(_, v)| v))
Ok(self.tx.get::<tables::TransactionHashNumbers>(tx_hash)?)


#[cfg(all(unix, feature = "rocksdb"))]
{
// Commit the shared history index batch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Commit the shared history index batch


#[cfg(all(unix, feature = "rocksdb"))]
{
// Commit the shared history index batch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Commit the shared history index batch

Copy link
Collaborator

@joshieDo joshieDo left a comment

Choose a reason for hiding this comment

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

i think overall this needs to be refactored so rocksdb logic is mostly contained in a rocksdb type, right now its leaking with the mutexes and/or methods ending with _rocksdb

@github-project-automation github-project-automation bot moved this from Backlog to In Progress in Reth Tracker Dec 22, 2025
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch 2 times, most recently from 85460a0 to 9497ae7 Compare December 23, 2025 04:24
@yongkangc yongkangc force-pushed the yk/pr2-rocksdb-infrastructure branch from fad81ba to 59af41c Compare December 23, 2025 04:45
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch from 9497ae7 to 878fc62 Compare December 23, 2025 04:45
@yongkangc yongkangc force-pushed the yk/pr2-rocksdb-infrastructure branch from 59af41c to f3021e2 Compare December 23, 2025 06:17
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 pure find_changeset_block_from_index API with lazy cursor.prev()
@yongkangc yongkangc force-pushed the yk/pr3-rocksdb-history-routing branch from 878fc62 to 0a6a08f Compare December 23, 2025 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants