diff --git a/crates/storage/provider/src/either_writer.rs b/crates/storage/provider/src/either_writer.rs index efa1032420..fcd3dea08a 100644 --- a/crates/storage/provider/src/either_writer.rs +++ b/crates/storage/provider/src/either_writer.rs @@ -83,14 +83,17 @@ pub type RawRocksDBBatch = (); /// Helper type for `RocksDB` transaction reference argument in reader constructors. /// -/// When `rocksdb` feature is enabled, this is a reference to a `RocksDB` transaction. -/// Otherwise, it's `()` (unit type) to allow the same API without feature gates. +/// When `rocksdb` feature is enabled, this is an optional reference to a `RocksDB` transaction. +/// The `Option` allows callers to skip transaction creation when `RocksDB` isn't needed +/// (e.g., on legacy MDBX-only nodes). +/// When `rocksdb` feature is disabled, it's `()` (unit type) to allow the same API without +/// feature gates. #[cfg(all(unix, feature = "rocksdb"))] -pub type RocksTxRefArg<'a> = &'a crate::providers::rocksdb::RocksTx<'a>; +pub type RocksTxRefArg<'a> = Option<&'a crate::providers::rocksdb::RocksTx<'a>>; /// Helper type for `RocksDB` transaction reference argument in reader constructors. /// -/// When `rocksdb` feature is enabled, this is a reference to a `RocksDB` transaction. -/// Otherwise, it's `()` (unit type) to allow the same API without feature gates. +/// When `rocksdb` feature is disabled, it's `()` (unit type) to allow the same API without +/// feature gates. #[cfg(not(all(unix, feature = "rocksdb")))] pub type RocksTxRefArg<'a> = (); @@ -762,7 +765,9 @@ impl<'a> EitherReader<'a, (), ()> { { #[cfg(all(unix, feature = "rocksdb"))] if provider.cached_storage_settings().storages_history_in_rocksdb { - return Ok(EitherReader::RocksDB(_rocksdb_tx)); + return Ok(EitherReader::RocksDB( + _rocksdb_tx.expect("storages_history_in_rocksdb requires rocksdb tx"), + )); } Ok(EitherReader::Database( @@ -782,7 +787,9 @@ impl<'a> EitherReader<'a, (), ()> { { #[cfg(all(unix, feature = "rocksdb"))] if provider.cached_storage_settings().transaction_hash_numbers_in_rocksdb { - return Ok(EitherReader::RocksDB(_rocksdb_tx)); + return Ok(EitherReader::RocksDB( + _rocksdb_tx.expect("transaction_hash_numbers_in_rocksdb requires rocksdb tx"), + )); } Ok(EitherReader::Database( @@ -802,7 +809,9 @@ impl<'a> EitherReader<'a, (), ()> { { #[cfg(all(unix, feature = "rocksdb"))] if provider.cached_storage_settings().account_history_in_rocksdb { - return Ok(EitherReader::RocksDB(_rocksdb_tx)); + return Ok(EitherReader::RocksDB( + _rocksdb_tx.expect("account_history_in_rocksdb requires rocksdb tx"), + )); } Ok(EitherReader::Database( @@ -1814,4 +1823,20 @@ mod rocksdb_tests { "Data should be visible after provider.commit()" ); } + + /// Test that `EitherReader::new_accounts_history` panics when settings require + /// `RocksDB` but no tx is provided (`None`). This is an invariant violation that + /// indicates a bug - `with_rocksdb_tx` should always provide a tx when needed. + #[test] + #[should_panic(expected = "account_history_in_rocksdb requires rocksdb tx")] + fn test_settings_mismatch_panics() { + let factory = create_test_provider_factory(); + + factory.set_storage_settings_cache( + StorageSettings::legacy().with_account_history_in_rocksdb(true), + ); + + let provider = factory.database_provider_ro().unwrap(); + let _ = EitherReader::<(), ()>::new_accounts_history(&provider, None); + } } diff --git a/crates/storage/provider/src/traits/rocksdb_provider.rs b/crates/storage/provider/src/traits/rocksdb_provider.rs index 07c9b47866..06548d2275 100644 --- a/crates/storage/provider/src/traits/rocksdb_provider.rs +++ b/crates/storage/provider/src/traits/rocksdb_provider.rs @@ -2,6 +2,7 @@ use crate::{ either_writer::{RawRocksDBBatch, RocksBatchArg, RocksTxRefArg}, providers::RocksDBProvider, }; +use reth_storage_api::StorageSettingsCache; use reth_storage_errors::provider::ProviderResult; /// `RocksDB` provider factory. @@ -21,15 +22,21 @@ pub trait RocksDBProviderFactory { /// Executes a closure with a `RocksDB` transaction for reading. /// /// This helper encapsulates all the cfg-gated `RocksDB` transaction handling for reads. + /// On legacy MDBX-only nodes (where `any_in_rocksdb()` is false), this skips creating + /// the `RocksDB` transaction entirely, avoiding unnecessary overhead. fn with_rocksdb_tx(&self, f: F) -> ProviderResult where + Self: StorageSettingsCache, F: FnOnce(RocksTxRefArg<'_>) -> ProviderResult, { #[cfg(all(unix, feature = "rocksdb"))] { - let rocksdb = self.rocksdb_provider(); - let tx = rocksdb.tx(); - f(&tx) + if self.cached_storage_settings().any_in_rocksdb() { + let rocksdb = self.rocksdb_provider(); + let tx = rocksdb.tx(); + return f(Some(&tx)); + } + f(None) } #[cfg(not(all(unix, feature = "rocksdb")))] f(()) @@ -85,3 +92,99 @@ pub trait RocksDBProviderFactory { } } } + +#[cfg(all(test, unix, feature = "rocksdb"))] +mod tests { + use super::*; + use reth_db_api::models::StorageSettings; + use std::sync::atomic::{AtomicUsize, Ordering}; + + /// Mock `RocksDB` provider that tracks `tx()` calls. + struct MockRocksDBProvider { + tx_call_count: AtomicUsize, + } + + impl MockRocksDBProvider { + const fn new() -> Self { + Self { tx_call_count: AtomicUsize::new(0) } + } + + fn tx_call_count(&self) -> usize { + self.tx_call_count.load(Ordering::SeqCst) + } + + fn increment_tx_count(&self) { + self.tx_call_count.fetch_add(1, Ordering::SeqCst); + } + } + + /// Test provider that implements [`RocksDBProviderFactory`] + [`StorageSettingsCache`]. + struct TestProvider { + settings: StorageSettings, + mock_rocksdb: MockRocksDBProvider, + temp_dir: tempfile::TempDir, + } + + impl TestProvider { + fn new(settings: StorageSettings) -> Self { + Self { + settings, + mock_rocksdb: MockRocksDBProvider::new(), + temp_dir: tempfile::TempDir::new().unwrap(), + } + } + + fn tx_call_count(&self) -> usize { + self.mock_rocksdb.tx_call_count() + } + } + + impl StorageSettingsCache for TestProvider { + fn cached_storage_settings(&self) -> StorageSettings { + self.settings + } + + fn set_storage_settings_cache(&self, _settings: StorageSettings) {} + } + + impl RocksDBProviderFactory for TestProvider { + fn rocksdb_provider(&self) -> RocksDBProvider { + self.mock_rocksdb.increment_tx_count(); + RocksDBProvider::new(self.temp_dir.path()).unwrap() + } + + fn set_pending_rocksdb_batch(&self, _batch: rocksdb::WriteBatchWithTransaction) {} + } + + #[test] + fn test_legacy_settings_skip_rocksdb_tx_creation() { + let provider = TestProvider::new(StorageSettings::legacy()); + + let result = provider.with_rocksdb_tx(|tx| { + assert!(tx.is_none(), "legacy settings should pass None tx"); + Ok(42) + }); + + assert_eq!(result.unwrap(), 42); + assert_eq!(provider.tx_call_count(), 0, "should not create RocksDB tx for legacy settings"); + } + + #[test] + fn test_rocksdb_settings_create_tx() { + let settings = + StorageSettings { account_history_in_rocksdb: true, ..StorageSettings::legacy() }; + let provider = TestProvider::new(settings); + + let result = provider.with_rocksdb_tx(|tx| { + assert!(tx.is_some(), "rocksdb settings should pass Some tx"); + Ok(42) + }); + + assert_eq!(result.unwrap(), 42); + assert_eq!( + provider.tx_call_count(), + 1, + "should create RocksDB tx when any_in_rocksdb is true" + ); + } +}