From 47544d9a7e19a080bcbd22d65db97e0e5170edc1 Mon Sep 17 00:00:00 2001 From: Georgios Konstantopoulos Date: Wed, 18 Feb 2026 12:31:04 -0800 Subject: [PATCH] fix(txpool): ensure transactions are added to pending subpool in nonce order (#22308) Co-authored-by: Amp Co-authored-by: Matthias Seitz --- .changelog/warm-pandas-cook.md | 5 + crates/transaction-pool/src/pool/txpool.rs | 195 ++++++++++++++++++++- 2 files changed, 195 insertions(+), 5 deletions(-) create mode 100644 .changelog/warm-pandas-cook.md diff --git a/.changelog/warm-pandas-cook.md b/.changelog/warm-pandas-cook.md new file mode 100644 index 0000000000..5993059ffc --- /dev/null +++ b/.changelog/warm-pandas-cook.md @@ -0,0 +1,5 @@ +--- +reth-transaction-pool: patch +--- + +Fixed a bug where transactions from the same sender were added to the pending subpool out of nonce order. Ensured `process_updates` runs before `add_new_transaction` so that lower-nonce promotions are enqueued before the newly inserted higher-nonce transaction, preserving correct ordering for live `BestTransactions` iterators. diff --git a/crates/transaction-pool/src/pool/txpool.rs b/crates/transaction-pool/src/pool/txpool.rs index 27565ba188..6b2b33869d 100644 --- a/crates/transaction-pool/src/pool/txpool.rs +++ b/crates/transaction-pool/src/pool/txpool.rs @@ -771,12 +771,41 @@ impl TxPool { .update(on_chain_nonce, on_chain_balance); match self.all_transactions.insert_tx(tx, on_chain_balance, on_chain_nonce) { - Ok(InsertOk { transaction, move_to, replaced_tx, updates, state }) => { - // replace the new tx and remove the replaced in the subpool(s) - self.add_new_transaction(transaction.clone(), replaced_tx.clone(), move_to); - // Update inserted transactions metric + Ok(InsertOk { transaction, move_to, replaced_tx, mut updates, state }) => { + // Interleave update processing and new-tx insertion so that live + // `BestTransactions` iterators always receive transactions in nonce order. + // Updates are already in nonce-ascending order, so we split them around + // the new transaction's nonce: + // 1. Promote lower-nonce txs first (e.g. balance-unlock scenario) + // 2. Add the new transaction + // 3. Promote higher-nonce txs last (e.g. gap-fill scenario) + let new_nonce = transaction.id().nonce; + let split = updates.iter().position(|u| u.id.nonce >= new_nonce); + let (promoted, discarded) = match split { + // All updates are lower-nonce — promote them first, then add new tx + None => { + let UpdateOutcome { promoted, discarded } = self.process_updates(updates); + self.add_new_transaction(transaction.clone(), replaced_tx.clone(), move_to); + (promoted, discarded) + } + // All updates are higher-nonce — add new tx first, then promote + Some(0) => { + self.add_new_transaction(transaction.clone(), replaced_tx.clone(), move_to); + let UpdateOutcome { promoted, discarded } = self.process_updates(updates); + (promoted, discarded) + } + // Mixed — split and interleave + Some(i) => { + let after = updates.split_off(i); + let mut outcome = self.process_updates(updates); + self.add_new_transaction(transaction.clone(), replaced_tx.clone(), move_to); + let after_outcome = self.process_updates(after); + outcome.promoted.extend(after_outcome.promoted); + outcome.discarded.extend(after_outcome.discarded); + (outcome.promoted, outcome.discarded) + } + }; self.metrics.inserted_transactions.increment(1); - let UpdateOutcome { promoted, discarded } = self.process_updates(updates); let replaced = replaced_tx.map(|(tx, _)| tx); @@ -4571,4 +4600,160 @@ mod tests { "Non-4844 tx should gain ENOUGH_FEE_CAP_BLOCK bit after basefee decrease" ); } + + /// Test for + /// + /// When a new transaction is added and its `updates` contain a same-sender transaction with + /// a lower nonce, the lower-nonce tx must be added to the pending subpool *before* the + /// higher-nonce tx. Otherwise, live `BestTransactions` iterators receive them out of order. + #[test] + fn best_transactions_nonce_order_on_balance_unlock() { + let mut f = MockTransactionFactory::default(); + let mut pool = TxPool::new(MockOrdering::default(), Default::default()); + + let sender = Address::random(); + let on_chain_balance = U256::from(10_000); + + // tx0: nonce 0, cheap — will go straight to pending + let tx0 = MockTransaction::eip1559().with_sender(sender).set_gas_price(100).inc_limit(); + // tx1: nonce 1, very expensive — cumulative cost (tx0 + tx1) will exceed balance + let tx1 = tx0.next().inc_limit().with_value(U256::from(on_chain_balance)); + // tx2: nonce 2 + let tx2 = tx1.next().inc_limit().with_value(U256::ZERO); + + let v0 = f.validated(tx0); + let v1 = f.validated(tx1); + let v2 = f.validated(tx2); + + // Add tx0 with limited balance — goes to pending (tx0 cost is small: 1 * 100 = 100) + pool.add_transaction(v0, on_chain_balance, 0, None).unwrap(); + + // Create a live BestTransactions iterator that will receive new pending txs + let mut best = pool.best_transactions(); + + // Drain tx0 from the iterator + let first = best.next().expect("should yield tx0"); + assert_eq!(first.id().nonce, 0); + + // Add tx1 with the same limited balance — cumulative cost exceeds balance, goes to + // queued + pool.add_transaction(v1, on_chain_balance, 0, None).unwrap(); + + // tx1 should be queued, nothing new in best + assert!(best.next().is_none(), "tx1 should be queued, not pending"); + + // Now add tx2 with U256::MAX balance — tx2 goes to pending AND tx1 gets promoted. + // The bug: tx2 was added to pending *before* tx1, so BestTransactions yielded tx2 + // first, violating nonce ordering. + pool.add_transaction(v2, U256::MAX, 0, None).unwrap(); + + let t1 = best.next().expect("should yield a transaction"); + let t2 = best.next().expect("should yield a transaction"); + + // Correct order: tx1 (nonce 1) before tx2 (nonce 2) + assert_eq!( + t1.id().nonce, + 1, + "first yielded tx should be nonce 1, got nonce {}", + t1.id().nonce + ); + assert_eq!( + t2.id().nonce, + 2, + "second yielded tx should be nonce 2, got nonce {}", + t2.id().nonce + ); + } + + /// Gap-fill scenario: inserting a low-nonce transaction promotes a queued higher-nonce + /// transaction. The new (lower-nonce) tx must appear before the promoted (higher-nonce) + /// tx in the `BestTransactions` iterator. + #[test] + fn best_transactions_nonce_order_on_gap_fill() { + let mut f = MockTransactionFactory::default(); + let mut pool = TxPool::new(MockOrdering::default(), Default::default()); + + let sender = Address::random(); + let balance = U256::MAX; + + // tx0: nonce 0 + let tx0 = MockTransaction::eip1559().with_sender(sender).set_gas_price(100).inc_limit(); + // tx1: nonce 1 + let tx1 = tx0.next().inc_limit(); + + let v0 = f.validated(tx0); + let v1 = f.validated(tx1); + + // Add tx1 first — goes to queued because nonce 0 is missing (nonce gap) + pool.add_transaction(v1, balance, 0, None).unwrap(); + + // Create a live BestTransactions iterator (currently empty — nothing pending) + let mut best = pool.best_transactions(); + assert!(best.next().is_none(), "pool should have no pending txs yet"); + + // Add tx0 — fills the gap, tx1 gets promoted + pool.add_transaction(v0, balance, 0, None).unwrap(); + + let t0 = best.next().expect("should yield a transaction"); + let t1 = best.next().expect("should yield a transaction"); + + assert_eq!(t0.id().nonce, 0, "first yielded tx should be nonce 0, got {}", t0.id().nonce); + assert_eq!(t1.id().nonce, 1, "second yielded tx should be nonce 1, got {}", t1.id().nonce); + } + + /// Mixed scenario: inserting a mid-nonce transaction promotes both a lower-nonce tx + /// (via balance update) and a higher-nonce tx (via gap fill). All three must appear + /// in nonce order in `BestTransactions`. + #[test] + fn best_transactions_nonce_order_mixed_promotions() { + let mut f = MockTransactionFactory::default(); + let mut pool = TxPool::new(MockOrdering::default(), Default::default()); + + let sender = Address::random(); + let low_balance = U256::from(10_000); + + // tx0: nonce 0, cheap + let tx0 = MockTransaction::eip1559().with_sender(sender).set_gas_price(100).inc_limit(); + // tx1: nonce 1, very expensive — will exceed balance + let tx1 = tx0.next().inc_limit().with_value(U256::from(low_balance)); + // tx2: nonce 2 + let tx2 = tx1.next().inc_limit().with_value(U256::ZERO); + // tx3: nonce 3 + let tx3 = tx2.next().inc_limit().with_value(U256::ZERO); + + let v0 = f.validated(tx0); + let v1 = f.validated(tx1); + let v2 = f.validated(tx2); + let v3 = f.validated(tx3); + + // Add tx0 — goes to pending + pool.add_transaction(v0, low_balance, 0, None).unwrap(); + + // Add tx1 — queued (cumulative cost exceeds balance) + pool.add_transaction(v1, low_balance, 0, None).unwrap(); + + // Add tx3 — queued (nonce gap: tx2 is missing) + pool.add_transaction(v3, low_balance, 0, None).unwrap(); + + let mut best = pool.best_transactions(); + + // Drain tx0 + let first = best.next().expect("should yield tx0"); + assert_eq!(first.id().nonce, 0); + assert!(best.next().is_none(), "only tx0 should be pending"); + + // Add tx2 with U256::MAX balance — this should: + // - promote tx1 (lower-nonce, was queued due to balance) + // - add tx2 itself to pending + // - promote tx3 (higher-nonce, was queued due to nonce gap) + pool.add_transaction(v2, U256::MAX, 0, None).unwrap(); + + let t1 = best.next().expect("should yield nonce 1"); + let t2 = best.next().expect("should yield nonce 2"); + let t3 = best.next().expect("should yield nonce 3"); + + assert_eq!(t1.id().nonce, 1, "expected nonce 1, got {}", t1.id().nonce); + assert_eq!(t2.id().nonce, 2, "expected nonce 2, got {}", t2.id().nonce); + assert_eq!(t3.id().nonce, 3, "expected nonce 3, got {}", t3.id().nonce); + } }