Remove buffer gas added in the gas consumption of EVM.dryRun
#6844
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6844 +/- ##
==========================================
- Coverage 41.19% 41.18% -0.01%
==========================================
Files 2109 2109
Lines 185664 185630 -34
==========================================
- Hits 76479 76452 -27
+ Misses 102777 102766 -11
- Partials 6408 6412 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
m-Peter
force-pushed
the
mpeter/dry-run-remove-buffer-gas
branch
from
99d8c10 to
0b7c592
Compare
|
The code looks good, but I'm not too familiar with why we have this and why we can now remove it. Could you please add some text describing this in the PR description? |
Good point 👌 Updated the description #6844 (comment) |
fvm/evm/evm_test.go
Outdated
| @@ -1779,11 +1764,13 @@ func TestDryRun(t *testing.T) { | |||
| evmAddress, | |||
| )) | |||
|
|
|||
| // use the gas estimation from Evm.dryRun with the necessary buffer gas | |||
| gasLimit := dryRunResult.GasConsumed + gethParams.SstoreSentryGasEIP2200 + gethParams.SstoreClearsScheduleRefundEIP3529 | |||
There was a problem hiding this comment.
Why choosing SstoreSentryGasEIP2200 and SstoreClearsScheduleRefundEIP3529 as gas buffer? Is there any specific reason?
There was a problem hiding this comment.
Yep, this specific case is testing the scenario test dry run clear current value.
Whenever a user executes a tx that clears the value at a storage slot, he will get some gas refund (SstoreClearsScheduleRefundEIP3529). However, this gas amount has to be present in the gas limit set by the tx, and after the successful execution, the user will get refunded.
Note: SstoreSentryGasEIP2200 was not really necessary here, so I removed it in 532136b .
| @@ -1513,11 +1512,13 @@ func TestDryRun(t *testing.T) { | |||
| evmAddress, | |||
| )) | |||
|
|
|||
| // Use the gas estimation from Evm.dryRun with some buffer | |||
| gasLimit := dryRunResult.GasConsumed + gethParams.SstoreSentryGasEIP2200 | |||
There was a problem hiding this comment.
Any specific reason to use SstoreSentryGasEIP2200 as buffer?
There was a problem hiding this comment.
Here as well, the buffer gas is required to be present in the gas limit.
This case is testing the scenario: test dry run store current value.
The description of SstoreSentryGasEIP2200 is the following:
Minimum gas required to be present for an SSTORE call, not consumedI think this is something special for avoiding re-entrancy attacks.
m-Peter
force-pushed
the
mpeter/dry-run-remove-buffer-gas
branch
from
0b7c592 to
532136b
Compare
Closes: #6843
Relevant issue: #6301
The buffer gas added in
EVM.dryRun, was a best-effort solution, to find the minimum required gas, that would make a transaction succeed. The calculation was based on top of the actual gas used for a given transaction. However, this did have some issues, and it would fail for certain cases.With the implementation of the local state index on EVM Gateway, we have completely reworked the
eth_estimateGasendpoint, and it uses the same logic asGeth, to calculate the minimum required gas needed for a transaction to succeed.With that change deployed, we no longer need to add any buffer gas to the result of
EVM.dryRun. And this can in fact cause confusion to developers, as it is always a bit higher that the gas usage they would get from tracing the same transaction / contract call.