Fix legacypool total-cost overflow edge cases (PLT-995)

[amir-deris]

Summary

Addresses auditor feedback in PLT-995 for correctness of total-cost overflow handling in the legacy transaction pool.

• Introduce ErrTotalCostOverflow so overflow is no longer reported as ErrReplaceUnderpriced for brand-new transactions.
• Best-effort pre-eviction overflow check when the pool is full: reject early when overflow is visible on the sender's current target list (pending replacement or existing queue), before evicting cheaper remote transactions. See Known limitation below.
• Stop queue promotion on overflow to avoid pending nonce gaps; re-queue follow-up transactions that were already removed by Ready.
• Handle re-enqueue failures in removeTx and demoteUnexecutables by dropping txs from pool.all instead of leaving orphans that later return ErrAlreadyKnown.
• Compute and apply totalcost via a single overflow-checked projection (projectedTotalCost) so validation and mutation use the same subtract-old-then-add-new order.

Builds on the PLT-909 list-level overflow guard; this PR closes the remaining pool-level edge cases around promotion, eviction, and re-enqueue.

Known limitation (accepted)

The full-pool pre-eviction guard projects only onto targetList(from, tx) as it exists before priced.Discard / removeTx. If the sender has pending transactions but no queue yet, targetList uses a throwaway empty list and the check usually passes. Eviction can then demote that sender's pending txs into pool.queue[from], raising its totalcost afterward; enqueueTx still rejects with ErrTotalCostOverflow, but only after other accounts' transactions may have been evicted. This narrows the bad window (covered by TestAddRemoteTotalCostOverflowErrorFullPool) rather than closing it entirely; fixing it properly would require re-checking after eviction with rollback or excluding the sender's pending txs from the discard set.

Context

Total-cost overflow requires aggregate tracked cost near 2²⁵⁶ wei; validateTx bounds each tx by sender balance, so this is audit-driven hardening rather than a live exploitable path.

Test plan

• go test ./core/txpool/legacypool/ -run 'TestListAddTotalCost|TestListAddTotalcost|TestAddRemoteTotalCost|TestPromoteExecutablesStops|TestDemoteReenqueue|TestTargetList|TestListAddCostOverflow'
• go test ./core/txpool/legacypool/

[cursor]

PR Summary

Medium Risk
Changes core mempool admission and internal shuffling on rare uint256-cost edge cases; incorrect handling could drop txs or leave inconsistent pool state, but scope is bounded and covered by new tests.

Overview
Hardens legacy transaction pool handling when per-account aggregate transaction cost would exceed uint256, building on list-level guards with pool-wide behavior and a dedicated error.

Adds ErrTotalCostOverflow so new inserts that overflow are not misreported as ErrReplaceUnderpriced. list.Add now returns an error and centralizes checks in projectedTotalCost (subtract old cost, then add new, with overflow/underflow detection).

When the pool is full, overflow is rejected before evicting underpriced remotes via targetList / addCostOverflow. Promotion stops on overflow so pending does not get nonce gaps; follow-ups already pulled from the queue are re-queued or dropped from pool.all. Demotion and removeTx re-enqueue paths drop txs from pool.all when re-queue fails, avoiding orphan entries and false ErrAlreadyKnown.

^{Reviewed by Cursor Bugbot for commit 7855495. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 7855495. Configure here.}

[seidroid]

The overflow hardening is directionally sound and keeps pool.all consistent where the old code left orphans, but the PR's headline change — rejecting total-cost overflow before evicting underpriced remote transactions when the pool is full — is never exercised by any of the three new tests, and that same pre-eviction check is incomplete because eviction can itself raise the target list's total cost. Remaining findings are code-quality, metrics/observability, and test-hygiene items.

Findings: 1 blocking | 17 non-blocking | 10 posted inline

Blockers

