Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: addresses mmr find_peaks bug #5182

Merged
merged 10 commits into from
Feb 21, 2023
71 changes: 55 additions & 16 deletions base_layer/mmr/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<usize> {
pub fn find_peaks(size: usize) -> Option<Vec<usize>> {
if size == 0 {
return vec![];
return Some(vec![]);
}
let mut peak_size = ALL_ONES >> size.leading_zeros();
let mut num_left = size;
Expand All @@ -78,9 +78,21 @@ pub fn find_peaks(size: usize) -> Vec<usize> {
peak_size >>= 1;
}
if num_left > 0 {
return vec![];
// 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
// is invalid, as it can be completed to form
// 6
// / \
// 2 5
// / \ / \
// 0 1 3 4
// which is of size 7 (with single peak [6])
return None;
}
peaks
Some(peaks)
}

/// Calculates the positions of the (parent, sibling) of the node at the provided position.
Expand Down Expand Up @@ -245,12 +257,39 @@ mod test {

#[test]
fn peak_vectors() {
assert_eq!(find_peaks(0), Vec::<usize>::new());
assert_eq!(find_peaks(1), vec![0]);
assert_eq!(find_peaks(3), vec![2]);
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(0), Some(Vec::<usize>::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]
Expand Down Expand Up @@ -350,11 +389,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!(find_peaks(2).is_empty());
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!(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);
}
}
2 changes: 2 additions & 0 deletions base_layer/mmr/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 2 additions & 1 deletion base_layer/mmr/src/merkle_mountain_range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down
4 changes: 2 additions & 2 deletions base_layer/mmr/src/merkle_proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI If the MMR has been bagged, find_peaks will not error

let mut peak_hashes = Vec::with_capacity(peaks.len() - 1);
for peak_index in peaks {
if peak_index != peak_pos {
Expand Down Expand Up @@ -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::<D>(root, hash, pos, &peaks)
}

Expand Down
2 changes: 1 addition & 1 deletion base_layer/mmr/src/pruned_hashset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ where

fn try_from(base_mmr: &MerkleMountainRange<D, B>) -> Result<Self, Self::Error> {
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)? {
Expand Down