Merge pull request #450 from NilFoundation/fix-txpool-replacement

Fix replacement in txpool

nil/services/txnpool/txnpool.go

@@ -1,7 +1,6 @@
 package txnpool
 
 import (
-	"bytes"
 	"container/heap"
 	"context"
 	"fmt"
@@ -16,6 +15,11 @@ import (
 	"github.com/rs/zerolog"
 )
 
+// FeeBumpPercentage is the percentage of the priorityFee that a transaction must exceed to replace another transaction.
+// For example, if the priorityFee of a transaction is 100 and FeeBumpPercentage is 5, then the transaction must have a
+// priorityFee of at least 105 to replace the existing transaction.
+const FeeBumpPercentage = 5
+
 type Pool interface {
 	Add(ctx context.Context, txns ...*types.Transaction) ([]DiscardReason, error)
 	Discard(ctx context.Context, txns []common.Hash, reason DiscardReason) error
@@ -229,18 +233,8 @@ func (p *TxnPool) getLocked(hash common.Hash) *metaTxn {
 }
 
 func shouldReplace(existing, candidate *metaTxn) bool {
-	if candidate.FeeCredit.Cmp(existing.FeeCredit) <= 0 {
-		return false
-	}
-
-	// Discard the previous transaction if it is the same but at a lower fee
-	existingFee := existing.FeeCredit
-	existing.FeeCredit = candidate.FeeCredit
-	defer func() {
-		existing.FeeCredit = existingFee
-	}()
-
-	return bytes.Equal(existing.Transaction.Hash().Bytes(), candidate.Hash().Bytes())
+	adjustedFee := existing.effectivePriorityFee.Mul64(100 + FeeBumpPercentage).Div64(100)
+	return candidate.effectivePriorityFee.Cmp(adjustedFee) >= 0
 }
 
 func (p *TxnPool) addLocked(txn *metaTxn) DiscardReason {
@@ -251,8 +245,6 @@ func (p *TxnPool) addLocked(txn *metaTxn) DiscardReason {
 		if !shouldReplace(found, txn) {
 			return NotReplaced
 		}
-
-		p.queue.Remove(found)
 		p.discardLocked(found, ReplacedByHigherTip)
 	}
 

nil/services/txnpool/txnpool_test.go

@@ -79,12 +79,14 @@ func (s *SuiteTxnPool) addTransactionsToPoolSuccessfully(pool Pool, txn ...*type
 	s.Equal(count+len(txn), s.getTransactionCount(pool))
 }
 
-func (s *SuiteTxnPool) addTransactions(txn ...*types.Transaction) {
+func (s *SuiteTxnPool) addTransactions(txn ...*types.Transaction) []DiscardReason {
 	s.T().Helper()
 
 	reasons, err := s.pool.Add(s.ctx, txn...)
 	s.Require().NoError(err)
 	s.Require().Len(reasons, len(txn))
+
+	return reasons
 }
 
 func (s *SuiteTxnPool) getTransactions() []*types.TxnWithHash {
@@ -130,21 +132,12 @@ func (s *SuiteTxnPool) TestAdd() {
 	// Send the same transaction with higher fee - OK
 	// Doesn't use the same helper because here transaction count doesn't change
 	txn2 := common.CopyPtr(txn1)
-	txn2.FeeCredit = txn2.FeeCredit.Add64(1)
+	txn2.MaxPriorityFeePerGas = txn2.MaxPriorityFeePerGas.Add64(100)
 	reasons, err := s.pool.Add(s.ctx, txn2)
 	s.Require().NoError(err)
 	s.Require().Equal([]DiscardReason{NotSet}, reasons)
 	s.Equal(1, s.getTransactionCount(s.pool))
 
-	// Send a different transaction with the same seqno - NotReplaced
-	txn3 := common.CopyPtr(txn1)
-	// Force the transaction to be different
-	txn3.Data = append(txn3.Data, 0x01)
-	s.Require().NotEqual(txn1.Hash(), txn3.Hash())
-	// Add a higher fee (otherwise, no replacement can be expected anyway)
-	txn3.FeeCredit = txn3.FeeCredit.Add64(1)
-	s.addTransactionWithDiscardReason(txn3, NotReplaced)
-
 	// Add a transaction with higher seqno to the same receiver
 	s.addTransactionsSuccessfully(newTransaction(defaultAddress, 1, 124))
 
@@ -294,6 +287,32 @@ func (s *SuiteTxnPool) TestBaseFeeChanged() {
 	s.checkTransactionsOrder()
 }
 
+func (s *SuiteTxnPool) TestReplacement() {
+	err := s.pool.OnCommitted(s.ctx, types.NewValueFromUint64(1000), nil)
+	s.Require().NoError(err)
+
+	txn1 := newTransaction2(defaultAddress, 0, 100, 1100, 0)
+	s.addTransactions(txn1)
+	s.checkTransactionsOrder(0)
+
+	// Not replaced: new priorityFee is less than FeeBumpPercentage
+	txn1 = newTransaction2(defaultAddress, 0, 100+FeeBumpPercentage-1, 1200, 1)
+	reasons := s.addTransactions(txn1)
+	s.checkTransactionsOrder(0)
+	s.Require().Equal([]DiscardReason{NotReplaced}, reasons)
+
+	// Replaced: new priorityFee is equal to FeeBumpPercentage
+	txn1 = newTransaction2(defaultAddress, 0, 100+FeeBumpPercentage, 1200, 2)
+	s.addTransactions(txn1)
+	s.checkTransactionsOrder(2)
+
+	// Not replaced: maxFeePerGas is small
+	txn1 = newTransaction2(defaultAddress, 0, 150, 1100, 3)
+	reasons = s.addTransactions(txn1)
+	s.checkTransactionsOrder(2)
+	s.Require().Equal([]DiscardReason{NotReplaced}, reasons)
+}
+
 func (s *SuiteTxnPool) TestNetwork() {
 	nms := network.NewTestManagers(s.T(), s.ctx, 9100, 2)