VM, Common: Add EIP-3670 (EOF - Code Validation) #1743
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
holgerd77
changed the title
packages/common/src/eips/3670.json
Outdated
| "minimumHardfork": "london", | ||
| "requiredEIPs": [ | ||
| 3540, | ||
| 3541 |
There was a problem hiding this comment.
Please do not add 3541 here, this should be only in 3540, otherwise this will be redundant.
There was a problem hiding this comment.
Understood. What about 3860? I just noticed that EIP3670 says its requires that one as well which would notionally make this PR dependent on #1619.
There was a problem hiding this comment.
Ah, yes, then it makes sense to add here as well.
There was a problem hiding this comment.
I will finish 3860 this week, once that is merged then we should also add 3860 there (and rebase/merge master into #1719)
|
Any thoughts on what will determine the definition of done for this PR and letting it safely be merged into the EIP-3540 PR? I've got tests written that reflect the test scenarios identified in the EIP (excluding ones already testing EIP3540). The |
I was able to get some of the new tests to pass after finding a bug in the |
No (objection 🙂), I guess that makes sense. We now have this cleanly separated - which is nice - and it now makes a lot of sense to continue to work on this together. Maybe @jochem-brouwer can give this some post-merge review at some point during the week? That would be nice. |
| ) | ||
| ) { | ||
| result = { | ||
| ...result, |
There was a problem hiding this comment.
This branch is not covered by tests
| // Skip data block following push | ||
| x += opcode - 0x5f | ||
| if (x > code.length - 1) { | ||
| // Push blocks mmust not exceed end of code section |
There was a problem hiding this comment.
Nit: spelling mistake mmust -> must
There was a problem hiding this comment.
LGTM, but I think tests need some love here:
- Add a test for the uncovered codecov line (code section does not end with a terminating instruction)
- Add a test where the code section ends with
0xFE(INVALID) (this is a sanity check) - Add a check for each terminating instruction (
00,F3,FD,FF) (also a sanity check) - Tests for a "create transaction" (so a transaction from an EOA which
tofield is empty) which tries to deploy a contract using invalid code (so it does not stop with a trunacting opcode, truncatedPUSHninstruction, or it has a non-assigned instrucition besidesINVALID/0xFE) and verifies that this consumes all gas - Same as above but now the code is contained in
CREATE/CREATE2
Also, the "test cases" section of the EIP has a few test cases not mentioned here and also not covered in tests yet. (All of them seem like sanity checks to me)
|
Thanks for the review! Will work on adding the additional tests in the other PR |
* EIP3670 EOF1 code validation changes
* EIP3670 EOF1 code validation changes
* Add EIP json * Partial changes to enable EIp3540 and start code checks * Finish code validation checks and API tests * Move eof params to common * Code execution context updates * Add exception for invalid EOF format * Various fixes * Gate push handler changes behind EIP * Remove ethereum/tests tests * Clarify eof helper variable names * more naming clarifications * rename bytecode to container * check that section sizes are greater than 0 * VM, Common: Add EIP-3670 (EOF - Code Validation) (#1743) * EIP3670 EOF1 code validation changes * Fix typos, add tests, update error EOF handler * EIP3540 tests * Lint fixes * Fix tests * Lint/uncomment tests * More adjustments to EOF1 logic * compartmentalize tests * Add checks for newly deployed contract code * Fix state test runner for specified EIPs * vm: add eip3540 tests invalid eof initcode * vm: lint * vm/tests: cleanup 3540 tests * Address feedback * lint Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com> Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com>
Implements EIP-3670 on top of EIP-3540 PR
https://eips.ethereum.org/EIPS/eip-3670