From 996a7d5ac3fd1729303968887a75f9134cf821f0 Mon Sep 17 00:00:00 2001 From: jorgeantonio21 Date: Tue, 14 Feb 2023 20:00:09 +0000 Subject: [PATCH 1/5] first commit --- base_layer/mmr/src/common.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/base_layer/mmr/src/common.rs b/base_layer/mmr/src/common.rs index 136bbb5ade..7a2285c944 100644 --- a/base_layer/mmr/src/common.rs +++ b/base_layer/mmr/src/common.rs @@ -72,7 +72,11 @@ pub fn find_peaks(size: usize) -> Vec { sum_prev_peaks += peak_size; num_left -= peak_size; } - peak_size >>= 1; + // need to verify that peaks can exist on the same height, in which case + // the size doesn't give rise to a complete mmr + if num_left < peak_size { + peak_size >>= 1; + } } if num_left > 0 { return vec![]; @@ -248,6 +252,13 @@ mod test { assert_eq!(find_peaks(4), vec![2, 3]); assert_eq!(find_peaks(15), vec![14]); assert_eq!(find_peaks(23), vec![14, 21, 22]); + assert_eq!(find_peaks(123), vec![62, 93, 108, 115, 122]); + assert_eq!(find_peaks(130), vec![126, 129]); + assert_eq!(find_peaks(56), vec![30, 45, 52, 55]); + assert_eq!(find_peaks(60), vec![30, 45, 52, 59]); + assert_eq!(find_peaks(28), vec![14, 21, 24, 27]); + assert_eq!(find_peaks(5), vec![2, 3, 4]); + assert_eq!(find_peaks(6), vec![2, 5]); } #[test] @@ -349,9 +360,9 @@ mod test { fn find_peaks_when_num_left_gt_zero() { assert!(find_peaks(0).is_empty()); assert_eq!(find_peaks(1), vec![0]); - assert!(find_peaks(2).is_empty()); + assert_eq!(find_peaks(2), vec![0, 1]); assert_eq!(find_peaks(3), vec![2]); assert_eq!(find_peaks(usize::MAX), [18446744073709551614].to_vec()); - assert_eq!(find_peaks(usize::MAX - 1).len(), 0); + assert_eq!(find_peaks(usize::MAX - 1).len(), 2); } } From dfecc9310e5361665781ae1f17a8da77bf39350f Mon Sep 17 00:00:00 2001 From: jorgeantonio21 Date: Tue, 14 Feb 2023 21:40:08 +0000 Subject: [PATCH 2/5] add tests --- base_layer/mmr/src/common.rs | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/base_layer/mmr/src/common.rs b/base_layer/mmr/src/common.rs index 7a2285c944..387ab12312 100644 --- a/base_layer/mmr/src/common.rs +++ b/base_layer/mmr/src/common.rs @@ -248,17 +248,34 @@ mod test { fn peak_vectors() { assert_eq!(find_peaks(0), Vec::::new()); assert_eq!(find_peaks(1), vec![0]); + assert_eq!(find_peaks(2), vec![0, 1]); assert_eq!(find_peaks(3), vec![2]); assert_eq!(find_peaks(4), vec![2, 3]); + assert_eq!(find_peaks(5), vec![2, 3, 4]); + assert_eq!(find_peaks(6), vec![2, 5]); + assert_eq!(find_peaks(7), vec![6]); + assert_eq!(find_peaks(8), vec![6, 7]); + assert_eq!(find_peaks(9), vec![6, 7, 8]); + assert_eq!(find_peaks(10), vec![6, 9]); + assert_eq!(find_peaks(11), vec![6, 9, 10]); + assert_eq!(find_peaks(12), vec![6, 9, 10, 11]); + assert_eq!(find_peaks(13), vec![6, 9, 12]); + assert_eq!(find_peaks(14), vec![6, 13]); assert_eq!(find_peaks(15), vec![14]); + assert_eq!(find_peaks(16), vec![14, 15]); + assert_eq!(find_peaks(17), vec![14, 15, 16]); + assert_eq!(find_peaks(18), vec![14, 17]); + assert_eq!(find_peaks(19), vec![14, 17, 18]); + assert_eq!(find_peaks(20), vec![14, 17, 18, 19]); + assert_eq!(find_peaks(21), vec![14, 17, 20]); + assert_eq!(find_peaks(22), vec![14, 21]); assert_eq!(find_peaks(23), vec![14, 21, 22]); - assert_eq!(find_peaks(123), vec![62, 93, 108, 115, 122]); - assert_eq!(find_peaks(130), vec![126, 129]); + assert_eq!(find_peaks(24), vec![14, 21, 22, 23]); + assert_eq!(find_peaks(28), vec![14, 21, 24, 27]); assert_eq!(find_peaks(56), vec![30, 45, 52, 55]); assert_eq!(find_peaks(60), vec![30, 45, 52, 59]); - assert_eq!(find_peaks(28), vec![14, 21, 24, 27]); - assert_eq!(find_peaks(5), vec![2, 3, 4]); - assert_eq!(find_peaks(6), vec![2, 5]); + assert_eq!(find_peaks(123), vec![62, 93, 108, 115, 122]); + assert_eq!(find_peaks(130), vec![126, 129]); } #[test] From 15176a9ffa6e14b886c0d3177355019226f28ba6 Mon Sep 17 00:00:00 2001 From: jorgeantonio21 Date: Tue, 14 Feb 2023 21:45:12 +0000 Subject: [PATCH 3/5] update comments --- base_layer/mmr/src/common.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/base_layer/mmr/src/common.rs b/base_layer/mmr/src/common.rs index e26cded44d..fd63229b9b 100644 --- a/base_layer/mmr/src/common.rs +++ b/base_layer/mmr/src/common.rs @@ -75,8 +75,12 @@ pub fn find_peaks(size: usize) -> Vec { sum_prev_peaks += peak_size; num_left -= peak_size; } - // need to verify that peaks can exist on the same height, in which case - // the size doesn't give rise to a complete mmr + // need to verify if other peaks exist on the same height, in which case + // the size doesn't generate a complete mmr, e.g. + // 2 + // / \ + // 0 1 3 4 + // where we have two peaks at height 0. if num_left < peak_size { peak_size >>= 1; } From 17bd112051cb40e8949b4042365bf458bf09fd27 Mon Sep 17 00:00:00 2001 From: jorgeantonio21 Date: Wed, 15 Feb 2023 10:16:45 +0000 Subject: [PATCH 4/5] address PR comments --- base_layer/mmr/src/common.rs | 101 ++++++++++---------- base_layer/mmr/src/error.rs | 2 + base_layer/mmr/src/merkle_mountain_range.rs | 3 +- base_layer/mmr/src/merkle_proof.rs | 4 +- base_layer/mmr/src/pruned_hashset.rs | 2 +- 5 files changed, 60 insertions(+), 52 deletions(-) diff --git a/base_layer/mmr/src/common.rs b/base_layer/mmr/src/common.rs index fd63229b9b..42a6cc4e97 100644 --- a/base_layer/mmr/src/common.rs +++ b/base_layer/mmr/src/common.rs @@ -61,9 +61,9 @@ pub fn is_leaf(pos: usize) -> bool { /// Gets the postorder traversal index of all peaks in a MMR given its size. /// Starts with the top peak, which is always on the left side of the range, and navigates toward lower siblings /// toward the right of the range. -pub fn find_peaks(size: usize) -> Vec { +pub fn find_peaks(size: usize) -> Option> { if size == 0 { - return vec![]; + return Some(vec![]); } let mut peak_size = ALL_ONES >> size.leading_zeros(); let mut num_left = size; @@ -75,20 +75,22 @@ pub fn find_peaks(size: usize) -> Vec { sum_prev_peaks += peak_size; num_left -= peak_size; } - // need to verify if other peaks exist on the same height, in which case - // the size doesn't generate a complete mmr, e.g. + peak_size >>= 1; + } + if num_left > 0 { + // This happens, whenever the MMR is not valid, that is, all nodes are not + // fully spawned. For example, in this case // 2 // / \ // 0 1 3 4 - // where we have two peaks at height 0. - if num_left < peak_size { - peak_size >>= 1; - } - } - if num_left > 0 { - return vec![]; + // is invalid, as it can be completed to form + // 2 5 + // / \ / \ + // 0 1 3 4 + // which is of size 6 (with peaks [2, 5]) + return None; } - peaks + Some(peaks) } /// Calculates the positions of the (parent, sibling) of the node at the provided position. @@ -253,36 +255,39 @@ mod test { #[test] fn peak_vectors() { - assert_eq!(find_peaks(0), Vec::::new()); - assert_eq!(find_peaks(1), vec![0]); - assert_eq!(find_peaks(2), vec![0, 1]); - assert_eq!(find_peaks(3), vec![2]); - assert_eq!(find_peaks(4), vec![2, 3]); - assert_eq!(find_peaks(5), vec![2, 3, 4]); - assert_eq!(find_peaks(6), vec![2, 5]); - assert_eq!(find_peaks(7), vec![6]); - assert_eq!(find_peaks(8), vec![6, 7]); - assert_eq!(find_peaks(9), vec![6, 7, 8]); - assert_eq!(find_peaks(10), vec![6, 9]); - assert_eq!(find_peaks(11), vec![6, 9, 10]); - assert_eq!(find_peaks(12), vec![6, 9, 10, 11]); - assert_eq!(find_peaks(13), vec![6, 9, 12]); - assert_eq!(find_peaks(14), vec![6, 13]); - assert_eq!(find_peaks(15), vec![14]); - assert_eq!(find_peaks(16), vec![14, 15]); - assert_eq!(find_peaks(17), vec![14, 15, 16]); - assert_eq!(find_peaks(18), vec![14, 17]); - assert_eq!(find_peaks(19), vec![14, 17, 18]); - assert_eq!(find_peaks(20), vec![14, 17, 18, 19]); - assert_eq!(find_peaks(21), vec![14, 17, 20]); - assert_eq!(find_peaks(22), vec![14, 21]); - assert_eq!(find_peaks(23), vec![14, 21, 22]); - assert_eq!(find_peaks(24), vec![14, 21, 22, 23]); - assert_eq!(find_peaks(28), vec![14, 21, 24, 27]); - assert_eq!(find_peaks(56), vec![30, 45, 52, 55]); - assert_eq!(find_peaks(60), vec![30, 45, 52, 59]); - assert_eq!(find_peaks(123), vec![62, 93, 108, 115, 122]); - assert_eq!(find_peaks(130), vec![126, 129]); + assert_eq!(find_peaks(0), Some(Vec::::new())); + assert_eq!(find_peaks(1), Some(vec![0])); + assert_eq!(find_peaks(2), None); + assert_eq!(find_peaks(3), Some(vec![2])); + assert_eq!(find_peaks(4), Some(vec![2, 3])); + assert_eq!(find_peaks(5), None); + assert_eq!(find_peaks(6), None); + assert_eq!(find_peaks(7), Some(vec![6])); + assert_eq!(find_peaks(8), Some(vec![6, 7])); + assert_eq!(find_peaks(9), None); + assert_eq!(find_peaks(10), Some(vec![6, 9])); + assert_eq!(find_peaks(11), Some(vec![6, 9, 10])); + assert_eq!(find_peaks(12), None); + assert_eq!(find_peaks(13), None); + assert_eq!(find_peaks(14), None); + assert_eq!(find_peaks(15), Some(vec![14])); + assert_eq!(find_peaks(16), Some(vec![14, 15])); + assert_eq!(find_peaks(17), None); + assert_eq!(find_peaks(18), Some(vec![14, 17])); + assert_eq!(find_peaks(19), Some(vec![14, 17, 18])); + assert_eq!(find_peaks(20), None); + assert_eq!(find_peaks(21), None); + assert_eq!(find_peaks(22), Some(vec![14, 21])); + assert_eq!(find_peaks(23), Some(vec![14, 21, 22])); + assert_eq!(find_peaks(24), None); + assert_eq!(find_peaks(25), Some(vec![14, 21, 24])); + assert_eq!(find_peaks(26), Some(vec![14, 21, 24, 25])); + assert_eq!(find_peaks(27), None); + assert_eq!(find_peaks(28), None); + assert_eq!(find_peaks(56), Some(vec![30, 45, 52, 55])); + assert_eq!(find_peaks(60), None); + assert_eq!(find_peaks(123), None); + assert_eq!(find_peaks(130), Some(vec![126, 129])); } #[test] @@ -382,11 +387,11 @@ mod test { #[test] fn find_peaks_when_num_left_gt_zero() { - assert!(find_peaks(0).is_empty()); - assert_eq!(find_peaks(1), vec![0]); - assert_eq!(find_peaks(2), vec![0, 1]); - assert_eq!(find_peaks(3), vec![2]); - assert_eq!(find_peaks(usize::MAX), [18446744073709551614].to_vec()); - assert_eq!(find_peaks(usize::MAX - 1).len(), 2); + assert!(find_peaks(0).unwrap().is_empty()); + assert_eq!(find_peaks(1).unwrap(), vec![0]); + assert_eq!(find_peaks(2), None); + assert_eq!(find_peaks(3).unwrap(), vec![2]); + assert_eq!(find_peaks(usize::MAX).unwrap(), [18446744073709551614].to_vec()); + assert_eq!(find_peaks(usize::MAX - 1), None); } } diff --git a/base_layer/mmr/src/error.rs b/base_layer/mmr/src/error.rs index d8c3024d90..9d13a22b54 100644 --- a/base_layer/mmr/src/error.rs +++ b/base_layer/mmr/src/error.rs @@ -38,6 +38,8 @@ pub enum MerkleMountainRangeError { OutOfRange, #[error("Conflicting or invalid configuration parameters provided.")] InvalidConfig, + #[error("Invalid Merkle Mountain Range size")] + InvalidMmrSize, } impl MerkleMountainRangeError { diff --git a/base_layer/mmr/src/merkle_mountain_range.rs b/base_layer/mmr/src/merkle_mountain_range.rs index b05926bb5a..b378f69172 100644 --- a/base_layer/mmr/src/merkle_mountain_range.rs +++ b/base_layer/mmr/src/merkle_mountain_range.rs @@ -149,7 +149,8 @@ where self.hashes .len() .map_err(|e| MerkleMountainRangeError::BackendError(e.to_string()))?, - ); + ) + .ok_or(MerkleMountainRangeError::InvalidMmrSize)?; Ok(peaks .into_iter() .map(|i| { diff --git a/base_layer/mmr/src/merkle_proof.rs b/base_layer/mmr/src/merkle_proof.rs index d1c9ec10f1..afb41991eb 100644 --- a/base_layer/mmr/src/merkle_proof.rs +++ b/base_layer/mmr/src/merkle_proof.rs @@ -137,7 +137,7 @@ impl MerkleProof { // Get the peaks of the merkle trees, which are bagged together to form the root // For the proof, we must leave out the local root for the candidate node - let peaks = find_peaks(mmr_size); + let peaks = find_peaks(mmr_size).ok_or(MerkleMountainRangeError::InvalidMmrSize)?; let mut peak_hashes = Vec::with_capacity(peaks.len() - 1); for peak_index in peaks { if peak_index != peak_pos { @@ -174,7 +174,7 @@ impl MerkleProof { ) -> Result<(), MerkleProofError> { let mut proof = self.clone(); // calculate the peaks once as these are based on overall MMR size (and will not change) - let peaks = find_peaks(self.mmr_size); + let peaks = find_peaks(self.mmr_size).ok_or(MerkleMountainRangeError::InvalidMmrSize)?; proof.verify_consume::(root, hash, pos, &peaks) } diff --git a/base_layer/mmr/src/pruned_hashset.rs b/base_layer/mmr/src/pruned_hashset.rs index a692b79068..b318f4a609 100644 --- a/base_layer/mmr/src/pruned_hashset.rs +++ b/base_layer/mmr/src/pruned_hashset.rs @@ -58,7 +58,7 @@ where fn try_from(base_mmr: &MerkleMountainRange) -> Result { let base_offset = base_mmr.len()?; - let peak_indices = find_peaks(base_offset); + let peak_indices = find_peaks(base_offset).ok_or(MerkleMountainRangeError::InvalidMmrSize)?; let peak_hashes = peak_indices .iter() .map(|i| match base_mmr.get_node_hash(*i)? { From 65226a48013b8d20a02b0f1c2ce7910dca558956 Mon Sep 17 00:00:00 2001 From: jorgeantonio21 Date: Wed, 15 Feb 2023 10:18:39 +0000 Subject: [PATCH 5/5] update comment --- base_layer/mmr/src/common.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/base_layer/mmr/src/common.rs b/base_layer/mmr/src/common.rs index 42a6cc4e97..b2123f1ab0 100644 --- a/base_layer/mmr/src/common.rs +++ b/base_layer/mmr/src/common.rs @@ -84,10 +84,12 @@ pub fn find_peaks(size: usize) -> Option> { // / \ // 0 1 3 4 // is invalid, as it can be completed to form + // 6 + // / \ // 2 5 // / \ / \ // 0 1 3 4 - // which is of size 6 (with peaks [2, 5]) + // which is of size 7 (with single peak [6]) return None; } Some(peaks)