chore(tests): remove EIP-7610 create collision tests - #3417
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3417 +/- ##
===================================================
+ Coverage 93.53% 93.77% +0.23%
===================================================
Files 624 624
Lines 37074 36905 -169
Branches 3394 3326 -68
===================================================
- Hits 34679 34606 -73
+ Misses 1645 1597 -48
+ Partials 750 702 -48
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:
|
e86375d to
9944afe
Compare
spencer-tb
changed the title
9944afe to
13bd1f4
Compare
LouisTsai-Csie
left a comment
LouisTsai-Csie
left a comment
There was a problem hiding this comment.
Is there anything we need to remove from the spec side?
jochem-brouwer
left a comment
jochem-brouwer
left a comment
There was a problem hiding this comment.
Some small comments regarding a test where I think we should directly expand coverage.
Address review feedback from jochem-brouwer on ethereum#3417. Parametrize the collision account over the full product of nonce, code, storage and balance where EIP-684 defines an abort, so a client that incorrectly aborts on storage is caught. Cells with zero nonce and empty code are excluded: with storage the behavior is undefined in protocol since EIP-7610 was declined, without storage the account is deployable. Add balance-only success tests for the deployable case, for creation transactions and for the CREATE and CREATE2 opcodes.
danceratopz
left a comment
danceratopz
left a comment
There was a problem hiding this comment.
Thanks @spencer-tb, LGTM, I'd just suggest updating the BAL test_cases.md in the scope of this PR. Claude suggested two other state-reference corrections. I added them up front here:
I was reviewing when @jochem-brouwer's comment came in. I think this could be follow-up to get this in quickly, although it would make sense to get them in the next release:
|
@jochem-brouwer can you revert the requested changes, so we merge this and then switch to @danceratopz's follow up in #3425? |
The answer is yes! @gurukamath could you review d7baf6b? |
My comments are followed-up in #3425
Address review feedback from jochem-brouwer on ethereum#3417. Parametrize the collision account over the full product of nonce, code, storage and balance where EIP-684 defines an abort, so a client that incorrectly aborts on storage is caught. Cells with zero nonce and empty code are excluded: with storage the behavior is undefined in protocol since EIP-7610 was declined, without storage the account is deployable. Add balance-only success tests for the deployable case, for creation transactions and for the CREATE and CREATE2 opcodes.
4 participants
Description
Remove the EIP-7610 storage collision tests so their fixtures drop out of the next release. EIP-7610 is declined for inclusion in Glamsterdam (EIP-7773): no mainnet account has non-empty storage with empty code and nonce, and clients are dropping the storage collision check.
Also removes the two ported static tests that depend on the check,
test_revert_in_create_in_init_paris.pyandtest_failed_create_reverts_deletion_paris.py. These are the CREATE variants of the same scenario and only exist as their Paris (EIP-7610) versions.Coverage that does not depend on EIP-7610 is kept. The EIP-684 code and nonce collision cases move to
tests/frontier/create/test_create_collision.py, andtest_collision_selfdestruct.pymoves to the EIP-6780 directory since its collision target has code and nonce. Also rewords two stale EIP-7610 mentions in the EIP-7708 and EIP-8038 tests, which cover EIP-684 collisions.Related Issues or PRs
Related to #2819 and #2957.
Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.