Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(block-producer): borrow instead of cloning txs #544

Merged
merged 2 commits into from
Nov 27, 2024
Merged

feat(block-producer): borrow instead of cloning txs #544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcea9c9
Borrow and don't clone transactions
Mirko-von-Leipzig Nov 5, 2024
685d233
refactor: simplify solution (#550)
polydez Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 41 additions & 35 deletions crates/block-producer/src/batch_builder/batch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +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: 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) => {
Expand Down Expand Up @@ -121,26 +125,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();
Expand Down Expand Up @@ -208,8 +214,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());
}
Expand All @@ -224,7 +230,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 {
Expand Down Expand Up @@ -283,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:?}"
Expand All @@ -297,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())
},
Expand All @@ -308,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);

Expand All @@ -333,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())
},
Expand All @@ -351,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
Expand Down Expand Up @@ -395,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()];
Expand Down
12 changes: 4 additions & 8 deletions crates/block-producer/src/batch_builder/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
38 changes: 17 additions & 21 deletions crates/block-producer/src/block_builder/prover/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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]
Expand Down Expand Up @@ -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(),
Expand All @@ -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(),
Expand Down Expand Up @@ -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]
};
Expand Down Expand Up @@ -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]
};
Expand Down Expand Up @@ -474,7 +470,7 @@ async fn test_compute_note_root_empty_notes_success() {
.unwrap();

let batches: Vec<TransactionBatch> = {
let batch = TransactionBatch::new(vec![], Default::default()).unwrap();
let batch = TransactionBatch::new([], Default::default()).unwrap();
vec![batch]
};

Expand Down Expand Up @@ -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]
};
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down
4 changes: 2 additions & 2 deletions crates/block-producer/src/test_utils/batch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -36,6 +36,6 @@ impl TransactionBatchConstructor for TransactionBatch {
})
.collect();

Self::new(txs, Default::default()).unwrap()
Self::new(&txs, Default::default()).unwrap()
}
}
Loading