Improve gas estimations #5968

emlautarom1 · 2023-07-26T15:28:26Z

Fixes #5706

Changes

Change Gas estimation algorithm for more consistent results

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Notes on testing

Some tests were updated to reflect the behavior changes.

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

Remarks

The code for Gas estimation is still very complex compared to the competition (see go-ethereum implementation: https://github.com/ethereum/go-ethereum/blob/2274a03e339f213361453590b54917bbfd0a0c31/internal/ethapi/api.go#L1137). A refactor would be highly suggested.

- Cap Tx 'GasLimit'

- Set block gas limit

- Removed 'NotSupported' exceptions, unused fields, redundant casts

emlautarom1 · 2023-07-26T15:44:37Z

About gas estimation:

When a user triggers a eth_estimateGas, the payload can include an optional gas field to limit the amount of gas to be used in the tx, that is, the estimation will be at most gas. If no gas value is provided, an arbitrary default of 100_000_000 is used at the moment by Nethermind.

Here is (or was) the buggy code: If the gas value is higher than what block gas limit where the tx will be executed, an alternative gas estimation is used. This alternative estimation is not clear to me, nor are the reasons for it to exist. Since the default value is 100_000_000, on mainnet which has a block gas limit of 30_000_000, this alternative estimation is used. In practice this method is the most used given that users rarely include a gas field when asking for estimations.

This PR limits the tx gas value to be at most the block gas limit, and then uses the "default" gas estimation method, which involves a binary search to find the required amount of gas until no OutOfGas exception is triggered.

emlautarom1 · 2023-07-26T17:27:34Z

A different problem regarding gas estimations comes when a contract performs arbitrary checks on the remaining gas, like in #5637. Here, the contract used makes the following check:

require(gasleft() >= safeTxGas, "Not enough gas to execute safe transaction");

If the remaining gas doesn't satisfy the condition, execution halts and all changes get reverted, but importantly, this does not fail with an OutOfGas exception. Remember that Nethermind tries to find the minimum amount of gas required for the transaction to not fail with an OutOfGas exception, not the minimum amount of gas for the transaction to be completed successfully.

Here, there are as far as I can tell two options:

Change the estimation method to find the minimum amount of gas required for the tx to go through.
Change the contract to fail with an OutOfGas exception, somehow.

LukaszRozmej · 2023-07-27T00:36:37Z

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

-                return Math.Max(intrinsicGas, gasTracer.GasSpent + gasTracer.CalculateAdditionalGasRequired(tx, releaseSpec));
-            }
-
+            tx.GasLimit = Math.Min(tx.GasLimit, header.GasLimit); // Limit Gas to the header


What geth does here? Does it use they 100mln gas limit from transaction or 30mln gas limit for block?

Geth uses the gas value provided by the user as long as it's at least the minimum amount of gas required by any Tx (21_000), or the block gas limit by default (so in this case, 30_000_000).

LukaszRozmej · 2023-07-27T00:37:09Z

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

-            if (tx.GasLimit > header.GasLimit)
-            {
-                return Math.Max(intrinsicGas, gasTracer.GasSpent + gasTracer.CalculateAdditionalGasRequired(tx, releaseSpec));
-            }
-


This wasn't bad edge case fallback if we didn't have that gas limit discrepancy, maybe worth to keep it?

And where would we use this fallback? The problem that I had with this piece of code is that I cannot fully grasp how is this actually computing the estimated gas. I'd only keep the binary search approach with appropriate defaults and timeouts through cancellation tokens.

emlautarom1 · 2023-07-27T13:52:14Z

Added a CancellationToken to stop estimation if requested

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

# Conflicts: # src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

- Update test suite with higher estimations (mostly +1)

- Removes ugly hack that dealt with class mutation

emlautarom1 · 2023-07-28T19:14:14Z

src/Nethermind/Nethermind.Evm.Test/TransactionProcessorTests.cs

@@ -458,7 +458,7 @@ public void Can_estimate_with_destroy_refund_and_below_intrinsic_pre_berlin()
            tracer.CalculateAdditionalGasRequired(tx, releaseSpec).Should().Be(24080);
            tracer.GasSpent.Should().Be(35228L);
            long estimate = estimator.Estimate(tx, block.Header, tracer);
-            estimate.Should().Be(59307);


I cannot fully figure out why this estimations are higher than what we had before.

- Value can be < 0, so we ignore it if that's the case

