EVM/VM refactor

[gabrocheleau]

Started from #1831

TODO

• Rebase on top of current develop
• Fix remaining failing tests
• Implement the changes in vm: refactor Message and TxContext #1819
• Unify executeMessage and runCall (see EVM/VM Refactor #1831 (comment))

For another PR

• Extract the EEI
• Extract the refactored parts into separate packages (VmExecution, VmContext, ExternalEnvironment, etc.?)

[gabrocheleau]

@holgerd77 , I think this is somewhat close to being finished, but I just ran into a bit of a blocker. The blocker is caused by the conflicting changes brought by the refactoring of the EVM in your PR (where we assign a state manager to the EVM instance) and @g11tech's PR #1817 , which abstracts stateManager out and adds vmState to wrap its context in the vm.

Since VM/EVM/EEI are still highly intertwined, with the state or vmState of one being assign to others in a couple places, we now get conflicting types. My understanding is that we need the stateManager in the EVM, since we need to be accessing the database and updating the trie, whereas we do not in the VM (which then uses vmState). See for example this line: which is currently broken since we're trying to assign the evm's state: StateManager to the Interpreter's state: VmState

Happy to "ping-pong" on this, or just to discuss the desired approach before continuing the work.

[g11tech]

@holgerd77 , I think this is somewhat close to being finished, but I just ran into a bit of a blocker. The blocker is caused by the conflicting changes brought by the refactoring of the EVM in your PR (where we assign a state manager to the EVM instance) and @g11tech's PR #1817 , which abstracts stateManager out and adds vmState to wrap its context in the vm.

Since VM/EVM/EEI are still highly intertwined, with the state or vmState of one being assign to others in a couple places, we now get conflicting types. My understanding is that we need the stateManager in the EVM, since we need to be accessing the database and updating the trie, whereas we do not in the VM (which then uses vmState). See for example this line: which is currently broken since we're trying to assign the evm's state: StateManager to the Interpreter's state: VmState

Happy to "ping-pong" on this, or just to discuss the desired approach before continuing the work.

assigning vmState should work, as it proxies calls to the underlying statemanager (evm's state should also be vmState i think)

[codecov]

Codecov Report

Merging #1862 (7658494) into develop (44dac5b) will decrease coverage by 0.15%.
The diff coverage is 66.39%.

Flag	Coverage Δ
block	85.28% <ø> (ø)
blockchain	83.13% <ø> (ø)
client	77.75% <100.00%> (ø)
common	94.92% <ø> (ø)
devp2p	82.70% <ø> (-0.14%)	⬇️
ethash	90.81% <ø> (ø)
statemanager	84.24% <ø> (ø)
trie	80.14% <ø> (ø)
tx	92.09% <ø> (ø)
util	87.29% <ø> (ø)
vm	81.06% <66.25%> (-0.56%)	⬇️

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

[gabrocheleau]

@holgerd77 , I think this is somewhat close to being finished, but I just ran into a bit of a blocker. The blocker is caused by the conflicting changes brought by the refactoring of the EVM in your PR (where we assign a state manager to the EVM instance) and @g11tech's PR #1817 , which abstracts stateManager out and adds vmState to wrap its context in the vm.
Since VM/EVM/EEI are still highly intertwined, with the state or vmState of one being assign to others in a couple places, we now get conflicting types. My understanding is that we need the stateManager in the EVM, since we need to be accessing the database and updating the trie, whereas we do not in the VM (which then uses vmState). See for example this line: which is currently broken since we're trying to assign the evm's state: StateManager to the Interpreter's state: VmState
Happy to "ping-pong" on this, or just to discuss the desired approach before continuing the work.

assigning vmState should work, as it proxies calls to the underlying statemanager (evm's state should also be vmState i think)

Cool, thanks for your help with this @g11tech. So the issue I was mentioning above seems to be fixed, some of the tests are at least passing now. Next step will be going through the failing tests :)

[gabrocheleau]

Still some failing tests, but getting closer. I will not be working on this today/tomorrow, so if anyone feels like fixing a couple tests, feel free! :) Btw majority of the remaining failing vm:API tests are related to eip-3529.spec.ts.

[acolytec3]

The issue with the EIP-3529 tests is that vm._evm._refund isn't getting cleared at the end of each runCode. I put a hack in the test to verify that it works but I think there's a deeper issue in the refactor where this refund needs to be cleared when the interpreter returns its result or something. Will look at it more later today.

[gabrocheleau]

Fixed some additional tests related to EIP-1153 (transient storage). There are still 3 failing tests in eip-1153.spec.ts, all related to the expected stack differing from the actual stack.

[jochem-brouwer]

Ok, this fixes some of the problems, I should have thorougly read the comments before working on this since @acolytec3 already found the problem.

I don't like this fix since this makes the VM edit rather "inner" variables of the EVM which we want to avoid. EVM should (?) b e a black box and should handle this stuff internally. I think we can actually do that, if a message finishes, we can check the callstack depth, if the depth is 0 then we know that the "entire" message (i.e. a transaction) has finished and we can cleanup internally.

[jochem-brouwer]

CI on Chainstart:

2022-05-13T14:32:49.8619367Z 1..5882
2022-05-13T14:32:49.8619556Z # tests 5882
2022-05-13T14:32:49.8619651Z # pass  3314
2022-05-13T14:32:49.8619740Z # fail  2568

After a fresh clone, install and git submodule update --init --recursive to pull ethereum/tests, locally:

1..4456
# tests 4456
# pass  4456

# ok

Can't reproduce whatever CI is doing 🤔 (Also re-ran CI to be sure)

[holgerd77]

Super cool! 🎉 🙏 😀

If you look at the commit history, there is still this one commit 34762b6 from Emmett which sneaked in, this would need to be removed.

Also in this commit 57ef8e5 there went in a lot of changes from other work which shouldn't be there.

I guess this needs one other round of commit-cherry-picking one by one, starting with the post-Emmett-commit and then going forward. 🙂 I guess for the 57ef8e5 commit it is not possible to use this commit in any way and you need to re-create the work (copy-and-paste) manually and just taking in the changes you intend to apply on this step. Then you should be able to continue to cherry-pick the additional commits.

This might sound laborious but I guess this should be done in < 1 hour (if nothing goes wrong 😋). Sorry, but we are so far in the develop work process, we really need to be picky here since we can't risk damaging the branch in this late state of the work.

I guess then we can merge in really quickly though, really looking forward to get the main substance of this into the working base state! 🚀

[gabrocheleau]

Closing in favor of #1892