EIP3540 run EOF code correctly #2368

jochem-brouwer · 2022-10-18T11:16:47Z

After the new updates in ethereum/tests#847, we have many failing tests, and one of the causes is that we do not run the actual EOF code, but we run the entire container (so EOF code immediately runs into 0xEF, which is the invalid opcode).

Also thanks to @pk910 who discovered this on Shangdong testnet :)

To run:

cd ./packages/ethereum-tests
gh pr checkout 847
cd ../vm

Run tests;

Current:

npm run test:state -- --fork=London+3540+3670
# tests 293
# pass  171
# fail  122

npm run test:blockchain -- --fork=London+3540+3670
# tests 2426
# pass  1588
# fail  838

codecov · 2022-10-18T11:32:02Z

Codecov Report

Merging #2368 (b317ddd) into master (f9ff861) will decrease coverage by 0.33%.
The diff coverage is n/a.

Additional details and impacted files

Flag	Coverage Δ
block	`?`
blockchain	`90.21% <ø> (ø)`
common	`98.13% <ø> (ø)`
devp2p	`91.63% <ø> (-0.14%)`	⬇️
ethash	`∅ <ø> (∅)`
rlp	`∅ <ø> (∅)`
statemanager	`89.46% <ø> (ø)`
trie	`90.66% <ø> (-0.35%)`	⬇️
tx	`?`
util	`?`
vm	`85.30% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

acolytec3 · 2022-10-18T19:16:11Z

packages/evm/src/evm.ts

@@ -858,6 +858,9 @@ export class EVM implements EVMInterface {
      } else {
        message.code = await this.eei.getContractCode(message.codeAddress)
        message.isCompiled = false
+        if (this._common.isActivatedEIP(3540)) {
+          message.code = getEOFCode(message.code)


Do we really need this? runInterpreter checks the code passed in as part of the message for the EOF header and sets the PC to the correct position based on the sections header.

I see that more tests fail when I comment it out but from how we handle code in the interpreter, it seems like they shouldn't be failing. I looked back at the original PR and the idea was that we handled all the bits about defining the code section inside interpreter.run so this theoretically should be duplicative.

jochem-brouwer · 2022-10-20T17:28:29Z

Now have 4 tests failing. This is due to CODECOPY and likely also CODESIZE. If you run these if you CALL a contract, then interpreter.code is not the entire container, it is the runtime code. At this point I do not know how to cleanly fix this though. It seems that we are inconsistent in situations, because if you try to create a contract (so, the to field is undefined) then the .code is actually set to the entire container...

acolytec3 · 2022-10-21T16:19:45Z

Now have 4 tests failing. This is due to CODECOPY and likely also CODESIZE. If you run these if you CALL a contract, then interpreter.code is not the entire container, it is the runtime code. At this point I do not know how to cleanly fix this though. It seems that we are inconsistent in situations, because if you try to create a contract (so, the to field is undefined) then the .code is actually set to the entire container...

See my latest commit. I think this relatively cleanly solves it by adding a new codeContainer field that's only populated when you call _loadCode. The codeContainer only gets referenced by CODESIZE and CODECOPY. EXTCODESIZE and EXTCODECOPY both reference the entire code pulled from the state manager so should work as currently coded with no changes.

holgerd77 · 2022-10-21T16:25:37Z

Cool!

Why the hell didn't we pack this in the hotfix releases? 😋

acolytec3 · 2022-10-21T16:36:40Z

Never too late 🙃

acolytec3 · 2022-10-22T01:25:02Z

I added a test for the new getEOFCode function so hopefully that will help with the code coverage. Are we ready to merge this? It'd be awesome to restart the Shandong network early next week with EIP-3540 working correctly.

jochem-brouwer · 2022-10-22T03:52:01Z

I think in general we also want another client (i.e. Geth) to run alongside our Shangdong testnet (we should run this ourselves) so we can quickly figure out if there is a consensus bug.

@acolytec3 can you confirm that the current EIP3540 tests pass?

acolytec3 · 2022-10-22T12:14:28Z

I need to run the state tests but all of the Blockchain tests pass except for that over where I raised the question about the container length which appears to be invalid. I'll check the state tests today and see if there are any remaining failures.

acolytec3 · 2022-10-22T12:15:23Z

I think in general we also want another client (i.e. Geth) to run alongside our Shangdong testnet (we should run this ourselves) so we can quickly figure out if there is a consensus bug.

I believe Nethermind is planning to join and they have implemented EIP3540/3670 as well. I'm not sure if Geth has it integrated in their master branch yet

acolytec3

LGTM!

holgerd77 · 2022-10-24T15:23:59Z

I need to run the state tests but all of the Blockchain tests pass except for that over where I raised the question about the container length which appears to be invalid.

@acolytec3 has this been confirmed (e.g. by the test authors on ethereum/tests or similar) that this test is invalid? Can you provide a reference here?

jochem-brouwer · 2022-10-24T15:30:37Z

gitpoap-bot · 2022-10-24T15:54:52Z

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 EthereumJS Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

holgerd77 · 2022-10-24T15:56:36Z

I wonder if we should also prepare a quick EVM-only bugfix release on this? Could do this tomorrow morning.

jochem-brouwer added PR state: WIP package: evm labels Oct 18, 2022

evm: run EOF code correctly [no ci]

e914ac5

jochem-brouwer force-pushed the eip3540-fixes branch from 9ef66ca to e914ac5 Compare October 18, 2022 11:17

vm/tests: print failing tests

05ff457

acolytec3 reviewed Oct 18, 2022

View reviewed changes

holgerd77 mentioned this pull request Oct 20, 2022

Shandong Management Meta Issue #2375

Closed

jochem-brouwer force-pushed the eip3540-fixes branch from 643ed03 to 438b9fa Compare October 20, 2022 17:13

evm: update push for EIP3540

06206b5

jochem-brouwer force-pushed the eip3540-fixes branch from 438b9fa to 06206b5 Compare October 20, 2022 17:20

acolytec3 added 2 commits October 21, 2022 11:00

Use container code for CODESIZE/CODECOPY

7d76df9

Merge remote-tracking branch 'origin/master' into eip3540-fixes

96f5105

acolytec3 marked this pull request as ready for review October 22, 2022 01:09

Add test for getEOFCode

a430106

acolytec3 added PR state: needs review and removed PR state: WIP labels Oct 22, 2022

Merge branch 'master' into eip3540-fixes

b6473b4

acolytec3 approved these changes Oct 24, 2022

View reviewed changes

Merge branch 'master' into eip3540-fixes

b317ddd

holgerd77 merged commit e65e00f into master Oct 24, 2022

holgerd77 deleted the eip3540-fixes branch October 24, 2022 15:55

holgerd77 mentioned this pull request Oct 25, 2022

New EVM (EIP-3540) and Blockchain (ensure Lock) Releases #2380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP3540 run EOF code correctly #2368

EIP3540 run EOF code correctly #2368

jochem-brouwer commented Oct 18, 2022

codecov bot commented Oct 18, 2022 •

edited

Loading

acolytec3 Oct 18, 2022

acolytec3 Oct 19, 2022

jochem-brouwer commented Oct 20, 2022

acolytec3 commented Oct 21, 2022

holgerd77 commented Oct 21, 2022

acolytec3 commented Oct 21, 2022

acolytec3 commented Oct 22, 2022

jochem-brouwer commented Oct 22, 2022

acolytec3 commented Oct 22, 2022

acolytec3 commented Oct 22, 2022

acolytec3 left a comment

holgerd77 commented Oct 24, 2022

jochem-brouwer commented Oct 24, 2022

gitpoap-bot bot commented Oct 24, 2022

holgerd77 commented Oct 24, 2022

EIP3540 run EOF code correctly #2368

EIP3540 run EOF code correctly #2368

Conversation

jochem-brouwer commented Oct 18, 2022

codecov bot commented Oct 18, 2022 • edited Loading

Codecov Report

acolytec3 Oct 18, 2022

Choose a reason for hiding this comment

acolytec3 Oct 19, 2022

Choose a reason for hiding this comment

jochem-brouwer commented Oct 20, 2022

acolytec3 commented Oct 21, 2022

holgerd77 commented Oct 21, 2022

acolytec3 commented Oct 21, 2022

acolytec3 commented Oct 22, 2022

jochem-brouwer commented Oct 22, 2022

acolytec3 commented Oct 22, 2022

acolytec3 commented Oct 22, 2022

acolytec3 left a comment

Choose a reason for hiding this comment

holgerd77 commented Oct 24, 2022

jochem-brouwer commented Oct 24, 2022

gitpoap-bot bot commented Oct 24, 2022

holgerd77 commented Oct 24, 2022

codecov bot commented Oct 18, 2022 •

edited

Loading