From 22c1de501b63e6ea8700cb3f200789d6c5cc6d3f Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin <5773434+shekhirin@users.noreply.github.com> Date: Tue, 28 Jan 2025 12:00:39 +0000 Subject: [PATCH] feat(tree): handle no-op updates in trie update differences (#14013) --- crates/engine/tree/src/tree/trie_updates.rs | 150 ++++++++++---------- 1 file changed, 77 insertions(+), 73 deletions(-) diff --git a/crates/engine/tree/src/tree/trie_updates.rs b/crates/engine/tree/src/tree/trie_updates.rs index a2e7dee3a..6ada60ea0 100644 --- a/crates/engine/tree/src/tree/trie_updates.rs +++ b/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 { @@ -20,7 +20,7 @@ struct EntryDiff { struct TrieUpdatesDiff { account_nodes: HashMap>>, removed_nodes: HashMap>, - storage_tries: HashMap, + storage_tries: HashMap, } 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>, @@ -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::>() { - 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( task: &mut StorageTrieUpdates, regular: &mut StorageTrieUpdates, ) -> Result { - 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( .cloned() .collect::>() { - 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, + }, + ); } }