[FVM] Fix metering limits #2217
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
janezpodhostnik
force-pushed
the
janez/fix-execution-effort-metering-limits-port2master
branch
from
89d2e62
to
d3ae6c6
Compare
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 7939698 The command Bench tests were run a total of 7 times on each branch. Results
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ramtinms
approved these changes
Mar 28, 2022
Codecov Report
@@ Coverage Diff @@
## master #2217 +/- ##
==========================================
- Coverage 57.55% 57.54% -0.02%
==========================================
Files 642 642
Lines 38129 38133 +4
==========================================
- Hits 21946 21944 -2
- Misses 13389 13400 +11
+ Partials 2794 2789 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
turbolent
approved these changes
Mar 30, 2022
|
bors merge |
bors
bot
deleted the
janez/fix-execution-effort-metering-limits-port2master
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
v0.25 version
This fixes three things:
During testing with the emulator, I discovered that the metering "limit reached" error text did not include the correct limit. The error used the limit at the meters internal precision, instead of the actual limit. (now covered in the weighted meter tests)
When testing I noticed that setting a gas limit of 999 I was only able to get a transaction to 941 execution effort before it failed. This was due to the limit enforcement happening after the fee deduction which added computation and potentially failed the transaction.
When the transaction failed, the fee deduction code also failed due to
meter.MergeMeterstill emitting an error even though limit enforcement was off.The new
fvm/fvm_test.gotest covers 2. and 3. It also makes sure that if the limit is reached (or overshot) the user is only charged up to their gas limit.