-
Notifications
You must be signed in to change notification settings - Fork 412
feat(testing/forks): Implement bytecode.gas_cost(fork)
#2002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(testing/forks): Implement bytecode.gas_cost(fork)
#2002
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2002 +/- ##
================================================
Coverage 86.33% 86.33%
================================================
Files 538 538
Lines 34557 34557
Branches 3222 3222
================================================
Hits 29835 29835
Misses 4148 4148
Partials 574 574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ca87d04 to
eba0d07
Compare
|
Awesome! I’ve partially reviewed the PR, and the framework changes look good so far. I still need to take a closer look at the per-opcode gas cost calculations and the updated opcode tests. I’ve also spent some time thinking about how to continue with our original plan for gas repricing and integrate it into this feature. |
|
Note, we decided not to go with an implementation that uses the exact gas returned from the specs / EVM trace. I will leave this commit hash diff here so we have a reference to it in case it is ever useful in the future: 858336e...fselmo:execution-specs:7770f14daa131b25450a67059f055978f1fe3e3d. |
ac8bb59 to
b6c3664
Compare
packages/testing/src/execution_testing/forks/tests/test_opcode_gas_costs.py
Show resolved
Hide resolved
5baf0cb to
d07b076
Compare
|
@fselmo I think I've applied all comments plus other fixes, thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't wait to use this 😄. Great changes here.
I left some very minor remaining thoughts to think about. As I scrolled through forks.py I can't help but feel there's a better way to organize things rather than overloading forks indefinitely. I'm good with the state of this PR so I'm going to approve it in order to not block... but I do wonder if starting a mixin design pattern here wouldn't help us trend in the right direction. I also left a nit on the "Prototype" naming you can feel free to take or leave 😃.
|
|
||
|
|
||
| class BaseFork(ABC, metaclass=BaseForkMeta): | ||
| class BaseFork(ForkPrototype, metaclass=BaseForkMeta): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm my only thought left in this PR is that Frontier (first fork impl) is getting really busy. And that's not going anywhere. I was trying to brainstorm some ways where we could abstract not just abstract classes out that Frontier then implements... but what if we refactored some code out of Frontier entirely into Mixin classes that it then inherits and can still override methods based on the fork?
This is a larger refactoring thought, out of scope for this PR... but I created an issue to look into this #2025.
If we do like this idea of mixins, though, we could start with this PR and name ForkPrototype -> OpcodeGasMixin - then have the actual base opcode gas implementations the gas calculator, refund calculator, gas map, etc in that class. We could have something like forks/mixins.py to abstract this out.
I think even with this mixin architecture, forks.py will still grow quite a bit as different forks require different overrides of mixin methods... but at least some of the setup can be abstracted out.
Curious on your thoughts here.
edited: BaseFork -> Frontier that's the first impl class so this is where mixins would be inherited.
d07b076 to
31316fe
Compare
…g contract size to below allowed max (ethereum#2002) * fixed push stack_overflow test by reducing contract size below allowed max * use push1(0) instead of push0 * make coverage yaml more robust * chore(ci): fix coverage when modifying existing ported tests. * fix --------- Co-authored-by: spencer-tb <spencer@spencertaylorbrown.uk>
bytecode.gas_cost(fork)bytecode.gas_costs(fork)
bytecode.gas_costs(fork)bytecode.gas_cost(fork)
🗒️ Description
Implements
bytecode.gas_cost(fork)to calculate the exact gas cost of a bytecode for a given fork.This is done by adding "metadata" to the opcodes that are used within bytecode.
Example:
In this example
Op.MSTORE8now containsnew_memory_sizemetadata to include the memory expansion calculation whenbytecode.gas_costoperates.Also
Op.RETURNincludes acode_deposit_sizeto signal that this opcode is supposed to operate within initcode and returns bytecode that creates a contract, because otherwise the cost would be zero.Updated tests
Includes updates to the following tests:
These were ported in order to debug and validate the gas calculations in the bytecodes included in them.
Bytecode retains list of opcodes
The bytecode instances now include a list of all opcodes they contain.
It was done by creating an Opcode prototype to avoid circular dependencies.
This list is used to calculate the gas cost of each opcode.
fork.opcode_gas_calculatorThe fork base type now contains a method that returns a calculator which returns the gas cost of a given opcode, which inspects the opcode object "metadata" to be able to determine dynamic gas calculation.
🔗 Related Issues or PRs
N/A.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture