fix(mempool): Remove tx immediately during FinalizeBlock success

mempool/mempool.go

@@ -455,8 +455,14 @@ func (m *ExperimentalEVMMempool) RemoveWithReason(ctx context.Context, tx sdk.Tx
 	hash := msgEthereumTx.Hash()
 
 	if m.shouldRemoveFromEVMPool(hash, reason) {
-		m.logger.Debug("Manually removing EVM transaction", "tx_hash", hash)
-		m.legacyTxPool.RemoveTx(hash, false, true, convertRemovalReason(reason.Caller))
+		includedInBlock := reason.Caller == sdkmempool.CallerRunTxFinalize && reason.Error == nil
+		m.logger.Debug("Manually removing EVM transaction", "tx_hash", hash, "included_in_block", includedInBlock)
+		m.legacyTxPool.RemoveTx(
+			hash,
+			txpool.WithUnreserve(),
+			txpool.WithStrictOverride(!includedInBlock), // if this tx has been included in a block, do not dequeue future txs
+			txpool.WithRemovalReason(convertRemovalReason(reason.Caller)),
+		)
 	}
 
 	if reason.Caller == sdkmempool.CallerRunTxFinalize {
@@ -498,8 +504,8 @@ func (m *ExperimentalEVMMempool) removeCosmosTx(ctx context.Context, tx sdk.Tx,
 
 // shouldRemoveFromEVMPool determines whether an EVM transaction should be manually removed.
 func (m *ExperimentalEVMMempool) shouldRemoveFromEVMPool(hash common.Hash, reason sdkmempool.RemoveReason) bool {
-	if reason.Error == nil {
-		return false
+	if reason.Caller == sdkmempool.CallerRunTxFinalize && reason.Error == nil {
+		return true
 	}
 
 	// Comet will attempt to remove transactions from the mempool after completing successfully.

mempool/txpool/legacypool/legacypool.go

@@ -484,7 +484,12 @@ func (pool *LegacyPool) loop() {
 				if time.Since(pool.beats[addr]) > pool.config.Lifetime {
 					list := pool.queue[addr].Flatten()
 					for _, tx := range list {
-						pool.removeTx(tx.Hash(), true, true, RemovalReasonLifetime)
+						pool.removeTx(
+							tx.Hash(),
+							txpool.WithUnreserve(),
+							txpool.WithOutOfBound(),
+							txpool.WithRemovalReason(RemovalReasonLifetime),
+						)
 					}
 					queuedEvictionMeter.Mark(int64(len(list)))
 				}
@@ -537,7 +542,7 @@ func (pool *LegacyPool) SetGasTip(tip *big.Int) {
 		// pool.priced is sorted by GasFeeCap, so we have to iterate through pool.all instead
 		drop := pool.all.TxsBelowTip(tip)
 		for _, tx := range drop {
-			pool.removeTx(tx.Hash(), false, true, RemovalReasonBelowTip)
+			pool.removeTx(tx.Hash(), txpool.WithUnreserve(), txpool.WithRemovalReason(RemovalReasonBelowTip))
 		}
 		pool.priced.Removed(len(drop))
 	}
@@ -877,7 +882,13 @@ func (pool *LegacyPool) add(tx *types.Transaction) (replaced bool, err error) {
 			underpricedTxMeter.Mark(1)
 
 			sender, _ := types.Sender(pool.signer, tx)
-			dropped := pool.removeTx(tx.Hash(), false, sender != from, RemovalReasonUnderpricedFull) // Don't unreserve the sender of the tx being added if last from the acc
+			opts := []txpool.RemoveTxOption{txpool.WithRemovalReason(RemovalReasonUnderpricedFull)}
+			if sender == from {
+				// Don't unreserve the sender of the tx being added if last
+				// from the acc
+				opts = append(opts, txpool.WithUnreserve())
+			}
+			dropped := pool.removeTx(tx.Hash(), opts...)
 
 			pool.changesSinceReorg += dropped
 		}
@@ -1194,33 +1205,26 @@ func (pool *LegacyPool) Has(hash common.Hash) bool {
 	return pool.all.Get(hash) != nil
 }
 
-// RemoveTx removes a single transaction from the queue, moving all subsequent
-// transactions back to the future queue.
-//
-// In unreserve is false, the account will not be relinquished to the main txpool
-// even if there are no more references to it. This is used to handle a race when
-// a tx being added, and it evicts a previously scheduled tx from the same account,
-// which could lead to a premature release of the lock.
+// RemoveTx removes a single transaction from the queue.
 //
 // Returns the number of transactions removed from the pending queue.
-func (pool *LegacyPool) RemoveTx(hash common.Hash, outofbound bool, unreserve bool, reason txpool.RemovalReason) int {
+func (pool *LegacyPool) RemoveTx(hash common.Hash, opts ...txpool.RemoveTxOption) int {
 	pool.mu.Lock()
 	defer pool.mu.Unlock()
-	return pool.removeTx(hash, outofbound, unreserve, reason)
+	return pool.removeTx(hash, opts...)
 }
 
-// removeTx removes a single transaction from the queue, moving all subsequent
-// transactions back to the future queue.
-//
-// If unreserve is false, the account will not be relinquished to the main txpool
-// even if there are no more references to it. This is used to handle a race when
-// a tx being added, and it evicts a previously scheduled tx from the same account,
-// which could lead to a premature release of the lock.
+// removeTx removes a single transaction from the queue.
 //
 // Returns the number of transactions removed from the pending queue.
 //
 // The transaction pool lock must be held.
-func (pool *LegacyPool) removeTx(hash common.Hash, outofbound bool, unreserve bool, reason txpool.RemovalReason) int {
+func (pool *LegacyPool) removeTx(hash common.Hash, opts ...txpool.RemoveTxOption) int {
+	config := txpool.NewRemoveTxConfig()
+	for _, opt := range opts {
+		opt(config)
+	}
+
 	// Fetch the transaction we wish to delete
 	tx := pool.all.Get(hash)
 	if tx == nil {
@@ -1231,7 +1235,7 @@ func (pool *LegacyPool) removeTx(hash common.Hash, outofbound bool, unreserve bo
 	// If after deletion there are no more transactions belonging to this account,
 	// relinquish the address reservation. It's a bit convoluted do this, via a
 	// defer, but it's safer vs. the many return pathways.
-	if unreserve {
+	if config.Unreserve {
 		defer func() {
 			var (
 				_, hasPending = pool.pending[addr]
@@ -1244,14 +1248,14 @@ func (pool *LegacyPool) removeTx(hash common.Hash, outofbound bool, unreserve bo
 	}
 	// Remove it from the list of known transactions
 	pool.all.Remove(hash)
-	if outofbound {
+	if config.OutOfBound {
 		pool.priced.Removed(1)
 	}
 	// Remove the transaction from the pending lists and reset the account nonce
 	if pending := pool.pending[addr]; pending != nil {
-		if removed, invalids := pending.Remove(tx); removed {
+		if removed, invalids := pending.Remove(tx, config.StrictOverride); removed {
 			pool.markTxRemoved(tx, Pending)
-			pendingRemovalMetric(reason).Mark(1)
+			pendingRemovalMetric(config.Reason).Mark(1)
 
 			// If no more pending transactions are left, remove the list
 			if pending.Empty() {
@@ -1272,9 +1276,9 @@ func (pool *LegacyPool) removeTx(hash common.Hash, outofbound bool, unreserve bo
 	}
 	// Transaction is in the future queue
 	if future := pool.queue[addr]; future != nil {
-		if removed, _ := future.Remove(tx); removed {
+		if removed, _ := future.Remove(tx, config.StrictOverride); removed {
 			pool.markTxRemoved(tx, Queue)
-			queueRemovalMetric(reason).Mark(1)
+			queueRemovalMetric(config.Reason).Mark(1)
 
 			// Reduce the queued counter
 			queuedGauge.Dec(1)
@@ -1725,7 +1729,7 @@ func (pool *LegacyPool) truncateQueue() {
 		// Drop all transactions if they are less than the overflow
 		if size := uint64(list.Len()); size <= drop {
 			for _, tx := range list.Flatten() {
-				pool.removeTx(tx.Hash(), true, true, RemovalReasonTruncatedOverflow)
+				pool.removeTx(tx.Hash(), txpool.WithOutOfBound(), txpool.WithUnreserve(), txpool.WithRemovalReason(RemovalReasonTruncatedOverflow))
 			}
 			drop -= size
 			queuedRateLimitMeter.Mark(int64(size))
@@ -1734,7 +1738,7 @@ func (pool *LegacyPool) truncateQueue() {
 		// Otherwise drop only last few transactions
 		txs := list.Flatten()
 		for i := len(txs) - 1; i >= 0 && drop > 0; i-- {
-			pool.removeTx(txs[i].Hash(), true, true, RemovalReasonTruncatedLast)
+			pool.removeTx(txs[i].Hash(), txpool.WithOutOfBound(), txpool.WithUnreserve(), txpool.WithRemovalReason(RemovalReasonTruncatedLast))
 			drop--
 			queuedRateLimitMeter.Mark(1)
 		}

mempool/txpool/legacypool/legacypool_test.go

@@ -568,7 +568,7 @@ func TestChainFork(t *testing.T) {
 	if _, err := pool.add(tx); err != nil {
 		t.Error("didn't expect error", err)
 	}
-	pool.removeTx(tx.Hash(), true, true, txpool.RemovalReason(""))
+	pool.removeTx(tx.Hash(), txpool.WithOutOfBound(), txpool.WithUnreserve())
 
 	// reset the pool's internal state
 	resetState()
@@ -2664,7 +2664,7 @@ func TestRemoveTxTruncatePoolRace(t *testing.T) {
 	go func() {
 		defer wg.Done()
 		for _, hash := range hashes {
-			_ = pool.RemoveTx(hash, false, true, "")
+			_ = pool.RemoveTx(hash, txpool.WithUnreserve())
 		}
 	}()
 

mempool/txpool/legacypool/list.go

@@ -18,6 +18,7 @@ package legacypool
 
 import (
 	"container/heap"
+	"fmt"
 	"math"
 	"math/big"
 	"slices"
@@ -479,15 +480,28 @@ func (l *list) Cap(threshold int) types.Transactions {
 // Remove deletes a transaction from the maintained list, returning whether the
 // transaction was found, and also returning any transaction invalidated due to
 // the deletion (strict mode only).
-func (l *list) Remove(tx *types.Transaction) (bool, types.Transactions) {
+//
+// If the strict parameter is non nil, it will override the lists default strict
+// behavior that was set when it was constructed.
+func (l *list) Remove(tx *types.Transaction, strict *bool) (bool, types.Transactions) {
 	// Remove the transaction from the set
 	nonce := tx.Nonce()
 	if removed := l.txs.Remove(nonce); !removed {
 		return false, nil
 	}
 	l.subTotalCost([]*types.Transaction{tx})
-	// In strict mode, filter out non-executable transactions
-	if l.strict {
+
+	// If strict param is non nil, the user wants to override the lists default
+	// strict behavior
+	var filterNonExecutables bool
+	if strict != nil {
+		filterNonExecutables = *strict
+	} else {
+		filterNonExecutables = l.strict
+	}
+
+	if filterNonExecutables {
+		fmt.Println("STRICT!!! DEQUEUING SUBSEQUENT")
 		txs := l.txs.Filter(func(tx *types.Transaction) bool { return tx.Nonce() > nonce })
 		l.subTotalCost(txs)
 		return true, txs

mempool/txpool/subpool.go

@@ -91,6 +91,68 @@ type TxMetadata struct {
 // RemovalReason is a string describing why a tx is being removed.
 type RemovalReason string
 
+// RemoveTxConfig configures how txs should be removed from the Subpool.
+type RemoveTxConfig struct {
+	// OutOfBound configures if the tx should be removed from the priced list
+	// as well.
+	OutOfBound bool
+
+	// Unreserve configures if teh account will be relinquished to the main
+	// txpool even if there are no references to it.
+	Unreserve bool
+
+	// StrictOverride determines if txs after the removed tx will also be
+	// removed.
+	StrictOverride *bool
+
+	// Reason is the reason why this tx is being removed. Used for metrics.
+	Reason RemovalReason
+}
+
+// NewRemoveTxConfig creates a new set of configuration options for removing
+// txs.
+func NewRemoveTxConfig() *RemoveTxConfig {
+	return &RemoveTxConfig{}
+}
+
+// RemoveTxOption sets values on a RemoveTxConfig
+type RemoveTxOption func(opts *RemoveTxConfig)
+
+// WithOutOfBound configures if the tx should be removed from the priced list
+// as well.
+func WithOutOfBound() RemoveTxOption {
+	return func(opts *RemoveTxConfig) {
+		opts.OutOfBound = true
+	}
+}
+
+// WithUnreserve is false, the account will not be relinquished to the main
+// txpool even if there are no more references to it. This is used to handle a
+// race when a tx being added, and it evicts a previously scheduled tx from the
+// same account, which could lead to a premature release of the lock.
+func WithUnreserve() RemoveTxOption {
+	return func(opts *RemoveTxConfig) {
+		opts.Unreserve = true
+	}
+}
+
+// WithStrictOverride if not set will default to the lists default removing
+// strictness. If set to true, this will force the list to remove all
+// subsequent nonces tx after the tx being removed. If the tx is in pending and
+// strict is true, it will enqueue all removed txs.
+func WithStrictOverride(strict bool) RemoveTxOption {
+	return func(opts *RemoveTxConfig) {
+		opts.StrictOverride = &strict
+	}
+}
+
+// WithRemovalReason specifies why a tx is being removed. This is for metrics.
+func WithRemovalReason(reason RemovalReason) RemoveTxOption {
+	return func(opts *RemoveTxConfig) {
+		opts.Reason = reason
+	}
+}
+
 // SubPool represents a specialized transaction pool that lives on its own (e.g.
 // blob pool). Since independent of how many specialized pools we have, they do
 // need to be updated in lockstep and assemble into one coherent view for block
@@ -188,5 +250,5 @@ type SubPool interface {
 	Clear()
 
 	// RemoveTx removes a tracked transaction from the pool
-	RemoveTx(hash common.Hash, outofbound bool, unreserve bool, reason RemovalReason) int
+	RemoveTx(hash common.Hash, opts ...RemoveTxOption) int
 }