style(MutatorSetUpdate): Apply PR feedback

Co-authored-by: Alan Szepieniec <alan@neptune.cash>

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,

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::<Vec<_>>();
         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
     }
 }
 

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(),