From 9f70d300ec7a31e7703ff69518cae463be0aa71f Mon Sep 17 00:00:00 2001 From: amit-momin <108959691+amit-momin@users.noreply.github.com> Date: Thu, 25 Jan 2024 14:10:17 -0600 Subject: [PATCH] Add bump legacy gas implementation to the Suggested Price Estimator (#11805) * Added bump legacy gas implementation to the Suggested Price Estimator * Updated logic to add a buffer on top of the new suggested gas price when bumping * Addressed feedback * Added comments to the SuggestedPriceEstimator BumpLegacyGas method * Removed BumpPercent and BumpMin configs from arbgas cmd --- common/fee/models.go | 4 +- core/chains/evm/gas/arbitrum_estimator.go | 4 +- .../chains/evm/gas/arbitrum_estimator_test.go | 44 ++++- core/chains/evm/gas/cmd/arbgas/main.go | 12 +- core/chains/evm/gas/models.go | 4 +- .../evm/gas/suggested_price_estimator.go | 100 +++++++++--- .../evm/gas/suggested_price_estimator_test.go | 153 ++++++++++++++++-- core/config/docs/chains-evm.toml | 8 +- docs/CHANGELOG.md | 1 + docs/CONFIG.md | 8 +- 10 files changed, 283 insertions(+), 55 deletions(-) diff --git a/common/fee/models.go b/common/fee/models.go index 1fe4d2b053b..ff397fd2f9f 100644 --- a/common/fee/models.go +++ b/common/fee/models.go @@ -41,7 +41,7 @@ func CalculateBumpedFee( toChainUnit feeUnitToChainUnit, ) (*big.Int, error) { maxFeePrice := bigmath.Min(maxFeePriceInput, maxBumpPrice) - bumpedFeePrice := maxBumpedFee(originalfeePrice, bumpPercent, bumpMin) + bumpedFeePrice := MaxBumpedFee(originalfeePrice, bumpPercent, bumpMin) // Update bumpedFeePrice if currentfeePrice is higher than bumpedFeePrice and within maxFeePrice bumpedFeePrice = maxFee(lggr, currentfeePrice, bumpedFeePrice, maxFeePrice, "fee price", toChainUnit) @@ -61,7 +61,7 @@ func CalculateBumpedFee( } // Returns highest bumped fee price of originalFeePrice bumped by fixed units or percentage. -func maxBumpedFee(originalFeePrice *big.Int, feeBumpPercent uint16, feeBumpUnits *big.Int) *big.Int { +func MaxBumpedFee(originalFeePrice *big.Int, feeBumpPercent uint16, feeBumpUnits *big.Int) *big.Int { return bigmath.Max( addPercentage(originalFeePrice, feeBumpPercent), new(big.Int).Add(originalFeePrice, feeBumpUnits), diff --git a/core/chains/evm/gas/arbitrum_estimator.go b/core/chains/evm/gas/arbitrum_estimator.go index ca819ba04b3..a94fddc0e9d 100644 --- a/core/chains/evm/gas/arbitrum_estimator.go +++ b/core/chains/evm/gas/arbitrum_estimator.go @@ -24,6 +24,8 @@ import ( type ArbConfig interface { LimitMax() uint32 + BumpPercent() uint16 + BumpMin() *assets.Wei } //go:generate mockery --quiet --name ethClient --output ./mocks/ --case=underscore --structname ETHClient @@ -56,7 +58,7 @@ func NewArbitrumEstimator(lggr logger.Logger, cfg ArbConfig, rpcClient rpcClient lggr = logger.Named(lggr, "ArbitrumEstimator") return &arbitrumEstimator{ cfg: cfg, - EvmEstimator: NewSuggestedPriceEstimator(lggr, rpcClient), + EvmEstimator: NewSuggestedPriceEstimator(lggr, rpcClient, cfg), client: ethClient, pollPeriod: 10 * time.Second, logger: lggr, diff --git a/core/chains/evm/gas/arbitrum_estimator_test.go b/core/chains/evm/gas/arbitrum_estimator_test.go index 9d56e2c10ad..dad48b528bb 100644 --- a/core/chains/evm/gas/arbitrum_estimator_test.go +++ b/core/chains/evm/gas/arbitrum_estimator_test.go @@ -23,13 +23,23 @@ import ( ) type arbConfig struct { - v uint32 + v uint32 + bumpPercent uint16 + bumpMin *assets.Wei } func (a *arbConfig) LimitMax() uint32 { return a.v } +func (a *arbConfig) BumpPercent() uint16 { + return a.bumpPercent +} + +func (a *arbConfig) BumpMin() *assets.Wei { + return a.bumpMin +} + func TestArbitrumEstimator(t *testing.T) { t.Parallel() @@ -38,6 +48,8 @@ func TestArbitrumEstimator(t *testing.T) { calldata := []byte{0x00, 0x00, 0x01, 0x02, 0x03} const gasLimit uint32 = 80000 const gasPriceBufferPercentage = 50 + const bumpPercent = 10 + var bumpMin = assets.NewWei(big.NewInt(1)) t.Run("calling GetLegacyGas on unstarted estimator returns error", func(t *testing.T) { rpcClient := mocks.NewRPCClient(t) @@ -66,7 +78,7 @@ func TestArbitrumEstimator(t *testing.T) { assert.Equal(t, big.NewInt(-1), blockNumber) }).Return(zeros.Bytes(), nil) - o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{v: maxGasLimit}, rpcClient, ethClient) + o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{v: maxGasLimit, bumpPercent: bumpPercent, bumpMin: bumpMin}, rpcClient, ethClient) servicetest.RunHealthy(t, o) gasPrice, chainSpecificGasLimit, err := o.GetLegacyGas(testutils.Context(t), calldata, gasLimit, maxGasPrice) require.NoError(t, err) @@ -124,12 +136,12 @@ func TestArbitrumEstimator(t *testing.T) { assert.Equal(t, uint32(0), chainSpecificGasLimit) }) - t.Run("calling BumpLegacyGas always returns error", func(t *testing.T) { + t.Run("calling BumpLegacyGas on unstarted arbitrum estimator returns error", func(t *testing.T) { rpcClient := mocks.NewRPCClient(t) ethClient := mocks.NewETHClient(t) o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{}, rpcClient, ethClient) _, _, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(42), gasLimit, assets.NewWeiI(10), nil) - assert.EqualError(t, err, "bump gas is not supported for this chain") + assert.EqualError(t, err, "estimator is not started") }) t.Run("calling GetLegacyGas on started estimator if initial call failed returns error", func(t *testing.T) { @@ -152,6 +164,26 @@ func TestArbitrumEstimator(t *testing.T) { assert.EqualError(t, err, "failed to estimate gas; gas price not set") }) + t.Run("calling GetDynamicFee always returns error", func(t *testing.T) { + rpcClient := mocks.NewRPCClient(t) + ethClient := mocks.NewETHClient(t) + o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{}, rpcClient, ethClient) + _, _, err := o.GetDynamicFee(testutils.Context(t), gasLimit, maxGasPrice) + assert.EqualError(t, err, "dynamic fees are not implemented for this estimator") + }) + + t.Run("calling BumpDynamicFee always returns error", func(t *testing.T) { + rpcClient := mocks.NewRPCClient(t) + ethClient := mocks.NewETHClient(t) + o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{}, rpcClient, ethClient) + fee := gas.DynamicFee{ + FeeCap: assets.NewWeiI(42), + TipCap: assets.NewWeiI(5), + } + _, _, err := o.BumpDynamicFee(testutils.Context(t), fee, gasLimit, maxGasPrice, nil) + assert.EqualError(t, err, "dynamic fees are not implemented for this estimator") + }) + t.Run("limit computes", func(t *testing.T) { rpcClient := mocks.NewRPCClient(t) ethClient := mocks.NewETHClient(t) @@ -177,7 +209,7 @@ func TestArbitrumEstimator(t *testing.T) { assert.Equal(t, big.NewInt(-1), blockNumber) }).Return(b.Bytes(), nil) - o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{v: maxGasLimit}, rpcClient, ethClient) + o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{v: maxGasLimit, bumpPercent: bumpPercent, bumpMin: bumpMin}, rpcClient, ethClient) servicetest.RunHealthy(t, o) gasPrice, chainSpecificGasLimit, err := o.GetLegacyGas(testutils.Context(t), calldata, gasLimit, maxGasPrice) require.NoError(t, err) @@ -211,7 +243,7 @@ func TestArbitrumEstimator(t *testing.T) { assert.Equal(t, big.NewInt(-1), blockNumber) }).Return(b.Bytes(), nil) - o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{v: maxGasLimit}, rpcClient, ethClient) + o := gas.NewArbitrumEstimator(logger.Test(t), &arbConfig{v: maxGasLimit, bumpPercent: bumpPercent, bumpMin: bumpMin}, rpcClient, ethClient) servicetest.RunHealthy(t, o) gasPrice, chainSpecificGasLimit, err := o.GetLegacyGas(testutils.Context(t), calldata, gasLimit, maxGasPrice) require.Error(t, err, "expected error but got (%s, %d)", gasPrice, chainSpecificGasLimit) diff --git a/core/chains/evm/gas/cmd/arbgas/main.go b/core/chains/evm/gas/cmd/arbgas/main.go index e4fb1b4e882..c457b091c96 100644 --- a/core/chains/evm/gas/cmd/arbgas/main.go +++ b/core/chains/evm/gas/cmd/arbgas/main.go @@ -67,9 +67,19 @@ func withEstimator(ctx context.Context, lggr logger.SugaredLogger, url string, f var _ gas.ArbConfig = &config{} type config struct { - max uint32 + max uint32 + bumpPercent uint16 + bumpMin *assets.Wei } func (c *config) LimitMax() uint32 { return c.max } + +func (c *config) BumpPercent() uint16 { + return c.bumpPercent +} + +func (c *config) BumpMin() *assets.Wei { + return c.bumpMin +} diff --git a/core/chains/evm/gas/models.go b/core/chains/evm/gas/models.go index 8d977df0991..9f0e8ab0ba6 100644 --- a/core/chains/evm/gas/models.go +++ b/core/chains/evm/gas/models.go @@ -87,7 +87,7 @@ func NewEstimator(lggr logger.Logger, ethClient evmclient.Client, cfg Config, ge } case "L2Suggested", "SuggestedPrice": newEstimator = func(l logger.Logger) EvmEstimator { - return NewSuggestedPriceEstimator(lggr, ethClient) + return NewSuggestedPriceEstimator(lggr, ethClient, geCfg) } default: lggr.Warnf("GasEstimator: unrecognised mode '%s', falling back to FixedPriceEstimator", s) @@ -404,7 +404,7 @@ func BumpDynamicFeeOnly(config bumpConfig, feeCapBufferBlocks uint16, lggr logge // - A configured percentage bump (EVM.GasEstimator.BumpPercent) on top of the baseline tip cap. // - A configured fixed amount of Wei (ETH_GAS_PRICE_WEI) on top of the baseline tip cap. // The baseline tip cap is the maximum of the previous tip cap attempt and the node's current tip cap. -// It increases the max fee cap by GasBumpPercent +// It increases the max fee cap by BumpPercent // // NOTE: We would prefer to have set a large FeeCap and leave it fixed, bumping // the Tip only. Unfortunately due to a flaw of how EIP-1559 is implemented we diff --git a/core/chains/evm/gas/suggested_price_estimator.go b/core/chains/evm/gas/suggested_price_estimator.go index 3b02b7f4362..fe6483f40f3 100644 --- a/core/chains/evm/gas/suggested_price_estimator.go +++ b/core/chains/evm/gas/suggested_price_estimator.go @@ -2,6 +2,7 @@ package gas import ( "context" + "fmt" "slices" "sync" "time" @@ -12,7 +13,9 @@ import ( "github.com/smartcontractkit/chainlink-common/pkg/logger" "github.com/smartcontractkit/chainlink-common/pkg/services" "github.com/smartcontractkit/chainlink-common/pkg/utils" + bigmath "github.com/smartcontractkit/chainlink-common/pkg/utils/big_math" + "github.com/smartcontractkit/chainlink/v2/common/fee" feetypes "github.com/smartcontractkit/chainlink/v2/common/fee/types" "github.com/smartcontractkit/chainlink/v2/core/chains/evm/assets" evmclient "github.com/smartcontractkit/chainlink/v2/core/chains/evm/client" @@ -23,6 +26,11 @@ var ( _ EvmEstimator = &SuggestedPriceEstimator{} ) +type suggestedPriceConfig interface { + BumpPercent() uint16 + BumpMin() *assets.Wei +} + //go:generate mockery --quiet --name rpcClient --output ./mocks/ --case=underscore --structname RPCClient type rpcClient interface { CallContext(ctx context.Context, result interface{}, method string, args ...interface{}) error @@ -32,6 +40,7 @@ type rpcClient interface { type SuggestedPriceEstimator struct { services.StateMachine + cfg suggestedPriceConfig client rpcClient pollPeriod time.Duration logger logger.Logger @@ -46,11 +55,12 @@ type SuggestedPriceEstimator struct { } // NewSuggestedPriceEstimator returns a new Estimator which uses the suggested gas price. -func NewSuggestedPriceEstimator(lggr logger.Logger, client rpcClient) EvmEstimator { +func NewSuggestedPriceEstimator(lggr logger.Logger, client rpcClient, cfg suggestedPriceConfig) EvmEstimator { return &SuggestedPriceEstimator{ client: client, pollPeriod: 10 * time.Second, logger: logger.Named(lggr, "SuggestedPriceEstimator"), + cfg: cfg, chForceRefetch: make(chan (chan struct{})), chInitialised: make(chan struct{}), chStop: make(chan struct{}), @@ -122,42 +132,43 @@ func (o *SuggestedPriceEstimator) refreshPrice() (t *time.Timer) { return } +// Uses the force refetch chan to trigger a price update and blocks until complete +func (o *SuggestedPriceEstimator) forceRefresh(ctx context.Context) (err error) { + ch := make(chan struct{}) + select { + case o.chForceRefetch <- ch: + case <-o.chStop: + return errors.New("estimator stopped") + case <-ctx.Done(): + return ctx.Err() + } + select { + case <-ch: + case <-o.chStop: + return errors.New("estimator stopped") + case <-ctx.Done(): + return ctx.Err() + } + return +} + func (o *SuggestedPriceEstimator) OnNewLongestChain(context.Context, *evmtypes.Head) {} func (*SuggestedPriceEstimator) GetDynamicFee(_ context.Context, _ uint32, _ *assets.Wei) (fee DynamicFee, chainSpecificGasLimit uint32, err error) { - err = errors.New("dynamic fees are not implemented for this layer 2") + err = errors.New("dynamic fees are not implemented for this estimator") return } func (*SuggestedPriceEstimator) BumpDynamicFee(_ context.Context, _ DynamicFee, _ uint32, _ *assets.Wei, _ []EvmPriorAttempt) (bumped DynamicFee, chainSpecificGasLimit uint32, err error) { - err = errors.New("dynamic fees are not implemented for this layer 2") + err = errors.New("dynamic fees are not implemented for this estimator") return } func (o *SuggestedPriceEstimator) GetLegacyGas(ctx context.Context, _ []byte, GasLimit uint32, maxGasPriceWei *assets.Wei, opts ...feetypes.Opt) (gasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) { chainSpecificGasLimit = GasLimit - ok := o.IfStarted(func() { if slices.Contains(opts, feetypes.OptForceRefetch) { - ch := make(chan struct{}) - select { - case o.chForceRefetch <- ch: - case <-o.chStop: - err = errors.New("estimator stopped") - return - case <-ctx.Done(): - err = ctx.Err() - return - } - select { - case <-ch: - case <-o.chStop: - err = errors.New("estimator stopped") - return - case <-ctx.Done(): - err = ctx.Err() - return - } + err = o.forceRefresh(ctx) } if gasPrice = o.getGasPrice(); gasPrice == nil { err = errors.New("failed to estimate gas; gas price not set") @@ -177,8 +188,47 @@ func (o *SuggestedPriceEstimator) GetLegacyGas(ctx context.Context, _ []byte, Ga return } -func (o *SuggestedPriceEstimator) BumpLegacyGas(_ context.Context, _ *assets.Wei, _ uint32, _ *assets.Wei, _ []EvmPriorAttempt) (bumpedGasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) { - return nil, 0, errors.New("bump gas is not supported for this chain") +// Refreshes the gas price by making a call to the RPC in case the current one has gone stale. +// Adds the larger of BumpPercent and BumpMin configs as a buffer on top of the price returned from the RPC. +// The only reason bumping logic would be called on the SuggestedPriceEstimator is if there was a significant price spike +// between the last price update and when the tx was submitted. Refreshing the price helps ensure the latest market changes are accounted for. +func (o *SuggestedPriceEstimator) BumpLegacyGas(ctx context.Context, originalFee *assets.Wei, feeLimit uint32, maxGasPriceWei *assets.Wei, _ []EvmPriorAttempt) (newGasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) { + chainSpecificGasLimit = feeLimit + ok := o.IfStarted(func() { + // Immediately return error if original fee is greater than or equal to the max gas price + // Prevents a loop of resubmitting the attempt with the max gas price + if originalFee.Cmp(maxGasPriceWei) >= 0 { + err = fmt.Errorf("original fee (%s) greater than or equal to max gas price (%s) so cannot be bumped further", originalFee.String(), maxGasPriceWei.String()) + return + } + err = o.forceRefresh(ctx) + if newGasPrice = o.getGasPrice(); newGasPrice == nil { + err = errors.New("failed to refresh and return gas; gas price not set") + return + } + o.logger.Debugw("GasPrice", newGasPrice, "GasLimit", feeLimit) + }) + if !ok { + return nil, 0, errors.New("estimator is not started") + } else if err != nil { + return + } + if newGasPrice != nil && newGasPrice.Cmp(maxGasPriceWei) > 0 { + return nil, 0, errors.Errorf("estimated gas price: %s is greater than the maximum gas price configured: %s", newGasPrice.String(), maxGasPriceWei.String()) + } + // Add a buffer on top of the gas price returned by the RPC. + // Bump logic when using the suggested gas price from an RPC is realistically only needed when there is increased volatility in gas price. + // This buffer is a precaution to increase the chance of getting this tx on chain + bufferedPrice := fee.MaxBumpedFee(newGasPrice.ToInt(), o.cfg.BumpPercent(), o.cfg.BumpMin().ToInt()) + // If the new suggested price is less than or equal to the max and the buffer puts the new price over the max, return the max price instead + // The buffer is added on top of the suggested price during bumping as just a precaution. It is better to resubmit the transaction with the max gas price instead of erroring. + newGasPrice = assets.NewWei(bigmath.Min(bufferedPrice, maxGasPriceWei.ToInt())) + + // Return the original price if the refreshed price with the buffer is lower to ensure the bumped gas price is always equal or higher to the previous attempt + if originalFee != nil && originalFee.Cmp(newGasPrice) > 0 { + return originalFee, chainSpecificGasLimit, nil + } + return } func (o *SuggestedPriceEstimator) getGasPrice() (GasPrice *assets.Wei) { diff --git a/core/chains/evm/gas/suggested_price_estimator_test.go b/core/chains/evm/gas/suggested_price_estimator_test.go index 8e45f5b759a..9006554564d 100644 --- a/core/chains/evm/gas/suggested_price_estimator_test.go +++ b/core/chains/evm/gas/suggested_price_estimator_test.go @@ -26,9 +26,11 @@ func TestSuggestedPriceEstimator(t *testing.T) { calldata := []byte{0x00, 0x00, 0x01, 0x02, 0x03} const gasLimit uint32 = 80000 + cfg := &gas.MockGasEstimatorConfig{BumpPercentF: 10, BumpMinF: assets.NewWei(big.NewInt(1)), BumpThresholdF: 1} + t.Run("calling GetLegacyGas on unstarted estimator returns error", func(t *testing.T) { client := mocks.NewRPCClient(t) - o := gas.NewSuggestedPriceEstimator(logger.Test(t), client) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) _, _, err := o.GetLegacyGas(testutils.Context(t), calldata, gasLimit, maxGasPrice) assert.EqualError(t, err, "estimator is not started") }) @@ -40,7 +42,7 @@ func TestSuggestedPriceEstimator(t *testing.T) { (*big.Int)(res).SetInt64(42) }) - o := gas.NewSuggestedPriceEstimator(logger.Test(t), client) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) servicetest.RunHealthy(t, o) gasPrice, chainSpecificGasLimit, err := o.GetLegacyGas(testutils.Context(t), calldata, gasLimit, maxGasPrice) require.NoError(t, err) @@ -50,7 +52,7 @@ func TestSuggestedPriceEstimator(t *testing.T) { t.Run("gas price is lower than user specified max gas price", func(t *testing.T) { client := mocks.NewRPCClient(t) - o := gas.NewSuggestedPriceEstimator(logger.Test(t), client) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(nil).Run(func(args mock.Arguments) { res := args.Get(1).(*hexutil.Big) @@ -67,7 +69,7 @@ func TestSuggestedPriceEstimator(t *testing.T) { t.Run("gas price is lower than global max gas price", func(t *testing.T) { client := mocks.NewRPCClient(t) - o := gas.NewSuggestedPriceEstimator(logger.Test(t), client) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(nil).Run(func(args mock.Arguments) { res := args.Get(1).(*hexutil.Big) @@ -81,16 +83,9 @@ func TestSuggestedPriceEstimator(t *testing.T) { assert.Equal(t, uint32(0), chainSpecificGasLimit) }) - t.Run("calling BumpLegacyGas always returns error", func(t *testing.T) { - client := mocks.NewRPCClient(t) - o := gas.NewSuggestedPriceEstimator(logger.Test(t), client) - _, _, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(42), gasLimit, assets.NewWeiI(10), nil) - assert.EqualError(t, err, "bump gas is not supported for this chain") - }) - t.Run("calling GetLegacyGas on started estimator if initial call failed returns error", func(t *testing.T) { client := mocks.NewRPCClient(t) - o := gas.NewSuggestedPriceEstimator(logger.Test(t), client) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(errors.New("kaboom")) @@ -99,4 +94,138 @@ func TestSuggestedPriceEstimator(t *testing.T) { _, _, err := o.GetLegacyGas(testutils.Context(t), calldata, gasLimit, maxGasPrice) assert.EqualError(t, err, "failed to estimate gas; gas price not set") }) + + t.Run("calling GetDynamicFee always returns error", func(t *testing.T) { + client := mocks.NewRPCClient(t) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + _, _, err := o.GetDynamicFee(testutils.Context(t), gasLimit, maxGasPrice) + assert.EqualError(t, err, "dynamic fees are not implemented for this estimator") + }) + + t.Run("calling BumpLegacyGas on unstarted estimator returns error", func(t *testing.T) { + client := mocks.NewRPCClient(t) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + _, _, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(42), gasLimit, maxGasPrice, nil) + assert.EqualError(t, err, "estimator is not started") + }) + + t.Run("calling BumpDynamicFee always returns error", func(t *testing.T) { + client := mocks.NewRPCClient(t) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + fee := gas.DynamicFee{ + FeeCap: assets.NewWeiI(42), + TipCap: assets.NewWeiI(5), + } + _, _, err := o.BumpDynamicFee(testutils.Context(t), fee, gasLimit, maxGasPrice, nil) + assert.EqualError(t, err, "dynamic fees are not implemented for this estimator") + }) + + t.Run("calling BumpLegacyGas on started estimator returns new price buffered with bumpPercent", func(t *testing.T) { + client := mocks.NewRPCClient(t) + client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(nil).Run(func(args mock.Arguments) { + res := args.Get(1).(*hexutil.Big) + (*big.Int)(res).SetInt64(40) + }) + + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + servicetest.RunHealthy(t, o) + gasPrice, chainSpecificGasLimit, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(10), gasLimit, maxGasPrice, nil) + require.NoError(t, err) + assert.Equal(t, assets.NewWeiI(44), gasPrice) + assert.Equal(t, gasLimit, chainSpecificGasLimit) + }) + + t.Run("calling BumpLegacyGas on started estimator returns new price buffered with bumpMin", func(t *testing.T) { + client := mocks.NewRPCClient(t) + client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(nil).Run(func(args mock.Arguments) { + res := args.Get(1).(*hexutil.Big) + (*big.Int)(res).SetInt64(40) + }) + + testCfg := &gas.MockGasEstimatorConfig{BumpPercentF: 1, BumpMinF: assets.NewWei(big.NewInt(1)), BumpThresholdF: 1} + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, testCfg) + servicetest.RunHealthy(t, o) + gasPrice, chainSpecificGasLimit, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(10), gasLimit, maxGasPrice, nil) + require.NoError(t, err) + assert.Equal(t, assets.NewWeiI(41), gasPrice) + assert.Equal(t, gasLimit, chainSpecificGasLimit) + }) + + t.Run("calling BumpLegacyGas on started estimator returns original price when lower than previous", func(t *testing.T) { + client := mocks.NewRPCClient(t) + client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(nil).Run(func(args mock.Arguments) { + res := args.Get(1).(*hexutil.Big) + (*big.Int)(res).SetInt64(5) + }) + + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + servicetest.RunHealthy(t, o) + gasPrice, chainSpecificGasLimit, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(10), gasLimit, maxGasPrice, nil) + require.NoError(t, err) + assert.Equal(t, assets.NewWeiI(10), gasPrice) + assert.Equal(t, gasLimit, chainSpecificGasLimit) + }) + + t.Run("calling BumpLegacyGas on started estimator returns error, suggested gas price is higher than max gas price", func(t *testing.T) { + client := mocks.NewRPCClient(t) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + + client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(nil).Run(func(args mock.Arguments) { + res := args.Get(1).(*hexutil.Big) + (*big.Int)(res).SetInt64(42) + }) + + servicetest.RunHealthy(t, o) + gasPrice, chainSpecificGasLimit, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(10), gasLimit, assets.NewWeiI(40), nil) + require.Error(t, err) + assert.EqualError(t, err, "estimated gas price: 42 wei is greater than the maximum gas price configured: 40 wei") + assert.Nil(t, gasPrice) + assert.Equal(t, uint32(0), chainSpecificGasLimit) + }) + + t.Run("calling BumpLegacyGas on started estimator returns max gas price when suggested price under max but the buffer exceeds it", func(t *testing.T) { + client := mocks.NewRPCClient(t) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + + client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(nil).Run(func(args mock.Arguments) { + res := args.Get(1).(*hexutil.Big) + (*big.Int)(res).SetInt64(39) + }) + + servicetest.RunHealthy(t, o) + gasPrice, chainSpecificGasLimit, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(10), gasLimit, assets.NewWeiI(40), nil) + require.NoError(t, err) + assert.Equal(t, assets.NewWeiI(40), gasPrice) + assert.Equal(t, gasLimit, chainSpecificGasLimit) + }) + + t.Run("calling BumpLegacyGas on started estimator if initial call failed returns error", func(t *testing.T) { + client := mocks.NewRPCClient(t) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + + client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(errors.New("kaboom")) + + servicetest.RunHealthy(t, o) + + _, _, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(10), gasLimit, maxGasPrice, nil) + assert.EqualError(t, err, "failed to refresh and return gas; gas price not set") + }) + + t.Run("calling BumpLegacyGas on started estimator if refresh call failed returns price from previous update", func(t *testing.T) { + client := mocks.NewRPCClient(t) + o := gas.NewSuggestedPriceEstimator(logger.Test(t), client, cfg) + + client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(nil).Run(func(args mock.Arguments) { + res := args.Get(1).(*hexutil.Big) + (*big.Int)(res).SetInt64(40) + }).Once() + client.On("CallContext", mock.Anything, mock.Anything, "eth_gasPrice").Return(errors.New("kaboom")) + + servicetest.RunHealthy(t, o) + + gasPrice, chainSpecificGasLimit, err := o.BumpLegacyGas(testutils.Context(t), assets.NewWeiI(10), gasLimit, maxGasPrice, nil) + require.NoError(t, err) + assert.Equal(t, assets.NewWeiI(44), gasPrice) + assert.Equal(t, gasLimit, chainSpecificGasLimit) + }) } diff --git a/core/config/docs/chains-evm.toml b/core/config/docs/chains-evm.toml index 53f5cb68529..8799bb9adea 100644 --- a/core/config/docs/chains-evm.toml +++ b/core/config/docs/chains-evm.toml @@ -161,7 +161,9 @@ LimitMultiplier = '1.0' # Default LimitTransfer = 21_000 # Default # BumpMin is the minimum fixed amount of wei by which gas is bumped on each transaction attempt. BumpMin = '5 gwei' # Default -# BumpPercent is the percentage by which to bump gas on a transaction that has exceeded `BumpThreshold`. The larger of `GasBumpPercent` and `GasBumpWei` is taken for gas bumps. +# BumpPercent is the percentage by which to bump gas on a transaction that has exceeded `BumpThreshold`. The larger of `BumpPercent` and `BumpMin` is taken for gas bumps. +# +# The `SuggestedPriceEstimator` adds the larger of `BumpPercent` and `BumpMin` on top of the price provided by the RPC when bumping a transaction's gas. BumpPercent = 20 # Default # BumpThreshold is the number of blocks to wait for a transaction stuck in the mempool before automatically bumping the gas price. Set to 0 to disable gas bumping completely. BumpThreshold = 3 # Default @@ -193,8 +195,8 @@ BumpTxDepth = 16 # Example # # Bumping works as follows: # -# - Increase tipcap by `max(tipcap * (1 + GasBumpPercent), tipcap + GasBumpWei)` -# - Increase feecap by `max(feecap * (1 + GasBumpPercent), feecap + GasBumpWei)` +# - Increase tipcap by `max(tipcap * (1 + BumpPercent), tipcap + BumpMin)` +# - Increase feecap by `max(feecap * (1 + BumpPercent), feecap + BumpMin)` # # A quick note on terminology - Chainlink nodes use the same terms used internally by go-ethereum source code to describe various prices. This is not the same as the externally used terms. For reference: # diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 0f3d9a2f053..cb4a24e0b69 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 echo "Baz=Val" >> median.env CL_MEDIAN_ENV="median.env" ``` +- Gas bumping logic to the `SuggestedPriceEstimator` ### Fixed diff --git a/docs/CONFIG.md b/docs/CONFIG.md index 2c98a41c7bf..5f831950519 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -5742,7 +5742,9 @@ BumpMin is the minimum fixed amount of wei by which gas is bumped on each transa ```toml BumpPercent = 20 # Default ``` -BumpPercent is the percentage by which to bump gas on a transaction that has exceeded `BumpThreshold`. The larger of `GasBumpPercent` and `GasBumpWei` is taken for gas bumps. +BumpPercent is the percentage by which to bump gas on a transaction that has exceeded `BumpThreshold`. The larger of `BumpPercent` and `BumpMin` is taken for gas bumps. + +The `SuggestedPriceEstimator` adds the larger of `BumpPercent` and `BumpMin` on top of the price provided by the RPC when bumping a transaction's gas. ### BumpThreshold ```toml @@ -5786,8 +5788,8 @@ If you are using BlockHistoryEstimator (default for most chains): Bumping works as follows: -- Increase tipcap by `max(tipcap * (1 + GasBumpPercent), tipcap + GasBumpWei)` -- Increase feecap by `max(feecap * (1 + GasBumpPercent), feecap + GasBumpWei)` +- Increase tipcap by `max(tipcap * (1 + BumpPercent), tipcap + BumpMin)` +- Increase feecap by `max(feecap * (1 + BumpPercent), feecap + BumpMin)` A quick note on terminology - Chainlink nodes use the same terms used internally by go-ethereum source code to describe various prices. This is not the same as the externally used terms. For reference: