Skip to content

refactor(l1): make TrieError::InconsistentTree more specific #4836

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

Merged
merged 22 commits into from
Oct 22, 2025
Merged

refactor(l1): make TrieError::InconsistentTree more specific #4836

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3f423cf
make TrieError::InconsistentTree point the node's hash
cdiielsi Oct 9, 2025
a0d62b6
Merge branch 'main' into more-specific-trie-error
cdiielsi Oct 13, 2025
335cac6
add more errors
cdiielsi Oct 13, 2025
8f37de6
upgrade error name to be shorter
cdiielsi Oct 13, 2025
91f0139
print NodeRef instead of the hash for all cases
cdiielsi Oct 14, 2025
d1ba664
merge main into branch
cdiielsi Oct 15, 2025
782f3bf
Merge branch 'main' into more-specific-trie-error
cdiielsi Oct 16, 2025
61c17d6
show node hashes for any type of node
cdiielsi Oct 16, 2025
569a4c1
fix for clippy
cdiielsi Oct 16, 2025
c96891c
drop impl Debug for NodeHash
cdiielsi Oct 16, 2025
5f0c9bf
new enum for inconsistent tree error more specific
cdiielsi Oct 17, 2025
3118135
new struct for ExtensionNodeError metadata boxed to not mess with per…
cdiielsi Oct 17, 2025
7d44ad2
new get_root_node method
cdiielsi Oct 17, 2025
2b94661
change errors for insertion on Extension node
cdiielsi Oct 21, 2025
47863ea
Merge branch 'main' into more-specific-trie-error
cdiielsi Oct 21, 2025
ab4f7cd
Box InconsistentTreeError
cdiielsi Oct 21, 2025
e721265
drop Box used in InconsistentTreeError and add comment on why Box
cdiielsi Oct 21, 2025
aa26484
Merge branch 'main' into more-specific-trie-error
cdiielsi Oct 21, 2025
ff58529
Merge branch 'main' into more-specific-trie-error
cdiielsi Oct 21, 2025
dad633a
add comment on why Box
cdiielsi Oct 21, 2025
7d2cc47
Merge branch 'more-specific-trie-error' of github.com:lambdaclass/eth…
cdiielsi Oct 21, 2025
8a1b84a
Merge branch 'main' into more-specific-trie-error
cdiielsi Oct 21, 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
11 changes: 9 additions & 2 deletions crates/common/trie/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use ethereum_types::H256;
use ethrex_rlp::error::RLPDecodeError;
use thiserror::Error;

Expand All @@ -7,8 +8,14 @@ pub enum TrieError {
RLPDecode(#[from] RLPDecodeError),
#[error("Verification Error: {0}")]
Verify(String),
#[error("Inconsistent internal tree structure")]
InconsistentTree,
#[error("Inconsistent internal tree structure: Node with hash {0:?} not found")]
InconsistentTree(H256),
#[error(
"Inconsistent internal tree structure: Node with hash {0:?} not found from Intermediate Node with hash {1:?}"
)]
IntermediateNodeNotFound(H256, H256),
#[error("Root node with hash {0:#x} not found")]
RootNotFound(H256),
Copy link
Contributor

@JereSalo JereSalo Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this comment may be kind of annoying but isn't it better if we have just one enum for trie node not found and then we have the different variants inside of it? For me it would make more sense but I prefer hearing other people's opinions. We have 3 errors for saying the same thing, node wasn't found.

Copy link
Collaborator

@MegaRedHand MegaRedHand Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea, but maybe in another PR. Removing InconsistentTree might also be worth it, since it's really unspecific, and probably could be replaced with multiple variants on this new enum.

