Skip to content

core/txpool/legacypool: check pending replace eligibility before eviction in add()#34665

Open
Mayveskii wants to merge 1 commit intoethereum:masterfrom
Mayveskii:fix/legacypool-restore-dropped-on-failed-pending-replace
Open

core/txpool/legacypool: check pending replace eligibility before eviction in add()#34665
Mayveskii wants to merge 1 commit intoethereum:masterfrom
Mayveskii:fix/legacypool-restore-dropped-on-failed-pending-replace

Conversation

@Mayveskii
Copy link
Copy Markdown
Contributor

Problem

In (*LegacyPool).add(), when the pool is full the code:

  1. Calls pool.priced.Discard() — removes transactions from the price heap only; pool.all is still intact.
  2. Runs the eviction loop (pool.removeTx) — permanently deletes the collected transactions from pool.all. This is the point of no return.
  3. After the eviction loop, checks pending-replacement eligibility via list.Add(tx, pool.config.PriceBump).

If list.Add returns !inserted (price bump not met), the function returns txpool.ErrReplaceUnderpriced — but the transactions evicted in step 2 are already gone from pool.all with no rollback path.

This creates a DoS vector: an attacker can craft transactions whose gasFeeCap exceeds the current underpriced floor (passing the Underpriced() check) but falls short of the pending-replacement price bump threshold. Each submission permanently evicts legitimate transactions from other senders without adding a new one.

Note the contrast with the isGapped rollback that already exists in the same function: that check runs before the eviction loop, so pool.all is still intact and pool.priced.Put(dropTx) is sufficient to restore state. The pending-replacement case has no equivalent guard.

Fix

Before the eviction loop, read the existing pending transaction via list.txs.Get(tx.Nonce()) (read-only) and reproduce the price-bump threshold comparison from list.Add(). If the incoming transaction would fail replacement, restore the price heap with pool.priced.Put() (sufficient because pool.all is untouched by Discard()) and return early.

// Before committing to the irreversible eviction, verify that any
// replacement for a pending transaction meets the price-bump requirement.
// Discard() only removes entries from the price heap; pool.all remains
// intact, so a rollback requires only pool.priced.Put() calls.
if list := pool.pending[from]; list != nil {
    if old := list.txs.Get(tx.Nonce()); old != nil {
        a := big.NewInt(100 + int64(pool.config.PriceBump))
        b := big.NewInt(100)
        thresholdFeeCap := new(big.Int).Div(new(big.Int).Mul(a, old.GasFeeCap()), b)
        thresholdTip := new(big.Int).Div(new(big.Int).Mul(a, old.GasTipCap()), b)
        if old.GasFeeCapCmp(tx) >= 0 || old.GasTipCapCmp(tx) >= 0 ||
            tx.GasFeeCapIntCmp(thresholdFeeCap) < 0 || tx.GasTipCapIntCmp(thresholdTip) < 0 {
            for _, dropTx := range drop {
                pool.priced.Put(dropTx)
            }
            pendingDiscardMeter.Mark(1)
            return false, txpool.ErrReplaceUnderpriced
        }
    }
}

When the transaction pool is full, underpriced transactions are collected
via pool.priced.Discard() and then permanently removed from pool.all through
the eviction loop (pool.removeTx). The pending-replacement eligibility check
that follows (list.Add with PriceBump) runs after this point of no return.

If list.Add returns !inserted (price bump not met), the collected transactions
are already gone from pool.all with no rollback path, causing a silent loss of
legitimate mempool entries.

Fix: before the eviction loop, verify the price-bump requirement by reading
the existing pending transaction via list.txs.Get(). Discard() only modifies
the price heap; pool.all is still intact at this point, so an early return
needs only pool.priced.Put() calls to restore the heap.
@Mayveskii Mayveskii requested a review from rjl493456442 as a code owner April 5, 2026 11:48
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