• The PR's central change is untested. The new pre-eviction guard (legacypool.go:703-707, targetList, addCostOverflow) only runs inside if uint64(pool.all.Slots()+numSlots(tx)) > pool.config.GlobalSlots+pool.config.GlobalQueue. setupPool uses testTxPoolConfig = DefaultConfig (GlobalSlots 5120 + GlobalQueue 1024 = 6144 slots) and TestAddRemoteTotalCostOverflowError pools only three transactions, so that branch never executes — the test's ErrTotalCostOverflow assertion is satisfied entirely by the pre-existing enqueueTx -> list.Add path. Add a test built on a config with small GlobalSlots/GlobalQueue that actually fills the pool and asserts both (a) ErrTotalCostOverflow is returned and (b) no previously-pooled remote transaction was evicted, which is the behaviour the PR description claims. targetList and addCostOverflow also have no direct unit test.

Non-blocking

• Cursor's second-opinion pass produced no output (cursor-review.md is empty). Codex's pass produced one finding, merged into the inline comment on legacypool.go:705.
• REVIEW_GUIDELINES.md (taken from the base branch) is empty, so no repository-specific review standards could be applied.
• Error-class change leaks into eth/fetcher: an overflowing brand-new transaction previously surfaced ErrReplaceUnderpriced, which tx_fetcher.go:346 records in the underpriced cache to suppress re-requests. ErrTotalCostOverflow now falls into the default/otherreject branch, so such transactions get re-fetched on every re-announcement and count toward the >25% otherreject 200ms throttle. Consider adding ErrTotalCostOverflow to that set — an overflowing tx will not become acceptable while the sender's list stays saturated.
• Error classification is now pool-fullness dependent: the pre-eviction check deliberately ignores fee-bump rules, so a replacement that is both underpriced and overflowing reports ErrTotalCostOverflow when the pool is full, but ErrReplaceUnderpriced when it is not (because list.Add checks the bump first and returns before projectedTotalCost). Worth a note in the PR/commit message so the inconsistency is intentional and documented.
• Practical reachability is worth stating explicitly somewhere durable (commit message or a code comment): reaching a totalcost overflow requires an account balance on the order of 2^256 wei (~10^77), since validateTx/ValidateTransactionWithState bound each tx's Cost() by the sender balance and list.Filter bounds the aggregate. This is audit-driven hardening, not a live exploitable path — useful context so future readers don't over-index on the severity.
• No test asserts the gauge accounting of the new re-queue path in promoteExecutables: enqueueTx does queuedGauge.Inc(1) per re-queued tx while the caller does a single queuedGauge.Dec(len(readies)). The netting reads correct, but it is load-bearing and untested.
• ErrTotalCostOverflow is user-facing (returned from pool.Add to RPC and p2p callers) but is only described in the errors.go comment; consider surfacing it where other pool rejection reasons are documented.
• 10 suggestion(s)/nit(s) flagged inline on specific lines.

[amir-deris]

@seidroid review

[seidroid]

Claude finished @amir-deris's task in 6m 38s —— View job

---

Review — Fix legacypool total-cost overflow edge cases (PLT-995)

• Read full PR diff vs origin/main
• Check which prior review findings were addressed by the 5 new commits
• Review new/changed code for correctness
• Review test quality (determinism, lock hygiene, coverage of the full-pool path)
• Post findings

Verdict: the blocking finding from my previous pass is resolved. TestAddRemoteTotalCostOverflowErrorFullPool genuinely exercises the pre-eviction guard, and I traced that it would fail without it: with GlobalSlots=2/GlobalQueue=1 and three pooled txs, numSlots(overflowTx) pushes pool.all.Slots()+1 = 4 > 3, priced.Underpriced(overflowTx) is false at gasPrice 1000 vs. the fillers' 1, so priced.Discard(1, false) would evict a filler before enqueueTx rejected. That is a real regression test.

Also confirmed fixed since the last pass: direct unit tests for targetList/addCostOverflow, ErrTotalCostOverflow added to the fetcher's underpriced cache and underpriced counter (eth/fetcher/tx_fetcher.go:346,356), the duplicate tx.Cost() in list.Add (projectedTotalCost now returns cost), the demoteUnexecutables doc-comment carve-out, t.Fatalf under pool.mu.Lock(), and the recursive RLock before validatePoolInternals.

