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

@@ -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),

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,

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)

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
+}

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

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) {