Add bump legacy gas implementation to the Suggested Price Estimator #11805

amit-momin · 2024-01-17T21:30:46Z

The SuggestedPriceEstimator provides a gas price from the eth_gasPrice RPC call. The price returned should not require bumping but there is a chance that prices spike between the call to the estimator and the tx submission. If that's the case, the confirmer would need a new price to rebroadcast but the previous implementation did not support this. This change enables the SuggestedPriceEstimator to return a new gas price for a tx using the BumpLegacyGas method. Its implementation forces a refresh of the stored gas price and returns the latest value if it is equal or higher to the previous attempt's gas price.

github-actions · 2024-01-17T21:31:10Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

…hen bumping

dimriou · 2024-01-22T13:40:38Z

core/chains/evm/gas/suggested_price_estimator.go

+	// 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())


This is a weird thing to do here. Suggested estimator is supposed to only fetch what the RPC suggests. Although bumping seems good on paper it comes with a few drawbacks. On specific chains, the buffer needs to be disabled as it will mess up the tx inclusion unless you post exactly what the RPC suggests (i.e. Fantom, Klaytn). For chains that do want to enable this, we just rely on the same configs from the block history estimator, which might not be entirely applicable here. SCALE would have to figure those values out for existing and new chains.
Side note: if we keep the buffer feature, we would have to update BumpPercent description in the docs.

Would it be possible to disable this buffering behavior by setting BumpThreshold = 0?
I think most chains (except for Fantom) that use Suggested estimator has this set in their config, for example (Klaytn, Metis).
So we can make sure that the Suggested behavior is the same by setting the BumpThreshold as needed.
We can set this for Fantom to ensure backward compatibility.

+1 on updating the docs as well.

Setting BumpThreshold = 0 should disable bumping all together in the Confirmer so don't think this should be an issue for Klaytn or Metis.
I've updated the docs for BumpPercent but worried that it requires too much knowledge of our gas estimator implementation to understand. Let me know if you have had something else in mind. I also found references to a couple related configs GasBumpPercent and GasBumpWei that seem legacy. I replaced them with BumpPercent and BumpMin to clean them up.

By default for SuggestedPrice, we already have BumpThreshold = 0
So bumping is OFF by default.
Whoever goes and changes that to enabled, must also understand that the bumpPercentrage and BumpMin will also apply to this estimator's bumping logic.

I think this is reasonable

By default for SuggestedPrice, we already have BumpThreshold = 0

@prashantkumar1982 where is this logic set? Looking at the config docs the BumpThreshold default seems to be 3 so it will not be off by default for FTM .

https://github.com/smartcontractkit/chainlink/blob/develop/docs/CONFIG.md#bumpthreshold

prashantkumar1982 · 2024-01-23T01:52:32Z

core/chains/evm/gas/suggested_price_estimator.go

-	return nil, 0, errors.New("bump gas is not supported for this chain")
+// Refresh the gas price in case the current one has gone stale.
+// 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.


Could you please add some comments describing how bumping makes use of the RPC call, and bump% and bumpMin settings?

I've updated the comments in the latest commit

prashantkumar1982

Looks good to me. Just minor comment request

dimriou · 2024-01-23T11:18:44Z

core/chains/evm/gas/cmd/arbgas/main.go

@@ -45,14 +46,17 @@ func printGetLegacyGas(ctx context.Context, e gas.EvmEstimator, calldata []byte,
 }

 const max = 50_000_000
+const bumpPercent = 20
+
+var bumpMin = assets.NewWei(big.NewInt(5_000_000_000))


How did we come up with this number? This seems very high for Arbitrum.

Since the Arbitrum configs don't have BumpPercent or BumpMin set I set these using the ones in the fallback.toml. If this seems wrong, I could run it by the Ship pod. Although I don't think these values will be used. The script is only calling GetLegacyGas on the estimator that we're creating with this config which doesn't add the buffer.

I think for the reader it would be better to either remove them completely or use some values that make sense based on the Arbitrum configs.

You're right. I've removed them so they don't cause any confusion

