Skip to content

Commit b82e2e4

Browse files
slowlislowli
authored
fix(vm): Fix VM divergence in revert data (#3570)
## What ❔ Fixes the result tracer of the legacy VM so that it doesn't return random-ish revert data in marginal cases. ## Why ❔ The revert data should be correct. ## Checklist - [x] PR title corresponds to the body of PR (we generate changelog entries from PRs). - [x] Tests for the changes have been added / updated. - [ ] Documentation comments have been added / updated. - [x] Code has been formatted via `zkstack dev fmt` and `zkstack dev lint`.
1 parent 944059b commit b82e2e4

File tree

15 files changed

+85
-24
lines changed

15 files changed

+85
-24
lines changed

‎.github/workflows/ci-core-reusable.yml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -474,15 +474,15 @@ jobs:
474474
475475
- name: Run integration tests
476476
run: |
477-
ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --no-deps --ignore-prerequisites" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }}
477+
ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --verbose --no-deps --ignore-prerequisites" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }}
478478
479479
- name: Repeat integration tests on push to main to check for flakiness
480480
if: ${{ (github.ref == 'refs/heads/main') }}
481481
run: |
482482
for i in {1..10}; do
483483
echo "Iteration $i"
484484
mkdir -p ${{ env.INTEGRATION_TESTS_LOGS_DIR }}/$i
485-
ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --no-deps --ignore-prerequisites" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }}/$i
485+
ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --verbose --no-deps --ignore-prerequisites" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }}/$i
486486
done
487487
488488
- name: Init external nodes
@@ -527,15 +527,15 @@ jobs:
527527
528528
- name: Run integration tests en
529529
run: |
530-
ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --no-deps --ignore-prerequisites --external-node" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }}
530+
ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --verbose --no-deps --ignore-prerequisites --external-node" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }}
531531
532532
- name: Repeat integration tests en on push to main to check for flakiness
533533
if: ${{ (github.ref == 'refs/heads/main') }}
534534
run: |
535535
for i in {1..10}; do
536536
echo "Iteration $i"
537537
mkdir -p ${{ env.INTEGRATION_TESTS_LOGS_DIR }}/$i
538-
ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --no-deps --ignore-prerequisites --external-node" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }}/$i
538+
ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --verbose --no-deps --ignore-prerequisites --external-node" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }}/$i
539539
done
540540
541541
- name: Fee projection tests

‎core/lib/multivm/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ ethabi.workspace = true
3939
assert_matches.workspace = true
4040
pretty_assertions.workspace = true
4141
rand.workspace = true
42+
serde_json.workspace = true
4243
test-casing.workspace = true
4344
zksync_eth_signer.workspace = true
4445
zksync_test_contracts.workspace = true
45-
serde_json.workspace = true

‎core/lib/multivm/src/versions/shadow/tests.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -663,6 +663,11 @@ mod simple_execution {
663663
fn reusing_create2_salt() {
664664
test_reusing_create2_salt::<super::ShadowedFastVm>();
665665
}
666+
667+
#[test]
668+
fn transfer_to_self_with_low_gas_limit() {
669+
test_transfer_to_self_with_low_gas_limit::<super::ShadowedFastVm<_>>();
670+
}
666671
}
667672

668673
mod storage {

‎core/lib/multivm/src/versions/testonly/mod.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,13 @@ where
119119
vm.inspect(tracer, InspectExecutionMode::OneTx)
120120
}
121121

122+
pub(crate) fn execute_oneshot_dump<VM>(dump: VmDump) -> VmExecutionResultAndLogs
123+
where
124+
VM: VmFactory<StorageView<StorageSnapshot>>,
125+
{
126+
inspect_oneshot_dump::<VM>(dump, &mut <VM::TracerDispatcher>::default())
127+
}
128+
122129
pub(crate) fn mock_validation_params(
123130
tx: &Transaction,
124131
accessed_tokens: &[Address],

‎core/lib/multivm/src/versions/testonly/simple_execution.rs

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,13 @@ use zksync_test_contracts::{TestContract, TxType};
44
use zksync_types::{utils::deployed_address_create, Address, Execute, H256};
55

66
use super::{
7-
default_pubdata_builder, extract_deploy_events, tester::VmTesterBuilder, ContractToDeploy,
8-
TestedVm,
7+
default_pubdata_builder, execute_oneshot_dump, extract_deploy_events, load_vm_dump,
8+
tester::VmTesterBuilder, ContractToDeploy, TestedVm,
9+
};
10+
use crate::interface::{
11+
storage::{StorageSnapshot, StorageView},
12+
ExecutionResult, InspectExecutionMode, VmFactory, VmInterfaceExt, VmRevertReason,
913
};
10-
use crate::interface::{ExecutionResult, InspectExecutionMode, VmInterfaceExt};
1114

1215
pub(crate) fn test_estimate_fee<VM: TestedVm>() {
1316
let mut vm_tester = VmTesterBuilder::new().with_rich_accounts(1).build::<VM>();
@@ -168,3 +171,17 @@ pub(crate) fn test_reusing_create2_salt<VM: TestedVm>() {
168171
let res = vm_tester.vm.execute(InspectExecutionMode::OneTx);
169172
assert_matches!(res.result, ExecutionResult::Success { .. });
170173
}
174+
175+
pub(crate) fn test_transfer_to_self_with_low_gas_limit<VM>()
176+
where
177+
VM: VmFactory<StorageView<StorageSnapshot>>,
178+
{
179+
let dump = load_vm_dump("estimate_fee_for_transfer_to_self");
180+
let result = execute_oneshot_dump::<VM>(dump);
181+
assert_eq!(
182+
result.result,
183+
ExecutionResult::Revert {
184+
output: VmRevertReason::from(&[] as &[u8]),
185+
}
186+
);
187+
}

‎core/lib/multivm/src/versions/vm_fast/tests/simple_execution.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::{
22
versions::testonly::simple_execution::{
33
test_create2_deployment_address, test_estimate_fee, test_reusing_create2_salt,
4-
test_reusing_create_address, test_simple_execute,
4+
test_reusing_create_address, test_simple_execute, test_transfer_to_self_with_low_gas_limit,
55
},
66
vm_fast::Vm,
77
};
@@ -30,3 +30,8 @@ fn reusing_create_address() {
3030
fn reusing_create2_salt() {
3131
test_reusing_create2_salt::<Vm<_>>();
3232
}
33+
34+
#[test]
35+
fn transfer_to_self_with_low_gas_limit() {
36+
test_transfer_to_self_with_low_gas_limit::<Vm<_>>();
37+
}

‎core/lib/multivm/src/versions/vm_latest/tests/simple_execution.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::{
22
versions::testonly::simple_execution::{
33
test_create2_deployment_address, test_estimate_fee, test_reusing_create2_salt,
4-
test_reusing_create_address, test_simple_execute,
4+
test_reusing_create_address, test_simple_execute, test_transfer_to_self_with_low_gas_limit,
55
},
66
vm_latest::{HistoryEnabled, Vm},
77
};
@@ -30,3 +30,8 @@ fn reusing_create_address() {
3030
fn reusing_create2_salt() {
3131
test_reusing_create2_salt::<Vm<_, HistoryEnabled>>();
3232
}
33+
34+
#[test]
35+
fn transfer_to_self_with_low_gas_limit() {
36+
test_transfer_to_self_with_low_gas_limit::<Vm<_, HistoryEnabled>>();
37+
}

‎core/lib/multivm/src/versions/vm_latest/tracers/result_tracer.rs

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -159,15 +159,12 @@ impl<S, H: HistoryMode> DynTracer<S, SimpleMemory<H>> for ResultTracer<S> {
159159
if matches!(hook, Some(VmHook::PostResult)) {
160160
let vm_hook_params = get_vm_hook_params(memory, self.subversion);
161161
let success = vm_hook_params[0];
162-
let returndata = self
163-
.far_call_tracker
164-
.get_latest_returndata()
165-
.map(|ptr| read_pointer(memory, ptr))
166-
.unwrap_or_default();
162+
let return_ptr = FatPointer::from_u256(vm_hook_params[1]);
163+
let returndata = read_pointer(memory, return_ptr);
167164
if success == U256::zero() {
168165
self.result = Some(Result::Error {
169166
// Tx has reverted, without bootloader error, we can simply parse the revert reason
170-
error_reason: (VmRevertReason::from(returndata.as_slice())),
167+
error_reason: VmRevertReason::from(returndata.as_slice()),
171168
});
172169
} else {
173170
self.result = Some(Result::Success {

‎core/lib/multivm/tests/vm_dumps/README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,5 @@ This directory contains VM dumps for regression testing.
77
- [`validation_adjacent_storage_slots`](validation_adjacent_storage_slots.json): Accesses adjacent storage slots in a
88
mapping during account validation (i.e., mapping values occupy >1 slot). Adjacent storage slots were previously
99
disallowed for the fast VM.
10+
- [`estimate_fee_for_transfer_to_self`](estimate_fee_for_transfer_to_self.json): Fee estimation step for a 0 base token
11+
transfer to self with a low gas limit. The legacy VM previously returned the incorrect revert reason.

‎core/lib/multivm/tests/vm_dumps/estimate_fee_for_transfer_to_self.json

Lines changed: 1 addition & 0 deletions
Large diffs are not rendered by default.

0 commit comments

Comments
 (0)