fix(mutator set): Fix flaky test

The problem was located in `Block::accumulate_transaction`, which is invoked repeatedly by a miner as he selects which mempool transactions to include in his block. Recall that the block has only one transaction, which represents the merger of all transactions of the miner's choice.

The problem is that the block-in-preparation contains the updated mutator set accumulator. When a new transaction is aggregated, the mutator set accumulator is updated *from this intermediate state, with the new transaction's addition and removal records*. It is not obvious why this leads to a problem, so let's use an example:

 - The current transaction has `inputs_1` and `outputs_1` and the current block-in-preparation has mutator set accumulator `msa_1`, which is what results from applying `outputs_1` and `inputs_1` to the previous block's mutator set accumulator: `msa_1 = msa_0.add(outputs_1).remove(inputs_1)`.
 - The new transaction has `inputs_2` and `outputs_2`. The `accumulate_transaction` (up until now) computes the new mutator set accumulator as `msa_1.add(outputs_2).remove(inputs_2)`.
 - As far as mutator set accumulators go, additions commute with removals. The subtle caveat for this otherwise true statement is that it does require the removal records to be in sync with the mutator set at the time they are applied. If the removal records are not in sync, computing the next mutator set accumulator might fail. Indeed, this is what happened.
 - The removal records of the incoming transaction are not in sync with the mutator set accumulator `msa_1`.

The solution here is to modify the type signature of the function `accumulate_transaction`, which now takes an additional argument `previous_mutator_set_accumulator`. The new mutator set is computed by starting from this value and applying all addition records (`outputs_1 || outputs_2`) first and all removal records (`inputs_1 || inputs_2`) second. The halfway-in-between mutator set accumulator is ignored.

Also:
 1. Drops the generic type argument `H` from all structs and functions associated with the mutator set. Throughout this code base we only ever use `Tip5` which itself is already hidden behind type alias `Hash`. The genericity dates to the time where we needed an alternative to the relatively slow Rescue-Prime hash functino.
 2. Removes the return values of type `Result<(),_>` from functions where there is no control path leading to error. Accordingly catch-and-report-error clauses where these functions are called, are removed as well.

src/mine_loop.rs

