From 8402544066c2a10ea05acfb95ba97d8df4443fa5 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 5 Apr 2023 10:59:32 +0200 Subject: [PATCH] core/txpool: disallow future churn by remote txs (#26907) Prior to this change, it was possible that transactions are erroneously deemed as 'future' although they are in fact 'pending', causing them to be dropped due to 'future' not being allowed to replace 'pending'. This change fixes that, by doing a more in-depth inspection of the queue. --- core/txpool/list.go | 8 ++++---- core/txpool/txpool.go | 34 +++++++++++++++++++++------------- core/txpool/txpool2_test.go | 28 ++++++++++++++++++++++++++-- 3 files changed, 51 insertions(+), 19 deletions(-) diff --git a/core/txpool/list.go b/core/txpool/list.go index 639d69bcbed4..fae7c2fcac2d 100644 --- a/core/txpool/list.go +++ b/core/txpool/list.go @@ -270,10 +270,10 @@ func newList(strict bool) *list { } } -// Overlaps returns whether the transaction specified has the same nonce as one -// already contained within the list. -func (l *list) Overlaps(tx *types.Transaction) bool { - return l.txs.Get(tx.Nonce()) != nil +// Contains returns whether the list contains a transaction +// with the provided nonce. +func (l *list) Contains(nonce uint64) bool { + return l.txs.Get(nonce) != nil } // Add tries to insert a new transaction into the list, returning whether the diff --git a/core/txpool/txpool.go b/core/txpool/txpool.go index 3a5ed695681c..9eb19b0094ba 100644 --- a/core/txpool/txpool.go +++ b/core/txpool/txpool.go @@ -745,11 +745,11 @@ func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err e } // If the new transaction is a future transaction it should never churn pending transactions - if !isLocal && pool.isFuture(from, tx) { + if !isLocal && pool.isGapped(from, tx) { var replacesPending bool for _, dropTx := range drop { dropSender, _ := types.Sender(pool.signer, dropTx) - if list := pool.pending[dropSender]; list != nil && list.Overlaps(dropTx) { + if list := pool.pending[dropSender]; list != nil && list.Contains(dropTx.Nonce()) { replacesPending = true break } @@ -774,7 +774,7 @@ func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err e } // Try to replace an existing transaction in the pending pool - if list := pool.pending[from]; list != nil && list.Overlaps(tx) { + 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) if !inserted { @@ -817,18 +817,26 @@ func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err e return replaced, nil } -// isFuture reports whether the given transaction is immediately executable. -func (pool *TxPool) isFuture(from common.Address, tx *types.Transaction) bool { - list := pool.pending[from] - if list == nil { - return pool.pendingNonces.get(from) != tx.Nonce() +// isGapped reports whether the given transaction is immediately executable. +func (pool *TxPool) isGapped(from common.Address, tx *types.Transaction) bool { + // Short circuit if transaction matches pending nonce and can be promoted + // to pending list as an executable transaction. + next := pool.pendingNonces.get(from) + if tx.Nonce() == next { + return false } - // Sender has pending transactions. - if old := list.txs.Get(tx.Nonce()); old != nil { - return false // It replaces a pending transaction. + // The transaction has a nonce gap with pending list, it's only considered + // as executable if transactions in queue can fill up the nonce gap. + queue, ok := pool.queue[from] + if !ok { + return true } - // Not replacing, check if parent nonce exists in pending. - return list.txs.Get(tx.Nonce()-1) == nil + for nonce := next; nonce < tx.Nonce(); nonce++ { + if !queue.Contains(nonce) { + return true // txs in queue can't fill up the nonce gap + } + } + return false } // enqueueTx inserts a new transaction into the non-executable transaction queue. diff --git a/core/txpool/txpool2_test.go b/core/txpool/txpool2_test.go index 6d84975d83f9..7e2a9eb908d3 100644 --- a/core/txpool/txpool2_test.go +++ b/core/txpool/txpool2_test.go @@ -42,7 +42,7 @@ func count(t *testing.T, pool *TxPool) (pending int, queued int) { return pending, queued } -func fillPool(t *testing.T, pool *TxPool) { +func fillPool(t testing.TB, pool *TxPool) { t.Helper() // Create a number of test accounts, fund them and make transactions executableTxs := types.Transactions{} @@ -189,7 +189,7 @@ func TestTransactionZAttack(t *testing.T) { key, _ := crypto.GenerateKey() pool.currentState.AddBalance(crypto.PubkeyToAddress(key.PublicKey), big.NewInt(100000000000)) for j := 0; j < int(pool.config.GlobalSlots); j++ { - overDraftTxs = append(overDraftTxs, pricedValuedTransaction(uint64(j), 60000000000, 21000, big.NewInt(500), key)) + overDraftTxs = append(overDraftTxs, pricedValuedTransaction(uint64(j), 600000000000, 21000, big.NewInt(500), key)) } } pool.AddRemotesSync(overDraftTxs) @@ -210,3 +210,27 @@ func TestTransactionZAttack(t *testing.T) { newIvPending, ivPending, pool.config.GlobalSlots, newQueued) } } + +func BenchmarkFutureAttack(b *testing.B) { + // Create the pool to test the limit enforcement with + statedb, _ := state.New(common.Hash{}, state.NewDatabase(rawdb.NewMemoryDatabase()), nil) + blockchain := newTestBlockChain(1000000, statedb, new(event.Feed)) + config := testTxPoolConfig + config.GlobalQueue = 100 + config.GlobalSlots = 100 + pool := NewTxPool(config, eip1559Config, blockchain) + defer pool.Stop() + fillPool(b, pool) + + key, _ := crypto.GenerateKey() + pool.currentState.AddBalance(crypto.PubkeyToAddress(key.PublicKey), big.NewInt(100000000000)) + futureTxs := types.Transactions{} + + for n := 0; n < b.N; n++ { + futureTxs = append(futureTxs, pricedTransaction(1000+uint64(n), 100000, big.NewInt(500), key)) + } + b.ResetTimer() + for i := 0; i < 5; i++ { + pool.AddRemotesSync(futureTxs) + } +}