Skip to content

Conversation

cskiraly
Copy link
Contributor

@cskiraly cskiraly commented Oct 7, 2025

invalidTxMeter was counting txs, while validTxMeter was counting accounts.
Better make the two comparable.

@cskiraly cskiraly requested a review from rjl493456442 as a code owner October 7, 2025 11:47
@jonbarrientos02-bit

This comment was marked as spam.

pool.mu.Lock()
newErrs, dirtyAddrs := pool.addTxsLocked(news)
pool.mu.Unlock()
validTxMeter.Mark(int64(len(news)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this count all txs in "news" including the ones that error on stateful checks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a valid point raised by Matt.

maybe change like this?

diff --git a/core/txpool/legacypool/legacypool.go b/core/txpool/legacypool/legacypool.go
index 80a9faf23f..9274def280 100644
--- a/core/txpool/legacypool/legacypool.go
+++ b/core/txpool/legacypool/legacypool.go
@@ -985,16 +985,22 @@ func (pool *LegacyPool) Add(txs []*types.Transaction, sync bool) []error {
 // addTxsLocked attempts to queue a batch of transactions if they are valid.
 // The transaction pool lock must be held.
 func (pool *LegacyPool) addTxsLocked(txs []*types.Transaction) ([]error, *accountSet) {
-	dirty := newAccountSet(pool.signer)
-	errs := make([]error, len(txs))
+	var (
+		dirty = newAccountSet(pool.signer)
+		errs  = make([]error, len(txs))
+		valid int
+	)
 	for i, tx := range txs {
 		replaced, err := pool.add(tx)
 		errs[i] = err
 		if err == nil && !replaced {
 			dirty.addTx(tx)
 		}
+		if err == nil {
+			valid += 1
+		}
 	}
-	validTxMeter.Mark(int64(len(dirty.accounts)))
+	validTxMeter.Mark(int64(valid))
 	return errs, dirty
 }
 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In first instance I just wanted to TX level, without changing essence, but I think you are right.
Will fix later today, but I do want to count both valid and invalid. Maybe a new counter here for those that return err.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add in addTxsLocked updates counters on err, while it simply returns for those that are added, so it seems ok to update with valid here.

@rjl493456442 rjl493456442 added this to the 1.16.5 milestone Oct 10, 2025
@rjl493456442 rjl493456442 merged commit 4d6d5a3 into ethereum:master Oct 10, 2025
6 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants