feat(tests): expand create collision matrix and add balance-only cases - #3425
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3425 +/- ##
================================================
Coverage 93.77% 93.77%
================================================
Files 624 624
Lines 36905 36905
Branches 3326 3326
================================================
Hits 34606 34606
Misses 1597 1597
Partials 702 702
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:
|
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.
ff2a572 to
617e037
Compare
spencer-tb
left a comment
There was a problem hiding this comment.
LGTM from my end! Would like an approve and merge from @jochem-brouwer
jochem-brouwer
left a comment
There was a problem hiding this comment.
Hi @danceratopz I checked the tests and it looks correct. However, the docs are not correct.
Could you edit the docs? Let me know if I should address this, then I will change and push a commit 😄 👍
| """ | ||
| Test that a contract creation opcode succeeds when the target | ||
| address has only a balance: an account with zero nonce, no code and | ||
| no storage is not a collision (EIP-684). |
There was a problem hiding this comment.
This is not correct, EIP-684 only verifies: no code and nonce is zero. The storage check is added in EIP-7610, however, since this is DFId this is still undefined behavior (we shouldo also not test such behavior, which we are not doing here, so that's fine! 😄 )
If a contract creation is attempted due to a creation transaction, the CREATE opcode, the CREATE2 opcode, or any other reason, and the destination address already has either a nonzero nonce, or a nonzero code length, then the creation MUST throw
|
Wait, it's a double negation. We could also add "no balance" but that would be correct also. Ok I will approve it, merge if you are fine, or edit the comment if you think it should change. |
Description
Expand the create collision tests to the full parameter matrix of nonce, code, storage and balance for all cases where EIP-684 defines an abort. The new no-storage cells catch a client that incorrectly aborts on storage instead of code or nonce. Cells with zero nonce and empty code are excluded at param generation: with non-empty storage the behavior is undefined in protocol (EIP-7610 was declined for Glamsterdam), with empty storage the account is deployable.
Also add balance-only success tests for the deployable case, for creation transactions and for the CREATE and CREATE2 opcodes: an account with only a balance is not a collision, creation must succeed and the balance is kept.
Filled locally at Frontier, Paris, Cancun and Amsterdam, all pass, re-verified after rebasing onto
forks/amsterdamonce #3417 merged.Related Issues or PRs
Implements the coverage expansion requested in the review of #3417 (comment 1, comment 2).
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.Cute Animal Picture