From 930dcba1fd9e98f49b4042cf2b1a7e6c24dd6591 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Thu, 28 Sep 2023 14:54:31 +0200 Subject: [PATCH 1/8] eth/fetcher: add fetcher timeout test --- eth/fetcher/tx_fetcher.go | 4 ++-- eth/fetcher/tx_fetcher_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/eth/fetcher/tx_fetcher.go b/eth/fetcher/tx_fetcher.go index 574762696869..a591ac9c472f 100644 --- a/eth/fetcher/tx_fetcher.go +++ b/eth/fetcher/tx_fetcher.go @@ -284,7 +284,7 @@ func (f *TxFetcher) Notify(peer string, types []byte, sizes []uint32, hashes []c // isKnownUnderpriced reports whether a transaction hash was recently found to be underpriced. func (f *TxFetcher) isKnownUnderpriced(hash common.Hash) bool { prevTime, ok := f.underpriced.Peek(hash) - if ok && prevTime+maxTxUnderpricedTimeout < time.Now().Unix() { + if ok && prevTime+maxTxUnderpricedTimeout < time.Now().UnixNano() { f.underpriced.Remove(hash) return false } @@ -335,7 +335,7 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool) // Avoid re-request this transaction when we receive another // announcement. if errors.Is(err, txpool.ErrUnderpriced) || errors.Is(err, txpool.ErrReplaceUnderpriced) { - f.underpriced.Add(batch[j].Hash(), batch[j].Time().Unix()) + f.underpriced.Add(batch[j].Hash(), batch[j].Time().UnixNano()) } // Track a few interesting failure types switch { diff --git a/eth/fetcher/tx_fetcher_test.go b/eth/fetcher/tx_fetcher_test.go index fbb9ff9dccc9..bd75d1791583 100644 --- a/eth/fetcher/tx_fetcher_test.go +++ b/eth/fetcher/tx_fetcher_test.go @@ -1993,3 +1993,34 @@ func containsHash(slice []common.Hash, hash common.Hash) bool { } return false } + +// Tests that a transaction is forgotten after the timeout. +func TestTransactionForgotten(t *testing.T) { + fetcher := NewTxFetcher( + func(common.Hash) bool { return false }, + func(txs []*types.Transaction) []error { + errs := make([]error, len(txs)) + for i := 0; i < len(errs); i++ { + errs[i] = txpool.ErrUnderpriced + } + return errs + }, + func(string, []common.Hash) error { return nil }, + ) + go fetcher.loop() + tx1 := types.NewTransaction(0, common.Address{}, common.Big0, 0, common.Big0, nil) + tx1.SetTime(time.Now().Add(-5 * time.Minute)) + tx2 := types.NewTransaction(1, common.Address{}, common.Big0, 0, common.Big0, nil) + if fetcher.isKnownUnderpriced(tx1.Hash()) { + t.Fatal("unknown hash can not be underpriced") + } + if err := fetcher.Enqueue("asdf", []*types.Transaction{tx1, tx2}, false); err != nil { + t.Fatal(err) + } + if fetcher.isKnownUnderpriced(tx1.Hash()) { + t.Fatal("transaction should be evicted by this point") + } + if !fetcher.isKnownUnderpriced(tx2.Hash()) { + t.Fatal("transaction should not be known underpriced") + } +} From be07a00a2647ebd94ecaf3fb74e80a156f77c0fb Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Mon, 2 Oct 2023 13:23:22 +0200 Subject: [PATCH 2/8] eth/fetcher: use time.duration --- eth/fetcher/tx_fetcher.go | 4 ++-- eth/fetcher/tx_fetcher_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/eth/fetcher/tx_fetcher.go b/eth/fetcher/tx_fetcher.go index a591ac9c472f..eb49ea67c954 100644 --- a/eth/fetcher/tx_fetcher.go +++ b/eth/fetcher/tx_fetcher.go @@ -61,7 +61,7 @@ const ( maxTxUnderpricedSetSize = 32768 // maxTxUnderpricedTimeout is the max time a transaction should be stuck in the underpriced set. - maxTxUnderpricedTimeout = int64(5 * time.Minute) + maxTxUnderpricedTimeout = 5 * time.Minute // txArriveTimeout is the time allowance before an announced transaction is // explicitly requested. @@ -284,7 +284,7 @@ func (f *TxFetcher) Notify(peer string, types []byte, sizes []uint32, hashes []c // isKnownUnderpriced reports whether a transaction hash was recently found to be underpriced. func (f *TxFetcher) isKnownUnderpriced(hash common.Hash) bool { prevTime, ok := f.underpriced.Peek(hash) - if ok && prevTime+maxTxUnderpricedTimeout < time.Now().UnixNano() { + if ok && prevTime < time.Now().Add(-maxTxUnderpricedTimeout).UnixNano() { f.underpriced.Remove(hash) return false } diff --git a/eth/fetcher/tx_fetcher_test.go b/eth/fetcher/tx_fetcher_test.go index bd75d1791583..1df5a175c34a 100644 --- a/eth/fetcher/tx_fetcher_test.go +++ b/eth/fetcher/tx_fetcher_test.go @@ -2009,7 +2009,7 @@ func TestTransactionForgotten(t *testing.T) { ) go fetcher.loop() tx1 := types.NewTransaction(0, common.Address{}, common.Big0, 0, common.Big0, nil) - tx1.SetTime(time.Now().Add(-5 * time.Minute)) + tx1.SetTime(time.Now().Add(-maxTxUnderpricedTimeout)) tx2 := types.NewTransaction(1, common.Address{}, common.Big0, 0, common.Big0, nil) if fetcher.isKnownUnderpriced(tx1.Hash()) { t.Fatal("unknown hash can not be underpriced") From 80551de81bb3660add731ea2fff4e64ca931dea0 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 3 Oct 2023 13:57:35 +0200 Subject: [PATCH 3/8] eth/fetcher: moar use types --- eth/fetcher/tx_fetcher.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/eth/fetcher/tx_fetcher.go b/eth/fetcher/tx_fetcher.go index eb49ea67c954..06e01e1205bd 100644 --- a/eth/fetcher/tx_fetcher.go +++ b/eth/fetcher/tx_fetcher.go @@ -167,7 +167,7 @@ type TxFetcher struct { drop chan *txDrop quit chan struct{} - underpriced *lru.Cache[common.Hash, int64] // Transactions discarded as too cheap (don't re-fetch) + underpriced *lru.Cache[common.Hash, time.Time] // Transactions discarded as too cheap (don't re-fetch) // Stage 1: Waiting lists for newly discovered transactions that might be // broadcast without needing explicit request/reply round trips. @@ -222,7 +222,7 @@ func NewTxFetcherForTests( fetching: make(map[common.Hash]string), requests: make(map[string]*txRequest), alternates: make(map[common.Hash]map[string]struct{}), - underpriced: lru.NewCache[common.Hash, int64](maxTxUnderpricedSetSize), + underpriced: lru.NewCache[common.Hash, time.Time](maxTxUnderpricedSetSize), hasTx: hasTx, addTxs: addTxs, fetchTxs: fetchTxs, @@ -284,7 +284,7 @@ func (f *TxFetcher) Notify(peer string, types []byte, sizes []uint32, hashes []c // isKnownUnderpriced reports whether a transaction hash was recently found to be underpriced. func (f *TxFetcher) isKnownUnderpriced(hash common.Hash) bool { prevTime, ok := f.underpriced.Peek(hash) - if ok && prevTime < time.Now().Add(-maxTxUnderpricedTimeout).UnixNano() { + if ok && prevTime.Before(time.Now().Add(-maxTxUnderpricedTimeout)) { f.underpriced.Remove(hash) return false } @@ -335,7 +335,7 @@ func (f *TxFetcher) Enqueue(peer string, txs []*types.Transaction, direct bool) // Avoid re-request this transaction when we receive another // announcement. if errors.Is(err, txpool.ErrUnderpriced) || errors.Is(err, txpool.ErrReplaceUnderpriced) { - f.underpriced.Add(batch[j].Hash(), batch[j].Time().UnixNano()) + f.underpriced.Add(batch[j].Hash(), batch[j].Time()) } // Track a few interesting failure types switch { From 296e67d8c63e2a63a452bea2353c4b9b501f2abc Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Fri, 13 Oct 2023 14:59:08 +0200 Subject: [PATCH 4/8] eth/fetcher: fix test --- eth/fetcher/tx_fetcher_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eth/fetcher/tx_fetcher_test.go b/eth/fetcher/tx_fetcher_test.go index 1df5a175c34a..0aea66f00a92 100644 --- a/eth/fetcher/tx_fetcher_test.go +++ b/eth/fetcher/tx_fetcher_test.go @@ -2009,7 +2009,7 @@ func TestTransactionForgotten(t *testing.T) { ) go fetcher.loop() tx1 := types.NewTransaction(0, common.Address{}, common.Big0, 0, common.Big0, nil) - tx1.SetTime(time.Now().Add(-maxTxUnderpricedTimeout)) + tx1.SetTime(time.Now().Add(-maxTxUnderpricedTimeout - 1*time.Second)) tx2 := types.NewTransaction(1, common.Address{}, common.Big0, 0, common.Big0, nil) if fetcher.isKnownUnderpriced(tx1.Hash()) { t.Fatal("unknown hash can not be underpriced") From 5e52d8dc8bd1ff632a9ec3ec2ac2fac1c0695aa2 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Mon, 16 Oct 2023 09:50:13 +0200 Subject: [PATCH 5/8] eth/fetcher: clean up test a bit --- eth/fetcher/tx_fetcher_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/eth/fetcher/tx_fetcher_test.go b/eth/fetcher/tx_fetcher_test.go index 0aea66f00a92..214025959f97 100644 --- a/eth/fetcher/tx_fetcher_test.go +++ b/eth/fetcher/tx_fetcher_test.go @@ -2008,19 +2008,21 @@ func TestTransactionForgotten(t *testing.T) { func(string, []common.Hash) error { return nil }, ) go fetcher.loop() - tx1 := types.NewTransaction(0, common.Address{}, common.Big0, 0, common.Big0, nil) + // Create a TX which is 5 minutes old, and one which is recent + tx1 := types.NewTx(&types.LegacyTx{Nonce: 0}) tx1.SetTime(time.Now().Add(-maxTxUnderpricedTimeout - 1*time.Second)) - tx2 := types.NewTransaction(1, common.Address{}, common.Big0, 0, common.Big0, nil) - if fetcher.isKnownUnderpriced(tx1.Hash()) { - t.Fatal("unknown hash can not be underpriced") - } + tx2 := types.NewTx(&types.LegacyTx{Nonce: 1}) + + // Enqueue both in the fetcher. They will be immediately tagged as underpriced if err := fetcher.Enqueue("asdf", []*types.Transaction{tx1, tx2}, false); err != nil { t.Fatal(err) } + // isKnownUnderpriced should trigger removal of the first tx (no longer be known underpriced) if fetcher.isKnownUnderpriced(tx1.Hash()) { - t.Fatal("transaction should be evicted by this point") + t.Fatal("transaction should be forgotten by now") } + // isKnownUnderpriced should not trigger removal of the second if !fetcher.isKnownUnderpriced(tx2.Hash()) { - t.Fatal("transaction should not be known underpriced") + t.Fatal("transaction should be known underpriced") } } From b186dedd8d26c64b05cf7e1f354218de1276b9da Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Mon, 16 Oct 2023 09:54:41 +0200 Subject: [PATCH 6/8] eth/fetcher: plug goroutine-leak in test --- eth/fetcher/tx_fetcher_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/eth/fetcher/tx_fetcher_test.go b/eth/fetcher/tx_fetcher_test.go index 214025959f97..5946cf374771 100644 --- a/eth/fetcher/tx_fetcher_test.go +++ b/eth/fetcher/tx_fetcher_test.go @@ -2007,8 +2007,10 @@ func TestTransactionForgotten(t *testing.T) { }, func(string, []common.Hash) error { return nil }, ) - go fetcher.loop() - // Create a TX which is 5 minutes old, and one which is recent + fetcher.Start() + defer fetcher.Stop() + // Create a TXuno + //which is 5 minutes old, and one which is recent tx1 := types.NewTx(&types.LegacyTx{Nonce: 0}) tx1.SetTime(time.Now().Add(-maxTxUnderpricedTimeout - 1*time.Second)) tx2 := types.NewTx(&types.LegacyTx{Nonce: 1}) From da9b640c501fe443ae509ff79c172c58fcb45f9b Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Mon, 16 Oct 2023 11:22:15 +0200 Subject: [PATCH 7/8] Update eth/fetcher/tx_fetcher_test.go --- eth/fetcher/tx_fetcher_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/eth/fetcher/tx_fetcher_test.go b/eth/fetcher/tx_fetcher_test.go index 5946cf374771..a3a44f1609d8 100644 --- a/eth/fetcher/tx_fetcher_test.go +++ b/eth/fetcher/tx_fetcher_test.go @@ -2009,8 +2009,7 @@ func TestTransactionForgotten(t *testing.T) { ) fetcher.Start() defer fetcher.Stop() - // Create a TXuno - //which is 5 minutes old, and one which is recent + // Create one TX which is 5 minutes old, and one which is recent tx1 := types.NewTx(&types.LegacyTx{Nonce: 0}) tx1.SetTime(time.Now().Add(-maxTxUnderpricedTimeout - 1*time.Second)) tx2 := types.NewTx(&types.LegacyTx{Nonce: 1}) From 2b0f2467cc76a983ed70c98ca3ac36b3a5dad958 Mon Sep 17 00:00:00 2001 From: Marius van der Wijden Date: Mon, 16 Oct 2023 11:37:40 +0200 Subject: [PATCH 8/8] eth/fetcher: rebase test --- eth/fetcher/tx_fetcher_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/eth/fetcher/tx_fetcher_test.go b/eth/fetcher/tx_fetcher_test.go index a3a44f1609d8..77b89085d317 100644 --- a/eth/fetcher/tx_fetcher_test.go +++ b/eth/fetcher/tx_fetcher_test.go @@ -2006,6 +2006,7 @@ func TestTransactionForgotten(t *testing.T) { return errs }, func(string, []common.Hash) error { return nil }, + func(string) {}, ) fetcher.Start() defer fetcher.Stop()