fix: Fill storage_refunds / pubdata_costs data

[slowli]

What ❔

These fields don't seem to be used by ENs, but are expected to be filled during the main state keeper execution cycle.

[slowli]

The relevant branch in the vm2 repo is this one; probably needs to be merged before this PR.

[montekki]

I have used this patchset to fix the require_eip712 test with this diff:

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

[montekki]

What is the reason for removing this snapshot and the rollbacks further in this function?

[slowli]

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.

[joonazan]

Is the actual snapshotting done in the sequencer now?

[slowli]

It was performed in the sequencer previously as well, but it's now performed entirely there, yes.

[slowli]

@joonazan Are VM performance changes OK / expected, or did I do something wrong with the integration?

[joonazan]

@slowli Hard to say. This shouldn't improve performance, right?
Probably it is because of using merges rather than rebasing, so the start of the branch is older than the end.

Help me merge this and we will at least know that the same number of instructions are executed: #2276

[joonazan]

I now understand why the benchmarks are faster. It is because the snapshotting is removed.

[github-actions]

Detected VM performance changes

Benchmark name	change in estimated runtime	change in number of opcodes executed
slot_hash_collision	-5.6%	N/A
decode_shl_sub	-6.2%	N/A
deploy_simple_contract	-14.4%	N/A
write_and_decode	-5.3%	N/A
call_far	-4.5%	N/A
access_memory	-4.3%	N/A

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.