From 129a23e1a9a2fd80b466ae37bd871d8c5d385715 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 10 Jul 2024 15:10:47 -0400 Subject: [PATCH 01/17] first pass implementing proof --- firewood/src/db.rs | 10 +- firewood/src/hashednode.rs | 31 ++++- firewood/src/merkle.rs | 112 +++++++++------ firewood/src/proof.rs | 279 +++++++++++++++++++++++++------------ firewood/src/v2/api.rs | 6 +- firewood/src/v2/emptydb.rs | 2 +- firewood/src/v2/propose.rs | 5 +- firewood/tests/db.rs | 138 +++++++++--------- 8 files changed, 369 insertions(+), 214 deletions(-) diff --git a/firewood/src/db.rs b/firewood/src/db.rs index f4a0cfbbc..077e13916 100644 --- a/firewood/src/db.rs +++ b/firewood/src/db.rs @@ -79,7 +79,7 @@ impl api::DbView for HistoricalRev { async fn single_key_proof( &self, _key: K, - ) -> Result>>, api::Error> { + ) -> Result, api::Error> { todo!() } @@ -124,14 +124,14 @@ impl HistoricalRev { todo!() } - pub fn prove(&self, _key: &[u8]) -> Result>, MerkleError> { + pub fn prove(&self, _key: &[u8]) -> Result { todo!() } /// Verifies a range proof is valid for a set of keys. - pub fn verify_range_proof + Send, V: AsRef<[u8]>>( + pub fn verify_range_proof>( &self, - _proof: Proof, + _proof: Proof, _first_key: &[u8], _last_key: &[u8], _keys: Vec<&[u8]>, @@ -177,7 +177,7 @@ impl api::DbView for Proposal { todo!() } - async fn single_key_proof(&self, _key: K) -> Result>>, api::Error> + async fn single_key_proof(&self, _key: K) -> Result, api::Error> where K: api::KeyType, { diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index 3b512fb44..f83f0fe63 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -18,6 +18,7 @@ const MAX_VARINT_SIZE: usize = 10; const BITS_PER_NIBBLE: u64 = 4; use crate::merkle::MerkleError; +use crate::proof::ProofNode; use storage::PathIterItem; /// A [HashedNodeStore] keeps track of nodes as they change when they are backed by a LinearStore. @@ -315,7 +316,7 @@ pub(super) enum ValueDigest<'a> { Value(&'a [u8]), /// The hash of a a node's value. /// TODO danlaine: Use this variant when implement ProofNode - _Hash(Box<[u8]>), + Hash(Box<[u8]>), } trait Hashable { @@ -438,6 +439,32 @@ impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { } } +impl Hashable for &ProofNode { + fn key(&self) -> impl Iterator + Clone { + self.key.as_ref().iter().copied() + } + + fn value_digest(&self) -> Option { + match self.value_digest.as_ref() { + None => None, + Some(value) if value.len() > 32 => { + unreachable!("TODO danlaine: prevent this from happening. The digest should never be more than 32 bytes.") + } + Some(value) if value.len() == 32 => { + Some(ValueDigest::Hash(value.to_vec().into_boxed_slice())) + } + Some(value) => Some(ValueDigest::Value(value)), + } + } + + fn children(&self) -> impl Iterator + Clone { + self.child_hashes + .iter() + .enumerate() + .filter_map(|(i, hash)| hash.as_ref().map(|h| (i, h))) + } +} + fn add_value_digest_to_buf(buf: &mut H, value_digest: Option) { let Some(value_digest) = value_digest else { let value_exists: u8 = 0; @@ -456,7 +483,7 @@ fn add_value_digest_to_buf(buf: &mut H, value_digest: Option { add_len_and_value_to_buf(buf, value); } - ValueDigest::_Hash(hash) => { + ValueDigest::Hash(hash) => { add_len_and_value_to_buf(buf, hash.as_ref()); } } diff --git a/firewood/src/merkle.rs b/firewood/src/merkle.rs index b283c75eb..ee4bef3d6 100644 --- a/firewood/src/merkle.rs +++ b/firewood/src/merkle.rs @@ -2,7 +2,7 @@ // See the file LICENSE.md for licensing terms. use crate::hashednode::HashedNodeStore; -use crate::proof::{Proof, ProofError}; +use crate::proof::{Proof, ProofError, ProofNode}; use crate::stream::{MerkleKeyValueStream, PathIterator}; use crate::v2::api; use futures::{StreamExt, TryStreamExt}; @@ -23,8 +23,8 @@ pub type Value = Vec; #[derive(Debug, Error)] pub enum MerkleError { - #[error("uninitialized")] - Uninitialized, + #[error("can't generate proof for empty node")] + Empty, #[error("read only")] ReadOnly, #[error("node not a branch node")] @@ -106,35 +106,44 @@ impl Merkle { self.0.root_hash() } - /// Constructs a merkle proof for key. The result contains all encoded nodes - /// on the path to the value at key. The value itself is also included in the - /// last node and can be retrieved by verifying the proof. - /// - /// If the trie does not contain a value for key, the returned proof contains - /// all nodes of the longest existing prefix of the key, ending with the node - /// that proves the absence of the key (at least the root node). - pub fn prove(&self, _key: &[u8]) -> Result>, MerkleError> { - todo!() - // let mut proofs = HashMap::new(); - // if root_addr.is_null() { - // return Ok(Proof(proofs)); - // } - - // let sentinel_node = self.get_node(root_addr)?; - - // let path_iter = self.path_iter(sentinel_node, key.as_ref()); - - // let nodes = path_iter - // .map(|result| result.map(|(_, node)| node)) - // .collect::, MerkleError>>()?; - - // // Get the hashes of the nodes. - // for node in nodes.into_iter() { - // let encoded = node.get_encoded(&self.store); - // let hash: [u8; TRIE_HASH_LEN] = sha3::Keccak256::digest(encoded).into(); - // proofs.insert(hash, encoded.to_vec()); - // } - // Ok(Proof(proofs)) + /// Returns a proof that the given key has a certain value, + /// or that the key isn't in the trie. + pub fn prove(&self, key: &[u8]) -> Result { + let Some(root_addr) = self.root_address() else { + return Err(MerkleError::Empty); + }; + + // Get the path to the key + let path_iter = self.path_iter(key)?; + let mut proof = Vec::new(); + for node in path_iter { + let node = node?; + proof.push(ProofNode::from(node)); + } + + if proof.is_empty() { + // No nodes, even the root, are before `key`. + // The root alone proves the non-existence of `key`. + let root = self.read_node(root_addr)?; + + // TODO reduce duplicate code with ProofNode::from + let mut child_hashes: [Option; BranchNode::MAX_CHILDREN] = Default::default(); + if let Some(branch) = root.as_branch() { + // TODO danlaine: can we avoid indexing? + #[allow(clippy::indexing_slicing)] + for (i, hash) in branch.children_iter() { + child_hashes[i] = Some(hash.clone()); + } + } + + proof.push(ProofNode { + key: root.partial_path().bytes(), + value_digest: root.value().map(|value| value.into()), + child_hashes, + }) + } + + Ok(Proof(proof.into_boxed_slice())) } pub fn get(&self, key: &[u8]) -> Result>, MerkleError> { @@ -161,17 +170,17 @@ impl Merkle { } } - pub fn verify_proof + Send>( + pub fn verify_proof( &self, _key: &[u8], - _proof: &Proof, + _proof: &Proof, ) -> Result>, MerkleError> { todo!() } - pub fn verify_range_proof + Send, V: AsRef<[u8]>>( + pub fn verify_range_proof>( &self, - _proof: &Proof, + _proof: &Proof, _first_key: &[u8], _last_key: &[u8], _keys: Vec<&[u8]>, @@ -1048,15 +1057,32 @@ mod tests { assert!(merkle.root_address().is_none()); } - // #[test] - // fn get_empty_proof() { - // let merkle = create_in_memory_merkle(); - // let root_addr = merkle.init_sentinel().unwrap(); + #[test] + fn get_empty_proof() { + let merkle = create_in_memory_merkle(); + let proof = merkle.prove(b"any-key"); + assert!(matches!(proof.unwrap_err(), MerkleError::Empty)); + } - // let proof = merkle.prove(b"any-key", root_addr).unwrap(); + #[test_case(vec![(&[],&[])] ; "leaf without partial path encoding with Bincode")] - // assert!(proof.0.is_empty()); - // } + fn single_key_proof(kvs: Vec<(&[u8], &[u8])>) { + let mut merkle = create_in_memory_merkle(); + + for (key, val) in &kvs { + merkle.insert(key, Box::from(*val)).unwrap(); + } + + let merkle = merkle.freeze().unwrap(); + + let root_hash = merkle.root_hash().unwrap(); + + for (key, value) in kvs { + let proof = merkle.prove(key).unwrap(); + + proof.verify(key, Some(value), &root_hash).unwrap(); + } + } // #[tokio::test] // async fn empty_range_proof() { diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index e3c4c9e84..33a035e0d 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -1,16 +1,13 @@ // Copyright (C) 2023, Ava Labs, Inc. All rights reserved. // See the file LICENSE.md for licensing terms. -use std::collections::HashMap; - -use crate::{merkle::Merkle, v2::api::HashKey}; +use crate::hashednode::{Preimage, ValueDigest}; +use crate::{db::DbError, merkle::MerkleError}; use nix::errno::Errno; -use sha2::Digest; -use storage::ReadLinearStore; +use sha2::{Digest, Sha256}; +use storage::{BranchNode, NibblesIterator, PathIterItem, TrieHash}; use thiserror::Error; -use crate::{db::DbError, merkle::MerkleError}; - #[derive(Debug, Error)] pub enum ProofError { #[error("decoding error")] @@ -27,6 +24,24 @@ pub enum ProofError { InvalidData, #[error("invalid proof")] InvalidProof, + #[error("unexpected hash")] + UnexpectedHash, + #[error("unexpected value")] + UnexpectedValue, + #[error("value mismatch")] + ValueMismatch, + #[error("expected value but got None")] + ExpectedValue, + #[error("proof can't be empty")] + Empty, + #[error("each proof node key should be a prefix of the proven key")] + ShouldBePrefixOfProvenKey, + #[error("each proof node key should be a prefix of the next key")] + ShouldBePrefixOfNextKey, + #[error("child index is out of bounds")] + ChildIndexOutOfBounds, + #[error("only nodes with even length key can have values")] + ValueAtOddNibbleLength, #[error("invalid edge keys")] InvalidEdgeKeys, #[error("node insertion error")] @@ -65,107 +80,197 @@ impl From for ProofError { } } -/// A proof that a single key is present -/// -/// The generic N represents the storage for the node #[derive(Clone, Debug)] -pub struct Proof(pub HashMap); +pub struct ProofNode { + /// The key this node is at. Each byte is a nibble. + pub key: Box<[u8]>, + /// None if the node has no value. + /// The value associated with `key` if the value is < 32 bytes. + /// The hash of the value if the node's value is >= 32 bytes. + /// TODO danlaine: Can we enforce that this is at most 32 bytes? + pub value_digest: Option>, + /// The hash of each child, or None if the child does not exist. + pub child_hashes: [Option; BranchNode::MAX_CHILDREN], +} -/// `SubProof` contains the value or the hash of a node that maps -/// to a single proof step. If reaches an end step during proof verification, -/// the `SubProof` should be the `Value` variant. +impl From for ProofNode { + fn from(item: PathIterItem) -> Self { + let mut child_hashes: [Option; BranchNode::MAX_CHILDREN] = Default::default(); -#[derive(Debug)] -#[allow(dead_code)] // TODO use or remove this type -enum SubProof { - Value(Vec), - Hash(HashKey), -} + if let Some(branch) = item.node.as_branch() { + // TODO danlaine: can we avoid indexing? + #[allow(clippy::indexing_slicing)] + for (i, hash) in branch.children_iter() { + child_hashes[i] = Some(hash.clone()); + } + } -impl + Send> Proof { - /// verify_proof checks merkle proofs. The given proof must contain the value for - /// key in a trie with the given root hash. VerifyProof returns an error if the - /// proof contains invalid trie nodes or the wrong value. - /// - /// The generic N represents the storage for the node - pub fn verify>( - &self, - _key: K, - _root_hash: HashKey, - ) -> Result>, ProofError> { - todo!() + Self { + key: item.key_nibbles, + value_digest: item.node.value().map(|value| value.into()), + child_hashes, + } } +} - pub fn extend(&mut self, other: Proof) { - self.0.extend(other.0) +impl From<&ProofNode> for TrieHash { + fn from(node: &ProofNode) -> Self { + node.to_hash() } +} - pub fn verify_range_proof( +/// A proof that a given key-value pair either exists or does not exist in a trie. +#[derive(Clone, Debug)] +pub struct Proof(pub Box<[ProofNode]>); + +impl Proof { + pub fn verify, V: AsRef<[u8]>>( &self, - _root_hash: HashKey, - _first_key: K, - _last_key: K, - keys: Vec, - vals: Vec, - ) -> Result - where - K: AsRef<[u8]>, - V: AsRef<[u8]>, - { - if keys.len() != vals.len() { - return Err(ProofError::InconsistentProofData); - } + key: K, + expected_value: Option, + root_hash: &TrieHash, + ) -> Result<(), ProofError> { + match self.value_digest(key, root_hash)? { + None => { + // This proof proves that `key` maps to None. + if expected_value.is_some() { + return Err(ProofError::ExpectedValue); + } + } + Some(ValueDigest::Value(got_value)) => { + // This proof proves that `key` maps to `got_value`. + let Some(expected_value) = expected_value else { + // We were expecting `key` to map to None. + return Err(ProofError::UnexpectedValue); + }; - // Ensure the received batch is monotonic increasing and contains no deletions - #[allow(clippy::indexing_slicing)] - if !keys.windows(2).all(|w| w[0].as_ref() < w[1].as_ref()) { - return Err(ProofError::NonMonotonicIncreaseRange); - } + if got_value != expected_value.as_ref() { + // `key` maps to an unexpected value. + return Err(ProofError::ValueMismatch); + } + } + Some(ValueDigest::Hash(got_hash)) => { + // This proof proves that `key` maps to a value + // whose hash is `got_hash`. + let Some(expected_value) = expected_value else { + // We were expecting `key` to map to None. + return Err(ProofError::UnexpectedValue); + }; - // create an empty merkle trie in memory - todo!(); + let value_hash = Sha256::digest(expected_value.as_ref()); + if got_hash.as_ref() != value_hash.as_slice() { + // `key` maps to an unexpected value. + return Err(ProofError::ValueMismatch); + } + } + } + Ok(()) } - /// proofToPath converts a merkle proof to trie node path. The main purpose of - /// this function is recovering a node path from the merkle proof stream. All - /// necessary nodes will be resolved and leave the remaining as hashnode. - /// - /// The given edge proof is allowed to be an existent or non-existent proof. - fn _proof_to_path( + /// Returns the value digest associated with the given `key` in the trie revision + /// with the given `root_hash`. If the key does not exist in the trie, returns `None`. + /// Returns an error if the proof is invalid or doesn't prove the key for the + /// given revision. + fn value_digest>( &self, - _key: K, - _root_hash: HashKey, - _in_mem_merkle: &mut Merkle, - _allow_non_existent_node: bool, - ) -> Result>, ProofError> - where - K: AsRef<[u8]>, - { + key: K, + root_hash: &TrieHash, + ) -> Result, ProofError> { + let key: Vec = NibblesIterator::new(key.as_ref()).collect(); + + let Some(last_node) = self.0.last() else { + return Err(ProofError::Empty); + }; + + let mut expected_hash = root_hash; + + // TODO danlaine: Is there a better way to do this loop? + for i in 0..self.0.len() { + #[allow(clippy::indexing_slicing)] + let node = &self.0[i]; + + if node.to_hash() != *expected_hash { + return Err(ProofError::UnexpectedHash); + } + + // Assert that only nodes whose keys are an even number of nibbles + // have a `value_digest`. + if node.key.len() % 2 != 0 && node.value_digest.is_some() { + return Err(ProofError::ValueAtOddNibbleLength); + } + + if i != self.0.len() - 1 { + // Assert that every node's key is a prefix of the proven key, + // with the exception of the last node, which is a suffix of the + // proven key in exclusion proofs. + let next_nibble = next_nibble(&mut node.key.iter(), &mut key.iter())?; + + let Some(next_nibble) = next_nibble else { + return Err(ProofError::ShouldBePrefixOfProvenKey); + }; + + expected_hash = node + .child_hashes + .get(next_nibble as usize) + .ok_or(ProofError::ChildIndexOutOfBounds)? + .as_ref() + .ok_or(ProofError::NodeNotInTrie)?; + + // Assert that each node's key is a prefix of the next node's key. + #[allow(clippy::indexing_slicing)] + let next_node_key = &self.0[i + 1].key; + if !is_prefix(&mut node.key.iter(), &mut next_node_key.iter()) { + return Err(ProofError::ShouldBePrefixOfNextKey); + } + } + } + + if last_node.key.len() == key.len() { + // This is an inclusion proof. + return Ok(last_node.value_digest.as_ref().map(|value| { + if value.len() < 32 { + ValueDigest::Value(value) + } else { + // TODO danlaine: I think we can remove this copy by not + // requiring Hash to own its data. + ValueDigest::Hash(value.to_vec().into_boxed_slice()) + } + })); + } + todo!() } } -fn _generate_subproof_hash(encoded: &[u8]) -> Result { - match encoded.len() { - 0..=31 => { - let sub_hash = sha2::Sha256::digest(encoded).into(); - Ok(sub_hash) +/// Returns the next nibble in `c` after `b`. +/// Returns an error if `b` is not a prefix of `c`. +fn next_nibble<'a, I>(b: &mut I, c: &mut I) -> Result, ProofError> +where + I: Iterator, +{ + // Check if b is a prefix of c + for b_item in b { + match c.next() { + Some(c_item) if b_item == c_item => continue, + _ => return Err(ProofError::ShouldBePrefixOfNextKey), } + } - 32 => { - let sub_hash = encoded - .try_into() - .expect("slice length checked in match arm"); + // If a is a prefix, return the first element in c after b + Ok(c.next().copied()) +} - Ok(sub_hash) +fn is_prefix<'a, I>(b: &mut I, c: &mut I) -> bool +where + I: Iterator, +{ + for b_item in b { + let Some(c_item) = c.next() else { + return false; + }; + if b_item != c_item { + return false; } - - len => Err(ProofError::DecodeError(Box::new( - bincode::ErrorKind::Custom(format!("invalid proof length: {len}")), - ))), } -} - -fn _generate_subproof(encoded: &[u8]) -> Result { - Ok(SubProof::Hash(_generate_subproof_hash(encoded)?)) + true } diff --git a/firewood/src/v2/api.rs b/firewood/src/v2/api.rs index 1cf561d7f..9f074445c 100644 --- a/firewood/src/v2/api.rs +++ b/firewood/src/v2/api.rs @@ -104,8 +104,8 @@ impl From for Error { /// and a vector of all key/value pairs #[derive(Debug)] pub struct RangeProof { - pub first_key_proof: Proof>, - pub last_key_proof: Proof>, + pub first_key_proof: Proof, + pub last_key_proof: Proof, pub middle: Vec<(K, V)>, } @@ -166,7 +166,7 @@ pub trait DbView { async fn val(&self, key: K) -> Result>, Error>; /// Obtain a proof for a single key - async fn single_key_proof(&self, key: K) -> Result>>, Error>; + async fn single_key_proof(&self, key: K) -> Result, Error>; /// Obtain a range proof over a set of keys /// diff --git a/firewood/src/v2/emptydb.rs b/firewood/src/v2/emptydb.rs index a791fd1f1..26b2c0956 100644 --- a/firewood/src/v2/emptydb.rs +++ b/firewood/src/v2/emptydb.rs @@ -67,7 +67,7 @@ impl DbView for HistoricalImpl { Ok(None) } - async fn single_key_proof(&self, _key: K) -> Result>>, Error> { + async fn single_key_proof(&self, _key: K) -> Result, Error> { Ok(None) } diff --git a/firewood/src/v2/propose.rs b/firewood/src/v2/propose.rs index 13a152847..e33329a89 100644 --- a/firewood/src/v2/propose.rs +++ b/firewood/src/v2/propose.rs @@ -125,10 +125,7 @@ impl api::DbView for Proposal { } } - async fn single_key_proof( - &self, - _key: K, - ) -> Result>>, api::Error> { + async fn single_key_proof(&self, _key: K) -> Result, api::Error> { todo!(); } diff --git a/firewood/tests/db.rs b/firewood/tests/db.rs index 7eded4729..90daa250e 100644 --- a/firewood/tests/db.rs +++ b/firewood/tests/db.rs @@ -7,7 +7,7 @@ use firewood::{ }; use tokio::task::block_in_place; -use std::{collections::VecDeque, env::temp_dir, path::PathBuf}; +use std::collections::VecDeque; mod common; use common::TestDbCreator; @@ -133,74 +133,74 @@ async fn test_revisions() { } } -#[ignore = "unimplemented"] -#[tokio::test(flavor = "multi_thread")] -#[allow(clippy::unwrap_used)] -async fn create_db_issue_proof() { - let cfg = DbConfig::builder(); - - let mut tmpdir: PathBuf = std::env::var_os("CARGO_TARGET_DIR") - .unwrap_or(temp_dir().into()) - .into(); - tmpdir.push("/tmp/test_db_proof"); - - let db = firewood::db::Db::new(tmpdir, cfg.truncate(true).build()) - .await - .unwrap(); - - let items = vec![ - ("d", "verb"), - ("do", "verb"), - ("doe", "reindeer"), - ("e", "coin"), - ]; - - let mut batch = Vec::new(); - for (k, v) in items { - let write = BatchOp::Put { - key: k.as_bytes(), - value: v.as_bytes().to_vec(), - }; - batch.push(write); - } - let proposal = db.propose(batch).await.unwrap(); - proposal.commit().await.unwrap(); - - let root_hash = db.root_hash().await.unwrap(); - - // Add second commit - let mut batch = Vec::new(); - for (k, v) in Vec::from([("x", "two")]).iter() { - let write = BatchOp::Put { - key: k.to_string().as_bytes().to_vec(), - value: v.as_bytes().to_vec(), - }; - batch.push(write); - } - let proposal = db.propose(batch).await.unwrap(); - proposal.commit().await.unwrap(); - - let rev = db.revision(root_hash).await.unwrap(); - let key = "doe".as_bytes(); - let root_hash = rev.root_hash().await.unwrap(); - - match rev.single_key_proof(key).await { - Ok(proof) => { - let verification = proof.unwrap().verify(key, root_hash).unwrap(); - assert!(verification.is_some()); - } - Err(e) => { - panic!("Error: {}", e); - } - } - - let missing_key = "dog".as_bytes(); - // The proof for the missing key will return the path to the missing key - if let Err(e) = rev.single_key_proof(missing_key).await { - println!("Error: {}", e); - // TODO do type assertion on error - } -} +// #[ignore = "unimplemented"] +// #[tokio::test(flavor = "multi_thread")] +// #[allow(clippy::unwrap_used)] +// async fn create_db_issue_proof() { +// let cfg = DbConfig::builder(); + +// let mut tmpdir: PathBuf = std::env::var_os("CARGO_TARGET_DIR") +// .unwrap_or(temp_dir().into()) +// .into(); +// tmpdir.push("/tmp/test_db_proof"); + +// let db = firewood::db::Db::new(tmpdir, cfg.truncate(true).build()) +// .await +// .unwrap(); + +// let items = vec![ +// ("d", "verb"), +// ("do", "verb"), +// ("doe", "reindeer"), +// ("e", "coin"), +// ]; + +// let mut batch = Vec::new(); +// for (k, v) in items { +// let write = BatchOp::Put { +// key: k.as_bytes(), +// value: v.as_bytes().to_vec(), +// }; +// batch.push(write); +// } +// let proposal = db.propose(batch).await.unwrap(); +// proposal.commit().await.unwrap(); + +// let root_hash = db.root_hash().await.unwrap(); + +// // Add second commit +// let mut batch = Vec::new(); +// for (k, v) in Vec::from([("x", "two")]).iter() { +// let write = BatchOp::Put { +// key: k.to_string().as_bytes().to_vec(), +// value: v.as_bytes().to_vec(), +// }; +// batch.push(write); +// } +// let proposal = db.propose(batch).await.unwrap(); +// proposal.commit().await.unwrap(); + +// let rev = db.revision(root_hash).await.unwrap(); +// let key = "doe".as_bytes(); +// let root_hash = rev.root_hash().await.unwrap(); + +// match rev.single_key_proof(key).await { +// Ok(proof) => { +// let verification = proof.unwrap().verify(key, root_hash).unwrap(); +// assert!(verification.is_some()); +// } +// Err(e) => { +// panic!("Error: {}", e); +// } +// } + +// let missing_key = "dog".as_bytes(); +// // The proof for the missing key will return the path to the missing key +// if let Err(e) = rev.single_key_proof(missing_key).await { +// println!("Error: {}", e); +// // TODO do type assertion on error +// } +// } macro_rules! assert_val { ($rev: ident, $key:literal, $expected_val:literal) => { From bdfaba1ae7860d84de81388db01ae28f6d684516 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 10 Jul 2024 15:16:40 -0400 Subject: [PATCH 02/17] appease clippy; remove unused error types --- firewood/src/db.rs | 2 -- firewood/src/merkle.rs | 2 +- firewood/src/proof.rs | 49 +++--------------------------------------- firewood/src/v2/db.rs | 4 +--- 4 files changed, 5 insertions(+), 52 deletions(-) diff --git a/firewood/src/db.rs b/firewood/src/db.rs index 077e13916..7fb78b735 100644 --- a/firewood/src/db.rs +++ b/firewood/src/db.rs @@ -30,7 +30,6 @@ pub enum DbError { InvalidParams, Merkle(MerkleError), System(nix::Error), - KeyNotFound, CreateError, IO(std::io::Error), InvalidProposal, @@ -42,7 +41,6 @@ impl fmt::Display for DbError { DbError::InvalidParams => write!(f, "invalid parameters provided"), DbError::Merkle(e) => write!(f, "merkle error: {e:?}"), DbError::System(e) => write!(f, "system error: {e:?}"), - DbError::KeyNotFound => write!(f, "not found"), DbError::CreateError => write!(f, "database create error"), DbError::IO(e) => write!(f, "I/O error: {e:?}"), DbError::InvalidProposal => write!(f, "invalid proposal"), diff --git a/firewood/src/merkle.rs b/firewood/src/merkle.rs index ee4bef3d6..ce596dd91 100644 --- a/firewood/src/merkle.rs +++ b/firewood/src/merkle.rs @@ -1080,7 +1080,7 @@ mod tests { for (key, value) in kvs { let proof = merkle.prove(key).unwrap(); - proof.verify(key, Some(value), &root_hash).unwrap(); + proof.verify(key, Some(value), root_hash).unwrap(); } } diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index 33a035e0d..1989ace31 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -2,28 +2,15 @@ // See the file LICENSE.md for licensing terms. use crate::hashednode::{Preimage, ValueDigest}; -use crate::{db::DbError, merkle::MerkleError}; -use nix::errno::Errno; +use crate::merkle::MerkleError; use sha2::{Digest, Sha256}; use storage::{BranchNode, NibblesIterator, PathIterItem, TrieHash}; use thiserror::Error; #[derive(Debug, Error)] pub enum ProofError { - #[error("decoding error")] - DecodeError(#[from] bincode::Error), - #[error("no such node")] - NoSuchNode, - #[error("proof node missing")] - ProofNodeMissing, - #[error("inconsistent proof data")] - InconsistentProofData, #[error("non-monotonic range increase")] NonMonotonicIncreaseRange, - #[error("invalid data")] - InvalidData, - #[error("invalid proof")] - InvalidProof, #[error("unexpected hash")] UnexpectedHash, #[error("unexpected value")] @@ -42,42 +29,12 @@ pub enum ProofError { ChildIndexOutOfBounds, #[error("only nodes with even length key can have values")] ValueAtOddNibbleLength, - #[error("invalid edge keys")] - InvalidEdgeKeys, - #[error("node insertion error")] - NodesInsertionError, #[error("node not in trie")] NodeNotInTrie, - #[error("invalid node {0:?}")] - InvalidNode(#[from] MerkleError), + #[error("{0:?}")] + Merkle(#[from] MerkleError), #[error("empty range")] EmptyRange, - #[error("fork left")] - ForkLeft, - #[error("fork right")] - ForkRight, - #[error("system error: {0:?}")] - SystemError(Errno), - #[error("invalid root hash")] - InvalidRootHash, -} - -impl From for ProofError { - fn from(d: DbError) -> ProofError { - match d { - DbError::InvalidParams => ProofError::InvalidProof, - DbError::Merkle(e) => ProofError::InvalidNode(e), - DbError::System(e) => ProofError::SystemError(e), - DbError::KeyNotFound => ProofError::InvalidEdgeKeys, - DbError::CreateError => ProofError::NoSuchNode, - // TODO: fix better by adding a new error to ProofError - #[allow(clippy::unwrap_used)] - DbError::IO(e) => { - ProofError::SystemError(nix::errno::Errno::from_raw(e.raw_os_error().unwrap())) - } - DbError::InvalidProposal => ProofError::InvalidProof, - } - } } #[derive(Clone, Debug)] diff --git a/firewood/src/v2/db.rs b/firewood/src/v2/db.rs index d27bb88eb..cff28aa85 100644 --- a/firewood/src/v2/db.rs +++ b/firewood/src/v2/db.rs @@ -20,9 +20,7 @@ impl From for api::Error { DbError::InvalidParams => api::Error::InternalError(Box::new(value)), DbError::Merkle(e) => api::Error::InternalError(Box::new(e)), DbError::System(e) => api::Error::IO(e.into()), - DbError::KeyNotFound | DbError::CreateError => { - api::Error::InternalError(Box::new(value)) - } + DbError::CreateError => api::Error::InternalError(Box::new(value)), DbError::IO(e) => api::Error::IO(e), DbError::InvalidProposal => api::Error::InvalidProposal, } From 9f124d6e8d9931042bf912b0d99f27639c7541d8 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 10 Jul 2024 16:10:20 -0400 Subject: [PATCH 03/17] add randomized test; fix value digest creation bug --- firewood/src/merkle.rs | 41 ++++++++++++++++++++++++++++++++++++----- firewood/src/proof.rs | 14 ++++++++++++-- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/firewood/src/merkle.rs b/firewood/src/merkle.rs index ce596dd91..ae613e5a2 100644 --- a/firewood/src/merkle.rs +++ b/firewood/src/merkle.rs @@ -805,10 +805,33 @@ impl<'a, T: PartialEq> PrefixOverlap<'a, T> { #[cfg(test)] #[allow(clippy::indexing_slicing, clippy::unwrap_used)] mod tests { + use std::time::{SystemTime, UNIX_EPOCH}; + use super::*; + use rand::{rngs::StdRng, Rng, SeedableRng}; use storage::MemStore; use test_case::test_case; + // Returns n random key-value pairs. + fn generate_random_kvs(seed: u64, n: usize) -> Vec<(Vec, Vec)> { + println!("Used seed: {}", seed); + + let mut rng = StdRng::seed_from_u64(seed); + + let mut kvs: Vec<(Vec, Vec)> = Vec::new(); + for _ in 0..n { + let key_len = rng.gen_range(1..=4096); + let key: Vec = (0..key_len).map(|_| rng.gen()).collect(); + + let val_len = rng.gen_range(1..=4096); + let val: Vec = (0..val_len).map(|_| rng.gen()).collect(); + + kvs.push((key, val)); + } + + kvs + } + #[test] fn test_get_regression() { let mut merkle = create_in_memory_merkle(); @@ -1064,13 +1087,21 @@ mod tests { assert!(matches!(proof.unwrap_err(), MerkleError::Empty)); } - #[test_case(vec![(&[],&[])] ; "leaf without partial path encoding with Bincode")] - - fn single_key_proof(kvs: Vec<(&[u8], &[u8])>) { + #[test] + fn single_key_proof() { let mut merkle = create_in_memory_merkle(); + let seed = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_secs(); + + const TEST_SIZE: usize = 1; + + let kvs = generate_random_kvs(seed, TEST_SIZE); + for (key, val) in &kvs { - merkle.insert(key, Box::from(*val)).unwrap(); + merkle.insert(key, val.clone().into_boxed_slice()).unwrap(); } let merkle = merkle.freeze().unwrap(); @@ -1078,7 +1109,7 @@ mod tests { let root_hash = merkle.root_hash().unwrap(); for (key, value) in kvs { - let proof = merkle.prove(key).unwrap(); + let proof = merkle.prove(&key).unwrap(); proof.verify(key, Some(value), root_hash).unwrap(); } diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index 1989ace31..1822a47ac 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -64,7 +64,16 @@ impl From for ProofNode { Self { key: item.key_nibbles, - value_digest: item.node.value().map(|value| value.into()), + value_digest: item.node.value().map(|value| { + if value.len() < 32 { + value.to_vec().into_boxed_slice() + } else { + // TODO danlaine: I think we can remove this copy by not + // requiring Hash to own its data. + let hash = Sha256::digest(value); + hash.as_slice().to_vec().into_boxed_slice() + } + }), child_hashes, } } @@ -195,7 +204,8 @@ impl Proof { })); } - todo!() + // This is an exclusion proof. + Ok(None) } } From 249a1ca16dc61397495ba175d5d5250f2aeeed7a Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 10 Jul 2024 16:21:22 -0400 Subject: [PATCH 04/17] adhere to value_digest invariant --- firewood/src/merkle.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/firewood/src/merkle.rs b/firewood/src/merkle.rs index ae613e5a2..5230cc50d 100644 --- a/firewood/src/merkle.rs +++ b/firewood/src/merkle.rs @@ -6,6 +6,7 @@ use crate::proof::{Proof, ProofError, ProofNode}; use crate::stream::{MerkleKeyValueStream, PathIterator}; use crate::v2::api; use futures::{StreamExt, TryStreamExt}; +use sha2::{Digest, Sha256}; use std::collections::HashSet; use std::future::ready; use std::io::Write; @@ -138,7 +139,16 @@ impl Merkle { proof.push(ProofNode { key: root.partial_path().bytes(), - value_digest: root.value().map(|value| value.into()), + value_digest: root.value().map(|value| { + if value.len() < 32 { + value.to_vec().into_boxed_slice() + } else { + // TODO danlaine: I think we can remove this copy by not + // requiring Hash to own its data. + let hash = Sha256::digest(value); + hash.as_slice().to_vec().into_boxed_slice() + } + }), child_hashes, }) } From 0583257c593f62de0c7270dd485062518ecc6055 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 10 Jul 2024 16:22:09 -0400 Subject: [PATCH 05/17] remove todo --- firewood/src/hashednode.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index f83f0fe63..c7eaa6ba9 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -315,7 +315,6 @@ pub(super) enum ValueDigest<'a> { /// A node's value. Value(&'a [u8]), /// The hash of a a node's value. - /// TODO danlaine: Use this variant when implement ProofNode Hash(Box<[u8]>), } From c12a70f95c0a024360197c21765a833a460526e6 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 10 Jul 2024 17:27:04 -0400 Subject: [PATCH 06/17] genericize ValueDigest --- firewood/src/hashednode.rs | 130 ++++++++++++++++++++----------------- firewood/src/proof.rs | 14 ++-- 2 files changed, 77 insertions(+), 67 deletions(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index c7eaa6ba9..77c3833c1 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -278,18 +278,22 @@ pub fn hash_preimage(node: &Node, path_prefix: &Path) -> Box<[u8]> { let mut buf = Vec::with_capacity(est_len); match node { Node::Branch(node) => { - NodeAndPrefix { - node: node.as_ref(), - prefix: path_prefix, - } - .write(&mut buf); + write_hash_preimage( + NodeAndPrefix { + node: node.as_ref(), + prefix: path_prefix, + }, + &mut buf, + ); } Node::Leaf(node) => { - NodeAndPrefix { - node, - prefix: path_prefix, - } - .write(&mut buf); + write_hash_preimage( + NodeAndPrefix { + node, + prefix: path_prefix, + }, + &mut buf, + ); } } buf.into_boxed_slice() @@ -311,67 +315,70 @@ impl HasUpdate for Vec { } } -pub(super) enum ValueDigest<'a> { +pub(super) enum ValueDigest +where + V: AsRef<[u8]>, + H: AsRef<[u8]>, +{ /// A node's value. - Value(&'a [u8]), + Value(V), /// The hash of a a node's value. - Hash(Box<[u8]>), + Hash(H), } -trait Hashable { +pub(crate) trait Hashable +where + V: AsRef<[u8]>, + H: AsRef<[u8]>, +{ /// The key of the node where each byte is a nibble. fn key(&self) -> impl Iterator + Clone; /// The node's value or hash. - fn value_digest(&self) -> Option; + fn value_digest(&self) -> Option>; /// Each element is a child's index and hash. /// Yields 0 elements if the node is a leaf. fn children(&self) -> impl Iterator + Clone; } -pub(super) trait Preimage { - /// Returns the hash of this preimage. - fn to_hash(self) -> TrieHash; - /// Write this hash preimage to `buf`. - fn write(self, buf: &mut impl HasUpdate); +pub(crate) fn hash(preimage: impl Hashable) -> TrieHash +where + V: AsRef<[u8]>, + H: AsRef<[u8]>, +{ + let mut hasher = Sha256::new(); + write_hash_preimage(preimage, &mut hasher); + hasher.finalize().into() } -// Implement Preimage for all types that implement Hashable -impl Preimage for T +fn write_hash_preimage(preimage: impl Hashable, buf: &mut impl HasUpdate) where - T: Hashable, + V: AsRef<[u8]>, + H: AsRef<[u8]>, { - fn to_hash(self) -> TrieHash { - let mut hasher = Sha256::new(); - self.write(&mut hasher); - hasher.finalize().into() - } + let children = preimage.children(); - fn write(self, buf: &mut impl HasUpdate) { - let children = self.children(); + let num_children = children.clone().count() as u64; + add_varint_to_buf(buf, num_children); - let num_children = children.clone().count() as u64; - add_varint_to_buf(buf, num_children); - - for (index, hash) in children { - add_varint_to_buf(buf, index as u64); - buf.update(hash); - } + for (index, hash) in children { + add_varint_to_buf(buf, index as u64); + buf.update(hash); + } - // Add value digest (if any) to hash pre-image - add_value_digest_to_buf(buf, self.value_digest()); + // Add value digest (if any) to hash pre-image + add_value_digest_to_buf(buf, preimage.value_digest()); - // Add key length (in bits) to hash pre-image - let mut key = self.key(); - // let mut key = key.as_ref().iter(); - let key_bit_len = BITS_PER_NIBBLE * key.clone().count() as u64; - add_varint_to_buf(buf, key_bit_len); + // Add key length (in bits) to hash pre-image + let mut key = preimage.key(); + // let mut key = key.as_ref().iter(); + let key_bit_len = BITS_PER_NIBBLE * key.clone().count() as u64; + add_varint_to_buf(buf, key_bit_len); - // Add key to hash pre-image - while let Some(high_nibble) = key.next() { - let low_nibble = key.next().unwrap_or(0); - let byte = (high_nibble << 4) | low_nibble; - buf.update([byte]); - } + // Add key to hash pre-image + while let Some(high_nibble) = key.next() { + let low_nibble = key.next().unwrap_or(0); + let byte = (high_nibble << 4) | low_nibble; + buf.update([byte]); } } @@ -416,11 +423,11 @@ struct NodeAndPrefix<'a, N: HashableNode> { impl<'a, N: HashableNode> From> for TrieHash { fn from(node: NodeAndPrefix<'a, N>) -> Self { - node.to_hash() + hash(node) } } -impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { +impl<'a, N: HashableNode> Hashable<&'a [u8], Box<[u8]>> for NodeAndPrefix<'a, N> { fn key(&self) -> impl Iterator + Clone { self.prefix .0 @@ -429,7 +436,7 @@ impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { .chain(self.node.partial_path()) } - fn value_digest(&self) -> Option { + fn value_digest(&self) -> Option>> { self.node.value().map(ValueDigest::Value) } @@ -438,12 +445,12 @@ impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { } } -impl Hashable for &ProofNode { +impl<'a> Hashable<&'a [u8], Box<[u8]>> for &'a ProofNode { fn key(&self) -> impl Iterator + Clone { self.key.as_ref().iter().copied() } - fn value_digest(&self) -> Option { + fn value_digest(&self) -> Option>> { match self.value_digest.as_ref() { None => None, Some(value) if value.len() > 32 => { @@ -464,7 +471,10 @@ impl Hashable for &ProofNode { } } -fn add_value_digest_to_buf(buf: &mut H, value_digest: Option) { +fn add_value_digest_to_buf, H: AsRef<[u8]>>( + buf: &mut U, + value_digest: Option>, +) { let Some(value_digest) = value_digest else { let value_exists: u8 = 0; buf.update([value_exists]); @@ -477,21 +487,21 @@ fn add_value_digest_to_buf(buf: &mut H, value_digest: Option= 32 => { let hash = Sha256::digest(value); - add_len_and_value_to_buf(buf, hash.as_ref()); + add_len_and_value_to_buf(buf, hash); } ValueDigest::Value(value) => { add_len_and_value_to_buf(buf, value); } ValueDigest::Hash(hash) => { - add_len_and_value_to_buf(buf, hash.as_ref()); + add_len_and_value_to_buf(buf, hash); } } } #[inline] /// Writes the length of `value` and `value` to `buf`. -fn add_len_and_value_to_buf(buf: &mut H, value: &[u8]) { - let value_len = value.len(); +fn add_len_and_value_to_buf>(buf: &mut H, value: V) { + let value_len = value.as_ref().len(); buf.update([value_len as u8]); buf.update(value); } diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index 1822a47ac..ada83ecb7 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -1,7 +1,7 @@ // Copyright (C) 2023, Ava Labs, Inc. All rights reserved. // See the file LICENSE.md for licensing terms. -use crate::hashednode::{Preimage, ValueDigest}; +use crate::hashednode::{hash, ValueDigest}; use crate::merkle::MerkleError; use sha2::{Digest, Sha256}; use storage::{BranchNode, NibblesIterator, PathIterItem, TrieHash}; @@ -81,7 +81,7 @@ impl From for ProofNode { impl From<&ProofNode> for TrieHash { fn from(node: &ProofNode) -> Self { - node.to_hash() + hash(node) } } @@ -137,11 +137,11 @@ impl Proof { /// with the given `root_hash`. If the key does not exist in the trie, returns `None`. /// Returns an error if the proof is invalid or doesn't prove the key for the /// given revision. - fn value_digest>( - &self, + fn value_digest<'a, K: AsRef<[u8]>>( + &'a self, key: K, root_hash: &TrieHash, - ) -> Result, ProofError> { + ) -> Result>>, ProofError> { let key: Vec = NibblesIterator::new(key.as_ref()).collect(); let Some(last_node) = self.0.last() else { @@ -155,7 +155,7 @@ impl Proof { #[allow(clippy::indexing_slicing)] let node = &self.0[i]; - if node.to_hash() != *expected_hash { + if hash(node) != *expected_hash { return Err(ProofError::UnexpectedHash); } @@ -195,7 +195,7 @@ impl Proof { // This is an inclusion proof. return Ok(last_node.value_digest.as_ref().map(|value| { if value.len() < 32 { - ValueDigest::Value(value) + ValueDigest::Value(value.as_ref()) } else { // TODO danlaine: I think we can remove this copy by not // requiring Hash to own its data. From beb704eee8807e1670ba46c6d6ead60a1660b995 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 11 Jul 2024 10:12:36 -0400 Subject: [PATCH 07/17] refactor value digest --- firewood/src/hashednode.rs | 45 +++++++++------------- firewood/src/merkle.rs | 16 ++------ firewood/src/proof.rs | 78 +++++++++++++++----------------------- 3 files changed, 52 insertions(+), 87 deletions(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index 77c3833c1..2c3e6fbad 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -315,22 +315,17 @@ impl HasUpdate for Vec { } } -pub(super) enum ValueDigest -where - V: AsRef<[u8]>, - H: AsRef<[u8]>, -{ - /// A node's value. +#[derive(Clone, Debug)] +/// A ValueDigest is either a node's value or the hash of its value. +pub enum ValueDigest { Value(V), - /// The hash of a a node's value. - Hash(H), + /// TODO this variant will be used when we deserialize a proof node + /// from a remote Firewood instance. The serialized proof node they + /// send us may the hash of the value, not the value itself. + _Hash(H), } -pub(crate) trait Hashable -where - V: AsRef<[u8]>, - H: AsRef<[u8]>, -{ +pub(crate) trait Hashable { /// The key of the node where each byte is a nibble. fn key(&self) -> impl Iterator + Clone; /// The node's value or hash. @@ -427,7 +422,7 @@ impl<'a, N: HashableNode> From> for TrieHash { } } -impl<'a, N: HashableNode> Hashable<&'a [u8], Box<[u8]>> for NodeAndPrefix<'a, N> { +impl<'a, N: HashableNode> Hashable<&'a [u8], &'a [u8]> for NodeAndPrefix<'a, N> { fn key(&self) -> impl Iterator + Clone { self.prefix .0 @@ -436,7 +431,7 @@ impl<'a, N: HashableNode> Hashable<&'a [u8], Box<[u8]>> for NodeAndPrefix<'a, N> .chain(self.node.partial_path()) } - fn value_digest(&self) -> Option>> { + fn value_digest(&self) -> Option> { self.node.value().map(ValueDigest::Value) } @@ -445,22 +440,16 @@ impl<'a, N: HashableNode> Hashable<&'a [u8], Box<[u8]>> for NodeAndPrefix<'a, N> } } -impl<'a> Hashable<&'a [u8], Box<[u8]>> for &'a ProofNode { +impl<'a> Hashable<&'a [u8], &'a [u8]> for &'a ProofNode { fn key(&self) -> impl Iterator + Clone { self.key.as_ref().iter().copied() } - fn value_digest(&self) -> Option>> { - match self.value_digest.as_ref() { - None => None, - Some(value) if value.len() > 32 => { - unreachable!("TODO danlaine: prevent this from happening. The digest should never be more than 32 bytes.") - } - Some(value) if value.len() == 32 => { - Some(ValueDigest::Hash(value.to_vec().into_boxed_slice())) - } - Some(value) => Some(ValueDigest::Value(value)), - } + fn value_digest(&self) -> Option> { + self.value_digest.as_ref().map(|vd| match vd { + ValueDigest::Value(v) => ValueDigest::Value(v.as_ref()), + ValueDigest::_Hash(h) => ValueDigest::_Hash(h.as_ref()), + }) } fn children(&self) -> impl Iterator + Clone { @@ -492,7 +481,7 @@ fn add_value_digest_to_buf, H: AsRef<[u8]>>( ValueDigest::Value(value) => { add_len_and_value_to_buf(buf, value); } - ValueDigest::Hash(hash) => { + ValueDigest::_Hash(hash) => { add_len_and_value_to_buf(buf, hash); } } diff --git a/firewood/src/merkle.rs b/firewood/src/merkle.rs index 5230cc50d..bfc1a23ff 100644 --- a/firewood/src/merkle.rs +++ b/firewood/src/merkle.rs @@ -1,12 +1,11 @@ // Copyright (C) 2023, Ava Labs, Inc. All rights reserved. // See the file LICENSE.md for licensing terms. -use crate::hashednode::HashedNodeStore; +use crate::hashednode::{HashedNodeStore, ValueDigest}; use crate::proof::{Proof, ProofError, ProofNode}; use crate::stream::{MerkleKeyValueStream, PathIterator}; use crate::v2::api; use futures::{StreamExt, TryStreamExt}; -use sha2::{Digest, Sha256}; use std::collections::HashSet; use std::future::ready; use std::io::Write; @@ -139,16 +138,9 @@ impl Merkle { proof.push(ProofNode { key: root.partial_path().bytes(), - value_digest: root.value().map(|value| { - if value.len() < 32 { - value.to_vec().into_boxed_slice() - } else { - // TODO danlaine: I think we can remove this copy by not - // requiring Hash to own its data. - let hash = Sha256::digest(value); - hash.as_slice().to_vec().into_boxed_slice() - } - }), + value_digest: root + .value() + .map(|value| ValueDigest::Value(value.to_vec().into_boxed_slice())), child_hashes, }) } diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index ada83ecb7..c8aacc456 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -37,15 +37,15 @@ pub enum ProofError { EmptyRange, } +pub type OwnedValueDigest = ValueDigest, Box<[u8]>>; + #[derive(Clone, Debug)] pub struct ProofNode { /// The key this node is at. Each byte is a nibble. pub key: Box<[u8]>, - /// None if the node has no value. - /// The value associated with `key` if the value is < 32 bytes. - /// The hash of the value if the node's value is >= 32 bytes. - /// TODO danlaine: Can we enforce that this is at most 32 bytes? - pub value_digest: Option>, + /// None if the node does not have a value. + /// Otherwise, the node's value or the hash of its value. + pub value_digest: Option, /// The hash of each child, or None if the child does not exist. pub child_hashes: [Option; BranchNode::MAX_CHILDREN], } @@ -64,16 +64,10 @@ impl From for ProofNode { Self { key: item.key_nibbles, - value_digest: item.node.value().map(|value| { - if value.len() < 32 { - value.to_vec().into_boxed_slice() - } else { - // TODO danlaine: I think we can remove this copy by not - // requiring Hash to own its data. - let hash = Sha256::digest(value); - hash.as_slice().to_vec().into_boxed_slice() - } - }), + value_digest: item + .node + .value() + .map(|value| ValueDigest::Value(value.to_vec().into_boxed_slice())), child_hashes, } } @@ -96,33 +90,32 @@ impl Proof { expected_value: Option, root_hash: &TrieHash, ) -> Result<(), ProofError> { - match self.value_digest(key, root_hash)? { - None => { - // This proof proves that `key` maps to None. - if expected_value.is_some() { - return Err(ProofError::ExpectedValue); - } + let value_digest = self.value_digest(key, root_hash)?; + + let Some(value_digest) = value_digest else { + // This proof proves that `key` maps to None. + if expected_value.is_some() { + return Err(ProofError::ExpectedValue); } - Some(ValueDigest::Value(got_value)) => { - // This proof proves that `key` maps to `got_value`. - let Some(expected_value) = expected_value else { - // We were expecting `key` to map to None. - return Err(ProofError::UnexpectedValue); - }; + return Ok(()); + }; - if got_value != expected_value.as_ref() { + let Some(expected_value) = expected_value else { + // We were expecting `key` to map to None. + return Err(ProofError::UnexpectedValue); + }; + + match value_digest { + ValueDigest::Value(got_value) => { + // This proof proves that `key` maps to `got_value`. + if got_value.as_ref() != expected_value.as_ref() { // `key` maps to an unexpected value. return Err(ProofError::ValueMismatch); } } - Some(ValueDigest::Hash(got_hash)) => { + ValueDigest::_Hash(got_hash) => { // This proof proves that `key` maps to a value // whose hash is `got_hash`. - let Some(expected_value) = expected_value else { - // We were expecting `key` to map to None. - return Err(ProofError::UnexpectedValue); - }; - let value_hash = Sha256::digest(expected_value.as_ref()); if got_hash.as_ref() != value_hash.as_slice() { // `key` maps to an unexpected value. @@ -137,11 +130,11 @@ impl Proof { /// with the given `root_hash`. If the key does not exist in the trie, returns `None`. /// Returns an error if the proof is invalid or doesn't prove the key for the /// given revision. - fn value_digest<'a, K: AsRef<[u8]>>( - &'a self, + fn value_digest>( + &self, key: K, root_hash: &TrieHash, - ) -> Result>>, ProofError> { + ) -> Result, ProofError> { let key: Vec = NibblesIterator::new(key.as_ref()).collect(); let Some(last_node) = self.0.last() else { @@ -192,16 +185,7 @@ impl Proof { } if last_node.key.len() == key.len() { - // This is an inclusion proof. - return Ok(last_node.value_digest.as_ref().map(|value| { - if value.len() < 32 { - ValueDigest::Value(value.as_ref()) - } else { - // TODO danlaine: I think we can remove this copy by not - // requiring Hash to own its data. - ValueDigest::Hash(value.to_vec().into_boxed_slice()) - } - })); + return Ok(last_node.value_digest.as_ref()); } // This is an exclusion proof. From 4c2ac0970abd0cdefcbc665213716c6fb9bb3a6c Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 11 Jul 2024 10:45:55 -0400 Subject: [PATCH 08/17] add to test --- firewood/src/merkle.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/firewood/src/merkle.rs b/firewood/src/merkle.rs index bfc1a23ff..907dc0fae 100644 --- a/firewood/src/merkle.rs +++ b/firewood/src/merkle.rs @@ -1113,7 +1113,23 @@ mod tests { for (key, value) in kvs { let proof = merkle.prove(&key).unwrap(); - proof.verify(key, Some(value), root_hash).unwrap(); + proof + .verify(key.clone(), Some(value.clone()), root_hash) + .unwrap(); + + { + // Test that the proof is invalid when the value is different + let mut value = value.clone(); + value[0] = value[0].wrapping_add(1); + assert!(proof.verify(key.clone(), Some(value), root_hash).is_err()); + } + + { + // Test that the proof is invalid when the hash is different + assert!(proof + .verify(key, Some(value), &TrieHash::default()) + .is_err()); + } } } From a3232f9c3cd198e049b6bfac690ff8026fe1f47d Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 11 Jul 2024 11:22:27 -0400 Subject: [PATCH 09/17] bound value digest value and hash to be AsRef<[u8]> --- firewood/src/hashednode.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index 2c3e6fbad..12f755f60 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -325,7 +325,11 @@ pub enum ValueDigest { _Hash(H), } -pub(crate) trait Hashable { +pub(crate) trait Hashable +where + V: AsRef<[u8]>, + H: AsRef<[u8]>, +{ /// The key of the node where each byte is a nibble. fn key(&self) -> impl Iterator + Clone; /// The node's value or hash. From 2eb810bef529cab5c07c64d3a2e4272ee12abf57 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 11 Jul 2024 12:51:52 -0400 Subject: [PATCH 10/17] re-add Preimage trait; use associated type for Hashable --- firewood/src/hashednode.rs | 113 +++++++++++++++++++------------------ firewood/src/proof.rs | 6 +- 2 files changed, 62 insertions(+), 57 deletions(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index 12f755f60..3390ed935 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -278,23 +278,17 @@ pub fn hash_preimage(node: &Node, path_prefix: &Path) -> Box<[u8]> { let mut buf = Vec::with_capacity(est_len); match node { Node::Branch(node) => { - write_hash_preimage( - NodeAndPrefix { - node: node.as_ref(), - prefix: path_prefix, - }, - &mut buf, - ); + NodeAndPrefix { + node: node.as_ref(), + prefix: path_prefix, + } + .write(&mut buf); } - Node::Leaf(node) => { - write_hash_preimage( - NodeAndPrefix { - node, - prefix: path_prefix, - }, - &mut buf, - ); + Node::Leaf(node) => NodeAndPrefix { + node, + prefix: path_prefix, } + .write(&mut buf), } buf.into_boxed_slice() } @@ -325,59 +319,64 @@ pub enum ValueDigest { _Hash(H), } -pub(crate) trait Hashable -where - V: AsRef<[u8]>, - H: AsRef<[u8]>, -{ +pub(crate) trait Hashable { + type V; + type H; + /// The key of the node where each byte is a nibble. fn key(&self) -> impl Iterator + Clone; /// The node's value or hash. - fn value_digest(&self) -> Option>; + fn value_digest(&self) -> Option>; /// Each element is a child's index and hash. /// Yields 0 elements if the node is a leaf. fn children(&self) -> impl Iterator + Clone; } -pub(crate) fn hash(preimage: impl Hashable) -> TrieHash -where - V: AsRef<[u8]>, - H: AsRef<[u8]>, -{ - let mut hasher = Sha256::new(); - write_hash_preimage(preimage, &mut hasher); - hasher.finalize().into() +pub(super) trait Preimage { + /// Returns the hash of this preimage. + fn to_hash(self) -> TrieHash; + /// Write this hash preimage to `buf`. + fn write(self, buf: &mut impl HasUpdate); } -fn write_hash_preimage(preimage: impl Hashable, buf: &mut impl HasUpdate) +// Implement Preimage for all types that implement Hashable +impl Preimage for T where - V: AsRef<[u8]>, - H: AsRef<[u8]>, + T::V: AsRef<[u8]>, + T::H: AsRef<[u8]>, { - let children = preimage.children(); + fn to_hash(self) -> TrieHash { + let mut hasher = Sha256::new(); + self.write(&mut hasher); + hasher.finalize().into() + } - let num_children = children.clone().count() as u64; - add_varint_to_buf(buf, num_children); + fn write(self, buf: &mut impl HasUpdate) { + let children = self.children(); - for (index, hash) in children { - add_varint_to_buf(buf, index as u64); - buf.update(hash); - } + let num_children = children.clone().count() as u64; + add_varint_to_buf(buf, num_children); - // Add value digest (if any) to hash pre-image - add_value_digest_to_buf(buf, preimage.value_digest()); + for (index, hash) in children { + add_varint_to_buf(buf, index as u64); + buf.update(hash); + } - // Add key length (in bits) to hash pre-image - let mut key = preimage.key(); - // let mut key = key.as_ref().iter(); - let key_bit_len = BITS_PER_NIBBLE * key.clone().count() as u64; - add_varint_to_buf(buf, key_bit_len); + // Add value digest (if any) to hash pre-image + add_value_digest_to_buf(buf, self.value_digest()); - // Add key to hash pre-image - while let Some(high_nibble) = key.next() { - let low_nibble = key.next().unwrap_or(0); - let byte = (high_nibble << 4) | low_nibble; - buf.update([byte]); + // Add key length (in bits) to hash pre-image + let mut key = self.key(); + // let mut key = key.as_ref().iter(); + let key_bit_len = BITS_PER_NIBBLE * key.clone().count() as u64; + add_varint_to_buf(buf, key_bit_len); + + // Add key to hash pre-image + while let Some(high_nibble) = key.next() { + let low_nibble = key.next().unwrap_or(0); + let byte = (high_nibble << 4) | low_nibble; + buf.update([byte]); + } } } @@ -422,11 +421,14 @@ struct NodeAndPrefix<'a, N: HashableNode> { impl<'a, N: HashableNode> From> for TrieHash { fn from(node: NodeAndPrefix<'a, N>) -> Self { - hash(node) + node.to_hash() } } -impl<'a, N: HashableNode> Hashable<&'a [u8], &'a [u8]> for NodeAndPrefix<'a, N> { +impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { + type V = &'a [u8]; + type H = &'a [u8]; + fn key(&self) -> impl Iterator + Clone { self.prefix .0 @@ -444,7 +446,10 @@ impl<'a, N: HashableNode> Hashable<&'a [u8], &'a [u8]> for NodeAndPrefix<'a, N> } } -impl<'a> Hashable<&'a [u8], &'a [u8]> for &'a ProofNode { +impl<'a> Hashable for &'a ProofNode { + type V = &'a [u8]; + type H = &'a [u8]; + fn key(&self) -> impl Iterator + Clone { self.key.as_ref().iter().copied() } diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index c8aacc456..4741299a0 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -1,7 +1,7 @@ // Copyright (C) 2023, Ava Labs, Inc. All rights reserved. // See the file LICENSE.md for licensing terms. -use crate::hashednode::{hash, ValueDigest}; +use crate::hashednode::{Preimage, ValueDigest}; use crate::merkle::MerkleError; use sha2::{Digest, Sha256}; use storage::{BranchNode, NibblesIterator, PathIterItem, TrieHash}; @@ -75,7 +75,7 @@ impl From for ProofNode { impl From<&ProofNode> for TrieHash { fn from(node: &ProofNode) -> Self { - hash(node) + node.to_hash() } } @@ -148,7 +148,7 @@ impl Proof { #[allow(clippy::indexing_slicing)] let node = &self.0[i]; - if hash(node) != *expected_hash { + if node.to_hash() != *expected_hash { return Err(ProofError::UnexpectedHash); } From 8a4ae635f42f6bcf8bca5ba43d92e74b2c4353be Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 11 Jul 2024 12:56:54 -0400 Subject: [PATCH 11/17] constraint Hashable V and H to be bytes --- firewood/src/hashednode.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index 3390ed935..c06269f0a 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -320,8 +320,8 @@ pub enum ValueDigest { } pub(crate) trait Hashable { - type V; - type H; + type V: AsRef<[u8]>; + type H: AsRef<[u8]>; /// The key of the node where each byte is a nibble. fn key(&self) -> impl Iterator + Clone; From db998d24d95de48ebea63120c8d33b6d94a08ca0 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 11 Jul 2024 14:25:20 -0400 Subject: [PATCH 12/17] parameterize Preimage methods on borrowed self --- firewood/src/hashednode.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index c06269f0a..0181785fb 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -334,9 +334,9 @@ pub(crate) trait Hashable { pub(super) trait Preimage { /// Returns the hash of this preimage. - fn to_hash(self) -> TrieHash; + fn to_hash(&self) -> TrieHash; /// Write this hash preimage to `buf`. - fn write(self, buf: &mut impl HasUpdate); + fn write(&self, buf: &mut impl HasUpdate); } // Implement Preimage for all types that implement Hashable @@ -345,13 +345,13 @@ where T::V: AsRef<[u8]>, T::H: AsRef<[u8]>, { - fn to_hash(self) -> TrieHash { + fn to_hash(&self) -> TrieHash { let mut hasher = Sha256::new(); self.write(&mut hasher); hasher.finalize().into() } - fn write(self, buf: &mut impl HasUpdate) { + fn write(&self, buf: &mut impl HasUpdate) { let children = self.children(); let num_children = children.clone().count() as u64; From 23213566cdc2e29869ce2d62ce4e454781c71a08 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Thu, 11 Jul 2024 16:40:08 -0400 Subject: [PATCH 13/17] remove associated type from ValueDigest --- firewood/src/hashednode.rs | 30 +++++++++++++----------------- firewood/src/proof.rs | 6 ++---- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index 0181785fb..79903a9f9 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -311,22 +311,21 @@ impl HasUpdate for Vec { #[derive(Clone, Debug)] /// A ValueDigest is either a node's value or the hash of its value. -pub enum ValueDigest { - Value(V), +pub enum ValueDigest { + Value(T), /// TODO this variant will be used when we deserialize a proof node /// from a remote Firewood instance. The serialized proof node they /// send us may the hash of the value, not the value itself. - _Hash(H), + _Hash(T), } pub(crate) trait Hashable { - type V: AsRef<[u8]>; - type H: AsRef<[u8]>; + type T: AsRef<[u8]>; /// The key of the node where each byte is a nibble. fn key(&self) -> impl Iterator + Clone; /// The node's value or hash. - fn value_digest(&self) -> Option>; + fn value_digest(&self) -> Option>; /// Each element is a child's index and hash. /// Yields 0 elements if the node is a leaf. fn children(&self) -> impl Iterator + Clone; @@ -342,8 +341,7 @@ pub(super) trait Preimage { // Implement Preimage for all types that implement Hashable impl Preimage for T where - T::V: AsRef<[u8]>, - T::H: AsRef<[u8]>, + T::T: AsRef<[u8]>, { fn to_hash(&self) -> TrieHash { let mut hasher = Sha256::new(); @@ -426,8 +424,7 @@ impl<'a, N: HashableNode> From> for TrieHash { } impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { - type V = &'a [u8]; - type H = &'a [u8]; + type T = &'a [u8]; fn key(&self) -> impl Iterator + Clone { self.prefix @@ -437,7 +434,7 @@ impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { .chain(self.node.partial_path()) } - fn value_digest(&self) -> Option> { + fn value_digest(&self) -> Option> { self.node.value().map(ValueDigest::Value) } @@ -447,14 +444,13 @@ impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { } impl<'a> Hashable for &'a ProofNode { - type V = &'a [u8]; - type H = &'a [u8]; + type T = &'a [u8]; fn key(&self) -> impl Iterator + Clone { self.key.as_ref().iter().copied() } - fn value_digest(&self) -> Option> { + fn value_digest(&self) -> Option> { self.value_digest.as_ref().map(|vd| match vd { ValueDigest::Value(v) => ValueDigest::Value(v.as_ref()), ValueDigest::_Hash(h) => ValueDigest::_Hash(h.as_ref()), @@ -469,9 +465,9 @@ impl<'a> Hashable for &'a ProofNode { } } -fn add_value_digest_to_buf, H: AsRef<[u8]>>( - buf: &mut U, - value_digest: Option>, +fn add_value_digest_to_buf>( + buf: &mut H, + value_digest: Option>, ) { let Some(value_digest) = value_digest else { let value_exists: u8 = 0; diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index 4741299a0..1702d79bf 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -37,15 +37,13 @@ pub enum ProofError { EmptyRange, } -pub type OwnedValueDigest = ValueDigest, Box<[u8]>>; - #[derive(Clone, Debug)] pub struct ProofNode { /// The key this node is at. Each byte is a nibble. pub key: Box<[u8]>, /// None if the node does not have a value. /// Otherwise, the node's value or the hash of its value. - pub value_digest: Option, + pub value_digest: Option>>, /// The hash of each child, or None if the child does not exist. pub child_hashes: [Option; BranchNode::MAX_CHILDREN], } @@ -134,7 +132,7 @@ impl Proof { &self, key: K, root_hash: &TrieHash, - ) -> Result, ProofError> { + ) -> Result>>, ProofError> { let key: Vec = NibblesIterator::new(key.as_ref()).collect(); let Some(last_node) = self.0.last() else { From f6b73a1169e6db2d97599d76344823b61f22b17e Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Tue, 23 Jul 2024 17:24:34 -0400 Subject: [PATCH 14/17] address PR comments --- firewood/src/hashednode.rs | 13 +++++-------- firewood/src/merkle.rs | 17 +++++++++-------- firewood/src/proof.rs | 5 +---- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/firewood/src/hashednode.rs b/firewood/src/hashednode.rs index 79903a9f9..3c717f8c3 100644 --- a/firewood/src/hashednode.rs +++ b/firewood/src/hashednode.rs @@ -320,12 +320,12 @@ pub enum ValueDigest { } pub(crate) trait Hashable { - type T: AsRef<[u8]>; + type ValueDigestType: AsRef<[u8]>; /// The key of the node where each byte is a nibble. fn key(&self) -> impl Iterator + Clone; /// The node's value or hash. - fn value_digest(&self) -> Option>; + fn value_digest(&self) -> Option>; /// Each element is a child's index and hash. /// Yields 0 elements if the node is a leaf. fn children(&self) -> impl Iterator + Clone; @@ -339,10 +339,7 @@ pub(super) trait Preimage { } // Implement Preimage for all types that implement Hashable -impl Preimage for T -where - T::T: AsRef<[u8]>, -{ +impl Preimage for T { fn to_hash(&self) -> TrieHash { let mut hasher = Sha256::new(); self.write(&mut hasher); @@ -424,7 +421,7 @@ impl<'a, N: HashableNode> From> for TrieHash { } impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { - type T = &'a [u8]; + type ValueDigestType = &'a [u8]; fn key(&self) -> impl Iterator + Clone { self.prefix @@ -444,7 +441,7 @@ impl<'a, N: HashableNode> Hashable for NodeAndPrefix<'a, N> { } impl<'a> Hashable for &'a ProofNode { - type T = &'a [u8]; + type ValueDigestType = &'a [u8]; fn key(&self) -> impl Iterator + Clone { self.key.as_ref().iter().copied() diff --git a/firewood/src/merkle.rs b/firewood/src/merkle.rs index 907dc0fae..c5497f298 100644 --- a/firewood/src/merkle.rs +++ b/firewood/src/merkle.rs @@ -807,16 +807,14 @@ impl<'a, T: PartialEq> PrefixOverlap<'a, T> { #[cfg(test)] #[allow(clippy::indexing_slicing, clippy::unwrap_used)] mod tests { - use std::time::{SystemTime, UNIX_EPOCH}; - use super::*; - use rand::{rngs::StdRng, Rng, SeedableRng}; + use rand::{rngs::StdRng, thread_rng, Rng, SeedableRng}; use storage::MemStore; use test_case::test_case; // Returns n random key-value pairs. fn generate_random_kvs(seed: u64, n: usize) -> Vec<(Vec, Vec)> { - println!("Used seed: {}", seed); + eprintln!("Used seed: {}", seed); let mut rng = StdRng::seed_from_u64(seed); @@ -1093,10 +1091,13 @@ mod tests { fn single_key_proof() { let mut merkle = create_in_memory_merkle(); - let seed = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_secs(); + let seed = std::env::var("FIREWOOD_TEST_SEED") + .ok() + .map_or_else( + || None, + |s| Some(str::parse(&s).expect("couldn't parse FIREWOOD_TEST_SEED; must be a u64")), + ) + .unwrap_or_else(|| thread_rng().gen()); const TEST_SIZE: usize = 1; diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index 1702d79bf..f4959e4e0 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -142,10 +142,7 @@ impl Proof { let mut expected_hash = root_hash; // TODO danlaine: Is there a better way to do this loop? - for i in 0..self.0.len() { - #[allow(clippy::indexing_slicing)] - let node = &self.0[i]; - + for (i, node) in self.0.iter().enumerate() { if node.to_hash() != *expected_hash { return Err(ProofError::UnexpectedHash); } From 0ab0bf3e16d6246d37a573146770678ca37fe63f Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Tue, 23 Jul 2024 17:37:26 -0400 Subject: [PATCH 15/17] use peekable to avoid indexing slice --- firewood/src/proof.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index f4959e4e0..d6c68d159 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -142,7 +142,8 @@ impl Proof { let mut expected_hash = root_hash; // TODO danlaine: Is there a better way to do this loop? - for (i, node) in self.0.iter().enumerate() { + let mut iter = self.0.iter().peekable(); + while let Some(node) = iter.next() { if node.to_hash() != *expected_hash { return Err(ProofError::UnexpectedHash); } @@ -153,7 +154,7 @@ impl Proof { return Err(ProofError::ValueAtOddNibbleLength); } - if i != self.0.len() - 1 { + if let Some(next_node) = iter.peek() { // Assert that every node's key is a prefix of the proven key, // with the exception of the last node, which is a suffix of the // proven key in exclusion proofs. @@ -171,9 +172,7 @@ impl Proof { .ok_or(ProofError::NodeNotInTrie)?; // Assert that each node's key is a prefix of the next node's key. - #[allow(clippy::indexing_slicing)] - let next_node_key = &self.0[i + 1].key; - if !is_prefix(&mut node.key.iter(), &mut next_node_key.iter()) { + if !is_prefix(&mut node.key.iter(), &mut next_node.key.iter()) { return Err(ProofError::ShouldBePrefixOfNextKey); } } From 2ccce3627d0c63e0c15560bc52ff71a6d4d7dec9 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 24 Jul 2024 14:49:30 -0400 Subject: [PATCH 16/17] nit change next_nibble signature --- firewood/src/proof.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index d6c68d159..44fc633b8 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -141,7 +141,6 @@ impl Proof { let mut expected_hash = root_hash; - // TODO danlaine: Is there a better way to do this loop? let mut iter = self.0.iter().peekable(); while let Some(node) = iter.next() { if node.to_hash() != *expected_hash { @@ -158,7 +157,7 @@ impl Proof { // Assert that every node's key is a prefix of the proven key, // with the exception of the last node, which is a suffix of the // proven key in exclusion proofs. - let next_nibble = next_nibble(&mut node.key.iter(), &mut key.iter())?; + let next_nibble = next_nibble(&node.key, &key)?; let Some(next_nibble) = next_nibble else { return Err(ProofError::ShouldBePrefixOfProvenKey); @@ -189,10 +188,10 @@ impl Proof { /// Returns the next nibble in `c` after `b`. /// Returns an error if `b` is not a prefix of `c`. -fn next_nibble<'a, I>(b: &mut I, c: &mut I) -> Result, ProofError> -where - I: Iterator, -{ +fn next_nibble(b: impl AsRef<[u8]>, c: impl AsRef<[u8]>) -> Result, ProofError> { + let b = b.as_ref(); + let mut c = c.as_ref().iter(); + // Check if b is a prefix of c for b_item in b { match c.next() { From 5dd9806f80fae4baba612df48fd837aedde6da28 Mon Sep 17 00:00:00 2001 From: Dan Laine Date: Wed, 24 Jul 2024 14:58:03 -0400 Subject: [PATCH 17/17] nit change is_prefix signature --- firewood/src/proof.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/firewood/src/proof.rs b/firewood/src/proof.rs index 44fc633b8..7d43cde4e 100644 --- a/firewood/src/proof.rs +++ b/firewood/src/proof.rs @@ -171,7 +171,7 @@ impl Proof { .ok_or(ProofError::NodeNotInTrie)?; // Assert that each node's key is a prefix of the next node's key. - if !is_prefix(&mut node.key.iter(), &mut next_node.key.iter()) { + if !is_prefix(&node.key, &next_node.key) { return Err(ProofError::ShouldBePrefixOfNextKey); } } @@ -204,11 +204,10 @@ fn next_nibble(b: impl AsRef<[u8]>, c: impl AsRef<[u8]>) -> Result, P Ok(c.next().copied()) } -fn is_prefix<'a, I>(b: &mut I, c: &mut I) -> bool -where - I: Iterator, -{ - for b_item in b { +/// Returns true iff `b` is a prefix of `c`. +fn is_prefix(b: impl AsRef<[u8]>, c: impl AsRef<[u8]>) -> bool { + let mut c = c.as_ref().iter(); + for b_item in b.as_ref() { let Some(c_item) = c.next() else { return false; };