Skip to content

Commit

Permalink
fix(vm): fix circuit tracer (matter-labs#837)
Browse files Browse the repository at this point in the history 
## What ❔

`finish_cycle` method of circuit tracer wasn't called and there were
bugs in it.

## Why ❔

Fix tracer

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Tests for the changes have been added / updated.
- [x] Documentation comments have been added / updated.
- [x] Code has been formatted via `zk fmt` and `zk lint`.
- [x] Spellcheck has been run via `cargo spellcheck
--cfg=./spellcheck/era.cfg --code 1`.
  • Loading branch information
@perekopskiy
perekopskiy authored Jan 8, 2024
1 parent b685180 commit 83fc7be
Show file tree
Hide file tree
Showing 8 changed files with 62 additions and 62 deletions.
2 changes: 1 addition & 1 deletion core/lib/multivm/src/versions/vm_boojum_integration/old_vm/oracles/precompile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::vm_boojum_integration::old_vm::history_recorder::{
#[derive(Debug, Clone)]
pub struct PrecompilesProcessorWithHistory<H: HistoryMode> {
pub timestamp_history: HistoryRecorder<Vec<Timestamp>, H>,
pub precompile_cycles_history: HistoryRecorder<Vec<(PrecompileAddress, usize)>, HistoryEnabled>,
pub precompile_cycles_history: HistoryRecorder<Vec<(PrecompileAddress, usize)>, H>,
}

impl<H: HistoryMode> Default for PrecompilesProcessorWithHistory<H> {
Expand Down
7 changes: 4 additions & 3 deletions core/lib/multivm/src/versions/vm_boojum_integration/tests/circuits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ fn test_circuits() {
vm.vm.push_transaction(tx);
let res = vm.vm.inspect(Default::default(), VmExecutionMode::OneTx);

const EXPECTED_CIRCUITS_USED: f32 = 1.5521;
const EXPECTED_CIRCUITS_USED: f32 = 4.8685;
let delta =
(res.statistics.estimated_circuits_used - EXPECTED_CIRCUITS_USED) / EXPECTED_CIRCUITS_USED;

if delta.abs() > 0.1 {
panic!(
"Estimation differs from expected result by too much: {}%",
delta * 100.0
"Estimation differs from expected result by too much: {}%, expected value: {}",
delta * 100.0,
res.statistics.estimated_circuits_used
);
}
}
47 changes: 20 additions & 27 deletions core/lib/multivm/src/versions/vm_boojum_integration/tracers/circuits_tracer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ use crate::{
interface::{dyn_tracers::vm_1_4_0::DynTracer, tracer::TracerExecutionStatus},
vm_boojum_integration::{
bootloader_state::BootloaderState,
old_vm::{
history_recorder::{HistoryMode, VectorHistoryEvent},
memory::SimpleMemory,
},
old_vm::{history_recorder::HistoryMode, memory::SimpleMemory},
tracers::traits::VmTracer,
types::internals::ZkSyncVmState,
},
Expand All @@ -28,7 +25,7 @@ pub(crate) struct CircuitsTracer<S> {
last_decommitment_history_entry_checked: Option<usize>,
last_written_keys_history_entry_checked: Option<usize>,
last_read_keys_history_entry_checked: Option<usize>,
last_precompile_history_entry_checked: Option<usize>,
last_precompile_inner_entry_checked: Option<usize>,
_phantom_data: PhantomData<S>,
}

Expand All @@ -39,7 +36,7 @@ impl<S: WriteStorage> CircuitsTracer<S> {
last_decommitment_history_entry_checked: None,
last_written_keys_history_entry_checked: None,
last_read_keys_history_entry_checked: None,
last_precompile_history_entry_checked: None,
last_precompile_inner_entry_checked: None,
_phantom_data: Default::default(),
}
}
Expand Down Expand Up @@ -97,11 +94,11 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {

self.last_read_keys_history_entry_checked = Some(state.storage.read_keys.history().len());

self.last_precompile_history_entry_checked = Some(
self.last_precompile_inner_entry_checked = Some(
state
.precompiles_processor
.precompile_cycles_history
.history()
.inner()
.len(),
);
}
Expand All @@ -121,7 +118,7 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {
.history();
for (_, history_event) in &history[last_decommitment_history_entry_checked..] {
// We assume that only insertions may happen during a single VM inspection.
assert!(history_event.value.is_some());
assert!(history_event.value.is_none());
let bytecode_len = state
.decommittment_processor
.known_bytecodes
Expand All @@ -145,7 +142,7 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {
let history = state.storage.written_keys.history();
for (_, history_event) in &history[last_writes_history_entry_checked..] {
// We assume that only insertions may happen during a single VM inspection.
assert!(history_event.value.is_some());
assert!(history_event.value.is_none());

self.estimated_circuits_used += 2.0 * STORAGE_APPLICATION_CYCLE_FRACTION;
}
Expand All @@ -158,7 +155,7 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {
let history = state.storage.read_keys.history();
for (_, history_event) in &history[last_reads_history_entry_checked..] {
// We assume that only insertions may happen during a single VM inspection.
assert!(history_event.value.is_some());
assert!(history_event.value.is_none());

// If the slot is already written to, then we've already taken 2 cycles into account.
if !state
Expand All @@ -173,26 +170,22 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {
self.last_read_keys_history_entry_checked = Some(history.len());

// Process precompiles.
let last_precompile_history_entry_checked = self
.last_precompile_history_entry_checked
let last_precompile_inner_entry_checked = self
.last_precompile_inner_entry_checked
.expect("Value must be set during init");
let history = state
let inner = state
.precompiles_processor
.precompile_cycles_history
.history();
for (_, history_event) in &history[last_precompile_history_entry_checked..] {
if let VectorHistoryEvent::Push((precompile, cycles)) = history_event {
let fraction = match precompile {
PrecompileAddress::Ecrecover => ECRECOVER_CYCLE_FRACTION,
PrecompileAddress::SHA256 => SHA256_CYCLE_FRACTION,
PrecompileAddress::Keccak256 => KECCAK256_CYCLE_FRACTION,
};
self.estimated_circuits_used += (*cycles as f32) * fraction;
} else {
panic!("Precompile calls should not be rolled back");
}
.inner();
for (precompile, cycles) in &inner[last_precompile_inner_entry_checked..] {
let fraction = match precompile {
PrecompileAddress::Ecrecover => ECRECOVER_CYCLE_FRACTION,
PrecompileAddress::SHA256 => SHA256_CYCLE_FRACTION,
PrecompileAddress::Keccak256 => KECCAK256_CYCLE_FRACTION,
};
self.estimated_circuits_used += (*cycles as f32) * fraction;
}
self.last_precompile_history_entry_checked = Some(history.len());
self.last_precompile_inner_entry_checked = Some(inner.len());

TracerExecutionStatus::Continue
}
Expand Down
6 changes: 6 additions & 0 deletions core/lib/multivm/src/versions/vm_boojum_integration/tracers/default_tracers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,12 @@ impl<S: WriteStorage, H: HistoryMode> DefaultExecutionTracer<S, H> {
.finish_cycle(state, bootloader_state)
.stricter(&result);
}

result = self
.circuits_tracer
.finish_cycle(state, bootloader_state)
.stricter(&result);

result.stricter(&self.should_stop_execution())
}

Expand Down
2 changes: 1 addition & 1 deletion core/lib/multivm/src/versions/vm_latest/old_vm/oracles/precompile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use crate::vm_latest::old_vm::history_recorder::{HistoryEnabled, HistoryMode, Hi
#[derive(Debug, Clone)]
pub struct PrecompilesProcessorWithHistory<H: HistoryMode> {
pub timestamp_history: HistoryRecorder<Vec<Timestamp>, H>,
pub precompile_cycles_history: HistoryRecorder<Vec<(PrecompileAddress, usize)>, HistoryEnabled>,
pub precompile_cycles_history: HistoryRecorder<Vec<(PrecompileAddress, usize)>, H>,
}

impl<H: HistoryMode> Default for PrecompilesProcessorWithHistory<H> {
Expand Down
7 changes: 4 additions & 3 deletions core/lib/multivm/src/versions/vm_latest/tests/circuits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ fn test_circuits() {
vm.vm.push_transaction(tx);
let res = vm.vm.inspect(Default::default(), VmExecutionMode::OneTx);

const EXPECTED_CIRCUITS_USED: f32 = 1.5521;
const EXPECTED_CIRCUITS_USED: f32 = 4.8685;
let delta =
(res.statistics.estimated_circuits_used - EXPECTED_CIRCUITS_USED) / EXPECTED_CIRCUITS_USED;

if delta.abs() > 0.1 {
panic!(
"Estimation differs from expected result by too much: {}%",
delta * 100.0
"Estimation differs from expected result by too much: {}%, expected value: {}",
delta * 100.0,
res.statistics.estimated_circuits_used
);
}
}
47 changes: 20 additions & 27 deletions core/lib/multivm/src/versions/vm_latest/tracers/circuits_tracer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@ use crate::{
interface::{dyn_tracers::vm_1_4_0::DynTracer, tracer::TracerExecutionStatus},
vm_latest::{
bootloader_state::BootloaderState,
old_vm::{
history_recorder::{HistoryMode, VectorHistoryEvent},
memory::SimpleMemory,
},
old_vm::{history_recorder::HistoryMode, memory::SimpleMemory},
tracers::traits::VmTracer,
types::internals::ZkSyncVmState,
},
Expand All @@ -28,7 +25,7 @@ pub(crate) struct CircuitsTracer<S> {
last_decommitment_history_entry_checked: Option<usize>,
last_written_keys_history_entry_checked: Option<usize>,
last_read_keys_history_entry_checked: Option<usize>,
last_precompile_history_entry_checked: Option<usize>,
last_precompile_inner_entry_checked: Option<usize>,
_phantom_data: PhantomData<S>,
}

Expand All @@ -39,7 +36,7 @@ impl<S: WriteStorage> CircuitsTracer<S> {
last_decommitment_history_entry_checked: None,
last_written_keys_history_entry_checked: None,
last_read_keys_history_entry_checked: None,
last_precompile_history_entry_checked: None,
last_precompile_inner_entry_checked: None,
_phantom_data: Default::default(),
}
}
Expand Down Expand Up @@ -97,11 +94,11 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {

self.last_read_keys_history_entry_checked = Some(state.storage.read_keys.history().len());

self.last_precompile_history_entry_checked = Some(
self.last_precompile_inner_entry_checked = Some(
state
.precompiles_processor
.precompile_cycles_history
.history()
.inner()
.len(),
);
}
Expand All @@ -121,7 +118,7 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {
.history();
for (_, history_event) in &history[last_decommitment_history_entry_checked..] {
// We assume that only insertions may happen during a single VM inspection.
assert!(history_event.value.is_some());
assert!(history_event.value.is_none());
let bytecode_len = state
.decommittment_processor
.known_bytecodes
Expand All @@ -145,7 +142,7 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {
let history = state.storage.written_keys.history();
for (_, history_event) in &history[last_writes_history_entry_checked..] {
// We assume that only insertions may happen during a single VM inspection.
assert!(history_event.value.is_some());
assert!(history_event.value.is_none());

self.estimated_circuits_used += 2.0 * STORAGE_APPLICATION_CYCLE_FRACTION;
}
Expand All @@ -158,7 +155,7 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {
let history = state.storage.read_keys.history();
for (_, history_event) in &history[last_reads_history_entry_checked..] {
// We assume that only insertions may happen during a single VM inspection.
assert!(history_event.value.is_some());
assert!(history_event.value.is_none());

// If the slot is already written to, then we've already taken 2 cycles into account.
if !state
Expand All @@ -173,26 +170,22 @@ impl<S: WriteStorage, H: HistoryMode> VmTracer<S, H> for CircuitsTracer<S> {
self.last_read_keys_history_entry_checked = Some(history.len());

// Process precompiles.
let last_precompile_history_entry_checked = self
.last_precompile_history_entry_checked
let last_precompile_inner_entry_checked = self
.last_precompile_inner_entry_checked
.expect("Value must be set during init");
let history = state
let inner = state
.precompiles_processor
.precompile_cycles_history
.history();
for (_, history_event) in &history[last_precompile_history_entry_checked..] {
if let VectorHistoryEvent::Push((precompile, cycles)) = history_event {
let fraction = match precompile {
PrecompileAddress::Ecrecover => ECRECOVER_CYCLE_FRACTION,
PrecompileAddress::SHA256 => SHA256_CYCLE_FRACTION,
PrecompileAddress::Keccak256 => KECCAK256_CYCLE_FRACTION,
};
self.estimated_circuits_used += (*cycles as f32) * fraction;
} else {
panic!("Precompile calls should not be rolled back");
}
.inner();
for (precompile, cycles) in &inner[last_precompile_inner_entry_checked..] {
let fraction = match precompile {
PrecompileAddress::Ecrecover => ECRECOVER_CYCLE_FRACTION,
PrecompileAddress::SHA256 => SHA256_CYCLE_FRACTION,
PrecompileAddress::Keccak256 => KECCAK256_CYCLE_FRACTION,
};
self.estimated_circuits_used += (*cycles as f32) * fraction;
}
self.last_precompile_history_entry_checked = Some(history.len());
self.last_precompile_inner_entry_checked = Some(inner.len());

TracerExecutionStatus::Continue
}
Expand Down
6 changes: 6 additions & 0 deletions core/lib/multivm/src/versions/vm_latest/tracers/default_tracers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,12 @@ impl<S: WriteStorage, H: HistoryMode> DefaultExecutionTracer<S, H> {
.finish_cycle(state, bootloader_state)
.stricter(&result);
}

result = self
.circuits_tracer
.finish_cycle(state, bootloader_state)
.stricter(&result);

result.stricter(&self.should_stop_execution())
}

Expand Down

0 comments on commit 83fc7be

Please sign in to comment.