feat(specs,tests): EIP-8037 state gas ordering and clarifications#2526
Conversation
f0813e0 to
d871d66
Compare
|
Tagging @fselmo re-STEEL call. This will be added to |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## eips/amsterdam/eip-8037 #2526 +/- ##
==========================================================
Coverage ? 83.63%
==========================================================
Files ? 642
Lines ? 39708
Branches ? 4058
==========================================================
Hits ? 33210
Misses ? 5798
Partials ? 700
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:
|
3a5d42e to
b557be3
Compare
fselmo
left a comment
There was a problem hiding this comment.
Found one bug and one possible improvement, narrowing the calldata floor gap. I put them as commits here but feel free to drop them if I missed something, add on to them, etc. It just felt easier to sanity check the bug with a test, pairing with Claude, see that it failed and then the change itself was a one-liner... so I feel like we can use this test 👀.
Also a nit on aligning comments / logic design across similar opcodes for consistency.
Lmk your thoughts :)
| value, | ||
| gas, | ||
| Uint(evm.gas_left), | ||
| extend_memory.cost, |
There was a problem hiding this comment.
We charge the mem cost now on line 456 so we should zero this out, as we did with extra_gas here, right?
There was a problem hiding this comment.
Follow-up comment got lost here I think. I put this as two separate commits here. The first is a test I worked with Claude on to prove this was a bug and debug it - this did indeed fail to fill. The next commit was the fix to zero this out and it fills after this change. Lmk your thoughts.
There was a problem hiding this comment.
Thx so much for this!
| gas limit cap, the transaction must be rejected at validation — | ||
| even though the regular intrinsic gas may be within the cap. | ||
|
|
||
| at_cap: calldata floor exactly equals the cap — transaction accepted. |
There was a problem hiding this comment.
Hmm this is sort of a nit but we need to be more clear here regardless so also not really a nit. This isn't exactly at the cap because it's not divisible enough to be exact. I asked Claude to see what the tightest gap here can be and I got this:
The tightest gaps achievable:
┌──────────────────────────────┬───────────┬────────────┬─────┐
│ Approach │ Tokens │ Floor │ Gap │
├──────────────────────────────┼───────────┼────────────┼─────┤
│ All nonzero (current test) │ 1,675,620 │ 16,777,200 │ -16 │
├──────────────────────────────┼───────────┼────────────┼─────┤
│ Mixed (nonzero + zero bytes) │ 1,675,621 │ 16,777,210 │ -6 │
├──────────────────────────────┼───────────┼────────────┼─────┤
│ Next token up │ 1,675,622 │ 16,777,220 │ +4 │
└──────────────────────────────┴───────────┴────────────┴─────┘
So the current all-nonzero approach gives a gap of 16. Using mixed bytes could tighten it to 6. But exact boundary is unreachable — this is the tightest we can get with only nonzero bytes.
The docstring should acknowledge this: the at_cap case is "largest calldata floor that fits within the cap" and exceeds_cap is "smallest calldata floor that exceeds the cap," not "exactly equals" and
"exceeds by 1."
It would be nice to tighten this gap to be as close as possible to the cap and still pass.
There was a problem hiding this comment.
Actually this ended up being trivial to tighten up so I'm putting it in a commit here. Feel free to drop any of these commits too or build on top. It's just a bit easier to do for these smaller changes.
|
|
||
| transfer_gas_cost = Uint(0) if value == 0 else GAS_CALL_VALUE | ||
|
|
||
| # check static gas before state access |
There was a problem hiding this comment.
One thing I noticed is that these messages are inconsistent atm across the opcodes that introduced this concept with BALs. I think it would be good not only to align on the design pattern for these similar opcodes, but also this is maybe a chance to make things more clear now that we have 2D gas.
A Claude sweep analysis on this thought:
- Current — inconsistent wording (selfdestruct), no mention of "regular" gas:
- call() L428:
# check static gas before state access - callcode() L548:
# check static gas before state access - selfdestruct() L646:
# check access gas cost before state access - delegatecall() L725:
# check static gas before state access - staticcall() L822:
# check static gas before state access
- call() L428:
Maybe all these comments can align on "regular" gas wording? I'm not sure what we're calling this... "regular" seems weird so maybe we can say something like:
# ensure enough non-state gas before accessing state
Still not sure if that's super clear... but this is just a nit to make specs a bit more readable with 2D gas.
Another catch that Claude caught that was unrelated to my ask but fair enough since it aligns with consistent messaging across opcodes:
Current — exists in sstore (L148-150) and selfdestruct (L659-661):
# Charge regular gas before state gas so that a regular-gas OOG
# does not consume state gas that would inflate the parent's
# reservoir on frame failure.
Missing from — call() (L456), create() (L189), create2() (L241).
This is for the charge_gas() call in each opcode.
Thoughts on this? 👀. Simple one-liner comments but helps to align the design on the specs side since they are bits of logic that are trying to achieve the same goal.
There was a problem hiding this comment.
Very fair! We should add these. I think the whole 8037 EIP needs a full review before we merge into forks/amsterdam in the future, there will be more things like this I'm sure! :D
There was a problem hiding this comment.
Yeah makes sense to get the logic working first. Can skip this for this PR, take or leave :)
There was a problem hiding this comment.
Going to approve here @spencer-tb. Since I added my proposed fixes, let me know if you agree or not, etc. If you want another review with any more changes / dropped commits, feel free to re-request and I'll try to prioritize this and quickly get another review in but lgtm!
| code_hash = get_account(tx_state, code_address).code_hash | ||
| code = get_code(tx_state, code_hash) | ||
|
|
||
| charge_gas(evm, extra_gas + extend_memory.cost) |
There was a problem hiding this comment.
I think there is an issue with the design to charge_gas multiple times. There's a bit of context here from implementing EIP-7928. Some of this pertains to EVM trace specification where we should only charge it once. Perhaps there is more nuance there too.
I believe we can account for this gas, and pass it to calculate_message_call_gas, without calling charge_gas more than once. We can use check_gas and we can do proper accounting within the logic and pass this accurate value to the calculate_message_call_gas().
Curious on your thoughts here.
cc: @gurukamath
…L, SELFDESTRUCT When an opcode charges both regular and state gas, regular gas must be charged first. If regular gas OOGs, state gas is not consumed. This prevents the parent's reservoir from being inflated on frame failure when state gas spills into gas_left. Fixes SSTORE, CALL, and SELFDESTRUCT ordering to match reth (revm). CREATE, CREATE2, and code deposit already had the correct order. Adds detection tests for all opcodes that charge both gas types. Each test gives a child frame 1 gas less than needed, then probes the parent's reservoir to detect inflation from incorrectly consumed state gas.
Add tests for EIP-8037 behaviors clarified in ethereum/EIPs#11414: - SSTORE stipend check uses gas_left only, excluding the reservoir - CREATE does not double-charge GAS_NEW_ACCOUNT (regular and state gas are independent charges) - Calldata floor exceeding TX_MAX_GAS_LIMIT rejects the transaction
Fix line length violations in forks.py and test files, remove unused import and unused variable.
- Specs were not zeroing out mem expansion cost when calculating the call message gas even though this gas was already charged.
0bebc8b to
006c51e
Compare
Add a TODO noting that separate charge_gas + charge_state_gas calls may produce duplicate EVM trace entries. Flagged by fselmo in ethereum#2526.
…hereum#2526) Co-authored-by: fselmo <fselmo2@gmail.com>
…hereum#2526) Co-authored-by: fselmo <fselmo2@gmail.com>
…hereum#2526) Co-authored-by: fselmo <fselmo2@gmail.com>
…hereum#2526) Co-authored-by: fselmo <fselmo2@gmail.com>
…hereum#2526) Co-authored-by: fselmo <fselmo2@gmail.com>
🗒️ Description
Note: these changes are already added to
devnets/bal/3, and this PR is to highlight key changes to the EIP.Fix EIP-8037 state gas charging order and add detection/clarification tests. Includes
forks/amsterdamrebase changes.Spec fix: When an opcode charges both regular gas and state gas, regular gas must be charged first, EIPs PR: ethereum/EIPs#11421. If regular gas OOGs, state gas is not consumed. The previous ordering (state before regular) allowed state gas spilled into
gas_leftto be lifted into the parent's reservoir viaincorporate_child_on_error, inflating it and changing the transaction's effective gas consumption.This was identified by @rakita (reth/revm) who found a failing test fixture on the
tests-bal@v5.3.0tag. The revm implementation already charges regular gas before state gas across all opcodes.Opcodes fixed (state gas moved after regular gas):
SSTORE-- storage set (zero to non-zero)CALL-- value transfer to non-existent accountSELFDESTRUCT-- dead beneficiary with non-zero balanceCREATE,CREATE2, and code deposit already had the correct order.CALL memory expansion double-charge fix:
CALLwith value transfer to a new account was double-charging memory expansion costs — once in regular gas and again in state gas. Fixed to only charge memory costs in regular gas.1D to 2D gas accounting fix (
test_sstore_oog_no_reservoir_inflationintest_state_gas_create.py): The originaltest_max_initcode_size_gas_metering_via_createfromtests-bal@v5.3.0used 1D gas accounting (all gas through CALL, zero reservoir). This test uses the same pattern withreservoir=0, verifying the gas accounting is correct in the fixture for bothexact_gasandshort_one_gascases.Detection tests (
test_state_gas_ordering.py): Each test gives a child frame 1 gas less than needed, then probes the parent's reservoir with SSTOREs. Clients with wrong ordering produce different storage values (not just different gas accounting), causing fixture assertion failures.Edge case tests for ethereum/EIPs#11414 (EIP-8037 text clarifications by @nerolation ):
gas_leftonly, excluding reservoirGAS_NEW_ACCOUNTEdge case test from @jwasinger (via @MariusVanDerWijden):
TX_MAX_GAS_LIMITrejects the transaction at validationFixture format divergence fix: The
blocksfixture intests/prague/eip7002_el_triggerable_withdrawals/conftest.pymutatedWithdrawalRequestContract.tx_gas_limitin-place (+=) to add EIP-8037 state gas costs. Since pytest shares parameter objects across fixture format runs, the gas cost compounded, added once forblockchain_test, then again forblockchain_test_engineon the already-modified object, producing different transactions and block hashes between formats. Fixed by deep copying the parameters before mutation. Reported by @jangko (nimbus).🔗 Related Issues or PRs
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.Cute Animal Picture