Commit 33da2e1f authored by Joshua Gutow's avatar Joshua Gutow

txmgr: Don't submit underpriced replacements

This prevents the transaction manager from resubmiting transactions that do
not have a price increase that is large enough to pass geth's checks. The
other option is to increase the gas price bump, but because the gas price
has not changed much in the intervening period, it seems better to leave
the currently existing transaction in the mempool.
parent 3969dc49
...@@ -361,6 +361,7 @@ func (l *L2OutputSubmitter) SendTransaction(ctx context.Context, tx *types.Trans ...@@ -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. // receipt is received it's likely our gas price was too low.
cCtx, cancel := context.WithTimeout(ctx, 100*time.Second) cCtx, cancel := context.WithTimeout(ctx, 100*time.Second)
defer cancel() defer cancel()
l.log.Info("Sending transaction", "tx_hash", tx.Hash())
receipt, err := l.txMgr.Send(cCtx, tx) receipt, err := l.txMgr.Send(cCtx, tx)
if err != nil { if err != nil {
l.log.Error("proposer unable to publish tx", "err", err) l.log.Error("proposer unable to publish tx", "err", err)
......
...@@ -15,6 +15,14 @@ import ( ...@@ -15,6 +15,14 @@ import (
opcrypto "github.com/ethereum-optimism/optimism/op-service/crypto" 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 // UpdateGasPriceSendTxFunc defines a function signature for publishing a
// desired tx with a specific gas price. Implementations of this signature // desired tx with a specific gas price. Implementations of this signature
// should also return promptly when the context is canceled. // should also return promptly when the context is canceled.
...@@ -112,15 +120,32 @@ func (m *SimpleTxManager) IncreaseGasPrice(ctx context.Context, tx *types.Transa ...@@ -112,15 +120,32 @@ func (m *SimpleTxManager) IncreaseGasPrice(ctx context.Context, tx *types.Transa
gasTipCap = tip 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 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 { if head, err := m.backend.HeaderByNumber(ctx, nil); err != nil {
return nil, err return nil, err
} else if head.BaseFee == nil { } else if head.BaseFee == nil {
return nil, errors.New("txmgr does not support pre-london blocks that do not have a basefee") return nil, errors.New("txmgr does not support pre-london blocks that do not have a basefee")
} else { } else {
// CalcGasFeeCap ensure that the fee cap is large enough for the tip.
gasFeeCap = CalcGasFeeCap(head.BaseFee, gasTipCap) 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 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{ rawTx := &types.DynamicFeeTx{
ChainID: tx.ChainId(), ChainID: tx.ChainId(),
......
...@@ -11,7 +11,6 @@ import ( ...@@ -11,7 +11,6 @@ import (
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
"github.com/ethereum-optimism/optimism/op-node/testlog" "github.com/ethereum-optimism/optimism/op-node/testlog"
"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core" "github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/core/types"
...@@ -290,6 +289,7 @@ func TestTxMgrConfirmsAtHigherGasPrice(t *testing.T) { ...@@ -290,6 +289,7 @@ func TestTxMgrConfirmsAtHigherGasPrice(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel() defer cancel()
receipt, err := h.mgr.Send(ctx, tx) receipt, err := h.mgr.Send(ctx, tx)
require.Nil(t, err) require.Nil(t, err)
require.NotNil(t, receipt) require.NotNil(t, receipt)
...@@ -529,6 +529,7 @@ func TestManagerPanicOnZeroConfs(t *testing.T) { ...@@ -529,6 +529,7 @@ func TestManagerPanicOnZeroConfs(t *testing.T) {
type failingBackend struct { type failingBackend struct {
returnSuccessBlockNumber bool returnSuccessBlockNumber bool
returnSuccessReceipt bool returnSuccessReceipt bool
baseFee, gasTip *big.Int
} }
// BlockNumber for the failingBackend returns errRpcFailure on the first // BlockNumber for the failingBackend returns errRpcFailure on the first
...@@ -559,7 +560,9 @@ func (b *failingBackend) TransactionReceipt( ...@@ -559,7 +560,9 @@ func (b *failingBackend) TransactionReceipt(
} }
func (b *failingBackend) HeaderByNumber(_ context.Context, _ *big.Int) (*types.Header, error) { 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 { func (b *failingBackend) SendTransaction(_ context.Context, _ *types.Transaction) error {
...@@ -567,7 +570,7 @@ func (b *failingBackend) SendTransaction(_ context.Context, _ *types.Transaction ...@@ -567,7 +570,7 @@ func (b *failingBackend) SendTransaction(_ context.Context, _ *types.Transaction
} }
func (b *failingBackend) SuggestGasTipCap(_ context.Context) (*big.Int, error) { 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 // TestWaitMinedReturnsReceiptAfterFailure asserts that WaitMined is able to
...@@ -602,3 +605,82 @@ func TestWaitMinedReturnsReceiptAfterFailure(t *testing.T) { ...@@ -602,3 +605,82 @@ func TestWaitMinedReturnsReceiptAfterFailure(t *testing.T) {
require.NotNil(t, receipt) require.NotNil(t, receipt)
require.Equal(t, receipt.TxHash, txHash) 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: common.Big0,
baseFee: common.Big0,
}
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(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")
}
// 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")
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment