feat(tree): handle no-op updates in trie update differences (#14013)

crates/engine/tree/src/tree/trie_updates.rs

@@ -7,7 +7,7 @@ use reth_trie::{
     updates::{StorageTrieUpdates, TrieUpdates},
     BranchNodeCompact, Nibbles,
 };
-use tracing::debug;
+use tracing::warn;
 
 #[derive(Debug)]
 struct EntryDiff<T> {
@@ -20,7 +20,7 @@ struct EntryDiff<T> {
 struct TrieUpdatesDiff {
     account_nodes: HashMap<Nibbles, EntryDiff<Option<BranchNodeCompact>>>,
     removed_nodes: HashMap<Nibbles, EntryDiff<bool>>,
-    storage_tries: HashMap<B256, StorageTrieDiffEntry>,
+    storage_tries: HashMap<B256, StorageTrieUpdatesDiff>,
 }
 
 impl TrieUpdatesDiff {
@@ -33,7 +33,7 @@ impl TrieUpdatesDiff {
     pub(super) fn log_differences(mut self) {
         if self.has_differences() {
             for (path, EntryDiff { task, regular, database }) in &mut self.account_nodes {
-                debug!(target: "engine::tree", ?path, ?task, ?regular, ?database, "Difference in account trie updates");
+                warn!(target: "engine::tree", ?path, ?task, ?regular, ?database, "Difference in account trie updates");
             }
 
             for (
@@ -45,7 +45,7 @@ impl TrieUpdatesDiff {
                 },
             ) in &self.removed_nodes
             {
-                debug!(target: "engine::tree", ?path, ?task_removed, ?regular_removed, ?database_not_exists, "Difference in removed account trie nodes");
+                warn!(target: "engine::tree", ?path, ?task_removed, ?regular_removed, ?database_not_exists, "Difference in removed account trie nodes");
             }
 
             for (address, storage_diff) in self.storage_tries {
@@ -55,47 +55,6 @@ impl TrieUpdatesDiff {
     }
 }
 
-#[derive(Debug)]
-enum StorageTrieDiffEntry {
-    /// Storage Trie entry exists for one of the task or regular trie updates, but not the other.
-    Existence(bool, bool),
-    /// Storage Trie entries exists for both task and regular trie updates, but their values
-    /// differ.
-    Value(StorageTrieUpdatesDiff),
-}
-
-impl StorageTrieDiffEntry {
-    fn log_differences(self, address: B256) {
-        match self {
-            Self::Existence(task, regular) => {
-                debug!(target: "engine::tree", ?address, ?task, ?regular, "Difference in storage trie existence");
-            }
-            Self::Value(mut storage_diff) => {
-                if let Some(EntryDiff { task, regular, database }) = storage_diff.is_deleted {
-                    debug!(target: "engine::tree", ?address, ?task, ?regular, ?database, "Difference in storage trie deletion");
-                }
-
-                for (path, EntryDiff { task, regular, database }) in &mut storage_diff.storage_nodes
-                {
-                    debug!(target: "engine::tree", ?address, ?path, ?task, ?regular, ?database, "Difference in storage trie updates");
-                }
-
-                for (
-                    path,
-                    EntryDiff {
-                        task: task_removed,
-                        regular: regular_removed,
-                        database: database_not_exists,
-                    },
-                ) in &storage_diff.removed_nodes
-                {
-                    debug!(target: "engine::tree", ?address, ?path, ?task_removed, ?regular_removed, ?database_not_exists, "Difference in removed storage trie nodes");
-                }
-            }
-        }
-    }
-}
-
 #[derive(Debug, Default)]
 struct StorageTrieUpdatesDiff {
     is_deleted: Option<EntryDiff<bool>>,
@@ -109,6 +68,33 @@ impl StorageTrieUpdatesDiff {
             !self.storage_nodes.is_empty() ||
             !self.removed_nodes.is_empty()
     }
+
+    fn log_differences(&self, address: B256) {
+        if let Some(EntryDiff {
+            task: task_deleted,
+            regular: regular_deleted,
+            database: database_not_exists,
+        }) = self.is_deleted
+        {
+            warn!(target: "engine::tree", ?address, ?task_deleted, ?regular_deleted, ?database_not_exists, "Difference in storage trie deletion");
+        }
+
+        for (path, EntryDiff { task, regular, database }) in &self.storage_nodes {
+            warn!(target: "engine::tree", ?address, ?path, ?task, ?regular, ?database, "Difference in storage trie updates");
+        }
+
+        for (
+            path,
+            EntryDiff {
+                task: task_removed,
+                regular: regular_removed,
+                database: database_not_exists,
+            },
+        ) in &self.removed_nodes
+        {
+            warn!(target: "engine::tree", ?address, ?path, ?task_removed, ?regular_removed, ?database_not_exists, "Difference in removed storage trie nodes");
+        }
+    }
 }
 
 /// Compares the trie updates from state root task, regular state root calculation and database,
@@ -149,11 +135,20 @@ pub(super) fn compare_trie_updates(
         .cloned()
         .collect::<BTreeSet<_>>()
     {
-        let (task, regular) =
+        let (task_removed, regular_removed) =
             (task.removed_nodes.contains(&key), regular.removed_nodes.contains(&key));
-        let database = account_trie_cursor.seek_exact(key.clone())?.is_none();
-        if task != regular {
-            diff.removed_nodes.insert(key, EntryDiff { task, regular, database });
+        let database_not_exists = account_trie_cursor.seek_exact(key.clone())?.is_none();
+        // If the deletion is a no-op, meaning that the entry is not in the
+        // database, do not add it to the diff.
+        if task_removed != regular_removed && !database_not_exists {
+            diff.removed_nodes.insert(
+                key,
+                EntryDiff {
+                    task: task_removed,
+                    regular: regular_removed,
+                    database: database_not_exists,
+                },
+            );
         }
     }
 
@@ -168,20 +163,15 @@ pub(super) fn compare_trie_updates(
         let (mut task, mut regular) =
             (task.storage_tries.remove(&key), regular.storage_tries.remove(&key));
         if task != regular {
-            if let Some((task, regular)) = task.as_mut().zip(regular.as_mut()) {
-                let storage_diff = compare_storage_trie_updates(
-                    || trie_cursor_factory.storage_trie_cursor(key),
-                    task,
-                    regular,
-                )?;
-                if storage_diff.has_differences() {
-                    diff.storage_tries.insert(key, StorageTrieDiffEntry::Value(storage_diff));
-                }
-            } else {
-                diff.storage_tries.insert(
-                    key,
-                    StorageTrieDiffEntry::Existence(task.is_some(), regular.is_some()),
-                );
+            #[allow(clippy::or_fun_call)]
+            let storage_diff = compare_storage_trie_updates(
+                || trie_cursor_factory.storage_trie_cursor(key),
+                // Compare non-existent storage tries as empty.
+                task.as_mut().unwrap_or(&mut Default::default()),
+                regular.as_mut().unwrap_or(&mut Default::default()),
+            )?;
+            if storage_diff.has_differences() {
+                diff.storage_tries.insert(key, storage_diff);
             }
         }
     }
@@ -197,13 +187,17 @@ fn compare_storage_trie_updates<C: TrieCursor>(
     task: &mut StorageTrieUpdates,
     regular: &mut StorageTrieUpdates,
 ) -> Result<StorageTrieUpdatesDiff, DatabaseError> {
-    let database_deleted = trie_cursor()?.next()?.is_none();
+    let database_not_exists = trie_cursor()?.next()?.is_none();
     let mut diff = StorageTrieUpdatesDiff {
-        is_deleted: (task.is_deleted != regular.is_deleted).then_some(EntryDiff {
-            task: task.is_deleted,
-            regular: regular.is_deleted,
-            database: database_deleted,
-        }),
+        // If the deletion is a no-op, meaning that the entry is not in the
+        // database, do not add it to the diff.
+        is_deleted: (task.is_deleted != regular.is_deleted && !database_not_exists).then_some(
+            EntryDiff {
+                task: task.is_deleted,
+                regular: regular.is_deleted,
+                database: database_not_exists,
+            },
+        ),
         ..Default::default()
     };
 
@@ -232,11 +226,21 @@ fn compare_storage_trie_updates<C: TrieCursor>(
         .cloned()
         .collect::<BTreeSet<_>>()
     {
-        let (task, regular) =
+        let (task_removed, regular_removed) =
             (task.removed_nodes.contains(&key), regular.removed_nodes.contains(&key));
-        let database = storage_trie_cursor.seek_exact(key.clone())?.map(|x| x.1).is_none();
-        if task != regular {
-            diff.removed_nodes.insert(key, EntryDiff { task, regular, database });
+        let database_not_exists =
+            storage_trie_cursor.seek_exact(key.clone())?.map(|x| x.1).is_none();
+        // If the deletion is a no-op, meaning that the entry is not in the
+        // database, do not add it to the diff.
+        if task_removed != regular_removed && !database_not_exists {
+            diff.removed_nodes.insert(
+                key,
+                EntryDiff {
+                    task: task_removed,
+                    regular: regular_removed,
+                    database: database_not_exists,
+                },
+            );
         }
     }