-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix: Fill storage_refunds
/ pubdata_costs
data
#2431
Conversation
storage_refunds
/ pubdata_costs
datastorage_refunds
/ pubdata_costs
data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relevant branch in the vm2
repo is this one; probably needs to be merged before this PR.
I have used this patchset to fix the diff --git a/core/lib/multivm/src/versions/vm_fast/tests/require_eip712.rs b/core/lib/multivm/src/versions/vm_fast/tests/require_eip712.rs
index 97f929927..fce3404f0 100644
--- a/core/lib/multivm/src/versions/vm_fast/tests/require_eip712.rs
+++ b/core/lib/multivm/src/versions/vm_fast/tests/require_eip712.rs
@@ -23,7 +23,13 @@ impl VmTester {
AccountTreeId::new(L2_BASE_TOKEN_ADDRESS),
&address,
);
- h256_to_u256(self.vm.world.storage.read_value(&key))
+ self.vm
+ .inner
+ .world_diff
+ .get_storage_state()
+ .get(&(L2_BASE_TOKEN_ADDRESS, h256_to_u256(*key.key())))
+ .cloned()
+ .unwrap_or(h256_to_u256(self.vm.world.storage.read_value(&key)))
}
}
@@ -134,7 +140,8 @@ async fn test_require_eip712() {
Default::default(),
);
- let transaction_request: TransactionRequest = tx_712.into();
+ let mut transaction_request: TransactionRequest = tx_712.into();
+ transaction_request.chain_id = Some(chain_id.into());
let domain = Eip712Domain::new(L2ChainId::from(chain_id));
let signature = private_account
I am pasting this here since I have no better place to do it atm I suppose |
// Create a snapshot to roll back the bootloader call frame on halt. | ||
self.make_snapshot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for removing this snapshot and the rollbacks further in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've introduced this additional snapshot in my previous PR, and it looks redundant now that I understand VM function better 🙃 The only thing this snapshot did was zeroing the VM outputs before a rollback of the original snapshot (which would occur later), and this is easy to reproduce without taking a snapshot, which I did in this PR. Another option would be to ignore the VM outputs on the caller side, but it looks more complex to implement / maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the actual snapshotting done in the sequencer now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was performed in the sequencer previously as well, but it's now performed entirely there, yes.
@joonazan Are VM performance changes OK / expected, or did I do something wrong with the integration? |
I now understand why the benchmarks are faster. It is because the snapshotting is removed. |
8e26e75
to
bb2c663
Compare
Detected VM performance changes
Changes in number of opcodes executed indicate that the gas price of the benchmark has changed, which causes it run out of gas at a different time. Or that it is behaving completely differently. |
What ❔
These fields don't seem to be used by ENs, but are expected to be filled during the main state keeper execution cycle.