emlautarom1 · 2023-07-28T19:39:55Z

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

            {
-                return gasTracer.CalculateAdditionalGasRequired(tx, releaseSpec);
+                return BinarySearchEstimate(leftBound, rightBound, tx, header, cancellationToken)
+                       + Math.Max(0, gasTracer.CalculateAdditionalGasRequired(tx, releaseSpec));


There is a chance (looks more like a bug) for the AdditionalGasRequired to be < 0, so we ignore it in those cases.

Shouldn't this be handled inside the tracer?

Why this is even added? I don't see this in the original code, if we estimate with actual running this shouldn't be needed and probably makes the tests behave differently in the first place?

Why this is even added?

I had to add this after running the whole test suite and seeing that there were failures that weren't flaky tests: there are integrations tests that verify the gas estimation (which have been updated on this PR) that fail if we don't include this AdditionalGasRequired.

emlautarom1 · 2023-07-28T19:41:08Z

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

+            long originalGasLimit = transaction.GasLimit;
+
            transaction.GasLimit = gasLimit;
-            _transactionProcessor.CallAndRestore(transaction, block, tracer);
+            _transactionProcessor.CallAndRestore(transaction, block, tracer.WithCancellation(cancellationToken));
+
+            transaction.GasLimit = originalGasLimit;


We want to preserve the original Gas limit so we are forced to do this workaround. Ideally, Transaction would implement ICloneable, so we could make a copy with the desired amount of Gas instead of mutating the original one.

LukaszRozmej · 2023-07-31T08:18:47Z

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

            {
-                return gasTracer.CalculateAdditionalGasRequired(tx, releaseSpec);
+                return BinarySearchEstimate(leftBound, rightBound, tx, header, cancellationToken)
+                       + Math.Max(0, gasTracer.CalculateAdditionalGasRequired(tx, releaseSpec));


Shouldn't this be handled inside the tracer?

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

LukaszRozmej · 2023-07-31T08:23:08Z

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

            {
-                return gasTracer.CalculateAdditionalGasRequired(tx, releaseSpec);
+                return BinarySearchEstimate(leftBound, rightBound, tx, header, cancellationToken)
+                       + Math.Max(0, gasTracer.CalculateAdditionalGasRequired(tx, releaseSpec));


Why this is even added? I don't see this in the original code, if we estimate with actual running this shouldn't be needed and probably makes the tests behave differently in the first place?

emlautarom1 · 2023-07-31T13:42:58Z

Superseded by #5973. Reason for this decision is documented there.

emlautarom1 added 3 commits July 25, 2023 14:39

Initial gas estimator fix

80ee812

- Cap Tx 'GasLimit'

Fix test case

fe18ce3

- Set block gas limit

Code cleanup

81b6072

- Removed 'NotSupported' exceptions, unused fields, redundant casts

emlautarom1 self-assigned this Jul 26, 2023

LukaszRozmej reviewed Jul 27, 2023

View reviewed changes

emlautarom1 added 2 commits July 27, 2023 10:17

Remove redundant parameter;

bb0c94c

Use 'CancellationToken'

4414046

emlautarom1 marked this pull request as ready for review July 27, 2023 20:54

LukaszRozmej reviewed Jul 27, 2023

View reviewed changes

src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs Outdated Show resolved Hide resolved

emlautarom1 added 5 commits July 28, 2023 10:35

Merge branch 'master' into fix/gas_estimation

6327d11

# Conflicts: # src/Nethermind/Nethermind.Evm/Tracing/GasEstimator.cs

Code cleanup

8ad8c96

Return 'long.MaxValue' on cancellation

fcf2c5a

Include 'AdditionalGasRequire' in estimation

51d8039

- Update test suite with higher estimations (mostly +1)

Code cleanup

b5cf2a8

- Removes ugly hack that dealt with class mutation

emlautarom1 commented Jul 28, 2023

View reviewed changes

Fix for additional gas

4b3478f

- Value can be < 0, so we ignore it if that's the case

emlautarom1 commented Jul 28, 2023

View reviewed changes

LukaszRozmej reviewed Jul 31, 2023

View reviewed changes

emlautarom1 mentioned this pull request Jul 31, 2023

Fix gas estimations #5973

Merged

16 tasks

emlautarom1 closed this Jul 31, 2023

emlautarom1 deleted the fix/gas_estimation branch July 31, 2023 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve gas estimations #5968

Improve gas estimations #5968

emlautarom1 commented Jul 26, 2023

emlautarom1 commented Jul 26, 2023 •

edited

Loading

emlautarom1 commented Jul 26, 2023

LukaszRozmej Jul 27, 2023

emlautarom1 Jul 27, 2023

LukaszRozmej Jul 27, 2023

emlautarom1 Jul 27, 2023

emlautarom1 commented Jul 27, 2023

emlautarom1 Jul 28, 2023

emlautarom1 Jul 28, 2023

LukaszRozmej Jul 31, 2023

LukaszRozmej Jul 31, 2023

emlautarom1 Jul 31, 2023

emlautarom1 Jul 28, 2023

LukaszRozmej Jul 31, 2023

LukaszRozmej Jul 31, 2023

emlautarom1 commented Jul 31, 2023 •

edited

Loading

Improve gas estimations #5968

Improve gas estimations #5968

Conversation

emlautarom1 commented Jul 26, 2023

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

Remarks

emlautarom1 commented Jul 26, 2023 • edited Loading

emlautarom1 commented Jul 26, 2023

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

emlautarom1 commented Jul 27, 2023

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

emlautarom1 commented Jul 31, 2023 • edited Loading

emlautarom1 commented Jul 26, 2023 •

edited

Loading

emlautarom1 commented Jul 31, 2023 •

edited

Loading