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

Diff with the current geth codebase's vm.interpreter.Run function #110

Open
hsyodyssey opened this issue May 30, 2022 · 2 comments
Open

Comments

@hsyodyssey
Copy link

hsyodyssey commented May 30, 2022

Hi, just found that our EVM's interpreter's Run function is a bit different than the current codebase of Geth. In order to improve performance, they removed the code in this place that https://github.com/scroll-tech/go-ethereum/blob/zkrollup/core/vm/interpreter.go#L183-L188. (Current Geth: https://github.com/ethereum/go-ethereum/blob/master/core/vm/interpreter.go#L181-L185)

Since this part is the core function of EVM, maybe we should align with them in the next release?

FYI:

@hsyodyssey hsyodyssey changed the title diff with the latest geth codebaes of Diff with the current geth codebase's vm.interpreter.Run function May 30, 2022
@lispc
Copy link

lispc commented May 30, 2022

I am not sure whether this commit only improves performance, or change the "interface".

PR like this one ethereum#24867 changes the "interface" (or changes behavior of API) of tracing. We may have to upgrade both zkevm-circuits to handle this change AND rebase the diff into this l2geth repo at the same time later.

But if a commit only improves performance but does not change the API input/output, we don't need to apply the diff urgently.

@hsyodyssey
Copy link
Author

It looks like they moved some of the logic to the instruction level. I'm not that familiar with circuit design, so maybe you can see if the opJump and opJumpi instructions have had an impact on the current circuit design.

They have merged this pr to 1.10.14. It looks like we are using 1.10.13. which is only a minor release away😂.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants