refactor(l2): remove references to vm internal api. #2299
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lines of code reportTotal lines added: Detailed view |
mpaulucci
force-pushed
the
fix-leak-zkvm
branch
from
24b47d5 to
69dd752
Compare
Benchmark Results ComparisonPR ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
Main ResultsBenchmark Results: Factorial
Benchmark Results: Factorial - Recursive
Benchmark Results: Fibonacci
Benchmark Results: ManyHashes
Benchmark Results: BubbleSort
Benchmark Results: ERC20 - Transfer
Benchmark Results: ERC20 - Mint
Benchmark Results: ERC20 - Approval
|
JereSalo
reviewed
Apr 1, 2025
Contributor
JereSalo
left a comment
JereSalo
left a comment
There was a problem hiding this comment.
I know it is still in draft but I reviewed all the code and left some comments :)
Contributor
There was a problem hiding this comment.
Are we 100% sure that we want to delete these tests? I know we are not currently running them. The only case in which I consider them to be useful is for debugging purposes when modifying opcodes behavior (e.g. for improving performance) when the EF Tests are not descriptive enough.
It is not a big deal though.
Collaborator
There was a problem hiding this comment.
The tests were useful to us before we integrated with the EF tests; now that we've done that there's not much point in keeping them, and considering they rely on a separate trait (Db) that's only used for them I think it's just better to delete them
| } | ||
| } | ||
|
|
||
| pub fn from_execution_db(db: ExecutionDB) -> Self { |
Contributor
There was a problem hiding this comment.
Maybe we can send EvmEngine as parameter too when we call this so that both implementations for LEVM and REVM are available.
Contributor
There was a problem hiding this comment.
Of course this would mean having access to that variable within the provers and I don't know if we want that.
Collaborator
There was a problem hiding this comment.
The idea is to transition to support just levm for the prover, as for L2 for example we won't be able to prove all we need to prove using revm anyway, and maintaining support for both vms on the prover just adds more work
crates/vm/backends/mod.rs
Outdated
|
|
||
| pub fn from_execution_db(db: ExecutionDB) -> Self { | ||
| Evm::LEVM { | ||
| store_wrapper: Arc::new(db), |
Contributor
There was a problem hiding this comment.
Should we change the name store_wrapper for something else now that the DB can be either a StoreWrapper or ExecutionDB?
Collaborator
There was a problem hiding this comment.
Check this PR first because things have changed over there #2371
| #[test] | ||
| fn test_state_file_integration( | ||
| evm_engine: EvmEngine, | ||
| _evm_engine: EvmEngine, |
Contributor
There was a problem hiding this comment.
We can use this evm_engine if when instantiating the Blockchain struct so that the test runs for both LEVM and REVM.
Instead of
let blockchain = Blockchain::default_with_store(store.clone());
We could do:
let blockchain = Blockchain::new(evm_engine, store.clone());
With the current implementation it would only run with REVM because it is the default engine.
Comment on lines
98
to
112
| fn get_chain_config(&self) -> ethrex_common::types::ChainConfig { | ||
| self.get_chain_config() | ||
| } | ||
|
|
||
| fn get_account_info_by_hash( | ||
| &self, | ||
| _block_hash: ethrex_common::types::BlockHash, | ||
| _address: CoreAddress, | ||
| ) -> Option<ethrex_common::types::AccountInfo> { | ||
| unreachable!() | ||
| } | ||
|
|
||
| fn get_account_code(&self, _code_hash: CoreH256) -> Option<bytes::Bytes> { | ||
| unreachable!() | ||
| } |
Contributor
There was a problem hiding this comment.
I personally don't like the fact that we are adding methods to the trait LevmDatabase that aren't actually used inside the VM but I understand that it makes things way more simple.
If someday we want to make a crate out of LEVM, we'll need to refactor this.
Collaborator
There was a problem hiding this comment.
I agree, but preferred to keep it for another refactor PR, since I'm not sure yet how we want to go about it (also just in case, these unreachable! calls were replaced by errors on this PR #2371)
**Motivation** This PR refactors levm introducing a `GeneralizedDatabase` (the name is not the best, suggestions are welcome), which is simply the combination of the regular store (i.e. the on-disk database) and the cache (the in memory structure we use to keep track of changes done to accounts as execution happens). More importantly, it makes levm (the `VM` struct) hold a mutable reference to said database instead of a regular owned value. This is to bring the levm api more in line to what we need it to be for ethrex; the main problem we are solving is to be able to run multiple transactions while keeping the underlying cache changes from those transactions. Once we are done with execution (because we've finished executing or building the block or group of blocks), calling `get_state_transitions` will drain the cachedb for all the account updates and return them. This change allowed, among other things, to implement for levm the previously uninplemented function `execute_block_without_clearing_state`, which is used for syncing. Additionally, this PR introduces error handling for the levm `Database` trait; previously its member functions did not return errors and we were just unwrapping on some of their implementations. **Description** <!-- A clear and concise general description of the changes this PR introduces --> <!-- Link to issues: Resolves #111, Resolves #222 --> Closes #issue_number --------- Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com> Co-authored-by: JereSalo <jeresalo17@gmail.com> Co-authored-by: Martin Paulucci <martin.c.paulucci@gmail.com> Co-authored-by: fmoletta <99273364+fmoletta@users.noreply.github.com>
jrchatruc
approved these changes
Apr 3, 2025
JereSalo
approved these changes
Apr 3, 2025
pedrobergamini
pushed a commit
to pedrobergamini/ethrex
that referenced
this pull request
Aug 24, 2025
**Motivation** L2 code was accessing internal apis from the vm crate, specifically `revm` constructs. This is attempt to replace those with the public api, so that we can easily switch between revm and levm. **Description** - Replaces references to `ethrex_vm::backends::` from the prover backends. - Moved `ExecutionDB ` to `vm/db.rs`. It is still somewhat coupled with revm but less than before. It should be totally decoupled. --------- Co-authored-by: Javier Chatruc <jrchatruc@gmail.com> Co-authored-by: Javier Rodríguez Chatruc <49622509+jrchatruc@users.noreply.github.com> Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com> Co-authored-by: JereSalo <jeresalo17@gmail.com> Co-authored-by: fmoletta <99273364+fmoletta@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
L2 code was accessing internal apis from the vm crate, specifically
revmconstructs. This is attempt to replace those with the public api, so that we can easily switch between revm and levm.Description
ethrex_vm::backends::from the prover backends.ExecutionDBtovm/db.rs. It is still somewhat coupled with revm but less than before. It should be totally decoupled.