From dfddc669e3e1271b098c8b271e13f076ca79b039 Mon Sep 17 00:00:00 2001 From: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com> Date: Tue, 28 Mar 2023 12:37:51 -0500 Subject: [PATCH] fix: ensures mutable MMR bitmaps are compressed (#5278) Description --- Ensures that mutable MMR root computation first performs deletion bitmap compression. Closes [issue 5277](https://github.com/tari-project/tari/issues/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](https://github.com/tari-project/tari/issues/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. --- .../sync/horizon_state_sync/synchronizer.rs | 1 - .../src/chain_storage/blockchain_database.rs | 2 -- base_layer/mmr/src/functions.rs | 1 - base_layer/mmr/src/mutable_mmr.rs | 18 ++++++++++-------- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs b/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs index 06fa4ba45a..0dede2033a 100644 --- a/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs +++ b/base_layer/core/src/base_node/sync/horizon_state_sync/synchronizer.rs @@ -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())?; diff --git a/base_layer/core/src/chain_storage/blockchain_database.rs b/base_layer/core/src/chain_storage/blockchain_database.rs index 3c8a8da837..ed851c4d11 100644 --- a/base_layer/core/src/chain_storage/blockchain_database.rs +++ b/base_layer/core/src/chain_storage/blockchain_database.rs @@ -1360,8 +1360,6 @@ pub fn calculate_mmr_roots( } } - 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 { diff --git a/base_layer/mmr/src/functions.rs b/base_layer/mmr/src/functions.rs index f16800c231..33ebe42432 100644 --- a/base_layer/mmr/src/functions.rs +++ b/base_layer/mmr/src/functions.rs @@ -95,7 +95,6 @@ where for index in deletions { pruned_mmr.delete(index); } - pruned_mmr.compress(); pruned_mmr.get_merkle_root() } diff --git a/base_layer/mmr/src/mutable_mmr.rs b/base_layer/mmr/src/mutable_mmr.rs index 2397c20c3f..37869fdd1c 100644 --- a/base_layer/mmr/src/mutable_mmr.rs +++ b/base_layer/mmr/src/mutable_mmr.rs @@ -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 @@ -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 { let mut deleted = self.deleted.clone();