dimriou · 2024-01-23T12:46:56Z

core/chains/evm/gas/suggested_price_estimator.go

+	} else if err != nil {
+		return
+	}
+	if newGasPrice != nil && newGasPrice.Cmp(maxGasPriceWei) > 0 {


No need to return error here, we can let it cap on the max price down below. Just add a log below if the price gets capped so we get alerted.

I think we only want to cap at the max gas price if the price from the RPC is still lower than the max. In that scenario, we'd still be using a price that's equal or above the one recommended by the RPC. This clause checks if the price from the RPC is higher than the max gas price. If it is then capping at the max gas price would submit a tx below the recommended price which wouldn't succeed. I was thinking to treat this the same as the other estimators that have bumped past the max and return an error.

So here our maxGasPriceWei is lower than what the chain recommends.
Either we hard error, or keep bumping at maxGasPrice.
I think hard error might be better, to get NOP's attention to the bad config, instead of masking that error, and sending a Tx that we know is underpriced.

I see this as a bad configuration by the NOP.

dimriou · 2024-01-23T12:53:40Z

core/chains/evm/gas/arbitrum_estimator.go

I see some overlap between the BumpLegacyGas method we added and the gasPriceWithBuffer on the Arbitrum Estimator. gasPriceWithBuffer was implemented to solve the issue BumpLegacyGas is fixing. Perhaps it would make sense to combine them and have the arbitrum estimator bump as well and utilize the bumped price of the suggested estimator.

I think this change may make more sense with the gas estimator refactor proposal. The gasPriceWithBuffer method is adding to the gas price in the GetLegacyGas method since we retry with a new estimation for the L2 FeeOutOfValidRange error. We can remove the need for gasPriceWithBuffer and use the SuggestedPriceEstimator bump logic once this error is consolidated with Underpriced. We could then just use configs to achieve the same thing as the 50% buffer.

cl-sonarqube-production · 2024-01-24T17:53:01Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

80.5% Coverage
0.0% Duplication

prashantkumar1982 · 2024-01-24T20:19:28Z

docs/CHANGELOG.md

@@ -13,6 +13,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 - `chainlink health` CLI command and HTML `/health` endpoint, to provide human-readable views of the underlying JSON health data.
 - New job type `stream` to represent streamspecs. This job type is not yet used anywhere but will be required for Data Streams V1.
+- Environment variables `CL_MEDIAN_ENV`, `CL_SOLANA_ENV`, and `CL_STARKNET_ENV` for setting environment variables in LOOP Plugins with an `.env` file.
+  ```


Is this a bad merge?

This was from resolving a merge conflict

prashantkumar1982

Could you please spot check all chains here, and make sure to set BumpThreshold=0 everywhere as a default setting to go with your PR?

Later, we can go and manually enable it on chains that we want explicitly.
This way we don't accidentally enable gas bumping on any chain.

amit-momin · 2024-01-24T21:24:15Z

Could you please spot check all chains here, and make sure to set BumpThreshold=0 everywhere as a default setting to go with your PR?

I just double checked all of the configs and Fantom is the only chain using SuggestedPrice that doesn't have BumpThreshold=0 set. Since updating Fantom's configs will be handled by this PR, no additional changes are needed.

amit-momin requested review from samsondav and prashantkumar1982 as code owners January 17, 2024 21:30

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

amit-momin temporarily deployed to sdlc January 17, 2024 21:31 — with GitHub Actions Inactive

amit-momin force-pushed the feature/suggested-price-estimator-bump-fee branch from 2db9cff to 6e2cc86 Compare January 17, 2024 21:44

amit-momin temporarily deployed to sdlc January 17, 2024 21:44 — with GitHub Actions Inactive

Added bump legacy gas implementation to the Suggested Price Estimator

0abe662

amit-momin force-pushed the feature/suggested-price-estimator-bump-fee branch from 6e2cc86 to 0abe662 Compare January 17, 2024 21:48

amit-momin temporarily deployed to sdlc January 17, 2024 21:48 — with GitHub Actions Inactive

amit-momin requested a review from a team as a code owner January 19, 2024 22:21

amit-momin temporarily deployed to sdlc January 19, 2024 22:21 — with GitHub Actions Inactive

Updated logic to add a buffer on top of the new suggested gas price w…

9c576ad

…hen bumping

amit-momin force-pushed the feature/suggested-price-estimator-bump-fee branch from ae35326 to 9c576ad Compare January 19, 2024 22:28

amit-momin temporarily deployed to sdlc January 19, 2024 22:28 — with GitHub Actions Inactive

dimriou reviewed Jan 22, 2024

View reviewed changes

amit-momin temporarily deployed to sdlc January 22, 2024 19:17 — with GitHub Actions Inactive

Addressed feedback

0996152

amit-momin force-pushed the feature/suggested-price-estimator-bump-fee branch from 0e36b91 to 0996152 Compare January 22, 2024 19:31

amit-momin temporarily deployed to sdlc January 22, 2024 19:31 — with GitHub Actions Inactive

prashantkumar1982 reviewed Jan 23, 2024

View reviewed changes

dimriou reviewed Jan 23, 2024

View reviewed changes

Added comments to the SuggestedPriceEstimator BumpLegacyGas method

f124432

amit-momin temporarily deployed to sdlc January 23, 2024 21:09 — with GitHub Actions Inactive

Removed BumpPercent and BumpMin configs from arbgas cmd

b60cb15

amit-momin temporarily deployed to sdlc January 24, 2024 15:55 — with GitHub Actions Inactive

Merge branch 'develop' into feature/suggested-price-estimator-bump-fee

d7dd4b4

amit-momin temporarily deployed to sdlc January 24, 2024 17:42 — with GitHub Actions Inactive

prashantkumar1982 reviewed Jan 24, 2024

View reviewed changes

prashantkumar1982 approved these changes Jan 25, 2024

View reviewed changes

mohamed-mehany approved these changes Jan 25, 2024

View reviewed changes

prashantkumar1982 added this pull request to the merge queue Jan 25, 2024

Merged via the queue into develop with commit 9f70d30 Jan 25, 2024
92 checks passed

prashantkumar1982 deleted the feature/suggested-price-estimator-bump-fee branch January 25, 2024 20:36

HelloKashif mentioned this pull request Feb 2, 2024

update Fantom defaults: PriceMin = '0' #11882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bump legacy gas implementation to the Suggested Price Estimator #11805

Add bump legacy gas implementation to the Suggested Price Estimator #11805

amit-momin commented Jan 17, 2024

github-actions bot commented Jan 17, 2024

dimriou Jan 22, 2024

mohamed-mehany Jan 22, 2024

amit-momin Jan 22, 2024

prashantkumar1982 Jan 23, 2024

HelloKashif Feb 2, 2024 •

edited

Loading

prashantkumar1982 Jan 23, 2024

amit-momin Jan 23, 2024

prashantkumar1982 left a comment

dimriou Jan 23, 2024

amit-momin Jan 23, 2024

dimriou Jan 24, 2024

amit-momin Jan 24, 2024

dimriou Jan 23, 2024

amit-momin Jan 23, 2024

prashantkumar1982 Jan 24, 2024

dimriou Jan 23, 2024 •

edited

Loading

amit-momin Jan 23, 2024

cl-sonarqube-production bot commented Jan 24, 2024

prashantkumar1982 Jan 24, 2024

amit-momin Jan 24, 2024

prashantkumar1982 left a comment

amit-momin commented Jan 24, 2024

Add bump legacy gas implementation to the Suggested Price Estimator #11805

Add bump legacy gas implementation to the Suggested Price Estimator #11805

Conversation

amit-momin commented Jan 17, 2024

github-actions bot commented Jan 17, 2024

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

HelloKashif Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantkumar1982 left a 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 Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashantkumar1982 left a comment

Choose a reason for hiding this comment

amit-momin commented Jan 24, 2024

HelloKashif Feb 2, 2024 •

edited

Loading

dimriou Jan 23, 2024 •

edited

Loading