Repository navigation
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.
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
Changes from 1 commit
7855495
14d0788
fd742e0
d3a14cd
f66ccc8
dd54f7a
70edfc4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -700,6 +700,12 @@ 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 target list's tracked | ||
| // total cost, before evicting cheaper remote transactions. | ||
| if err := pool.targetList(from, tx).addCostOverflow(tx); err != nil { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] Consider having if l := pool.targetList(from, tx); l != nil {
if err := l.addCostOverflow(tx); err != nil {
return false, err
}
}
|
||
| 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()) | ||
|
|
@@ -762,7 +768,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 | ||
|
|
@@ -792,6 +801,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) | ||
|
amir-deris marked this conversation as resolved.
|
||
| } | ||
|
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 | ||
|
|
@@ -825,7 +845,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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 That matters because all three unreserve sites gate on the queue entry's absence — This is pre-existing for the 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
}( |
||
| } | ||
| if !inserted { | ||
| // An older transaction was better, discard this | ||
| queuedDiscardMeter.Mark(1) | ||
|
|
@@ -867,7 +890,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 { | ||
|
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) | ||
|
|
@@ -1093,7 +1122,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()) | ||
|
|
@@ -1428,11 +1460,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 | ||
|
amir-deris marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] In practice |
||
| // 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))) | ||
|
|
@@ -1620,7 +1662,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 { | ||
|
amir-deris marked this conversation as resolved.
|
||
| pool.all.Remove(hash) | ||
| pool.priced.Removed(1) | ||
| } | ||
| } | ||
| pendingGauge.Dec(int64(len(olds) + len(drops) + len(invalids))) | ||
|
|
||
|
|
@@ -1632,7 +1677,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))) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| // Copyright 2026 The go-ethereum Authors | ||
| // This file is part of the go-ethereum library. | ||
| // | ||
| // The go-ethereum library is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU Lesser General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
| // | ||
| // The go-ethereum library is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU Lesser General Public License for more details. | ||
| // | ||
| // You should have received a copy of the GNU Lesser General Public License | ||
| // along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| package legacypool | ||
|
|
||
| import ( | ||
| "crypto/ecdsa" | ||
| "errors" | ||
| "math/big" | ||
| "testing" | ||
|
|
||
| "github.com/ethereum/go-ethereum/common" | ||
| "github.com/ethereum/go-ethereum/common/math" | ||
| "github.com/ethereum/go-ethereum/core/txpool" | ||
| "github.com/ethereum/go-ethereum/core/types" | ||
| ) | ||
|
|
||
| func valueTx(nonce uint64, value *big.Int, key *ecdsa.PrivateKey) *types.Transaction { | ||
| const intrinsicGas = 21000 | ||
| tx, _ := types.SignTx(types.NewTx(&types.LegacyTx{ | ||
| Nonce: nonce, | ||
| To: &common.Address{}, | ||
| Value: value, | ||
| Gas: intrinsicGas, | ||
| GasPrice: big.NewInt(1), | ||
| }), types.HomesteadSigner{}, key) | ||
| return tx | ||
| } | ||
|
|
||
| func maxAffordableValue(extra *big.Int) *big.Int { | ||
|
amir-deris marked this conversation as resolved.
|
||
| value := new(big.Int).Sub(math.MaxBig256, big.NewInt(21000)) | ||
| if extra != nil { | ||
| value.Sub(value, extra) | ||
| } | ||
| return value | ||
| } | ||
|
|
||
| // TestAddRemoteTotalCostOverflowError verifies that a brand-new queued transaction | ||
| // rejected for total-cost overflow surfaces ErrTotalCostOverflow rather than | ||
| // ErrReplaceUnderpriced. | ||
| func TestAddRemoteTotalCostOverflowError(t *testing.T) { | ||
|
amir-deris marked this conversation as resolved.
|
||
| pool, key := setupPool() | ||
| defer pool.Close() | ||
|
|
||
| from, _ := types.Sender(types.HomesteadSigner{}, valueTx(0, big.NewInt(1), key)) | ||
| testAddBalance(pool, from, math.MaxBig256) | ||
|
|
||
| if err := pool.addRemote(valueTx(0, big.NewInt(1), key)); err != nil { | ||
| t.Fatalf("failed to add base transaction: %v", err) | ||
| } | ||
|
|
||
| filler := valueTx(1, maxAffordableValue(nil), key) | ||
| pool.mu.Lock() | ||
| if pool.queue[from] == nil { | ||
| pool.queue[from] = newList(false) | ||
| } | ||
| if _, _, err := pool.queue[from].Add(filler, pool.config.PriceBump); err != nil { | ||
| t.Fatalf("failed to seed queue filler: %v", err) | ||
|
amir-deris marked this conversation as resolved.
Outdated
|
||
| } | ||
| pool.all.Add(filler) | ||
| pool.mu.Unlock() | ||
|
|
||
| err := pool.addRemote(valueTx(2, big.NewInt(1), key)) | ||
| if !errors.Is(err, txpool.ErrTotalCostOverflow) { | ||
| t.Fatalf("expected ErrTotalCostOverflow, got %v", err) | ||
| } | ||
| if err := validatePoolInternals(pool); err != nil { | ||
| t.Fatalf("pool internals inconsistent: %v", err) | ||
| } | ||
| } | ||
|
|
||
| // TestPromoteExecutablesStopsOnOverflow verifies that promotion halts after a | ||
| // total-cost overflow instead of leaving a nonce gap in pending. | ||
| func TestPromoteExecutablesStopsOnOverflow(t *testing.T) { | ||
| pool, key := setupPool() | ||
| defer pool.Close() | ||
|
|
||
| from, _ := types.Sender(types.HomesteadSigner{}, valueTx(0, maxAffordableValue(nil), key)) | ||
| tx0 := valueTx(0, maxAffordableValue(nil), key) | ||
| tx1 := valueTx(1, big.NewInt(1), key) | ||
| tx2 := valueTx(2, big.NewInt(1), key) | ||
|
|
||
| pool.mu.Lock() | ||
| pool.pending[from] = newList(true) | ||
| if _, _, err := pool.pending[from].Add(tx0, pool.config.PriceBump); err != nil { | ||
| t.Fatalf("failed to seed pending tx0: %v", err) | ||
| } | ||
| pool.all.Add(tx0) | ||
| pool.pendingNonces.set(from, 1) | ||
|
|
||
| pool.queue[from] = newList(false) | ||
| if _, _, err := pool.queue[from].Add(tx1, pool.config.PriceBump); err != nil { | ||
| t.Fatalf("failed to seed queue tx1: %v", err) | ||
| } | ||
| if _, _, err := pool.queue[from].Add(tx2, pool.config.PriceBump); err != nil { | ||
| t.Fatalf("failed to seed queue tx2: %v", err) | ||
| } | ||
| pool.all.Add(tx1) | ||
| pool.all.Add(tx2) | ||
| pool.mu.Unlock() | ||
|
|
||
| testAddBalance(pool, from, math.MaxBig256) | ||
| pool.promoteExecutables([]common.Address{from}) | ||
|
|
||
| pool.mu.RLock() | ||
| defer pool.mu.RUnlock() | ||
|
|
||
| pending := pool.pending[from] | ||
| if pending != nil && pending.Contains(2) { | ||
| t.Fatalf("pending must not contain nonce 2 after overflow at nonce 1") | ||
| } | ||
| if pending == nil || !pending.Contains(0) { | ||
| t.Fatalf("pending should contain nonce 0") | ||
| } | ||
| if pool.queue[from] == nil || !pool.queue[from].Contains(2) { | ||
| t.Fatalf("nonce 2 should remain queued after promotion stopped") | ||
| } | ||
| if pool.all.Get(tx1.Hash()) != nil { | ||
| t.Fatalf("overflowing promoted transaction should be dropped from pool.all") | ||
| } | ||
| if err := validatePoolInternals(pool); err != nil { | ||
|
amir-deris marked this conversation as resolved.
|
||
| t.Fatalf("pool internals inconsistent: %v", err) | ||
| } | ||
| } | ||
|
|
||
| // TestDemoteReenqueueTotalCostOverflow verifies demoted transactions that cannot | ||
| // be re-queued due to overflow are dropped instead of lingering in pool.all. | ||
| func TestDemoteReenqueueTotalCostOverflow(t *testing.T) { | ||
| pool, key := setupPool() | ||
| defer pool.Close() | ||
|
|
||
| from, _ := types.Sender(types.HomesteadSigner{}, valueTx(0, big.NewInt(1), key)) | ||
| tx0 := valueTx(0, big.NewInt(1), key) | ||
| tx1 := valueTx(1, big.NewInt(1), key) | ||
| filler := valueTx(100, maxAffordableValue(nil), key) | ||
|
|
||
| pool.mu.Lock() | ||
| pool.pending[from] = newList(true) | ||
| if _, _, err := pool.pending[from].Add(tx0, pool.config.PriceBump); err != nil { | ||
| t.Fatalf("failed to seed pending tx0: %v", err) | ||
| } | ||
| if _, _, err := pool.pending[from].Add(tx1, pool.config.PriceBump); err != nil { | ||
| t.Fatalf("failed to seed pending tx1: %v", err) | ||
| } | ||
| pool.queue[from] = newList(false) | ||
| if _, _, err := pool.queue[from].Add(filler, pool.config.PriceBump); err != nil { | ||
| t.Fatalf("failed to seed queue filler: %v", err) | ||
| } | ||
| for _, tx := range append(pool.pending[from].Flatten(), filler) { | ||
| pool.all.Add(tx) | ||
| } | ||
| pool.mu.Unlock() | ||
|
|
||
| pool.removeTx(tx0.Hash(), false, true) | ||
|
|
||
| pool.mu.RLock() | ||
| defer pool.mu.RUnlock() | ||
|
|
||
| if pool.all.Get(tx1.Hash()) != nil { | ||
| t.Fatalf("overflowing demoted transaction should be removed from pool.all") | ||
| } | ||
| if err := validatePoolInternals(pool); err != nil { | ||
| t.Fatalf("pool internals inconsistent: %v", err) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.