Gas limit estimation feature #14041

amit-momin · 2024-08-05T23:57:30Z

Added gas limit estimation feature to the EVM gas estimators
Added EstimateGasLimit config to toggle the new feature

…estimation-feature

prashantkumar1982 · 2024-08-10T00:16:00Z

core/chains/evm/gas/models.go

-	fee.Legacy, chainSpecificFeeLimit, err = e.EvmEstimator.GetLegacyGas(ctx, calldata, feeLimit, maxFeePrice, opts...)
-	if err != nil {
-		return
+	if e.geCfg.EstimateGasLimit() {


For readability, could you create a new function, called getEstimatedGasUsed()?
And the ethereum.CallMsg, .geCfg.EstimateGasLimit() checks, client calls, please move all to that function.
This makes it very clear to reader what part is for the GetFee(), vs for getEstimatedGasUsed().

prashantkumar1982 · 2024-08-10T00:19:42Z

core/chains/evm/gas/models.go

@@ -261,32 +264,53 @@ func (e *evmFeeEstimator) L1Oracle() rollups.L1Oracle {
 	return e.EvmEstimator.L1Oracle()
 }

-func (e *evmFeeEstimator) GetFee(ctx context.Context, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, opts ...feetypes.Opt) (fee EvmFee, chainSpecificFeeLimit uint64, err error) {
+func (e *evmFeeEstimator) GetFee(ctx context.Context, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, toAddress *common.Address, opts ...feetypes.Opt) (fee EvmFee, chainSpecificFeeLimit uint64, err error) {


After your changes, the return value from this function, param "chainSpecificFeeLimit", doesn't represent the absolute gas limit that caller provided. Instead, it represents what we estimate this specific attempt would consume, provided these fee params are used.

So better to rename the param to suggestedGasLimit. And in comment for this function, explain what this is.

Also, could you check the callers of this functions, whether they use this return value correctly, and next time when they gas bump, they pass in the input param "feeLimit" as original limit sent by caller, not the feeLimit used in previous attempt before a gas bump.

prashantkumar1982 · 2024-08-10T00:27:37Z

core/chains/evm/gas/models.go

+			// Do not return error if estimating gas failed, we can still use the provided limit instead since it is an upper limit
+			e.lggr.Errorw("failed to estimate gas limit. falling back to provided gas limit.", "toAddress", toAddress, "data", calldata, "gasLimit", chainSpecificFeeLimit, "error", estimateErr)
+		} else {
+			chainSpecificFeeLimit = estimatedGasLimit


This estimatedGasLimit is the estimated Gas returned to us by the chain.
Due to fluctuations or errors, the actual gas used may be slightly more than this, causing the actual Tx to fail.
I think we should add a buffer on top of what the RPC returns to us.
Make that buffer a config value, EstimatedGasBuffer, set to default of 25%.
This protects us from fluctuations.

Also, you need to ensure that feeLimit > estimatedGasLimit. If not, then we can error out. Basically the chain is saying the gas used will be more than what the callers allows us to use, thus making this Tx infeasible.

Lastly, ensure that SuggestedGasLimit = MIN(feeLimit, estimatedGasLimit+Buffer)

I wanted to avoid adding another config so I was using the existing LimitMultiplier as the buffer. Though, I could see the distinction between the 2 causing confusion because LimitMultiplier was expected to be added on top of the provided limit but estimated gas * EstimatedGasBuffer would be expected to stay under the provided limit

huangzhen1997 · 2024-08-12T20:36:34Z

core/chains/evm/gas/helpers_test.go

@@ -157,6 +157,7 @@ type MockGasEstimatorConfig struct {
 	FeeCapDefaultF      *assets.Wei
 	LimitMaxF           uint64
 	ModeF               string
+	EstimateGasLimitF   bool


curious what's the F for ?

Think we do this because the linter doesn't like it when the field has the same name as the method to access it. Not sure why "F" but just following the pattern here

huangzhen1997 · 2024-08-12T22:20:33Z

core/services/relay/evm/chain_writer.go

@@ -186,7 +186,7 @@ func (w *chainWriter) GetFeeComponents(ctx context.Context) (*commontypes.ChainF
 		return nil, fmt.Errorf("gas estimator not available")
 	}

-	fee, _, err := w.ge.GetFee(ctx, nil, 0, w.maxGasPrice)
+	fee, _, err := w.ge.GetFee(ctx, nil, 0, w.maxGasPrice, nil)


If e.geCfg.EstimateGasLimit() is enabled, will we get/log error in this case with empty callMsg?

I don't think we would because all of the parameters for the eth_estimateGas call are actually optional. We'd probably get some default value if EstimateGasLimit is enabled but it won't be used in cases like this.

Joe does have a point though that if CW wants to access gas limits they would have to update their API and provide a to address. Something to keep in mind.

Right, they would. If we move forward with exposing EstimateGas to products, we had discussed we'd do it through this. But they'd have to update the other parameters too.

dimriou · 2024-08-13T12:01:19Z

core/chains/evm/gas/models.go

@@ -33,11 +33,11 @@ type EvmFeeEstimator interface {

 	// L1Oracle returns the L1 gas price oracle only if the chain has one, e.g. OP stack L2s and Arbitrum.
 	L1Oracle() rollups.L1Oracle
-	GetFee(ctx context.Context, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, opts ...feetypes.Opt) (fee EvmFee, chainSpecificFeeLimit uint64, err error)
+	GetFee(ctx context.Context, calldata []byte, feeLimit uint64, maxFeePrice *assets.Wei, toAddress *common.Address, opts ...feetypes.Opt) (fee EvmFee, chainSpecificFeeLimit uint64, err error)


Perhaps using common.Address instead of a pointer simplifies the API?

I kept this as a pointer so GetFee callers that don't care about the gas limit can pass a nil like in the case of the ChainWriter.

Makes sense, but we also need to add a check here to make sure the address is properly set.

The To field in ethereum.CallMsg is actually *common.Address so we can pass this directly in whether it is set or not.

Yes, but I'm guessing if we do and it's empty it's going to use the 0x0 which might not be the intended behavior?

If it's a nil address I think geth handles it properly as an estimation for contract creation. If someone passes a non-nil empty common.Address, it's still a valid address for estimation.

dimriou · 2024-08-13T12:38:31Z

core/chains/evm/gas/models.go

-	if err != nil {
-		return
+	if e.geCfg.EstimateGasLimit() {
+		callMsg.Gas = chainSpecificFeeLimit


We don't have to set a gas limit to get an estimation. This can be omitted.

I'm interested in knowing what is the exact requirement from the product teams regarding gas limit estimation. Personally, I would remove any requirements for GasLimit limits if EstimateGasLimit is enabled. Usually, when we have fallback values people tend to set them wrong because they're not aware what is the impact. I've observed the same pattern with the gas estimators where we have default values that are only used once.

I was using it as a way to cap the estimation to the limit products sent us but I'll remove this and add that restriction in code for better flexibility

At least for the initial solution that's needed for Mantle, I'm trying to honor the gas limits products provide as a cap to avoid any changes in behavior/assumptions. Functions has mentioned that they would need us to honor the cap otherwise it could lead to failures at the contract level. We can discuss more in the thread though.

…estimation-feature

dimriou · 2024-08-21T16:40:55Z

core/chains/evm/gas/models.go

+		Data: calldata,
+	}
+	estimatedGas, estimateErr := e.ethClient.EstimateGas(ctx, callMsg)
+	if estimateErr != nil {


In the future, we could perhaps add a retry.

I think we technically already do. This would surface as a gas estimator error which are retryable here

…tMultiplier

…estimation-feature

huangzhen1997

lgtm, one small nit

huangzhen1997 · 2024-08-21T22:08:05Z

core/chains/evm/gas/models.go

+			e.lggr.Errorw("failed to estimate gas limit. falling back to provided gas limit.", "callMsg", callMsg, "providedGasLimit", providedGasLimit, "error", estimateErr)
+			return providedGasLimit, nil
+		}
+		return estimatedFeeLimit, fmt.Errorf("gas estimation failed and provided gas limit is 0: %w", estimateErr)


nit:
for providedGasLimit , do we want to mention this provided gas limit is calculated from provided fee and multiplier ?

Think it's worth calling out to be explicit. I've made the change.

…estimation-feature

huangzhen1997 · 2024-08-21T22:18:31Z

core/chains/evm/gas/models.go

+	// If provided gas limit is not 0, fallback to it if the buffer causes the estimated gas limit to exceed it
+	// The provided gas limit should be used as an upper bound to avoid unexpected behavior for products
+	if providedGasLimit > 0 && estimatedFeeLimit > providedGasLimit {
+		e.lggr.Debugw("estimated gas limit with buffer exceeds provided limit. falling back to the provided gas limit", "estimatedGasLimit", estimatedFeeLimit, "providedGasLimit", providedGasLimit)


noticed one more here for the multiplier to make it consistent with line 369

Good catch! I just updated all of the logs. Also updated the log to be written more like spoken word than a formula.

…estimation-feature

cl-sonarqube-production · 2024-08-22T20:09:15Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
96.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

amit-momin added 2 commits August 5, 2024 18:55

Added gas limit estimation feature to EVM gas estimators

baaeef6

Added changeset

2ccdf9d

amit-momin temporarily deployed to sdlc August 5, 2024 23:57 — with GitHub Actions Inactive

Fixed linting

b1d16de

amit-momin temporarily deployed to sdlc August 6, 2024 00:18 — with GitHub Actions Inactive

Merge branch 'develop' into BCI-3658-Implement-the-dynamic-gas-limit-…

8c4b278

…estimation-feature

amit-momin had a problem deploying to sdlc August 6, 2024 23:05 — with GitHub Actions Failure

Added new failure tests

392b93b

amit-momin temporarily deployed to sdlc August 7, 2024 18:18 — with GitHub Actions Inactive

Fixed test

8797154

amit-momin temporarily deployed to sdlc August 7, 2024 21:30 — with GitHub Actions Inactive

amit-momin added 2 commits August 7, 2024 16:53

Merge branch 'develop' into BCI-3658-Implement-the-dynamic-gas-limit-…

47ab728

…estimation-feature

Fixed mock configs in ccip capabilities

7ded8c4

amit-momin temporarily deployed to sdlc August 7, 2024 21:59 — with GitHub Actions Inactive

Fixed configs

67a665e

amit-momin temporarily deployed to sdlc August 7, 2024 23:50 — with GitHub Actions Inactive

Fixed test

a43d391

amit-momin temporarily deployed to sdlc August 8, 2024 00:10 — with GitHub Actions Inactive

Refactored GetFee method

e09a7ca

amit-momin temporarily deployed to sdlc August 8, 2024 20:08 — with GitHub Actions Inactive

amit-momin marked this pull request as ready for review August 8, 2024 22:31

amit-momin requested review from a team as code owners August 8, 2024 22:31

prashantkumar1982 requested changes Aug 10, 2024

View reviewed changes

huangzhen1997 reviewed Aug 12, 2024

View reviewed changes

dimriou reviewed Aug 13, 2024

View reviewed changes

amit-momin added 2 commits August 14, 2024 01:26

Added EstimatedGasBuffer and addressed feedback

4305ee3

Merge branch 'develop' into BCI-3658-Implement-the-dynamic-gas-limit-…

b6537da

…estimation-feature

Merge branch 'develop' into BCI-3658-Implement-the-dynamic-gas-limit-…

7b8a668

…estimation-feature

dimriou reviewed Aug 21, 2024

View reviewed changes

dimriou previously approved these changes Aug 21, 2024

View reviewed changes

huangzhen1997 previously approved these changes Aug 21, 2024

View reviewed changes

amit-momin added 2 commits August 21, 2024 16:25

Updated estimate gas buffer to be a fixed value instead of using Limi…

327d434

…tMultiplier

Merge branch 'develop' into BCI-3658-Implement-the-dynamic-gas-limit-…

9e896c0

…estimation-feature

amit-momin dismissed stale reviews from huangzhen1997 and dimriou via 9e896c0 August 21, 2024 21:25

huangzhen1997 previously approved these changes Aug 21, 2024

View reviewed changes

amit-momin added 2 commits August 21, 2024 17:13

Updated log message

4dc5cb5

Merge branch 'develop' into BCI-3658-Implement-the-dynamic-gas-limit-…

03db68e

…estimation-feature

amit-momin dismissed huangzhen1997’s stale review via 03db68e August 21, 2024 22:13

huangzhen1997 reviewed Aug 21, 2024

View reviewed changes

Updated logs

fe6bb79

huangzhen1997 approved these changes Aug 21, 2024

View reviewed changes

dimriou approved these changes Aug 22, 2024

View reviewed changes

prashantkumar1982 approved these changes Aug 22, 2024

View reviewed changes

prashantkumar1982 added this pull request to the merge queue Aug 22, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 22, 2024

Merge branch 'develop' into BCI-3658-Implement-the-dynamic-gas-limit-…

be17d56

…estimation-feature

prashantkumar1982 enabled auto-merge August 22, 2024 19:53

prashantkumar1982 added this pull request to the merge queue Aug 22, 2024

Merged via the queue into develop with commit 8d818ea Aug 22, 2024
150 of 151 checks passed

prashantkumar1982 deleted the BCI-3658-Implement-the-dynamic-gas-limit-estimation-feature branch August 22, 2024 20:37

github-actions bot mentioned this pull request Aug 22, 2024

[DO NOT MERGE] Changeset Release Preview - v2.19.0 #13148

Draft

github-actions bot mentioned this pull request Sep 5, 2024

[DO NOT MERGE] Changeset Release Preview - v2.17.0 horberlan/chainlink#1

Draft

github-actions bot mentioned this pull request Sep 20, 2024

[DO NOT MERGE] Changeset Release Preview - v2.17.0 YoutacRandS-VA/chainlink#23

Draft

ma33r mentioned this pull request Sep 23, 2024

Cherry-Picked Dynamic Gas Estimation for Mantle smartcontractkit/ccip#1460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gas limit estimation feature #14041

Gas limit estimation feature #14041

amit-momin commented Aug 5, 2024 •

edited by jira bot

Loading

prashantkumar1982 Aug 10, 2024

prashantkumar1982 Aug 10, 2024

prashantkumar1982 Aug 10, 2024

amit-momin Aug 13, 2024 •

edited

Loading

huangzhen1997 Aug 12, 2024

amit-momin Aug 13, 2024

huangzhen1997 Aug 12, 2024

amit-momin Aug 13, 2024

dimriou Aug 21, 2024

amit-momin Aug 21, 2024

dimriou Aug 13, 2024

amit-momin Aug 13, 2024

dimriou Aug 19, 2024

amit-momin Aug 19, 2024

dimriou Aug 19, 2024

amit-momin Aug 19, 2024

dimriou Aug 13, 2024 •

edited

Loading

amit-momin Aug 13, 2024

dimriou Aug 21, 2024

amit-momin Aug 21, 2024

huangzhen1997 left a comment

huangzhen1997 Aug 21, 2024 •

edited

Loading

amit-momin Aug 21, 2024

huangzhen1997 Aug 21, 2024

amit-momin Aug 21, 2024

cl-sonarqube-production bot commented Aug 22, 2024

Gas limit estimation feature #14041

Gas limit estimation feature #14041

Conversation

amit-momin commented Aug 5, 2024 • edited by jira bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amit-momin Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimriou Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huangzhen1997 left a comment

Choose a reason for hiding this comment

huangzhen1997 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Aug 22, 2024

Quality Gate passed

amit-momin commented Aug 5, 2024 •

edited by jira bot

Loading

amit-momin Aug 13, 2024 •

edited

Loading

dimriou Aug 13, 2024 •

edited

Loading

huangzhen1997 Aug 21, 2024 •

edited

Loading