feat(block-producer): remove deep clone of tx data (#570)

This was previously added in #544 but was mistakenly dropped by a rebase.

Author: Mirko-von-Leipzig

crates/block-producer/src/batch_builder/batch.rs

@@ -70,9 +70,13 @@ impl TransactionBatch {
     // CONSTRUCTORS
     // --------------------------------------------------------------------------------------------
 
-    /// Returns a new [TransactionBatch] instantiated from the provided vector of proven
-    /// transactions. If a map of unauthenticated notes found in the store is provided, it is used
-    /// for transforming unauthenticated notes into authenticated notes.
+    /// Returns a new [TransactionBatch] built from the provided transactions. If a map of
+    /// unauthenticated notes found in the store is provided, it is used for transforming
+    /// unauthenticated notes into authenticated notes.
+    ///
+    /// The tx input takes an `IntoIterator` of a reference, which effectively allows for cheap
+    /// cloning of the iterator. Or put differently, we want something similar to `impl
+    /// Iterator<Item = ProvenTransaction> + Clone` which this provides.
     ///
     /// # Errors
     ///
@@ -81,17 +85,21 @@ impl TransactionBatch {
     ///   in the batch.
     /// - Hashes for corresponding input notes and output notes don't match.
     #[instrument(target = "miden-block-producer", name = "new_batch", skip_all, err)]
-    pub fn new(
-        txs: Vec<ProvenTransaction>,
+    pub fn new<'a, I>(
+        txs: impl IntoIterator<Item = &'a ProvenTransaction, IntoIter = I>,
         found_unauthenticated_notes: NoteAuthenticationInfo,
-    ) -> Result<Self, BuildBatchError> {
-        let id = Self::compute_id(&txs);
+    ) -> Result<Self, BuildBatchError>
+    where
+        I: Iterator<Item = &'a ProvenTransaction> + Clone,
+    {
+        let tx_iter = txs.into_iter();
+        let id = Self::compute_id(tx_iter.clone());
 
         // Populate batch output notes and updated accounts.
-        let mut output_notes = OutputNoteTracker::new(&txs)?;
+        let mut output_notes = OutputNoteTracker::new(tx_iter.clone())?;
         let mut updated_accounts = BTreeMap::<AccountId, AccountUpdate>::new();
         let mut unauthenticated_input_notes = BTreeSet::new();
-        for tx in &txs {
+        for tx in tx_iter.clone() {
             // Merge account updates so that state transitions A->B->C become A->C.
             match updated_accounts.entry(tx.account_id()) {
                 Entry::Vacant(vacant) => {
@@ -121,26 +129,28 @@ impl TransactionBatch {
         // note `x` (i.e., have a circular dependency between transactions), but this is not
         // a problem.
         let mut input_notes = vec![];
-        for input_note in txs.iter().flat_map(|tx| tx.input_notes().iter()) {
-            // Header is presented only for unauthenticated input notes.
-            let input_note = match input_note.header() {
-                Some(input_note_header) => {
-                    if output_notes.remove_note(input_note_header)? {
-                        continue;
-                    }
-
-                    // If an unauthenticated note was found in the store, transform it to an
-                    // authenticated one (i.e. erase additional note details
-                    // except the nullifier)
-                    if found_unauthenticated_notes.contains_note(&input_note_header.id()) {
-                        InputNoteCommitment::from(input_note.nullifier())
-                    } else {
-                        input_note.clone()
-                    }
-                },
-                None => input_note.clone(),
-            };
-            input_notes.push(input_note)
+        for tx in tx_iter {
+            for input_note in tx.input_notes().iter() {
+                // Header is presented only for unauthenticated input notes.
+                let input_note = match input_note.header() {
+                    Some(input_note_header) => {
+                        if output_notes.remove_note(input_note_header)? {
+                            continue;
+                        }
+
+                        // If an unauthenticated note was found in the store, transform it to an
+                        // authenticated one (i.e. erase additional note details
+                        // except the nullifier)
+                        if found_unauthenticated_notes.contains_note(&input_note_header.id()) {
+                            InputNoteCommitment::from(input_note.nullifier())
+                        } else {
+                            input_note.clone()
+                        }
+                    },
+                    None => input_note.clone(),
+                };
+                input_notes.push(input_note)
+            }
         }
 
         let output_notes = output_notes.into_notes();
@@ -208,8 +218,8 @@ impl TransactionBatch {
     // HELPER FUNCTIONS
     // --------------------------------------------------------------------------------------------
 
-    fn compute_id(txs: &[ProvenTransaction]) -> BatchId {
-        let mut buf = Vec::with_capacity(32 * txs.len());
+    fn compute_id<'a>(txs: impl Iterator<Item = &'a ProvenTransaction>) -> BatchId {
+        let mut buf = Vec::with_capacity(32 * txs.size_hint().0);
         for tx in txs {
             buf.extend_from_slice(&tx.id().as_bytes());
         }
@@ -224,7 +234,7 @@ struct OutputNoteTracker {
 }
 
 impl OutputNoteTracker {
-    fn new(txs: &[ProvenTransaction]) -> Result<Self, BuildBatchError> {
+    fn new<'a>(txs: impl Iterator<Item = &'a ProvenTransaction>) -> Result<Self, BuildBatchError> {
         let mut output_notes = vec![];
         let mut output_note_index = BTreeMap::new();
         for tx in txs {
@@ -283,7 +293,7 @@ mod tests {
     fn output_note_tracker_duplicate_output_notes() {
         let mut txs = mock_proven_txs();
 
-        let result = OutputNoteTracker::new(&txs);
+        let result = OutputNoteTracker::new(txs.iter());
         assert!(
             result.is_ok(),
             "Creation of output note tracker was not expected to fail: {result:?}"
@@ -297,7 +307,7 @@ mod tests {
             vec![duplicate_output_note.clone(), mock_output_note(8), mock_output_note(4)],
         ));
 
-        match OutputNoteTracker::new(&txs) {
+        match OutputNoteTracker::new(txs.iter()) {
             Err(BuildBatchError::DuplicateOutputNote(note_id)) => {
                 assert_eq!(note_id, duplicate_output_note.id())
             },
@@ -308,7 +318,7 @@ mod tests {
     #[test]
     fn output_note_tracker_remove_in_place_consumed_note() {
         let txs = mock_proven_txs();
-        let mut tracker = OutputNoteTracker::new(&txs).unwrap();
+        let mut tracker = OutputNoteTracker::new(txs.iter()).unwrap();
 
         let note_to_remove = mock_note(4);
 
@@ -333,7 +343,7 @@ mod tests {
         let mut txs = mock_proven_txs();
         let duplicate_note = mock_note(5);
         txs.push(mock_proven_tx(4, vec![duplicate_note.clone()], vec![mock_output_note(9)]));
-        match TransactionBatch::new(txs, Default::default()) {
+        match TransactionBatch::new(&txs, Default::default()) {
             Err(BuildBatchError::DuplicateUnauthenticatedNote(note_id)) => {
                 assert_eq!(note_id, duplicate_note.id())
             },
@@ -351,7 +361,7 @@ mod tests {
             vec![mock_output_note(9), mock_output_note(10)],
         ));
 
-        let batch = TransactionBatch::new(txs, Default::default()).unwrap();
+        let batch = TransactionBatch::new(&txs, Default::default()).unwrap();
 
         // One of the unauthenticated notes must be removed from the batch due to the consumption
         // of the corresponding output note
@@ -395,7 +405,7 @@ mod tests {
             note_proofs: found_unauthenticated_notes,
             block_proofs: Default::default(),
         };
-        let batch = TransactionBatch::new(txs, found_unauthenticated_notes).unwrap();
+        let batch = TransactionBatch::new(&txs, found_unauthenticated_notes).unwrap();
 
         let expected_input_notes =
             vec![mock_unauthenticated_note_commitment(1), mock_note(5).nullifier().into()];

crates/block-producer/src/batch_builder/mod.rs

@@ -243,13 +243,7 @@ impl WorkerPool {
         info!(target: COMPONENT, num_txs, "Building a transaction batch");
         debug!(target: COMPONENT, txs = %format_array(txs.iter().map(|tx| tx.id().to_hex())));
 
-        // TODO: This is a deep clone which can be avoided by change batch building to using
-        // refs or arcs.
-        let txs = txs
-            .iter()
-            .map(AuthenticatedTransaction::raw_proven_transaction)
-            .cloned()
-            .collect();
+        let txs = txs.iter().map(AuthenticatedTransaction::raw_proven_transaction);
         // TODO: Found unauthenticated notes are no longer required.. potentially?
         let batch = TransactionBatch::new(txs, Default::default())?;
 

crates/block-producer/src/block_builder/prover/tests.rs

@@ -69,7 +69,7 @@ fn block_witness_validation_inconsistent_account_ids() {
             )
             .build();
 
-            TransactionBatch::new(vec![tx], Default::default()).unwrap()
+            TransactionBatch::new([&tx], Default::default()).unwrap()
         };
 
         let batch_2 = {
@@ -80,7 +80,7 @@ fn block_witness_validation_inconsistent_account_ids() {
             )
             .build();
 
-            TransactionBatch::new(vec![tx], Default::default()).unwrap()
+            TransactionBatch::new([&tx], Default::default()).unwrap()
         };
 
         vec![batch_1, batch_2]
@@ -132,7 +132,7 @@ fn block_witness_validation_inconsistent_account_hashes() {
 
     let batches = {
         let batch_1 = TransactionBatch::new(
-            vec![MockProvenTxBuilder::with_account(
+            [&MockProvenTxBuilder::with_account(
                 account_id_1,
                 account_1_hash_batches,
                 Digest::default(),
@@ -142,7 +142,7 @@ fn block_witness_validation_inconsistent_account_hashes() {
         )
         .unwrap();
         let batch_2 = TransactionBatch::new(
-            vec![MockProvenTxBuilder::with_account(
+            [&MockProvenTxBuilder::with_account(
                 account_id_2,
                 Digest::default(),
                 Digest::default(),
@@ -233,12 +233,8 @@ fn block_witness_multiple_batches_per_account() {
     };
 
     let batches = {
-        let batch_1 =
-            TransactionBatch::new(vec![x_txs[0].clone(), y_txs[1].clone()], Default::default())
-                .unwrap();
-        let batch_2 =
-            TransactionBatch::new(vec![y_txs[0].clone(), x_txs[1].clone()], Default::default())
-                .unwrap();
+        let batch_1 = TransactionBatch::new([&x_txs[0], &y_txs[1]], Default::default()).unwrap();
+        let batch_2 = TransactionBatch::new([&y_txs[0], &x_txs[1]], Default::default()).unwrap();
 
         vec![batch_1, batch_2]
     };
@@ -334,8 +330,8 @@ async fn compute_account_root_success() {
             })
             .collect();
 
-        let batch_1 = TransactionBatch::new(txs[..2].to_vec(), Default::default()).unwrap();
-        let batch_2 = TransactionBatch::new(txs[2..].to_vec(), Default::default()).unwrap();
+        let batch_1 = TransactionBatch::new(&txs[..2], Default::default()).unwrap();
+        let batch_2 = TransactionBatch::new(&txs[2..], Default::default()).unwrap();
 
         vec![batch_1, batch_2]
     };
@@ -552,8 +548,8 @@ async fn compute_note_root_success() {
             })
             .collect();
 
-        let batch_1 = TransactionBatch::new(txs[..2].to_vec(), Default::default()).unwrap();
-        let batch_2 = TransactionBatch::new(txs[2..].to_vec(), Default::default()).unwrap();
+        let batch_1 = TransactionBatch::new(&txs[..2], Default::default()).unwrap();
+        let batch_2 = TransactionBatch::new(&txs[2..], Default::default()).unwrap();
 
         vec![batch_1, batch_2]
     };
@@ -609,13 +605,13 @@ fn block_witness_validation_inconsistent_nullifiers() {
         let batch_1 = {
             let tx = MockProvenTxBuilder::with_account_index(0).nullifiers_range(0..1).build();
 
-            TransactionBatch::new(vec![tx], Default::default()).unwrap()
+            TransactionBatch::new([&tx], Default::default()).unwrap()
         };
 
         let batch_2 = {
             let tx = MockProvenTxBuilder::with_account_index(1).nullifiers_range(1..2).build();
 
-            TransactionBatch::new(vec![tx], Default::default()).unwrap()
+            TransactionBatch::new([&tx], Default::default()).unwrap()
         };
 
         vec![batch_1, batch_2]
@@ -688,13 +684,13 @@ async fn compute_nullifier_root_empty_success() {
         let batch_1 = {
             let tx = MockProvenTxBuilder::with_account_index(0).build();
 
-            TransactionBatch::new(vec![tx], Default::default()).unwrap()
+            TransactionBatch::new([&tx], Default::default()).unwrap()
         };
 
         let batch_2 = {
             let tx = MockProvenTxBuilder::with_account_index(1).build();
 
-            TransactionBatch::new(vec![tx], Default::default()).unwrap()
+            TransactionBatch::new([&tx], Default::default()).unwrap()
         };
 
         vec![batch_1, batch_2]
@@ -742,13 +738,13 @@ async fn compute_nullifier_root_success() {
         let batch_1 = {
             let tx = MockProvenTxBuilder::with_account_index(0).nullifiers_range(0..1).build();
 
-            TransactionBatch::new(vec![tx], Default::default()).unwrap()
+            TransactionBatch::new([&tx], Default::default()).unwrap()
         };
 
         let batch_2 = {
             let tx = MockProvenTxBuilder::with_account_index(1).nullifiers_range(1..2).build();
 
-            TransactionBatch::new(vec![tx], Default::default()).unwrap()
+            TransactionBatch::new([&tx], Default::default()).unwrap()
         };
 
         vec![batch_1, batch_2]

crates/block-producer/src/mempool/mod.rs

@@ -416,11 +416,8 @@ mod tests {
         uut.batch_failed(child_batch_a);
         assert_eq!(uut, reference);
 
-        let proof = TransactionBatch::new(
-            vec![txs[2].raw_proven_transaction().clone()],
-            Default::default(),
-        )
-        .unwrap();
+        let proof =
+            TransactionBatch::new([txs[2].raw_proven_transaction()], Default::default()).unwrap();
         uut.batch_proved(child_batch_b, proof);
         assert_eq!(uut, reference);
     }

crates/block-producer/src/test_utils/batch.rs

@@ -24,7 +24,7 @@ impl TransactionBatchConstructor for TransactionBatch {
             })
             .collect();
 
-        Self::new(txs, Default::default()).unwrap()
+        Self::new(&txs, Default::default()).unwrap()
     }
 
     fn from_txs(starting_account_index: u32, num_txs_in_batch: u64) -> Self {
@@ -36,6 +36,6 @@ impl TransactionBatchConstructor for TransactionBatch {
             })
             .collect();
 
-        Self::new(txs, Default::default()).unwrap()
+        Self::new(&txs, Default::default()).unwrap()
     }
 }