From 02c4e20c74f8a3046263970200c2320d55b92376 Mon Sep 17 00:00:00 2001 From: Roberto Bayardo Date: Thu, 15 Jun 2023 13:28:00 -0700 Subject: [PATCH] require transaction fees increase regardless of fee suggestion to avoid tx replacement issues --- op-service/txmgr/price_bump_test.go | 2 +- op-service/txmgr/txmgr.go | 66 +++++++++++++++++------------ op-service/txmgr/txmgr_test.go | 24 +++++------ 3 files changed, 51 insertions(+), 41 deletions(-) diff --git a/op-service/txmgr/price_bump_test.go b/op-service/txmgr/price_bump_test.go index 702b02032f70..0e0378d8db5a 100644 --- a/op-service/txmgr/price_bump_test.go +++ b/op-service/txmgr/price_bump_test.go @@ -34,7 +34,7 @@ func TestUpdateFees(t *testing.T) { { prevGasTip: 100, prevBasefee: 1000, newGasTip: 90, newBasefee: 900, - expectedTip: 100, expectedFC: 2100, + expectedTip: 115, expectedFC: 2415, }, { prevGasTip: 100, prevBasefee: 1000, diff --git a/op-service/txmgr/txmgr.go b/op-service/txmgr/txmgr.go index cc18d7fad28c..5f3447273699 100644 --- a/op-service/txmgr/txmgr.go +++ b/op-service/txmgr/txmgr.go @@ -20,9 +20,14 @@ 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 defaults the priceBump to 10 + // Set it to 15% to be more aggressive about including transactions + priceBump int64 = 15 + + // 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) @@ -418,32 +423,42 @@ 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. +// 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. // -// If it encounters an error with creating the new transaction, it will return the old transaction. +// 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 { + 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) - return tx + // 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) } - 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("bumped tip getting capped at 5x the suggested value", "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) + bumpedFee.Set(maxFee) } - rawTx := &types.DynamicFeeTx{ ChainID: tx.ChainId(), Nonce: tx.Nonce(), - GasTipCap: gasTipCap, - GasFeeCap: gasFeeCap, + GasTipCap: bumpedTip, + GasFeeCap: bumpedFee, Gas: tx.Gas(), To: tx.To(), Value: tx.Value(), @@ -490,18 +505,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 15% 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 { diff --git a/op-service/txmgr/txmgr_test.go b/op-service/txmgr/txmgr_test.go index 5018868fbdd9..2f5f9cabc3d4 100644 --- a/op-service/txmgr/txmgr_test.go +++ b/op-service/txmgr/txmgr_test.go @@ -792,13 +792,6 @@ func TestIncreaseGasPrice(t *testing.T) { 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") - }, - }, } for _, test := range tests { test := test @@ -815,7 +808,6 @@ func TestIncreaseGasPriceNotExponential(t *testing.T) { gasTip: big.NewInt(10), baseFee: big.NewInt(45), } - feeCap := calcGasFeeCap(borkedBackend.baseFee, borkedBackend.gasTip) mgr := &SimpleTxManager{ cfg: Config{ @@ -841,12 +833,18 @@ 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++ { 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 = mgr.increaseGasPrice(ctx, tx) + } + 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) + // Confirm that fees stop rising + for i := 0; i < 5; i++ { + ctx := context.Background() + tx = mgr.increaseGasPrice(ctx, tx) + 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) {