Merge pull request #4831 from ethereum-optimism/jg/txmgr_cleanup_pt3

txmgr: Don't submit underpriced replacements

op-proposer/proposer/l2_output_submitter.go

@@ -361,6 +361,7 @@ func (l *L2OutputSubmitter) SendTransaction(ctx context.Context, tx *types.Trans
 	// receipt is received it's likely our gas price was too low.
 	cCtx, cancel := context.WithTimeout(ctx, 100*time.Second)
 	defer cancel()
+	l.log.Info("Sending transaction", "tx_hash", tx.Hash())
 	receipt, err := l.txMgr.Send(cCtx, tx)
 	if err != nil {
 		l.log.Error("proposer unable to publish tx", "err", err)

op-service/txmgr/txmgr.go

@@ -15,6 +15,14 @@ import (
 	opcrypto "github.com/ethereum-optimism/optimism/op-service/crypto"
 )
 
+// Geth defaults the priceBump to 10
+// Set it to 15% to be more aggressive about including transactions
+const priceBump int64 = 15
+
+// new = old * (100 + priceBump) / 100
+var priceBumpPercent = big.NewInt(100 + priceBump)
+var oneHundred = big.NewInt(100)
+
 // UpdateGasPriceSendTxFunc defines a function signature for publishing a
 // desired tx with a specific gas price. Implementations of this signature
 // should also return promptly when the context is canceled.
@@ -94,8 +102,10 @@ type SimpleTxManager struct {
 }
 
 // IncreaseGasPrice takes the previous transaction & potentially clones then signs it with a higher tip.
-// If the basefee + priority fee did not increase by a minimum percent (geth's replacement percent) an
-// error will be returned.
+// If the tip + basefee suggested by the network are not greater than the previous values, the same transaction
+// will be returned. If they are greater, this function will ensure that they are at least greater by 15% than
+// the previous transaction's value to ensure that the price bump is large enough.
+//
 // We do not re-estimate the amount of gas used because for some stateful transactions (like output proposals) the
 // act of including the transaction renders the repeat of the transaction invalid.
 func (m *SimpleTxManager) IncreaseGasPrice(ctx context.Context, tx *types.Transaction) (*types.Transaction, error) {
@@ -112,15 +122,38 @@ func (m *SimpleTxManager) IncreaseGasPrice(ctx context.Context, tx *types.Transa
 		gasTipCap = tip
 	}
 
+	// new = old * (100 + priceBump) / 100
+	// Enforce a min priceBump on the tip. Do this before the feeCap is calculated
+	thresholdTip := new(big.Int).Mul(priceBumpPercent, tx.GasTipCap())
+	thresholdTip = thresholdTip.Div(thresholdTip, oneHundred)
+	if tx.GasTipCapIntCmp(gasTipCap) >= 0 {
+		m.l.Debug("Reusing the previous tip", "previous", tx.GasTipCap(), "suggested", gasTipCap)
+		gasTipCap = tx.GasTipCap()
+	} else if thresholdTip.Cmp(gasTipCap) > 0 {
+		m.l.Debug("Overriding the tip to enforce a price bump", "previous", tx.GasTipCap(), "suggested", gasTipCap, "new", thresholdTip)
+		gasTipCap = thresholdTip
+	}
+
 	if head, err := m.backend.HeaderByNumber(ctx, nil); err != nil {
 		return nil, err
 	} else if head.BaseFee == nil {
 		return nil, errors.New("txmgr does not support pre-london blocks that do not have a basefee")
 	} else {
+		// CalcGasFeeCap ensure that the fee cap is large enough for the tip.
 		gasFeeCap = CalcGasFeeCap(head.BaseFee, gasTipCap)
 	}
 
-	// TODO (CLI-2630): Check for a large enough price bump
+	// new = old * (100 + priceBump) / 100
+	// Enforce a min priceBump on the feeCap
+	thresholdFeeCap := new(big.Int).Mul(priceBumpPercent, tx.GasFeeCap())
+	thresholdFeeCap = thresholdFeeCap.Div(thresholdFeeCap, oneHundred)
+	if tx.GasFeeCapIntCmp(gasFeeCap) >= 0 {
+		m.l.Debug("Reusing the previous fee cap", "previous", tx.GasFeeCap(), "suggested", gasFeeCap)
+		gasFeeCap = tx.GasFeeCap()
+	} else if thresholdFeeCap.Cmp(gasFeeCap) > 0 {
+		m.l.Debug("Overriding the fee cap to enforce a price bump", "previous", tx.GasFeeCap(), "suggested", gasFeeCap, "new", thresholdFeeCap)
+		gasFeeCap = thresholdFeeCap
+	}
 
 	rawTx := &types.DynamicFeeTx{
 		ChainID:    tx.ChainId(),

op-service/txmgr/txmgr_test.go

@@ -11,7 +11,6 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/ethereum-optimism/optimism/op-node/testlog"
-	"github.com/ethereum/go-ethereum"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core"
 	"github.com/ethereum/go-ethereum/core/types"
@@ -290,6 +289,7 @@ func TestTxMgrConfirmsAtHigherGasPrice(t *testing.T) {
 
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()
+
 	receipt, err := h.mgr.Send(ctx, tx)
 	require.Nil(t, err)
 	require.NotNil(t, receipt)
@@ -529,6 +529,7 @@ func TestManagerPanicOnZeroConfs(t *testing.T) {
 type failingBackend struct {
 	returnSuccessBlockNumber bool
 	returnSuccessReceipt     bool
+	baseFee, gasTip          *big.Int
 }
 
 // BlockNumber for the failingBackend returns errRpcFailure on the first
@@ -559,7 +560,9 @@ func (b *failingBackend) TransactionReceipt(
 }
 
 func (b *failingBackend) HeaderByNumber(_ context.Context, _ *big.Int) (*types.Header, error) {
-	return nil, ethereum.NotFound
+	return &types.Header{
+		BaseFee: b.baseFee,
+	}, nil
 }
 
 func (b *failingBackend) SendTransaction(_ context.Context, _ *types.Transaction) error {
@@ -567,7 +570,7 @@ func (b *failingBackend) SendTransaction(_ context.Context, _ *types.Transaction
 }
 
 func (b *failingBackend) SuggestGasTipCap(_ context.Context) (*big.Int, error) {
-	return nil, errors.New("unimplemented")
+	return b.gasTip, nil
 }
 
 // TestWaitMinedReturnsReceiptAfterFailure asserts that WaitMined is able to
@@ -602,3 +605,125 @@ func TestWaitMinedReturnsReceiptAfterFailure(t *testing.T) {
 	require.NotNil(t, receipt)
 	require.Equal(t, receipt.TxHash, txHash)
 }
+
+// TestIncreaseGasPriceEnforcesMinBump asserts that if the suggest gas tip
+// returned from L1 is less than the required price bump the price bump is
+// used instead.
+func TestIncreaseGasPriceEnforcesMinBump(t *testing.T) {
+	t.Parallel()
+
+	borkedBackend := failingBackend{
+		gasTip:  big.NewInt(101),
+		baseFee: big.NewInt(460),
+	}
+
+	mgr := &SimpleTxManager{
+		Config: Config{
+			ResubmissionTimeout:       time.Second,
+			ReceiptQueryInterval:      50 * time.Millisecond,
+			NumConfirmations:          1,
+			SafeAbortNonceTooLowCount: 3,
+			Signer: func(ctx context.Context, from common.Address, tx *types.Transaction) (*types.Transaction, error) {
+				return tx, nil
+			},
+			From: common.Address{},
+		},
+		name:    "TEST",
+		backend: &borkedBackend,
+		l:       testlog.Logger(t, log.LvlCrit),
+	}
+
+	tx := types.NewTx(&types.DynamicFeeTx{
+		GasTipCap: big.NewInt(100),
+		GasFeeCap: big.NewInt(1000),
+	})
+
+	ctx := context.Background()
+	newTx, err := mgr.IncreaseGasPrice(ctx, tx)
+	require.NoError(t, err)
+	require.True(t, newTx.GasFeeCap().Cmp(tx.GasFeeCap()) > 0, "new tx fee cap must be larger")
+	require.True(t, newTx.GasTipCap().Cmp(tx.GasTipCap()) > 0, "new tx tip must be larger")
+}
+
+// TestIncreaseGasPriceNotExponential asserts that if the L1 basefee & tip remain the
+// same, repeated calls to IncreaseGasPrice do not continually increase the gas price.
+func TestIncreaseGasPriceNotExponential(t *testing.T) {
+	t.Parallel()
+
+	borkedBackend := failingBackend{
+		gasTip:  big.NewInt(10),
+		baseFee: big.NewInt(45),
+	}
+	feeCap := CalcGasFeeCap(borkedBackend.baseFee, borkedBackend.gasTip)
+
+	mgr := &SimpleTxManager{
+		Config: Config{
+			ResubmissionTimeout:       time.Second,
+			ReceiptQueryInterval:      50 * time.Millisecond,
+			NumConfirmations:          1,
+			SafeAbortNonceTooLowCount: 3,
+			Signer: func(ctx context.Context, from common.Address, tx *types.Transaction) (*types.Transaction, error) {
+				return tx, nil
+			},
+			From: common.Address{},
+		},
+		name:    "TEST",
+		backend: &borkedBackend,
+		l:       testlog.Logger(t, log.LvlCrit),
+	}
+	tx := types.NewTx(&types.DynamicFeeTx{
+		GasTipCap: big.NewInt(10),
+		GasFeeCap: big.NewInt(100),
+	})
+
+	// Run IncreaseGasPrice a bunch of times in a row to simulate a very fast resubmit loop.
+	for i := 0; i < 20; i++ {
+		ctx := context.Background()
+		newTx, err := mgr.IncreaseGasPrice(ctx, tx)
+		require.NoError(t, err)
+		require.True(t, newTx.GasFeeCap().Cmp(feeCap) == 0, "new tx fee cap must be equal L1")
+		require.True(t, newTx.GasTipCap().Cmp(borkedBackend.gasTip) == 0, "new tx tip must be equal L1")
+		tx = newTx
+	}
+
+}
+
+// TestIncreaseGasPriceUseLargeIncrease asserts that if the suggest gas tip
+// returned from L1 is much larger than the required price bump the L1 value
+// is used instead of the price bump
+func TestIncreaseGasPriceUseLargeIncrease(t *testing.T) {
+	t.Parallel()
+
+	borkedBackend := failingBackend{
+		gasTip:  big.NewInt(50),
+		baseFee: big.NewInt(200),
+	}
+	feeCap := CalcGasFeeCap(borkedBackend.baseFee, borkedBackend.gasTip)
+
+	mgr := &SimpleTxManager{
+		Config: Config{
+			ResubmissionTimeout:       time.Second,
+			ReceiptQueryInterval:      50 * time.Millisecond,
+			NumConfirmations:          1,
+			SafeAbortNonceTooLowCount: 3,
+			Signer: func(ctx context.Context, from common.Address, tx *types.Transaction) (*types.Transaction, error) {
+				return tx, nil
+			},
+			From: common.Address{},
+		},
+		name:    "TEST",
+		backend: &borkedBackend,
+		l:       testlog.Logger(t, log.LvlCrit),
+	}
+
+	tx := types.NewTx(&types.DynamicFeeTx{
+		GasTipCap: big.NewInt(10),
+		GasFeeCap: big.NewInt(100),
+	})
+
+	ctx := context.Background()
+	newTx, err := mgr.IncreaseGasPrice(ctx, tx)
+	require.NoError(t, err)
+	require.True(t, newTx.GasFeeCap().Cmp(feeCap) == 0, "new tx fee cap must be equal L1")
+	require.True(t, newTx.GasTipCap().Cmp(borkedBackend.gasTip) == 0, "new tx tip must be equal L1")
+}