@@ -50,7 +50,7 @@ fn make_block_template(
 ) -> (BlockHeader, BlockBody) {
     let additions = transaction.kernel.outputs.clone();
     let removals = transaction.kernel.inputs.clone();
-    let mut next_mutator_set_accumulator: MutatorSetAccumulator<Hash> =
+    let mut next_mutator_set_accumulator: MutatorSetAccumulator =
         previous_block.kernel.body.mutator_set_accumulator.clone();
 
     // Apply the mutator set update to the mutator set accumulator
@@ -176,7 +176,7 @@ fn make_coinbase_transaction(
     receiver_digest: Digest,
     wallet_secret: &WalletSecret,
     block_height: BlockHeight,
-    mutator_set_accumulator: MutatorSetAccumulator<Hash>,
+    mutator_set_accumulator: MutatorSetAccumulator,
 ) -> (Transaction, Digest) {
     let sender_randomness: Digest =
         wallet_secret.generate_sender_randomness(block_height, receiver_digest);
@@ -190,7 +190,7 @@ fn make_coinbase_transaction(
                 .expect("Make coinbase transaction: failed to parse coin state as amount.")
         })
         .sum();
-    let coinbase_addition_record = commit::<Hash>(
+    let coinbase_addition_record = commit(
         Hash::hash(coinbase_utxo),
         sender_randomness,
         receiver_digest,

src/models/blockchain/block/block_body.rs

@@ -37,7 +37,7 @@ pub struct BlockBody {
     /// The mutator set accumulator represents the UTXO set. It is simultaneously an
     /// accumulator (=> compact representation and membership proofs) and an anonymity
     /// construction (=> outputs from one transaction do not look like inputs to another).
-    pub mutator_set_accumulator: MutatorSetAccumulator<Hash>,
+    pub mutator_set_accumulator: MutatorSetAccumulator,
 
     /// Lock-free UTXOs do not come with lock scripts and do not live in the mutator set.
     pub lock_free_mmr_accumulator: MmrAccumulator<Hash>,

src/models/blockchain/block/mod.rs

@@ -121,7 +121,7 @@ impl Block {
     }
 
     pub fn genesis_block() -> Self {
-        let mut genesis_mutator_set = MutatorSetAccumulator::<Hash>::default();
+        let mut genesis_mutator_set = MutatorSetAccumulator::default();
         let mut ms_update = MutatorSetUpdate::default();
 
         let premine_distribution = Self::premine_distribution();
@@ -141,7 +141,7 @@ impl Block {
                 timestamp,
                 public_announcements: vec![],
                 coinbase: Some(total_premine_amount),
-                mutator_set_hash: MutatorSetAccumulator::<Hash>::new().hash(),
+                mutator_set_hash: MutatorSetAccumulator::new().hash(),
             },
             witness: Witness::Faith,
         };
@@ -157,7 +157,7 @@ impl Block {
             let receiver_digest = receiving_address.privacy_digest;
 
             // Add pre-mine UTXO to MutatorSet
-            let addition_record = commit::<Hash>(utxo_digest, bad_randomness, receiver_digest);
+            let addition_record = commit(utxo_digest, bad_randomness, receiver_digest);
             ms_update.additions.push(addition_record);
             genesis_mutator_set.add(&addition_record);
 
@@ -215,33 +215,50 @@ impl Block {
 
     /// Merge a transaction into this block's transaction.
     /// The mutator set data must be valid in all inputs.
-    pub fn accumulate_transaction(&mut self, transaction: Transaction) {
-        // merge
+    pub fn accumulate_transaction(
+        &mut self,
+        transaction: Transaction,
+        old_mutator_set_accumulator: &MutatorSetAccumulator,
+    ) {
+        // merge transactions
         let merged_timestamp = BFieldElement::new(max(
             self.kernel.header.timestamp.value(),
             max(
                 self.kernel.body.transaction.kernel.timestamp.value(),
                 transaction.kernel.timestamp.value(),
             ),
         ));
+        let new_transaction = self
+            .kernel
+            .body
+            .transaction
+            .clone()
+            .merge_with(transaction.clone());
 
-        // accumulate
-        let mut next_mutator_set_accumulator = self.kernel.body.mutator_set_accumulator.clone();
-
+        // accumulate mutator set updates
+        // Can't use the current mutator sat accumulator because it is in an in-between state.
+        let mut new_mutator_set_accumulator = old_mutator_set_accumulator.clone();
         let mutator_set_update = MutatorSetUpdate::new(
-            transaction.kernel.inputs.clone(),
-            transaction.kernel.outputs.clone(),
+            [
+                self.kernel.body.transaction.kernel.inputs.clone(),
+                transaction.kernel.inputs,
+            ]
+            .concat(),
+            [
+                self.kernel.body.transaction.kernel.outputs.clone(),
+                transaction.kernel.outputs.clone(),
+            ]
+            .concat(),
         );
 
-        let new_transaction = self.kernel.body.transaction.clone().merge_with(transaction);
         // Apply the mutator set update to get the `next_mutator_set_accumulator`
         mutator_set_update
-            .apply(&mut next_mutator_set_accumulator)
+            .apply(&mut new_mutator_set_accumulator)
             .expect("Mutator set mutation must work");
 
         let block_body: BlockBody = BlockBody {
             transaction: new_transaction,
-            mutator_set_accumulator: next_mutator_set_accumulator.clone(),
+            mutator_set_accumulator: new_mutator_set_accumulator.clone(),
             lock_free_mmr_accumulator: self.kernel.body.lock_free_mmr_accumulator.clone(),
             block_mmr_accumulator: self.kernel.body.block_mmr_accumulator.clone(),
             uncle_blocks: self.kernel.body.uncle_blocks.clone(),
@@ -560,7 +577,7 @@ mod block_tests {
             .unwrap();
         assert!(new_tx.is_valid(), "Created tx must be valid");
 
-        block_1.accumulate_transaction(new_tx);
+        block_1.accumulate_transaction(new_tx, &genesis_block.kernel.body.mutator_set_accumulator);
         assert!(
             block_1.is_valid(&genesis_block),
             "Block 1 must be valid after adding a transaction; previous mutator set hash: {} and next mutator set hash: {}",

src/models/blockchain/block/mutator_set_update.rs

@@ -1,25 +1,22 @@
-use anyhow::{bail, Result};
+use anyhow::Result;
 
 use serde::{Deserialize, Serialize};
 
-use crate::{
-    models::blockchain::shared::Hash,
-    util_types::mutator_set::{
-        addition_record::AdditionRecord, mutator_set_accumulator::MutatorSetAccumulator,
-        mutator_set_trait::MutatorSet, removal_record::RemovalRecord,
-    },
+use crate::util_types::mutator_set::{
+    addition_record::AdditionRecord, mutator_set_accumulator::MutatorSetAccumulator,
+    mutator_set_trait::MutatorSet, removal_record::RemovalRecord,
 };
 
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Default)]
 pub struct MutatorSetUpdate {
     // The ordering of the removal/addition records must match that of
     // the block.
-    pub removals: Vec<RemovalRecord<Hash>>,
+    pub removals: Vec<RemovalRecord>,
     pub additions: Vec<AdditionRecord>,
 }
 
 impl MutatorSetUpdate {
-    pub fn new(removals: Vec<RemovalRecord<Hash>>, additions: Vec<AdditionRecord>) -> Self {
+    pub fn new(removals: Vec<RemovalRecord>, additions: Vec<AdditionRecord>) -> Self {
         Self {
             additions,
             removals,
@@ -28,33 +25,24 @@ impl MutatorSetUpdate {
 
     /// Apply a mutator set update to a mutator set accumulator. Changes the input mutator set
     /// accumulator according to the provided additions and removals.
-    pub fn apply(&self, ms_accumulator: &mut MutatorSetAccumulator<Hash>) -> Result<()> {
+    pub fn apply(&self, ms_accumulator: &mut MutatorSetAccumulator) -> Result<()> {
         let mut addition_records: Vec<AdditionRecord> = self.additions.clone();
         addition_records.reverse();
         let mut removal_records = self.removals.clone();
         removal_records.reverse();
-        let mut removal_records: Vec<&mut RemovalRecord<Hash>> =
+        let mut removal_records: Vec<&mut RemovalRecord> =
             removal_records.iter_mut().collect::<Vec<_>>();
         while let Some(addition_record) = addition_records.pop() {
-            let update_res = RemovalRecord::batch_update_from_addition(
+            RemovalRecord::batch_update_from_addition(
                 &mut removal_records,
                 &mut ms_accumulator.kernel,
             );
 
-            if update_res.is_err() {
-                bail!("Failed to update removal records with addition record");
-            }
-
             ms_accumulator.add(&addition_record);
         }
 
         while let Some(removal_record) = removal_records.pop() {
-            let update_res =
-                RemovalRecord::batch_update_from_remove(&mut removal_records, removal_record);
-
-            if update_res.is_err() {
-                bail!("Failed to update removal records with addition record");
-            }
+            RemovalRecord::batch_update_from_remove(&mut removal_records, removal_record);
 
             ms_accumulator.remove(removal_record);
         }

src/models/blockchain/block/validity/correct_mutator_set_update.rs

@@ -6,16 +6,13 @@ use tasm_lib::{
 };
 
 use crate::{
-    models::{
-        blockchain::shared::Hash,
-        consensus::{SecretWitness, SupportedClaim},
-    },
+    models::consensus::{SecretWitness, SupportedClaim},
     util_types::mutator_set::mutator_set_accumulator::MutatorSetAccumulator,
 };
 
 #[derive(Debug, Clone, BFieldCodec, GetSize, PartialEq, Eq, Serialize, Deserialize)]
 pub struct CorrectMutatorSetUpdateWitness {
-    previous_mutator_set_accumulator: MutatorSetAccumulator<Hash>,
+    previous_mutator_set_accumulator: MutatorSetAccumulator,
 }
 
 impl SecretWitness for CorrectMutatorSetUpdateWitness {

src/models/blockchain/transaction/mod.rs

@@ -58,10 +58,10 @@ pub struct TransactionPrimitiveWitness {
     pub input_lock_scripts: Vec<LockScript>,
     pub type_scripts: Vec<TypeScript>,
     pub lock_script_witnesses: Vec<Vec<BFieldElement>>,
-    pub input_membership_proofs: Vec<MsMembershipProof<Hash>>,
+    pub input_membership_proofs: Vec<MsMembershipProof>,
     pub output_utxos: Vec<Utxo>,
     pub public_announcements: Vec<PublicAnnouncement>,
-    pub mutator_set_accumulator: MutatorSetAccumulator<Hash>,
+    pub mutator_set_accumulator: MutatorSetAccumulator,
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, GetSize, BFieldCodec)]
@@ -89,18 +89,18 @@ impl Transaction {
     /// witnesses the witness data can be and is updated.
     pub fn update_mutator_set_records(
         &mut self,
-        previous_mutator_set_accumulator: &MutatorSetAccumulator<Hash>,
+        previous_mutator_set_accumulator: &MutatorSetAccumulator,
         block: &Block,
     ) -> Result<()> {
-        let mut msa_state: MutatorSetAccumulator<Hash> = previous_mutator_set_accumulator.clone();
+        let mut msa_state: MutatorSetAccumulator = previous_mutator_set_accumulator.clone();
         let block_addition_records: Vec<AdditionRecord> =
             block.kernel.body.transaction.kernel.outputs.clone();
-        let mut transaction_removal_records: Vec<RemovalRecord<Hash>> = self.kernel.inputs.clone();
-        let mut transaction_removal_records: Vec<&mut RemovalRecord<Hash>> =
+        let mut transaction_removal_records: Vec<RemovalRecord> = self.kernel.inputs.clone();
+        let mut transaction_removal_records: Vec<&mut RemovalRecord> =
             transaction_removal_records.iter_mut().collect();
         let mut block_removal_records = block.kernel.body.transaction.kernel.inputs.clone();
         block_removal_records.reverse();
-        let mut block_removal_records: Vec<&mut RemovalRecord<Hash>> =
+        let mut block_removal_records: Vec<&mut RemovalRecord> =
             block_removal_records.iter_mut().collect::<Vec<_>>();
 
         // Apply all addition records in the block
@@ -109,15 +109,13 @@ impl Transaction {
             RemovalRecord::batch_update_from_addition(
                 &mut block_removal_records,
                 &mut msa_state.kernel,
-            )
-            .expect("MS removal record update from add must succeed in wallet handler");
+            );
 
             // Batch update transaction's removal records
             RemovalRecord::batch_update_from_addition(
                 &mut transaction_removal_records,
                 &mut msa_state.kernel,
-            )
-            .expect("MS removal record update from add must succeed in wallet handler");
+            );
 
             // Batch update primitive witness membership proofs
             if let Witness::Primitive(witness) = &mut self.witness {
@@ -138,16 +136,14 @@ impl Transaction {
 
         while let Some(removal_record) = block_removal_records.pop() {
             // Batch update block's removal records to keep them valid after next removal
-            RemovalRecord::batch_update_from_remove(&mut block_removal_records, removal_record)
-                .expect("MS removal record update from remove must succeed in wallet handler");
+            RemovalRecord::batch_update_from_remove(&mut block_removal_records, removal_record);
 
             // batch update transaction's removal records
             // Batch update block's removal records to keep them valid after next removal
             RemovalRecord::batch_update_from_remove(
                 &mut transaction_removal_records,
                 removal_record,
-            )
-            .expect("MS removal record update from remove must succeed in wallet handler");
+            );
 
             // Batch update primitive witness membership proofs
             if let Witness::Primitive(witness) = &mut self.witness {
@@ -322,7 +318,7 @@ impl Transaction {
     /// window Bloom filter, and whether the MMR membership proofs are valid.
     pub fn is_confirmable_relative_to(
         &self,
-        mutator_set_accumulator: &MutatorSetAccumulator<Hash>,
+        mutator_set_accumulator: &MutatorSetAccumulator,
     ) -> bool {
         self.kernel
             .inputs
@@ -532,7 +528,7 @@ mod transaction_tests {
             coins: NeptuneCoins::new(42).to_native_coins(),
             lock_script_hash: LockScript::anyone_can_spend().hash(),
         };
-        let ar = commit::<Hash>(Hash::hash(&output_1), random(), random());
+        let ar = commit(Hash::hash(&output_1), random(), random());
 
         // Verify that a sane timestamp is returned. `make_mock_transaction` must follow
         // the correct time convention for this test to work.

src/models/blockchain/transaction/transaction_kernel.rs

@@ -13,12 +13,9 @@ use twenty_first::shared_math::{
 };
 
 use super::{neptune_coins::pseudorandom_amount, NeptuneCoins, PublicAnnouncement};
-use crate::{
-    util_types::mutator_set::{
-        addition_record::{pseudorandom_addition_record, AdditionRecord},
-        removal_record::{pseudorandom_removal_record, RemovalRecord},
-    },
-    Hash,
+use crate::util_types::mutator_set::{
+    addition_record::{pseudorandom_addition_record, AdditionRecord},
+    removal_record::{pseudorandom_removal_record, RemovalRecord},
 };
 
 pub fn pseudorandom_public_announcement(seed: [u8; 32]) -> PublicAnnouncement {
@@ -30,7 +27,7 @@ pub fn pseudorandom_public_announcement(seed: [u8; 32]) -> PublicAnnouncement {
 
 #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, GetSize, BFieldCodec, TasmObject)]
 pub struct TransactionKernel {
-    pub inputs: Vec<RemovalRecord<Hash>>,
+    pub inputs: Vec<RemovalRecord>,
 
     // `outputs` contains the commitments (addition records) that go into the AOCL
     pub outputs: Vec<AdditionRecord>,

src/models/blockchain/transaction/validity/removal_records_integrity.rs

@@ -40,7 +40,7 @@ use crate::{
 )]
 pub struct RemovalRecordsIntegrityWitness {
     pub input_utxos: Vec<Utxo>,
-    pub membership_proofs: Vec<MsMembershipProof<Hash>>,
+    pub membership_proofs: Vec<MsMembershipProof>,
     pub aocl: MmrAccumulator<Hash>,
     pub swbfi: MmrAccumulator<Hash>,
     pub swbfa_hash: Digest,

src/models/blockchain/transaction/validity/tasm/compute_canonical_commitment.rs

@@ -45,7 +45,7 @@ impl BasicSnippet for ComputeCanonicalCommitment {
     }
 
     fn code(&self, library: &mut Library) -> Vec<triton_vm::instruction::LabelledInstruction> {
-        type MsMpH = MsMembershipProof<Hash>;
+        type MsMpH = MsMembershipProof;
         let mp_to_sr = tasm_lib::field!(MsMpH::sender_randomness);
         let mp_to_rp = tasm_lib::field!(MsMpH::receiver_preimage);
         let commit = library.import(Box::new(Commit));
@@ -135,7 +135,7 @@ impl Function for ComputeCanonicalCommitment {
         }
 
         // decode object
-        let membership_proof = *MsMembershipProof::<Hash>::decode(&encoding).unwrap();
+        let membership_proof = *MsMembershipProof::decode(&encoding).unwrap();
 
         // compute commitment
         println!("receiver_preimage: {}", membership_proof.receiver_preimage);
@@ -146,7 +146,7 @@ impl Function for ComputeCanonicalCommitment {
             membership_proof.sender_randomness
         );
         println!("\nitem:\n{}", item);
-        let c = commit::<Hash>(item, membership_proof.sender_randomness, receiver_digest);
+        let c = commit(item, membership_proof.sender_randomness, receiver_digest);
 
         // push onto stack
         stack.push(mp_pointer);
@@ -165,7 +165,7 @@ impl Function for ComputeCanonicalCommitment {
         let mut rng: StdRng = SeedableRng::from_seed(seed);
 
         // generate random ms membership proof object
-        let membership_proof = pseudorandom_mutator_set_membership_proof::<Hash>(rng.gen());
+        let membership_proof = pseudorandom_mutator_set_membership_proof(rng.gen());
 
         // populate memory, with the size of the encoding prepended
         let address = BFieldElement::new(rng.next_u64() % (1 << 20));