Skip to content

Commit

Permalink
fix(l1): remove "inconsistent internal tree structure" panics (#1288)
Browse files Browse the repository at this point in the history
**Motivation**

The `Trie` library assumes that all tries are well-formed: this means
that if some node contains the hash of a child, then that child is in
the tire's database. If this doesn't hold, then a `get()` call panics
with an `inconsistent internal tree structure` message.

For the L2 prover we're using "pruned tries" which only store the
relevant nodes for the set of values touched in some execution, breaking
the previous hypothesis. One option was to capture the panics on the get
calls, but this wasn't safe because `Trie` has internal mutability. The
path chosen is to remove the panics altogether and replace them with
errors.

**Description**

- replaces mentioned panics for errors
  • Loading branch information
xqft authored Nov 29, 2024
1 parent 1208981 commit 3b25e94
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 23 deletions.
31 changes: 22 additions & 9 deletions crates/l2/prover/zkvm/interface/src/lib.rs
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)?,
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
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,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 @@ -112,7 +112,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 @@ -291,7 +291,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 @@ -306,7 +306,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

0 comments on commit 3b25e94

Please sign in to comment.