perf: avoid creating RocksDB transactions for legacy MDBX-only nodes (#21325)

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);
+    }
 }

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<F, R>(&self, f: F) -> ProviderResult<R>
     where
+        Self: StorageSettingsCache,
         F: FnOnce(RocksTxRefArg<'_>) -> ProviderResult<R>,
     {
         #[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<true>) {}
+    }
+
+    #[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"
+        );
+    }
+}