From 35d2bedd40f1e879292922c8b2553e2116bd5ccf Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Thu, 16 Mar 2023 15:14:35 +0100 Subject: [PATCH 1/6] core/txpool: check queued transactions as well --- core/txpool/txpool.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/core/txpool/txpool.go b/core/txpool/txpool.go index ac4486c6cba8..76b886e34b74 100644 --- a/core/txpool/txpool.go +++ b/core/txpool/txpool.go @@ -812,6 +812,15 @@ func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err e func (pool *TxPool) isFuture(from common.Address, tx *types.Transaction) bool { list := pool.pending[from] if list == nil { + // Transactions are first inserted into the queued list and bubbled up from there. + // Thus we also need to check the queued list as well. + queued := pool.queue[from] + if queued != nil { + if old := queued.txs.Get(tx.Nonce()); old != nil { + return false + } + return queued.txs.Get(tx.Nonce()-1) != nil + } return pool.pendingNonces.get(from) != tx.Nonce() } // Sender has pending transactions. From 8110738e2aa5a07d9fa9f1dfcb81403ae5f32251 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Fri, 17 Mar 2023 10:05:10 +0100 Subject: [PATCH 2/6] core/txpool: apply gary's suggestion --- core/txpool/list.go | 8 ++++---- core/txpool/txpool.go | 43 +++++++++++++++++++++---------------------- 2 files changed, 25 insertions(+), 26 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 76b886e34b74..311fcdb127a9 100644 --- a/core/txpool/txpool.go +++ b/core/txpool/txpool.go @@ -736,11 +736,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 } @@ -765,7 +765,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 { @@ -808,27 +808,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 { - // Transactions are first inserted into the queued list and bubbled up from there. - // Thus we also need to check the queued list as well. - queued := pool.queue[from] - if queued != nil { - if old := queued.txs.Get(tx.Nonce()); old != nil { - return false - } - return queued.txs.Get(tx.Nonce()-1) != 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 as executable one. + 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 + } + for nonce := next; nonce < tx.Nonce(); nonce++ { + if !queue.Contains(nonce) { + return true // txs in queue can't fill up the nonce gap + } } - // Not replacing, check if parent nonce exists in pending. - return list.txs.Get(tx.Nonce()-1) == nil + return false } // enqueueTx inserts a new transaction into the non-executable transaction queue. From 8f51b84e70f4bd32dc4f0b2adfefb38e1def8e73 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Mon, 20 Mar 2023 09:57:12 +0100 Subject: [PATCH 3/6] core/txpool: correctly overdraft transaction This commit changes a very subtle issue in the test, where the transactions did not actually overdraft the sender. The issue was not found previously, because the future rule was too strict --- core/txpool/txpool2_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/txpool/txpool2_test.go b/core/txpool/txpool2_test.go index 6d84975d83f9..fea94a122bcd 100644 --- a/core/txpool/txpool2_test.go +++ b/core/txpool/txpool2_test.go @@ -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) From d0e93186934bb9deee99f34f78ef857cf165fb11 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 29 Mar 2023 11:53:52 +0200 Subject: [PATCH 4/6] core/txpool: fix comment --- core/txpool/txpool.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/txpool/txpool.go b/core/txpool/txpool.go index 311fcdb127a9..b5f7d4a6266f 100644 --- a/core/txpool/txpool.go +++ b/core/txpool/txpool.go @@ -811,7 +811,7 @@ func (pool *TxPool) add(tx *types.Transaction, local bool) (replaced bool, err e // 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 as executable one. + // to pending list as an executable transaction. next := pool.pendingNonces.get(from) if tx.Nonce() == next { return false From b392c15e4bb17b6d82041c74bfa85503f86a9d5e Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Wed, 29 Mar 2023 14:50:54 +0200 Subject: [PATCH 5/6] core/txpool: add future attack benchmark --- core/txpool/txpool2_test.go | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/core/txpool/txpool2_test.go b/core/txpool/txpool2_test.go index fea94a122bcd..dd7f0da29f3e 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{} @@ -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{} + + b.ResetTimer() + for n := 0; n < b.N; n++ { + futureTxs = append(futureTxs, pricedTransaction(1000+uint64(n), 100000, big.NewInt(500), key)) + } + for i := 0; i < 5; i++ { + pool.AddRemotesSync(futureTxs) + } +} From f2287b609e7a9b4fef24d033f845272e4f2dc790 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Wed, 5 Apr 2023 03:38:28 -0400 Subject: [PATCH 6/6] Update core/txpool/txpool2_test.go --- core/txpool/txpool2_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/txpool/txpool2_test.go b/core/txpool/txpool2_test.go index dd7f0da29f3e..7e2a9eb908d3 100644 --- a/core/txpool/txpool2_test.go +++ b/core/txpool/txpool2_test.go @@ -226,10 +226,10 @@ func BenchmarkFutureAttack(b *testing.B) { pool.currentState.AddBalance(crypto.PubkeyToAddress(key.PublicKey), big.NewInt(100000000000)) futureTxs := types.Transactions{} - b.ResetTimer() 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) }