From 8fcf025711e6ae70867e660066fbb9ae7dd2ffe5 Mon Sep 17 00:00:00 2001 From: Dimitris Date: Tue, 3 Sep 2024 14:42:08 +0300 Subject: [PATCH] Add optimizations --- core/chains/evm/gas/fee_history_estimator.go | 51 ++++++++++++------- .../evm/gas/fee_history_estimator_test.go | 38 ++++---------- 2 files changed, 43 insertions(+), 46 deletions(-) diff --git a/core/chains/evm/gas/fee_history_estimator.go b/core/chains/evm/gas/fee_history_estimator.go index d6a4d7c881c..31e4dcc8801 100644 --- a/core/chains/evm/gas/fee_history_estimator.go +++ b/core/chains/evm/gas/fee_history_estimator.go @@ -91,19 +91,21 @@ type FeeHistoryEstimator struct { l1Oracle rollups.L1Oracle - wg *sync.WaitGroup - stopCh services.StopChan + wg *sync.WaitGroup + stopCh services.StopChan + refreshCh chan struct{} } func NewFeeHistoryEstimator(lggr logger.Logger, client feeHistoryEstimatorClient, cfg FeeHistoryEstimatorConfig, chainID *big.Int, l1Oracle rollups.L1Oracle) *FeeHistoryEstimator { return &FeeHistoryEstimator{ - client: client, - logger: logger.Named(lggr, "FeeHistoryEstimator"), - config: cfg, - chainID: chainID, - l1Oracle: l1Oracle, - wg: new(sync.WaitGroup), - stopCh: make(chan struct{}), + client: client, + logger: logger.Named(lggr, "FeeHistoryEstimator"), + config: cfg, + chainID: chainID, + l1Oracle: l1Oracle, + wg: new(sync.WaitGroup), + stopCh: make(chan struct{}), + refreshCh: make(chan struct{}), } } @@ -143,13 +145,15 @@ func (f *FeeHistoryEstimator) run() { select { case <-f.stopCh: return + case <-f.refreshCh: + t.Reset() case <-t.C: if f.config.EIP1559 { if err := f.RefreshDynamicPrice(); err != nil { f.logger.Error(err) } } else { - if err := f.RefreshGasPrice(); err != nil { + if _, err := f.RefreshGasPrice(); err != nil { f.logger.Error(err) } } @@ -172,13 +176,13 @@ func (f *FeeHistoryEstimator) GetLegacyGas(ctx context.Context, _ []byte, gasLim } // RefreshGasPrice will use eth_gasPrice to fetch and cache the latest gas price from the RPC. -func (f *FeeHistoryEstimator) RefreshGasPrice() error { +func (f *FeeHistoryEstimator) RefreshGasPrice() (*assets.Wei, error) { ctx, cancel := f.stopCh.CtxCancel(evmclient.ContextWithDefaultTimeout()) defer cancel() gasPrice, err := f.client.SuggestGasPrice(ctx) if err != nil { - return fmt.Errorf("failed to fetch gas price: %w", err) + return nil, err } promFeeHistoryEstimatorGasPrice.WithLabelValues(f.chainID.String()).Set(float64(gasPrice.Int64())) @@ -190,7 +194,7 @@ func (f *FeeHistoryEstimator) RefreshGasPrice() error { f.gasPriceMu.Lock() defer f.gasPriceMu.Unlock() f.gasPrice = gasPriceWei - return nil + return f.gasPrice, nil } func (f *FeeHistoryEstimator) getGasPrice() (*assets.Wei, error) { @@ -233,7 +237,7 @@ func (f *FeeHistoryEstimator) RefreshDynamicPrice() error { // RewardPercentile will be used for maxPriorityFeePerGas estimations and connectivityPercentile to set the highest threshold for bumping. feeHistory, err := f.client.FeeHistory(ctx, max(f.config.BlockHistorySize, 1), []float64{f.config.RewardPercentile, ConnectivityPercentile}) if err != nil { - return fmt.Errorf("failed to fetch dynamic prices: %w", err) + return err } // eth_feeHistory doesn't return the latest baseFee of the range but rather the latest + 1, because it can be derived from the existing @@ -313,10 +317,11 @@ func (f *FeeHistoryEstimator) BumpLegacyGas(ctx context.Context, originalGasPric commonfee.ErrBump, originalGasPrice, maxPrice) } - currentGasPrice, err := f.getGasPrice() + currentGasPrice, err := f.RefreshGasPrice() if err != nil { return nil, 0, err } + f.IfStarted(func() { f.refreshCh <- struct{}{} }) bumpedGasPrice := originalGasPrice.AddPercentage(f.config.BumpPercent) bumpedGasPrice, err = LimitBumpedFee(originalGasPrice, currentGasPrice, bumpedGasPrice, maxPrice) @@ -336,13 +341,21 @@ func (f *FeeHistoryEstimator) BumpLegacyGas(ctx context.Context, originalGasPric // See: https://github.com/ethereum/go-ethereum/issues/24284 // It aggregates the market, bumped, and max price to provide a correct value, for both maxFeePerGas as well as maxPriorityFerPergas. func (f *FeeHistoryEstimator) BumpDynamicFee(ctx context.Context, originalFee DynamicFee, maxPrice *assets.Wei, _ []EvmPriorAttempt) (bumped DynamicFee, err error) { - // For chains that don't have a mempool there is no concept of gas bumping so we force-call FetchDynamicPrice to update the underlying base fee value + // For chains that don't have a mempool there is no concept of gas bumping so we force-call RefreshDynamicPrice to update the underlying base fee value if f.config.BlockHistorySize == 0 { - if err = f.RefreshDynamicPrice(); err != nil { - return + if !f.IfStarted(func() { + if refreshErr := f.RefreshDynamicPrice(); refreshErr != nil { + err = refreshErr + return + } + f.refreshCh <- struct{}{} + bumped, err = f.GetDynamicFee(ctx, maxPrice) + }) { + return bumped, fmt.Errorf("estimator not started") } - return f.GetDynamicFee(ctx, maxPrice) + return bumped, err } + // Sanitize original fee input // According to geth's spec we need to bump both maxFeePerGas and maxPriorityFeePerGas for the new attempt to be accepted by the RPC if originalFee.FeeCap == nil || diff --git a/core/chains/evm/gas/fee_history_estimator_test.go b/core/chains/evm/gas/fee_history_estimator_test.go index dcfae049d5d..6e42e0e2094 100644 --- a/core/chains/evm/gas/fee_history_estimator_test.go +++ b/core/chains/evm/gas/fee_history_estimator_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/smartcontractkit/chainlink-common/pkg/logger" + "github.com/smartcontractkit/chainlink-common/pkg/services/servicetest" "github.com/smartcontractkit/chainlink-common/pkg/utils/tests" "github.com/smartcontractkit/chainlink/v2/core/chains/evm/assets" @@ -85,7 +86,7 @@ func TestFeeHistoryEstimatorGetLegacyGas(t *testing.T) { cfg := gas.FeeHistoryEstimatorConfig{} u := gas.NewFeeHistoryEstimator(logger.Test(t), client, cfg, chainID, nil) - err := u.RefreshGasPrice() + _, err := u.RefreshGasPrice() assert.NoError(t, err) gasPrice, _, err := u.GetLegacyGas(tests.Context(t), nil, gasLimit, maxPrice) assert.NoError(t, err) @@ -100,7 +101,7 @@ func TestFeeHistoryEstimatorGetLegacyGas(t *testing.T) { maxPrice := assets.NewWeiI(1) u := gas.NewFeeHistoryEstimator(logger.Test(t), client, cfg, chainID, nil) - err := u.RefreshGasPrice() + _, err := u.RefreshGasPrice() assert.NoError(t, err) gas1, _, err := u.GetLegacyGas(tests.Context(t), nil, gasLimit, maxPrice) assert.NoError(t, err) @@ -128,13 +129,12 @@ func TestFeeHistoryEstimatorBumpLegacyGas(t *testing.T) { t.Run("bumps a previous attempt by BumpPercent", func(t *testing.T) { client := mocks.NewFeeHistoryEstimatorClient(t) originalGasPrice := assets.NewWeiI(10) - client.On("SuggestGasPrice", mock.Anything).Return(big.NewInt(10), nil).Once() + client.On("SuggestGasPrice", mock.Anything).Return(big.NewInt(10), nil) - cfg := gas.FeeHistoryEstimatorConfig{BumpPercent: 50} + cfg := gas.FeeHistoryEstimatorConfig{BumpPercent: 50, CacheTimeout: 5 * time.Second} u := gas.NewFeeHistoryEstimator(logger.Test(t), client, cfg, chainID, nil) - err := u.RefreshGasPrice() - assert.NoError(t, err) + servicetest.RunHealthy(t, u) gasPrice, _, err := u.BumpLegacyGas(tests.Context(t), originalGasPrice, gasLimit, maxPrice, nil) assert.NoError(t, err) assert.Equal(t, assets.NewWeiI(15), gasPrice) @@ -155,17 +155,6 @@ func TestFeeHistoryEstimatorBumpLegacyGas(t *testing.T) { assert.Error(t, err) }) - t.Run("fails if we try to bump but gas price has not been set", func(t *testing.T) { - originalGasPrice := assets.NewWeiI(10) - - cfg := gas.FeeHistoryEstimatorConfig{} - - u := gas.NewFeeHistoryEstimator(logger.Test(t), nil, cfg, chainID, nil) - _, _, err := u.BumpLegacyGas(tests.Context(t), originalGasPrice, gasLimit, maxPrice, nil) - assert.Error(t, err) - assert.ErrorContains(t, err, "gas price not set") - }) - t.Run("returns market gas price if bumped original fee is lower", func(t *testing.T) { client := mocks.NewFeeHistoryEstimatorClient(t) client.On("SuggestGasPrice", mock.Anything).Return(big.NewInt(80), nil).Once() @@ -174,8 +163,6 @@ func TestFeeHistoryEstimatorBumpLegacyGas(t *testing.T) { cfg := gas.FeeHistoryEstimatorConfig{} u := gas.NewFeeHistoryEstimator(logger.Test(t), client, cfg, chainID, nil) - err := u.RefreshGasPrice() - assert.NoError(t, err) gas, _, err := u.BumpLegacyGas(tests.Context(t), originalGasPrice, gasLimit, maxPrice, nil) assert.NoError(t, err) assert.Equal(t, assets.NewWeiI(80), gas) @@ -190,8 +177,6 @@ func TestFeeHistoryEstimatorBumpLegacyGas(t *testing.T) { maxPrice := assets.NewWeiI(14) u := gas.NewFeeHistoryEstimator(logger.Test(t), client, cfg, chainID, nil) - err := u.RefreshGasPrice() - assert.NoError(t, err) gas, _, err := u.BumpLegacyGas(tests.Context(t), originalGasPrice, gasLimit, maxPrice, nil) assert.NoError(t, err) assert.Equal(t, maxPrice, gas) @@ -206,8 +191,6 @@ func TestFeeHistoryEstimatorBumpLegacyGas(t *testing.T) { maxPrice := assets.NewWeiI(14) u := gas.NewFeeHistoryEstimator(logger.Test(t), client, cfg, chainID, nil) - err := u.RefreshGasPrice() - assert.NoError(t, err) gas, _, err := u.BumpLegacyGas(tests.Context(t), originalGasPrice, gasLimit, maxPrice, nil) assert.NoError(t, err) assert.Equal(t, maxPrice, gas) @@ -223,9 +206,7 @@ func TestFeeHistoryEstimatorBumpLegacyGas(t *testing.T) { // Price will be capped by the max price maxPrice := assets.NewWeiI(101) u := gas.NewFeeHistoryEstimator(logger.Test(t), client, cfg, chainID, nil) - err := u.RefreshGasPrice() - assert.NoError(t, err) - _, _, err = u.BumpLegacyGas(tests.Context(t), originalGasPrice, gasLimit, maxPrice, nil) + _, _, err := u.BumpLegacyGas(tests.Context(t), originalGasPrice, gasLimit, maxPrice, nil) assert.Error(t, err) }) } @@ -512,15 +493,18 @@ func TestFeeHistoryEstimatorBumpDynamicFee(t *testing.T) { BaseFee: []*big.Int{baseFee}, GasUsedRatio: nil, } - client.On("FeeHistory", mock.Anything, mock.Anything, mock.Anything).Return(feeHistoryResult, nil).Once() + client.On("FeeHistory", mock.Anything, mock.Anything, mock.Anything).Return(feeHistoryResult, nil) cfg := gas.FeeHistoryEstimatorConfig{ BlockHistorySize: 0, BumpPercent: 20, + CacheTimeout: 10 * time.Second, + EIP1559: true, } maxFeePerGas := assets.NewWei(baseFee).AddPercentage(gas.BaseFeeBufferPercentage) u := gas.NewFeeHistoryEstimator(logger.Test(t), client, cfg, chainID, nil) + servicetest.RunHealthy(t, u) bumpedFee, err := u.BumpDynamicFee(tests.Context(t), originalFee, globalMaxPrice, nil) assert.NoError(t, err) assert.Equal(t, assets.NewWeiI(0), (*assets.Wei)(maxPriorityFeePerGas))