eth/fetcher: Fix flaky TestTransactionForgotten test using mock clock (ethereum#31468)

Fixes ethereum#31169

The TestTransactionForgotten test was flaky due to real time
dependencies. This PR:

- Replaces real time with mock clock for deterministic timing control
- Adds precise state checks at timeout boundaries
- Verifies underpriced cache states and cleanup
- Improves test reliability by controlling transaction timestamps
- Adds checks for transaction re-enqueueing behavior

The changes ensure consistent test behavior without timing-related
flakiness.

---------

Co-authored-by: Zsolt Felfoldi <[email protected]>

Author: crStiv

eth/fetcher/tx_fetcher.go

@@ -202,22 +202,23 @@ type TxFetcher struct {
 	fetchTxs func(string, []common.Hash) error  // Retrieves a set of txs from a remote peer
 	dropPeer func(string)                       // Drops a peer in case of announcement violation
 
-	step  chan struct{} // Notification channel when the fetcher loop iterates
-	clock mclock.Clock  // Time wrapper to simulate in tests
-	rand  *mrand.Rand   // Randomizer to use in tests instead of map range loops (soft-random)
+	step     chan struct{}    // Notification channel when the fetcher loop iterates
+	clock    mclock.Clock     // Monotonic clock or simulated clock for tests
+	realTime func() time.Time // Real system time or simulated time for tests
+	rand     *mrand.Rand      // Randomizer to use in tests instead of map range loops (soft-random)
 }
 
 // NewTxFetcher creates a transaction fetcher to retrieve transaction
 // based on hash announcements.
 func NewTxFetcher(hasTx func(common.Hash) bool, addTxs func([]*types.Transaction) []error, fetchTxs func(string, []common.Hash) error, dropPeer func(string)) *TxFetcher {
-	return NewTxFetcherForTests(hasTx, addTxs, fetchTxs, dropPeer, mclock.System{}, nil)
+	return NewTxFetcherForTests(hasTx, addTxs, fetchTxs, dropPeer, mclock.System{}, time.Now, nil)
 }
 
 // NewTxFetcherForTests is a testing method to mock out the realtime clock with
 // a simulated version and the internal randomness with a deterministic one.
 func NewTxFetcherForTests(
 	hasTx func(common.Hash) bool, addTxs func([]*types.Transaction) []error, fetchTxs func(string, []common.Hash) error, dropPeer func(string),
-	clock mclock.Clock, rand *mrand.Rand) *TxFetcher {
+	clock mclock.Clock, realTime func() time.Time, rand *mrand.Rand) *TxFetcher {
 	return &TxFetcher{
 		notify:      make(chan *txAnnounce),
 		cleanup:     make(chan *txDelivery),
@@ -237,6 +238,7 @@ func NewTxFetcherForTests(
 		fetchTxs:    fetchTxs,
 		dropPeer:    dropPeer,
 		clock:       clock,
+		realTime:    realTime,
 		rand:        rand,
 	}
 }
@@ -293,7 +295,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.Before(time.Now().Add(-maxTxUnderpricedTimeout)) {
+	if ok && prevTime.Before(f.realTime().Add(-maxTxUnderpricedTimeout)) {
 		f.underpriced.Remove(hash)
 		return false
 	}

eth/fetcher/tx_fetcher_test.go

@@ -2150,9 +2150,22 @@ func containsHashInAnnounces(slice []announce, hash common.Hash) bool {
 	return false
 }
 
-// Tests that a transaction is forgotten after the timeout.
+// TestTransactionForgotten verifies that underpriced transactions are properly
+// forgotten after the timeout period, testing both the exact timeout boundary
+// and the cleanup of the underpriced cache.
 func TestTransactionForgotten(t *testing.T) {
-	fetcher := NewTxFetcher(
+	// Test ensures that underpriced transactions are properly forgotten after a timeout period,
+	// including checks for timeout boundary and cache cleanup.
+	t.Parallel()
+
+	// Create a mock clock for deterministic time control
+	mockClock := new(mclock.Simulated)
+	mockTime := func() time.Time {
+		nanoTime := int64(mockClock.Now())
+		return time.Unix(nanoTime/1000000000, nanoTime%1000000000)
+	}
+
+	fetcher := NewTxFetcherForTests(
 		func(common.Hash) bool { return false },
 		func(txs []*types.Transaction) []error {
 			errs := make([]error, len(txs))
@@ -2163,24 +2176,83 @@ func TestTransactionForgotten(t *testing.T) {
 		},
 		func(string, []common.Hash) error { return nil },
 		func(string) {},
+		mockClock,
+		mockTime,
+		rand.New(rand.NewSource(0)), // Use fixed seed for deterministic behavior
 	)
 	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 {
+	// Create two test transactions with the same timestamp
+	tx1 := types.NewTransaction(0, common.Address{}, big.NewInt(100), 21000, big.NewInt(1), nil)
+	tx2 := types.NewTransaction(1, common.Address{}, big.NewInt(100), 21000, big.NewInt(1), nil)
+
+	now := mockTime()
+	tx1.SetTime(now)
+	tx2.SetTime(now)
+
+	// Initial state: both transactions should be marked as underpriced
+	if err := fetcher.Enqueue("peer", []*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")
+	if !fetcher.isKnownUnderpriced(tx1.Hash()) {
+		t.Error("tx1 should be underpriced")
+	}
+	if !fetcher.isKnownUnderpriced(tx2.Hash()) {
+		t.Error("tx2 should be underpriced")
+	}
+
+	// Verify cache size
+	if size := fetcher.underpriced.Len(); size != 2 {
+		t.Errorf("wrong underpriced cache size: got %d, want %d", size, 2)
+	}
+
+	// Just before timeout: transactions should still be underpriced
+	mockClock.Run(maxTxUnderpricedTimeout - time.Second)
+	if !fetcher.isKnownUnderpriced(tx1.Hash()) {
+		t.Error("tx1 should still be underpriced before timeout")
+	}
+	if !fetcher.isKnownUnderpriced(tx2.Hash()) {
+		t.Error("tx2 should still be underpriced before timeout")
+	}
+
+	// Exactly at timeout boundary: transactions should still be present
+	mockClock.Run(time.Second)
+	if !fetcher.isKnownUnderpriced(tx1.Hash()) {
+		t.Error("tx1 should be present exactly at timeout")
 	}
-	// isKnownUnderpriced should not trigger removal of the second
 	if !fetcher.isKnownUnderpriced(tx2.Hash()) {
-		t.Fatal("transaction should be known underpriced")
+		t.Error("tx2 should be present exactly at timeout")
+	}
+
+	// After timeout: transactions should be forgotten
+	mockClock.Run(time.Second)
+	if fetcher.isKnownUnderpriced(tx1.Hash()) {
+		t.Error("tx1 should be forgotten after timeout")
+	}
+	if fetcher.isKnownUnderpriced(tx2.Hash()) {
+		t.Error("tx2 should be forgotten after timeout")
+	}
+
+	// Verify cache is empty
+	if size := fetcher.underpriced.Len(); size != 0 {
+		t.Errorf("wrong underpriced cache size after timeout: got %d, want 0", size)
+	}
+
+	// Re-enqueue tx1 with updated timestamp
+	tx1.SetTime(mockTime())
+	if err := fetcher.Enqueue("peer", []*types.Transaction{tx1}, false); err != nil {
+		t.Fatal(err)
+	}
+	if !fetcher.isKnownUnderpriced(tx1.Hash()) {
+		t.Error("tx1 should be underpriced after re-enqueueing with new timestamp")
+	}
+	if fetcher.isKnownUnderpriced(tx2.Hash()) {
+		t.Error("tx2 should remain forgotten")
+	}
+
+	// Verify final cache state
+	if size := fetcher.underpriced.Len(); size != 1 {
+		t.Errorf("wrong final underpriced cache size: got %d, want 1", size)
 	}
 }

tests/fuzzers/txfetcher/txfetcher_fuzzer.go

@@ -84,7 +84,12 @@ func fuzz(input []byte) int {
 		},
 		func(string, []common.Hash) error { return nil },
 		nil,
-		clock, rand,
+		clock,
+		func() time.Time {
+			nanoTime := int64(clock.Now())
+			return time.Unix(nanoTime/1000000000, nanoTime%1000000000)
+		},
+		rand,
 	)
 	f.Start()
 	defer f.Stop()