Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evm refactor (rebased on master a4c379a, 2022-06-13) #1955

Merged
merged 50 commits into from
Jun 15, 2022

Conversation

holgerd77
Copy link
Member

Rebased version and potential replacement for #1950

Rebase was a bit more hairy than expected 😋. 17 tests failing (VM -> API), most seem to be easy fixes (some base object not present, some EVM result not available "as is".

Nevertheless did not directly force-pushed on the original branch, but I would assume that it's worth to take this over here, fix the tests and then continue from here.

@holgerd77 holgerd77 added PR state: WIP package: vm type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. target: master Work to be done towards master branch labels Jun 13, 2022
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #1955 (bfad6ed) into master (aff80c4) will increase coverage by 3.67%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 86.15% <ø> (ø)
blockchain 81.56% <ø> (ø)
client ?
common 95.01% <ø> (?)
devp2p 82.64% <ø> (?)
ethash 90.81% <ø> (ø)
rlp ?
statemanager 84.55% <ø> (?)
trie 79.97% <ø> (-0.13%) ⬇️
tx 92.17% <ø> (ø)
util 87.03% <ø> (ø)
vm ?

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

@holgerd77 holgerd77 force-pushed the evm-refactor-rebased-on-master-2 branch from db25167 to 8772284 Compare June 15, 2022 10:30
@holgerd77
Copy link
Member Author

Rebased this after a local test rebase, also did a backup of the branch state before the rebase.

Will push soon afterwards with a VM api test fix.

@holgerd77
Copy link
Member Author

Will leave some generic TODOs here, absolutely don't need to be answered right now.

  • We shouldn't just double on the supported EIPs in EVM and VM and instead have a closer look which EIP belongs to what entity (my guess would be that the very most EIPs just only belong to EVM)
  • Blockchain and optimally Block dependencies should be removed from EVM
  • I am not really sure if I am getting for 100% why Interpreter is encapsulating so much EEI/state accessing logic (again?) but it looks clean and I guess I might be ok with it 😋

Ok, I will actually merge this in here so that we can proceed.

This still need an in-depth after-merge review, but then we can go on and e.g. do the package separation. Review work can be done in separate PRs.

Uff. No way back anyhow. 😜 😅 🥳

Copy link
Member Author

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Still needs an in-depth post-merge review.

Will merge by admin-merge for procedural reasons.

@holgerd77 holgerd77 merged commit d066bd3 into master Jun 15, 2022
@holgerd77 holgerd77 deleted the evm-refactor-rebased-on-master-2 branch March 2, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: vm PR state: WIP target: master Work to be done towards master branch type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants