Skip to content

N
nebula
Commit ef1d2266 authored by Joshua Gutow's avatar Joshua Gutow
Browse files
Options

txmgr: Better price bump algorithm 

TODO: Can rework to table based tests instead of going through a full flow.
parent ef45bec2
Show whitespace changes
Inline Side-by-side
Showing with 222 additions and 256 deletions
op-service/txmgr/price_bump_test.go 0 → 100644
View file @ ef1d2266
package txmgr
import (
"fmt"
"math/big"
"testing"
"github.com/ethereum-optimism/optimism/op-node/testlog"
"github.com/ethereum/go-ethereum/log"
"github.com/stretchr/testify/require"
)
type priceBumpTest struct {
prevGasTip int64
prevBasefee int64
newGasTip int64
newBasefee int64
expectedTip int64
expectedFC int64
}
func (tc *priceBumpTest) run(t *testing.T) {
prevFC := CalcGasFeeCap(big.NewInt(tc.prevBasefee), big.NewInt(tc.prevGasTip))
lgr := testlog.Logger(t, log.LvlCrit)
tip, fc := updateFees(big.NewInt(tc.prevGasTip), prevFC, big.NewInt(tc.newGasTip), big.NewInt(tc.newBasefee), lgr)
require.Equal(t, tc.expectedTip, tip.Int64(), "tip must be as expected")
require.Equal(t, tc.expectedFC, fc.Int64(), "fee cap must be as expected")
}
func TestUpdateFees(t *testing.T) {
tests := []priceBumpTest{
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 90, newBasefee: 900,
expectedTip: 100, expectedFC: 2100,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 101, newBasefee: 1000,
expectedTip: 115, expectedFC: 2415,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 100, newBasefee: 1001,
expectedTip: 115, expectedFC: 2415,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 101, newBasefee: 900,
expectedTip: 115, expectedFC: 2415,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 90, newBasefee: 1010,
expectedTip: 115, expectedFC: 2415,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 101, newBasefee: 2000,
expectedTip: 115, expectedFC: 4115,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 120, newBasefee: 900,
expectedTip: 120, expectedFC: 2415,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 120, newBasefee: 1100,
expectedTip: 120, expectedFC: 2415,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 120, newBasefee: 1140,
expectedTip: 120, expectedFC: 2415,
},
{
prevGasTip: 100, prevBasefee: 1000,
newGasTip: 120, newBasefee: 1200,
expectedTip: 120, expectedFC: 2520,
},
}
for i, test := range tests {
i := i
test := test
t.Run(fmt.Sprint(i), test.run)
}
}
op-service/txmgr/txmgr.go
View file @ ef1d2266
......@@ -13,7 +13,6 @@ import (
"github.com/ethereum/go-ethereum/core/txpool"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/params"
opcrypto "github.com/ethereum-optimism/optimism/op-service/crypto"
)
......@@ -133,32 +132,67 @@ type TxCandidate struct {
From common.Address
}
// calcGasTipAndFeeCap queries L1 to determine what a suitable miner tip & basefee limit would be for timely inclusion
func (m *SimpleTxManager) calcGasTipAndFeeCap(ctx context.Context) (gasTipCap *big.Int, gasFeeCap *big.Int, err error) {
childCtx, cancel := context.WithTimeout(ctx, m.Config.NetworkTimeout)
gasTipCap, err = m.backend.SuggestGasTipCap(childCtx)
cancel()
if err != nil {
return nil, nil, fmt.Errorf("failed to get suggested gas tip cap: %w", err)
}
// calcThresholdValue returns x * priceBumpPercent / 100
func calcThresholdValue(x *big.Int) *big.Int {
threshold := new(big.Int).Mul(priceBumpPercent, x)
threshold = threshold.Div(threshold, oneHundred)
return threshold
}
if gasTipCap == nil {
m.l.Warn("unexpected unset gasTipCap, using default 2 gwei")
gasTipCap = new(big.Int).SetUint64(params.GWei * 2)
// 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
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 {
lgr.Debug("Using new tip and feecap")
return newTip, newFeeCap
} else if newTip.Cmp(thresholdTip) >= 0 && newFeeCap.Cmp(thresholdFeeCap) < 0 {
// Tip has gone up, but basefee is flat or down.
// TODO(CLI-3714): Do we need to recalculate the FC here?
lgr.Debug("Using new tip and threshold feecap")
return newTip, thresholdFeeCap
} else if newTip.Cmp(thresholdTip) < 0 && newFeeCap.Cmp(thresholdFeeCap) >= 0 {
// Basefee has gone up, but the tip hasn't. Recalculate the feecap because if the tip went up a lot
// not enough of the feecap may be dedicated to paying the basefee.
lgr.Debug("Using threshold tip and recalculated feecap")
return thresholdTip, CalcGasFeeCap(newBaseFee, thresholdTip)
} else {
// TODO(CLI-3713): Should we skip the bump in this case?
lgr.Debug("Using threshold tip and threshold feecap")
return thresholdTip, thresholdFeeCap
}
}
childCtx, cancel = context.WithTimeout(ctx, m.Config.NetworkTimeout)
head, err := m.backend.HeaderByNumber(childCtx, nil)
cancel()
if err != nil || head == nil {
return nil, nil, fmt.Errorf("failed to get L1 head block for fee cap: %w", err)
// suggestGasPriceCaps suggests what the new tip & new basefee should be based on the current L1 conditions
func (m *SimpleTxManager) suggestGasPriceCaps(ctx context.Context) (*big.Int, *big.Int, error) {
cCtx, cancel := context.WithTimeout(ctx, m.Config.NetworkTimeout)
defer cancel()
tip, err := m.backend.SuggestGasTipCap(cCtx)
if err != nil {
return nil, nil, fmt.Errorf("failed to fetch the suggested gas tip cap: %w", err)
} else if tip == nil {
return nil, nil, errors.New("the suggested tip was nil")
}
if head.BaseFee == nil {
return nil, nil, fmt.Errorf("failed to get L1 basefee in block %d for fee cap", head.Number)
cCtx, cancel = context.WithTimeout(ctx, m.Config.NetworkTimeout)
defer cancel()
head, err := m.backend.HeaderByNumber(cCtx, nil)
if err != nil {
return nil, nil, fmt.Errorf("failed to fetch the suggested basefee: %w", err)
} else if head.BaseFee == nil {
return nil, nil, errors.New("txmgr does not support pre-london blocks that do not have a basefee")
}
gasFeeCap = CalcGasFeeCap(head.BaseFee, gasTipCap)
return gasTipCap, gasFeeCap, nil
return tip, head.BaseFee, nil
}
// craftTx creates the signed transaction
......@@ -167,10 +201,11 @@ func (m *SimpleTxManager) calcGasTipAndFeeCap(ctx context.Context) (gasTipCap *b
// NOTE: If the [TxCandidate.GasLimit] is non-zero, it will be used as the transaction's gas.
// NOTE: Otherwise, the [SimpleTxManager] will query the specified backend for an estimate.
func (m *SimpleTxManager) craftTx(ctx context.Context, candidate TxCandidate) (*types.Transaction, error) {
gasTipCap, gasFeeCap, err := m.calcGasTipAndFeeCap(ctx)
gasTipCap, basefee, err := m.suggestGasPriceCaps(ctx)
if err != nil {
return nil, err
}
gasFeeCap := CalcGasFeeCap(basefee, gasTipCap)
// Fetch the sender's nonce from the latest known block (nil `blockNumber`)
childCtx, cancel := context.WithTimeout(ctx, m.Config.NetworkTimeout)
......@@ -222,71 +257,13 @@ func (m *SimpleTxManager) craftTx(ctx context.Context, candidate TxCandidate) (*
// 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) {
ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()
var gasTipCap, gasFeeCap *big.Int
if tip, err := m.backend.SuggestGasTipCap(ctx); err != nil {
return nil, err
} else if tip == nil {
return nil, errors.New("the suggested tip was nil")
} else {
gasTipCap = tip
}
// Return the same transaction if we don't update any fields.
// We do this because ethereum signatures are not deterministic and therefore the transaction hash will change
// when we re-sign the tx. We don't want to do that because we want to see ErrAlreadyKnown instead of ErrReplacementUnderpriced
var reusedTip, reusedFeeCap bool
// 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()
reusedTip = true
} 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 {
tip, basefee, err := m.suggestGasPriceCaps(ctx)
if 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)
}
// 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 {
if reusedTip {
m.l.Debug("Reusing the previous fee cap", "previous", tx.GasFeeCap(), "suggested", gasFeeCap)
gasFeeCap = tx.GasFeeCap()
reusedFeeCap = true
} else {
m.l.Debug("Overriding the fee cap to enforce a price bump because we increased the tip", "previous", tx.GasFeeCap(), "suggested", gasFeeCap, "new", thresholdFeeCap)
gasFeeCap = thresholdFeeCap
}
} else if thresholdFeeCap.Cmp(gasFeeCap) > 0 {
if reusedTip {
// TODO (CLI-3620): Increase the basefee then recompute the feecap
m.l.Warn("Overriding the fee cap to enforce a price bump without increasing the tip. Will likely result in ErrReplacementUnderpriced",
"previous", tx.GasFeeCap(), "suggested", gasFeeCap, "new", thresholdFeeCap)
} else {
m.l.Debug("Overriding the fee cap to enforce a price bump", "previous", tx.GasFeeCap(), "suggested", gasFeeCap, "new", thresholdFeeCap)
}
gasFeeCap = thresholdFeeCap
}
gasTipCap, gasFeeCap := updateFees(tx.GasTipCap(), tx.GasFeeCap(), tip, basefee, m.l)
if reusedTip && reusedFeeCap {
if tx.GasTipCapIntCmp(gasTipCap) == 0 && tx.GasFeeCapIntCmp(gasFeeCap) == 0 {
return tx, nil
}
......
op-service/txmgr/txmgr_test.go
View file @ ef1d2266
......@@ -4,7 +4,6 @@ import (
"context"
"errors"
"math/big"
"math/rand"
"sync"
"testing"
"time"
......@@ -12,13 +11,10 @@ import (
"github.com/stretchr/testify/require"
"github.com/ethereum-optimism/optimism/op-node/testlog"
"github.com/ethereum-optimism/optimism/op-node/testutils"
opcrypto "github.com/ethereum-optimism/optimism/op-service/crypto"
"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/log"
)
......@@ -691,15 +687,10 @@ func TestWaitMinedReturnsReceiptAfterFailure(t *testing.T) {
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()
func doGasPriceIncrease(t *testing.T, txTipCap, txFeeCap, newTip, newBaseFee int64) (*types.Transaction, *types.Transaction) {
borkedBackend := failingBackend{
gasTip: big.NewInt(101),
baseFee: big.NewInt(460),
gasTip: big.NewInt(newTip),
baseFee: big.NewInt(newBaseFee),
}
mgr := &SimpleTxManager{
......@@ -715,97 +706,84 @@ func TestIncreaseGasPriceEnforcesMinBump(t *testing.T) {
},
name: "TEST",
backend: &borkedBackend,
l: testlog.Logger(t, log.LvlTrace),
l: testlog.Logger(t, log.LvlCrit),
}
tx := types.NewTx(&types.DynamicFeeTx{
GasTipCap: big.NewInt(100),
GasFeeCap: big.NewInt(1000),
GasTipCap: big.NewInt(txTipCap),
GasFeeCap: big.NewInt(txFeeCap),
})
ctx := context.Background()
newTx, err := mgr.IncreaseGasPrice(ctx, tx)
newTx, err := mgr.IncreaseGasPrice(context.Background(), tx)
require.NoError(t, err)
return tx, newTx
}
func TestIncreaseGasPrice(t *testing.T) {
// t.Parallel()
tests := []struct {
name string
run func(t *testing.T)
}{
{
name: "enforces min bump",
run: func(t *testing.T) {
tx, newTx := doGasPriceIncrease(t, 100, 1000, 101, 460)
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")
}
// TestIncreaseGasPriceEnforcesMinBumpForBothOnTipIncrease asserts that if the gasTip goes up,
// but the baseFee doesn't, both values are increased by 10%
func TestIncreaseGasPriceEnforcesMinBumpForBothOnTipIncrease(t *testing.T) {
t.Parallel()
borkedBackend := failingBackend{
gasTip: big.NewInt(101),
baseFee: big.NewInt(440),
}
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)
{
name: "enforces min bump on only tip incrase",
run: func(t *testing.T) {
tx, newTx := doGasPriceIncrease(t, 100, 1000, 101, 440)
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")
}
// TestIncreaseGasPriceEnforcesMinBumpForBothOnBaseFeeIncrease asserts that if the baseFee goes up,
// but the tip doesn't, both values are increased by 10%
// TODO(CLI-3620): This test will fail until we implemented CLI-3620.
func TestIncreaseGasPriceEnforcesMinBumpForBothOnBaseFeeIncrease(t *testing.T) {
t.Skip("Failing until CLI-3620 is implemented")
t.Parallel()
borkedBackend := failingBackend{
gasTip: big.NewInt(99),
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)
{
name: "enforces min bump on only basefee incrase",
run: func(t *testing.T) {
tx, newTx := doGasPriceIncrease(t, 100, 1000, 99, 460)
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")
},
},
{
name: "uses L1 values when larger",
run: func(t *testing.T) {
_, newTx := doGasPriceIncrease(t, 10, 100, 50, 200)
require.True(t, newTx.GasFeeCap().Cmp(big.NewInt(450)) == 0, "new tx fee cap must be equal L1")
require.True(t, newTx.GasTipCap().Cmp(big.NewInt(50)) == 0, "new tx tip must be equal L1")
},
},
{
name: "uses L1 tip when larger and threshold FC",
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")
},
},
{
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")
},
},
}
for _, test := range tests {
test := test
t.Run(test.name, test.run)
}
}
// TestIncreaseGasPriceNotExponential asserts that if the L1 basefee & tip remain the
......@@ -850,82 +828,3 @@ func TestIncreaseGasPriceNotExponential(t *testing.T) {
}
}
// 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")
}
// TestIncreaseGasPriceReusesTransaction asserts that if the L1 basefee & tip remain the
// same, the transaction is returned with the same signature values. The means that the error
// when submitting the transaction to the network is ErrAlreadyKnown instead of ErrReplacementUnderpriced
func TestIncreaseGasPriceReusesTransaction(t *testing.T) {
t.Parallel()
borkedBackend := failingBackend{
gasTip: big.NewInt(10),
baseFee: big.NewInt(45),
}
pk := testutils.InsecureRandomKey(rand.New(rand.NewSource(123)))
signer := opcrypto.PrivateKeySignerFn(pk, big.NewInt(10))
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 signer(from, tx)
},
From: crypto.PubkeyToAddress(pk.PublicKey),
},
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.Equal(t, tx.Hash(), newTx.Hash())
}
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