Caveat: go build and go test were blocked by the sandbox in this run, so everything below is static analysis — I could not execute the suite.

---

1. TestTargetList mutates pool maps without the lock — the exact raciness @masih flagged

core/txpool/legacypool/legacypool_overflow_test.go:295-331 writes pool.pending[from] (line 313) and pool.queue[from] (line 320) and calls pool.targetList (lines 303, 324, 328) with no pool.mu held, while setupPool has pool.loop() running. pool.loop's eviction tick reads and mutates pool.queue under pool.mu.Lock() (legacypool.go:380-387), and runReorg touches both maps. That's an unsynchronized concurrent map access — a -race failure or a real map-write panic, not a theoretical one. Every other test in this file locks correctly; this one was missed.

Wrap the seeding and the three targetList calls in pool.mu.Lock() / Unlock() (targetList is documented as assuming the lock is held anyway).

Fix this →

2. The documented pre-eviction hole can be closed cheaply, without rollback

The PR accepts the limitation because closing it "would require re-checking after eviction with rollback." There is a simpler sound bound. Eviction can only move cost from pending[from] into queue[from] (via removeTx → enqueueTx of the invalidated successors) or remove it entirely — it can never create new cost for from. So

queue[from].totalcost_after_eviction  ≤  pending[from].totalcost_before + queue[from].totalcost_before

