re-estimate gas on tx retry in txmgr

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: 115, expectedFC: 2415,
+			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

@@ -21,9 +21,8 @@ import (
 )
 
 const (
-	// Geth defaults the priceBump to 10
-	// Set it to 15% to be more aggressive about including transactions
-	priceBump int64 = 15
+	// 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
@@ -285,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)
@@ -424,34 +431,28 @@ func (m *SimpleTxManager) queryReceipt(ctx context.Context, txHash common.Hash,
 }
 
 // increaseGasPrice takes the previous transaction, clones it, and returns it with fee values that
-// are at least 15% 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 5x suggested values.
-//
-// 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 {
+// 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())
-	var tip, basefee *big.Int
 	tip, basefee, err := m.suggestGasPriceCaps(ctx)
 	if err != nil {
 		m.l.Warn("failed to get suggested gas tip and basefee", "err", err)
-		// In this case we'll use the old tx's tip as the tip suggestion and 0 for base fee so that
-		// we at least get the min geth fee bump.
-		tip = new(big.Int).Set(tx.GasTipCap())
-		basefee = new(big.Int)
+		return nil, err
 	}
 	bumpedTip, bumpedFee := updateFees(tx.GasTipCap(), tx.GasFeeCap(), tip, basefee, m.l)
 
 	// 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("bumped tip getting capped at 5x the suggested value", "bumped", bumpedTip, "suggestion", tip)
+		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 5x the implied suggested value", "bumped", bumpedFee, "suggestion", maxFee)
+		m.l.Warn("bumped fee getting capped at multiple of the implied suggested value", "bumped", bumpedFee, "suggestion", maxFee)
 		bumpedFee.Set(maxFee)
 	}
 	rawTx := &types.DynamicFeeTx{
@@ -459,20 +460,37 @@ func (m *SimpleTxManager) increaseGasPrice(ctx context.Context, tx *types.Transa
 		Nonce:      tx.Nonce(),
 		GasTipCap:  bumpedTip,
 		GasFeeCap:  bumpedFee,
-		Gas:        tx.Gas(),
 		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
@@ -508,7 +526,7 @@ func calcThresholdValue(x *big.Int) *big.Int {
 // 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 15% increase), and
+//	(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) {

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,15 +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")
+				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")
 			},
 		},
 	}
@@ -804,9 +807,11 @@ 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),
 	}
 
 	mgr := &SimpleTxManager{
@@ -831,17 +836,20 @@ 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()
-		tx = mgr.increaseGasPrice(ctx, tx)
+		tx, err = mgr.increaseGasPrice(ctx, tx)
+		require.NoError(t, err)
 	}
 	lastTip, lastFee := tx.GasTipCap(), tx.GasFeeCap()
-	require.Equal(t, lastTip.Int64(), int64(50))  // 5x borked tip
-	require.Equal(t, lastFee.Int64(), int64(500)) // 5x borked tip + 2*(5x borked base fee)
+	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 = mgr.increaseGasPrice(ctx, tx)
+		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")
 	}