-
Notifications
You must be signed in to change notification settings - Fork 391
Implement EIP-7904 #1677
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
Implement EIP-7904 #1677
Conversation
SamWilsn
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.
We try to maintain the simplest possible implementation in the spec itself. Adding a class to load from a file is significantly more complex than using native Python constants.
It also breaks "Jump to Definition" in IDEs (and docc), so would make discovering the value of a constant that much more difficult.
All that said, I do see the value in making gas pricing available outside of the spec without having to import an entire Python interpreter. On the flip side, I'm not sure updating a YAML file is that much easier than updating a Python file if you want to play with different gas prices while executing the spec.
I think my preferred solution to this would be to have a very strictly formatted gas.py and provide an algorithm to parse it (and a test to ensure that the python version matches the manually parsed version). Something like:
gas.py
# Named Constants
BASE = Uint(2)
"""
Cost of the simplest EVM instructions.
"""
VERY_LOW = Uint(3)
"""
Cost of common EVM instructions.
"""
# Instruction Costs
OP_STOP = BASE
"""
Cost of the [STOP](ref:ethereum.forks.frontier.vm...) instruction.
"""Parsing
Provide a regular expression that matches something like (?P<key>[^ ]+) = (?P<value>.*).
| Initialize GasCosts by loading from gas_cost.yaml file. | ||
| """ | ||
| if yaml_path is None: | ||
| yaml_path = Path(__file__).parent / "gas_cost.yaml" |
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.
You should use importlib.resources when accessing data files.
| from pathlib import Path | ||
| from typing import Dict | ||
|
|
||
| import yaml |
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.
Please no.
| This module provides a unified dataclass for accessing gas costs | ||
| loaded from the gas_cost.yaml configuration file. | ||
| Reference: https://github.com/ethereum/execution-specs/issues/1599 |
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.
The specs themselves are always the reference. Any relevant information should be moved into this file.
| # Try static constants first | ||
| if name in self._static_costs: | ||
| return Uint(self._static_costs[name]) | ||
|
|
||
| # Try opcode costs | ||
| if name in self._opcode_values_cache: | ||
| return Uint(self._opcode_values_cache[name]) |
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 keep these as separate dicts?
| if opcode in self._opcode_values_cache: | ||
| return self._opcode_values_cache[opcode] | ||
| raise KeyError(f"Opcode '{opcode}' not found") |
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 opcode in self._opcode_values_cache: | |
| return self._opcode_values_cache[opcode] | |
| raise KeyError(f"Opcode '{opcode}' not found") | |
| try: | |
| return self._opcode_values_cache[opcode] | |
| except KeyError as e: | |
| raise KeyError(f"Opcode '{opcode}' not found") from e |
| """ | ||
| return self._opcode_values_cache.values() | ||
|
|
||
| def get_opcode_cost_name(self, opcode: str) -> str: |
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.
Prefer omitting get_ prefix for getters.
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 you're going to use YAML, you could use anchors to implement the symbolic gas costs. Would make the parsing side a bit easier. Something like:
STOP: &BASE_OPCODE_COST 1
ADD: *BASE_OPCODE_COST
MUL: *BASE_OPCODE_COST
# And so on...b354304 to
2807253
Compare
2807253 to
769a849
Compare
LouisTsai-Csie
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.
@SamWilsn I agree the yaml format introduces unnecessary complexity to the spec, the original idea is to integrate pitsop, but we do not need this anymore (due to safety reason).
I create a new structure and have some prototype, it is not working now and need more refactoring.
Unify the gas cost variable and category between EELS and EEST:
- This is the preliminary analysis of the variables. We could replace all the EEST variable with EELS naming, which require less effort.
- Use specs as the only data source for the gas variable: Move the EEST-specific variable to EELS.
- Create a new file,
opcode.py(orconstant.py), which contains all the opcode gas category and the gas table for opcodes, while other gas cost remains ingas.py.
This change will minimize the effort required, and now dev that works on gas repricing could use opcode.py as the single source of data.
Some example changes:
opcode.py: This would be the single source of data for the opcode gas cost (excluding additional overhead like memory extension or warm/cold storage access, address access cost).
# Opcode Gas Category
AS_JUMPDEST = Uint(1)
GAS_BASE = Uint(2)
...
# Opcode gas cost table
OP_STOP = GAS_BASE
OP_ADD = GAS_VERY_LOW
...gas.py: This would contain other gas cost variable other than the opcode gas cost
from .constant import (
GAS_BASE,
GAS_JUMPDEST,
...
)
from .exceptions import OutOfGasError
# Opcode Gas Category
## Import from opcode.py
GAS_JUMPDEST = GAS_JUMPDEST
GAS_BASE = GAS_BASE
....
# Other gas variable
GAS_STORAGE_UPDATE = Uint(5000)
GAS_STORAGE_CLEAR_REFUND = Uint(4800)
....arithmetirc.py: Display as an example how to access the opcode gas cost
# Import the opcode gas cost for ADD
from ..constant import (
OP_ADD,
...
)
from ..gas import (
...,
charge_gas,
)
def add(evm: Evm) -> None:
...
x = pop(evm.stack)
y = pop(evm.stack)
# access the opcode gas here
charge_gas(evm, OP_ADD)
result = x.wrapping_add(y)
push(evm.stack, result)
evm.pc += Uint(1)forks.py: In EEST, we read the variable from EELS, this would be helpful as we could define a new function: op_costs for each forks, thus creating a fork dependent gas table.
@classmethod
def gas_costs(
cls, *, block_number: int = 0, timestamp: int = 0
) -> GasCosts:
"""Return the gas costs for the fork."""
from ethereum.forks.osaka.vm import gas as g
from dataclasses import fields
# Build kwargs by matching GasCosts field names with gas module variables
kwargs = {}
for field in fields(GasCosts):
if hasattr(g, field.name):
kwargs[field.name] = getattr(g, field.name)
return GasCosts(**kwargs)| GAS_JUMPDEST = Uint(1) | ||
| GAS_BASE = Uint(2) | ||
| GAS_VERY_LOW = Uint(3) | ||
| GAS_LOW = Uint(5) | ||
| GAS_MID = Uint(8) | ||
| GAS_HIGH = Uint(10) | ||
| GAS_EXPONENTIATION = Uint(10) | ||
| GAS_KECCAK256 = Uint(30) | ||
| GAS_COLD_ACCOUNT_ACCESS = Uint(2600) | ||
| GAS_BLOCK_HASH = Uint(20) | ||
| GAS_STORAGE_SET = Uint(20000) | ||
| GAS_WARM_ACCESS = Uint(100) | ||
| GAS_COLD_SLOAD = Uint(2100) | ||
| GAS_LOG = Uint(375) | ||
| GAS_CREATE = Uint(32000) | ||
| GAS_SELF_DESTRUCT = Uint(5000) |
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.
Define the gas cost category here, which will be later imported in gas.py at the same folder level.
Define the gas cost below like:
OP_STOP = GAS_BASE
OP_ADD = GAS_VERY_LOW
OP_MUL = GAS_LOW
...Rationale: Dev could use this as the opcode gas reference
| GAS_BASE = Uint(2) | ||
| GAS_VERY_LOW = Uint(3) | ||
| GAS_STORAGE_SET = Uint(20000) | ||
| GAS_JUMPDEST = GAS_JUMPDEST |
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.
re-import the gas cost category from gas.py
|
|
||
| # GAS | ||
| charge_gas(evm, GAS_VERY_LOW) | ||
| charge_gas(evm, OP_ADD) |
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.
Replace the opcode gas cost:
- charge_gas(evm, GAS_VERY_LOW)
+ charge_gas(evm, OP_ADD)In this case, if the gas cost for ADD is updated, we do not need to update this file but only opcode.py (or constant.py, naming TBD)
| @classmethod | ||
| def gas_costs( | ||
| cls, *, block_number: int = 0, timestamp: int = 0 | ||
| ) -> GasCosts: | ||
| """Return the gas costs for the fork.""" | ||
| from ethereum.forks.osaka.vm import gas as g | ||
| from dataclasses import fields | ||
|
|
||
| # Build kwargs by matching GasCosts field names with gas module variables | ||
| kwargs = {} | ||
| for field in fields(GasCosts): | ||
| if hasattr(g, field.name): | ||
| kwargs[field.name] = getattr(g, field.name) | ||
| # Handle special mappings where names differ | ||
| elif field.name == "G_WARM_SLOAD": | ||
| kwargs[field.name] = g.GAS_WARM_ACCESS | ||
| elif field.name == "G_INITCODE_WORD": | ||
| kwargs[field.name] = g.GAS_INIT_CODE_WORD_COST | ||
|
|
||
| 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.
We could import the variable in EEST like this, and create a new function for opcode cost. With this, we have a fork dependent gas table
|
There are still a lot of changes required - but would be helpful to receive some feedback for this format. |
|
Closed, in favor of PR #1776 |
🗒️ Description
Detailed background in this issue.
Basic concept is that we extract all the gas cost value into one configuration file, such as
gas_costs.yaml. It would be the gas cost data source for both specs & tests.Two major benefit:
More details for the changes:
gas_cost.yamland loaded as anGasCostsobject via script inconstant.py(both undersrc/ethereum/forks/osaka/utils/).src/ethereum/forks/osaka/vm. TakeADDfor example , we replacecharge_gas(evm, G.G_LOW)withcharge_gas(evm, G.ADD). This is applied to every opcode.packages/tests/src/ethereum_test_forks/forks.py::gas_costs.🔗 Related Issues or PRs
Issue #1479
Issue #1599
EIP-7904: https://eips.ethereum.org/EIPS/eip-7904
✅ 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