Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 28, 2025
+77 −73
Merged

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

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5cf3588
feat(tree): compare trie updates better
shekhirin Jan 27, 2025
1b0da64
Merge remote-tracking branch 'origin/main' into alexey/state-root-tas…
shekhirin Jan 27, 2025
030f6d1
do not log no-ops
shekhirin Jan 27, 2025
afc9dc5
add comments
shekhirin Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 77 additions & 73 deletions crates/engine/tree/src/tree/trie_updates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use reth_trie::{
updates::{StorageTrieUpdates, TrieUpdates},
BranchNodeCompact, Nibbles,
};
use tracing::debug;
use tracing::warn;

#[derive(Debug)]
struct EntryDiff<T> {
Expand All @@ -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 {
Expand All @@ -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 (
Expand All @@ -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 {
Expand All @@ -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>>,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
},
);
}
}

Expand All @@ -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);
}
}
}
Expand All @@ -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()
};

Expand Down Expand Up @@ -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,
},
);
}
}

Expand Down
Loading