From 043f069f7ceacbe360ef065207a40706a2ebd87a Mon Sep 17 00:00:00 2001 From: Andrew Fleming Date: Thu, 14 Nov 2024 18:09:55 -0600 Subject: [PATCH] Minor improvements to the merkle tree package (#1213) * improve doc comments * add PartialOrd comment, improve comments * fix fn name in comment * improve docs * fix comment * tiny fix, remove backticks from type --- docs/modules/ROOT/pages/api/merkle-tree.adoc | 6 +++--- packages/merkle_tree/src/hashes.cairo | 12 +++++++----- packages/merkle_tree/src/merkle_proof.cairo | 8 ++++---- .../src/tests/merkle_proof/test_with_pedersen.cairo | 2 +- .../src/tests/merkle_proof/test_with_poseidon.cairo | 2 +- 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/docs/modules/ROOT/pages/api/merkle-tree.adoc b/docs/modules/ROOT/pages/api/merkle-tree.adoc index 81fe1b7d5..1dad61463 100644 --- a/docs/modules/ROOT/pages/api/merkle-tree.adoc +++ b/docs/modules/ROOT/pages/api/merkle-tree.adoc @@ -40,7 +40,7 @@ These functions deal with verification of Merkle Tree proofs. The tree and the proofs can be generated using this {strk-merkle-tree}. You will find a quickstart guide in the readme. -WARNING: You should avoid using leaf values that are two felt252 long prior to hashing, or use a hash function +WARNING: You should avoid using leaf values that are two felt252 values long prior to hashing, or use a hash function other than the one used to hash internal nodes for hashing leaves. This is because the concatenation of a sorted pair of internal nodes in the Merkle tree could be reinterpreted as a leaf value. The JavaScript library generates Merkle trees that are safe against this attack out of the box. @@ -131,7 +131,7 @@ Not all Merkle trees admit multiproofs. To use multiproofs, it is sufficient to ensure that: 1. The tree is complete (but not necessarily perfect). -2. The leaves to be proven are in the opposite order they are in the tree. +2. The leaves to be proven are in the opposite order than they are in the tree. (i.e., as seen from right to left starting at the deepest layer and continuing at the next layer). ==== @@ -182,7 +182,7 @@ Declares a commutative hash function with the following signature: `commutative_hash(a: felt252, b: felt252) -> felt252;` -which computes a commutative hash of a sorted pair of `felt252`. +which computes a commutative hash of a sorted pair of felt252 values. This is usually implemented as an extension of a non-commutative hash function, like Pedersen or Poseidon, returning the hash of the concatenation of the two values by first diff --git a/packages/merkle_tree/src/hashes.cairo b/packages/merkle_tree/src/hashes.cairo index 78e01d5e3..2a0e85697 100644 --- a/packages/merkle_tree/src/hashes.cairo +++ b/packages/merkle_tree/src/hashes.cairo @@ -6,7 +6,7 @@ use core::pedersen::PedersenTrait; use core::poseidon::PoseidonTrait; use core::traits::PartialOrd; -/// Computes a commutative hash of a sorted pair of felt252. +/// Computes a commutative hash of a sorted pair of felt252 values. /// /// This is usually implemented as an extension of a non-commutative hash function, like /// Pedersen or Poseidon, returning the hash of the concatenation of the two values by first @@ -17,10 +17,10 @@ pub trait CommutativeHasher { fn commutative_hash(a: felt252, b: felt252) -> felt252; } -/// Computes Pedersen's commutative hash of a sorted pair of felt252. +/// Computes the Pedersen commutative hash of a sorted pair of felt252 values. pub impl PedersenCHasher of CommutativeHasher { - /// Computes the Pedersen hash of chaining the two values - /// with the len, sorting the pair first. + /// Computes the Pedersen hash by chaining the two values + /// with the length, sorting the pair first. fn commutative_hash(a: felt252, b: felt252) -> felt252 { let hash_state = PedersenTrait::new(0); if a < b { @@ -31,7 +31,7 @@ pub impl PedersenCHasher of CommutativeHasher { } } -/// Computes Poseidon's commutative hash of a sorted pair of felt252. +/// Computes the Poseidon commutative hash of a sorted pair of felt252 values. pub impl PoseidonCHasher of CommutativeHasher { /// Computes the Poseidon hash of the concatenation of two values, sorting the pair first. fn commutative_hash(a: felt252, b: felt252) -> felt252 { @@ -44,6 +44,8 @@ pub impl PoseidonCHasher of CommutativeHasher { } } +/// PartialOrd implementation for felt252 comparisons. +/// This is used in the CommutativeHasher impls. impl Felt252AsIntPartialOrd of PartialOrd { #[inline(always)] fn lt(lhs: felt252, rhs: felt252) -> bool { diff --git a/packages/merkle_tree/src/merkle_proof.cairo b/packages/merkle_tree/src/merkle_proof.cairo index 8fe78e172..d8fd24e62 100644 --- a/packages/merkle_tree/src/merkle_proof.cairo +++ b/packages/merkle_tree/src/merkle_proof.cairo @@ -71,7 +71,7 @@ pub fn verify_multi_proof( /// /// CAUTION: Not all Merkle trees admit multiproofs. To use multiproofs, it is sufficient to ensure /// that: 1) the tree is complete (but not necessarily perfect), 2) the leaves to be proven are in -/// the opposite order they are in the tree (i.e., as seen from right to left starting at the +/// the opposite order than they are in the tree (i.e., as seen from right to left starting at the /// deepest layer and continuing at the next layer). /// /// NOTE: The _empty set_ (i.e. the case where `proof.len() == 1 && leaves.len() == 0`) is @@ -93,16 +93,16 @@ pub fn process_multi_proof( } // The x_pos values are "pointers" to the next value to consume in each array. - // By incrementing the value we simulate a queue's pop operation. + // By incrementing the value, we simulate a queue's pop operation. let mut hashes = array![]; let mut leaf_pos = 0; let mut hash_pos = 0; let mut proof_pos = 0; // At each step, we compute the next hash using two values: - // - a value from the "main queue". If not all leaves have been consumed, we get the next leaf, + // 1. A value from the "main queue". If not all leaves have been consumed, we get the next leaf, // otherwise we get the next hash. - // - depending on the flag, either another value from the "main queue" (merging branches) or an + // 2. Depending on the flag, either another value from the "main queue" (merging branches) or an // element from the `proof` array. for i in 0 ..proof_flags_len { diff --git a/packages/merkle_tree/src/tests/merkle_proof/test_with_pedersen.cairo b/packages/merkle_tree/src/tests/merkle_proof/test_with_pedersen.cairo index 8a1d430c4..f2598f402 100644 --- a/packages/merkle_tree/src/tests/merkle_proof/test_with_pedersen.cairo +++ b/packages/merkle_tree/src/tests/merkle_proof/test_with_pedersen.cairo @@ -59,7 +59,7 @@ fn test_invalid_merkle_proof() { } // -// multi_proof_verify +// verify_multi_proof // #[test] diff --git a/packages/merkle_tree/src/tests/merkle_proof/test_with_poseidon.cairo b/packages/merkle_tree/src/tests/merkle_proof/test_with_poseidon.cairo index da424b7f6..b7dce4442 100644 --- a/packages/merkle_tree/src/tests/merkle_proof/test_with_poseidon.cairo +++ b/packages/merkle_tree/src/tests/merkle_proof/test_with_poseidon.cairo @@ -58,7 +58,7 @@ fn test_invalid_merkle_proof() { } // -// multi_proof_verify +// verify_multi_proof // #[test]