From 53329d7050e1e2eef5f4541c63a0ff40b89e27b8 Mon Sep 17 00:00:00 2001 From: Urix <43704209+uri-99@users.noreply.github.com> Date: Tue, 17 Dec 2024 21:10:14 -0300 Subject: [PATCH 1/8] fix: batcher queue ord --- batcher/aligned-batcher/src/connection.rs | 4 +++- batcher/aligned-batcher/src/types/batch_queue.rs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/batcher/aligned-batcher/src/connection.rs b/batcher/aligned-batcher/src/connection.rs index 9d71925228..404a2edac7 100644 --- a/batcher/aligned-batcher/src/connection.rs +++ b/batcher/aligned-batcher/src/connection.rs @@ -21,7 +21,9 @@ pub(crate) async fn send_batch_inclusion_data_responses( finalized_batch: Vec, batch_merkle_tree: &MerkleTree, ) -> Result<(), BatcherError> { - for (vd_batch_idx, entry) in finalized_batch.iter().enumerate() { + // iter in reverse because each sender wants to receive responses in ascending nonce order + // and finalized_batch is ordered as the PriorityQueue , low max_nonce first && high nonce first. + for (vd_batch_idx, entry) in finalized_batch.iter().enumerate().rev() { let batch_inclusion_data = BatchInclusionData::new( vd_batch_idx, batch_merkle_tree, diff --git a/batcher/aligned-batcher/src/types/batch_queue.rs b/batcher/aligned-batcher/src/types/batch_queue.rs index 17918e7a21..a4da78fb9d 100644 --- a/batcher/aligned-batcher/src/types/batch_queue.rs +++ b/batcher/aligned-batcher/src/types/batch_queue.rs @@ -102,7 +102,7 @@ impl Ord for BatchQueueEntryPriority { fn cmp(&self, other: &Self) -> std::cmp::Ordering { let ord = other.max_fee.cmp(&self.max_fee); if ord == std::cmp::Ordering::Equal { - self.nonce.cmp(&other.nonce).reverse() + self.nonce.cmp(&other.nonce) } else { ord } From 83d9cf4ec6d74efca9fff75da006d2995094f263 Mon Sep 17 00:00:00 2001 From: Urix <43704209+uri-99@users.noreply.github.com> Date: Tue, 17 Dec 2024 22:14:33 -0300 Subject: [PATCH 2/8] feat: add unit tests --- .../aligned-batcher/src/types/batch_queue.rs | 200 ++++++++++++++++++ 1 file changed, 200 insertions(+) diff --git a/batcher/aligned-batcher/src/types/batch_queue.rs b/batcher/aligned-batcher/src/types/batch_queue.rs index a4da78fb9d..7c55267417 100644 --- a/batcher/aligned-batcher/src/types/batch_queue.rs +++ b/batcher/aligned-batcher/src/types/batch_queue.rs @@ -311,6 +311,206 @@ mod test { assert_eq!(batch[2].nonced_verification_data.max_fee, max_fee_1); } + #[test] + fn batch_finalization_algorithm_works_from_same_sender_same_fee() { + // The following information will be the same for each entry, it is just some dummy data to see + // algorithm working. + + let proof_generator_addr = Address::random(); + let payment_service_addr = Address::random(); + let sender_addr = Address::random(); + let bytes_for_verification_data = vec![42_u8; 10]; + let dummy_signature = Signature { + r: U256::from(1), + s: U256::from(2), + v: 3, + }; + let verification_data = VerificationData { + proving_system: ProvingSystemId::Risc0, + proof: bytes_for_verification_data.clone(), + pub_input: Some(bytes_for_verification_data.clone()), + verification_key: Some(bytes_for_verification_data.clone()), + vm_program_code: Some(bytes_for_verification_data), + proof_generator_addr, + }; + let chain_id = U256::from(42); + + // Here we create different entries for the batch queue. + // All with the same fee + + let max_fee = U256::from(130000000000000u128); + + // Entry 1 + let nonce_1 = U256::from(1); + let nonced_verification_data_1 = NoncedVerificationData::new( + verification_data.clone(), + nonce_1, + max_fee, + chain_id, + payment_service_addr, + ); + let vd_commitment_1: VerificationDataCommitment = nonced_verification_data_1.clone().into(); + let entry_1 = BatchQueueEntry::new_for_testing( + nonced_verification_data_1, + vd_commitment_1, + dummy_signature, + sender_addr, + ); + let batch_priority_1 = BatchQueueEntryPriority::new(max_fee, nonce_1); + + // Entry 2 + let nonce_2 = U256::from(2); + let nonced_verification_data_2 = NoncedVerificationData::new( + verification_data.clone(), + nonce_2, + max_fee, + chain_id, + payment_service_addr, + ); + let vd_commitment_2: VerificationDataCommitment = nonced_verification_data_2.clone().into(); + let entry_2 = BatchQueueEntry::new_for_testing( + nonced_verification_data_2, + vd_commitment_2, + dummy_signature, + sender_addr, + ); + let batch_priority_2 = BatchQueueEntryPriority::new(max_fee, nonce_2); + + // Entry 3 + let nonce_3 = U256::from(3); + let nonced_verification_data_3 = NoncedVerificationData::new( + verification_data.clone(), + nonce_3, + max_fee, + chain_id, + payment_service_addr, + ); + let vd_commitment_3: VerificationDataCommitment = nonced_verification_data_3.clone().into(); + let entry_3 = BatchQueueEntry::new_for_testing( + nonced_verification_data_3, + vd_commitment_3, + dummy_signature, + sender_addr, + ); + let batch_priority_3 = BatchQueueEntryPriority::new(max_fee, nonce_3); + + let mut batch_queue = BatchQueue::new(); + batch_queue.push(entry_1, batch_priority_1); + batch_queue.push(entry_2, batch_priority_2); + batch_queue.push(entry_3, batch_priority_3); + + let gas_price = U256::from(1); + let (resulting_batch_queue, batch) = + try_build_batch(batch_queue, gas_price, 5000000, 50).unwrap(); + + assert!(resulting_batch_queue.is_empty()); + + assert_eq!(batch[0].nonced_verification_data.nonce, nonce_3); + assert_eq!(batch[1].nonced_verification_data.nonce, nonce_2); + assert_eq!(batch[2].nonced_verification_data.nonce, nonce_1); + + // sanity check + assert_eq!(batch[2].nonced_verification_data.max_fee, max_fee); + } + + #[test] + fn batch_finalization_algorithm_works_from_same_sender_same_fee_nonempty_resulting_queue() { + // The following information will be the same for each entry, it is just some dummy data to see + // algorithm working. + + let proof_generator_addr = Address::random(); + let payment_service_addr = Address::random(); + let sender_addr = Address::random(); + let bytes_for_verification_data = vec![42_u8; 10]; + let dummy_signature = Signature { + r: U256::from(1), + s: U256::from(2), + v: 3, + }; + let verification_data = VerificationData { + proving_system: ProvingSystemId::Risc0, + proof: bytes_for_verification_data.clone(), + pub_input: Some(bytes_for_verification_data.clone()), + verification_key: Some(bytes_for_verification_data.clone()), + vm_program_code: Some(bytes_for_verification_data), + proof_generator_addr, + }; + let chain_id = U256::from(42); + + // Here we create different entries for the batch queue. + // All with the same fee + + let max_fee = U256::from(130000000000000u128); + + // Entry 1 + let nonce_1 = U256::from(1); + let nonced_verification_data_1 = NoncedVerificationData::new( + verification_data.clone(), + nonce_1, + max_fee, + chain_id, + payment_service_addr, + ); + let vd_commitment_1: VerificationDataCommitment = nonced_verification_data_1.clone().into(); + let entry_1 = BatchQueueEntry::new_for_testing( + nonced_verification_data_1, + vd_commitment_1, + dummy_signature, + sender_addr, + ); + let batch_priority_1 = BatchQueueEntryPriority::new(max_fee, nonce_1); + + // Entry 2 + let nonce_2 = U256::from(2); + let nonced_verification_data_2 = NoncedVerificationData::new( + verification_data.clone(), + nonce_2, + max_fee, + chain_id, + payment_service_addr, + ); + let vd_commitment_2: VerificationDataCommitment = nonced_verification_data_2.clone().into(); + let entry_2 = BatchQueueEntry::new_for_testing( + nonced_verification_data_2, + vd_commitment_2, + dummy_signature, + sender_addr, + ); + let batch_priority_2 = BatchQueueEntryPriority::new(max_fee, nonce_2); + + // Entry 3 + let nonce_3 = U256::from(3); + let nonced_verification_data_3 = NoncedVerificationData::new( + verification_data.clone(), + nonce_3, + max_fee, + chain_id, + payment_service_addr, + ); + let vd_commitment_3: VerificationDataCommitment = nonced_verification_data_3.clone().into(); + let entry_3 = BatchQueueEntry::new_for_testing( + nonced_verification_data_3, + vd_commitment_3, + dummy_signature, + sender_addr, + ); + let batch_priority_3 = BatchQueueEntryPriority::new(max_fee, nonce_3); + + let mut batch_queue = BatchQueue::new(); + batch_queue.push(entry_1, batch_priority_1); + batch_queue.push(entry_2, batch_priority_2); + batch_queue.push(entry_3, batch_priority_3); + + let gas_price = U256::from(1); + let (resulting_batch_queue, batch) = + try_build_batch(batch_queue, gas_price, 5000000, 2).unwrap(); + + assert!(resulting_batch_queue.len() == 1); //nonce_3 + + assert_eq!(batch[0].nonced_verification_data.nonce, nonce_2); + assert_eq!(batch[1].nonced_verification_data.nonce, nonce_1); + } + #[test] fn batch_finalization_algorithm_works_from_different_senders() { // The following information will be the same for each entry, it is just some dummy data to see From 4645e47735b1b94bf25396623da5465112ad8daf Mon Sep 17 00:00:00 2001 From: PatStiles Date: Wed, 18 Dec 2024 01:29:04 -0300 Subject: [PATCH 3/8] fmt + clippy --- batcher/aligned-batcher/src/types/batch_queue.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/batcher/aligned-batcher/src/types/batch_queue.rs b/batcher/aligned-batcher/src/types/batch_queue.rs index 7c55267417..27cae8febe 100644 --- a/batcher/aligned-batcher/src/types/batch_queue.rs +++ b/batcher/aligned-batcher/src/types/batch_queue.rs @@ -339,7 +339,7 @@ mod test { // All with the same fee let max_fee = U256::from(130000000000000u128); - + // Entry 1 let nonce_1 = U256::from(1); let nonced_verification_data_1 = NoncedVerificationData::new( @@ -441,7 +441,7 @@ mod test { // All with the same fee let max_fee = U256::from(130000000000000u128); - + // Entry 1 let nonce_1 = U256::from(1); let nonced_verification_data_1 = NoncedVerificationData::new( From 7e660ade6262d55dad1e4be14a80e68a8b5df25e Mon Sep 17 00:00:00 2001 From: Uriel Mihura <43704209+uri-99@users.noreply.github.com> Date: Wed, 18 Dec 2024 13:24:41 -0300 Subject: [PATCH 4/8] Update batcher/aligned-batcher/src/connection.rs --- batcher/aligned-batcher/src/connection.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/batcher/aligned-batcher/src/connection.rs b/batcher/aligned-batcher/src/connection.rs index 404a2edac7..a06d83c8e7 100644 --- a/batcher/aligned-batcher/src/connection.rs +++ b/batcher/aligned-batcher/src/connection.rs @@ -22,7 +22,7 @@ pub(crate) async fn send_batch_inclusion_data_responses( batch_merkle_tree: &MerkleTree, ) -> Result<(), BatcherError> { // iter in reverse because each sender wants to receive responses in ascending nonce order - // and finalized_batch is ordered as the PriorityQueue , low max_nonce first && high nonce first. + // and finalized_batch is ordered as the PriorityQueue , low max_fee first && high nonce first. for (vd_batch_idx, entry) in finalized_batch.iter().enumerate().rev() { let batch_inclusion_data = BatchInclusionData::new( vd_batch_idx, From 5d5e54240ffd60916610c1574b076be64d712667 Mon Sep 17 00:00:00 2001 From: Urix <43704209+uri-99@users.noreply.github.com> Date: Wed, 18 Dec 2024 14:28:37 -0300 Subject: [PATCH 5/8] refactor: comment --- batcher/aligned-batcher/src/connection.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/batcher/aligned-batcher/src/connection.rs b/batcher/aligned-batcher/src/connection.rs index a06d83c8e7..9a87230abc 100644 --- a/batcher/aligned-batcher/src/connection.rs +++ b/batcher/aligned-batcher/src/connection.rs @@ -21,8 +21,8 @@ pub(crate) async fn send_batch_inclusion_data_responses( finalized_batch: Vec, batch_merkle_tree: &MerkleTree, ) -> Result<(), BatcherError> { - // iter in reverse because each sender wants to receive responses in ascending nonce order - // and finalized_batch is ordered as the PriorityQueue , low max_fee first && high nonce first. + // Finalized_batch is ordered as the PriorityQueue, ordered by: ascending max_fee && if max_fee is equal, by descending nonce. + // We iter it in reverse because each sender wants to receive responses in ascending nonce order for (vd_batch_idx, entry) in finalized_batch.iter().enumerate().rev() { let batch_inclusion_data = BatchInclusionData::new( vd_batch_idx, From f98c13692a0ef7926caed49364ab96c6f659f4ba Mon Sep 17 00:00:00 2001 From: Urix <43704209+uri-99@users.noreply.github.com> Date: Thu, 19 Dec 2024 17:32:00 -0300 Subject: [PATCH 6/8] feat: better test --- batcher/aligned-batcher/src/types/batch_queue.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/batcher/aligned-batcher/src/types/batch_queue.rs b/batcher/aligned-batcher/src/types/batch_queue.rs index 27cae8febe..d711e294d2 100644 --- a/batcher/aligned-batcher/src/types/batch_queue.rs +++ b/batcher/aligned-batcher/src/types/batch_queue.rs @@ -499,13 +499,14 @@ mod test { let mut batch_queue = BatchQueue::new(); batch_queue.push(entry_1, batch_priority_1); batch_queue.push(entry_2, batch_priority_2); - batch_queue.push(entry_3, batch_priority_3); + batch_queue.push(entry_3.clone(), batch_priority_3.clone()); let gas_price = U256::from(1); let (resulting_batch_queue, batch) = try_build_batch(batch_queue, gas_price, 5000000, 2).unwrap(); assert!(resulting_batch_queue.len() == 1); //nonce_3 + assert!(resulting_batch_queue.clone().pop() == Some((entry_3.clone(), batch_priority_3.clone()))); //nonce_3 assert_eq!(batch[0].nonced_verification_data.nonce, nonce_2); assert_eq!(batch[1].nonced_verification_data.nonce, nonce_1); From 0905fb276e8ad972b92b563e2d327db176270466 Mon Sep 17 00:00:00 2001 From: Urix <43704209+uri-99@users.noreply.github.com> Date: Thu, 19 Dec 2024 18:13:46 -0300 Subject: [PATCH 7/8] feat: better comments about batch ordering --- batcher/aligned-batcher/src/types/batch_queue.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/batcher/aligned-batcher/src/types/batch_queue.rs b/batcher/aligned-batcher/src/types/batch_queue.rs index d711e294d2..8e5212cb92 100644 --- a/batcher/aligned-batcher/src/types/batch_queue.rs +++ b/batcher/aligned-batcher/src/types/batch_queue.rs @@ -100,8 +100,15 @@ impl PartialOrd for BatchQueueEntryPriority { impl Ord for BatchQueueEntryPriority { fn cmp(&self, other: &Self) -> std::cmp::Ordering { - let ord = other.max_fee.cmp(&self.max_fee); + // Implementation of lowest-first: + let ord: std::cmp::Ordering = other.max_fee.cmp(&self.max_fee); + // This means, less max_fee will go first + // We want this because we will .pop() to remove unwanted elements, low fee submitions. + if ord == std::cmp::Ordering::Equal { + // Case of same max_fee: + // Implementation of biggest-first: + // Since we want to .pop() entries with biggest nonce first, because we want to submit low nonce first self.nonce.cmp(&other.nonce) } else { ord @@ -157,6 +164,7 @@ pub(crate) fn try_build_batch( let batch_len = batch_queue.len(); let fee_per_proof = calculate_fee_per_proof(batch_len, gas_price); + // if batch is not acceptable: if batch_size > max_batch_byte_size || fee_per_proof > entry.nonced_verification_data.max_fee || batch_len > max_batch_proof_qty From 1a81782f91f658e04296a846ee39a0f1314c0c81 Mon Sep 17 00:00:00 2001 From: Urix <43704209+uri-99@users.noreply.github.com> Date: Thu, 19 Dec 2024 20:24:12 -0300 Subject: [PATCH 8/8] chore: cargo fmt --- batcher/aligned-batcher/src/types/batch_queue.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/batcher/aligned-batcher/src/types/batch_queue.rs b/batcher/aligned-batcher/src/types/batch_queue.rs index 8e5212cb92..555bbf5f9d 100644 --- a/batcher/aligned-batcher/src/types/batch_queue.rs +++ b/batcher/aligned-batcher/src/types/batch_queue.rs @@ -514,7 +514,10 @@ mod test { try_build_batch(batch_queue, gas_price, 5000000, 2).unwrap(); assert!(resulting_batch_queue.len() == 1); //nonce_3 - assert!(resulting_batch_queue.clone().pop() == Some((entry_3.clone(), batch_priority_3.clone()))); //nonce_3 + assert!( + resulting_batch_queue.clone().pop() + == Some((entry_3.clone(), batch_priority_3.clone())) + ); //nonce_3 assert_eq!(batch[0].nonced_verification_data.nonce, nonce_2); assert_eq!(batch[1].nonced_verification_data.nonce, nonce_1);