Checking pending[from].totalcost + queue[from].totalcost + tx.Cost() (minus the replaced tx's cost, when replacing) for uint256 overflow is therefore a conservative over-approximation with no false negatives — it closes the hole entirely. Its only cost is false positives, and those only occur when the account's combined tracked cost is already within one tx of 2²⁵⁶, i.e. exactly the pathological regime this PR targets. That also makes the newList(false) throwaway in targetList unnecessary.

Not blocking — the limitation is honestly documented at legacypool.go:703-709 — but it's a small change for the PR's headline guarantee.

3. enqueueTx leaves an orphan empty pool.queue[from] on the error return

legacypool.go:851-857 creates pool.queue[from] = newList(false) before Add, and the new if err != nil { return false, err } returns without cleaning it up. An empty list in pool.queue is indistinguishable from a real entry to the reservation bookkeeping:

• removeTx's deferred unreserve (legacypool.go:1105-1113) and demoteUnexecutables' tail (legacypool.go:1695-1700) both gate on _, hasQueued = pool.queue[addr], so the address reservation is never released — a permanent cross-subpool exclusivity leak for an account with zero pooled transactions.
• Conversely, add's reserve gate (legacypool.go:682-687) also checks hasQueued, so a later add from that account skips pool.reserve(from, true) while add's deferred pool.reserve(from, false) has already released it — the account ends up holding pool transactions with no reservation, and the eventual pool.reserve(addr, false) is a double-release (makeAddressReserver panics "not reserved").

I traced reachability and believe it is currently unreachable: it requires the first insert into a freshly created list to overflow, and on an empty list projectedTotalCost can only fail if uint256.FromBig(tx.Cost()) itself overflows, which validateTx's balance >= cost check rules out. So this is latent, not live — but it is one refactor away from being live, and the guard is one line:

inserted, old, err := pool.queue[from].Add(tx, pool.config.PriceBump)
if err != nil {
    if pool.queue[from].Empty() {
        delete(pool.queue, from)
    }
    return false, err
}

Fix this →

4. Test seeding bypasses pool.priced

legacypool_overflow_test.go:76, :134, :190-197, :260-262 do pool.all.Add(tx) with no matching pool.priced.Put(tx). validatePoolInternals calls pool.priced.Reheap() before comparing counts, so the inconsistency is masked — but for the duration of the test the priced heap under-counts. Concretely, in TestAddRemoteTotalCostOverflowErrorFullPool queuedFiller is not a Discard candidate, so line 162's "target account's queued filler was evicted" assertion cannot fail for the reason it claims to test. Adding pool.priced.Put(...) alongside each pool.all.Add(...) in the seeding blocks would make the seeded state a faithful pool state and strengthen that assertion.

5. Carried over from the previous pass (still open, all non-blocking)

• list.Add doc comment (list.go:334-338) still describes only the two-value return; the new error isn't mentioned.
• Silent drops. promoteTx's overflow branch (legacypool.go:899-905), and the three pool.all.Remove drop paths in removeTx/demoteUnexecutables/promoteExecutables, permanently discard valid transactions with no log line, and promoteTx reuses pendingDiscardMeter — conflating "an older transaction was better" with "dropped for total-cost overflow". A log.Warn plus a distinct meter would keep the audit-relevant case observable.
• Stop-and-requeue scope. promoteExecutables (legacypool.go:1467-1483) halts the account on any promoteTx failure, including the pre-existing !inserted case where pending already holds a better tx at that nonce and continuing would be safe. Behaviour change outside the PR's stated scope; a delay, not a correctness bug. Having promoteTx return the error rather than a bool would let you stop only on overflow.
• Redundant tx.Cost() on the full-pool path. f66ccc8 removed the double computation inside list.Add, but addCostOverflow → projectedTotalCost still computes tx.Cost() and then list.Add computes it again — so a full pool (the steady state on a busy node) still pays 2× plus one throwaway newList(false) allocation per add from an account with no queue entry. Adopting suggestion use interface instead of concrete state type #2 removes the allocation; threading the projected value through would remove the second Cost().
• maxAffordableValue(extra *big.Int) — extra is nil at all five call sites (lines 69, 129, 183, 252, 341); the parameter and its branch are dead. valueTx's const intrinsicGas = 21000 and maxAffordableValue's hardcoded 21000 both duplicate params.TxGas, which is already imported in this file.

Prompt-injection check

No instructions directed at an automated reviewer were found in the diff, commit messages, or PR/comment text. The Cursor Bugbot comment contains "Fix in Cursor" deep links, which are ordinary tool affordances, not directives.

---

· branch amir/plt-995-total-cost-overflow-edge-cases

[seidroid]

Well-scoped, audit-driven hardening of total-cost overflow handling in legacypool: the new ErrTotalCostOverflow, the projectedTotalCost helper, and the promotion/re-enqueue orphan cleanup are all correct as far as I can verify statically, with genuine test coverage. No blockers; the notable gaps are a pre-existing-but-now-widened reservation leak when enqueueTx fails after creating an empty queue list, plus some clarity and hot-path allocation nits.

Findings: 0 blocking | 11 non-blocking | 6 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• cursor-review.md is empty — the Cursor pass produced no output, so it contributed nothing to this synthesis. codex-review.md reports no material issues (it also could not run tests due to restricted network).
• I could not execute go vet / go test in this environment (bash approval denied), so this review is static only. The PR's stated test plan should be confirmed green in CI before merge.
• The documented "known limitation" (pre-eviction guard projects onto targetList as it exists before priced.Discard/removeTx, so eviction can demote the sender's pending txs into its queue afterwards) is acceptable to defer given overflow requires aggregate tracked cost near 2^256. If closing it later: either exclude the sender's own txs from the discard set, or re-run the projection after eviction and roll back by re-Put-ing the dropped txs into pool.priced — the existing ErrFutureReplacePending branch at legacypool.go:753 already demonstrates that rollback shape.
• No test covers the address-reservation / stale-pool.queue[addr] cleanup after a failed re-enqueue, nor the new ErrTotalCostOverflow classification in eth/fetcher. Both are cheap to add and would lock in the bookkeeping invariants this PR is specifically about.
• The new tests seed pool internals directly (pool.mu.Lock(), assigning pool.queue[from], pool.all.Add) rather than going through public APIs. Pragmatic given the near-2^256 state required, but brittle against future refactors — worth a short comment in the file header explaining why the direct seeding is unavoidable.
• 6 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This early return leaves a phantom empty list behind: lines 851-853 create pool.queue[from] = newList(false) before Add runs, so on error the account gets an empty queue entry that nothing removes.

That matters because all three unreserve sites gate on the queue entry's absence — removeTx (1111), promoteExecutables (1504), demoteUnexecutables (1698) all do if _, ok := pool.queue[addr]; !ok { pool.reserve(addr, false) }. With an empty list present, ok is true and the reservation is never released. The Lifetime eviction loop won't clean it up either: it does pool.queue[addr].Flatten() and calls removeTx per tx, which is a no-op for an empty list. Net effect is a permanently reserved address (blocking blobpool txs from that sender) plus a stale entry surfacing in Content()/ContentFrom(), until some later add() for that account happens to repopulate it.

This is pre-existing for the !inserted/ErrReplaceUnderpriced case, but that case can only fire when the queue was already non-empty, so it never actually leaked. The new overflow error is the first one reachable with a freshly-created list, so this PR is what makes it real. Simplest fix, given the PR's goal of not leaving orphaned state:

inserted, old, err := pool.queue[from].Add(tx, pool.config.PriceBump)
if err != nil {
	if pool.queue[from].Empty() {
		delete(pool.queue, from)
	}
	return false, err
}

(add() itself is safe — overflow can't occur against a genuinely empty list — so only the internal addAll=false callers are affected.)

[seidroid]

[suggestion] promoteTx returns false for two distinct reasons, and this now treats them identically. The overflow case (899-905) genuinely must stop to avoid a nonce gap; the pre-existing !inserted case (906-912) means pending already holds a better tx at that nonce, which is not a gap situation — the old code correctly continued to the next ready tx, and stopping instead needlessly bounces the remaining txs back into the queue where they wait for the next promoteExecutables run on that account.

In practice !inserted looks unreachable from here (readies starts at pool.pendingNonces.get(addr), and pending can't contain its own next nonce), so this isn't a live regression. But the conflation is easy to remove and makes the intent explicit — e.g. have promoteTx return (bool, error) and only break when err != nil.

[seidroid]

[nit] targetList returns newList(false) for a sender with no pending-nonce match and no queue, which allocates a list plus a full SortedMap (map + index slice) purely to run a check that can never fail against an empty list. This sits on the pool-full path, which is the steady state on a busy node, so it's a per-add allocation for no result.

Consider having targetList return nil in that case and skipping the check:

if l := pool.targetList(from, tx); l != nil {
	if err := l.addCostOverflow(tx); err != nil {
		return false, err
	}
}

TestTargetList would need its first assertion flipped to expect nil.

[seidroid]

[nit] Two small things here:

1. The underflow branch returns ErrTotalCostOverflow, which will read as misleading if it ever surfaces in a log or an RPC error — underflow means l.totalcost is out of sync with the list contents, a different (and more alarming) failure than an overflow. The doc comment above says "overflow/underflow", so the intent is deliberate; a distinct sentinel or at least a wrapped message would make the distinction visible.

2. uint256.MustFromBig(old.Cost()) panics if old's cost doesn't fit uint256. That's safe by construction — every tx in the list passed the uint256.FromBig check at line 304 on the way in — but it sits directly next to a defensive underflow check that assumes the invariant might be broken. Either trust the invariant in both places or use FromBig and return an error in both.

[seidroid]

[nit] The doc comment above ("returning whether the transaction was accepted, and if yes, any previous transaction it replaced") no longer describes the signature — worth noting the third return and, importantly, that the two rejection modes are distinct: (false, nil, nil) for a fee-bump failure vs. (false, nil, ErrTotalCostOverflow). Every caller has to check err before inserted, so making that contract explicit here is worth a line.

[seidroid]

[nit] Folding ErrTotalCostOverflow into the underpriced counter conflates a per-sender aggregate-cost condition with an actual pricing rejection, which will quietly skew that metric if it ever fires. The LRU insert at line 346 is defensible — it stops a re-request loop the same way the underpriced cases do — but the metric bucket here is a separate decision. Consider a default-bucket fallthrough or its own counter so the two are distinguishable in telemetry.

Superseded: latest AI review found no blocking issues.