refactor(levm): implement cache rollback #2417
Open
Conversation
This file contains 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 |
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
|
EF Tests Comparison
|
crates/vm/levm/src/db/gen_db.rs
Outdated
|
|
||
| /// Gets mutable account, first checking the cache and then the database | ||
| /// (caching in the second case) | ||
| /// This isn't a method of VM because it allows us to use it during VM initialization. |
There was a problem hiding this comment.
I don't think this line comment adds much to this method documentation. Having this method on the GeneralizedDatabase makes more sense than having it on the VM
crates/vm/levm/src/db/gen_db.rs
Outdated
| pub fn get_account_mut<'a>( | ||
| &'a mut self, | ||
| address: Address, | ||
| call_frame: Option<&mut CallFrame>, |
There was a problem hiding this comment.
I think that it is not super clear here why you are passing an Option<CallFrame>.
If you want to make the cache backup optional, then maybe we need something a bit more expressive.
The best I came up with right now is to have
type CacheBackup = HashMap<Address, Option<Account>>;
pub fn get_account_mut<'a>(
&'a mut self,
address: Address,
cache_backup: Option<&mut CacheBackup>);
// The call would be
self.db.get_account_mut(address, Some(call_frame.previous_cache_state))?;
self.db.get_account_mut(address, None)?;There was a problem hiding this comment.
Forgot to say, I changed it here
tomip01
reviewed
Apr 11, 2025
crates/vm/levm/src/vm.rs
Outdated
| @@ -571,4 +555,17 @@ impl<'a> VM<'a> { | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| /// Restores the cache state to the state before changes made during a callframe. | |||
| fn restore_cache_state(&mut self, call_frame: &CallFrame) { | |||
There was a problem hiding this comment.
Here maybe we can just pass the cache_backup instead of the whole call frame. What do you think?
There was a problem hiding this comment.
You are right! Good catch!
tomip01
approved these changes
Apr 11, 2025
crates/vm/levm/src/vm.rs
Outdated
Comment on lines
402
to
403
| // clear callframe backup because prepare_execution succeeded | ||
| initial_call_frame.cache_backup = HashMap::new(); |
There was a problem hiding this comment.
I understand this but I think maybe a clearer comment on why we do this would be better
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
Description
previous_cache_state, that stores the pre-write state of the account that the callframe is trying to mutate. If the context reverts that state is restored in the cache. Otherwise, the parent call frame inherits the changes of the child of the accounts that only the child has modified, so that if the parent callframe reverts it can revert what the child did.GeneralizedDatabase. Now they are methods.Some other changes that it makes:
finalize_execution. Specifically the reversion of value transfer and removal of check for coinbase transfer of gas fee.Closes #issue_number