Zksync era integration tests #208
Merged
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
Fixes decommit opcode, which was not account for if the code had already been decommitted
Adds mandated gass, handles decommit cost err, and fixes the params of mimic call
4 tasks
fkrause98
requested changes
Aug 20, 2024
Closed
Oppen
reviewed
Aug 22, 2024
| @@ -50,6 +51,23 @@ pub struct Execution { | |||
| pub use_hooks: bool, | |||
There was a problem hiding this comment.
If you extract the program, then the ExecutionSnapshot becomes just a clone of this. Maybe we could move the program out and turn ExecutionSnapshot in a simple wrapper?
There was a problem hiding this comment.
Yes I thought of that as well, but I didn't want to change the vm initial struct tbh. If you consider this change to be worth it, I'll go ahead and make the appropriate changes. I would prefer to leave it for a later general code refactor thou, but you call the shots!! 😉.
src/state.rs
Outdated
Comment on lines
309
to
310
| pub struct FullStateSnapshot { | ||
| pub internal_snapshot: StateSnapshot, |
There was a problem hiding this comment.
FullStateSnapshot and StateSnapshot are named in a confusing way IMO, maybe renaming StateSnapshot to InternalStateSnapshot is a bit better.
There was a problem hiding this comment.
I agree that names can be a bit confusing, I've added some new comments, and renamed FullStateSnapshot to ExternalStateSnapshot. Tell me if you think that is clearer 😁.
fkrause98
requested changes
Aug 26, 2024
| self.decommitted_hashes.map.insert(hash); | ||
| self.storage.borrow_mut().decommit(hash) | ||
| pub fn decommit(&mut self, hash: U256) -> (Option<Vec<U256>>, bool) { | ||
| let was_decommitted = !self.decommitted_hashes.map.insert(hash); |
There was a problem hiding this comment.
What we're doing here, Exactly? A bit confused since we're mixing Option + Bool. Let's discus this a bit.
There was a problem hiding this comment.
When decommiting, we want to know whether the hash has already been decommited or not to calculate refunds and decommit costs.
There was a problem hiding this comment.
Let's add a comment clarifying this then.
|
I’ve removed the cache for the initial storage to reduce confusion. We can consider re-adding it later if necessary. |
fkrause98
approved these changes
Aug 26, 2024
|
Ergs comparison results: |
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.
This pr introduces several changes and fixes to the vm:
Contex(IncrementTxNumber)opcode: it was not clearing the transient storage.decommitopcode: it was not accounting if the contract had already been decommitted.farcallopcode: adds mandated gas, handles decommit cost err, and fixes the params of mimic callfar_callswe were not supposed to add theevm_interpreterstipend gas to the new frame.All of this changes have been made to finish the integration with the bootloader. See here.