Refactor BlockHistoryEstimator check to halt excessive bumping #13297

amit-momin · 2024-05-22T21:10:39Z

Refactored the existing check to halt excessive bumping
- Previously, CheckInclusionBlocks amount of blocks had to pass since broadcast to assess an attempt
- Now, the check is an ongoing enforcement using the most recent CheckInclusionBlocks amount of blocks to calculate the CheckInclusionPercentile price
Fixed an issue where the oldest blocks in the cache were used for calculations instead of the latest ones. Depending on the configs, this could have had a negative affect on the gas estimation or the check to halt bumping.
- If theCheckInclusionBlocks config was set greater than BlockHistorySize, the latest gas estimation used would be CheckInclusionBlocks - BlockHistorySize blocks behind
- If the BlockHistorySize config was set greater than CheckInclusionBlocks, the check to halt bumping would not use all of the blocks available to it until BlockHistorySize - CheckInclusionBlocks passed

samsondav · 2024-05-23T17:56:26Z

core/chains/evm/gas/block_history_estimator.go

+	}
+	// Return error to prevent bumping if gas price is nil or if EIP1559 is enabled and tip cap is nil
+	if maxGasPrice == nil || (b.eConfig.EIP1559DynamicFees() && maxTipCap == nil) {
+		errorMsg := fmt.Sprintf("%d percentile price is not set. This is likely because there aren't any valid transactions to estimate from. Preventing bumping until valid price is available to compare", percentile)


Is this safe? This means that if head tracker is not working we can't bump txes. That seems potentially dangerous.

Good point. Think this was ok with a previous version of the proposal when we'd use the same block range for the gas estimation and this halt bumping check. Since we landed on using different number of blocks, it's possible we have a gas price set but don't have the cap set which would block bumping. I've updated this to return nil to allow bumping to continue.

After discussing with Dimitris, I don't think halting bumping if max gas price or max tip cap are set is dangerous so I'm reverting back to erroring on this condition. This issue can only arise on startup when the initial value for this have not been set yet. If there is an issue with the head tracker, we likely have bigger problems with the node. If the issue is the lack of transactions to calculate these values, we don't think it's a problem to hold on bumping until we have usable values. It'd be an extreme edge case to get consistent blocks without usable transactions after startup to reach a point that bumping needs to be blocked for a missing value. But it seems safer to block than blindly (and potentially aggressively) bump the transaction(s).

Yes, I see these values can be null only if we failed to calculate things on startup, which itself means BlockHistoryEstimator cannot work.
If you don't have blocks, you can't calculate %iles, thus not set gas prices at all.

If at a later time the HeadTracker stops, we will still have these values set, just to a stale value.

I'm ok with halting bumping in such cases. There is a risk, but then we already see HeadTracker not working, no no point going crazy with bumping.
I've seen many cases where our gas_prices are way way higher than the gas fees on a block, causing the nodes to go out of funds. So not halting is actually already causing production problems.

dimriou · 2024-05-24T10:48:19Z

core/chains/evm/gas/block_history_estimator.go

 	promBlockHistoryEstimatorSetGasPrice = promauto.NewGaugeVec(prometheus.GaugeOpts{
 		Name: "gas_updater_set_gas_price",
 		Help: "Gas updater set gas price (in Wei)",
 	},
 		[]string{"percentile", "evmChainID"},
 	)
-
+	promBlockHistoryEstimatorSetMaxPercentileGasPrice = promauto.NewGaugeVec(prometheus.GaugeOpts{


I'm a bit sceptical on adding more metrics like these. Do we even know if anyone uses them?

I'm ok with removing these but I was thinking it was harmless in case we wanted to every create dashboards in the future. I'll remove these since they can be added later if we ever do need them.

Removed in the following commit

…-s-checkConnectivity-method

dimriou · 2024-05-29T09:31:29Z

core/chains/evm/gas/block_history_estimator.go

+	blockRange := mathutil.Min(len(blockHistory), int(b.bhConfig.CheckInclusionBlocks()))
+	startIdx := len(blockHistory) - blockRange
+	checkInclusionBlocks := blockHistory[startIdx:]
+	// Check each attempt for any with a gas price or tip cap (if EIP1559 type) exceeds the latest CheckInclusionPercentile prices
 	for _, attempt := range attempts {
 		if attempt.BroadcastBeforeBlockNum == nil {


Is this check relevant anymore? What does this accomplish ?

I left this check in here as part of a belt and braces approach since it's still an assumption violation. Since we don't use attempt.BroadcastBeforeBlockNum anymore, you're right that this check isn't really protecting anything except halting bumping if a tx has an attempt without BroadcastBeforeBlockNum set.

Actually I had a major suspect this code is causing problems.

Inside our confirmer, we are setting BroadcastBeforeBlockNum to null here:

chainlink/common/txmgr/confirmer.go

Line 857 in d675d86

previousAttempt.BroadcastBeforeBlockNum = nil

I couldn't find where we do again set the value back to a valid block.
Could you check that, and ensure it isn't set to null forever?

Otherwise, I'm in favor of removing this check

I believe after this code the Confirmer tries broadcast the transaction. Whether it's successful or fails, it saves the attempt without a BroadcastBeforeBlockNum. Then on the next call of processHead, we set the block num for all broadcast attempts here:

chainlink/common/txmgr/confirmer.go

Lines 293 to 295 in 58d73ec

if err := ec.txStore.SetBroadcastBeforeBlockNum(ctx, head.BlockNumber(), ec.chainID); err != nil {

return fmt.Errorf("SetBroadcastBeforeBlockNum failed: %w", err)

}

The only time an attempt wouldn't have a BroadcastBeforeBlockNum is if it is either in_progress or insufficient_funds state. But txs with an insufficient_funds attempt are never passed to bumping and attempts aren't marked as in_progress anywhere in the call stack before bumping.

If we want to ensure we don't bump for txs with either in_progress or insufficient_funds attempts, I can change this validation to check that all attempts are broadcast state. Seems like it would be functionally the same as the BroadcastBeforeBlockNum check but much more straight forward. I'm also ok removing this check but seems like a good check to have for a belts and braces approach since we would want to block bumping if a tx has a non-broadcast state attempt.

Thanks for the validation. Looks good.

…r tip cap

dimriou · 2024-05-31T09:51:03Z

core/chains/evm/gas/block_history_estimator.go

+	gweiMultiplier := big.NewFloat(1_000_000_000)
+	float := new(big.Float).SetInt(maxPercentileGasPrice.ToInt())
+	gwei, _ := big.NewFloat(0).Quo(float, gweiMultiplier).Float64()
+	maxPercentileGasPriceGwei := fmt.Sprintf("%.2f", gwei)


Can you take a look at the assets package and see if you can reuse something from there to simplify this?

Good call! Found an existing method

dimriou · 2024-05-31T09:51:16Z

core/chains/evm/gas/block_history_estimator.go

+	float = new(big.Float).SetInt(maxPercentileTipCap.ToInt())
+	gwei, _ = big.NewFloat(0).Quo(float, gweiMultiplier).Float64()
+	maxPercentileTipCapGwei := fmt.Sprintf("%.2f", gwei)


Same as above

…-s-checkConnectivity-method

cl-sonarqube-production · 2024-05-31T17:40:39Z

Quality Gate passed

Issues
1 New issue
1 Fixed issue
0 Accepted issues

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

See analysis details on SonarQube

prashantkumar1982

Nice change, this simplifies the logic, and will hopefully fix all the runaway bumping bugs that we see.
Just added 1 comment around BroadcastBeforeBlockNum, which I suspect is a problem.
Otherwise looks good.

prashantkumar1982 · 2024-06-01T13:14:48Z

core/chains/evm/gas/block_history_estimator.go

+	blockRange := mathutil.Min(len(blockHistory), int(b.bhConfig.CheckInclusionBlocks()))
+	startIdx := len(blockHistory) - blockRange
+	checkInclusionBlocks := blockHistory[startIdx:]
+	// Check each attempt for any with a gas price or tip cap (if EIP1559 type) exceeds the latest CheckInclusionPercentile prices
 	for _, attempt := range attempts {
 		if attempt.BroadcastBeforeBlockNum == nil {


Actually I had a major suspect this code is causing problems.

Inside our confirmer, we are setting BroadcastBeforeBlockNum to null here:

chainlink/common/txmgr/confirmer.go

Line 857 in d675d86

previousAttempt.BroadcastBeforeBlockNum = nil

I couldn't find where we do again set the value back to a valid block.
Could you check that, and ensure it isn't set to null forever?

prashantkumar1982 · 2024-06-01T13:16:47Z

core/chains/evm/gas/block_history_estimator.go

+	blockRange := mathutil.Min(len(blockHistory), int(b.bhConfig.CheckInclusionBlocks()))
+	startIdx := len(blockHistory) - blockRange
+	checkInclusionBlocks := blockHistory[startIdx:]
+	// Check each attempt for any with a gas price or tip cap (if EIP1559 type) exceeds the latest CheckInclusionPercentile prices
 	for _, attempt := range attempts {
 		if attempt.BroadcastBeforeBlockNum == nil {


Otherwise, I'm in favor of removing this check

prashantkumar1982 · 2024-06-01T13:32:40Z

core/chains/evm/gas/block_history_estimator.go

+	}
+	// Return error to prevent bumping if gas price is nil or if EIP1559 is enabled and tip cap is nil
+	if maxGasPrice == nil || (b.eConfig.EIP1559DynamicFees() && maxTipCap == nil) {
+		errorMsg := fmt.Sprintf("%d percentile price is not set. This is likely because there aren't any valid transactions to estimate from. Preventing bumping until valid price is available to compare", percentile)


Yes, I see these values can be null only if we failed to calculate things on startup, which itself means BlockHistoryEstimator cannot work.
If you don't have blocks, you can't calculate %iles, thus not set gas prices at all.

If at a later time the HeadTracker stops, we will still have these values set, just to a stale value.

I'm ok with halting bumping in such cases. There is a risk, but then we already see HeadTracker not working, no no point going crazy with bumping.
I've seen many cases where our gas_prices are way way higher than the gas fees on a block, causing the nodes to go out of funds. So not halting is actually already causing production problems.

dimriou

For this scope I'm approving this but we need to tackle the issues discussed internally in slack.

amit-momin added 2 commits May 22, 2024 15:40

Refactored BlockHistoryEstimator check to halt excessive bumping

c82cce8

Added changeset

b7076ae

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

Fixed linting

0a2c735

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

amit-momin marked this pull request as ready for review May 23, 2024 14:57

amit-momin requested a review from a team as a code owner May 23, 2024 14:57

samsondav requested changes May 23, 2024

View reviewed changes

dimriou reviewed May 24, 2024

View reviewed changes

Addressed feedback

c9bea11

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

Merge branch 'develop' into BCI-2883-Refactor-Block-History-Estimator…

bb48f58

…-s-checkConnectivity-method

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

Merge branch 'develop' into BCI-2883-Refactor-Block-History-Estimator…

2959862

…-s-checkConnectivity-method

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

dimriou reviewed May 29, 2024

View reviewed changes

Reverted halt bumping check to prevent bumping on nil max gas price o…

d52b8fb

…r tip cap

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

Updated comment

5a270ae

amit-momin temporarily deployed to sdlc May 29, 2024 17:25 — with GitHub Actions Inactive

Fixed test

6d8da25

amit-momin temporarily deployed to sdlc May 30, 2024 16:28 — with GitHub Actions Inactive

dimriou reviewed May 31, 2024

View reviewed changes

Cleaned up gwei formatting

bc92433

amit-momin temporarily deployed to sdlc May 31, 2024 16:04 — with GitHub Actions Inactive

Merge branch 'develop' into BCI-2883-Refactor-Block-History-Estimator…

45788fa

…-s-checkConnectivity-method

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

prashantkumar1982 reviewed Jun 1, 2024

View reviewed changes

prashantkumar1982 approved these changes Jun 7, 2024

View reviewed changes

dimriou approved these changes Jun 7, 2024

View reviewed changes

prashantkumar1982 enabled auto-merge June 7, 2024 20:14

samsondav approved these changes Jun 10, 2024

View reviewed changes

prashantkumar1982 added this pull request to the merge queue Jun 10, 2024

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

dimriou added this pull request to the merge queue Jun 10, 2024

Merged via the queue into develop with commit e55e042 Jun 10, 2024
109 checks passed

dimriou deleted the BCI-2883-Refactor-Block-History-Estimator-s-checkConnectivity-method branch June 10, 2024 15:12

This was referenced Jun 10, 2024

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

Draft

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor BlockHistoryEstimator check to halt excessive bumping #13297

Refactor BlockHistoryEstimator check to halt excessive bumping #13297

amit-momin commented May 22, 2024

samsondav May 23, 2024

amit-momin May 24, 2024

amit-momin May 29, 2024

prashantkumar1982 Jun 1, 2024

dimriou May 24, 2024

amit-momin May 24, 2024

amit-momin May 24, 2024

dimriou May 29, 2024

amit-momin May 29, 2024

prashantkumar1982 Jun 1, 2024

prashantkumar1982 Jun 1, 2024

amit-momin Jun 3, 2024 •

edited

Loading

prashantkumar1982 Jun 7, 2024

dimriou May 31, 2024

amit-momin May 31, 2024

dimriou May 31, 2024

cl-sonarqube-production bot commented May 31, 2024

prashantkumar1982 left a comment

prashantkumar1982 Jun 1, 2024

prashantkumar1982 Jun 1, 2024

prashantkumar1982 Jun 1, 2024

dimriou left a comment

	if err := ec.txStore.SetBroadcastBeforeBlockNum(ctx, head.BlockNumber(), ec.chainID); err != nil {
	return fmt.Errorf("SetBroadcastBeforeBlockNum failed: %w", err)
	}

Refactor BlockHistoryEstimator check to halt excessive bumping #13297

Refactor BlockHistoryEstimator check to halt excessive bumping #13297

Conversation

amit-momin commented May 22, 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

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

amit-momin Jun 3, 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

cl-sonarqube-production bot commented May 31, 2024

Quality Gate passed

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

dimriou left a comment

Choose a reason for hiding this comment

amit-momin Jun 3, 2024 •

edited

Loading