Skip to content

fix(txpool): keep PendingPool sender caches in sync with by_id - #10

Open
rezzmah wants to merge 2 commits into
mainfrom
rez/fix-pending-pool-independent-transactions
Open

fix(txpool): keep PendingPool sender caches in sync with by_id#10
rezzmah wants to merge 2 commits into
mainfrom
rez/fix-pending-pool-independent-transactions

Conversation

@rezzmah

@rezzmah rezzmah commented May 25, 2026

Copy link
Copy Markdown
Owner

Summary

PendingPool::remove_transaction mutated the sender's cached independent_transactions entry BEFORE confirming the tx still lived in by_id. Any call to pending_pool.remove_transaction(id) for an id missing from by_id (for example through TxPool::remove_from_subpool after an all_transactions <-> subpool divergence) ratcheted the cache without removing anything. The cache then pointed at a non-minimum nonce for that sender, and because PendingPool::best seeds the iterator's independent set from self.independent_transactions.values(), every fresh best_transactions_with_attributes snapshot started iteration from the wrong nonce for that sender. EVM rejects each yielded tx with nonce too high, the iterator marks the sender invalid, nothing is applied, the next iterator recreates the same broken snapshot, and the sender effectively gets stuck.

The same shape of divergence is also reachable through TxPool's public API: prune_transactions removes a tx from both all_transactions and its subpool WITHOUT parking descendants, so it can leave gaps in pending_pool.by_id. The pre-fix remove_transaction advanced the cached lowest via get(&id.descendant()), which silently returns None across such a gap and CLEARED the cache, even though the sender still had pending transactions at higher nonces.

update_accounts cannot heal this state: AllTransactions::update only emits PoolUpdates when a tx's subpool changes, so a sender whose chain stays fully in Pending never gets its independent_transactions entry touched.

Operational observation and validation

We first noticed this as a live txpool / payload-builder symptom rather than as a unit-test failure. Under high-throughput payload-building load, one sender maintained a large contiguous pending range (~20k transactions). txpool_status reported a full pending pool, txpool_content showed contiguous sender nonces from the chain nonce through the pending nonce, and eth_getTransactionCount(..., "latest") matched the lowest pending nonce. Despite that, fresh best_transactions_with_attributes snapshots repeatedly started from a much higher sender nonce. The EVM rejected that yielded transaction with nonce too high; after the iterator marked the sender invalid, no transaction from that sender was applied. Recreating the iterator reproduced the same wrong first transaction because the stale sender cache remained the seed for PendingPool::best.

We tested the fix in two ways:

  1. Added focused txpool regressions that fail on the unfixed code and pass with this patch:
    • missing-id removal must not mutate sender caches;
    • removing the cached lowest tx must recompute the next lowest from by_id, including across nonce gaps;
    • the public TxPool API must recover after prune_transactions creates a pending nonce gap.
  2. Backported the exact txpool change onto a reth v2.2.0-based deployment and reran the same high-throughput load. After rollout, the txpool stayed populated and the sender nonce range kept advancing; the permanent nonce too high / no-progress loop disappeared. We still observed occasional low-transaction blocks during stress, but those correlated with payload build time exceeding the block cadence while the txpool remained populated and the latest/pending nonce range continued advancing. That points to a separate payload-building throughput/cadence issue, not this sender-cache desynchronization.

Fix

crates/transaction-pool/src/pool/pending.rs:

  • Move let tx = self.by_id.remove(id)?; to the top of remove_transaction. Sender-level caches are only mutated after a successful removal, so a stale id can no longer corrupt them.
  • When the cached lowest is removed, advance to the new lowest by scanning by_id forward for the sender (range query) instead of looking up id.descendant(), so a gap in the sender's pending nonces does not silently break the cache.
  • Strengthen assert_invariants to verify, per sender, that independent_transactions equals min(by_id[sender]) and highest_nonces equals max(by_id[sender]). This catches drift immediately in tests.

Tests

