From 26d44690784fc8637e36727fd951cb5798788bae Mon Sep 17 00:00:00 2001 From: sword_smith Date: Fri, 22 Nov 2024 13:11:28 +0100 Subject: [PATCH] style(MutatorSetUpdate): Apply PR feedback Co-authored-by: Alan Szepieniec --- src/models/blockchain/block/mod.rs | 4 +- .../blockchain/block/mutator_set_update.rs | 44 +++++++++++++++---- .../block/validity/block_program.rs | 11 +++-- 3 files changed, 44 insertions(+), 15 deletions(-) diff --git a/src/models/blockchain/block/mod.rs b/src/models/blockchain/block/mod.rs index 8bc95f9fc..13634a8c1 100644 --- a/src/models/blockchain/block/mod.rs +++ b/src/models/blockchain/block/mod.rs @@ -219,7 +219,7 @@ impl Block { Block::new(header, body, appendix, proof) } - pub(crate) async fn block_template_from_primitive_witness( + pub(crate) async fn block_template_from_block_primitive_witness( primitive_witness: BlockPrimitiveWitness, timestamp: Timestamp, nonce_preimage: Digest, @@ -268,7 +268,7 @@ impl Block { "Transaction proof must be valid to generate a block" ); let primitive_witness = BlockPrimitiveWitness::new(predecessor.to_owned(), transaction); - Self::block_template_from_primitive_witness( + Self::block_template_from_block_primitive_witness( primitive_witness, block_timestamp, nonce_preimage, diff --git a/src/models/blockchain/block/mutator_set_update.rs b/src/models/blockchain/block/mutator_set_update.rs index 88ad3c47e..cd87ec94f 100644 --- a/src/models/blockchain/block/mutator_set_update.rs +++ b/src/models/blockchain/block/mutator_set_update.rs @@ -27,7 +27,8 @@ impl MutatorSetUpdate { /// could be removed. In other words: This does not check if double spend is /// happening. pub(crate) fn apply_to_accumulator_unsafe(&self, ms_accumulator: &mut MutatorSetAccumulator) { - self.apply_to_accumulator_and_records_inner(ms_accumulator, &mut [], false).expect("This function shouldn't be allowed to fail, as we're not checking for double spends") + let _valid_removal_records = + self.apply_to_accumulator_and_records_inner(ms_accumulator, &mut []); } /// Apply a mutator-set-update to a mutator-set-accumulator. @@ -39,7 +40,13 @@ impl MutatorSetUpdate { /// /// Returns an error if some removal record could not be removed. pub fn apply_to_accumulator(&self, ms_accumulator: &mut MutatorSetAccumulator) -> Result<()> { - self.apply_to_accumulator_and_records_inner(ms_accumulator, &mut [], true) + let valid_removal_records = + self.apply_to_accumulator_and_records_inner(ms_accumulator, &mut []); + if valid_removal_records { + Ok(()) + } else { + bail!("Cannot remove item from mutator set."); + } } /// Apply a mutator-set-update to a mutator-set-accumulator and a bunch of @@ -52,21 +59,39 @@ impl MutatorSetUpdate { /// /// # Return Value /// - /// Returns an error if some removal record could not be removed. + /// Returns an error if some removal record could not be removed. This + /// return value **must** be verified to be OK. If it is not, then the + /// mutator set will be in an invalid state. pub fn apply_to_accumulator_and_records( &self, ms_accumulator: &mut MutatorSetAccumulator, removal_records: &mut [&mut RemovalRecord], ) -> Result<()> { - self.apply_to_accumulator_and_records_inner(ms_accumulator, removal_records, true) + let valid_removal_records = + self.apply_to_accumulator_and_records_inner(ms_accumulator, removal_records); + if valid_removal_records { + Ok(()) + } else { + bail!("Cannot remove item from mutator set."); + } } + /// Apply a mutator set update to a mutator set accumulator. Modifies the + /// mutator set according to the content of the mutator set update and + /// returns a boolean indicating if all removal records were valid. + /// + /// If this boolean is false, then at least one removal record was invalid + /// which could for example mean a double-spend, or an invalid MMR + /// membership proof into the sliding-window Bloom filter. + /// + /// This function should *not* be made public, as the caller should always + /// explicitly decide if they want the safe or unsafe version which checks + /// the returned boolean. fn apply_to_accumulator_and_records_inner( &self, ms_accumulator: &mut MutatorSetAccumulator, removal_records: &mut [&mut RemovalRecord], - check_for_double_spend: bool, - ) -> Result<()> { + ) -> bool { let mut cloned_removals = self.removals.clone(); let mut applied_removal_records = cloned_removals.iter_mut().rev().collect::>(); for addition_record in self.additions.iter() { @@ -77,6 +102,7 @@ impl MutatorSetUpdate { ms_accumulator.add(addition_record); } + let mut removal_records_are_valid = true; while let Some(applied_removal_record) = applied_removal_records.pop() { RemovalRecord::batch_update_from_remove( &mut applied_removal_records, @@ -85,13 +111,13 @@ impl MutatorSetUpdate { RemovalRecord::batch_update_from_remove(removal_records, applied_removal_record); - if check_for_double_spend && !ms_accumulator.can_remove(applied_removal_record) { - bail!("Cannot remove item from mutator set."); + if !ms_accumulator.can_remove(applied_removal_record) { + removal_records_are_valid = false; } ms_accumulator.remove(applied_removal_record); } - Ok(()) + removal_records_are_valid } } diff --git a/src/models/blockchain/block/validity/block_program.rs b/src/models/blockchain/block/validity/block_program.rs index eb93282cf..e8a03f36e 100644 --- a/src/models/blockchain/block/validity/block_program.rs +++ b/src/models/blockchain/block/validity/block_program.rs @@ -182,9 +182,12 @@ pub(crate) mod test { use triton_vm::prelude::Digest; use super::*; - use crate::job_queue::triton_vm::{TritonVmJobPriority, TritonVmJobQueue}; + use crate::job_queue::triton_vm::TritonVmJobPriority; + use crate::job_queue::triton_vm::TritonVmJobQueue; use crate::models::blockchain::block::validity::block_primitive_witness::test::deterministic_block_primitive_witness; - use crate::models::blockchain::block::{Block, BlockPrimitiveWitness, TritonVmProofJobOptions}; + use crate::models::blockchain::block::Block; + use crate::models::blockchain::block::BlockPrimitiveWitness; + use crate::models::blockchain::block::TritonVmProofJobOptions; use crate::models::blockchain::transaction::Transaction; use crate::models::proof_abstractions::mast_hash::MastHash; use crate::models::proof_abstractions::timestamp::Timestamp; @@ -248,7 +251,7 @@ pub(crate) mod test { let rt = tokio::runtime::Runtime::new().unwrap(); let _guard = rt.enter(); let current_block = rt - .block_on(Block::block_template_from_primitive_witness( + .block_on(Block::block_template_from_block_primitive_witness( current_pw, mock_now, Digest::default(), @@ -278,7 +281,7 @@ pub(crate) mod test { let mock_later = mock_now + Timestamp::hours(3); let next_pw = BlockPrimitiveWitness::new(current_block.clone(), updated_tx); let next_block = rt - .block_on(Block::block_template_from_primitive_witness( + .block_on(Block::block_template_from_block_primitive_witness( next_pw, mock_later, Digest::default(),