From fcea9c9391b58927479394013eda290a4f4d0507 Mon Sep 17 00:00:00 2001 From: Mirko von Leipzig Date: Tue, 5 Nov 2024 17:01:11 +0200 Subject: [PATCH 1/2] Borrow and don't clone transactions --- .../block-producer/src/batch_builder/batch.rs | 75 +++++++++++-------- .../block-producer/src/batch_builder/mod.rs | 12 +-- .../src/block_builder/prover/tests.rs | 2 +- 3 files changed, 49 insertions(+), 40 deletions(-) diff --git a/crates/block-producer/src/batch_builder/batch.rs b/crates/block-producer/src/batch_builder/batch.rs index ee3b7fbb..1c813f76 100644 --- a/crates/block-producer/src/batch_builder/batch.rs +++ b/crates/block-producer/src/batch_builder/batch.rs @@ -1,4 +1,5 @@ use std::{ + borrow::Borrow, collections::{btree_map::Entry, BTreeMap, BTreeSet}, mem, }; @@ -81,17 +82,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, + pub fn new( + txs: impl IntoIterator + Clone, found_unauthenticated_notes: NoteAuthenticationInfo, - ) -> Result { - let id = Self::compute_id(&txs); + ) -> Result + where + T: Borrow, + { + let id = Self::compute_id(txs.clone().into_iter()); // Populate batch output notes and updated accounts. - let mut output_notes = OutputNoteTracker::new(&txs)?; + let mut output_notes = OutputNoteTracker::new(txs.clone().into_iter())?; let mut updated_accounts = BTreeMap::::new(); let mut unauthenticated_input_notes = BTreeSet::new(); - for tx in &txs { + for tx in txs.clone().into_iter() { + let tx = tx.borrow(); // 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 +126,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 txs.into_iter() { + for input_note in tx.borrow().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,10 +215,13 @@ impl TransactionBatch { // HELPER FUNCTIONS // -------------------------------------------------------------------------------------------- - fn compute_id(txs: &[ProvenTransaction]) -> BatchId { - let mut buf = Vec::with_capacity(32 * txs.len()); + fn compute_id(txs: impl Iterator) -> BatchId + where + T: Borrow, + { + let mut buf = Vec::with_capacity(32 * txs.size_hint().0); for tx in txs { - buf.extend_from_slice(&tx.id().as_bytes()); + buf.extend_from_slice(&tx.borrow().id().as_bytes()); } Blake3_256::hash(&buf) } @@ -224,11 +234,14 @@ struct OutputNoteTracker { } impl OutputNoteTracker { - fn new(txs: &[ProvenTransaction]) -> Result { + fn new(txs: impl IntoIterator) -> Result + where + T: Borrow, + { let mut output_notes = vec![]; let mut output_note_index = BTreeMap::new(); for tx in txs { - for note in tx.output_notes().iter() { + for note in tx.borrow().output_notes().iter() { if output_note_index.insert(note.id(), output_notes.len()).is_some() { return Err(BuildBatchError::DuplicateOutputNote(note.id())); } diff --git a/crates/block-producer/src/batch_builder/mod.rs b/crates/block-producer/src/batch_builder/mod.rs index 830c68c3..528600af 100644 --- a/crates/block-producer/src/batch_builder/mod.rs +++ b/crates/block-producer/src/batch_builder/mod.rs @@ -177,15 +177,11 @@ 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(); // TODO: Found unauthenticated notes are no longer required.. potentially? - let batch = TransactionBatch::new(txs, Default::default())?; + let batch = TransactionBatch::new( + txs.iter().map(AuthenticatedTransaction::raw_proven_transaction), + Default::default(), + )?; Span::current().record("batch_id", format_blake3_digest(batch.id())); info!(target: COMPONENT, "Transaction batch built"); diff --git a/crates/block-producer/src/block_builder/prover/tests.rs b/crates/block-producer/src/block_builder/prover/tests.rs index a28f86f9..1b2342c3 100644 --- a/crates/block-producer/src/block_builder/prover/tests.rs +++ b/crates/block-producer/src/block_builder/prover/tests.rs @@ -474,7 +474,7 @@ async fn test_compute_note_root_empty_notes_success() { .unwrap(); let batches: Vec = { - let batch = TransactionBatch::new(vec![], Default::default()).unwrap(); + let batch = TransactionBatch::new(&vec![], Default::default()).unwrap(); vec![batch] }; From 685d233c2c95f241c407e4114be2d788368ee927 Mon Sep 17 00:00:00 2001 From: polydez <155382956+polydez@users.noreply.github.com> Date: Fri, 15 Nov 2024 16:40:01 +0500 Subject: [PATCH 2/2] refactor: simplify solution (#550) --- .../block-producer/src/batch_builder/batch.rs | 45 ++++++++----------- .../src/block_builder/prover/tests.rs | 38 +++++++--------- crates/block-producer/src/test_utils/batch.rs | 4 +- 3 files changed, 38 insertions(+), 49 deletions(-) diff --git a/crates/block-producer/src/batch_builder/batch.rs b/crates/block-producer/src/batch_builder/batch.rs index 1c813f76..cc6ee351 100644 --- a/crates/block-producer/src/batch_builder/batch.rs +++ b/crates/block-producer/src/batch_builder/batch.rs @@ -1,5 +1,4 @@ use std::{ - borrow::Borrow, collections::{btree_map::Entry, BTreeMap, BTreeSet}, mem, }; @@ -82,21 +81,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: impl IntoIterator + Clone, + pub fn new<'a, I>( + txs: impl IntoIterator, found_unauthenticated_notes: NoteAuthenticationInfo, ) -> Result where - T: Borrow, + I: Iterator + Clone, { - let id = Self::compute_id(txs.clone().into_iter()); + 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.clone().into_iter())?; + let mut output_notes = OutputNoteTracker::new(tx_iter.clone())?; let mut updated_accounts = BTreeMap::::new(); let mut unauthenticated_input_notes = BTreeSet::new(); - for tx in txs.clone().into_iter() { - let tx = tx.borrow(); + 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) => { @@ -126,8 +125,8 @@ impl TransactionBatch { // note `x` (i.e., have a circular dependency between transactions), but this is not // a problem. let mut input_notes = vec![]; - for tx in txs.into_iter() { - for input_note in tx.borrow().input_notes().iter() { + 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) => { @@ -215,13 +214,10 @@ impl TransactionBatch { // HELPER FUNCTIONS // -------------------------------------------------------------------------------------------- - fn compute_id(txs: impl Iterator) -> BatchId - where - T: Borrow, - { + fn compute_id<'a>(txs: impl Iterator) -> BatchId { let mut buf = Vec::with_capacity(32 * txs.size_hint().0); for tx in txs { - buf.extend_from_slice(&tx.borrow().id().as_bytes()); + buf.extend_from_slice(&tx.id().as_bytes()); } Blake3_256::hash(&buf) } @@ -234,14 +230,11 @@ struct OutputNoteTracker { } impl OutputNoteTracker { - fn new(txs: impl IntoIterator) -> Result - where - T: Borrow, - { + fn new<'a>(txs: impl Iterator) -> Result { let mut output_notes = vec![]; let mut output_note_index = BTreeMap::new(); for tx in txs { - for note in tx.borrow().output_notes().iter() { + for note in tx.output_notes().iter() { if output_note_index.insert(note.id(), output_notes.len()).is_some() { return Err(BuildBatchError::DuplicateOutputNote(note.id())); } @@ -296,7 +289,7 @@ mod tests { fn test_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:?}" @@ -310,7 +303,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()) }, @@ -321,7 +314,7 @@ mod tests { #[test] fn test_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); @@ -346,7 +339,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()) }, @@ -364,7 +357,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 @@ -408,7 +401,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()]; diff --git a/crates/block-producer/src/block_builder/prover/tests.rs b/crates/block-producer/src/block_builder/prover/tests.rs index 1b2342c3..0df93aea 100644 --- a/crates/block-producer/src/block_builder/prover/tests.rs +++ b/crates/block-producer/src/block_builder/prover/tests.rs @@ -69,7 +69,7 @@ fn test_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 test_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 test_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 test_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(), @@ -229,12 +229,8 @@ fn test_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] }; @@ -330,8 +326,8 @@ async fn test_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] }; @@ -474,7 +470,7 @@ async fn test_compute_note_root_empty_notes_success() { .unwrap(); let batches: Vec = { - let batch = TransactionBatch::new(&vec![], Default::default()).unwrap(); + let batch = TransactionBatch::new([], Default::default()).unwrap(); vec![batch] }; @@ -548,8 +544,8 @@ async fn test_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] }; @@ -605,13 +601,13 @@ fn test_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] @@ -682,13 +678,13 @@ async fn test_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] @@ -736,13 +732,13 @@ async fn test_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] diff --git a/crates/block-producer/src/test_utils/batch.rs b/crates/block-producer/src/test_utils/batch.rs index 2717a3a7..53a572b9 100644 --- a/crates/block-producer/src/test_utils/batch.rs +++ b/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() } }