From 21f303fc5bf5907303e64f1c9b7b9da93bfedf43 Mon Sep 17 00:00:00 2001 From: Alexey Shekhirin Date: Fri, 18 Oct 2024 10:45:17 +0100 Subject: [PATCH 1/2] fix(trie): removing a blinded leaf should result in an error --- crates/trie/sparse/src/trie.rs | 68 ++++++++++++++++++++++++++-------- 1 file changed, 52 insertions(+), 16 deletions(-) diff --git a/crates/trie/sparse/src/trie.rs b/crates/trie/sparse/src/trie.rs index 0b9ffb5c0ed4..d3991fadbc7a 100644 --- a/crates/trie/sparse/src/trie.rs +++ b/crates/trie/sparse/src/trie.rs @@ -267,11 +267,11 @@ impl RevealedSparseTrie { /// Remove leaf node from the trie. pub fn remove_leaf(&mut self, path: Nibbles) -> SparseTrieResult<()> { self.prefix_set.insert(path.clone()); - let existing = self.values.remove(&path); - if existing.is_none() { - // trie structure unchanged, return immediately - return Ok(()) - } + self.values.remove(&path); + + // If the path wasn't present in `values`, we still need to walk the trie and ensure that + // there is no node at the path. When a leaf node is a blinded `Hash`, it will have an entry + // in `nodes`, but not in the `values`. let mut removed_nodes = self.take_nodes_for_path(&path)?; debug!(target: "trie::sparse", ?path, ?removed_nodes, "Removed nodes for path"); @@ -726,6 +726,7 @@ mod tests { use super::*; use alloy_primitives::U256; + use assert_matches::assert_matches; use itertools::Itertools; use proptest::prelude::*; use rand::seq::IteratorRandom; @@ -959,7 +960,7 @@ mod tests { pretty_assertions::assert_eq!( sparse.nodes.clone().into_iter().collect::>(), BTreeMap::from_iter([ - (Nibbles::new(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), + (Nibbles::default(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), (Nibbles::from_nibbles([0x5]), SparseNode::new_branch(0b1101.into())), ( Nibbles::from_nibbles([0x5, 0x0]), @@ -971,11 +972,11 @@ mod tests { ), ( Nibbles::from_nibbles([0x5, 0x0, 0x2, 0x3, 0x1]), - SparseNode::new_leaf(Nibbles::new()) + SparseNode::new_leaf(Nibbles::default()) ), ( Nibbles::from_nibbles([0x5, 0x0, 0x2, 0x3, 0x3]), - SparseNode::new_leaf(Nibbles::new()) + SparseNode::new_leaf(Nibbles::default()) ), ( Nibbles::from_nibbles([0x5, 0x2]), @@ -1014,7 +1015,7 @@ mod tests { pretty_assertions::assert_eq!( sparse.nodes.clone().into_iter().collect::>(), BTreeMap::from_iter([ - (Nibbles::new(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), + (Nibbles::default(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), (Nibbles::from_nibbles([0x5]), SparseNode::new_branch(0b1001.into())), ( Nibbles::from_nibbles([0x5, 0x0]), @@ -1026,11 +1027,11 @@ mod tests { ), ( Nibbles::from_nibbles([0x5, 0x0, 0x2, 0x3, 0x1]), - SparseNode::new_leaf(Nibbles::new()) + SparseNode::new_leaf(Nibbles::default()) ), ( Nibbles::from_nibbles([0x5, 0x0, 0x2, 0x3, 0x3]), - SparseNode::new_leaf(Nibbles::new()) + SparseNode::new_leaf(Nibbles::default()) ), (Nibbles::from_nibbles([0x5, 0x3]), SparseNode::new_branch(0b1010.into())), ( @@ -1062,7 +1063,7 @@ mod tests { pretty_assertions::assert_eq!( sparse.nodes.clone().into_iter().collect::>(), BTreeMap::from_iter([ - (Nibbles::new(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), + (Nibbles::default(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), (Nibbles::from_nibbles([0x5]), SparseNode::new_branch(0b1001.into())), ( Nibbles::from_nibbles([0x5, 0x0]), @@ -1096,7 +1097,7 @@ mod tests { pretty_assertions::assert_eq!( sparse.nodes.clone().into_iter().collect::>(), BTreeMap::from_iter([ - (Nibbles::new(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), + (Nibbles::default(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), (Nibbles::from_nibbles([0x5]), SparseNode::new_branch(0b1001.into())), ( Nibbles::from_nibbles([0x5, 0x0]), @@ -1127,7 +1128,7 @@ mod tests { pretty_assertions::assert_eq!( sparse.nodes.clone().into_iter().collect::>(), BTreeMap::from_iter([ - (Nibbles::new(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), + (Nibbles::default(), SparseNode::new_ext(Nibbles::from_nibbles([0x5]))), (Nibbles::from_nibbles([0x5]), SparseNode::new_branch(0b1001.into())), ( Nibbles::from_nibbles([0x5, 0x0]), @@ -1146,7 +1147,7 @@ mod tests { pretty_assertions::assert_eq!( sparse.nodes.clone().into_iter().collect::>(), BTreeMap::from_iter([( - Nibbles::new(), + Nibbles::default(), SparseNode::new_leaf(Nibbles::from_nibbles([0x5, 0x3, 0x3, 0x0, 0x2])) ),]) ); @@ -1156,7 +1157,42 @@ mod tests { // Empty pretty_assertions::assert_eq!( sparse.nodes.clone().into_iter().collect::>(), - BTreeMap::from_iter([(Nibbles::new(), SparseNode::Empty),]) + BTreeMap::from_iter([(Nibbles::default(), SparseNode::Empty),]) + ); + } + + #[test] + fn sparse_trie_remove_leaf_blinded() { + let mut sparse = RevealedSparseTrie::default(); + + let leaf = LeafNode::new( + Nibbles::default(), + alloy_rlp::encode_fixed_size(&U256::from(1)).to_vec(), + ); + + // Reveal a branch node and one of its children + // + // Branch (Mask = 11) + // ├── 0 -> Hash (Path = 0) + // └── 1 -> Leaf (Path = 1) + sparse + .reveal_node( + Nibbles::default(), + TrieNode::Branch(BranchNode::new( + vec![ + RlpNode::word_rlp(&B256::repeat_byte(1)), + RlpNode::from_raw_rlp(&alloy_rlp::encode(leaf.clone())).unwrap(), + ], + TrieMask::new(0b11), + )), + ) + .unwrap(); + sparse.reveal_node(Nibbles::from_nibbles([0x1]), TrieNode::Leaf(leaf)).unwrap(); + + // Removing a blinded leaf should result in an error + assert_matches!( + sparse.remove_leaf(Nibbles::from_nibbles([0x0])), + Err(SparseTrieError::BlindedNode { path, hash }) if path == Nibbles::from_nibbles([0x0]) && hash == B256::repeat_byte(1) ); } From df9ea9e93eb04734b157ec928b60dd76f6f9ff91 Mon Sep 17 00:00:00 2001 From: Roman Krasiuk Date: Tue, 22 Oct 2024 10:12:58 +0200 Subject: [PATCH 2/2] fix test --- crates/trie/sparse/src/trie.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/trie/sparse/src/trie.rs b/crates/trie/sparse/src/trie.rs index 5d31598fb428..42f90d34c8a7 100644 --- a/crates/trie/sparse/src/trie.rs +++ b/crates/trie/sparse/src/trie.rs @@ -1192,7 +1192,7 @@ mod tests { // Removing a blinded leaf should result in an error assert_matches!( - sparse.remove_leaf(Nibbles::from_nibbles([0x0])), + sparse.remove_leaf(&Nibbles::from_nibbles([0x0])), Err(SparseTrieError::BlindedNode { path, hash }) if path == Nibbles::from_nibbles([0x0]) && hash == B256::repeat_byte(1) ); }