Three regression tests, all of which fail against the unfixed code and pass against the fix:

  • pool::pending::tests::test_remove_transaction_for_missing_id_is_noop (private-state, exercises the divergence directly on PendingPool).
  • pool::pending::tests::test_remove_lowest_advances_independent_across_gap (private-state, asserts the cache crosses a synthetic gap).
  • pool::txpool::tests::test_best_transactions_recovers_after_prune_induced_gap (end-to-end through TxPool's public API: add_transaction, prune_transactions, best_transactions_with_attributes).

Before / After for the public-API test

Reverting just the pending.rs::remove_transaction change (leaving the new test applied) and running the public-API test reproduces the production failure mode:

$ cargo test -p reth-transaction-pool --lib \
    test_best_transactions_recovers_after_prune_induced_gap

running 1 test

thread 'pool::txpool::tests::test_best_transactions_recovers_after_prune_induced_gap'
  panicked at crates/transaction-pool/src/pool/txpool.rs:4876:9:
assertion `left == right` failed: independent_transactions cache must
advance past the prune-induced gap
  left: None
 right: Some(2)

With the fix re-applied:

$ cargo test -p reth-transaction-pool --lib \
    test_best_transactions_recovers_after_prune_induced_gap

running 1 test
test pool::txpool::tests::test_best_transactions_recovers_after_prune_induced_gap ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 241 filtered out

Verification

cargo test -p reth-transaction-pool --lib — all 242 tests pass, including the 3 new ones. cargo +nightly fmt --all --check and cargo +nightly clippy -p reth-transaction-pool --lib --tests --all-features are clean.

rezzmah and others added 2 commits May 26, 2026 10:32
PendingPool::remove_transaction mutated `independent_transactions[sender]`
(advancing or clearing it) BEFORE proving the tx still lived in `by_id`.
Any caller that invoked `PendingPool::remove_transaction(id)` for an id
not actually present in `by_id` (for example through
`TxPool::remove_from_subpool` after any all_transactions <-> subpool
divergence) ratcheted the cache without removing anything. The cache
then pointed at a non-minimum nonce for that sender, and because
`PendingPool::best` seeds the iterator's `independent` set from
`self.independent_transactions.values()`, every subsequent
`best_transactions_with_attributes` snapshot would start iteration from
the wrong nonce. EVM rejects this with `nonce too high`, the iterator
marks the sender invalid, nothing is applied, and the next iterator
recreates the same broken snapshot.

`update_accounts` cannot heal this state either: `AllTransactions::update`
only records `PoolUpdate` entries when a tx's subpool changes, so if a
sender's whole pending chain stays in `Pending`, no updates fire and
`PendingPool::independent_transactions` is never touched.

Changes:
- Move `let tx = self.by_id.remove(id)?;` to the top of
  `remove_transaction`. Sender-level caches are only mutated after a
  successful removal.
- When the cached lowest is removed, advance to the new lowest by
  scanning `by_id` for the sender (range query) instead of looking up
  `id.descendant()`, so gaps in the sender's pending nonces do not
  silently break the cache.
- Strengthen `assert_invariants` to verify, per sender, that
  `independent_transactions` equals `min(by_id[sender])` and
  `highest_nonces` equals `max(by_id[sender])`. This catches drift
  immediately in tests.
- Add `test_remove_transaction_for_missing_id_is_noop` and
  `test_remove_lowest_advances_independent_across_gap`, both of which
  fail against the old code and pass against the patched code.

Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e61a5-9d24-7219-b391-e8b84325680d
Drives `TxPool` end-to-end through public APIs (`add_transaction`,
`prune_transactions`, `best_transactions_with_attributes`) to reproduce
the conduit-rust flashblocks builder failure mode. After pruning a
mid-chain pending tx, pruning the cached lowest tx must keep
`independent_transactions` consistent with `by_id` so that
`best_transactions_with_attributes(..).next()` yields the actual
lowest pending nonce (not `None`).

Pre-fix this test fails with:

  assertion `left == right` failed: independent_transactions cache must
  advance past the prune-induced gap
    left: None
   right: Some(2)

Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019e61a5-9d24-7219-b391-e8b84325680d
@rezzmah
rezzmah force-pushed the rez/fix-pending-pool-independent-transactions branch from 1015224 to 0a17ba4 Compare May 26, 2026 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant