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

test(transaction-pool): add unit tests for BestTransactionsWithFees next #9274

Merged
merged 3 commits into from
Jul 3, 2024
Merged

test(transaction-pool): add unit tests for BestTransactionsWithFees next #9274

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8b5eac
test(transaction-pool): add unit tests for BestTransactionsWithFees next
tcoratger Jul 3, 2024
3128e09
clean up
tcoratger Jul 3, 2024
c953594
fix clippy
tcoratger Jul 3, 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
179 changes: 168 additions & 11 deletions crates/transaction-pool/src/pool/best.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,16 @@ impl<T: TransactionOrdering> Iterator for BestTransactionsWithFees<T> {
// find the next transaction that satisfies the base fee
loop {
let best = self.best.next()?;
if best.transaction.max_fee_per_gas() < self.base_fee as u128 {
// tx violates base fee, mark it as invalid and continue
crate::traits::BestTransactions::mark_invalid(self, &best);
// If both the base fee and blob fee (if applicable for EIP-4844) are satisfied, return
// the transaction
if best.transaction.max_fee_per_gas() >= self.base_fee as u128 &&
best.transaction
.max_fee_per_blob_gas()
.map_or(true, |fee| fee >= self.base_fee_per_blob_gas as u128)
Comment on lines +52 to +57
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this inverts both conditions and combines them.
nice!

{
return Some(best);
} else {
// tx is EIP4844 and violates blob fee, mark it as invalid and continue
if best.transaction.max_fee_per_blob_gas().is_some_and(|max_fee_per_blob_gas| {
max_fee_per_blob_gas < self.base_fee_per_blob_gas as u128
}) {
crate::traits::BestTransactions::mark_invalid(self, &best);
continue
};
return Some(best)
crate::traits::BestTransactions::mark_invalid(self, &best);
}
}
}
Expand Down Expand Up @@ -321,4 +319,163 @@ mod tests {
// iterator is empty
assert!(best.next().is_none());
}

#[test]
fn test_best_with_fees_iter_base_fee_satisfied() {
let mut pool = PendingPool::new(MockOrdering::default());
let mut f = MockTransactionFactory::default();

let num_tx = 5;
let base_fee: u64 = 10;
let base_fee_per_blob_gas: u64 = 15;

// Insert transactions with a max_fee_per_gas greater than or equal to the base fee
// Without blob fee
for nonce in 0..num_tx {
let tx = MockTransaction::eip1559()
.rng_hash()
.with_nonce(nonce)
.with_max_fee(base_fee as u128 + 5);
let valid_tx = f.validated(tx);
pool.add_transaction(Arc::new(valid_tx), 0);
}

let mut best = pool.best_with_basefee_and_blobfee(base_fee, base_fee_per_blob_gas);

for nonce in 0..num_tx {
let tx = best.next().expect("Transaction should be returned");
assert_eq!(tx.nonce(), nonce);
assert!(tx.transaction.max_fee_per_gas() >= base_fee as u128);
}
}

#[test]
fn test_best_with_fees_iter_base_fee_violated() {
let mut pool = PendingPool::new(MockOrdering::default());
let mut f = MockTransactionFactory::default();

let num_tx = 5;
let base_fee: u64 = 20;
let base_fee_per_blob_gas: u64 = 15;

// Insert transactions with a max_fee_per_gas less than the base fee
for nonce in 0..num_tx {
let tx = MockTransaction::eip1559()
.rng_hash()
.with_nonce(nonce)
.with_max_fee(base_fee as u128 - 5);
let valid_tx = f.validated(tx);
pool.add_transaction(Arc::new(valid_tx), 0);
}

let mut best = pool.best_with_basefee_and_blobfee(base_fee, base_fee_per_blob_gas);

// No transaction should be returned since all violate the base fee
assert!(best.next().is_none());
}

#[test]
fn test_best_with_fees_iter_blob_fee_satisfied() {
let mut pool = PendingPool::new(MockOrdering::default());
let mut f = MockTransactionFactory::default();

let num_tx = 5;
let base_fee: u64 = 10;
let base_fee_per_blob_gas: u64 = 20;

// Insert transactions with a max_fee_per_blob_gas greater than or equal to the base fee per
// blob gas
for nonce in 0..num_tx {
let tx = MockTransaction::eip4844()
.rng_hash()
.with_nonce(nonce)
.with_max_fee(base_fee as u128 + 5)
.with_blob_fee(base_fee_per_blob_gas as u128 + 5);
let valid_tx = f.validated(tx);
pool.add_transaction(Arc::new(valid_tx), 0);
}

let mut best = pool.best_with_basefee_and_blobfee(base_fee, base_fee_per_blob_gas);

// All transactions should be returned in order since they satisfy both base fee and blob
// fee
for nonce in 0..num_tx {
let tx = best.next().expect("Transaction should be returned");
assert_eq!(tx.nonce(), nonce);
assert!(tx.transaction.max_fee_per_gas() >= base_fee as u128);
assert!(
tx.transaction.max_fee_per_blob_gas().unwrap() >= base_fee_per_blob_gas as u128
);
}

// No more transactions should be returned
assert!(best.next().is_none());
}

#[test]
fn test_best_with_fees_iter_blob_fee_violated() {
let mut pool = PendingPool::new(MockOrdering::default());
let mut f = MockTransactionFactory::default();

let num_tx = 5;
let base_fee: u64 = 10;
let base_fee_per_blob_gas: u64 = 20;

// Insert transactions with a max_fee_per_blob_gas less than the base fee per blob gas
for nonce in 0..num_tx {
let tx = MockTransaction::eip4844()
.rng_hash()
.with_nonce(nonce)
.with_max_fee(base_fee as u128 + 5)
.with_blob_fee(base_fee_per_blob_gas as u128 - 5);
let valid_tx = f.validated(tx);
pool.add_transaction(Arc::new(valid_tx), 0);
}

let mut best = pool.best_with_basefee_and_blobfee(base_fee, base_fee_per_blob_gas);

// No transaction should be returned since all violate the blob fee
assert!(best.next().is_none());
}

#[test]
fn test_best_with_fees_iter_mixed_fees() {
let mut pool = PendingPool::new(MockOrdering::default());
let mut f = MockTransactionFactory::default();

let base_fee: u64 = 10;
let base_fee_per_blob_gas: u64 = 20;

// Insert transactions with varying max_fee_per_gas and max_fee_per_blob_gas
let tx1 =
MockTransaction::eip1559().rng_hash().with_nonce(0).with_max_fee(base_fee as u128 + 5);
let tx2 = MockTransaction::eip4844()
.rng_hash()
.with_nonce(1)
.with_max_fee(base_fee as u128 + 5)
.with_blob_fee(base_fee_per_blob_gas as u128 + 5);
let tx3 = MockTransaction::eip4844()
.rng_hash()
.with_nonce(2)
.with_max_fee(base_fee as u128 + 5)
.with_blob_fee(base_fee_per_blob_gas as u128 - 5);
let tx4 =
MockTransaction::eip1559().rng_hash().with_nonce(3).with_max_fee(base_fee as u128 - 5);

pool.add_transaction(Arc::new(f.validated(tx1.clone())), 0);
pool.add_transaction(Arc::new(f.validated(tx2.clone())), 0);
pool.add_transaction(Arc::new(f.validated(tx3)), 0);
pool.add_transaction(Arc::new(f.validated(tx4)), 0);

let mut best = pool.best_with_basefee_and_blobfee(base_fee, base_fee_per_blob_gas);

let expected_order = vec![tx1, tx2];
for expected_tx in expected_order {
let tx = best.next().expect("Transaction should be returned");
assert_eq!(tx.transaction, expected_tx);
}

// No more transactions should be returned
assert!(best.next().is_none());
}
}
Loading