From 667966c5c10e7bf1e38a0439c62b11d5b26a132a Mon Sep 17 00:00:00 2001
From: Marius van der Wijden <m.vanderwijden@live.de>
Date: Tue, 17 Oct 2023 10:38:00 +0200
Subject: [PATCH] eth/fetcher: fix fetcher timeout (#28220)

This changes fixes a bug in the fetcher, where the timeout for how long to remember underpriced transaction was erroneously compared, and the timeout never hit.
---------

Co-authored-by: Martin Holst Swende <martin@swende.se>
---
 eth/fetcher/tx_fetcher.go      | 10 +++++-----
 eth/fetcher/tx_fetcher_test.go | 35 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/eth/fetcher/tx_fetcher.go b/eth/fetcher/tx_fetcher.go
index 385f8c5271..60b07a2b4b 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.
@@ -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+maxTxUnderpricedTimeout < time.Now().Unix() {
+	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().Unix())
+				f.underpriced.Add(batch[j].Hash(), batch[j].Time())
 			}
 			// Track a few interesting failure types
 			switch {
diff --git a/eth/fetcher/tx_fetcher_test.go b/eth/fetcher/tx_fetcher_test.go
index fbb9ff9dcc..77b89085d3 100644
--- a/eth/fetcher/tx_fetcher_test.go
+++ b/eth/fetcher/tx_fetcher_test.go
@@ -1993,3 +1993,38 @@ 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 },
+		func(string) {},
+	)
+	fetcher.Start()
+	defer fetcher.Stop()
+	// 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})
+
+	// 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 forgotten by now")
+	}
+	// isKnownUnderpriced should not trigger removal of the second
+	if !fetcher.isKnownUnderpriced(tx2.Hash()) {
+		t.Fatal("transaction should be known underpriced")
+	}
+}