feat(spec-specs, tests): alt to merge 8037 to forks/amsterdam#2901
feat(spec-specs, tests): alt to merge 8037 to forks/amsterdam#2901spencer-tb wants to merge 110 commits into
Conversation
…se (ethereum#2363) * feat(spec-specs): update EIP-8037 to match latest spec revision * feat(tests): add EIP-8037 state creation gas cost increase tests
Co-authored-by: Ben Adams <thundercat@illyriad.co.uk>
Add sstore_state_gas(), code_deposit_state_gas(), and create_state_gas() calculator methods to Fork. Replace Spec constants and manual gas arithmetic across all EIP-8037 tests with dynamic fork method calls. Remove redundant Op.STOP, hardcoded numbers from docstrings, and add fork: Fork parameter to all test functions that use fork methods.
Test CREATE with max initcode size using proper regular/state gas split via the reservoir. Verifies gas boundary behavior with EIP-8037 two-dimensional metering.
Align EIP-8037 gas constant references with upstream renames: - GAS_STORAGE_UPDATE → GAS_COLD_STORAGE_WRITE - GAS_COLD_SLOAD → GAS_COLD_STORAGE_ACCESS - GAS_WARM_ACCOUNT_ACCESS → GAS_WARM_ACCESS Bump gas_limit on tests added to upstream after EIP-8037 branched, which now need extra gas for EIP-8037 state gas costs.
… format runs in withdrawal request contract tests (ethereum#2532) * fix(tests): prevent tx_gas_limit double-accumulation across fixture format runs in withdrawal request contract tests * fix: mypy * fix: ruff * fix: ruff * chore(tests): refactor fix to fork-aware transactions to prevent mutation * chore(test): add a warning to all tests that could mutate vars; address in later PR Add a lightweight repr-based snapshot hook to the filler plugin that warns whenever any ``pytest.param`` value is mutated during a test run. A subsequent PR could address this by returning values instead of mutating, then flipping the hook to a hard failure. --------- Co-authored-by: fselmo <fselmo2@gmail.com>
…hereum#2526) Co-authored-by: fselmo <fselmo2@gmail.com>
… gas validity test (ethereum#2583) Co-authored-by: Stefan <22667037+qu0b@users.noreply.github.com>
Conditionally increase tx gas_limit (and env gas_limit where needed) when fork >= Amsterdam to account for EIP-8037 state creation gas costs. 137 files, 9 with env gas_limit bumps. Headroom: 2,000,000.
Move MAX_CODE_SIZE check before gas charges, charge keccak hash cost (regular gas) before code deposit state gas, and add tests for over-max code size and reservoir preservation after OOG.
On CREATE/CREATE2 address collision the 63/64 gas allocation is burned but was not added to regular_gas_used, leaving it invisible to 2D block gas accounting. Per EIP-684 collision behaves as an immediate exceptional halt, so the burned gas belongs in the regular dimension.
…and subcall pattern Co-authored-by: Mario Vega <marioevz@gmail.com>
The static test skip list and conftest were a temporary workaround for EIP-8037 gas failures. The ported static tests in tests/ported_static/ replace this approach; failures are tracked in ethereum#2601.
…thereum#2603) * fix(execute): use --sender-fund-refund-gas-limit for all funding txs On EIP-8037 networks, simple value transfers to new accounts require more than 21000 gas due to GAS_NEW_ACCOUNT state gas (112 * cpsb). The default Transaction gas_limit of 21000 causes 'intrinsic gas too low' errors during test setup. Changes: - contracts.py: Use 200000 gas for deterministic factory deployer funding - pre_alloc.py: Pass sender_fund_refund_gas_limit to Alloc and use it for simple EOA funding transactions Usage: --sender-fund-refund-gas-limit 200000 * chore: additional fixes for execute remote funds w/ higher gas limits * fix: bump all gas limits to 200k for EIP-8037 state creation costs - sender.py: bump --sender-fund-refund-gas-limit default 21000 → 200000 - pre_alloc.py: bump funding_gas_limit default 21000 → 200000 - pre_alloc.py: use configurable gas limit for per-test refund txs - execute_recover.py: bump recovery refund gas limit 21000 → 200000 EIP-8037 charges GAS_NEW_ACCOUNT (112 × cost_per_state_byte = 131488) for transfers to new accounts, making 21000 gas insufficient for all funding, refund, and recovery transactions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Felipe Selmo <fselmo2@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… gas (ethereum#2595) Co-authored-by: spencer-tb <spencer.tb@ethereum.org>
…um#2610) Co-authored-by: spencer-tb <spencer.tb@ethereum.org>
…ode size validation (ethereum#2608) * fix(spec): charge CREATE state gas after initcode size validation Move charge_state_gas(STATE_BYTES_PER_NEW_ACCOUNT) from create()/create2() into generic_create(), after the MAX_INIT_CODE_SIZE check. Previously, state gas was charged before the initcode size check, so a CREATE with oversized initcode would persist state_gas_used equal to the account creation state gas cost (STATE_BYTES_PER_NEW_ACCOUNT * cost_per_state_byte) even though no account was ever created and no state was touched. Reported by @AskDragan (reth): ethereum#2578 (comment) * fix(spec): check static context before gas in CREATE/CREATE2 Move the is_static check from generic_create() into create() and create2(), before stack pops and charge_gas(). This is consistent with SSTORE, CALL, and SELFDESTRUCT which all check static context before any gas charging.
… the correct token formula
…evel failure (ethereum#2689) Co-authored-by: spencer-tb <spencer.tb@ethereum.org>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2901 +/- ##
===================================================
- Coverage 90.44% 88.91% -1.53%
===================================================
Files 535 496 -39
Lines 32439 29758 -2681
Branches 3012 2723 -289
===================================================
- Hits 29338 26459 -2879
- Misses 2573 2814 +241
+ Partials 528 485 -43
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:
|
| @dataclass | ||
| class IntrinsicGasCost: | ||
| """ | ||
| Intrinsic gas costs for a transaction, split by gas type. | ||
|
|
||
| `regular`: `ethereum.base_types.Uint` | ||
| Regular execution gas (calldata, base cost, access list, etc.) | ||
| `state`: `ethereum.base_types.Uint` | ||
| State growth gas (account creation, storage set, authorization). | ||
| `calldata_floor`: `ethereum.base_types.Uint` | ||
| Minimum gas cost based on calldata size per [EIP-7623]. | ||
| """ | ||
|
|
||
| regular: Uint | ||
| state: Uint | ||
| calldata_floor: Uint |
There was a problem hiding this comment.
This will need to be backported.
There was a problem hiding this comment.
Agreed, will add to back meta item in EIP tracker, as a follow up: #2040 (comment)
marioevz
left a comment
There was a problem hiding this comment.
I've reviewed packages/testing and tests/amsterdam in this review round, and left a couple of comments, thanks!
|
|
kclowes
left a comment
There was a problem hiding this comment.
Got through Osaka, Berlin, and Byzantium today. I saw a few things that need to be updated, but nothing major!
| @@ -642,6 +648,13 @@ def intrinsic_cost_for_num_accounts(account_count: int) -> int: | |||
| ], | |||
| ) | |||
| @pytest.mark.valid_from("Osaka") | |||
| # TODO[EIP-8037]: cap math here uses the combined intrinsic (regular + state) | |||
| # Ensures that the precompile call is not starved by the 63/64 rule. | ||
| precompile_gas_with_margin = precompile_gas * 64 // 63 | ||
| gas_costs = fork.gas_costs() | ||
| sstore_gas = gas_costs.STORAGE_SET * (len(modexp_expected) // 32) |
There was a problem hiding this comment.
I'm curious about this change. It looks like len(modexp_expected) // 32 = 38 // 32 = 1, so it only budgets for 1 SSTORE, when the contract actually does
2-4. I believe the test passes because extra_gas covers the shortfall, but may be missing something, or maybe that's ok, just wanted to flag :)
| Alloc, | ||
| Bytes, | ||
| Environment, | ||
| Fork, |
There was a problem hiding this comment.
Looks like this can be removed. Not used anywhere.
| SYSTEM_MAX_SSTORES_PER_CALL = 16 | ||
|
|
||
|
|
||
| class EIP8037(BaseFork): |
There was a problem hiding this comment.
Shouldn't this class also update transaction_gas_limit_cap to return None again? It feels to me that the cap is effectively removed by this EIP. Not entirely sure about the repercussions though.
There was a problem hiding this comment.
The cap still exists it just relocates to regular gas only!
Co-authored-by: Sam Wilson <57262657+SamWilsn@users.noreply.github.com>
…e pattern Co-authored-by: marioevz <11726710+marioevz@users.noreply.github.com>
Co-authored-by: marioevz <11726710+marioevz@users.noreply.github.com>
…r 8037 Co-authored-by: marioevz <11726710+marioevz@users.noreply.github.com>
… 8037 Co-authored-by: marioevz <11726710+marioevz@users.noreply.github.com>
Co-authored-by: marioevz <11726710+marioevz@users.noreply.github.com>
9a978a8 to
b95369f
Compare
Co-authored-by: marioevz <11726710+marioevz@users.noreply.github.com>
🗒️ Description
This PR merges EIP-8037 into
forks/amsterdamincluding framework, specs and all test changes.Please assign yourself below for reviewing!
Review Plan
Specs -
src/ethereum/forks/amsterdam/@SamWilsnTests batch 1 - Targeted 8037 cases,
tests/amsterdam/, 54 files @marioevzTests batch 2 - Cross fork non-static,
tests/osakatotests/frontier, 78 filesTests batch 3 - Cross fork static-ported,
tests/ported_staticaudit @leolaraTesting Framework -
packages/testing@marioevzThe test batch 2/3 reviewers should aim to check that the changes don't break what the tests are intending to do in the first place.
🔗 Related Issues or PRs
N/A.
✅ Checklist
just statictype(scope):.