Skip to content

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

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

Merged
amir-deris merged 7 commits into from
Aug 26, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7855495
Fix legacypool total-cost overflow edge cases for PLT-995.
amir-deris Aug 19, 2026
14d0788
Adding test
amir-deris Aug 20, 2026
fd742e0
Fixing racy tests
amir-deris Aug 20, 2026
d3a14cd
Updated comment for edge case
amir-deris Aug 20, 2026
f66ccc8
Avoid duplicate tx.Cost() computation in list.Add.
amir-deris Aug 20, 2026
dd54f7a
Clarify demoteUnexecutables priced-list behavior and cache overflow r…
amir-deris Aug 20, 2026
70edfc4
Use addRemoteSync in total-cost overflow error test.
amir-deris Aug 20, 2026
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
4 changes: 4 additions & 0 deletions core/txpool/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ var (
// with a different one without the required price bump.
ErrReplaceUnderpriced = errors.New("replacement transaction underpriced")

// ErrTotalCostOverflow is returned if adding a transaction would overflow the
// tracked aggregate cost of an account's transaction list.
ErrTotalCostOverflow = errors.New("total cost overflow")

// ErrAccountLimitExceeded is returned if a transaction would exceed the number
// allowed by a pool for a single account.
ErrAccountLimitExceeded = errors.New("account limit exceeded")
Expand Down
72 changes: 64 additions & 8 deletions core/txpool/legacypool/legacypool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,6 +700,18 @@ func (pool *LegacyPool) add(tx *types.Transaction) (replaced bool, err error) {
}
// If the transaction pool is full, discard underpriced transactions
if uint64(pool.all.Slots()+numSlots(tx)) > pool.config.GlobalSlots+pool.config.GlobalQueue {
// Reject early if the transaction would overflow the sender's current
// target list total cost, before evicting cheaper remote transactions.
//
// Best-effort: this only projects onto targetList(from, tx) as it exists
// now. priced.Discard/removeTx below can demote the sender's pending txs
// into pool.queue[from], raising that list's totalcost after this check.
// enqueueTx will still reject with ErrTotalCostOverflow, but only after
// eviction may have already dropped other accounts' transactions.
if err := pool.targetList(from, tx).addCostOverflow(tx); err != nil {
Comment thread
amir-deris marked this conversation as resolved.

@seidroid seidroid Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

return false, err
}

// If the new transaction is underpriced, don't accept it
if pool.priced.Underpriced(tx) {
log.Trace("Discarding underpriced transaction", "hash", hash, "gasTipCap", tx.GasTipCap(), "gasFeeCap", tx.GasFeeCap())
Expand Down Expand Up @@ -762,7 +774,10 @@ func (pool *LegacyPool) add(tx *types.Transaction) (replaced bool, err error) {
// Try to replace an existing transaction in the pending pool
if list := pool.pending[from]; list != nil && list.Contains(tx.Nonce()) {
// Nonce already pending, check if required price bump is met
inserted, old := list.Add(tx, pool.config.PriceBump)
inserted, old, err := list.Add(tx, pool.config.PriceBump)
if err != nil {
return false, err
}
if !inserted {
pendingDiscardMeter.Mark(1)
return false, txpool.ErrReplaceUnderpriced
Expand Down Expand Up @@ -792,6 +807,17 @@ func (pool *LegacyPool) add(tx *types.Transaction) (replaced bool, err error) {
return replaced, nil
}

// targetList returns the list a transaction would be inserted into.
func (pool *LegacyPool) targetList(from common.Address, tx *types.Transaction) *list {
if list := pool.pending[from]; list != nil && list.Contains(tx.Nonce()) {
return list
}
if list := pool.queue[from]; list != nil {
return list
}
return newList(false)
Comment thread
amir-deris marked this conversation as resolved.
}
Comment thread
amir-deris marked this conversation as resolved.

// isGapped reports whether the given transaction is immediately executable.
func (pool *LegacyPool) isGapped(from common.Address, tx *types.Transaction) bool {
// Short circuit if transaction falls within the scope of the pending list
Expand Down Expand Up @@ -825,7 +851,10 @@ func (pool *LegacyPool) enqueueTx(hash common.Hash, tx *types.Transaction, addAl
if pool.queue[from] == nil {
pool.queue[from] = newList(false)
}
inserted, old := pool.queue[from].Add(tx, pool.config.PriceBump)
inserted, old, err := pool.queue[from].Add(tx, pool.config.PriceBump)
if err != nil {
return false, err

@seidroid seidroid Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.)

}
if !inserted {
// An older transaction was better, discard this
queuedDiscardMeter.Mark(1)
Expand Down Expand Up @@ -867,7 +896,13 @@ func (pool *LegacyPool) promoteTx(addr common.Address, hash common.Hash, tx *typ
}
list := pool.pending[addr]

inserted, old := list.Add(tx, pool.config.PriceBump)
inserted, old, err := list.Add(tx, pool.config.PriceBump)
if err != nil {
Comment thread
amir-deris marked this conversation as resolved.
pool.all.Remove(hash)
pool.priced.Removed(1)
pendingDiscardMeter.Mark(1)
return false
}
if !inserted {
// An older transaction was better, discard this
pool.all.Remove(hash)
Expand Down Expand Up @@ -1093,7 +1128,10 @@ func (pool *LegacyPool) removeTx(hash common.Hash, outofbound bool, unreserve bo
// Postpone any invalidated transactions
for _, tx := range invalids {
// Internal shuffle shouldn't touch the lookup set.
pool.enqueueTx(tx.Hash(), tx, false)
if _, err := pool.enqueueTx(tx.Hash(), tx, false); err != nil {
pool.all.Remove(tx.Hash())
pool.priced.Removed(1)
}
}
// Update the account nonce if needed
pool.pendingNonces.setIfLower(addr, tx.Nonce())
Expand Down Expand Up @@ -1428,11 +1466,21 @@ func (pool *LegacyPool) promoteExecutables(accounts []common.Address) []*types.T

// Gather all executable transactions and promote them
readies := list.Ready(pool.pendingNonces.get(addr))
for _, tx := range readies {
for i, tx := range readies {
hash := tx.Hash()
if pool.promoteTx(addr, hash, tx) {
promoted = append(promoted, tx)
continue
}
// Stop promotion for this account to avoid a nonce gap in pending and
Comment thread
amir-deris marked this conversation as resolved.

@seidroid seidroid Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[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.

// put any already-removed follow-up transactions back in the queue.
for _, remaining := range readies[i+1:] {
if _, err := pool.enqueueTx(remaining.Hash(), remaining, false); err != nil {
pool.all.Remove(remaining.Hash())
pool.priced.Removed(1)
}
}
break
}
log.Trace("Promoted queued transactions", "count", len(promoted))
queuedGauge.Dec(int64(len(readies)))
Expand Down Expand Up @@ -1592,7 +1640,9 @@ func (pool *LegacyPool) truncateQueue() {
//
// Note: transactions are not marked as removed in the priced list because re-heaping
// is always explicitly triggered by SetBaseFee and it would be unnecessary and wasteful
// to trigger a re-heap is this function
// to trigger a re-heap in this function. The exception is when re-enqueueing a demoted
// transaction fails (e.g. total-cost overflow): the tx is dropped from pool.all and
// marked removed in the priced heap because it genuinely leaves the pool.
func (pool *LegacyPool) demoteUnexecutables() {
// Iterate over all accounts and demote any non-executable transactions
gasLimit := pool.currentHead.Load().GasLimit
Expand Down Expand Up @@ -1620,7 +1670,10 @@ func (pool *LegacyPool) demoteUnexecutables() {
log.Trace("Demoting pending transaction", "hash", hash)

// Internal shuffle shouldn't touch the lookup set.
pool.enqueueTx(hash, tx, false)
if _, err := pool.enqueueTx(hash, tx, false); err != nil {
Comment thread
amir-deris marked this conversation as resolved.
pool.all.Remove(hash)
pool.priced.Removed(1)
}
}
pendingGauge.Dec(int64(len(olds) + len(drops) + len(invalids)))

Expand All @@ -1632,7 +1685,10 @@ func (pool *LegacyPool) demoteUnexecutables() {
log.Warn("Demoting invalidated transaction", "hash", hash)

// Internal shuffle shouldn't touch the lookup set.
pool.enqueueTx(hash, tx, false)
if _, err := pool.enqueueTx(hash, tx, false); err != nil {
pool.all.Remove(hash)
pool.priced.Removed(1)
}
}
pendingGauge.Dec(int64(len(gapped)))
}
Expand Down
Loading
Loading