#[error("Lock Error: Panicked when trying to acquire a lock")]
LockError,
#[error("Database error: {0}")]
Expand Down
32 changes: 20 additions & 12 deletions crates/common/trie/node/branch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ impl BranchNode {
// Delegate to children if present
let child_ref = &self.choices[choice];
if child_ref.is_valid() {
let child_node = child_ref
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?;
let child_node = child_ref.get_node(db, path.current())?.ok_or_else(|| {
TrieError::InconsistentTree(child_ref.compute_hash().finalize())
})?;
child_node.get(db, path)
} else {
Ok(None)
Expand Down Expand Up @@ -75,9 +75,9 @@ impl BranchNode {
}
// Insert into existing child and then update it
(choice_ref, ValueOrHash::Value(value)) => {
let child_node = choice_ref
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?;
let child_node = choice_ref.get_node(db, path.current())?.ok_or_else(|| {
TrieError::InconsistentTree(choice_ref.compute_hash().finalize())
})?;

*choice_ref = child_node.insert(db, path, value)?.into();
}
Expand All @@ -92,7 +92,9 @@ impl BranchNode {
} else {
*choice_ref = choice_ref
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?
.ok_or_else(|| {
TrieError::InconsistentTree(choice_ref.compute_hash().finalize())
})?
.insert(db, path, value)?
.into();
}
Expand Down Expand Up @@ -141,7 +143,11 @@ impl BranchNode {
if self.choices[choice_index].is_valid() {
let child_node = self.choices[choice_index]
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?;
.ok_or_else(|| {
TrieError::InconsistentTree(
self.choices[choice_index].compute_hash().finalize(),
)
})?;
// Remove value from child node
let (child_node, old_value) = child_node.remove(db, path.clone())?;
if let Some(child_node) = child_node {
Expand Down Expand Up @@ -182,7 +188,9 @@ impl BranchNode {
let (choice_index, child_ref) = children[0];
let child = child_ref
.get_node(db, base_path.current().append_new(choice_index as u8))?
.ok_or(TrieError::InconsistentTree)?;
.ok_or_else(|| {
TrieError::InconsistentTree(child_ref.compute_hash().finalize())
})?;
match child {
// Replace self with an extension node leading to the child
Node::Branch(_) => ExtensionNode::new(
Expand Down Expand Up @@ -249,9 +257,9 @@ impl BranchNode {
// Continue to child
let child_ref = &self.choices[choice];
if child_ref.is_valid() {
let child_node = child_ref
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?;
let child_node = child_ref.get_node(db, path.current())?.ok_or_else(|| {
TrieError::InconsistentTree(child_ref.compute_hash().finalize())
})?;
child_node.get_path(db, path, node_path)?;
}
}
Expand Down
19 changes: 11 additions & 8 deletions crates/common/trie/node/extension.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ impl ExtensionNode {
let child_node = self
.child
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?;
.ok_or_else(|| TrieError::InconsistentTree(self.child.compute_hash().finalize()))?;

child_node.get(db, path)
} else {
Expand Down Expand Up @@ -61,7 +61,7 @@ impl ExtensionNode {
let child_node = self
.child
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?;
.ok_or_else(|| TrieError::InconsistentTree(self.child.compute_hash().finalize()))?;
let new_child_node = child_node.insert(db, path, value)?;
self.child = new_child_node.into();
Ok(self.into())
Expand All @@ -75,7 +75,11 @@ impl ExtensionNode {
let branch_node = if self.prefix.at(0) == 16 {
match new_node.get_node(db, path.current())? {
Some(Node::Leaf(leaf)) => BranchNode::new_with_value(choices, leaf.value),
_ => return Err(TrieError::InconsistentTree),
_ => {
return Err(TrieError::InconsistentTree(
new_node.compute_hash().finalize(),
));
}
}
} else {
choices[self.prefix.at(0)] = new_node;
Expand Down Expand Up @@ -109,7 +113,7 @@ impl ExtensionNode {
let child_node = self
.child
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?;
.ok_or_else(|| TrieError::InconsistentTree(self.child.compute_hash().finalize()))?;
// Remove value from child subtrie
let (child_node, old_value) = child_node.remove(db, path)?;
// Restructure node based on removal
Expand Down Expand Up @@ -173,10 +177,9 @@ impl ExtensionNode {
};
// Continue to child
if path.skip_prefix(&self.prefix) {
let child_node = self
.child
.get_node(db, path.current())?
.ok_or(TrieError::InconsistentTree)?;
let child_node = self.child.get_node(db, path.current())?.ok_or_else(|| {
TrieError::InconsistentTree(self.child.clone().compute_hash().finalize())
})?;
child_node.get_path(db, path, node_path)?;
}
Ok(())
Expand Down
31 changes: 20 additions & 11 deletions crates/common/trie/trie.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ impl Trie {
&self
.db
.get(Nibbles::default())?
.ok_or(TrieError::InconsistentTree)?,
.ok_or_else(|| TrieError::RootNotFound(hash.finalize()))?,
)
.map_err(TrieError::RLPDecode)?
.get(self.db.as_ref(), path)?,
Expand All @@ -121,7 +121,7 @@ impl Trie {
// If the trie is not empty, call the root node's insertion logic.
self.root
.get_node(self.db.as_ref(), Nibbles::default())?
.ok_or(TrieError::InconsistentTree)?
.ok_or_else(|| TrieError::RootNotFound(self.root.compute_hash().finalize()))?
.insert(self.db.as_ref(), path, value)?
.into()
} else {
Expand All @@ -142,7 +142,7 @@ impl Trie {
let (node, value) = self
.root
.get_node(self.db.as_ref(), Nibbles::default())?
.ok_or(TrieError::InconsistentTree)?
.ok_or_else(|| TrieError::RootNotFound(self.root.compute_hash().finalize()))?
.remove(self.db.as_ref(), Nibbles::from_bytes(path))?;
self.root = node.map(Into::into).unwrap_or_default();

Expand Down Expand Up @@ -243,7 +243,7 @@ impl Trie {
let encoded_root = self
.root
.get_node(self.db.as_ref(), Nibbles::default())?
.ok_or(TrieError::InconsistentTree)?
.ok_or_else(|| TrieError::RootNotFound(self.root.compute_hash().finalize()))?
.encode_raw();

let mut node_path = HashSet::new();
Expand Down Expand Up @@ -277,7 +277,7 @@ impl Trie {

let root_rlp = all_nodes
.get(&root_hash)
.ok_or(TrieError::InconsistentTree)?;
.ok_or_else(|| TrieError::RootNotFound(root_hash))?;

fn get_embedded_node(
all_nodes: &BTreeMap<H256, Vec<u8>>,
Expand Down Expand Up @@ -401,9 +401,13 @@ impl Trie {
let child_ref = &branch_node.choices[idx];
if child_ref.is_valid() {
let child_path = current_path.append_new(idx as u8);
let child_node = child_ref
.get_node(db, child_path.clone())?
.ok_or(TrieError::InconsistentTree)?;
let child_node =
child_ref.get_node(db, child_path.clone())?.ok_or_else(|| {
TrieError::IntermediateNodeNotFound(
child_ref.compute_hash().finalize(),
branch_node.compute_hash().finalize(),
)
})?;
get_node_inner(db, child_path, child_node, partial_path)
} else {
Ok(vec![])
Expand All @@ -419,7 +423,12 @@ impl Trie {
let child_node = extension_node
.child
.get_node(db, child_path.clone())?
.ok_or(TrieError::InconsistentTree)?;
.ok_or_else(|| {
TrieError::IntermediateNodeNotFound(
extension_node.child.compute_hash().finalize(),
extension_node.compute_hash().finalize(),
)
})?;
get_node_inner(db, child_path, child_node, partial_path)
} else {
Ok(vec![])
Expand All @@ -436,7 +445,7 @@ impl Trie {
Default::default(),
self.root
.get_node(self.db.as_ref(), Default::default())?
.ok_or(TrieError::InconsistentTree)?,
.ok_or_else(|| TrieError::RootNotFound(self.root.compute_hash().finalize()))?,
partial_path,
)
} else {
Expand Down Expand Up @@ -481,7 +490,7 @@ impl ProofTrie {
self.0
.root
.get_node(self.0.db.as_ref(), Nibbles::default())?
.ok_or(TrieError::InconsistentTree)?
.ok_or_else(|| TrieError::RootNotFound(self.0.root.compute_hash().finalize()))?
.insert(self.0.db.as_ref(), partial_path, external_ref)?
.into()
} else {
Expand Down
2 changes: 1 addition & 1 deletion crates/networking/p2p/rlpx/connection/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ impl GenServer for PeerConnectionServer {
return CastResponse::Stop;
}
PeerConnectionError::StoreError(StoreError::Trie(
TrieError::InconsistentTree,
TrieError::InconsistentTree(_),
)) => {
if established_state.blockchain.is_synced() {
log_peer_error(
Expand Down