feat(spec-specs, tests): EIP-8038: move SSTORE access-cost check before read - #3064
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3064 +/- ##
================================================
Coverage 93.30% 93.30%
================================================
Files 624 624
Lines 36986 36988 +2
Branches 3383 3384 +1
================================================
+ Hits 34508 34510 +2
Misses 1695 1695
Partials 783 783
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@nerolation For review |
35a757b to
b270a21
Compare
bd52730 to
ada583d
Compare
spencer-tb
added
A-spec-specs
|
Related to the spec update ethereum/EIPs#11854 |
bd2244f to
1768ac2
Compare
… read Post EIP-8038 the cold storage access cost (3000) exceeds the EIP-2200 call stipend (2300), so clearing the stipend sentry no longer guarantees the access cost is affordable. The implicit storage read in SSTORE records the slot into the EIP-7928 Block Access List, and that record survives frame rollback. With gas_left in (stipend, access_cost), the old ordering recorded a phantom read for an SSTORE that then ran out of gas on the access cost itself. Compute the access cost first and check it (alongside the stipend sentry) before the read, warming the slot only once the access is affordable -- mirroring the CALL opcode and matching SLOAD, which already charges access before reading. Pivot test_bal_sstore_and_oog OOG boundaries on the access cost instead of the stipend, and refresh the test_sstore_stipend_check_excludes_reservoir docstring.
1768ac2 to
4d0bec2
Compare
4d0bec2 to
41dfeb4
Compare
jochem-brouwer
left a comment
jochem-brouwer
left a comment
There was a problem hiding this comment.
Hi @gurukamath, this looks correct 😄 👍 , could you as sanity check verify if the changes rolled back in instructions/storage.py fail the newly introduced tests (the new tx gas limits) and if you re-add them they now pass? (Should fail at most 2 tests, I think it fails only one test, fail ABOVE_STIPEND_BELOW_ACCESS, pass AT_ACCESS_COST, if you roll back the instructions.py changes?)
| OutOfGasAt.EIP_2200_STIPEND, | ||
| OutOfGasAt.EIP_2200_STIPEND_PLUS_1, | ||
| OutOfGasAt.ABOVE_STIPEND_BELOW_ACCESS, | ||
| OutOfGasAt.AT_ACCESS_COST, |
There was a problem hiding this comment.
| OutOfGasAt.AT_ACCESS_COST, | |
| OutOfGasAt.ABOVE_STIPEND_AT_ACCESS, |
Nit: better name, more descriptive?
There was a problem hiding this comment.
Good point. Don't think the name fully described the scenario. Went with ACCESS_COVERED_OOG_ON_WRITE to be more explicit about it.
I see there are two failure cases as expected |
Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com>
spencer-tb
left a comment
spencer-tb
left a comment
There was a problem hiding this comment.
LGTM! @jochem-brouwer for final approve here :)
jochem-brouwer
left a comment
jochem-brouwer
left a comment
There was a problem hiding this comment.
Thanks for the clarification, LGTM!
marioevz
changed the title
5 participants
🗒️ Description
Post EIP-8038, the cold storage access cost (
COLD_STORAGE_ACCESS= 3000)exceeds the EIP-2200 call stipend (2300). SSTORE's EIP-2200 stipend sentry
(
gas_left > CALL_STIPEND) therefore no longer guarantees that the accesscost is affordable before SSTORE performs its implicit storage read.
This PR moves up the access check. More context here on discord
🔗 Related Issues or PRs
✅ Checklist
just statictype(scope):.