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

fix(l1): remove "inconsistent internal tree structure" panics #1288

Merged
merged 10 commits into from
Nov 29, 2024
Merged

fix(l1): remove "inconsistent internal tree structure" panics #1288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3c2b46a
catch panics
xqft Nov 26, 2024
73232ef
Revert "catch panics"
xqft Nov 27, 2024
853ac3c
remove tree structure panics
xqft Nov 27, 2024
eceb832
handle inconsistent tree error
xqft Nov 27, 2024
bb151ff
better handling
xqft Nov 27, 2024
d831bdb
handle storage key in case of new account
xqft Nov 27, 2024
8420829
remove print
xqft Nov 27, 2024
620630e
handle case encoded account is none
xqft Nov 27, 2024
3180141
format
xqft Nov 27, 2024
962956c
fix clippy
xqft Nov 29, 2024
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
31 changes: 22 additions & 9 deletions crates/l2/prover/zkvm/interface/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,32 +100,45 @@ pub mod trie {
} else {
// Add or update AccountState in the trie
// Fetch current state or create a new state to be inserted
let mut account_state = match state_trie.get(&hashed_address)? {
Some(encoded_state) => AccountState::decode(&encoded_state)?,
None => AccountState::default(),
let account_state = state_trie.get(&hashed_address);

// if there isn't a path into the account (inconsistent tree error), then
// it's potentially a new account. This is because we're using pruned tries
// so the path into a new account might not be included in the pruned state trie.
let mut account_state = match account_state {
Ok(Some(encoded_state)) => AccountState::decode(&encoded_state)?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use "?" instead of matching with multiple Oks/Err?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't because we want to match the InconsistentTree error and interpret it as a "state not found" error instead of propagating it

Ok(None) | Err(TrieError::InconsistentTree) => AccountState::default(),
Err(err) => return Err(err.into()),
};
let is_account_new = account_state == AccountState::default();

if let Some(info) = &update.info {
account_state.nonce = info.nonce;
account_state.balance = info.balance;
account_state.code_hash = info.code_hash;
}
// Store the added storage in the account's storage trie and compute its new root
if !update.added_storage.is_empty() {
let storage_trie = storage_tries
.get_mut(&update.address)
.ok_or(Error::StorageNotFound(update.address))?;
let storage_trie = if is_account_new {
let trie = Trie::from_nodes(None, &[])?;
storage_tries.insert(update.address, trie);
storage_tries.get_mut(&update.address).unwrap()
} else {
storage_tries
.get_mut(&update.address)
.ok_or(Error::StorageNotFound(update.address))?
};
for (storage_key, storage_value) in &update.added_storage {
let hashed_key = hash_key(storage_key);
if storage_value.is_zero() {
if storage_value.is_zero() && !is_account_new {
storage_trie.remove(hashed_key)?;
} else {
} else if !storage_value.is_zero() {
storage_trie.insert(hashed_key, storage_value.encode_to_vec())?;
}
}
account_state.storage_root = storage_trie.hash_no_commit();
}
state_trie.insert(hashed_address, account_state.encode_to_vec())?;
println!("inserted new state");
}
}
Ok(())
Expand Down
2 changes: 2 additions & 0 deletions crates/storage/trie/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ pub enum TrieError {
RLPDecode(#[from] RLPDecodeError),
#[error("Verification Error: {0}")]
Verify(String),
#[error("Inconsistent internal tree structure")]
InconsistentTree,
}
10 changes: 5 additions & 5 deletions crates/storage/trie/node/branch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ impl BranchNode {
if child_hash.is_valid() {
let child_node = state
.get_node(child_hash.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
child_node.get(state, path)
} else {
Ok(None)
Expand Down Expand Up @@ -94,7 +94,7 @@ impl BranchNode {
choice_hash => {
let child_node = state
.get_node(choice_hash.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;

let child_node = child_node.insert(state, path, value)?;
*choice_hash = child_node.insert_self(state)?;
Expand Down Expand Up @@ -139,7 +139,7 @@ impl BranchNode {
if self.choices[choice_index].is_valid() {
let child_node = state
.get_node(self.choices[choice_index].clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
// Remove value from child node
let (child_node, old_value) = child_node.remove(state, path.clone())?;
if let Some(child_node) = child_node {
Expand Down Expand Up @@ -180,7 +180,7 @@ impl BranchNode {
let (choice_index, child_hash) = children[0];
let child = state
.get_node(child_hash.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
match child {
// Replace self with an extension node leading to the child
Node::Branch(_) => ExtensionNode::new(
Expand Down Expand Up @@ -254,7 +254,7 @@ impl BranchNode {
if child_hash.is_valid() {
let child_node = state
.get_node(child_hash.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
child_node.get_path(state, path, node_path)?;
}
}
Expand Down
10 changes: 5 additions & 5 deletions crates/storage/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 {
if path.skip_prefix(&self.prefix) {
let child_node = state
.get_node(self.child.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;

child_node.get(state, path)
} else {
Expand Down Expand Up @@ -59,7 +59,7 @@ impl ExtensionNode {
// Insert into child node
let child_node = state
.get_node(self.child)?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
let new_child_node =
child_node.insert(state, path.offset(match_index), value.clone())?;
self.child = new_child_node.insert_self(state)?;
Expand All @@ -76,7 +76,7 @@ impl ExtensionNode {
Some(Node::Leaf(leaf)) => {
BranchNode::new_with_value(Box::new(choices), leaf.value)
}
_ => panic!("inconsistent internal tree structure"),
_ => return Err(TrieError::InconsistentTree),
}
} else {
choices[self.prefix.at(0)] = new_node;
Expand Down Expand Up @@ -109,7 +109,7 @@ impl ExtensionNode {
if path.skip_prefix(&self.prefix) {
let child_node = state
.get_node(self.child)?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
// Remove value from child subtrie
let (child_node, old_value) = child_node.remove(state, path)?;
// Restructure node based on removal
Expand Down Expand Up @@ -189,7 +189,7 @@ impl ExtensionNode {
if path.skip_prefix(&self.prefix) {
let child_node = state
.get_node(self.child.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
child_node.get_path(state, path, node_path)?;
}
Ok(())
Expand Down
8 changes: 4 additions & 4 deletions crates/storage/trie/trie.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ impl Trie {
let root_node = self
.state
.get_node(root.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
root_node.get(&self.state, Nibbles::from_bytes(path))
} else {
Ok(None)
Expand Down Expand Up @@ -115,7 +115,7 @@ impl Trie {
let root_node = self
.state
.get_node(root)?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
let (root_node, old_value) =
root_node.remove(&mut self.state, Nibbles::from_bytes(&path))?;
self.root = root_node
Expand Down Expand Up @@ -294,7 +294,7 @@ impl Trie {
let child_node = self
.state
.get_node(child_hash.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
self.get_node_inner(child_node, partial_path)
} else {
Ok(vec![])
Expand All @@ -309,7 +309,7 @@ impl Trie {
let child_node = self
.state
.get_node(extension_node.child.clone())?
.expect("inconsistent internal tree structure");
.ok_or(TrieError::InconsistentTree)?;
self.get_node_inner(child_node, partial_path)
} else {
Ok(vec![])
Expand Down