Skip to content

Commit

Permalink
fix: ensures mutable MMR bitmaps are compressed (#5278)
Browse files Browse the repository at this point in the history
Description
---
Ensures that mutable MMR root computation first performs deletion bitmap
compression.

Closes [issue 5277](#5277).

Motivation and Context
---
Currently, when mutable MMR roots [are
computed](https://github.com/tari-project/tari/blob/e334a404e432b0911bae3054a28d8e8ca5876e6c/base_layer/mmr/src/mutable_mmr.rs#L119-L131),
it's implicitly assumed that the underlying deletion bitmap has been
compressed. If it has not, it's possible for the resulting root to be
different than if the bitmap were compressed. This already resulted in
an intermittent [test
failure](#5268).

To reduce the risk that a caller does not perform this compression
correctly, this PR adds compression to the Merkle root computation
functionality directly. It also removes a few compression calls that
become redundant with this change.

Note that this may constitute an efficiency regression. If a mutable MMR
does not have any deletions since its last bitmap compression,
subsequent compression is not necessary. Further, we perform the
compression on a clone of the bitmap; this is necessary to avoid
mutability and ensure that operations like equality (which rely on root
computation) function properly. The efficiency consequences of this
change should be examined to ensure they are acceptable.

How Has This Been Tested?
---
Existing CI passes.

What process can a PR reviewer use to test or verify this change?
---
Run existing CI. To further check for intermittent test failures, loop
affected tests.

Breaking Changes
---

- [ ] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [x] Other - Please specify

It may be the case that this change affects the way mutable MMR roots
are computed in certain cases, which in turn may be a breaking change.
  • Loading branch information
AaronFeickert authored Mar 28, 2023
1 parent e334a40 commit dfddc66
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,6 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> {
// in the output MMR
let bitmap = self.full_bitmap_mut();
bitmap.or_inplace(&diff_bitmap);
bitmap.run_optimize();

let pruned_output_set = output_mmr.get_pruned_hash_set()?;
let output_mmr = MutablePrunedOutputMmr::new(pruned_output_set.clone(), bitmap.clone())?;
Expand Down
2 changes: 0 additions & 2 deletions base_layer/core/src/chain_storage/blockchain_database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1360,8 +1360,6 @@ pub fn calculate_mmr_roots<T: BlockchainBackend>(
}
}

output_mmr.compress();

let block_height = block.header.height;
let epoch_len = rules.consensus_constants(block_height).epoch_length();
let validator_node_mr = if block_height % epoch_len == 0 {
Expand Down
1 change: 0 additions & 1 deletion base_layer/mmr/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ where
for index in deletions {
pruned_mmr.delete(index);
}
pruned_mmr.compress();
pruned_mmr.get_merkle_root()
}

Expand Down
18 changes: 10 additions & 8 deletions base_layer/mmr/src/mutable_mmr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,18 @@ where
// virtue of the fact that the underlying MMRs could be different, but all elements are marked as deleted in
// both sets.
let mmr_root = self.mmr.get_merkle_root()?;

// To avoid requiring mutability, we compress a clone of the bitmap
let mut bitmap = self.deleted.clone();
bitmap.run_optimize();
let bitmap_ser = bitmap.serialize();

// Include the compressed bitmap in the root hash
let mut hasher = D::new();
hasher.update(&mmr_root);
Ok(self.hash_deleted(hasher).finalize().to_vec())
hasher.update(&bitmap_ser);

Ok(hasher.finalize().to_vec())
}

/// Returns only the MMR merkle root without the compressed serialisation of the bitmap
Expand Down Expand Up @@ -197,13 +206,6 @@ where
self.mmr.validate()
}

/// Hash the roaring bitmap of nodes that are marked for deletion
fn hash_deleted(&self, mut hasher: D) -> D {
let bitmap_ser = self.deleted.serialize();
hasher.update(&bitmap_ser);
hasher
}

// Returns a bitmap with only the deleted nodes for the specified region in the MMR.
fn get_sub_bitmap(&self, leaf_index: LeafIndex, count: usize) -> Result<Bitmap, MerkleMountainRangeError> {
let mut deleted = self.deleted.clone();
Expand Down

0 comments on commit dfddc66

Please sign in to comment.