-
Notifications
You must be signed in to change notification settings - Fork 391
feat(tests): unify gas cost variable #1776
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
base: forks/osaka
Are you sure you want to change the base?
feat(tests): unify gas cost variable #1776
Conversation
| def test_fork_consistency() -> None: | ||
| """Test that Prague and Osaka have same base costs for common opcodes.""" | ||
| code = Op.PUSH1(0x01) + Op.PUSH1(0x02) + Op.ADD | ||
|
|
||
| prague_calc = OpcodeGasCalculator(Prague) | ||
| osaka_calc = OpcodeGasCalculator(Osaka) | ||
|
|
||
| prague_gas = prague_calc.calculate(code) | ||
| osaka_gas = osaka_calc.calculate(code) | ||
|
|
||
| assert prague_gas == osaka_gas == 9 # 3 + 3 + 3 |
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.
This is the example usage:
code = Op.PUSH0 + Op.PUSH0 + Op.ADD
prague_calc = OpcodeGasCalculator(Prague)
prague_gas = prague_calc.calculate(code)
# the total gas cost for the code sequneceThis could be extended to:
code = Op.PUSH0 + Op.PUSH0 + Op.ADD
calc = OpcodeGasCalculator(fork)
gas =calc.calculate(code)This version could help us avoid hardcoding the gas cost. As i believe in the near future there would be gas repricing for ZkVM, and we do not want to update all the gas cost again.
| @classmethod | ||
| def op_cost( | ||
| cls, opcode: Opcodes, *, block_number: int = 0, timestamp: int = 0 | ||
| ) -> int: |
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.
We need to define this op_cost function for every fork to enable fork-dependent gas table feature.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/osaka #1776 +/- ##
===============================================
+ Coverage 86.07% 86.14% +0.07%
===============================================
Files 743 743
Lines 44078 44261 +183
Branches 3894 3891 -3
===============================================
+ Hits 37938 38127 +189
+ Misses 5659 5656 -3
+ Partials 481 478 -3
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:
|
|
🇺🇸 |
| G_TX_DATA_ZERO = 4 | ||
| G_TX_DATA_NON_ZERO = 16 | ||
|
|
||
| # Transaction costs (from transactions.py) |
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.
Comments should describe the current state of the code. The git log gives us historical context.
| G_TX_DATA_ZERO = 4 | ||
| G_TX_DATA_NON_ZERO = 16 |
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.
Should these be typed?
| PER_EMPTY_ACCOUNT_COST = 25_000 | ||
| PER_AUTH_BASE_COST = 12_500 |
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.
Should these be typed?
| PER_AUTH_BASE_COST = 12_500 | ||
|
|
||
| # Opcode Costs | ||
| G_STOP = GAS_ZERO |
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.
We try to avoid abbreviations unless they are extremely common (eg. tx -> transaction). I'm not firmly opposed to g -> gas, but I did want to mention it.
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.
I think at the very least we would want to document that so it's clear. I'm not sure that saving two characters is worth the loss in readability though.
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.
I support unifying the gas cost constants to the GAS_OPCODE format, it’s more clear.
| G_SMOD = GAS_LOW | ||
| G_ADDMOD = GAS_MID | ||
| G_MULMOD = GAS_MID | ||
| G_EXP = GAS_EXPONENTIATION |
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.
Might be worth adding a docstring to each of these constants that explains the difference?
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.
I count 7 of these, so it wouldn't be a huge addition to document each. If we do end up reorganizing this list into groups by gas cost, then these would all be in one block which would make for easy reading.
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.
@Carsons-Eels What are the 7 categories you’re referring to here? I counted 16 unique gas categories, so I may be misunderstanding something.
1. GAS_ZERO
2. GAS_VERY_LOW
3. GAS_LOW
4. GAS_MID
5. GAS_EXPONENTIATION
6. GAS_KECCAK256
7. GAS_BASE
8. GAS_WARM_ACCESS
9. GAS_COLD_ACCOUNT_ACCESS
10. GAS_COLD_SLOAD
11. GAS_BLOCK_HASH
12. GAS_BLOBHASH_OPCODE
13. GAS_JUMPDEST
14. GAS_LOG
15. GAS_CREATE
16. GAS_SELF_DESTRUCT
And this is my idea for docstring as comment, is this the format you'd preferred?
"""Cost for zero-cost operations."""
GAS_ZERO = Uint(0)
"""Cost for the JUMPDEST opcode."""
GAS_JUMPDEST = Uint(1)
"""Base cost for simple environment and stack operations."""
GAS_BASE = Uint(2)
"""Cost for very low-complexity operations."""
GAS_VERY_LOW = Uint(3)
"""Cost for low-complexity operations."""
GAS_LOW = Uint(5)
"""Cost for medium-complexity operations."""
GAS_MID = Uint(8)
"""Cost for high-complexity operations."""
GAS_HIGH = Uint(10)
"""Warm account or storage access cost."""
GAS_WARM_ACCESS = Uint(100)
...| G_SHR = GAS_VERY_LOW | ||
| G_SAR = GAS_VERY_LOW | ||
| G_CLZ = GAS_LOW | ||
| G_KECCAK256 = GAS_KECCAK256 |
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.
Same comment here about explaining the difference.
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.
If we adopt the GAS_OPCODE naming pattern, do we still need this assignments for consistency? And do we still need docstring here?
GAS_SAR = GAS_VERY_LOW
GAS_CLZ = GAS_LOW
GAS_KECCAK256 = GAS_KECCAK256
...
| G_CLZ = GAS_LOW | ||
| G_KECCAK256 = GAS_KECCAK256 | ||
| G_ADDRESS = GAS_BASE | ||
| G_BALANCE = GAS_WARM_ACCESS # or GAS_COLD_ACCOUNT_ACCESS if cold |
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.
Hm, I'm not sold on this. If the price of an opcode is dynamic, should the associated gas constant have a different naming scheme? Like G_BASE_BALANCE or G_MIN_BALANCE? I'm just musing here, a change is not necessarily required.
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.
I’d like to discuss more on this, because I’m not fully convinced my current approach is ideal either
Many opcodes use a dynamic cost model, and assigning something like the BALANCE opcode to a fixed category is confusing. I’m leaning toward your suggestion, GAS_BASE_BALANCE, but I’m open to other naming options if you prefer.
Same rules apply to memory operations that contains memory expansion cost, like GAS_BASE_MLOAD for GAS_MLOAD
| G_PUSHx = GAS_VERY_LOW | ||
| G_DUPx = GAS_VERY_LOW | ||
| G_SWAPx = GAS_VERY_LOW | ||
| G_LOGx = GAS_LOG |
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.
| G_PUSHx = GAS_VERY_LOW | |
| G_DUPx = GAS_VERY_LOW | |
| G_SWAPx = GAS_VERY_LOW | |
| G_LOGx = GAS_LOG | |
| G_PUSH = GAS_VERY_LOW | |
| G_DUP = GAS_VERY_LOW | |
| G_SWAP = GAS_VERY_LOW | |
| G_LOG = GAS_LOG |
The lowercase "x" is unusual in Python constants.
| TX_BASE_COST = Uint(21_000) | ||
| TX_CREATE_COST = Uint(32_000) | ||
| TX_ACCESS_LIST_ADDRESS_COST = Uint(2_400) | ||
| TX_ACCESS_LIST_STORAGE_KEY_COST = Uint(1_900) | ||
| STANDARD_CALLDATA_TOKEN_COST = Uint(4) | ||
| FLOOR_CALLDATA_COST = Uint(10) |
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.
IIUC, these constants are not used by anything under the ethereum.forks.osaka.vm package, right? It seems odd that they live in here if they aren't related to the virtual machine itself.
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.
Yes, this is not used in vm package but defined in transaction.py.
My current approach is to put these gas cost constant together in gas.py only, and then import them in the testing framework, like the example below.
@classmethod
def gas_costs(
cls, *, block_number: int = 0, timestamp: int = 0
) -> GasCosts:
"""
On Prague, the standard token cost and the floor token costs are
introduced due to EIP-7623.
"""
from ethereum.forks.prague.vm import gas as g
from dataclasses import fields
kwargs = {}
for field in fields(GasCosts):
if hasattr(g, field.name):
kwargs[field.name] = int(getattr(g, field.name))
return GasCosts(**kwargs)I copy these gas cost constant from transaction.py / eoa_delegation.py so in the import logic i do not need to import two files for the gas costs definition.
But i could refactor the logic to something below, wdyt?
@classmethod
def gas_costs(
cls, *, block_number: int = 0, timestamp: int = 0
) -> GasCosts:
"""Return the gas costs for the fork."""
from dataclasses import fields
from ethereum.forks.osaka.vm import gas as g
from ethereum.forks.osaka.vm import eoa_delegation as d
from ethereum.forks.osaka import transactions as tx
modules = (g, tx, d)
kwargs = {}
for field in fields(GasCosts):
for mod in modules:
if hasattr(mod, field.name):
kwargs[field.name] = int(getattr(mod, field.name))
break
else:
# executed only if the inner loop completed without a break
raise AttributeError(
f"Field '{field.name}' not found in gas, transactions, or eoa_delegation modules."
)
return GasCosts(**kwargs)I am so shocked i do not know there is for-else statement before.
Carsons-Eels
left a comment
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.
I'm not sure that I like mixing the use of the "G" and "GAS" prefixes (G_OPCODE vs GAS_OPCODE) in the codebase. If I understand correctly, the intent is to use "G" in the EELS part of the code and "GAS" in the EEST part of the code? In my mind that should be reversed, because a IMO higher priority should be placed on the how readable the spec is. Should we maybe consider simply aligning the naming of them all?
I'll keep looking this over in the morning, this is great stuff 🚀
| STANDARD_CALLDATA_TOKEN_COST = Uint(4) | ||
| FLOOR_CALLDATA_COST = Uint(10) | ||
|
|
||
| # Authorization costs (from eoa_delegation.py) |
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.
Same as here, the "(from eoa_delegation.py)" is probably not required.
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.
Does this approach resolve the problem? Based on the multiple file import scheme we do not need to copy these variable from eoa_delegation.py and transaction.py.
| PER_EMPTY_ACCOUNT_COST = 25_000 | ||
| PER_AUTH_BASE_COST = 12_500 | ||
|
|
||
| # Opcode Costs |
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.
Right now these are in order of their opcode number, but the opcode numbers are not listed so it's not easy to reference them in that way. I'm wondering if there is a better way to organize this. Maybe by common cost?
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.
Do you mean organize them by something like this?
"""Operations for very low gas cost"""
G_ADD = GAS_VERY_LOW
G_SUB = GAS_VERY_LOW
G_LT = GAS_VERY_LOW
G_GT = GAS_VERY_LOW
G_SLT = GAS_VERY_LOW
...
"""Operations for low gas cost"""
G_MUL = GAS_LOW
G_DIV = GAS_LOW
G_SDIV = GAS_LOW
G_MOD = GAS_LOW
G_SMOD = GAS_LOW
...
"""Operations for medium gas cost"""
G_ADDMOD = GAS_MID
G_MULMOD = GAS_MID
...
# continue on different gas categories...| PER_AUTH_BASE_COST = 12_500 | ||
|
|
||
| # Opcode Costs | ||
| G_STOP = GAS_ZERO |
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.
I think at the very least we would want to document that so it's clear. I'm not sure that saving two characters is worth the loss in readability though.
| G_SMOD = GAS_LOW | ||
| G_ADDMOD = GAS_MID | ||
| G_MULMOD = GAS_MID | ||
| G_EXP = GAS_EXPONENTIATION |
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.
I count 7 of these, so it wouldn't be a huge addition to document each. If we do end up reorganizing this list into groups by gas cost, then these would all be in one block which would make for easy reading.
| kwargs = {} | ||
| for field in fields(GasCosts): | ||
| if hasattr(g, field.name): | ||
| kwargs[field.name] = int(getattr(g, field.name)) | ||
|
|
||
| return GasCosts(**kwargs) |
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.
Should we return an error here if the field is not found rather than skipping it silently? Not sure if this is intended.
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.
Yes, we should return an error here like this
| G_SIGNEXTEND, | ||
| G_SMOD, | ||
| G_SUB, | ||
| GAS_EXPONENTIATION_PER_BYTE, |
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.
Why does this one keep it's GAS_ prefix rather than aligning with the others?
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.
I agree to unify the pattern, i cannot even remember why i do this before tbh
🗒️ Description
Unify the gas cost variable name from Cancun to Osaka. Introduce a fork-dependent gas table.
The mapping for the EELS & EEST gas constant, I replace all the EEST variable name with the EELS naming, as this requires less effort. This documents the mapping: https://hackmd.io/@QmVpC8TxQ8a1nTCW46EsEQ/B1932RWy-x
Note: This PR is only a prototype and will be split into smaller PRs.
🔗 Related Issues or PRs
Issue #1599
✅ 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