Merge pull request #6031 from roberto-bayardo/fix-tx-replacement

require fee increase & gas re-estimation for retried transactions

op-service/txmgr/price_bump_test.go

@@ -30,51 +30,52 @@ func (tc *priceBumpTest) run(t *testing.T) {
 }
 
 func TestUpdateFees(t *testing.T) {
+	require.Equal(t, int64(10), priceBump, "test must be updated if priceBump is adjusted")
 	tests := []priceBumpTest{
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 90, newBasefee: 900,
-			expectedTip: 100, expectedFC: 2100,
+			expectedTip: 110, expectedFC: 2310,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 101, newBasefee: 1000,
-			expectedTip: 115, expectedFC: 2415,
+			expectedTip: 110, expectedFC: 2310,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 100, newBasefee: 1001,
-			expectedTip: 115, expectedFC: 2415,
+			expectedTip: 110, expectedFC: 2310,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 101, newBasefee: 900,
-			expectedTip: 115, expectedFC: 2415,
+			expectedTip: 110, expectedFC: 2310,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 90, newBasefee: 1010,
-			expectedTip: 115, expectedFC: 2415,
+			expectedTip: 110, expectedFC: 2310,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 101, newBasefee: 2000,
-			expectedTip: 115, expectedFC: 4115,
+			expectedTip: 110, expectedFC: 4110,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 120, newBasefee: 900,
-			expectedTip: 120, expectedFC: 2415,
+			expectedTip: 120, expectedFC: 2310,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 120, newBasefee: 1100,
-			expectedTip: 120, expectedFC: 2415,
+			expectedTip: 120, expectedFC: 2320,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,
 			newGasTip: 120, newBasefee: 1140,
-			expectedTip: 120, expectedFC: 2415,
+			expectedTip: 120, expectedFC: 2400,
 		},
 		{
 			prevGasTip: 100, prevBasefee: 1000,

op-service/txmgr/txmgr.go

@@ -20,9 +20,13 @@ import (
 	"github.com/ethereum-optimism/optimism/op-service/txmgr/metrics"
 )
 
-// Geth defaults the priceBump to 10
-// Set it to 15% to be more aggressive about including transactions
-const priceBump int64 = 15
+const (
+	// Geth requires a minimum fee bump of 10% for tx resubmission
+	priceBump int64 = 10
+
+	// The multiplier applied to fee suggestions to put a hard limit on fee increases
+	feeLimitMultiplier = 5
+)
 
 // new = old * (100 + priceBump) / 100
 var priceBumpPercent = big.NewInt(100 + priceBump)
@@ -280,7 +284,15 @@ func (m *SimpleTxManager) sendTx(ctx context.Context, tx *types.Transaction) (*t
 				return nil, errors.New("aborted transaction sending")
 			}
 			// Increase the gas price & submit the new transaction
-			tx = m.increaseGasPrice(ctx, tx)
+			newTx, err := m.increaseGasPrice(ctx, tx)
+			if err != nil || sendState.IsWaitingForConfirmation() {
+				// there is a chance the previous tx goes into "waiting for confirmation" state
+				// during the increaseGasPrice call. In some (but not all) cases increaseGasPrice
+				// will error out during gas estimation. In either case we should continue waiting
+				// rather than resubmit the tx.
+				continue
+			}
+			tx = newTx
 			wg.Add(1)
 			bumpCounter += 1
 			go sendTxAsync(tx)
@@ -418,46 +430,67 @@ func (m *SimpleTxManager) queryReceipt(ctx context.Context, txHash common.Hash,
 	return nil
 }
 
-// increaseGasPrice takes the previous transaction & potentially clones then signs it with a higher tip.
-// 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.
-//
-// If it encounters an error with creating the new transaction, it will return the old transaction.
-func (m *SimpleTxManager) increaseGasPrice(ctx context.Context, tx *types.Transaction) *types.Transaction {
+// increaseGasPrice takes the previous transaction, clones it, and returns it with fee values that
+// are at least `priceBump` percent higher than the previous ones to satisfy Geth's replacement
+// rules, and no lower than the values returned by the fee suggestion algorithm to ensure it
+// doesn't linger in the mempool. Finally to avoid runaway price increases, fees are capped at a
+// `feeLimitMultiplier` multiple of the suggested values.
+func (m *SimpleTxManager) increaseGasPrice(ctx context.Context, tx *types.Transaction) (*types.Transaction, error) {
+	m.l.Info("bumping gas price for tx", "hash", tx.Hash(), "tip", tx.GasTipCap(), "fee", tx.GasFeeCap(), "gaslimit", tx.Gas())
 	tip, basefee, err := m.suggestGasPriceCaps(ctx)
 	if err != nil {
 		m.l.Warn("failed to get suggested gas tip and basefee", "err", err)
-		return tx
+		return nil, err
 	}
-	gasTipCap, gasFeeCap := updateFees(tx.GasTipCap(), tx.GasFeeCap(), tip, basefee, m.l)
+	bumpedTip, bumpedFee := updateFees(tx.GasTipCap(), tx.GasFeeCap(), tip, basefee, m.l)
 
-	if tx.GasTipCapIntCmp(gasTipCap) == 0 && tx.GasFeeCapIntCmp(gasFeeCap) == 0 {
-		return tx
+	// Make sure increase is at most 5x the suggested values
+	maxTip := new(big.Int).Mul(tip, big.NewInt(feeLimitMultiplier))
+	if bumpedTip.Cmp(maxTip) > 0 {
+		m.l.Warn(fmt.Sprintf("bumped tip getting capped at %dx multiple of the suggested value", feeLimitMultiplier), "bumped", bumpedTip, "suggestion", tip)
+		bumpedTip.Set(maxTip)
+	}
+	maxFee := calcGasFeeCap(new(big.Int).Mul(basefee, big.NewInt(feeLimitMultiplier)), maxTip)
+	if bumpedFee.Cmp(maxFee) > 0 {
+		m.l.Warn("bumped fee getting capped at multiple of the implied suggested value", "bumped", bumpedFee, "suggestion", maxFee)
+		bumpedFee.Set(maxFee)
 	}
-
 	rawTx := &types.DynamicFeeTx{
 		ChainID:    tx.ChainId(),
 		Nonce:      tx.Nonce(),
-		GasTipCap:  gasTipCap,
-		GasFeeCap:  gasFeeCap,
-		Gas:        tx.Gas(),
+		GasTipCap:  bumpedTip,
+		GasFeeCap:  bumpedFee,
 		To:         tx.To(),
 		Value:      tx.Value(),
 		Data:       tx.Data(),
 		AccessList: tx.AccessList(),
 	}
+
+	// Re-estimate gaslimit in case things have changed or a previous gaslimit estimate was wrong
+	gas, err := m.backend.EstimateGas(ctx, ethereum.CallMsg{
+		From:      m.cfg.From,
+		To:        rawTx.To,
+		GasFeeCap: bumpedTip,
+		GasTipCap: bumpedFee,
+		Data:      rawTx.Data,
+	})
+	if err != nil {
+		m.l.Warn("failed to re-estimate gas", "err", err, "gaslimit", tx.Gas())
+		return nil, err
+	}
+	if tx.Gas() != gas {
+		m.l.Info("re-estimated gas differs", "oldgas", tx.Gas(), "newgas", gas)
+	}
+	rawTx.Gas = gas
+
 	ctx, cancel := context.WithTimeout(ctx, m.cfg.NetworkTimeout)
 	defer cancel()
 	newTx, err := m.cfg.Signer(ctx, m.cfg.From, types.NewTx(rawTx))
 	if err != nil {
 		m.l.Warn("failed to sign new transaction", "err", err)
-		return tx
+		return tx, nil
 	}
-	return newTx
+	return newTx, nil
 }
 
 // suggestGasPriceCaps suggests what the new tip & new basefee should be based on the current L1 conditions
@@ -490,18 +523,15 @@ func calcThresholdValue(x *big.Int) *big.Int {
 	return threshold
 }
 
-// updateFees takes the old tip/basefee & the new tip/basefee and then suggests
-// a gasTipCap and gasFeeCap that satisfies geth's required fee bumps
-// Geth: FC and Tip must be bumped if any increase
+// updateFees takes an old transaction's tip & fee cap plus a new tip & basefee, and returns
+// a suggested tip and fee cap such that:
+//
+//	(a) each satisfies geth's required tx-replacement fee bumps (we use a 10% increase), and
+//	(b) gasTipCap is no less than new tip, and
+//	(c) gasFeeCap is no less than calcGasFee(newBaseFee, newTip)
 func updateFees(oldTip, oldFeeCap, newTip, newBaseFee *big.Int, lgr log.Logger) (*big.Int, *big.Int) {
 	newFeeCap := calcGasFeeCap(newBaseFee, newTip)
 	lgr = lgr.New("old_tip", oldTip, "old_feecap", oldFeeCap, "new_tip", newTip, "new_feecap", newFeeCap)
-	// If the new prices are less than the old price, reuse the old prices
-	if oldTip.Cmp(newTip) >= 0 && oldFeeCap.Cmp(newFeeCap) >= 0 {
-		lgr.Debug("Reusing old tip and feecap")
-		return oldTip, oldFeeCap
-	}
-	// Determine if we need to increase the suggested values
 	thresholdTip := calcThresholdValue(oldTip)
 	thresholdFeeCap := calcThresholdValue(oldFeeCap)
 	if newTip.Cmp(thresholdTip) >= 0 && newFeeCap.Cmp(thresholdFeeCap) >= 0 {

op-service/txmgr/txmgr_test.go

@@ -734,12 +734,14 @@ func doGasPriceIncrease(t *testing.T, txTipCap, txFeeCap, newTip, newBaseFee int
 		GasTipCap: big.NewInt(txTipCap),
 		GasFeeCap: big.NewInt(txFeeCap),
 	})
-	newTx := mgr.increaseGasPrice(context.Background(), tx)
+	newTx, err := mgr.increaseGasPrice(context.Background(), tx)
+	require.NoError(t, err)
 	return tx, newTx
 }
 
 func TestIncreaseGasPrice(t *testing.T) {
 	// t.Parallel()
+	require.Equal(t, int64(10), priceBump, "test must be updated if priceBump is adjusted")
 	tests := []struct {
 		name string
 		run  func(t *testing.T)
@@ -781,22 +783,16 @@ func TestIncreaseGasPrice(t *testing.T) {
 			run: func(t *testing.T) {
 				_, newTx := doGasPriceIncrease(t, 100, 2200, 120, 1050)
 				require.True(t, newTx.GasTipCap().Cmp(big.NewInt(120)) == 0, "new tx tip must be equal L1")
-				require.True(t, newTx.GasFeeCap().Cmp(big.NewInt(2530)) == 0, "new tx fee cap must be equal to the threshold value")
+				require.True(t, newTx.GasFeeCap().Cmp(big.NewInt(2420)) == 0, "new tx fee cap must be equal to the threshold value")
 			},
 		},
 		{
 			name: "uses L1 FC when larger and threshold tip",
 			run: func(t *testing.T) {
 				_, newTx := doGasPriceIncrease(t, 100, 2200, 100, 2000)
-				require.True(t, newTx.GasTipCap().Cmp(big.NewInt(115)) == 0, "new tx tip must be equal the threshold value")
-				require.True(t, newTx.GasFeeCap().Cmp(big.NewInt(4115)) == 0, "new tx fee cap must be equal L1")
-			},
-		},
-		{
-			name: "reuses tx when no bump",
-			run: func(t *testing.T) {
-				tx, newTx := doGasPriceIncrease(t, 10, 100, 10, 45)
-				require.Equal(t, tx.Hash(), newTx.Hash(), "tx hash must be the same")
+				require.True(t, newTx.GasTipCap().Cmp(big.NewInt(110)) == 0, "new tx tip must be equal the threshold value")
+				t.Log("Vals:", newTx.GasFeeCap())
+				require.True(t, newTx.GasFeeCap().Cmp(big.NewInt(4110)) == 0, "new tx fee cap must be equal L1")
 			},
 		},
 	}
@@ -811,11 +807,12 @@ func TestIncreaseGasPrice(t *testing.T) {
 func TestIncreaseGasPriceNotExponential(t *testing.T) {
 	t.Parallel()
 
+	borkedTip := int64(10)
+	borkedFee := int64(45)
 	borkedBackend := failingBackend{
-		gasTip:  big.NewInt(10),
-		baseFee: big.NewInt(45),
+		gasTip:  big.NewInt(borkedTip),
+		baseFee: big.NewInt(borkedFee),
 	}
-	feeCap := calcGasFeeCap(borkedBackend.baseFee, borkedBackend.gasTip)
 
 	mgr := &SimpleTxManager{
 		cfg: Config{
@@ -839,14 +836,23 @@ func TestIncreaseGasPriceNotExponential(t *testing.T) {
 	})
 
 	// Run IncreaseGasPrice a bunch of times in a row to simulate a very fast resubmit loop.
-	for i := 0; i < 20; i++ {
+	var err error
+	for i := 0; i < 30; i++ {
 		ctx := context.Background()
-		newTx := mgr.increaseGasPrice(ctx, tx)
-		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
+		tx, err = mgr.increaseGasPrice(ctx, tx)
+		require.NoError(t, err)
+	}
+	lastTip, lastFee := tx.GasTipCap(), tx.GasFeeCap()
+	require.Equal(t, lastTip.Int64(), feeLimitMultiplier*borkedTip)
+	require.Equal(t, lastFee.Int64(), feeLimitMultiplier*(borkedTip+2*borkedFee))
+	// Confirm that fees stop rising
+	for i := 0; i < 5; i++ {
+		ctx := context.Background()
+		tx, err := mgr.increaseGasPrice(ctx, tx)
+		require.NoError(t, err)
+		require.True(t, tx.GasTipCap().Cmp(lastTip) == 0, "suggested tx tip must stop increasing")
+		require.True(t, tx.GasFeeCap().Cmp(lastFee) == 0, "suggested tx fee must stop increasing")
 	}
-
 }
 
 func TestErrStringMatch(t *testing.T) {