From ebc3232b49c10b249286016301842119b02e7878 Mon Sep 17 00:00:00 2001 From: Kuwon Sebastian Na Date: Sat, 1 Mar 2025 21:58:57 +0900 Subject: [PATCH] eth: do not add failed tx to localTxTracker (#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 --- core/txpool/blobpool/blobpool.go | 19 ++++++++++++------- core/txpool/legacypool/legacypool.go | 11 ++++------- core/txpool/locals/tx_tracker.go | 26 ++++++++++++++++++++++---- core/txpool/subpool.go | 6 ++++++ core/txpool/txpool.go | 11 +++++++++++ eth/api_backend.go | 16 +++++++++++++--- 6 files changed, 68 insertions(+), 21 deletions(-) diff --git a/core/txpool/blobpool/blobpool.go b/core/txpool/blobpool/blobpool.go index 51b8b67c61..7ad95612bf 100644 --- a/core/txpool/blobpool/blobpool.go +++ b/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) diff --git a/core/txpool/legacypool/legacypool.go b/core/txpool/legacypool/legacypool.go index fb10060496..1cc6856663 100644 --- a/core/txpool/legacypool/legacypool.go +++ b/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) diff --git a/core/txpool/locals/tx_tracker.go b/core/txpool/locals/tx_tracker.go index 0113646669..eccdcf422a 100644 --- a/core/txpool/locals/tx_tracker.go +++ b/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. diff --git a/core/txpool/subpool.go b/core/txpool/subpool.go index 5ad0f5b0e0..242af02136 100644 --- a/core/txpool/subpool.go +++ b/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. diff --git a/core/txpool/txpool.go b/core/txpool/txpool.go index 0ebf4c7e4b..55812415f9 100644 --- a/core/txpool/txpool.go +++ b/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. diff --git a/eth/api_backend.go b/eth/api_backend.go index 853c786775..75b039dbe1 100644 --- a/eth/api_backend.go +++ b/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 + } } - return b.eth.txPool.Add([]*types.Transaction{signedTx}, false)[0] + // 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 nil } func (b *EthAPIBackend) GetPoolTransactions() (types.Transactions, error) {