fix: Don't always clone in-memory overlays in OverlayStateProviderFactory (#19383)

crates/storage/provider/src/providers/state/overlay.rs

@@ -1,6 +1,6 @@
 use alloy_primitives::{BlockNumber, B256};
 use reth_db_api::DatabaseError;
-use reth_errors::ProviderError;
+use reth_errors::{ProviderError, ProviderResult};
 use reth_prune_types::PruneSegment;
 use reth_stages_types::StageId;
 use reth_storage_api::{
@@ -71,7 +71,7 @@ impl<F> OverlayStateProviderFactory<F> {
 impl<F> OverlayStateProviderFactory<F>
 where
     F: DatabaseProviderFactory,
-    F::Provider: TrieReader + StageCheckpointReader + PruneCheckpointReader,
+    F::Provider: TrieReader + StageCheckpointReader + PruneCheckpointReader + BlockNumReader,
 {
     /// Validates that there are sufficient changesets to revert to the requested block number.
     ///
@@ -82,7 +82,7 @@ where
         &self,
         provider: &F::Provider,
         requested_block: BlockNumber,
-    ) -> Result<(), ProviderError> {
+    ) -> ProviderResult<()> {
         // Get the MerkleChangeSets stage and prune checkpoints.
         let stage_checkpoint = provider.get_stage_checkpoint(StageId::MerkleChangeSets)?;
         let prune_checkpoint = provider.get_prune_checkpoint(PruneSegment::MerkleChangeSets)?;
@@ -132,7 +132,7 @@ where
     type Provider = OverlayStateProvider<F::Provider>;
 
     /// Create a read-only [`OverlayStateProvider`].
-    fn database_provider_ro(&self) -> Result<OverlayStateProvider<F::Provider>, ProviderError> {
+    fn database_provider_ro(&self) -> ProviderResult<OverlayStateProvider<F::Provider>> {
         // Get a read-only provider
         let provider = self.factory.database_provider_ro()?;
 
@@ -147,34 +147,50 @@ where
             self.validate_changesets_availability(&provider, from_block)?;
 
             // Collect trie reverts
-            let mut trie_updates_mut = provider.trie_reverts(from_block + 1)?;
+            let mut trie_reverts = provider.trie_reverts(from_block + 1)?;
 
-            // Collect state reverts using HashedPostState::from_reverts
-            let reverted_state = HashedPostState::from_reverts::<KeccakKeyHasher>(
+            // Collect state reverts
+            //
+            // TODO(mediocregopher) make from_reverts return sorted
+            // https://github.com/paradigmxyz/reth/issues/19382
+            let mut hashed_state_reverts = HashedPostState::from_reverts::<KeccakKeyHasher>(
                 provider.tx_ref(),
                 from_block + 1..,
-            )?;
-            let mut hashed_state_mut = reverted_state.into_sorted();
+            )?
+            .into_sorted();
 
-            // Extend with overlays if provided
-            if let Some(trie_overlay) = &self.trie_overlay {
-                trie_updates_mut.extend_ref(trie_overlay);
-            }
+            // Extend with overlays if provided. If the reverts are empty we should just use the
+            // overlays directly, because `extend_ref` will actually clone the overlay.
+            let trie_updates = match self.trie_overlay.as_ref() {
+                Some(trie_overlay) if trie_reverts.is_empty() => Arc::clone(trie_overlay),
+                Some(trie_overlay) => {
+                    trie_reverts.extend_ref(trie_overlay);
+                    Arc::new(trie_reverts)
+                }
+                None => Arc::new(trie_reverts),
+            };
 
-            if let Some(hashed_state_overlay) = &self.hashed_state_overlay {
-                hashed_state_mut.extend_ref(hashed_state_overlay);
-            }
+            let hashed_state_updates = match self.hashed_state_overlay.as_ref() {
+                Some(hashed_state_overlay) if hashed_state_reverts.is_empty() => {
+                    Arc::clone(hashed_state_overlay)
+                }
+                Some(hashed_state_overlay) => {
+                    hashed_state_reverts.extend_ref(hashed_state_overlay);
+                    Arc::new(hashed_state_reverts)
+                }
+                None => Arc::new(hashed_state_reverts),
+            };
 
             debug!(
                 target: "providers::state::overlay",
                 ?block_hash,
                 ?from_block,
-                num_trie_updates = ?trie_updates_mut.total_len(),
-                num_state_updates = ?hashed_state_mut.total_len(),
+                num_trie_updates = ?trie_updates.total_len(),
+                num_state_updates = ?hashed_state_updates.total_len(),
                 "Reverted to target block",
             );
 
-            (Arc::new(trie_updates_mut), Arc::new(hashed_state_mut))
+            (trie_updates, hashed_state_updates)
         } else {
             // If no block_hash, use overlays directly or defaults
             let trie_updates =

crates/trie/common/src/hashed_state.rs

@@ -486,6 +486,13 @@ impl HashedPostStateSorted {
         &self.storages
     }
 
+    /// Returns `true` if there are no account or storage updates.
+    pub fn is_empty(&self) -> bool {
+        self.accounts.accounts.is_empty() &&
+            self.accounts.destroyed_accounts.is_empty() &&
+            self.storages.is_empty()
+    }
+
     /// Returns the total number of updates including all accounts and storage updates.
     pub fn total_len(&self) -> usize {
         self.accounts.accounts.len() +