Supernova TxPool Part2: Fixed eviction consistency #7352

danielradu10 · 2025-10-17T12:04:12Z

Reasoning behind the pull request

Current issue: the transactions which are removed from the list of the sender might not be removed from the txByHashMap.

Proposed changes

before doing the actual removing, check if a transaction is tracked.
solved a bug on txListForSender -> the current method was removing maximum one tx, because of incorrect handling of the next element. This issue is also solved in this PR.

Testing procedure

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

danielradu10 · 2025-10-20T08:04:57Z

txcache/autoClean.go

-		// Keep track of removed transactions
-		txHashesToEvict = append(txHashesToEvict, tx.TxHash)
+		if !tracker.IsTransactionTracked(tx) {
+			_ = listForSender.items.Remove(element)


remove only if the transaction is not tracked

danielradu10 · 2025-10-20T08:05:50Z

txcache/selectionTracker.go

 	return breadcrumb.lastNonce.Value + 1, st.latestRootHash, nil
 }

-// GetBulkOfUntrackedTransactions returns the hashes of the untracked transactions


not used anymore

danielradu10 · 2025-10-20T08:10:51Z

txcache/txCache_test.go

+		require.Nil(t, err)
+
+		cache.AddTx(createTx([]byte("tx-alice-3"), "alice", 3).withGasLimit(1500000))
+		require.Equal(t, []string{"tx-alice-1", "tx-alice-2", "tx-alice-3", "tx-alice-4"}, cache.getHashesForSender("alice"))


because the nonce=3 is tracked for alice, no tx is removed from the list.

danielradu10 · 2025-10-20T08:48:42Z

txcache/txListForSender.go

 	evictedTxHashes := make([][]byte, 0)

 	// Iterate back to front
-	for element := listForSender.items.Back(); element != nil; element = element.Prev() {


discovered a bug here.

when removing an element, the previous becomes nil, so at the first tx removed, we break the loop. which means that we can't evict more txs. the bug is solved here by first finding the next element.

Copilot

Pull Request Overview

This PR fixes eviction consistency issues in the transaction pool by ensuring tracked transactions are handled properly during eviction. The main issue was that transactions removed from sender lists might not be removed from the txByHashMap, and there was a bug in the eviction loop iteration.

Key changes:

Added tracking checks before removing transactions during eviction
Fixed iterator bug in applySizeConstraints method that was preventing proper eviction
Updated function signatures to accept tracker parameter for tracking validation

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
txcache/txListForSender.go	Fixed eviction loop iterator and added tracking validation before removal
txcache/txCache.go	Simplified eviction logic by removing redundant tracking checks
txcache/eviction.go	Added tracking validation to eviction process
txcache/autoClean.go	Added tracking validation to sweepable transaction removal
txcache/selectionTracker.go	Made `IsTransactionTracked` method public and removed unused bulk methods
txcache/txListBySenderMap.go	Updated method signature to pass tracker parameter
Test files	Updated test calls to include tracker parameter

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-20T11:49:27Z

txcache/txListForSender.go

+	for element := listForSender.items.Back(); element != nil; {
 		if !listForSender.isCapacityExceeded() {
 			break
 		}

-		listForSender.items.Remove(element)
-		listForSender.onRemovedListElement(element)
-
-		// Keep track of removed transactions
 		value := element.Value.(*WrappedTransaction)
-		evictedTxHashes = append(evictedTxHashes, value.TxHash)
+
+		prevElem := element.Prev()
+
+		if !tracker.IsTransactionTracked(value) {
+			listForSender.items.Remove(element)
+			listForSender.onRemovedListElement(element)
+
+			// Keep track of removed transactions
+			evictedTxHashes = append(evictedTxHashes, value.TxHash)
+		}
+
+		element = prevElem
 	}


The logic for removing transactions is incorrect. Currently, only untracked transactions are removed from the list and added to evictedTxHashes, but tracked transactions that exceed capacity are skipped entirely. This means capacity constraints may not be properly enforced when transactions are tracked. The removal logic should handle both cases appropriately.

yes, added already a TODO for this discussion.

Copilot · 2025-10-20T11:49:28Z

txcache/autoClean.go

+		if !tracker.IsTransactionTracked(tx) {
+			_ = listForSender.items.Remove(element)
+			listForSender.onRemovedListElement(element)
+
+			// Keep track of removed transactions
+			txHashesToEvict = append(txHashesToEvict, tx.TxHash)
+		}
+
+		element = nextElement


Similar to the previous issue, this logic only removes untracked transactions but doesn't handle tracked transactions that should be swept. If a transaction with a lower nonce is tracked, it will remain in the list even though it should be removed based on the sweeping criteria.

yes, added already a TODO for this discussion.

…ency

codecov · 2025-10-20T15:27:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.29%. Comparing base (99ac8c4) to head (dd489da).

Additional details and impacted files

@@                       Coverage Diff                        @@
##           feat/mempool-supernova-part2    #7352      +/-   ##
================================================================
- Coverage                         75.30%   75.29%   -0.01%     
================================================================
  Files                               842      842              
  Lines                            138067   138017      -50     
================================================================
- Hits                             103969   103920      -49     
- Misses                            28270    28273       +3     
+ Partials                           5828     5824       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sstanculeanu · 2025-10-21T09:53:23Z

txcache/txListForSender.go

 // AddTx adds a transaction in sender's list
 // This is a "sorted" insert
-func (listForSender *txListForSender) AddTx(tx *WrappedTransaction) (bool, [][]byte) {
+func (listForSender *txListForSender) AddTx(tx *WrappedTransaction, tracker *selectionTracker) (bool, [][]byte) {


check tracker for against nil

mradian1 · 2025-10-21T12:59:32Z

txcache/autoClean_test.go

 		hashesBeforeEviction := list.getTxHashesAsStrings()

-		list.removeSweepableTransactionsReturnHashes(uint64(3))
+		list.removeSweepableTransactionsReturnHashes(uint64(3), txCache.tracker)


shouldn't we also check autoclean for tracked transactions and also consistency between the 2 maps?

fix eviction consistency

090ee54

danielradu10 self-assigned this Oct 17, 2025

danielradu10 added type:bug Something isn't working interested:protocol labels Oct 17, 2025

fixes and bug fix

4216d71

danielradu10 commented Oct 20, 2025

View reviewed changes

danielradu10 added 2 commits October 20, 2025 13:31

added a TODO

427145b

small fix

bf23bf6

mradian1 requested a review from Copilot October 20, 2025 11:48

Copilot AI reviewed Oct 20, 2025

View reviewed changes

Base automatically changed from todos-solved to feat/mempool-supernova-part2 October 20, 2025 14:08

Merge branch 'feat/mempool-supernova-part2' into fix-eviction-consist…

dd489da

…ency

sstanculeanu reviewed Oct 21, 2025

View reviewed changes

danielradu10 added 2 commits October 21, 2025 13:02

added nil check

f7686e9

fixed race conditions

7afad8a

sstanculeanu approved these changes Oct 21, 2025

View reviewed changes

mradian1 reviewed Oct 21, 2025

View reviewed changes

Supernova TxPool Part2: Fixed eviction consistency #7352

Are you sure you want to change the base?

Supernova TxPool Part2: Fixed eviction consistency #7352

Conversation

danielradu10 commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 20, 2025

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danielradu10 commented Oct 17, 2025 •

edited

Loading