eth: do not add failed tx to localTxTracker (ethereum#31202)

In transaction-sending APIs such as `eth_sendRawTransaction`, a submitted transaction 
failing the configured txpool validation rules (i.e. fee too low) would cause an error to be
returned, even though the transaction was successfully added into the locals tracker.
Once added there, the transaction may even be included into the chain at a later time,
when fee market conditions change.

This change improves on this by performing the validation in the locals tracker, basically
skipping some of the validation rules for local transactions. We still try to add the tx to the
main pool immediately, but an error will only be returned for transactions which are 
fundamentally invalid.

---------

Co-authored-by: Gary Rong <[email protected]>

Author: 2 people

core/txpool/blobpool/blobpool.go

@@ -1085,19 +1085,24 @@ func (p *BlobPool) SetGasTip(tip *big.Int) {
 	p.updateStorageMetrics()
 }
 
-// validateTx checks whether a transaction is valid according to the consensus
-// rules and adheres to some heuristic limits of the local node (price and size).
-func (p *BlobPool) validateTx(tx *types.Transaction) error {
-	// Ensure the transaction adheres to basic pool filters (type, size, tip) and
-	// consensus rules
-	baseOpts := &txpool.ValidationOptions{
+// ValidateTxBasics checks whether a transaction is valid according to the consensus
+// rules, but does not check state-dependent validation such as sufficient balance.
+// This check is meant as an early check which only needs to be performed once,
+// and does not require the pool mutex to be held.
+func (p *BlobPool) ValidateTxBasics(tx *types.Transaction) error {
+	opts := &txpool.ValidationOptions{
 		Config:  p.chain.Config(),
 		Accept:  1 << types.BlobTxType,
 		MaxSize: txMaxSize,
 		MinTip:  p.gasTip.ToBig(),
 	}
+	return txpool.ValidateTransaction(tx, p.head, p.signer, opts)
+}
 
-	if err := p.txValidationFn(tx, p.head, p.signer, baseOpts); err != nil {
+// validateTx checks whether a transaction is valid according to the consensus
+// rules and adheres to some heuristic limits of the local node (price and size).
+func (p *BlobPool) validateTx(tx *types.Transaction) error {
+	if err := p.ValidateTxBasics(tx); err != nil {
 		return err
 	}
 	// Ensure the transaction adheres to the stateful pool filters (nonce, balance)

core/txpool/legacypool/legacypool.go

@@ -558,11 +558,11 @@ func (pool *LegacyPool) Pending(filter txpool.PendingFilter) map[common.Address]
 	return pending
 }
 
-// validateTxBasics checks whether a transaction is valid according to the consensus
+// ValidateTxBasics checks whether a transaction is valid according to the consensus
 // rules, but does not check state-dependent validation such as sufficient balance.
 // This check is meant as an early check which only needs to be performed once,
 // and does not require the pool mutex to be held.
-func (pool *LegacyPool) validateTxBasics(tx *types.Transaction) error {
+func (pool *LegacyPool) ValidateTxBasics(tx *types.Transaction) error {
 	opts := &txpool.ValidationOptions{
 		Config: pool.chainconfig,
 		Accept: 0 |
@@ -573,10 +573,7 @@ func (pool *LegacyPool) validateTxBasics(tx *types.Transaction) error {
 		MaxSize: txMaxSize,
 		MinTip:  pool.gasTip.Load().ToBig(),
 	}
-	if err := txpool.ValidateTransaction(tx, pool.currentHead.Load(), pool.signer, opts); err != nil {
-		return err
-	}
-	return nil
+	return txpool.ValidateTransaction(tx, pool.currentHead.Load(), pool.signer, opts)
 }
 
 // validateTx checks whether a transaction is valid according to the consensus
@@ -929,7 +926,7 @@ func (pool *LegacyPool) Add(txs []*types.Transaction, sync bool) []error {
 		// Exclude transactions with basic errors, e.g invalid signatures and
 		// insufficient intrinsic gas as soon as possible and cache senders
 		// in transactions before obtaining lock
-		if err := pool.validateTxBasics(tx); err != nil {
+		if err := pool.ValidateTxBasics(tx); err != nil {
 			errs[i] = err
 			log.Trace("Discarding invalid transaction", "hash", tx.Hash(), "err", err)
 			invalidTxMeter.Mark(1)

core/txpool/locals/tx_tracker.go

@@ -74,28 +74,45 @@ func New(journalPath string, journalTime time.Duration, chainConfig *params.Chai
 
 // Track adds a transaction to the tracked set.
 // Note: blob-type transactions are ignored.
-func (tracker *TxTracker) Track(tx *types.Transaction) {
-	tracker.TrackAll([]*types.Transaction{tx})
+func (tracker *TxTracker) Track(tx *types.Transaction) error {
+	return tracker.TrackAll([]*types.Transaction{tx})[0]
 }
 
 // TrackAll adds a list of transactions to the tracked set.
 // Note: blob-type transactions are ignored.
-func (tracker *TxTracker) TrackAll(txs []*types.Transaction) {
+func (tracker *TxTracker) TrackAll(txs []*types.Transaction) []error {
 	tracker.mu.Lock()
 	defer tracker.mu.Unlock()
 
+	var errors []error
 	for _, tx := range txs {
 		if tx.Type() == types.BlobTxType {
+			errors = append(errors, nil)
+			continue
+		}
+		// Ignore the transactions which are failed for fundamental
+		// validation such as invalid parameters.
+		if err := tracker.pool.ValidateTxBasics(tx); err != nil {
+			log.Debug("Invalid transaction submitted", "hash", tx.Hash(), "err", err)
+			errors = append(errors, err)
 			continue
 		}
 		// If we're already tracking it, it's a no-op
 		if _, ok := tracker.all[tx.Hash()]; ok {
+			errors = append(errors, nil)
 			continue
 		}
+		// Theoretically, checking the error here is unnecessary since sender recovery
+		// is already part of basic validation. However, retrieving the sender address
+		// from the transaction cache is effectively a no-op if it was previously verified.
+		// Therefore, the error is still checked just in case.
 		addr, err := types.Sender(tracker.signer, tx)
-		if err != nil { // Ignore this tx
+		if err != nil {
+			errors = append(errors, err)
 			continue
 		}
+		errors = append(errors, nil)
+
 		tracker.all[tx.Hash()] = tx
 		if tracker.byAddr[addr] == nil {
 			tracker.byAddr[addr] = legacypool.NewSortedMap()
@@ -107,6 +124,7 @@ func (tracker *TxTracker) TrackAll(txs []*types.Transaction) {
 		}
 	}
 	localGauge.Update(int64(len(tracker.all)))
+	return errors
 }
 
 // recheck checks and returns any transactions that needs to be resubmitted.

core/txpool/subpool.go

@@ -129,6 +129,12 @@ type SubPool interface {
 	// retrieve blobs from the pools directly instead of the network.
 	GetBlobs(vhashes []common.Hash) ([]*kzg4844.Blob, []*kzg4844.Proof)
 
+	// ValidateTxBasics checks whether a transaction is valid according to the consensus
+	// rules, but does not check state-dependent validation such as sufficient balance.
+	// This check is meant as a static check which can be performed without holding the
+	// pool mutex.
+	ValidateTxBasics(tx *types.Transaction) error
+
 	// Add enqueues a batch of transactions into the pool if they are valid. Due
 	// to the large transaction churn, add may postpone fully integrating the tx
 	// to a later point to batch multiple ones together.

core/txpool/txpool.go

@@ -325,6 +325,17 @@ func (p *TxPool) GetBlobs(vhashes []common.Hash) ([]*kzg4844.Blob, []*kzg4844.Pr
 	return nil, nil
 }
 
+// ValidateTxBasics checks whether a transaction is valid according to the consensus
+// rules, but does not check state-dependent validation such as sufficient balance.
+func (p *TxPool) ValidateTxBasics(tx *types.Transaction) error {
+	for _, subpool := range p.subpools {
+		if subpool.Filter(tx) {
+			return subpool.ValidateTxBasics(tx)
+		}
+	}
+	return fmt.Errorf("%w: received type %d", core.ErrTxTypeNotSupported, tx.Type())
+}
+
 // Add enqueues a batch of transactions into the pool if they are valid. Due
 // to the large transaction churn, add may postpone fully integrating the tx
 // to a later point to batch multiple ones together.

eth/api_backend.go

@@ -272,10 +272,20 @@ func (b *EthAPIBackend) SubscribeLogsEvent(ch chan<- []*types.Log) event.Subscri
 }
 
 func (b *EthAPIBackend) SendTx(ctx context.Context, signedTx *types.Transaction) error {
-	if locals := b.eth.localTxTracker; locals != nil {
-		locals.Track(signedTx)
+	locals := b.eth.localTxTracker
+	if locals != nil {
+		if err := locals.Track(signedTx); err != nil {
+			return err
+		}
+	}
+	// No error will be returned to user if the transaction fails stateful
+	// validation (e.g., no available slot), as the locally submitted transactions
+	// may be resubmitted later via the local tracker.
+	err := b.eth.txPool.Add([]*types.Transaction{signedTx}, false)[0]
+	if err != nil && locals == nil {
+		return err
 	}
-	return b.eth.txPool.Add([]*types.Transaction{signedTx}, false)[0]
+	return nil
 }
 
 func (b *EthAPIBackend) GetPoolTransactions() (types.Transactions, error) {