fix(vm): Fix VM divergence in revert data #3570
Conversation
Detected VM performance changes
|
| @@ -460,15 +460,15 @@ jobs: | |||
|
|
|||
| - name: Run integration tests | |||
| run: | | |||
| ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --no-deps --ignore-prerequisites" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }} | |||
| ci_run ./bin/run_on_all_chains.sh "zkstack dev test integration --verbose --no-deps --ignore-prerequisites" ${{ env.CHAINS }} ${{ env.INTEGRATION_TESTS_LOGS_DIR }} | |||
There was a problem hiding this comment.
Is the size of the generated log files a concern? If not, verbose output can significantly help with investigating integration test failures. And it doesn't seem to add that much; the logs artifact with verbose output is ~7.8 MB against ~7.5 MB before.
| - [`estimate_fee_for_transfer_to_self`](estimate_fee_for_transfer_to_self.json): Fee estimation step for a 0 base token | ||
| transfer to self with a low gas limit. The legacy VM previously returned the incorrect revert reason. |
There was a problem hiding this comment.
JFYI, I haven't been able to reproduce the setup with pure code. May have to do with the account being previously active and/or block params.
| let returndata = self | ||
| .far_call_tracker | ||
| .get_latest_returndata() | ||
| .map(|ptr| read_pointer(memory, ptr)) | ||
| .unwrap_or_default(); | ||
| let return_ptr = FatPointer::from_u256(vm_hook_params[1]); | ||
| let returndata = read_pointer(memory, return_ptr); |
There was a problem hiding this comment.
Without this change, running the VM dump adding in this PR (i.e., estimating fee for a 0 transfer to self with a low tx gas limit) on the legacy VM leads to a non-sensical 32-byte returndata, which looks like a non-zero big-endian uint from some other call.
This is how this hook is implemented in the fast VM:
zksync-era/core/lib/multivm/src/versions/vm_fast/vm.rs
Lines 552 to 554 in 8d8421c
AFAICT, the 1st hook param is indeed set by the bootloader (and has been set for a long time), so the previous implementation looks like a legacy artifact. IIUC, the bootloader used with fee estimation has some optimizations enabled, which in this case led to call reordering (?), making the last returndata captured by the tracer (it's still used for the DebugReturnData hook, BTW) not match the returndata set by the PostResult hook.
slowli
requested review from
yorik,
alexandrst88,
artmakh,
hatemosphere,
onyxet,
otani88,
iluwaa,
Deniallugo and
joonazan
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
zkstack dev fmtandzkstack dev lint.