Skip to content
This repository has been archived by the owner on Aug 16, 2024. It is now read-only.

fix: use correct rollback tails for empty frames without history #158

Closed
wants to merge 9 commits into from
Closed
40 changes: 17 additions & 23 deletions src/tests/simple_tests/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,41 +2,35 @@
mod tests {
use crate::tests::simple_tests::{asm_tests::run_asm_based_test, Options};

#[test_log::test]
fn test_pubdata_and_storage_writes() {
fn test_snapshot_every_cycle(dir: &str, additional_contracts: &[i32]) {
run_asm_based_test(
"src/tests/simple_tests/testdata/log/storage/storage_writes",
&[],
&format!("src/tests/simple_tests/testdata/{}", dir),
additional_contracts,
Options {
// Do only 1 cycle per VM snapshot to really test all the boundary conditions.
cycles_per_vm_snapshot: 1,
..Default::default()
},
);
)
}

#[test_log::test]
fn test_pubdata_and_storage_writes() {
test_snapshot_every_cycle("log/storage/storage_writes", &[]);
}

#[test_log::test]
fn test_storage_reads() {
run_asm_based_test(
"src/tests/simple_tests/testdata/log/storage/storage_reads",
&[],
Options {
// Do only 1 cycle per VM snapshot to really test all the boundary conditions.
cycles_per_vm_snapshot: 1,
..Default::default()
},
)
test_snapshot_every_cycle("log/storage/storage_reads", &[]);
}

#[test_log::test]
fn test_storage_write_after_panic() {
test_snapshot_every_cycle("log/storage/storage_write_after_panic", &[]);
}

#[test_log::test]
fn test_storage_pubdata_refunds() {
run_asm_based_test(
"src/tests/simple_tests/testdata/log/storage/storage_pubdata_refunds",
&[],
Options {
// Do only 1 cycle per VM snapshot to really test all the boundary conditions.
cycles_per_vm_snapshot: 1,
..Default::default()
},
);
test_snapshot_every_cycle("log/storage/storage_pubdata_refunds", &[]);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
.text
.file "Test_26"
.rodata.cst32
.p2align 5
.text
.globl __entry
__entry:
.main:
add 1000, r0, r3
near_call r3, @test_invalid, @expected_panic
revert("Near call not panicked")

test_invalid:
ret.panic r0

expected_panic:
; check that we can access storage after panic
log.swrite r0, r0, r0
ret.ok r0
31 changes: 29 additions & 2 deletions src/witness/oracle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -552,8 +552,35 @@ pub fn create_artifacts_from_tracer<
// wherever we have this marker we should look at the tail of the item right before it
let pos = log_position_mapping[&rollback_tail_marker];
let tail = if pos == -1 {
// empty
global_end_of_storage_log
if frame_index > 1 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - this is more than 'adding tests' - please update the PR description

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mm-zk you are right. Even more, proposed changes do not fix corresponding issue. I will rewrite this part of logic

// empty frame
let pos_forward_head =
log_position_mapping[&ExtendedLogQuery::FrameForwardHeadMarker(frame_index)];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need similar code in zksync-era's oracle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need similar code in zksync-era's oracle?

I'm not sure that I understand your question: zksync-era calls this code via external_calls.rs

let pos_forward_tail =
log_position_mapping[&ExtendedLogQuery::FrameForwardTailMarker(frame_index)];
let pos_rollback_head =
log_position_mapping[&ExtendedLogQuery::FrameRollbackHeadMarker(frame_index)];

if pos_forward_head == -1 && pos_forward_tail == -1 && pos_rollback_head == -1 {
// absolutely empty frame, most likely leaf one,
// but we can not just use global end,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we cannot use global end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we can have some state changes after this rollback. But our rollback tail should be equal to the last state change tail before the rollback.

Global end was used under assumption that situation without rollback marker can happen only in trivial cases (like entry bytecode without any state changes)

// instead we should use previous frame's data
let pos_previous_frame_forward_tail = log_position_mapping
[&ExtendedLogQuery::FrameForwardTailMarker(frame_index - 1)];
if pos_previous_frame_forward_tail != -1 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add more explanations, as I couldn't fully understand the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea: since we do not have rollback tail marker, we should try to use forward tail of the parent of this frame.

Incorrect assumption: if parent is empty too, than it is "root" frame.
Actually this is wrong. We can have any hierarchy of empty frames

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the code and added more comments

let pointer = pos_previous_frame_forward_tail as usize;
let element = chain_of_states[pointer].2 .0;
0xVolosnikov marked this conversation as resolved.
Show resolved Hide resolved

element
} else {
global_end_of_storage_log
}
} else {
global_end_of_storage_log
}
} else {
global_end_of_storage_log
}
} else {
let pointer = pos as usize;
let element = chain_of_states[pointer].2 .1;
Expand Down
Loading