From 30a12bc992605cda49a92c515d04b3014b27f0f1 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Mon, 28 Oct 2024 18:37:23 +0100 Subject: [PATCH 01/46] integrate stage1 (tracers don't handle hooks yet) --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- .../multivm/src/versions/vm_fast/circuits_tracer.rs | 11 +++++++++-- prover/Cargo.lock | 4 ++-- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 597da3c1b31b..2a3df5e9da2e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11343,7 +11343,7 @@ dependencies = [ [[package]] name = "zksync_vm2" version = "0.2.1" -source = "git+https://github.com/matter-labs/vm2.git?rev=df5bec3d04d64d434f9b0ccb285ba4681008f7b3#df5bec3d04d64d434f9b0ccb285ba4681008f7b3" +source = "git+https://github.com/matter-labs/vm2.git?rev=f71f0650cb84888d37d58e5deece42f69eae5200#f71f0650cb84888d37d58e5deece42f69eae5200" dependencies = [ "enum_dispatch", "primitive-types", @@ -11355,7 +11355,7 @@ dependencies = [ [[package]] name = "zksync_vm2_interface" version = "0.2.1" -source = "git+https://github.com/matter-labs/vm2.git?rev=df5bec3d04d64d434f9b0ccb285ba4681008f7b3#df5bec3d04d64d434f9b0ccb285ba4681008f7b3" +source = "git+https://github.com/matter-labs/vm2.git?rev=f71f0650cb84888d37d58e5deece42f69eae5200#f71f0650cb84888d37d58e5deece42f69eae5200" dependencies = [ "primitive-types", ] diff --git a/Cargo.toml b/Cargo.toml index 6d51e5060aa8..dfdacb44a573 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -230,7 +230,7 @@ zk_evm_1_4_1 = { package = "zk_evm", version = "0.141" } zk_evm_1_5_0 = { package = "zk_evm", version = "=0.150.7" } # New VM; pinned to a specific commit because of instability -zksync_vm2 = { git = "https://github.com/matter-labs/vm2.git", rev = "df5bec3d04d64d434f9b0ccb285ba4681008f7b3" } +zksync_vm2 = { git = "https://github.com/matter-labs/vm2.git", rev = "f71f0650cb84888d37d58e5deece42f69eae5200" } # Consensus dependencies. zksync_concurrency = "=0.5.0" diff --git a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs b/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs index f588f20ab25d..3585ea876daf 100644 --- a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs @@ -1,5 +1,7 @@ use circuit_sequencer_api_1_5_0::{geometry_config::get_geometry_config, toolset::GeometryConfig}; -use zksync_vm2::interface::{CycleStats, GlobalStateInterface, Opcode, OpcodeType, Tracer}; +use zksync_vm2::interface::{ + CycleStats, ExecutionStatus, GlobalStateInterface, Opcode, OpcodeType, Tracer, +}; use zksync_vm_interface::CircuitStatistic; use crate::vm_latest::tracers::circuits_capacity::*; @@ -24,7 +26,10 @@ pub struct CircuitsTracer { } impl Tracer for CircuitsTracer { - fn after_instruction(&mut self, _: &mut S) { + fn after_instruction( + &mut self, + _: &mut S, + ) -> ExecutionStatus { self.main_vm_cycles += 1; match OP::VALUE { @@ -110,6 +115,8 @@ impl Tracer for CircuitsTracer { self.ram_permutation_cycles += UMA_READ_RAM_CYCLES; } } + + ExecutionStatus::Running } fn on_extra_prover_cycles(&mut self, stats: CycleStats) { diff --git a/prover/Cargo.lock b/prover/Cargo.lock index d68ef368a4aa..b4a1f786a81c 100644 --- a/prover/Cargo.lock +++ b/prover/Cargo.lock @@ -8685,7 +8685,7 @@ dependencies = [ [[package]] name = "zksync_vm2" version = "0.2.1" -source = "git+https://github.com/matter-labs/vm2.git?rev=df5bec3d04d64d434f9b0ccb285ba4681008f7b3#df5bec3d04d64d434f9b0ccb285ba4681008f7b3" +source = "git+https://github.com/matter-labs/vm2.git?rev=f71f0650cb84888d37d58e5deece42f69eae5200#f71f0650cb84888d37d58e5deece42f69eae5200" dependencies = [ "enum_dispatch", "primitive-types", @@ -8697,7 +8697,7 @@ dependencies = [ [[package]] name = "zksync_vm2_interface" version = "0.2.1" -source = "git+https://github.com/matter-labs/vm2.git?rev=df5bec3d04d64d434f9b0ccb285ba4681008f7b3#df5bec3d04d64d434f9b0ccb285ba4681008f7b3" +source = "git+https://github.com/matter-labs/vm2.git?rev=f71f0650cb84888d37d58e5deece42f69eae5200#f71f0650cb84888d37d58e5deece42f69eae5200" dependencies = [ "primitive-types", ] From 1983bd171aa53c69a890a1836e1e7b2ba395e96a Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Tue, 29 Oct 2024 12:39:56 +0100 Subject: [PATCH 02/46] version of vm2 that inlines ExecutionStatus::merge --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- prover/Cargo.lock | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2a3df5e9da2e..4160b66ed1d5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11343,7 +11343,7 @@ dependencies = [ [[package]] name = "zksync_vm2" version = "0.2.1" -source = "git+https://github.com/matter-labs/vm2.git?rev=f71f0650cb84888d37d58e5deece42f69eae5200#f71f0650cb84888d37d58e5deece42f69eae5200" +source = "git+https://github.com/matter-labs/vm2.git?rev=fd27994d1d88ea4868d9ad74746bf274b97126d1#fd27994d1d88ea4868d9ad74746bf274b97126d1" dependencies = [ "enum_dispatch", "primitive-types", @@ -11355,7 +11355,7 @@ dependencies = [ [[package]] name = "zksync_vm2_interface" version = "0.2.1" -source = "git+https://github.com/matter-labs/vm2.git?rev=f71f0650cb84888d37d58e5deece42f69eae5200#f71f0650cb84888d37d58e5deece42f69eae5200" +source = "git+https://github.com/matter-labs/vm2.git?rev=fd27994d1d88ea4868d9ad74746bf274b97126d1#fd27994d1d88ea4868d9ad74746bf274b97126d1" dependencies = [ "primitive-types", ] diff --git a/Cargo.toml b/Cargo.toml index dfdacb44a573..41c3df46c355 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -230,7 +230,7 @@ zk_evm_1_4_1 = { package = "zk_evm", version = "0.141" } zk_evm_1_5_0 = { package = "zk_evm", version = "=0.150.7" } # New VM; pinned to a specific commit because of instability -zksync_vm2 = { git = "https://github.com/matter-labs/vm2.git", rev = "f71f0650cb84888d37d58e5deece42f69eae5200" } +zksync_vm2 = { git = "https://github.com/matter-labs/vm2.git", rev = "fd27994d1d88ea4868d9ad74746bf274b97126d1" } # Consensus dependencies. zksync_concurrency = "=0.5.0" diff --git a/prover/Cargo.lock b/prover/Cargo.lock index b4a1f786a81c..3449f0758554 100644 --- a/prover/Cargo.lock +++ b/prover/Cargo.lock @@ -8685,7 +8685,7 @@ dependencies = [ [[package]] name = "zksync_vm2" version = "0.2.1" -source = "git+https://github.com/matter-labs/vm2.git?rev=f71f0650cb84888d37d58e5deece42f69eae5200#f71f0650cb84888d37d58e5deece42f69eae5200" +source = "git+https://github.com/matter-labs/vm2.git?rev=fd27994d1d88ea4868d9ad74746bf274b97126d1#fd27994d1d88ea4868d9ad74746bf274b97126d1" dependencies = [ "enum_dispatch", "primitive-types", @@ -8697,7 +8697,7 @@ dependencies = [ [[package]] name = "zksync_vm2_interface" version = "0.2.1" -source = "git+https://github.com/matter-labs/vm2.git?rev=f71f0650cb84888d37d58e5deece42f69eae5200#f71f0650cb84888d37d58e5deece42f69eae5200" +source = "git+https://github.com/matter-labs/vm2.git?rev=fd27994d1d88ea4868d9ad74746bf274b97126d1#fd27994d1d88ea4868d9ad74746bf274b97126d1" dependencies = [ "primitive-types", ] From 488c93ae51b82eb80f3b3f3c8d2a011697bf5061 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Thu, 12 Sep 2024 15:42:43 +0200 Subject: [PATCH 03/46] improve naming: paymaster/account validation use the same end hook! --- core/lib/multivm/src/versions/vm_fast/hook.rs | 4 ++-- core/lib/multivm/src/versions/vm_fast/vm.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/hook.rs b/core/lib/multivm/src/versions/vm_fast/hook.rs index 8d385f94f3e1..c582bed7562f 100644 --- a/core/lib/multivm/src/versions/vm_fast/hook.rs +++ b/core/lib/multivm/src/versions/vm_fast/hook.rs @@ -2,7 +2,7 @@ pub(crate) enum Hook { AccountValidationEntered, PaymasterValidationEntered, - AccountValidationExited, + ValidationExited, ValidationStepEnded, TxHasEnded, DebugLog, @@ -22,7 +22,7 @@ impl Hook { match hook { 0 => Hook::AccountValidationEntered, 1 => Hook::PaymasterValidationEntered, - 2 => Hook::AccountValidationExited, + 2 => Hook::ValidationExited, 3 => Hook::ValidationStepEnded, 4 => Hook::TxHasEnded, 5 => Hook::DebugLog, diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 6ebc4b9c5716..bfbf7c3275ce 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -208,7 +208,7 @@ impl Vm { }; match Hook::from_u32(hook) { - Hook::AccountValidationEntered | Hook::AccountValidationExited => { + Hook::AccountValidationEntered | Hook::ValidationExited => { // TODO (PLA-908): implement account validation } Hook::TxHasEnded => { From 3837492ecd2c1c1c78e8dae609c41619c367ce0e Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Thu, 12 Sep 2024 17:32:18 +0200 Subject: [PATCH 04/46] implement account validation gas limit --- core/lib/multivm/src/versions/vm_fast/vm.rs | 44 +++++++++++++++------ 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index bfbf7c3275ce..75463c9f143f 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -95,7 +95,6 @@ impl VmRunResult { pub struct Vm { pub(crate) world: World>, pub(crate) inner: VirtualMachine, World>>, - gas_for_account_validation: u32, pub(crate) bootloader_state: BootloaderState, pub(crate) batch_env: L1BatchEnv, pub(crate) system_env: SystemEnv, @@ -154,7 +153,6 @@ impl Vm { let mut this = Self { world: World::new(storage, program_cache), inner, - gas_for_account_validation: system_env.default_validation_computational_gas_limit, bootloader_state: BootloaderState::new( system_env.execution_mode, bootloader_memory.clone(), @@ -176,6 +174,14 @@ impl Vm { tracer: &mut (Tr, CircuitsTracer), track_refunds: bool, ) -> VmRunResult { + struct AccountValidationGasSplit { + gas_given: u32, + gas_hidden: u32, + } + let mut gas_left_for_account_validation = + self.system_env.default_validation_computational_gas_limit; + let mut account_validation_gas_split = None; + let mut refunds = Refunds { gas_refunded: 0, operator_suggested_refund: 0, @@ -208,8 +214,30 @@ impl Vm { }; match Hook::from_u32(hook) { - Hook::AccountValidationEntered | Hook::ValidationExited => { - // TODO (PLA-908): implement account validation + Hook::AccountValidationEntered => { + assert!( + account_validation_gas_split.is_none(), + "Account validation can't be nested" + ); + let gas = self.gas_remaining(); + let gas_given = gas.min(gas_left_for_account_validation); + account_validation_gas_split = Some(AccountValidationGasSplit { + gas_given, + gas_hidden: gas - gas_given, + }); + self.inner.current_frame().set_gas(gas_given); + } + + Hook::ValidationExited => { + if let Some(AccountValidationGasSplit { + gas_given, + gas_hidden, + }) = account_validation_gas_split.take() + { + let gas_left = self.inner.current_frame().gas(); + gas_left_for_account_validation -= gas_given - gas_left; + self.inner.current_frame().set_gas(gas_left + gas_hidden); + } } Hook::TxHasEnded => { if let VmExecutionMode::OneTx = execution_mode { @@ -737,7 +765,6 @@ impl VmInterface for Vm { #[derive(Debug)] struct VmSnapshot { bootloader_snapshot: BootloaderStateSnapshot, - gas_for_account_validation: u32, } impl VmInterfaceHistoryEnabled for Vm { @@ -750,19 +777,16 @@ impl VmInterfaceHistoryEnabled f self.inner.make_snapshot(); self.snapshot = Some(VmSnapshot { bootloader_snapshot: self.bootloader_state.get_snapshot(), - gas_for_account_validation: self.gas_for_account_validation, }); } fn rollback_to_the_latest_snapshot(&mut self) { let VmSnapshot { bootloader_snapshot, - gas_for_account_validation, } = self.snapshot.take().expect("no snapshots to rollback to"); self.inner.rollback(); self.bootloader_state.apply_snapshot(bootloader_snapshot); - self.gas_for_account_validation = gas_for_account_validation; } fn pop_snapshot_no_rollback(&mut self) { @@ -780,10 +804,6 @@ impl VmTrackingContracts for Vm { impl fmt::Debug for Vm { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Vm") - .field( - "gas_for_account_validation", - &self.gas_for_account_validation, - ) .field("bootloader_state", &self.bootloader_state) .field("storage", &self.world.storage) .field("program_cache", &self.world.program_cache) From fcb559de0929919283796e94a0aeaa9b1bd5cfac Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Tue, 8 Oct 2024 21:01:27 +0200 Subject: [PATCH 05/46] pass the validation gas limit test --- .../src/versions/vm_fast/circuits_tracer.rs | 3 + core/lib/multivm/src/versions/vm_fast/mod.rs | 6 +- .../src/versions/vm_fast/validation_tracer.rs | 48 +++++++++++++ core/lib/multivm/src/versions/vm_fast/vm.rs | 27 +++++-- core/lib/multivm/src/vm_instance.rs | 6 +- core/lib/vm_executor/src/oneshot/mod.rs | 72 +++++++++++++------ 6 files changed, 134 insertions(+), 28 deletions(-) create mode 100644 core/lib/multivm/src/versions/vm_fast/validation_tracer.rs diff --git a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs b/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs index 3585ea876daf..a1143738c1b5 100644 --- a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs @@ -4,6 +4,7 @@ use zksync_vm2::interface::{ }; use zksync_vm_interface::CircuitStatistic; +use super::Tracer as TracerExt; use crate::vm_latest::tracers::circuits_capacity::*; /// VM tracer tracking [`CircuitStatistic`]s. Statistics generally depend on the number of time some opcodes were invoked, @@ -132,6 +133,8 @@ impl Tracer for CircuitsTracer { } } +impl TracerExt for CircuitsTracer {} + impl CircuitsTracer { /// Obtains the current circuit stats from this tracer. pub fn circuit_statistic(&self) -> CircuitStatistic { diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index 35789c6cdc9a..12368f8d287e 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -1,6 +1,9 @@ pub use zksync_vm2::interface; -pub use self::{circuits_tracer::CircuitsTracer, vm::Vm}; +pub use self::{ + circuits_tracer::CircuitsTracer, + vm::{TracerExt as Tracer, Vm}, +}; mod bootloader_state; mod bytecode; @@ -14,4 +17,5 @@ mod refund; #[cfg(test)] mod tests; mod transaction_data; +pub mod validation_tracer; mod vm; diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs new file mode 100644 index 000000000000..71cc4cabccd1 --- /dev/null +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -0,0 +1,48 @@ +use zksync_vm2::interface::{ + self, CallframeInterface, Opcode::*, OpcodeType, ReturnType::*, Tracer, +}; + +use super::vm::TracerExt; + +pub struct ValidationTracer { + pub probably_out_of_gas: bool, + pub in_validation: bool, +} + +impl Default for ValidationTracer { + fn default() -> Self { + Self { + probably_out_of_gas: false, + in_validation: false, + } + } +} + +impl Tracer for ValidationTracer { + fn before_instruction(&mut self, state: &mut S) { + if !self.in_validation { + return; + } + match OP::VALUE { + /* TODO this does not work because there is some Ret(Normal) before exit_validation + NearCall | FarCall(_) | Ret(Normal) => { + if self.probably_out_of_gas { + dbg!("out of gas canceled"); + } + self.probably_out_of_gas = false + }*/ + Ret(Panic) if state.current_frame().gas() == 0 => self.probably_out_of_gas = true, + _ => {} + } + } +} + +impl TracerExt for ValidationTracer { + fn enter_validation(&mut self) { + self.in_validation = true; + } + + fn exit_validation(&mut self) { + self.in_validation = false; + } +} diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 75463c9f143f..0e166b1616ba 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -87,6 +87,23 @@ impl VmRunResult { } } +pub trait TracerExt: Tracer { + fn enter_validation(&mut self) {} + fn exit_validation(&mut self) {} +} + +impl TracerExt for () {} +impl TracerExt for (A, B) { + fn enter_validation(&mut self) { + self.0.enter_validation(); + self.1.enter_validation(); + } + fn exit_validation(&mut self) { + self.0.exit_validation(); + self.1.exit_validation(); + } +} + /// Fast VM wrapper. /// /// The wrapper is parametric by the storage and tracer types. Besides the [`Tracer`] trait, a tracer must have `'static` lifetime @@ -103,7 +120,7 @@ pub struct Vm { enforced_state_diffs: Option>, } -impl Vm { +impl Vm { pub fn custom(batch_env: L1BatchEnv, system_env: SystemEnv, storage: S) -> Self { assert!( is_supported_by_fast_vm(system_env.version), @@ -219,6 +236,7 @@ impl Vm { account_validation_gas_split.is_none(), "Account validation can't be nested" ); + tracer.enter_validation(); let gas = self.gas_remaining(); let gas_given = gas.min(gas_left_for_account_validation); account_validation_gas_split = Some(AccountValidationGasSplit { @@ -229,6 +247,7 @@ impl Vm { } Hook::ValidationExited => { + tracer.exit_validation(); if let Some(AccountValidationGasSplit { gas_given, gas_hidden, @@ -678,7 +697,7 @@ impl Vm { } } -impl VmFactory> for Vm, Tr> +impl VmFactory> for Vm, Tr> where S: ReadStorage, Tr: Tracer + Default + 'static, @@ -693,7 +712,7 @@ where } } -impl VmInterface for Vm { +impl VmInterface for Vm { type TracerDispatcher = Tr; fn push_transaction(&mut self, tx: Transaction) -> PushTransactionResult<'_> { @@ -767,7 +786,7 @@ struct VmSnapshot { bootloader_snapshot: BootloaderStateSnapshot, } -impl VmInterfaceHistoryEnabled for Vm { +impl VmInterfaceHistoryEnabled for Vm { fn make_snapshot(&mut self) { assert!( self.snapshot.is_none(), diff --git a/core/lib/multivm/src/vm_instance.rs b/core/lib/multivm/src/vm_instance.rs index e2f72bd24113..2880bd24632a 100644 --- a/core/lib/multivm/src/vm_instance.rs +++ b/core/lib/multivm/src/vm_instance.rs @@ -1,7 +1,6 @@ use std::{mem, rc::Rc}; use zksync_types::{vm::VmVersion, ProtocolVersionId, Transaction}; -use zksync_vm2::interface::Tracer; use zksync_vm_interface::{pubdata::PubdataBuilder, InspectExecutionMode}; use crate::{ @@ -14,6 +13,7 @@ use crate::{ VmMemoryMetrics, }, tracers::TracerDispatcher, + vm_fast::Tracer, vm_latest::HistoryEnabled, }; @@ -250,7 +250,9 @@ macro_rules! dispatch_fast_vm { }; } -impl VmInterface for FastVmInstance { +impl VmInterface + for FastVmInstance +{ type TracerDispatcher = ( crate::vm_latest::TracerDispatcher, HistoryEnabled>, Tr, diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 154c838f824f..dd032202e0ad 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -18,7 +18,7 @@ use zksync_multivm::{ interface::{ executor::{OneshotExecutor, TransactionValidator}, storage::{ReadStorage, StorageView, StorageWithOverrides}, - tracer::{ValidationError, ValidationParams}, + tracer::{ValidationError, ValidationParams, ViolatedValidationRule}, utils::{DivergenceHandler, ShadowVm}, Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, OneshotTransactionExecutionResult, StoredL2BlockEnv, TxExecutionArgs, TxExecutionMode, @@ -27,6 +27,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, tracers::{CallTracer, StorageInvocations, TracerDispatcher, ValidationTracer}, utils::adjust_pubdata_price_for_tx, + vm_fast::Tracer, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, FastVmInstance, HistoryMode, LegacyVmInstance, MultiVMTracer, @@ -179,7 +180,7 @@ where ); let sandbox = VmSandbox { - fast_vm_mode: FastVmMode::Old, + fast_vm_mode: FastVmMode::New, panic_on_divergence: self.panic_on_divergence, storage, env, @@ -195,22 +196,44 @@ where ); let tracers = vec![validation_tracer.into_tracer_pointer()]; - let exec_result = sandbox.execute_in_vm(|vm, transaction| { - let Vm::Legacy(vm) = vm else { - unreachable!("Fast VM is never used for validation yet"); - }; - vm.push_transaction(transaction); - vm.inspect(&mut tracers.into(), InspectExecutionMode::OneTx) - }); - let validation_result = Arc::make_mut(&mut validation_result) - .take() - .map_or(Ok(()), Err); - - match (exec_result.result, validation_result) { - (_, Err(violated_rule)) => Err(ValidationError::ViolatedRule(violated_rule)), - (ExecutionResult::Halt { reason }, _) => Err(ValidationError::FailedTx(reason)), - _ => Ok(()), - } + sandbox.execute_in_vm_with_tracer(|vm, transaction| match vm { + Vm::Legacy(vm) => { + vm.push_transaction(transaction); + let exec_result = vm.inspect(&mut tracers.into(), InspectExecutionMode::OneTx); + + let validation_result = Arc::make_mut(&mut validation_result) + .take() + .map_or(Ok(()), Err); + + match (exec_result.result, validation_result) { + (_, Err(violated_rule)) => { + Err(ValidationError::ViolatedRule(violated_rule)) + } + (ExecutionResult::Halt { reason }, _) => { + Err(ValidationError::FailedTx(reason)) + } + _ => Ok(()), + } + } + + Vm::Fast(FastVmInstance::Fast(vm)) => { + vm.push_transaction(transaction); + let mut tracer = + zksync_multivm::vm_fast::validation_tracer::ValidationTracer::default(); + vm.inspect(&mut tracer, InspectExecutionMode::OneTx); + if tracer.probably_out_of_gas { + Err(ValidationError::ViolatedRule( + ViolatedValidationRule::TookTooManyComputationalGas(1), + )) + } else { + Ok(()) + } + } + + _ => { + unimplemented!("Shadow validation is not supported.") + } + }) }) .await .context("VM execution panicked") @@ -218,9 +241,9 @@ where } #[derive(Debug)] -enum Vm { +enum Vm { Legacy(LegacyVmInstance), - Fast(FastVmInstance), + Fast(FastVmInstance), } impl Vm { @@ -341,8 +364,15 @@ impl VmSandbox { /// This method is blocking. fn execute_in_vm( - mut self, + self, action: impl FnOnce(&mut Vm>, Transaction) -> T, + ) -> T { + self.execute_in_vm_with_tracer(action) + } + + fn execute_in_vm_with_tracer( + mut self, + action: impl FnOnce(&mut Vm, Tr>, Transaction) -> T, ) -> T { Self::setup_storage( &mut self.storage, From 603937be1156ac4d45735bd432c4f62bcde0e7f3 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Thu, 10 Oct 2024 13:30:09 +0200 Subject: [PATCH 06/46] clean up validation gas limit - even after out of gas, some calls are performed, so we have to assume that one out of gas anywhere means that is the reason for panic - reverts are considered successes in validation --- .../src/versions/vm_fast/validation_tracer.rs | 17 ++++++++--------- core/lib/vm_executor/src/oneshot/mod.rs | 19 ++++++++++++------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 71cc4cabccd1..638f88f91d1c 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -5,8 +5,8 @@ use zksync_vm2::interface::{ use super::vm::TracerExt; pub struct ValidationTracer { - pub probably_out_of_gas: bool, - pub in_validation: bool, + probably_out_of_gas: bool, + in_validation: bool, } impl Default for ValidationTracer { @@ -24,13 +24,6 @@ impl Tracer for ValidationTracer { return; } match OP::VALUE { - /* TODO this does not work because there is some Ret(Normal) before exit_validation - NearCall | FarCall(_) | Ret(Normal) => { - if self.probably_out_of_gas { - dbg!("out of gas canceled"); - } - self.probably_out_of_gas = false - }*/ Ret(Panic) if state.current_frame().gas() == 0 => self.probably_out_of_gas = true, _ => {} } @@ -46,3 +39,9 @@ impl TracerExt for ValidationTracer { self.in_validation = false; } } + +impl ValidationTracer { + pub fn probably_out_of_gas(&self) -> bool { + self.probably_out_of_gas + } +} diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index dd032202e0ad..7c16c4448578 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -220,13 +220,18 @@ where vm.push_transaction(transaction); let mut tracer = zksync_multivm::vm_fast::validation_tracer::ValidationTracer::default(); - vm.inspect(&mut tracer, InspectExecutionMode::OneTx); - if tracer.probably_out_of_gas { - Err(ValidationError::ViolatedRule( - ViolatedValidationRule::TookTooManyComputationalGas(1), - )) - } else { - Ok(()) + let result_and_logs = vm.inspect(&mut tracer, InspectExecutionMode::OneTx); + match result_and_logs.result { + ExecutionResult::Halt { reason } => { + if tracer.probably_out_of_gas() { + Err(ValidationError::ViolatedRule( + ViolatedValidationRule::TookTooManyComputationalGas(1), + )) + } else { + Err(ValidationError::FailedTx(reason)) + } + } + ExecutionResult::Revert { .. } | ExecutionResult::Success { .. } => Ok(()), } } From 1689cf238242ba53d4ce0af72df91e0935fbbdaa Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Thu, 10 Oct 2024 14:07:33 +0200 Subject: [PATCH 07/46] add comment explaining why revert is ok --- core/lib/vm_executor/src/oneshot/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 7c16c4448578..8386117e7a15 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -231,6 +231,7 @@ where Err(ValidationError::FailedTx(reason)) } } + // We don't check that the transaction is valid, just that validation doesn't break any rules. ExecutionResult::Revert { .. } | ExecutionResult::Success { .. } => Ok(()), } } From b4608ff8df1c6fdb92bd7d34de2b49dc6b10b693 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Thu, 10 Oct 2024 18:18:20 +0200 Subject: [PATCH 08/46] more general passing of hooks to tracers --- core/lib/multivm/src/versions/vm_fast/hook.rs | 2 +- .../src/versions/vm_fast/validation_tracer.rs | 12 ++++++------ core/lib/multivm/src/versions/vm_fast/vm.rs | 19 +++++++------------ 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/hook.rs b/core/lib/multivm/src/versions/vm_fast/hook.rs index c582bed7562f..b138c6d496d9 100644 --- a/core/lib/multivm/src/versions/vm_fast/hook.rs +++ b/core/lib/multivm/src/versions/vm_fast/hook.rs @@ -1,4 +1,4 @@ -#[derive(Debug)] +#[derive(Debug, Copy, Clone)] pub(crate) enum Hook { AccountValidationEntered, PaymasterValidationEntered, diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 638f88f91d1c..cf2f2f5e5295 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -31,12 +31,12 @@ impl Tracer for ValidationTracer { } impl TracerExt for ValidationTracer { - fn enter_validation(&mut self) { - self.in_validation = true; - } - - fn exit_validation(&mut self) { - self.in_validation = false; + fn on_bootloader_hook(&mut self, hook: super::hook::Hook) { + match hook { + super::hook::Hook::AccountValidationEntered => self.in_validation = true, + super::hook::Hook::ValidationExited => self.in_validation = false, + _ => {} + } } } diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 0e166b1616ba..f1b36f83b995 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -88,19 +88,14 @@ impl VmRunResult { } pub trait TracerExt: Tracer { - fn enter_validation(&mut self) {} - fn exit_validation(&mut self) {} + fn on_bootloader_hook(&mut self, #[allow(unused_variables)] hook: Hook) {} } impl TracerExt for () {} impl TracerExt for (A, B) { - fn enter_validation(&mut self) { - self.0.enter_validation(); - self.1.enter_validation(); - } - fn exit_validation(&mut self) { - self.0.exit_validation(); - self.1.exit_validation(); + fn on_bootloader_hook(&mut self, hook: Hook) { + self.0.on_bootloader_hook(hook); + self.1.on_bootloader_hook(hook); } } @@ -230,13 +225,14 @@ impl Vm { } }; - match Hook::from_u32(hook) { + let hook = Hook::from_u32(hook); + tracer.on_bootloader_hook(hook); + match hook { Hook::AccountValidationEntered => { assert!( account_validation_gas_split.is_none(), "Account validation can't be nested" ); - tracer.enter_validation(); let gas = self.gas_remaining(); let gas_given = gas.min(gas_left_for_account_validation); account_validation_gas_split = Some(AccountValidationGasSplit { @@ -247,7 +243,6 @@ impl Vm { } Hook::ValidationExited => { - tracer.exit_validation(); if let Some(AccountValidationGasSplit { gas_given, gas_hidden, From dbc17426566bbe68a8099ab37161de9b950ac77d Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 11 Oct 2024 13:30:46 +0200 Subject: [PATCH 09/46] stop validation run after validation --- core/lib/multivm/src/versions/vm_fast/vm.rs | 13 ++++++++++++- core/lib/vm_executor/src/oneshot/mod.rs | 9 ++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index f1b36f83b995..312e43dceaf6 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -111,6 +111,7 @@ pub struct Vm { pub(crate) batch_env: L1BatchEnv, pub(crate) system_env: SystemEnv, snapshot: Option, + stop_after_validation: bool, #[cfg(test)] enforced_state_diffs: Option>, } @@ -173,6 +174,7 @@ impl Vm { system_env, batch_env, snapshot: None, + stop_after_validation: false, #[cfg(test)] enforced_state_diffs: None, }; @@ -253,6 +255,11 @@ impl Vm { self.inner.current_frame().set_gas(gas_left + gas_hidden); } } + Hook::ValidationStepEnded => { + if self.stop_after_validation { + break (ExecutionResult::Success { output: vec![] }, false); + } + } Hook::TxHasEnded => { if let VmExecutionMode::OneTx = execution_mode { // The bootloader may invoke `TxHasEnded` hook without posting a tx result previously. One case when this can happen @@ -388,7 +395,7 @@ impl Vm { self.write_to_bootloader_heap(memory_to_apply); } - Hook::PaymasterValidationEntered | Hook::ValidationStepEnded => { /* unused */ } + Hook::PaymasterValidationEntered => { /* unused */ } Hook::DebugLog => { let (log, log_arg) = self.get_debug_log(); let last_tx = self.bootloader_state.last_l2_block().txs.last(); @@ -409,6 +416,10 @@ impl Vm { } } + pub fn stop_after_validation(&mut self) { + self.stop_after_validation = true; + } + fn get_hook_params(&self) -> [U256; 3] { (get_vm_hook_params_start_position(VM_VERSION) ..get_vm_hook_params_start_position(VM_VERSION) + VM_HOOK_PARAMS_COUNT) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 8386117e7a15..352ab709eed4 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -20,7 +20,7 @@ use zksync_multivm::{ storage::{ReadStorage, StorageView, StorageWithOverrides}, tracer::{ValidationError, ValidationParams, ViolatedValidationRule}, utils::{DivergenceHandler, ShadowVm}, - Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, + Call, ExecutionResult, Halt, InspectExecutionMode, OneshotEnv, OneshotTracingParams, OneshotTransactionExecutionResult, StoredL2BlockEnv, TxExecutionArgs, TxExecutionMode, VmFactory, VmInterface, }, @@ -217,6 +217,7 @@ where } Vm::Fast(FastVmInstance::Fast(vm)) => { + vm.stop_after_validation(); vm.push_transaction(transaction); let mut tracer = zksync_multivm::vm_fast::validation_tracer::ValidationTracer::default(); @@ -231,8 +232,10 @@ where Err(ValidationError::FailedTx(reason)) } } - // We don't check that the transaction is valid, just that validation doesn't break any rules. - ExecutionResult::Revert { .. } | ExecutionResult::Success { .. } => Ok(()), + ExecutionResult::Revert { output } => { + Err(ValidationError::FailedTx(Halt::ValidationFailed(output))) + } + ExecutionResult::Success { .. } => Ok(()), } } From 07a202ee47e32a6ff33334c520e87f8e50b2abe5 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 11 Oct 2024 18:20:02 +0200 Subject: [PATCH 10/46] make it clear that revert is not supposed to happen --- core/lib/vm_executor/src/oneshot/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 352ab709eed4..c3a95363bf31 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -233,7 +233,7 @@ where } } ExecutionResult::Revert { output } => { - Err(ValidationError::FailedTx(Halt::ValidationFailed(output))) + unreachable!("Revert can only happen at the end of a transaction") } ExecutionResult::Success { .. } => Ok(()), } From 31e05aef8e11a29aa64fb21a233f3b24f1657a19 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Tue, 22 Oct 2024 18:55:54 +0200 Subject: [PATCH 11/46] improve error message --- core/lib/contracts/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/lib/contracts/src/lib.rs b/core/lib/contracts/src/lib.rs index cb5be504c8a0..6ab90e1fbf30 100644 --- a/core/lib/contracts/src/lib.rs +++ b/core/lib/contracts/src/lib.rs @@ -210,7 +210,7 @@ pub fn l1_messenger_contract() -> Contract { /// Reads bytecode from the path RELATIVE to the Cargo workspace location. pub fn read_bytecode(relative_path: impl AsRef + std::fmt::Debug) -> Vec { - read_bytecode_from_path(relative_path).expect("Exists") + read_bytecode_from_path(relative_path).expect("Failed to open file") } pub fn eth_contract() -> Contract { From 3c96ad3a9a9d8726a6b4ac91f78105fb98de52fe Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Tue, 22 Oct 2024 19:00:44 +0200 Subject: [PATCH 12/46] first working test for account validation --- .../testonly/account_validation_rules.rs | 79 +++++++++++ core/lib/multivm/src/versions/testonly/mod.rs | 6 + .../src/versions/vm_fast/circuits_tracer.rs | 2 +- core/lib/multivm/src/versions/vm_fast/mod.rs | 2 +- .../vm_fast/tests/account_validation_rules.rs | 9 ++ .../multivm/src/versions/vm_fast/tests/mod.rs | 10 +- .../src/versions/vm_fast/validation_tracer.rs | 10 +- .../tests/account_validation_rules.rs | 8 ++ .../src/versions/vm_latest/tests/mod.rs | 1 + core/lib/multivm/src/vm_instance.rs | 8 +- core/lib/vm_executor/src/batch/factory.rs | 2 +- core/lib/vm_executor/src/oneshot/mod.rs | 4 +- core/tests/vm-benchmark/src/vm.rs | 3 +- .../validation-rule-breaker.sol | 130 ++++++++++++++++++ 14 files changed, 251 insertions(+), 23 deletions(-) create mode 100644 core/lib/multivm/src/versions/testonly/account_validation_rules.rs create mode 100644 core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs create mode 100644 core/lib/multivm/src/versions/vm_latest/tests/account_validation_rules.rs create mode 100644 etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs new file mode 100644 index 000000000000..9ce86f54dd1f --- /dev/null +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -0,0 +1,79 @@ +use zksync_types::{ + fee::Fee, l2::L2Tx, transaction_request::TransactionRequest, Address, Eip712Domain, L2ChainId, + Transaction, U256, +}; +use zksync_vm_interface::{ExecutionResult, Halt, VmRevertReason}; + +use super::{read_validation_test_contract, tester::VmTesterBuilder, ContractToDeploy, TestedVm}; +use crate::interface::{TxExecutionMode, VmExecutionMode, VmInterfaceExt}; + +/// Checks that every limitation imposed on account validation results in an appropriate error. +pub(crate) fn test_account_validation_rules() { + let aa_address = Address::repeat_byte(0x10); + let beneficiary_address = Address::repeat_byte(0x20); + + let bytecode = read_validation_test_contract(); + let mut vm = VmTesterBuilder::new() + .with_empty_in_memory_storage() + .with_custom_contracts(vec![ + ContractToDeploy::account(bytecode, aa_address).funded() + ]) + .with_execution_mode(TxExecutionMode::VerifyExecute) + .with_rich_accounts(1) + .build::(); + + let chain_id: u32 = 270; + let private_account = vm.rich_accounts[0].clone(); + + let tx_712 = L2Tx::new( + Some(beneficiary_address), + vec![], + private_account.nonce, + Fee { + gas_limit: U256::from(1000000000), + max_fee_per_gas: U256::from(1000000000), + max_priority_fee_per_gas: U256::from(1000000000), + gas_per_pubdata_limit: U256::from(1000000000), + }, + aa_address, + U256::from(28374938), + vec![], + Default::default(), + ); + + 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 + .get_pk_signer() + .sign_typed_data(&domain, &transaction_request) + .unwrap(); + let encoded_tx = transaction_request.get_signed_bytes(&signature).unwrap(); + + let (aa_txn_request, aa_hash) = + TransactionRequest::from_bytes(&encoded_tx, L2ChainId::from(chain_id)).unwrap(); + + let mut l2_tx = L2Tx::from_request(aa_txn_request, 100000, false).unwrap(); + l2_tx.set_input(encoded_tx, aa_hash); + + let transaction: Transaction = l2_tx.into(); + vm.vm.push_transaction(transaction); + let result = vm.vm.execute(VmExecutionMode::OneTx); + + assert!(result.result.is_failed()); + match result.result { + ExecutionResult::Halt { + reason: + Halt::ValidationFailed(VmRevertReason::Unknown { + function_selector: s, + data, + }), + } => { + s.into_iter().for_each(|x| print!("{:x}", x)); + println!(); + dbg!(data); + } + _ => {} + } +} diff --git a/core/lib/multivm/src/versions/testonly/mod.rs b/core/lib/multivm/src/versions/testonly/mod.rs index eece1d475bba..e024c6296bfd 100644 --- a/core/lib/multivm/src/versions/testonly/mod.rs +++ b/core/lib/multivm/src/versions/testonly/mod.rs @@ -33,6 +33,7 @@ use crate::{ vm_latest::constants::BATCH_COMPUTATIONAL_GAS_LIMIT, }; +pub(super) mod account_validation_rules; pub(super) mod block_tip; pub(super) mod bootloader; pub(super) mod bytecode_publishing; @@ -128,6 +129,11 @@ pub(crate) fn read_simple_transfer_contract() -> Vec { ) } +pub(crate) fn read_validation_test_contract() -> Vec { + let path = "etc/contracts-test-data/artifacts-zk/contracts/custom-account/validation-rule-breaker.sol/ValidationRuleBreaker.json"; + read_bytecode(path) +} + pub(crate) fn get_bootloader(test: &str) -> SystemContractCode { let bootloader_code = read_bootloader_code(test); let bootloader_hash = hash_bytecode(&bootloader_code); diff --git a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs b/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs index a1143738c1b5..e744b547be4b 100644 --- a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs @@ -4,7 +4,7 @@ use zksync_vm2::interface::{ }; use zksync_vm_interface::CircuitStatistic; -use super::Tracer as TracerExt; +use super::TracerExt; use crate::vm_latest::tracers::circuits_capacity::*; /// VM tracer tracking [`CircuitStatistic`]s. Statistics generally depend on the number of time some opcodes were invoked, diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index 12368f8d287e..c17614b56906 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -2,7 +2,7 @@ pub use zksync_vm2::interface; pub use self::{ circuits_tracer::CircuitsTracer, - vm::{TracerExt as Tracer, Vm}, + vm::{TracerExt, Vm}, }; mod bootloader_state; diff --git a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs new file mode 100644 index 000000000000..fb3a66570595 --- /dev/null +++ b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs @@ -0,0 +1,9 @@ +use crate::{ + versions::testonly::account_validation_rules::test_account_validation_rules, + vm_fast::{validation_tracer::ValidationTracer, Vm}, +}; + +#[test] +fn test_account_validation_rules_fast() { + test_account_validation_rules::>(); +} diff --git a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs index 2b4665f82241..db7e7e226f86 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -8,13 +8,13 @@ use zksync_vm_interface::{ VmExecutionMode, VmExecutionResultAndLogs, VmInterface, }; -use super::Vm; +use super::{TracerExt, Vm}; use crate::{ interface::storage::{ImmutableStorageView, InMemoryStorage}, versions::testonly::TestedVm, - vm_fast::CircuitsTracer, }; +mod account_validation_rules; mod block_tip; mod bootloader; mod bytecode_publishing; @@ -76,7 +76,9 @@ impl PartialEq for VmStateDump { } } -impl TestedVm for Vm> { +impl TestedVm + for Vm, T> +{ type StateDump = VmStateDump; fn dump_state(&self) -> Self::StateDump { @@ -124,7 +126,7 @@ impl TestedVm for Vm> { fn manually_decommit(&mut self, code_hash: H256) -> bool { let (_, is_fresh) = self.inner.world_diff_mut().decommit_opcode( &mut self.world, - &mut ((), CircuitsTracer::default()), + &mut Default::default(), h256_to_u256(code_hash), ); is_fresh diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index cf2f2f5e5295..426b96d0c44c 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -4,20 +4,12 @@ use zksync_vm2::interface::{ use super::vm::TracerExt; +#[derive(Debug, Default)] pub struct ValidationTracer { probably_out_of_gas: bool, in_validation: bool, } -impl Default for ValidationTracer { - fn default() -> Self { - Self { - probably_out_of_gas: false, - in_validation: false, - } - } -} - impl Tracer for ValidationTracer { fn before_instruction(&mut self, state: &mut S) { if !self.in_validation { diff --git a/core/lib/multivm/src/versions/vm_latest/tests/account_validation_rules.rs b/core/lib/multivm/src/versions/vm_latest/tests/account_validation_rules.rs new file mode 100644 index 000000000000..8ee6c06c1c69 --- /dev/null +++ b/core/lib/multivm/src/versions/vm_latest/tests/account_validation_rules.rs @@ -0,0 +1,8 @@ +use crate::{ + versions::testonly::account_validation_rules::test_account_validation_rules, vm_latest::Vm, +}; + +#[test] +fn test_account_validation_rules_legacy() { + test_account_validation_rules::>(); +} diff --git a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs index 96d59f208b03..99d849564afe 100644 --- a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs @@ -33,6 +33,7 @@ mod bootloader; mod default_aa; // TODO - fix this test // `mod invalid_bytecode;` +mod account_validation_rules; mod block_tip; mod bytecode_publishing; mod call_tracer; diff --git a/core/lib/multivm/src/vm_instance.rs b/core/lib/multivm/src/vm_instance.rs index 2880bd24632a..e0a5437a27df 100644 --- a/core/lib/multivm/src/vm_instance.rs +++ b/core/lib/multivm/src/vm_instance.rs @@ -13,7 +13,7 @@ use crate::{ VmMemoryMetrics, }, tracers::TracerDispatcher, - vm_fast::Tracer, + vm_fast::TracerExt, vm_latest::HistoryEnabled, }; @@ -250,7 +250,7 @@ macro_rules! dispatch_fast_vm { }; } -impl VmInterface +impl VmInterface for FastVmInstance { type TracerDispatcher = ( @@ -300,7 +300,7 @@ impl VmInterface } } -impl VmInterfaceHistoryEnabled +impl VmInterfaceHistoryEnabled for FastVmInstance { fn make_snapshot(&mut self) { @@ -316,7 +316,7 @@ impl VmInterfaceHistoryEnabled } } -impl FastVmInstance { +impl FastVmInstance { /// Creates an isolated fast VM. pub fn fast( l1_batch_env: L1BatchEnv, diff --git a/core/lib/vm_executor/src/batch/factory.rs b/core/lib/vm_executor/src/batch/factory.rs index de0db5f0bf75..b010f8bf55cf 100644 --- a/core/lib/vm_executor/src/batch/factory.rs +++ b/core/lib/vm_executor/src/batch/factory.rs @@ -37,7 +37,7 @@ pub trait BatchTracer: fmt::Debug + 'static + Send + Sealed { const TRACE_CALLS: bool; /// Tracer for the fast VM. #[doc(hidden)] - type Fast: vm_fast::interface::Tracer + Default + 'static; + type Fast: vm_fast::TracerExt + Default + 'static; } impl Sealed for () {} diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index c3a95363bf31..ab142a429b79 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -27,7 +27,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, tracers::{CallTracer, StorageInvocations, TracerDispatcher, ValidationTracer}, utils::adjust_pubdata_price_for_tx, - vm_fast::Tracer, + vm_fast::TracerExt, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, FastVmInstance, HistoryMode, LegacyVmInstance, MultiVMTracer, @@ -379,7 +379,7 @@ impl VmSandbox { self.execute_in_vm_with_tracer(action) } - fn execute_in_vm_with_tracer( + fn execute_in_vm_with_tracer( mut self, action: impl FnOnce(&mut Vm, Tr>, Transaction) -> T, ) -> T { diff --git a/core/tests/vm-benchmark/src/vm.rs b/core/tests/vm-benchmark/src/vm.rs index bf969e0de5c0..b1c8bf7b5e06 100644 --- a/core/tests/vm-benchmark/src/vm.rs +++ b/core/tests/vm-benchmark/src/vm.rs @@ -9,7 +9,7 @@ use zksync_multivm::{ VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceExt, VmInterfaceHistoryEnabled, }, - vm_fast, + vm_fast::{self, TracerExt}, vm_latest::{self, constants::BATCH_COMPUTATIONAL_GAS_LIMIT, HistoryEnabled, ToTracerPointer}, zk_evm_latest::ethereum_types::{Address, U256}, }; @@ -112,6 +112,7 @@ impl CountInstructions for Fast { self.0 += 1; } } + impl TracerExt for InstructionCount {} let (system_env, l1_batch_env) = test_env(); let mut vm = diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol new file mode 100644 index 000000000000..0c5b3a2dd03c --- /dev/null +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 + +pragma solidity ^0.8.0; + +import "./Constants.sol"; +import "./TransactionHelper.sol"; + +import "./SystemContractsCaller.sol"; + +import "./interfaces/IAccount.sol"; + +contract ValidationRuleBreaker is IAccount { + using TransactionHelper for Transaction; + + uint32 public validationRulesBroken; + + constructor() { + validationRulesBroken = 0; + } + + function validateTransaction( + bytes32 _txHash, + bytes32 _suggestedSignedTxHash, + Transaction calldata _transaction + ) external payable override returns (bytes4 magic) { + // By default we consider the transaction as successful + magic = VALIDATION_SUCCESS_MAGIC; + + if (validationRulesBroken == 0) { + // Such a tx should not pass the validation step, because it depends on the balance of another account + require( + BOOTLOADER_FORMAL_ADDRESS.balance == 0, + "Bootloader balance must be zero" + ); + } + + validationRulesBroken += 1; + + _validateTransaction(_suggestedSignedTxHash, _transaction); + } + + function _validateTransaction( + bytes32 _suggestedSignedTxHash, + Transaction calldata _transaction + ) internal { + SystemContractsCaller.systemCallWithPropagatedRevert( + uint32(gasleft()), + address(NONCE_HOLDER_SYSTEM_CONTRACT), + 0, + abi.encodeCall( + INonceHolder.incrementMinNonceIfEquals, + (_transaction.nonce) + ) + ); + } + + function executeTransaction( + bytes32, + bytes32, + Transaction calldata _transaction + ) external payable override { + _execute(_transaction); + } + + function executeTransactionFromOutside( + Transaction calldata _transaction + ) external payable override { + _validateTransaction(bytes32(0), _transaction); + _execute(_transaction); + } + + function _execute(Transaction calldata _transaction) internal { + address to = address(uint160(_transaction.to)); + uint256 value = _transaction.reserved[1]; + bytes memory data = _transaction.data; + + if (to == address(DEPLOYER_SYSTEM_CONTRACT)) { + // We allow calling ContractDeployer with any calldata + SystemContractsCaller.systemCallWithPropagatedRevert( + uint32(gasleft()), + to, + uint128(_transaction.reserved[1]), // By convention, reserved[1] is `value` + _transaction.data + ); + } else { + bool success; + assembly { + success := call( + gas(), + to, + value, + add(data, 0x20), + mload(data), + 0, + 0 + ) + } + require(success); + } + } + + // Here, the user pays the bootloader for the transaction + function payForTransaction( + bytes32, + bytes32, + Transaction calldata _transaction + ) external payable { + bool success = _transaction.payToTheBootloader(); + require(success, "Failed to pay the fee to the operator"); + } + + // Here, the user should prepare for the transaction to be paid for by a paymaster + // Here, the account should set the allowance for the smart contracts + function prepareForPaymaster( + bytes32, + bytes32, + Transaction calldata _transaction + ) external payable { + _transaction.processPaymasterInput(); + } + + fallback() external payable { + // fallback of default AA shouldn't be called by bootloader under no circumstances + assert(msg.sender != BOOTLOADER_FORMAL_ADDRESS); + + // If the contract is called directly, behave like an EOA + } + + receive() external payable {} +} From 79b33023e3fc53ae1ae678d026a6d5df99832f9e Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 23 Oct 2024 14:11:01 +0200 Subject: [PATCH 13/46] framework for testing every validation rule violation is done --- .../testonly/account_validation_rules.rs | 69 +++++++++++-------- core/lib/multivm/src/versions/testonly/mod.rs | 6 +- .../src/versions/testonly/tester/mod.rs | 9 ++- .../multivm/src/versions/vm_fast/tests/mod.rs | 23 +++++-- .../src/versions/vm_fast/validation_tracer.rs | 16 ++++- .../src/versions/vm_latest/tests/mod.rs | 41 ++++++++++- core/lib/vm_executor/src/oneshot/mod.rs | 4 +- .../validation-rule-breaker.sol | 14 ++-- 8 files changed, 131 insertions(+), 51 deletions(-) diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs index 9ce86f54dd1f..a5aa3bea2c2f 100644 --- a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -1,18 +1,31 @@ +use assert_matches::assert_matches; +use ethabi::Token; use zksync_types::{ - fee::Fee, l2::L2Tx, transaction_request::TransactionRequest, Address, Eip712Domain, L2ChainId, - Transaction, U256, + fee::Fee, l2::L2Tx, transaction_request::TransactionRequest, Address, Eip712Domain, Execute, + L2ChainId, U256, }; -use zksync_vm_interface::{ExecutionResult, Halt, VmRevertReason}; +use zksync_vm_interface::{tracer::ViolatedValidationRule, InspectExecutionMode, VmInterfaceExt}; -use super::{read_validation_test_contract, tester::VmTesterBuilder, ContractToDeploy, TestedVm}; -use crate::interface::{TxExecutionMode, VmExecutionMode, VmInterfaceExt}; +use super::{ + read_validation_test_contract, tester::VmTesterBuilder, ContractToDeploy, TestedVm, + TestedVmForValidation, +}; +use crate::interface::TxExecutionMode; /// Checks that every limitation imposed on account validation results in an appropriate error. -pub(crate) fn test_account_validation_rules() { +pub(crate) fn test_account_validation_rules() { + assert_matches!(test_rule::(0), None); + assert_matches!( + test_rule::(1), + Some(ViolatedValidationRule::TouchedDisallowedStorageSlots(_, _)) + ); +} + +fn test_rule(rule: u32) -> Option { let aa_address = Address::repeat_byte(0x10); let beneficiary_address = Address::repeat_byte(0x20); - let bytecode = read_validation_test_contract(); + let (bytecode, contract) = read_validation_test_contract(); let mut vm = VmTesterBuilder::new() .with_empty_in_memory_storage() .with_custom_contracts(vec![ @@ -22,8 +35,28 @@ pub(crate) fn test_account_validation_rules() { .with_rich_accounts(1) .build::(); + let mut private_account = vm.rich_accounts[0].clone(); + + // Set the type of misbehaviour of the AA contract + let function = contract.function("setTypeOfRuleBreak").unwrap(); + let transaction = private_account.get_l2_tx_for_execute( + Execute { + contract_address: Some(aa_address), + calldata: function.encode_input(&[Token::Uint(rule.into())]).unwrap(), + value: U256::zero(), + factory_deps: vec![], + }, + None, + ); + vm.vm.push_transaction(transaction); + assert!(!vm + .vm + .execute(InspectExecutionMode::OneTx) + .result + .is_failed()); + + // Use account abstraction let chain_id: u32 = 270; - let private_account = vm.rich_accounts[0].clone(); let tx_712 = L2Tx::new( Some(beneficiary_address), @@ -57,23 +90,5 @@ pub(crate) fn test_account_validation_rules() { let mut l2_tx = L2Tx::from_request(aa_txn_request, 100000, false).unwrap(); l2_tx.set_input(encoded_tx, aa_hash); - let transaction: Transaction = l2_tx.into(); - vm.vm.push_transaction(transaction); - let result = vm.vm.execute(VmExecutionMode::OneTx); - - assert!(result.result.is_failed()); - match result.result { - ExecutionResult::Halt { - reason: - Halt::ValidationFailed(VmRevertReason::Unknown { - function_selector: s, - data, - }), - } => { - s.into_iter().for_each(|x| print!("{:x}", x)); - println!(); - dbg!(data); - } - _ => {} - } + vm.vm.run_validation(l2_tx) } diff --git a/core/lib/multivm/src/versions/testonly/mod.rs b/core/lib/multivm/src/versions/testonly/mod.rs index e024c6296bfd..522969671433 100644 --- a/core/lib/multivm/src/versions/testonly/mod.rs +++ b/core/lib/multivm/src/versions/testonly/mod.rs @@ -27,7 +27,7 @@ use zksync_vm_interface::{ pubdata::PubdataBuilder, L1BatchEnv, L2BlockEnv, SystemEnv, TxExecutionMode, }; -pub(super) use self::tester::{TestedVm, VmTester, VmTesterBuilder}; +pub(super) use self::tester::{TestedVm, TestedVmForValidation, VmTester, VmTesterBuilder}; use crate::{ interface::storage::InMemoryStorage, pubdata_builders::RollupPubdataBuilder, vm_latest::constants::BATCH_COMPUTATIONAL_GAS_LIMIT, @@ -129,9 +129,9 @@ pub(crate) fn read_simple_transfer_contract() -> Vec { ) } -pub(crate) fn read_validation_test_contract() -> Vec { +pub(crate) fn read_validation_test_contract() -> (Vec, Contract) { let path = "etc/contracts-test-data/artifacts-zk/contracts/custom-account/validation-rule-breaker.sol/ValidationRuleBreaker.json"; - read_bytecode(path) + (read_bytecode(path), load_contract(path)) } pub(crate) fn get_bootloader(test: &str) -> SystemContractCode { diff --git a/core/lib/multivm/src/versions/testonly/tester/mod.rs b/core/lib/multivm/src/versions/testonly/tester/mod.rs index 716b9386235f..41355ad6deae 100644 --- a/core/lib/multivm/src/versions/testonly/tester/mod.rs +++ b/core/lib/multivm/src/versions/testonly/tester/mod.rs @@ -3,13 +3,14 @@ use std::{collections::HashSet, fmt, rc::Rc}; use zksync_contracts::BaseSystemContracts; use zksync_test_account::{Account, TxType}; use zksync_types::{ + l2::L2Tx, utils::{deployed_address_create, storage_key_for_eth_balance}, writes::StateDiffRecord, Address, L1BatchNumber, StorageKey, Transaction, H256, U256, }; use zksync_vm_interface::{ - pubdata::PubdataBuilder, CurrentExecutionState, InspectExecutionMode, VmExecutionResultAndLogs, - VmInterfaceHistoryEnabled, + pubdata::PubdataBuilder, tracer::ViolatedValidationRule, CurrentExecutionState, + InspectExecutionMode, VmExecutionResultAndLogs, VmInterfaceHistoryEnabled, }; pub(crate) use self::transaction_test_info::{ExpectedError, TransactionTestInfo, TxModifier}; @@ -229,3 +230,7 @@ pub(crate) trait TestedVm: /// Pushes a transaction with predefined refund value. fn push_transaction_with_refund(&mut self, tx: Transaction, refund: u64); } + +pub(crate) trait TestedVmForValidation { + fn run_validation(&mut self, tx: L2Tx) -> Option; +} diff --git a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs index db7e7e226f86..ae329da6b9f6 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -1,17 +1,18 @@ use std::{any::Any, collections::HashSet, fmt, rc::Rc}; -use zksync_types::{writes::StateDiffRecord, StorageKey, Transaction, H160, H256, U256}; +use zksync_types::{l2::L2Tx, writes::StateDiffRecord, StorageKey, Transaction, H160, H256, U256}; use zksync_utils::h256_to_u256; use zksync_vm2::interface::{Event, HeapId, StateInterface}; use zksync_vm_interface::{ - pubdata::PubdataBuilder, storage::ReadStorage, CurrentExecutionState, L2BlockEnv, - VmExecutionMode, VmExecutionResultAndLogs, VmInterface, + pubdata::PubdataBuilder, storage::ReadStorage, tracer::ViolatedValidationRule, + CurrentExecutionState, InspectExecutionMode, L2BlockEnv, VmExecutionMode, + VmExecutionResultAndLogs, VmInterface, }; -use super::{TracerExt, Vm}; +use super::{validation_tracer::ValidationTracer, TracerExt, Vm}; use crate::{ interface::storage::{ImmutableStorageView, InMemoryStorage}, - versions::testonly::TestedVm, + versions::testonly::{TestedVm, TestedVmForValidation}, }; mod account_validation_rules; @@ -164,3 +165,15 @@ impl TestedVm self.push_transaction_inner(tx, refund, true); } } + +impl TestedVmForValidation for Vm, ValidationTracer> { + fn run_validation(&mut self, tx: L2Tx) -> Option { + self.push_transaction(tx.into()); + + let mut tracer = ValidationTracer::default(); + self.stop_after_validation(); + self.inspect(&mut tracer, InspectExecutionMode::OneTx); + + tracer.validation_error() + } +} diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 426b96d0c44c..b859f492aaef 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -1,12 +1,13 @@ use zksync_vm2::interface::{ self, CallframeInterface, Opcode::*, OpcodeType, ReturnType::*, Tracer, }; +use zksync_vm_interface::tracer::ViolatedValidationRule; use super::vm::TracerExt; #[derive(Debug, Default)] pub struct ValidationTracer { - probably_out_of_gas: bool, + out_of_gas: bool, in_validation: bool, } @@ -16,7 +17,8 @@ impl Tracer for ValidationTracer { return; } match OP::VALUE { - Ret(Panic) if state.current_frame().gas() == 0 => self.probably_out_of_gas = true, + // Out of gas once means out of gas for the whole validation, as the EIP forbids handling out of gas errors + Ret(Panic) if state.current_frame().gas() == 0 => self.out_of_gas = true, _ => {} } } @@ -34,6 +36,14 @@ impl TracerExt for ValidationTracer { impl ValidationTracer { pub fn probably_out_of_gas(&self) -> bool { - self.probably_out_of_gas + self.out_of_gas + } + + pub fn validation_error(&self) -> Option { + if self.out_of_gas { + Some(ViolatedValidationRule::TookTooManyComputationalGas(0)) + } else { + None + } } } diff --git a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs index 99d849564afe..07550a11591b 100644 --- a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs @@ -1,6 +1,7 @@ use std::{ collections::{HashMap, HashSet}, rc::Rc, + sync::Arc, }; use zk_evm_1_5_0::{ @@ -8,17 +9,25 @@ use zk_evm_1_5_0::{ vm_state::VmLocalState, zkevm_opcode_defs::{ContractCodeSha256Format, VersionedHashLen32}, }; -use zksync_types::{writes::StateDiffRecord, StorageKey, StorageValue, Transaction, H256, U256}; +use zksync_types::{ + l2::L2Tx, vm::VmVersion, writes::StateDiffRecord, StorageKey, StorageValue, Transaction, H256, + U256, +}; use zksync_utils::{bytecode::hash_bytecode, bytes_to_be_words, h256_to_u256}; use zksync_vm_interface::pubdata::PubdataBuilder; +use zksync_vm_interface::{ + tracer::{ValidationParams, ViolatedValidationRule}, + VmInterface, +}; -use super::{HistoryEnabled, Vm}; +use super::{HistoryEnabled, ToTracerPointer, Vm}; use crate::{ interface::{ storage::{InMemoryStorage, ReadStorage, StorageView, WriteStorage}, CurrentExecutionState, L2BlockEnv, VmExecutionMode, VmExecutionResultAndLogs, }, - versions::testonly::{filter_out_base_system_contracts, TestedVm}, + tracers::ValidationTracer, + versions::testonly::{filter_out_base_system_contracts, TestedVm, TestedVmForValidation}, vm_latest::{ constants::BOOTLOADER_HEAP_PAGE, old_vm::{event_sink::InMemoryEventSink, history_recorder::HistoryRecorder}, @@ -188,6 +197,32 @@ impl TestedVm for TestedLatestVm { } } +impl TestedVmForValidation for TestedLatestVm { + fn run_validation(&mut self, tx: L2Tx) -> Option { + let user_address = tx.common_data.initiator_address; + let paymaster_address = tx.common_data.paymaster_params.paymaster; + self.push_transaction(tx.into()); + + let (tracer, mut failures) = ValidationTracer::::new( + ValidationParams { + user_address, + paymaster_address, + trusted_slots: Default::default(), + trusted_addresses: Default::default(), + trusted_address_slots: Default::default(), + computational_gas_limit: 100_000, + }, + VmVersion::Vm1_5_0IncreasedBootloaderMemory, + ); + self.inspect_inner( + &mut tracer.into_tracer_pointer().into(), + VmExecutionMode::OneTx, + None, + ); + Arc::make_mut(&mut failures).take() + } +} + #[derive(Clone, Debug)] pub(crate) struct ModifiedKeysMap(HashMap); diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index ab142a429b79..23443f2bfe70 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -20,7 +20,7 @@ use zksync_multivm::{ storage::{ReadStorage, StorageView, StorageWithOverrides}, tracer::{ValidationError, ValidationParams, ViolatedValidationRule}, utils::{DivergenceHandler, ShadowVm}, - Call, ExecutionResult, Halt, InspectExecutionMode, OneshotEnv, OneshotTracingParams, + Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, OneshotTransactionExecutionResult, StoredL2BlockEnv, TxExecutionArgs, TxExecutionMode, VmFactory, VmInterface, }, @@ -232,7 +232,7 @@ where Err(ValidationError::FailedTx(reason)) } } - ExecutionResult::Revert { output } => { + ExecutionResult::Revert { .. } => { unreachable!("Revert can only happen at the end of a transaction") } ExecutionResult::Success { .. } => Ok(()), diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol index 0c5b3a2dd03c..6c0e0af9b148 100644 --- a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -12,10 +12,14 @@ import "./interfaces/IAccount.sol"; contract ValidationRuleBreaker is IAccount { using TransactionHelper for Transaction; - uint32 public validationRulesBroken; + uint32 public typeOfRuleBreak; constructor() { - validationRulesBroken = 0; + typeOfRuleBreak = 0; + } + + function setTypeOfRuleBreak(uint32 _typeOfRuleBreak) external { + typeOfRuleBreak = _typeOfRuleBreak; } function validateTransaction( @@ -26,15 +30,13 @@ contract ValidationRuleBreaker is IAccount { // By default we consider the transaction as successful magic = VALIDATION_SUCCESS_MAGIC; - if (validationRulesBroken == 0) { + if (typeOfRuleBreak == 1) { // Such a tx should not pass the validation step, because it depends on the balance of another account require( BOOTLOADER_FORMAL_ADDRESS.balance == 0, "Bootloader balance must be zero" ); - } - - validationRulesBroken += 1; + } else if (typeOfRuleBreak == 2) {} _validateTransaction(_suggestedSignedTxHash, _transaction); } From 0878b3bfc050a1095d33a82ca30836a6ac6925ba Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 23 Oct 2024 16:20:48 +0200 Subject: [PATCH 14/46] long comment explaining validation rules' purpose --- .../src/versions/vm_fast/validation_tracer.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index b859f492aaef..008dd22846e5 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -5,6 +5,30 @@ use zksync_vm_interface::tracer::ViolatedValidationRule; use super::vm::TracerExt; +/// Account abstraction exposes a chain to denial of service attacks because someone who fails to +/// authenticate does not pay for the failed transaction. Otherwise people could empty other's +/// wallets for free! +/// +/// If some address repeatedly posts transactions that validate during preliminary checks but fail +/// to validate during the actual execution, that address is considered a spammer. However, when +/// the spam comes from multiple addresses, that doesn't work. +/// +/// We want to ensure that a spammer has to pay for every account that fails validation. This is +/// achieved by limiting what the code of a custom account is allowed to do. If we allowed access +/// to things like time, a validation that fails in the sequencer could be crafted for free, so we +/// don't. +/// +/// However, we want to give access to storage. A spammer has to pay for changing storage but +/// could change just one storage slot to invalidate transactions from many accounts. To prevent +/// that, we make sure that the storage slots accessed by different accounts are disjoint by only +/// allowing access to storage in the account itself and slots derived from the account's address. +/// +/// Our rules are an extension of the rules are outlined in EIP-7562. +/// +/// This tracer enforces the rules by checking what the code does at runtime, even though the +/// properties checked are supposed to always hold for a well-written custom account. Proving +/// that a contract adheres to the rules ahead of time would be challenging or even impossible, +/// considering that contracts that the code depends on may get upgraded. #[derive(Debug, Default)] pub struct ValidationTracer { out_of_gas: bool, From 25a37571835be436566f2a418195f5bfbf015121 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 23 Oct 2024 19:07:57 +0200 Subject: [PATCH 15/46] prohibit gasleft --- .../testonly/account_validation_rules.rs | 5 ++++ .../src/versions/vm_fast/validation_tracer.rs | 27 ++++++++++++------- core/lib/vm_executor/src/oneshot/mod.rs | 15 ++++------- .../validation-rule-breaker.sol | 13 ++++----- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs index a5aa3bea2c2f..ee131288c2d5 100644 --- a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -13,12 +13,17 @@ use super::{ use crate::interface::TxExecutionMode; /// Checks that every limitation imposed on account validation results in an appropriate error. +/// The actual misbehaviours are found in "validation-rule-breaker.sol". pub(crate) fn test_account_validation_rules() { assert_matches!(test_rule::(0), None); assert_matches!( test_rule::(1), Some(ViolatedValidationRule::TouchedDisallowedStorageSlots(_, _)) ); + assert_matches!( + test_rule::(2), + Some(ViolatedValidationRule::TouchedDisallowedContext) + ); } fn test_rule(rule: u32) -> Option { diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 008dd22846e5..5ec4914742b4 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -31,8 +31,9 @@ use super::vm::TracerExt; /// considering that contracts that the code depends on may get upgraded. #[derive(Debug, Default)] pub struct ValidationTracer { - out_of_gas: bool, in_validation: bool, + validation_error: Option, + previous_instruction_was_gasleft: bool, } impl Tracer for ValidationTracer { @@ -40,9 +41,19 @@ impl Tracer for ValidationTracer { if !self.in_validation { return; } + if self.previous_instruction_was_gasleft { + match OP::VALUE { + // The far call wipes the register, so there is no way the gas number can be used for anything else + FarCall(_) => {} + _ => self.set_error(ViolatedValidationRule::TouchedDisallowedContext), + } + self.previous_instruction_was_gasleft = false; + } match OP::VALUE { // Out of gas once means out of gas for the whole validation, as the EIP forbids handling out of gas errors - Ret(Panic) if state.current_frame().gas() == 0 => self.out_of_gas = true, + Ret(Panic) if state.current_frame().gas() == 0 => { + self.set_error(ViolatedValidationRule::TookTooManyComputationalGas(0)) + } _ => {} } } @@ -59,15 +70,13 @@ impl TracerExt for ValidationTracer { } impl ValidationTracer { - pub fn probably_out_of_gas(&self) -> bool { - self.out_of_gas + fn set_error(&mut self, error: ViolatedValidationRule) { + if self.validation_error.is_none() { + self.validation_error = Some(error); + } } pub fn validation_error(&self) -> Option { - if self.out_of_gas { - Some(ViolatedValidationRule::TookTooManyComputationalGas(0)) - } else { - None - } + self.validation_error.clone() } } diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 23443f2bfe70..fcf1104c5ba4 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -18,7 +18,7 @@ use zksync_multivm::{ interface::{ executor::{OneshotExecutor, TransactionValidator}, storage::{ReadStorage, StorageView, StorageWithOverrides}, - tracer::{ValidationError, ValidationParams, ViolatedValidationRule}, + tracer::{ValidationError, ValidationParams}, utils::{DivergenceHandler, ShadowVm}, Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, OneshotTransactionExecutionResult, StoredL2BlockEnv, TxExecutionArgs, TxExecutionMode, @@ -222,16 +222,11 @@ where let mut tracer = zksync_multivm::vm_fast::validation_tracer::ValidationTracer::default(); let result_and_logs = vm.inspect(&mut tracer, InspectExecutionMode::OneTx); + if let Some(violation) = tracer.validation_error() { + return Err(ValidationError::ViolatedRule(violation)); + } match result_and_logs.result { - ExecutionResult::Halt { reason } => { - if tracer.probably_out_of_gas() { - Err(ValidationError::ViolatedRule( - ViolatedValidationRule::TookTooManyComputationalGas(1), - )) - } else { - Err(ValidationError::FailedTx(reason)) - } - } + ExecutionResult::Halt { reason } => Err(ValidationError::FailedTx(reason)), ExecutionResult::Revert { .. } => { unreachable!("Revert can only happen at the end of a transaction") } diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol index 6c0e0af9b148..4a40c0f88a1b 100644 --- a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -31,12 +31,13 @@ contract ValidationRuleBreaker is IAccount { magic = VALIDATION_SUCCESS_MAGIC; if (typeOfRuleBreak == 1) { - // Such a tx should not pass the validation step, because it depends on the balance of another account - require( - BOOTLOADER_FORMAL_ADDRESS.balance == 0, - "Bootloader balance must be zero" - ); - } else if (typeOfRuleBreak == 2) {} + // The balance of another account may not be read + // I'm writing assertions because otherwise the compiler would optimize away the access + require(BOOTLOADER_FORMAL_ADDRESS.balance != 0); + } else if (typeOfRuleBreak == 2) { + // Gas may only be queried to pass everything into a far call + gasleft(); + } _validateTransaction(_suggestedSignedTxHash, _transaction); } From 7b837aac2e512197af137d0d8acef2e49a8d0d6d Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 25 Oct 2024 21:16:11 +0200 Subject: [PATCH 16/46] pass tests --- .../testonly/account_validation_rules.rs | 12 +- core/lib/multivm/src/versions/testonly/mod.rs | 4 +- .../src/versions/testonly/tester/mod.rs | 19 ++- core/lib/multivm/src/versions/vm_fast/mod.rs | 1 + .../multivm/src/versions/vm_fast/tests/mod.rs | 5 +- .../lib/multivm/src/versions/vm_fast/utils.rs | 13 ++ .../src/versions/vm_fast/validation_tracer.rs | 136 ++++++++++++++++-- core/lib/multivm/src/versions/vm_fast/vm.rs | 3 + .../src/versions/vm_latest/tests/mod.rs | 18 +-- .../validation-rule-breaker.sol | 3 + 10 files changed, 183 insertions(+), 31 deletions(-) create mode 100644 core/lib/multivm/src/versions/vm_fast/utils.rs diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs index ee131288c2d5..5d2a4f835b50 100644 --- a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -22,8 +22,18 @@ pub(crate) fn test_account_validation_rules(2), - Some(ViolatedValidationRule::TouchedDisallowedContext) + Some(ViolatedValidationRule::CalledContractWithNoCode(_)) ); + + // Disallowing gasleft is difficult because it isn't immediately followed + // by far call in EraVM bytecode. + /*assert_matches!( + test_rule::(100), + Some(ViolatedValidationRule::TouchedDisallowedContext) + );*/ + + // TODO: test running out of gas but catching the failure. + // Can be accomplished via many nested far calls. } fn test_rule(rule: u32) -> Option { diff --git a/core/lib/multivm/src/versions/testonly/mod.rs b/core/lib/multivm/src/versions/testonly/mod.rs index 522969671433..b46d687c4612 100644 --- a/core/lib/multivm/src/versions/testonly/mod.rs +++ b/core/lib/multivm/src/versions/testonly/mod.rs @@ -27,7 +27,9 @@ use zksync_vm_interface::{ pubdata::PubdataBuilder, L1BatchEnv, L2BlockEnv, SystemEnv, TxExecutionMode, }; -pub(super) use self::tester::{TestedVm, TestedVmForValidation, VmTester, VmTesterBuilder}; +pub(super) use self::tester::{ + validation_params, TestedVm, TestedVmForValidation, VmTester, VmTesterBuilder, +}; use crate::{ interface::storage::InMemoryStorage, pubdata_builders::RollupPubdataBuilder, vm_latest::constants::BATCH_COMPUTATIONAL_GAS_LIMIT, diff --git a/core/lib/multivm/src/versions/testonly/tester/mod.rs b/core/lib/multivm/src/versions/testonly/tester/mod.rs index 41355ad6deae..2901b6251b90 100644 --- a/core/lib/multivm/src/versions/testonly/tester/mod.rs +++ b/core/lib/multivm/src/versions/testonly/tester/mod.rs @@ -9,8 +9,10 @@ use zksync_types::{ Address, L1BatchNumber, StorageKey, Transaction, H256, U256, }; use zksync_vm_interface::{ - pubdata::PubdataBuilder, tracer::ViolatedValidationRule, CurrentExecutionState, - InspectExecutionMode, VmExecutionResultAndLogs, VmInterfaceHistoryEnabled, + pubdata::PubdataBuilder, + tracer::{ValidationParams, ViolatedValidationRule}, + CurrentExecutionState, InspectExecutionMode, VmExecutionResultAndLogs, + VmInterfaceHistoryEnabled, }; pub(crate) use self::transaction_test_info::{ExpectedError, TransactionTestInfo, TxModifier}; @@ -234,3 +236,16 @@ pub(crate) trait TestedVm: pub(crate) trait TestedVmForValidation { fn run_validation(&mut self, tx: L2Tx) -> Option; } + +pub(crate) fn validation_params(tx: &L2Tx, system: &SystemEnv) -> ValidationParams { + let user_address = tx.common_data.initiator_address; + let paymaster_address = tx.common_data.paymaster_params.paymaster; + ValidationParams { + user_address, + paymaster_address, + trusted_slots: Default::default(), + trusted_addresses: Default::default(), + trusted_address_slots: Default::default(), + computational_gas_limit: system.default_validation_computational_gas_limit, + } +} diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index c17614b56906..8355f19142b6 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -17,5 +17,6 @@ mod refund; #[cfg(test)] mod tests; mod transaction_data; +mod utils; pub mod validation_tracer; mod vm; diff --git a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs index ae329da6b9f6..1c4264bf3b6b 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -12,7 +12,7 @@ use zksync_vm_interface::{ use super::{validation_tracer::ValidationTracer, TracerExt, Vm}; use crate::{ interface::storage::{ImmutableStorageView, InMemoryStorage}, - versions::testonly::{TestedVm, TestedVmForValidation}, + versions::testonly::{validation_params, TestedVm, TestedVmForValidation}, }; mod account_validation_rules; @@ -168,9 +168,10 @@ impl TestedVm impl TestedVmForValidation for Vm, ValidationTracer> { fn run_validation(&mut self, tx: L2Tx) -> Option { + let validation_params = validation_params(&tx, &self.system_env); self.push_transaction(tx.into()); - let mut tracer = ValidationTracer::default(); + let mut tracer = ValidationTracer::new(validation_params); self.stop_after_validation(); self.inspect(&mut tracer, InspectExecutionMode::OneTx); diff --git a/core/lib/multivm/src/versions/vm_fast/utils.rs b/core/lib/multivm/src/versions/vm_fast/utils.rs new file mode 100644 index 000000000000..f9e0d48a21af --- /dev/null +++ b/core/lib/multivm/src/versions/vm_fast/utils.rs @@ -0,0 +1,13 @@ +use zksync_types::U256; +use zksync_vm2::{interface::StateInterface, FatPointer}; + +pub(crate) fn read_fat_pointer(state: &S, raw: U256) -> Vec { + let pointer = FatPointer::from(raw); + let length = pointer.length - pointer.offset; + let start = pointer.start + pointer.offset; + let mut result = vec![0; length as usize]; + for i in 0..length { + result[i as usize] = state.read_heap_byte(pointer.memory_page, start + i); + } + result +} diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 5ec4914742b4..72734ee05fb9 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -1,9 +1,19 @@ +use std::collections::HashSet; + +use zk_evm_1_3_1::address_to_u256; +use zksync_types::{ + ACCOUNT_CODE_STORAGE_ADDRESS, BOOTLOADER_ADDRESS, CONTRACT_DEPLOYER_ADDRESS, H160, + KECCAK256_PRECOMPILE_ADDRESS, L2_BASE_TOKEN_ADDRESS, MSG_VALUE_SIMULATOR_ADDRESS, + SYSTEM_CONTEXT_ADDRESS, U256, +}; +use zksync_utils::{u256_to_account_address, u256_to_bytes_be}; use zksync_vm2::interface::{ - self, CallframeInterface, Opcode::*, OpcodeType, ReturnType::*, Tracer, + CallframeInterface, GlobalStateInterface, Opcode::*, OpcodeType, ReturnType::*, StateInterface, + Tracer, }; -use zksync_vm_interface::tracer::ViolatedValidationRule; +use zksync_vm_interface::tracer::{ValidationParams, ViolatedValidationRule}; -use super::vm::TracerExt; +use super::{utils::read_fat_pointer, vm::TracerExt}; /// Account abstraction exposes a chain to denial of service attacks because someone who fails to /// authenticate does not pay for the failed transaction. Otherwise people could empty other's @@ -32,28 +42,102 @@ use super::vm::TracerExt; #[derive(Debug, Default)] pub struct ValidationTracer { in_validation: bool, + add_return_value_to_allowed_slots: bool, + + slots_obtained_via_keccak: HashSet, + + user_address: H160, + validation_error: Option, - previous_instruction_was_gasleft: bool, } impl Tracer for ValidationTracer { - fn before_instruction(&mut self, state: &mut S) { + fn before_instruction(&mut self, state: &mut S) { if !self.in_validation { return; } - if self.previous_instruction_was_gasleft { - match OP::VALUE { - // The far call wipes the register, so there is no way the gas number can be used for anything else - FarCall(_) => {} - _ => self.set_error(ViolatedValidationRule::TouchedDisallowedContext), - } - self.previous_instruction_was_gasleft = false; - } + match OP::VALUE { // Out of gas once means out of gas for the whole validation, as the EIP forbids handling out of gas errors Ret(Panic) if state.current_frame().gas() == 0 => { self.set_error(ViolatedValidationRule::TookTooManyComputationalGas(0)) } + + ContextMeta => self.set_error(ViolatedValidationRule::TouchedDisallowedContext), + + StorageRead => { + let address = state.current_frame().address(); + let caller = state.current_frame().caller(); + + // Can unwrap because the instruction pointer does not point to a panic instruction + let pc = state.current_frame().program_counter().unwrap(); + let word = pc / 4; + let part = pc % 4; + let instruction = + state.current_frame().read_contract_code(word).0[3 - part as usize]; + let slot = state.read_register((instruction >> 16) as u8 & 0b1111).0; + + if !self.is_valid_storage_read( + address, + caller, + slot, + state.get_storage(address, slot), + ) { + self.set_error(ViolatedValidationRule::TouchedDisallowedStorageSlots( + address, slot, + )) + } + + // todo maybe allow some slot + } + + _ => {} + } + } + + fn after_instruction(&mut self, state: &mut S) { + if !self.in_validation { + return; + } + + match OP::VALUE { + FarCall(_) => { + // Intercept calls to keccak, whitelist storage slots corresponding to the hash + let code_address = state.current_frame().code_address(); + if code_address == KECCAK256_PRECOMPILE_ADDRESS { + let calldata = read_fat_pointer(state, state.read_register(1).0); + if calldata.len() != 64 { + return; + } + + // Solidity mappings store values at the keccak256 hash of `key ++ slot_of_mapping` + let (key, mapping) = calldata.split_at(32); + + let mapping_is_allowed = + self.slots_obtained_via_keccak.contains(&mapping.into()); + + if U256::from(key) == address_to_u256(&self.user_address) || mapping_is_allowed + { + self.add_return_value_to_allowed_slots = true; + } + } else if code_address != self.user_address + && state + .get_storage(ACCOUNT_CODE_STORAGE_ADDRESS, address_to_u256(&code_address)) + .is_zero() + { + self.set_error(ViolatedValidationRule::CalledContractWithNoCode( + code_address, + )) + } + } + Ret(kind) => { + if self.add_return_value_to_allowed_slots && kind == Normal { + let return_value = read_fat_pointer(state, state.read_register(1).0); + self.slots_obtained_via_keccak + .insert(return_value.as_slice().into()); + } + self.add_return_value_to_allowed_slots = false; + } _ => {} } } @@ -70,6 +154,32 @@ impl TracerExt for ValidationTracer { } impl ValidationTracer { + pub fn new(params: ValidationParams) -> Self { + let ValidationParams { user_address, .. } = params; + Self { + user_address, + ..Self::default() + } + } + + fn is_valid_storage_read(&self, address: H160, caller: H160, slot: U256, value: U256) -> bool { + // allow reading own slots + address == self.user_address + // allow reading slot + || slot == address_to_u256(&self.user_address) + || self.slots_obtained_via_keccak.contains(&slot) + // todo trusted slots + // certain system contracts are allowed to transfer ETH + || address == L2_BASE_TOKEN_ADDRESS + && (caller == MSG_VALUE_SIMULATOR_ADDRESS + || caller == CONTRACT_DEPLOYER_ADDRESS + || caller == BOOTLOADER_ADDRESS) + // allow getting chain_id + || address == SYSTEM_CONTEXT_ADDRESS && slot == U256::zero() + // allow reading code hashes of existing contracts + || address == ACCOUNT_CODE_STORAGE_ADDRESS && !value.is_zero() + } + fn set_error(&mut self, error: ViolatedValidationRule) { if self.validation_error.is_none() { self.validation_error = Some(error); diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 312e43dceaf6..0d668bee60d8 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -241,6 +241,9 @@ impl Vm { gas_given, gas_hidden: gas - gas_given, }); + // As long as gasleft is allowed during account validation, + // the VM must not be used in the sequencer because a malicious + // account cause proving failure by checking if gasleft > 100k self.inner.current_frame().set_gas(gas_given); } diff --git a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs index 07550a11591b..445bc3eece69 100644 --- a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs @@ -14,8 +14,8 @@ use zksync_types::{ U256, }; use zksync_utils::{bytecode::hash_bytecode, bytes_to_be_words, h256_to_u256}; -use zksync_vm_interface::pubdata::PubdataBuilder; use zksync_vm_interface::{ + pubdata::PubdataBuilder, tracer::{ValidationParams, ViolatedValidationRule}, VmInterface, }; @@ -27,7 +27,9 @@ use crate::{ CurrentExecutionState, L2BlockEnv, VmExecutionMode, VmExecutionResultAndLogs, }, tracers::ValidationTracer, - versions::testonly::{filter_out_base_system_contracts, TestedVm, TestedVmForValidation}, + versions::testonly::{ + filter_out_base_system_contracts, validation_params, TestedVm, TestedVmForValidation, + }, vm_latest::{ constants::BOOTLOADER_HEAP_PAGE, old_vm::{event_sink::InMemoryEventSink, history_recorder::HistoryRecorder}, @@ -199,19 +201,11 @@ impl TestedVm for TestedLatestVm { impl TestedVmForValidation for TestedLatestVm { fn run_validation(&mut self, tx: L2Tx) -> Option { - let user_address = tx.common_data.initiator_address; - let paymaster_address = tx.common_data.paymaster_params.paymaster; + let validation_params = validation_params(&tx, &self.system_env); self.push_transaction(tx.into()); let (tracer, mut failures) = ValidationTracer::::new( - ValidationParams { - user_address, - paymaster_address, - trusted_slots: Default::default(), - trusted_addresses: Default::default(), - trusted_address_slots: Default::default(), - computational_gas_limit: 100_000, - }, + validation_params, VmVersion::Vm1_5_0IncreasedBootloaderMemory, ); self.inspect_inner( diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol index 4a40c0f88a1b..b36d1699feac 100644 --- a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -35,6 +35,9 @@ contract ValidationRuleBreaker is IAccount { // I'm writing assertions because otherwise the compiler would optimize away the access require(BOOTLOADER_FORMAL_ADDRESS.balance != 0); } else if (typeOfRuleBreak == 2) { + // May not call an EOA + address(1234567890).call(""); + } else if (typeOfRuleBreak == 3) {} else if (typeOfRuleBreak == 100) { // Gas may only be queried to pass everything into a far call gasleft(); } From fcd918533bfbd1bd1b95dea6e1621d3f3fc39ad3 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 25 Oct 2024 21:38:21 +0200 Subject: [PATCH 17/46] properly use all ValidationParams --- .../src/versions/vm_fast/validation_tracer.rs | 45 ++++++++++++++----- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 72734ee05fb9..5930212a5ef0 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -2,14 +2,12 @@ use std::collections::HashSet; use zk_evm_1_3_1::address_to_u256; use zksync_types::{ - ACCOUNT_CODE_STORAGE_ADDRESS, BOOTLOADER_ADDRESS, CONTRACT_DEPLOYER_ADDRESS, H160, + Address, ACCOUNT_CODE_STORAGE_ADDRESS, BOOTLOADER_ADDRESS, CONTRACT_DEPLOYER_ADDRESS, KECCAK256_PRECOMPILE_ADDRESS, L2_BASE_TOKEN_ADDRESS, MSG_VALUE_SIMULATOR_ADDRESS, SYSTEM_CONTEXT_ADDRESS, U256, }; -use zksync_utils::{u256_to_account_address, u256_to_bytes_be}; use zksync_vm2::interface::{ - CallframeInterface, GlobalStateInterface, Opcode::*, OpcodeType, ReturnType::*, StateInterface, - Tracer, + CallframeInterface, GlobalStateInterface, Opcode::*, OpcodeType, ReturnType::*, Tracer, }; use zksync_vm_interface::tracer::{ValidationParams, ViolatedValidationRule}; @@ -45,8 +43,12 @@ pub struct ValidationTracer { add_return_value_to_allowed_slots: bool, slots_obtained_via_keccak: HashSet, + trusted_addresses: HashSet
, - user_address: H160, + user_address: Address, + trusted_storage: HashSet<(Address, U256)>, + /// These location's values are added to [Self::trusted_addresses] to support upgradeable proxies. + storage_containing_trusted_addresses: HashSet<(Address, U256)>, validation_error: Option, } @@ -77,7 +79,12 @@ impl Tracer for ValidationTracer { state.current_frame().read_contract_code(word).0[3 - part as usize]; let slot = state.read_register((instruction >> 16) as u8 & 0b1111).0; - if !self.is_valid_storage_read( + if self + .storage_containing_trusted_addresses + .contains(&(address, slot)) + { + self.trusted_addresses.insert(address); + } else if !self.is_valid_storage_read( address, caller, slot, @@ -87,8 +94,6 @@ impl Tracer for ValidationTracer { address, slot, )) } - - // todo maybe allow some slot } _ => {} @@ -155,20 +160,38 @@ impl TracerExt for ValidationTracer { impl ValidationTracer { pub fn new(params: ValidationParams) -> Self { - let ValidationParams { user_address, .. } = params; + let ValidationParams { + user_address, + trusted_slots, + trusted_addresses, + trusted_address_slots, + .. + } = params; Self { user_address, + trusted_storage: trusted_slots, + trusted_addresses, + storage_containing_trusted_addresses: trusted_address_slots, + ..Self::default() } } - fn is_valid_storage_read(&self, address: H160, caller: H160, slot: U256, value: U256) -> bool { + fn is_valid_storage_read( + &self, + address: Address, + caller: Address, + slot: U256, + value: U256, + ) -> bool { // allow reading own slots address == self.user_address // allow reading slot || slot == address_to_u256(&self.user_address) || self.slots_obtained_via_keccak.contains(&slot) - // todo trusted slots + // some storage locations are always allowed + || self.trusted_addresses.contains(&address) + || self.trusted_storage.contains(&(address, slot)) // certain system contracts are allowed to transfer ETH || address == L2_BASE_TOKEN_ADDRESS && (caller == MSG_VALUE_SIMULATOR_ADDRESS From 97911d543492ce2cc176e4fe6a62c4064f2c5cfc Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 25 Oct 2024 21:42:25 +0200 Subject: [PATCH 18/46] pass validation params to tracer in oneshot executor --- core/lib/vm_executor/src/oneshot/mod.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index fcf1104c5ba4..b174e3db339c 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -27,7 +27,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, tracers::{CallTracer, StorageInvocations, TracerDispatcher, ValidationTracer}, utils::adjust_pubdata_price_for_tx, - vm_fast::TracerExt, + vm_fast::{self, TracerExt}, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, FastVmInstance, HistoryMode, LegacyVmInstance, MultiVMTracer, @@ -189,15 +189,14 @@ where }; tokio::task::spawn_blocking(move || { - let (validation_tracer, mut validation_result) = - ValidationTracer::::new( - validation_params, - sandbox.env.system.version.into(), - ); - let tracers = vec![validation_tracer.into_tracer_pointer()]; + let version = sandbox.env.system.version.into(); sandbox.execute_in_vm_with_tracer(|vm, transaction| match vm { Vm::Legacy(vm) => { + let (validation_tracer, mut validation_result) = + ValidationTracer::::new(validation_params, version); + let tracers = vec![validation_tracer.into_tracer_pointer()]; + vm.push_transaction(transaction); let exec_result = vm.inspect(&mut tracers.into(), InspectExecutionMode::OneTx); @@ -220,7 +219,7 @@ where vm.stop_after_validation(); vm.push_transaction(transaction); let mut tracer = - zksync_multivm::vm_fast::validation_tracer::ValidationTracer::default(); + vm_fast::validation_tracer::ValidationTracer::new(validation_params); let result_and_logs = vm.inspect(&mut tracer, InspectExecutionMode::OneTx); if let Some(violation) = tracer.validation_error() { return Err(ValidationError::ViolatedRule(violation)); From 84f85ac43acf4a24248c50aaa579448ff849923b Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 25 Oct 2024 21:43:48 +0200 Subject: [PATCH 19/46] reminder --- .../contracts/custom-account/validation-rule-breaker.sol | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol index b36d1699feac..4b16b7d8396f 100644 --- a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -37,7 +37,9 @@ contract ValidationRuleBreaker is IAccount { } else if (typeOfRuleBreak == 2) { // May not call an EOA address(1234567890).call(""); - } else if (typeOfRuleBreak == 3) {} else if (typeOfRuleBreak == 100) { + } else if (typeOfRuleBreak == 3) { + // TODO make test that calls upgradeable proxy + } else if (typeOfRuleBreak == 100) { // Gas may only be queried to pass everything into a far call gasleft(); } From 920b92823ca58b6a458f737a442f62ddf7f2b1f4 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 25 Oct 2024 22:25:33 +0200 Subject: [PATCH 20/46] unused import --- core/lib/multivm/src/versions/vm_latest/tests/mod.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs index 445bc3eece69..5f42fcbe70b2 100644 --- a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs @@ -14,11 +14,7 @@ use zksync_types::{ U256, }; use zksync_utils::{bytecode::hash_bytecode, bytes_to_be_words, h256_to_u256}; -use zksync_vm_interface::{ - pubdata::PubdataBuilder, - tracer::{ValidationParams, ViolatedValidationRule}, - VmInterface, -}; +use zksync_vm_interface::{pubdata::PubdataBuilder, tracer::ViolatedValidationRule, VmInterface}; use super::{HistoryEnabled, ToTracerPointer, Vm}; use crate::{ From 4430472ef8a4e9cef8f84ffa86a1fe14a00804f5 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Mon, 28 Oct 2024 14:58:30 +0100 Subject: [PATCH 21/46] correctly set fast vm mode --- core/lib/vm_executor/src/oneshot/mod.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index b174e3db339c..21ab63a77b34 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -180,7 +180,11 @@ where ); let sandbox = VmSandbox { - fast_vm_mode: FastVmMode::New, + fast_vm_mode: if !is_supported_by_fast_vm(env.system.version) { + FastVmMode::Old // the fast VM doesn't support old protocol versions + } else { + self.fast_vm_mode + }, panic_on_divergence: self.panic_on_divergence, storage, env, From 2644a7f06236fc5c5d2732b37446df97bdb8af2d Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Tue, 29 Oct 2024 22:46:35 +0100 Subject: [PATCH 22/46] special case the builtin tracers to eliminate TracerExt --- .../multivm/src/versions/vm_fast/bytecode.rs | 2 +- .../src/versions/vm_fast/circuits_tracer.rs | 3 - core/lib/multivm/src/versions/vm_fast/mod.rs | 6 +- .../vm_fast/tests/account_validation_rules.rs | 2 +- .../multivm/src/versions/vm_fast/tests/mod.rs | 18 +++-- .../src/versions/vm_fast/validation_tracer.rs | 51 ++++++++---- core/lib/multivm/src/versions/vm_fast/vm.rs | 80 +++++++++---------- .../versions/vm_fast/with_builtin_tracers.rs | 80 +++++++++++++++++++ core/lib/multivm/src/vm_instance.rs | 27 ++++--- core/lib/vm_executor/src/batch/factory.rs | 4 +- core/lib/vm_executor/src/oneshot/mod.rs | 75 ++++++++++++++--- core/tests/vm-benchmark/src/vm.rs | 7 +- 12 files changed, 251 insertions(+), 104 deletions(-) create mode 100644 core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs diff --git a/core/lib/multivm/src/versions/vm_fast/bytecode.rs b/core/lib/multivm/src/versions/vm_fast/bytecode.rs index b75e33a21b05..fcb2398bf7ca 100644 --- a/core/lib/multivm/src/versions/vm_fast/bytecode.rs +++ b/core/lib/multivm/src/versions/vm_fast/bytecode.rs @@ -8,7 +8,7 @@ use crate::{ utils::bytecode, }; -impl Vm { +impl Vm { /// Checks the last transaction has successfully published compressed bytecodes and returns `true` if there is at least one is still unknown. pub(crate) fn has_unpublished_bytecodes(&mut self) -> bool { self.bootloader_state diff --git a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs b/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs index e744b547be4b..3585ea876daf 100644 --- a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs @@ -4,7 +4,6 @@ use zksync_vm2::interface::{ }; use zksync_vm_interface::CircuitStatistic; -use super::TracerExt; use crate::vm_latest::tracers::circuits_capacity::*; /// VM tracer tracking [`CircuitStatistic`]s. Statistics generally depend on the number of time some opcodes were invoked, @@ -133,8 +132,6 @@ impl Tracer for CircuitsTracer { } } -impl TracerExt for CircuitsTracer {} - impl CircuitsTracer { /// Obtains the current circuit stats from this tracer. pub fn circuit_statistic(&self) -> CircuitStatistic { diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index 8355f19142b6..c9c69ca01b74 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -1,9 +1,6 @@ pub use zksync_vm2::interface; -pub use self::{ - circuits_tracer::CircuitsTracer, - vm::{TracerExt, Vm}, -}; +pub use self::{circuits_tracer::CircuitsTracer, vm::Vm, with_builtin_tracers::WithBuiltinTracers}; mod bootloader_state; mod bytecode; @@ -20,3 +17,4 @@ mod transaction_data; mod utils; pub mod validation_tracer; mod vm; +mod with_builtin_tracers; diff --git a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs index fb3a66570595..996e5bcc161e 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs @@ -5,5 +5,5 @@ use crate::{ #[test] fn test_account_validation_rules_fast() { - test_account_validation_rules::>(); + test_account_validation_rules::>(); } diff --git a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs index 1c4264bf3b6b..89d21641929c 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -2,14 +2,17 @@ use std::{any::Any, collections::HashSet, fmt, rc::Rc}; use zksync_types::{l2::L2Tx, writes::StateDiffRecord, StorageKey, Transaction, H160, H256, U256}; use zksync_utils::h256_to_u256; -use zksync_vm2::interface::{Event, HeapId, StateInterface}; +use zksync_vm2::interface::{Event, HeapId, StateInterface, Tracer}; use zksync_vm_interface::{ pubdata::PubdataBuilder, storage::ReadStorage, tracer::ViolatedValidationRule, CurrentExecutionState, InspectExecutionMode, L2BlockEnv, VmExecutionMode, VmExecutionResultAndLogs, VmInterface, }; -use super::{validation_tracer::ValidationTracer, TracerExt, Vm}; +use super::{ + validation_tracer::{ValidationMode, ValidationTracer}, + Vm, WithBuiltinTracers, +}; use crate::{ interface::storage::{ImmutableStorageView, InMemoryStorage}, versions::testonly::{validation_params, TestedVm, TestedVmForValidation}, @@ -77,8 +80,10 @@ impl PartialEq for VmStateDump { } } -impl TestedVm - for Vm, T> +impl< + T: Tracer + Default + std::fmt::Debug + 'static, + V: ValidationMode + std::fmt::Debug + 'static, + > TestedVm for Vm, T, V> { type StateDump = VmStateDump; @@ -166,13 +171,12 @@ impl TestedVm } } -impl TestedVmForValidation for Vm, ValidationTracer> { +impl TestedVmForValidation for Vm, (), ValidationTracer> { fn run_validation(&mut self, tx: L2Tx) -> Option { let validation_params = validation_params(&tx, &self.system_env); self.push_transaction(tx.into()); - let mut tracer = ValidationTracer::new(validation_params); - self.stop_after_validation(); + let mut tracer = WithBuiltinTracers::for_validation((), validation_params); self.inspect(&mut tracer, InspectExecutionMode::OneTx); tracer.validation_error() diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 5930212a5ef0..fab3af6889a3 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -7,11 +7,27 @@ use zksync_types::{ SYSTEM_CONTEXT_ADDRESS, U256, }; use zksync_vm2::interface::{ - CallframeInterface, GlobalStateInterface, Opcode::*, OpcodeType, ReturnType::*, Tracer, + CallframeInterface, ExecutionStatus, GlobalStateInterface, Opcode::*, OpcodeType, + ReturnType::*, Tracer, }; use zksync_vm_interface::tracer::{ValidationParams, ViolatedValidationRule}; -use super::{utils::read_fat_pointer, vm::TracerExt}; +use super::utils::read_fat_pointer; + +pub trait ValidationMode: Tracer + Default { + const STOP_AFTER_VALIDATION: bool; + fn account_validation_entered(&mut self); + fn validation_exited(&mut self); +} + +#[derive(Debug, Default)] +pub struct ValidationGasLimitOnly; +impl Tracer for ValidationGasLimitOnly {} +impl ValidationMode for ValidationGasLimitOnly { + const STOP_AFTER_VALIDATION: bool = false; + fn account_validation_entered(&mut self) {} + fn validation_exited(&mut self) {} +} /// Account abstraction exposes a chain to denial of service attacks because someone who fails to /// authenticate does not pay for the failed transaction. Otherwise people could empty other's @@ -53,6 +69,18 @@ pub struct ValidationTracer { validation_error: Option, } +impl ValidationMode for ValidationTracer { + const STOP_AFTER_VALIDATION: bool = true; + + fn account_validation_entered(&mut self) { + self.in_validation = true; + } + + fn validation_exited(&mut self) { + self.in_validation = false; + } +} + impl Tracer for ValidationTracer { fn before_instruction(&mut self, state: &mut S) { if !self.in_validation { @@ -100,9 +128,12 @@ impl Tracer for ValidationTracer { } } - fn after_instruction(&mut self, state: &mut S) { + fn after_instruction( + &mut self, + state: &mut S, + ) -> ExecutionStatus { if !self.in_validation { - return; + return ExecutionStatus::Running; } match OP::VALUE { @@ -112,7 +143,7 @@ impl Tracer for ValidationTracer { if code_address == KECCAK256_PRECOMPILE_ADDRESS { let calldata = read_fat_pointer(state, state.read_register(1).0); if calldata.len() != 64 { - return; + return ExecutionStatus::Running; } // Solidity mappings store values at the keccak256 hash of `key ++ slot_of_mapping` @@ -145,16 +176,8 @@ impl Tracer for ValidationTracer { } _ => {} } - } -} -impl TracerExt for ValidationTracer { - fn on_bootloader_hook(&mut self, hook: super::hook::Hook) { - match hook { - super::hook::Hook::AccountValidationEntered => self.in_validation = true, - super::hook::Hook::ValidationExited => self.in_validation = false, - _ => {} - } + ExecutionStatus::Running } } diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 0d668bee60d8..a49b84f5d753 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, fmt, mem, rc::Rc}; +use std::{collections::HashMap, fmt, rc::Rc}; use zk_evm_1_5_0::{ aux_structures::LogQuery, zkevm_opcode_defs::system_params::INITIAL_FRAME_FORMAL_EH_LOCATION, @@ -26,10 +26,11 @@ use zksync_vm_interface::{pubdata::PubdataBuilder, InspectExecutionMode}; use super::{ bootloader_state::{BootloaderState, BootloaderStateSnapshot}, bytecode::compress_bytecodes, - circuits_tracer::CircuitsTracer, hook::Hook, initial_bootloader_memory::bootloader_initial_memory, transaction_data::TransactionData, + validation_tracer::{ValidationGasLimitOnly, ValidationMode}, + WithBuiltinTracers, }; use crate::{ glue::GlueInto, @@ -60,8 +61,6 @@ use crate::{ const VM_VERSION: MultiVMSubversion = MultiVMSubversion::IncreasedBootloaderMemory; -type FullTracer = (Tr, CircuitsTracer); - #[derive(Debug)] struct VmRunResult { execution_result: ExecutionResult, @@ -87,36 +86,26 @@ impl VmRunResult { } } -pub trait TracerExt: Tracer { - fn on_bootloader_hook(&mut self, #[allow(unused_variables)] hook: Hook) {} -} - -impl TracerExt for () {} -impl TracerExt for (A, B) { - fn on_bootloader_hook(&mut self, hook: Hook) { - self.0.on_bootloader_hook(hook); - self.1.on_bootloader_hook(hook); - } -} - /// Fast VM wrapper. /// -/// The wrapper is parametric by the storage and tracer types. Besides the [`Tracer`] trait, a tracer must have `'static` lifetime -/// and implement [`Default`] (the latter is necessary to complete batches). [`CircuitsTracer`] is currently always enabled; +/// The wrapper is parametric by the storage and tracer types. Besides the [`Tracer`] trait and implement [`Default`] +/// (the latter is necessary to complete batches). [`CircuitsTracer`] is currently always enabled; /// you don't need to specify it explicitly. -pub struct Vm { - pub(crate) world: World>, - pub(crate) inner: VirtualMachine, World>>, +pub struct Vm { + pub(crate) world: World>, + pub(crate) inner: VirtualMachine< + WithBuiltinTracers, + World>, + >, pub(crate) bootloader_state: BootloaderState, pub(crate) batch_env: L1BatchEnv, pub(crate) system_env: SystemEnv, snapshot: Option, - stop_after_validation: bool, #[cfg(test)] enforced_state_diffs: Option>, } -impl Vm { +impl Vm { pub fn custom(batch_env: L1BatchEnv, system_env: SystemEnv, storage: S) -> Self { assert!( is_supported_by_fast_vm(system_env.version), @@ -174,7 +163,6 @@ impl Vm { system_env, batch_env, snapshot: None, - stop_after_validation: false, #[cfg(test)] enforced_state_diffs: None, }; @@ -185,7 +173,7 @@ impl Vm { fn run( &mut self, execution_mode: VmExecutionMode, - tracer: &mut (Tr, CircuitsTracer), + tracer: &mut WithBuiltinTracers, track_refunds: bool, ) -> VmRunResult { struct AccountValidationGasSplit { @@ -228,13 +216,14 @@ impl Vm { }; let hook = Hook::from_u32(hook); - tracer.on_bootloader_hook(hook); match hook { Hook::AccountValidationEntered => { assert!( account_validation_gas_split.is_none(), "Account validation can't be nested" ); + tracer.validation().account_validation_entered(); + let gas = self.gas_remaining(); let gas_given = gas.min(gas_left_for_account_validation); account_validation_gas_split = Some(AccountValidationGasSplit { @@ -248,6 +237,8 @@ impl Vm { } Hook::ValidationExited => { + tracer.validation().validation_exited(); + if let Some(AccountValidationGasSplit { gas_given, gas_hidden, @@ -258,11 +249,13 @@ impl Vm { self.inner.current_frame().set_gas(gas_left + gas_hidden); } } + Hook::ValidationStepEnded => { - if self.stop_after_validation { - break (ExecutionResult::Success { output: vec![] }, false); + if Validation::STOP_AFTER_VALIDATION { + break (ExecutionResult::Success { output: vec![] }, true); } } + Hook::TxHasEnded => { if let VmExecutionMode::OneTx = execution_mode { // The bootloader may invoke `TxHasEnded` hook without posting a tx result previously. One case when this can happen @@ -419,10 +412,6 @@ impl Vm { } } - pub fn stop_after_validation(&mut self) { - self.stop_after_validation = true; - } - fn get_hook_params(&self) -> [U256; 3] { (get_vm_hook_params_start_position(VM_VERSION) ..get_vm_hook_params_start_position(VM_VERSION) + VM_HOOK_PARAMS_COUNT) @@ -614,7 +603,7 @@ impl Vm { pub(crate) fn inspect_inner( &mut self, - tracer: &mut Tr, + tracer: &mut WithBuiltinTracers, execution_mode: VmExecutionMode, ) -> VmExecutionResultAndLogs { let mut track_refunds = false; @@ -627,9 +616,7 @@ impl Vm { let start = self.inner.world_diff().snapshot(); let gas_before = self.gas_remaining(); - let mut full_tracer = (mem::take(tracer), CircuitsTracer::default()); - let result = self.run(execution_mode, &mut full_tracer, track_refunds); - *tracer = full_tracer.0; // place the tracer back + let result = self.run(execution_mode, tracer, track_refunds); let ignore_world_diff = matches!(execution_mode, VmExecutionMode::OneTx) && result.should_ignore_vm_logs(); @@ -695,7 +682,7 @@ impl Vm { gas_remaining, computational_gas_used: gas_used, // since 1.5.0, this always has the same value as `gas_used` pubdata_published: result.pubdata_published, - circuit_statistic: full_tracer.1.circuit_statistic(), + circuit_statistic: tracer.circuit().circuit_statistic(), contracts_used: 0, cycles_used: 0, total_log_queries: 0, @@ -706,10 +693,11 @@ impl Vm { } } -impl VmFactory> for Vm, Tr> +impl VmFactory> + for Vm, Tr, Validation> where S: ReadStorage, - Tr: Tracer + Default + 'static, + Tr: Tracer + Default, { fn new( batch_env: L1BatchEnv, @@ -721,8 +709,10 @@ where } } -impl VmInterface for Vm { - type TracerDispatcher = Tr; +impl VmInterface + for Vm +{ + type TracerDispatcher = WithBuiltinTracers; fn push_transaction(&mut self, tx: Transaction) -> PushTransactionResult<'_> { self.push_transaction_inner(tx, 0, true); @@ -767,7 +757,7 @@ impl VmInterface for Vm) -> FinishedL1Batch { - let result = self.inspect_inner(&mut Tr::default(), VmExecutionMode::Batch); + let result = self.inspect_inner(&mut Default::default(), VmExecutionMode::Batch); let execution_state = self.get_current_execution_state(); let bootloader_memory = self.bootloader_state.bootloader_memory(); FinishedL1Batch { @@ -795,7 +785,9 @@ struct VmSnapshot { bootloader_snapshot: BootloaderStateSnapshot, } -impl VmInterfaceHistoryEnabled for Vm { +impl VmInterfaceHistoryEnabled + for Vm +{ fn make_snapshot(&mut self) { assert!( self.snapshot.is_none(), @@ -829,7 +821,7 @@ impl VmTrackingContracts for Vm { } } -impl fmt::Debug for Vm { +impl fmt::Debug for Vm { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Vm") .field("bootloader_state", &self.bootloader_state) diff --git a/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs b/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs new file mode 100644 index 000000000000..6bab893c4b57 --- /dev/null +++ b/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs @@ -0,0 +1,80 @@ +use zksync_vm2::interface::Tracer; +use zksync_vm_interface::tracer::{ValidationParams, ViolatedValidationRule}; + +use super::{ + validation_tracer::{ValidationGasLimitOnly, ValidationMode, ValidationTracer}, + CircuitsTracer, +}; + +#[derive(Default, Debug)] +pub struct WithBuiltinTracers((External, (CircuitsTracer, Validation))); + +impl WithBuiltinTracers { + pub fn for_validation(external: External, validation_params: ValidationParams) -> Self { + Self(( + external, + ( + CircuitsTracer::default(), + ValidationTracer::new(validation_params), + ), + )) + } + + pub fn validation_error(&self) -> Option { + self.0 .1 .1.validation_error() + } +} + +impl WithBuiltinTracers { + pub fn for_api(external: External) -> Self { + Self((external, Default::default())) + } +} + +impl WithBuiltinTracers { + pub fn for_sequencer(external: External) -> Self { + Self((external, Default::default())) + } +} + +impl WithBuiltinTracers { + pub fn into_inner(self) -> External { + self.0 .0 + } + + pub fn validation(&mut self) -> &mut Validation { + &mut self.0 .1 .1 + } + + pub fn circuit(&mut self) -> &mut CircuitsTracer { + &mut self.0 .1 .0 + } +} + +impl Tracer + for WithBuiltinTracers +{ + fn before_instruction< + OP: zksync_vm2::interface::OpcodeType, + S: zksync_vm2::interface::GlobalStateInterface, + >( + &mut self, + state: &mut S, + ) { + self.0.before_instruction::(state) + } + + fn after_instruction< + OP: zksync_vm2::interface::OpcodeType, + S: zksync_vm2::interface::GlobalStateInterface, + >( + &mut self, + state: &mut S, + ) -> zksync_vm2::interface::ExecutionStatus { + self.0.after_instruction::(state) + } + + fn on_extra_prover_cycles(&mut self, stats: zksync_vm2::interface::CycleStats) { + self.0.on_extra_prover_cycles(stats); + } +} diff --git a/core/lib/multivm/src/vm_instance.rs b/core/lib/multivm/src/vm_instance.rs index e0a5437a27df..62ab8ed89079 100644 --- a/core/lib/multivm/src/vm_instance.rs +++ b/core/lib/multivm/src/vm_instance.rs @@ -13,7 +13,10 @@ use crate::{ VmMemoryMetrics, }, tracers::TracerDispatcher, - vm_fast::TracerExt, + vm_fast::{ + validation_tracer::{ValidationGasLimitOnly, ValidationMode}, + WithBuiltinTracers, + }, vm_latest::HistoryEnabled, }; @@ -226,19 +229,19 @@ impl LegacyVmInstance { } /// Fast VM shadowed by the latest legacy VM. -pub type ShadowedFastVm = ShadowVm< +pub type ShadowedFastVm = ShadowVm< S, crate::vm_latest::Vm, HistoryEnabled>, - crate::vm_fast::Vm, Tr>, + crate::vm_fast::Vm, Tr, Validation>, >; /// Fast VM variants. #[derive(Debug)] -pub enum FastVmInstance { +pub enum FastVmInstance { /// Fast VM running in isolation. - Fast(crate::vm_fast::Vm, Tr>), + Fast(crate::vm_fast::Vm, Tr, Validation>), /// Fast VM shadowed by the latest legacy VM. - Shadowed(ShadowedFastVm), + Shadowed(ShadowedFastVm), } macro_rules! dispatch_fast_vm { @@ -250,12 +253,12 @@ macro_rules! dispatch_fast_vm { }; } -impl VmInterface - for FastVmInstance +impl + VmInterface for FastVmInstance { type TracerDispatcher = ( crate::vm_latest::TracerDispatcher, HistoryEnabled>, - Tr, + WithBuiltinTracers, ); fn push_transaction(&mut self, tx: Transaction) -> PushTransactionResult<'_> { @@ -300,7 +303,7 @@ impl VmInterf } } -impl VmInterfaceHistoryEnabled +impl VmInterfaceHistoryEnabled for FastVmInstance { fn make_snapshot(&mut self) { @@ -316,7 +319,9 @@ impl VmInterfaceHistoryEnable } } -impl FastVmInstance { +impl + FastVmInstance +{ /// Creates an isolated fast VM. pub fn fast( l1_batch_env: L1BatchEnv, diff --git a/core/lib/vm_executor/src/batch/factory.rs b/core/lib/vm_executor/src/batch/factory.rs index b010f8bf55cf..6ec958882cc9 100644 --- a/core/lib/vm_executor/src/batch/factory.rs +++ b/core/lib/vm_executor/src/batch/factory.rs @@ -37,7 +37,7 @@ pub trait BatchTracer: fmt::Debug + 'static + Send + Sealed { const TRACE_CALLS: bool; /// Tracer for the fast VM. #[doc(hidden)] - type Fast: vm_fast::TracerExt + Default + 'static; + type Fast: vm_fast::interface::Tracer + Default; } impl Sealed for () {} @@ -228,7 +228,7 @@ impl BatchVm { with_compression, ), Self::Fast(vm) => { - let mut tracer = (legacy_tracer.into(), ::default()); + let mut tracer = (legacy_tracer.into(), Default::default()); vm.inspect_transaction_with_bytecode_compression(&mut tracer, tx, with_compression) } }; diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 21ab63a77b34..7b0c62760169 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -27,7 +27,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, tracers::{CallTracer, StorageInvocations, TracerDispatcher, ValidationTracer}, utils::adjust_pubdata_price_for_tx, - vm_fast::{self, TracerExt}, + vm_fast::{self, validation_tracer::ValidationGasLimitOnly, WithBuiltinTracers}, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, FastVmInstance, HistoryMode, LegacyVmInstance, MultiVMTracer, @@ -199,7 +199,7 @@ where Vm::Legacy(vm) => { let (validation_tracer, mut validation_result) = ValidationTracer::::new(validation_params, version); - let tracers = vec![validation_tracer.into_tracer_pointer()]; + let tracers = validation_tracer.into_tracer_pointer(); vm.push_transaction(transaction); let exec_result = vm.inspect(&mut tracers.into(), InspectExecutionMode::OneTx); @@ -220,10 +220,8 @@ where } Vm::Fast(FastVmInstance::Fast(vm)) => { - vm.stop_after_validation(); vm.push_transaction(transaction); - let mut tracer = - vm_fast::validation_tracer::ValidationTracer::new(validation_params); + let mut tracer = WithBuiltinTracers::for_validation((), validation_params); let result_and_logs = vm.inspect(&mut tracer, InspectExecutionMode::OneTx); if let Some(violation) = tracer.validation_error() { return Err(ValidationError::ViolatedRule(violation)); @@ -237,8 +235,55 @@ where } } - _ => { - unimplemented!("Shadow validation is not supported.") + Vm::Fast(FastVmInstance::Shadowed(vm)) => { + vm.push_transaction(transaction); + let tracer = WithBuiltinTracers::for_validation((), validation_params.clone()); + + let (validation_tracer, mut validation_result) = + ValidationTracer::::new(validation_params, version); + let legacy_tracers: Box< + dyn MultiVMTracer>, HistoryEnabled>, + > = validation_tracer.into_tracer_pointer(); + + let mut aggregate_tracer = ( + TracerDispatcher::<_, _>::from(legacy_tracers).into(), + tracer, + ); + let result_and_logs = + vm.inspect(&mut aggregate_tracer, InspectExecutionMode::OneTx); + + let fast_result = if let Some(violation) = aggregate_tracer.1.validation_error() + { + Err(ValidationError::ViolatedRule(violation)) + } else { + match &result_and_logs.result { + ExecutionResult::Halt { reason } => { + Err(ValidationError::FailedTx(reason.clone())) + } + ExecutionResult::Revert { .. } => { + unreachable!("Revert can only happen at the end of a transaction") + } + ExecutionResult::Success { .. } => Ok(()), + } + }; + + let validation_result = Arc::make_mut(&mut validation_result) + .take() + .map_or(Ok(()), Err); + + let legacy_result = match (result_and_logs.result, validation_result) { + (_, Err(violated_rule)) => { + Err(ValidationError::ViolatedRule(violated_rule)) + } + (ExecutionResult::Halt { reason }, _) => { + Err(ValidationError::FailedTx(reason)) + } + _ => Ok(()), + }; + + // TODO compare validation to legacy + + legacy_result } }) }) @@ -248,9 +293,9 @@ where } #[derive(Debug)] -enum Vm { +enum Vm { Legacy(LegacyVmInstance), - Fast(FastVmInstance), + Fast(FastVmInstance), } impl Vm { @@ -279,7 +324,7 @@ impl Vm { missed_storage_invocation_limit, None, ); - let mut full_tracer = (legacy_tracers.into(), ()); + let mut full_tracer = (legacy_tracers.into(), WithBuiltinTracers::for_api(())); vm.inspect_transaction_with_bytecode_compression( &mut full_tracer, tx, @@ -374,12 +419,16 @@ impl VmSandbox { self, action: impl FnOnce(&mut Vm>, Transaction) -> T, ) -> T { - self.execute_in_vm_with_tracer(action) + self.execute_in_vm_with_tracer::(action) } - fn execute_in_vm_with_tracer( + fn execute_in_vm_with_tracer< + T, + Tr: vm_fast::interface::Tracer + Default, + Validation: vm_fast::validation_tracer::ValidationMode, + >( mut self, - action: impl FnOnce(&mut Vm, Tr>, Transaction) -> T, + action: impl FnOnce(&mut Vm, Tr, Validation>, Transaction) -> T, ) -> T { Self::setup_storage( &mut self.storage, diff --git a/core/tests/vm-benchmark/src/vm.rs b/core/tests/vm-benchmark/src/vm.rs index b1c8bf7b5e06..3fd690dfb536 100644 --- a/core/tests/vm-benchmark/src/vm.rs +++ b/core/tests/vm-benchmark/src/vm.rs @@ -9,7 +9,7 @@ use zksync_multivm::{ VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceExt, VmInterfaceHistoryEnabled, }, - vm_fast::{self, TracerExt}, + vm_fast::{self, WithBuiltinTracers}, vm_latest::{self, constants::BATCH_COMPUTATIONAL_GAS_LIMIT, HistoryEnabled, ToTracerPointer}, zk_evm_latest::ethereum_types::{Address, U256}, }; @@ -112,15 +112,14 @@ impl CountInstructions for Fast { self.0 += 1; } } - impl TracerExt for InstructionCount {} let (system_env, l1_batch_env) = test_env(); let mut vm = vm_fast::Vm::<_, InstructionCount>::custom(l1_batch_env, system_env, &*STORAGE); vm.push_transaction(tx.clone()); - let mut tracer = InstructionCount(0); + let mut tracer = WithBuiltinTracers::for_sequencer(InstructionCount(0)); vm.inspect(&mut tracer, InspectExecutionMode::OneTx); - tracer.0 + tracer.into_inner().0 } } From 85196b4fe58c0454c20035e49bb8a7576e48d813 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Tue, 29 Oct 2024 22:58:15 +0100 Subject: [PATCH 23/46] stop validation on error --- .../src/versions/vm_fast/validation_tracer.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index fab3af6889a3..ef9038a4d5ee 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -6,9 +6,12 @@ use zksync_types::{ KECCAK256_PRECOMPILE_ADDRESS, L2_BASE_TOKEN_ADDRESS, MSG_VALUE_SIMULATOR_ADDRESS, SYSTEM_CONTEXT_ADDRESS, U256, }; -use zksync_vm2::interface::{ - CallframeInterface, ExecutionStatus, GlobalStateInterface, Opcode::*, OpcodeType, - ReturnType::*, Tracer, +use zksync_vm2::{ + interface::{ + CallframeInterface, ExecutionStatus, GlobalStateInterface, Opcode::*, OpcodeType, + ReturnType::*, Tracer, + }, + ExecutionEnd, }; use zksync_vm_interface::tracer::{ValidationParams, ViolatedValidationRule}; @@ -136,6 +139,10 @@ impl Tracer for ValidationTracer { return ExecutionStatus::Running; } + if self.validation_error.is_some() { + return ExecutionStatus::Stopped(ExecutionEnd::Panicked); + } + match OP::VALUE { FarCall(_) => { // Intercept calls to keccak, whitelist storage slots corresponding to the hash @@ -163,7 +170,8 @@ impl Tracer for ValidationTracer { { self.set_error(ViolatedValidationRule::CalledContractWithNoCode( code_address, - )) + )); + return ExecutionStatus::Stopped(ExecutionEnd::Panicked); } } Ret(kind) => { From 50e2fadffa50a6e5f481f9da26ac62d5b8387401 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 30 Oct 2024 01:10:14 +0100 Subject: [PATCH 24/46] hide some of the type complexity --- .../multivm/src/versions/vm_fast/bytecode.rs | 2 +- core/lib/multivm/src/versions/vm_fast/mod.rs | 9 +- .../vm_fast/tests/account_validation_rules.rs | 4 +- .../multivm/src/versions/vm_fast/tests/mod.rs | 15 +- core/lib/multivm/src/versions/vm_fast/vm.rs | 423 +++++++++--------- .../versions/vm_fast/with_builtin_tracers.rs | 14 +- core/lib/multivm/src/vm_instance.rs | 56 ++- core/lib/vm_executor/src/batch/factory.rs | 4 +- core/lib/vm_executor/src/oneshot/mod.rs | 25 +- core/tests/vm-benchmark/src/vm.rs | 3 +- 10 files changed, 286 insertions(+), 269 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/bytecode.rs b/core/lib/multivm/src/versions/vm_fast/bytecode.rs index fcb2398bf7ca..b75e33a21b05 100644 --- a/core/lib/multivm/src/versions/vm_fast/bytecode.rs +++ b/core/lib/multivm/src/versions/vm_fast/bytecode.rs @@ -8,7 +8,7 @@ use crate::{ utils::bytecode, }; -impl Vm { +impl Vm { /// Checks the last transaction has successfully published compressed bytecodes and returns `true` if there is at least one is still unknown. pub(crate) fn has_unpublished_bytecodes(&mut self) -> bool { self.bootloader_state diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index c9c69ca01b74..d663bb5fcd74 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -1,6 +1,13 @@ pub use zksync_vm2::interface; -pub use self::{circuits_tracer::CircuitsTracer, vm::Vm, with_builtin_tracers::WithBuiltinTracers}; +pub use self::{ + circuits_tracer::CircuitsTracer, + vm::Vm, + with_builtin_tracers::{ + DefaultTracers, WithBuiltinTracers, WithBuiltinTracersForApi, + WithBuiltinTracersForSequencer, WithBuiltinTracersForValidation, + }, +}; mod bootloader_state; mod bytecode; diff --git a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs index 996e5bcc161e..3623ec5c95d5 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs @@ -1,9 +1,9 @@ use crate::{ versions::testonly::account_validation_rules::test_account_validation_rules, - vm_fast::{validation_tracer::ValidationTracer, Vm}, + vm_fast::{Vm, WithBuiltinTracersForValidation}, }; #[test] fn test_account_validation_rules_fast() { - test_account_validation_rules::>(); + test_account_validation_rules::>>(); } diff --git a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs index 89d21641929c..e4b3d1d103ee 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -10,8 +10,7 @@ use zksync_vm_interface::{ }; use super::{ - validation_tracer::{ValidationMode, ValidationTracer}, - Vm, WithBuiltinTracers, + validation_tracer::ValidationMode, Vm, WithBuiltinTracers, WithBuiltinTracersForValidation, }; use crate::{ interface::storage::{ImmutableStorageView, InMemoryStorage}, @@ -80,10 +79,10 @@ impl PartialEq for VmStateDump { } } -impl< - T: Tracer + Default + std::fmt::Debug + 'static, - V: ValidationMode + std::fmt::Debug + 'static, - > TestedVm for Vm, T, V> +impl TestedVm + for Vm, WithBuiltinTracers> +where + WithBuiltinTracers: std::fmt::Debug + 'static, { type StateDump = VmStateDump; @@ -171,7 +170,9 @@ impl< } } -impl TestedVmForValidation for Vm, (), ValidationTracer> { +impl TestedVmForValidation + for Vm, WithBuiltinTracersForValidation<()>> +{ fn run_validation(&mut self, tx: L2Tx) -> Option { let validation_params = validation_params(&tx, &self.system_env); self.push_transaction(tx.into()); diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index a49b84f5d753..7fcf3410ad66 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -91,12 +91,9 @@ impl VmRunResult { /// The wrapper is parametric by the storage and tracer types. Besides the [`Tracer`] trait and implement [`Default`] /// (the latter is necessary to complete batches). [`CircuitsTracer`] is currently always enabled; /// you don't need to specify it explicitly. -pub struct Vm { - pub(crate) world: World>, - pub(crate) inner: VirtualMachine< - WithBuiltinTracers, - World>, - >, +pub struct Vm> { + pub(crate) world: World, + pub(crate) inner: VirtualMachine>, pub(crate) bootloader_state: BootloaderState, pub(crate) batch_env: L1BatchEnv, pub(crate) system_env: SystemEnv, @@ -105,7 +102,7 @@ pub struct Vm { enforced_state_diffs: Option>, } -impl Vm { +impl Vm { pub fn custom(batch_env: L1BatchEnv, system_env: SystemEnv, storage: S) -> Self { assert!( is_supported_by_fast_vm(system_env.version), @@ -170,10 +167,204 @@ impl Vm [U256; 3] { + (get_vm_hook_params_start_position(VM_VERSION) + ..get_vm_hook_params_start_position(VM_VERSION) + VM_HOOK_PARAMS_COUNT) + .map(|word| self.read_word_from_bootloader_heap(word as usize)) + .collect::>() + .try_into() + .unwrap() + } + + fn get_tx_result(&self) -> U256 { + let tx_idx = self.bootloader_state.current_tx(); + let slot = get_result_success_first_slot(VM_VERSION) as usize + tx_idx; + self.read_word_from_bootloader_heap(slot) + } + + fn get_debug_log(&self) -> (String, String) { + let hook_params = self.get_hook_params(); + let mut msg = u256_to_h256(hook_params[0]).as_bytes().to_vec(); + // Trim 0 byte padding at the end. + while msg.last() == Some(&0) { + msg.pop(); + } + + let data = hook_params[1]; + let msg = String::from_utf8(msg).expect("Invalid debug message"); + + // For long data, it is better to use hex-encoding for greater readability + let data_str = if data > U256::from(u64::MAX) { + format!("0x{data:x}") + } else { + data.to_string() + }; + (msg, data_str) + } + + /// Should only be used when the bootloader is executing (e.g., when handling hooks). + pub(crate) fn read_word_from_bootloader_heap(&self, word: usize) -> U256 { + let start_address = word as u32 * 32; + self.inner.read_heap_u256(HeapId::FIRST, start_address) + } + + fn read_bytes_from_heap(&self, ptr: FatPointer) -> Vec { + assert_eq!(ptr.offset, 0); + (ptr.start..ptr.start + ptr.length) + .map(|addr| self.inner.read_heap_byte(ptr.memory_page, addr)) + .collect() + } + + pub(crate) fn has_previous_far_calls(&mut self) -> bool { + let callframe_count = self.inner.number_of_callframes(); + (1..callframe_count).any(|i| !self.inner.callframe(i).is_near_call()) + } + + /// Should only be used when the bootloader is executing (e.g., when handling hooks). + pub(crate) fn write_to_bootloader_heap( + &mut self, + memory: impl IntoIterator, + ) { + assert!( + !self.has_previous_far_calls(), + "Cannot write to bootloader heap when not in root call frame" + ); + + for (slot, value) in memory { + let start_address = slot as u32 * 32; + self.inner + .write_heap_u256(HeapId::FIRST, start_address, value); + } + } + + pub(crate) fn insert_bytecodes<'a>(&mut self, bytecodes: impl IntoIterator) { + for code in bytecodes { + let hash = h256_to_u256(hash_bytecode(code)); + self.world.bytecode_cache.insert(hash, code.into()); + } + } + + pub(crate) fn push_transaction_inner( + &mut self, + tx: zksync_types::Transaction, + refund: u64, + with_compression: bool, + ) { + let tx: TransactionData = tx.into(); + let overhead = tx.overhead_gas(); + + self.insert_bytecodes(tx.factory_deps.iter().map(|dep| &dep[..])); + + let compressed_bytecodes = if is_l1_tx_type(tx.tx_type) || !with_compression { + // L1 transactions do not need compression + vec![] + } else { + compress_bytecodes(&tx.factory_deps, |hash| { + self.inner + .world_diff() + .get_storage_state() + .get(&(KNOWN_CODES_STORAGE_ADDRESS, h256_to_u256(hash))) + .map(|x| !x.is_zero()) + .unwrap_or_else(|| self.world.storage.is_bytecode_known(&hash)) + }) + }; + + let trusted_ergs_limit = tx.trusted_ergs_limit(); + + let memory = self.bootloader_state.push_tx( + tx, + overhead, + refund, + compressed_bytecodes, + trusted_ergs_limit, + self.system_env.chain_id, + ); + + self.write_to_bootloader_heap(memory); + } + + #[cfg(test)] + pub(super) fn enforce_state_diffs(&mut self, diffs: Vec) { + self.enforced_state_diffs = Some(diffs); + } + + fn compute_state_diffs(&mut self) -> Vec { + #[cfg(test)] + if let Some(enforced_diffs) = self.enforced_state_diffs.take() { + return enforced_diffs; + } + + let storage = &mut self.world.storage; + let diffs = + self.inner + .world_diff() + .get_storage_changes() + .map(move |((address, key), change)| { + let storage_key = + StorageKey::new(AccountTreeId::new(address), u256_to_h256(key)); + StateDiffRecord { + address, + key, + derived_key: LogQuery::derive_final_address_for_params(&address, &key), + enumeration_index: storage + .get_enumeration_index(&storage_key) + .unwrap_or_default(), + initial_value: change.before, + final_value: change.after, + } + }); + diffs + .filter(|diff| diff.address != L1_MESSENGER_ADDRESS) + .collect() + } + + pub(crate) fn decommitted_hashes(&self) -> impl Iterator + '_ { + self.inner.world_diff().decommitted_hashes() + } + + pub(super) fn gas_remaining(&mut self) -> u32 { + self.inner.current_frame().gas() + } + + // visible for testing + pub(super) fn get_current_execution_state(&self) -> CurrentExecutionState { + let world_diff = self.inner.world_diff(); + let vm = &self.inner; + let events = merge_events(vm.events(), self.batch_env.number); + + let user_l2_to_l1_logs = extract_l2tol1logs_from_l1_messenger(&events) + .into_iter() + .map(Into::into) + .map(UserL2ToL1Log) + .collect(); + + CurrentExecutionState { + events, + deduplicated_storage_logs: world_diff + .get_storage_changes() + .map(|((address, key), change)| StorageLog { + key: StorageKey::new(AccountTreeId::new(address), u256_to_h256(key)), + value: u256_to_h256(change.after), + kind: StorageLogKind::RepeatedWrite, // Initialness doesn't matter here + }) + .collect(), + used_contract_hashes: self.decommitted_hashes().collect(), + system_logs: vm.l2_to_l1_logs().map(GlueInto::glue_into).collect(), + user_l2_to_l1_logs, + storage_refunds: world_diff.storage_refunds().to_vec(), + pubdata_costs: world_diff.pubdata_costs().to_vec(), + } + } +} + +impl Vm> +where + WithBuiltinTracers: Tracer, +{ fn run( &mut self, execution_mode: VmExecutionMode, - tracer: &mut WithBuiltinTracers, + tracer: &mut WithBuiltinTracers, track_refunds: bool, ) -> VmRunResult { struct AccountValidationGasSplit { @@ -251,7 +442,7 @@ impl Vm { - if Validation::STOP_AFTER_VALIDATION { + if V::STOP_AFTER_VALIDATION { break (ExecutionResult::Success { output: vec![] }, true); } } @@ -412,198 +603,9 @@ impl Vm [U256; 3] { - (get_vm_hook_params_start_position(VM_VERSION) - ..get_vm_hook_params_start_position(VM_VERSION) + VM_HOOK_PARAMS_COUNT) - .map(|word| self.read_word_from_bootloader_heap(word as usize)) - .collect::>() - .try_into() - .unwrap() - } - - fn get_tx_result(&self) -> U256 { - let tx_idx = self.bootloader_state.current_tx(); - let slot = get_result_success_first_slot(VM_VERSION) as usize + tx_idx; - self.read_word_from_bootloader_heap(slot) - } - - fn get_debug_log(&self) -> (String, String) { - let hook_params = self.get_hook_params(); - let mut msg = u256_to_h256(hook_params[0]).as_bytes().to_vec(); - // Trim 0 byte padding at the end. - while msg.last() == Some(&0) { - msg.pop(); - } - - let data = hook_params[1]; - let msg = String::from_utf8(msg).expect("Invalid debug message"); - - // For long data, it is better to use hex-encoding for greater readability - let data_str = if data > U256::from(u64::MAX) { - format!("0x{data:x}") - } else { - data.to_string() - }; - (msg, data_str) - } - - /// Should only be used when the bootloader is executing (e.g., when handling hooks). - pub(crate) fn read_word_from_bootloader_heap(&self, word: usize) -> U256 { - let start_address = word as u32 * 32; - self.inner.read_heap_u256(HeapId::FIRST, start_address) - } - - fn read_bytes_from_heap(&self, ptr: FatPointer) -> Vec { - assert_eq!(ptr.offset, 0); - (ptr.start..ptr.start + ptr.length) - .map(|addr| self.inner.read_heap_byte(ptr.memory_page, addr)) - .collect() - } - - pub(crate) fn has_previous_far_calls(&mut self) -> bool { - let callframe_count = self.inner.number_of_callframes(); - (1..callframe_count).any(|i| !self.inner.callframe(i).is_near_call()) - } - - /// Should only be used when the bootloader is executing (e.g., when handling hooks). - pub(crate) fn write_to_bootloader_heap( - &mut self, - memory: impl IntoIterator, - ) { - assert!( - !self.has_previous_far_calls(), - "Cannot write to bootloader heap when not in root call frame" - ); - - for (slot, value) in memory { - let start_address = slot as u32 * 32; - self.inner - .write_heap_u256(HeapId::FIRST, start_address, value); - } - } - - pub(crate) fn insert_bytecodes<'a>(&mut self, bytecodes: impl IntoIterator) { - for code in bytecodes { - let hash = h256_to_u256(hash_bytecode(code)); - self.world.bytecode_cache.insert(hash, code.into()); - } - } - - pub(crate) fn push_transaction_inner( - &mut self, - tx: zksync_types::Transaction, - refund: u64, - with_compression: bool, - ) { - let tx: TransactionData = tx.into(); - let overhead = tx.overhead_gas(); - - self.insert_bytecodes(tx.factory_deps.iter().map(|dep| &dep[..])); - - let compressed_bytecodes = if is_l1_tx_type(tx.tx_type) || !with_compression { - // L1 transactions do not need compression - vec![] - } else { - compress_bytecodes(&tx.factory_deps, |hash| { - self.inner - .world_diff() - .get_storage_state() - .get(&(KNOWN_CODES_STORAGE_ADDRESS, h256_to_u256(hash))) - .map(|x| !x.is_zero()) - .unwrap_or_else(|| self.world.storage.is_bytecode_known(&hash)) - }) - }; - - let trusted_ergs_limit = tx.trusted_ergs_limit(); - - let memory = self.bootloader_state.push_tx( - tx, - overhead, - refund, - compressed_bytecodes, - trusted_ergs_limit, - self.system_env.chain_id, - ); - - self.write_to_bootloader_heap(memory); - } - - #[cfg(test)] - pub(super) fn enforce_state_diffs(&mut self, diffs: Vec) { - self.enforced_state_diffs = Some(diffs); - } - - fn compute_state_diffs(&mut self) -> Vec { - #[cfg(test)] - if let Some(enforced_diffs) = self.enforced_state_diffs.take() { - return enforced_diffs; - } - - let storage = &mut self.world.storage; - let diffs = - self.inner - .world_diff() - .get_storage_changes() - .map(move |((address, key), change)| { - let storage_key = - StorageKey::new(AccountTreeId::new(address), u256_to_h256(key)); - StateDiffRecord { - address, - key, - derived_key: LogQuery::derive_final_address_for_params(&address, &key), - enumeration_index: storage - .get_enumeration_index(&storage_key) - .unwrap_or_default(), - initial_value: change.before, - final_value: change.after, - } - }); - diffs - .filter(|diff| diff.address != L1_MESSENGER_ADDRESS) - .collect() - } - - pub(crate) fn decommitted_hashes(&self) -> impl Iterator + '_ { - self.inner.world_diff().decommitted_hashes() - } - - pub(super) fn gas_remaining(&mut self) -> u32 { - self.inner.current_frame().gas() - } - - // visible for testing - pub(super) fn get_current_execution_state(&self) -> CurrentExecutionState { - let world_diff = self.inner.world_diff(); - let vm = &self.inner; - let events = merge_events(vm.events(), self.batch_env.number); - - let user_l2_to_l1_logs = extract_l2tol1logs_from_l1_messenger(&events) - .into_iter() - .map(Into::into) - .map(UserL2ToL1Log) - .collect(); - - CurrentExecutionState { - events, - deduplicated_storage_logs: world_diff - .get_storage_changes() - .map(|((address, key), change)| StorageLog { - key: StorageKey::new(AccountTreeId::new(address), u256_to_h256(key)), - value: u256_to_h256(change.after), - kind: StorageLogKind::RepeatedWrite, // Initialness doesn't matter here - }) - .collect(), - used_contract_hashes: self.decommitted_hashes().collect(), - system_logs: vm.l2_to_l1_logs().map(GlueInto::glue_into).collect(), - user_l2_to_l1_logs, - storage_refunds: world_diff.storage_refunds().to_vec(), - pubdata_costs: world_diff.pubdata_costs().to_vec(), - } - } - pub(crate) fn inspect_inner( &mut self, - tracer: &mut WithBuiltinTracers, + tracer: &mut WithBuiltinTracers, execution_mode: VmExecutionMode, ) -> VmExecutionResultAndLogs { let mut track_refunds = false; @@ -693,11 +695,10 @@ impl Vm VmFactory> - for Vm, Tr, Validation> +impl VmFactory> for Vm, Tr> where S: ReadStorage, - Tr: Tracer + Default, + Self: VmInterface, { fn new( batch_env: L1BatchEnv, @@ -709,10 +710,10 @@ where } } -impl VmInterface - for Vm +impl VmInterface + for Vm> { - type TracerDispatcher = WithBuiltinTracers; + type TracerDispatcher = WithBuiltinTracers; fn push_transaction(&mut self, tx: Transaction) -> PushTransactionResult<'_> { self.push_transaction_inner(tx, 0, true); @@ -785,8 +786,9 @@ struct VmSnapshot { bootloader_snapshot: BootloaderStateSnapshot, } -impl VmInterfaceHistoryEnabled - for Vm +impl VmInterfaceHistoryEnabled for Vm +where + Self: VmInterface, { fn make_snapshot(&mut self) { assert!( @@ -815,13 +817,16 @@ impl VmInterfaceHistory } } -impl VmTrackingContracts for Vm { +impl VmTrackingContracts for Vm +where + Self: VmInterface, +{ fn used_contract_hashes(&self) -> Vec { self.decommitted_hashes().map(u256_to_h256).collect() } } -impl fmt::Debug for Vm { +impl fmt::Debug for Vm { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Vm") .field("bootloader_state", &self.bootloader_state) diff --git a/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs b/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs index 6bab893c4b57..494a95d2212d 100644 --- a/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs +++ b/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs @@ -9,7 +9,11 @@ use super::{ #[derive(Default, Debug)] pub struct WithBuiltinTracers((External, (CircuitsTracer, Validation))); -impl WithBuiltinTracers { +pub type DefaultTracers = WithBuiltinTracersForSequencer<()>; + +pub type WithBuiltinTracersForValidation = WithBuiltinTracers; + +impl WithBuiltinTracersForValidation { pub fn for_validation(external: External, validation_params: ValidationParams) -> Self { Self(( external, @@ -25,13 +29,17 @@ impl WithBuiltinTracers { } } -impl WithBuiltinTracers { +pub type WithBuiltinTracersForApi = WithBuiltinTracers; + +impl WithBuiltinTracersForApi { pub fn for_api(external: External) -> Self { Self((external, Default::default())) } } -impl WithBuiltinTracers { +pub type WithBuiltinTracersForSequencer = WithBuiltinTracers; + +impl WithBuiltinTracersForSequencer { pub fn for_sequencer(external: External) -> Self { Self((external, Default::default())) } diff --git a/core/lib/multivm/src/vm_instance.rs b/core/lib/multivm/src/vm_instance.rs index 62ab8ed89079..89838fda3950 100644 --- a/core/lib/multivm/src/vm_instance.rs +++ b/core/lib/multivm/src/vm_instance.rs @@ -13,11 +13,8 @@ use crate::{ VmMemoryMetrics, }, tracers::TracerDispatcher, - vm_fast::{ - validation_tracer::{ValidationGasLimitOnly, ValidationMode}, - WithBuiltinTracers, - }, - vm_latest::HistoryEnabled, + vm_fast::{self, DefaultTracers}, + vm_latest::{self, HistoryEnabled}, }; /// Enumeration encompassing all supported legacy VM versions. @@ -37,7 +34,7 @@ pub enum LegacyVmInstance { VmBoojumIntegration(crate::vm_boojum_integration::Vm, H>), Vm1_4_1(crate::vm_1_4_1::Vm, H>), Vm1_4_2(crate::vm_1_4_2::Vm, H>), - Vm1_5_0(crate::vm_latest::Vm, H>), + Vm1_5_0(vm_latest::Vm, H>), } macro_rules! dispatch_legacy_vm { @@ -193,29 +190,29 @@ impl LegacyVmInstance { Self::Vm1_4_2(vm) } VmVersion::Vm1_5_0SmallBootloaderMemory => { - let vm = crate::vm_latest::Vm::new_with_subversion( + let vm = vm_latest::Vm::new_with_subversion( l1_batch_env, system_env, storage_view, - crate::vm_latest::MultiVMSubversion::SmallBootloaderMemory, + vm_latest::MultiVMSubversion::SmallBootloaderMemory, ); Self::Vm1_5_0(vm) } VmVersion::Vm1_5_0IncreasedBootloaderMemory => { - let vm = crate::vm_latest::Vm::new_with_subversion( + let vm = vm_latest::Vm::new_with_subversion( l1_batch_env, system_env, storage_view, - crate::vm_latest::MultiVMSubversion::IncreasedBootloaderMemory, + vm_latest::MultiVMSubversion::IncreasedBootloaderMemory, ); Self::Vm1_5_0(vm) } VmVersion::VmGateway => { - let vm = crate::vm_latest::Vm::new_with_subversion( + let vm = vm_latest::Vm::new_with_subversion( l1_batch_env, system_env, storage_view, - crate::vm_latest::MultiVMSubversion::Gateway, + vm_latest::MultiVMSubversion::Gateway, ); Self::Vm1_5_0(vm) } @@ -229,19 +226,19 @@ impl LegacyVmInstance { } /// Fast VM shadowed by the latest legacy VM. -pub type ShadowedFastVm = ShadowVm< +pub type ShadowedFastVm = ShadowVm< S, - crate::vm_latest::Vm, HistoryEnabled>, - crate::vm_fast::Vm, Tr, Validation>, + vm_latest::Vm, HistoryEnabled>, + vm_fast::Vm, Tr>, >; /// Fast VM variants. #[derive(Debug)] -pub enum FastVmInstance { +pub enum FastVmInstance { /// Fast VM running in isolation. - Fast(crate::vm_fast::Vm, Tr, Validation>), + Fast(vm_fast::Vm, Tr>), /// Fast VM shadowed by the latest legacy VM. - Shadowed(ShadowedFastVm), + Shadowed(ShadowedFastVm), } macro_rules! dispatch_fast_vm { @@ -253,12 +250,13 @@ macro_rules! dispatch_fast_vm { }; } -impl - VmInterface for FastVmInstance +impl VmInterface for FastVmInstance +where + vm_fast::Vm, Tr>: VmInterface, { type TracerDispatcher = ( - crate::vm_latest::TracerDispatcher, HistoryEnabled>, - WithBuiltinTracers, + vm_latest::TracerDispatcher, HistoryEnabled>, + Tr, ); fn push_transaction(&mut self, tx: Transaction) -> PushTransactionResult<'_> { @@ -305,6 +303,9 @@ impl VmInterfaceHistoryEnabled for FastVmInstance +where + vm_fast::Vm, Tr>: + VmInterface + VmInterfaceHistoryEnabled, { fn make_snapshot(&mut self) { dispatch_fast_vm!(self.make_snapshot()); @@ -319,8 +320,9 @@ impl VmInterfaceHis } } -impl - FastVmInstance +impl FastVmInstance +where + vm_fast::Vm, Tr>: VmInterface, { /// Creates an isolated fast VM. pub fn fast( @@ -328,11 +330,7 @@ impl>, ) -> Self { - Self::Fast(crate::vm_fast::Vm::new( - l1_batch_env, - system_env, - storage_view, - )) + Self::Fast(vm_fast::Vm::new(l1_batch_env, system_env, storage_view)) } /// Creates a shadowed fast VM. diff --git a/core/lib/vm_executor/src/batch/factory.rs b/core/lib/vm_executor/src/batch/factory.rs index 6ec958882cc9..d09012f4cea9 100644 --- a/core/lib/vm_executor/src/batch/factory.rs +++ b/core/lib/vm_executor/src/batch/factory.rs @@ -16,7 +16,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, pubdata_builders::pubdata_params_to_builder, tracers::CallTracer, - vm_fast, + vm_fast::{self, WithBuiltinTracersForSequencer}, vm_latest::HistoryEnabled, FastVmInstance, LegacyVmInstance, MultiVMTracer, }; @@ -150,7 +150,7 @@ type BytecodeResult = Result, BytecodeCompressionErr #[derive(Debug)] enum BatchVm { Legacy(LegacyVmInstance), - Fast(FastVmInstance), + Fast(FastVmInstance>), } macro_rules! dispatch_batch_vm { diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 7b0c62760169..194cbd771ef4 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -17,7 +17,7 @@ use once_cell::sync::OnceCell; use zksync_multivm::{ interface::{ executor::{OneshotExecutor, TransactionValidator}, - storage::{ReadStorage, StorageView, StorageWithOverrides}, + storage::{ImmutableStorageView, ReadStorage, StorageView, StorageWithOverrides}, tracer::{ValidationError, ValidationParams}, utils::{DivergenceHandler, ShadowVm}, Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, @@ -27,7 +27,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, tracers::{CallTracer, StorageInvocations, TracerDispatcher, ValidationTracer}, utils::adjust_pubdata_price_for_tx, - vm_fast::{self, validation_tracer::ValidationGasLimitOnly, WithBuiltinTracers}, + vm_fast::{self, WithBuiltinTracers, WithBuiltinTracersForApi}, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, FastVmInstance, HistoryMode, LegacyVmInstance, MultiVMTracer, @@ -293,12 +293,12 @@ where } #[derive(Debug)] -enum Vm { +enum Vm { Legacy(LegacyVmInstance), - Fast(FastVmInstance), + Fast(FastVmInstance), } -impl Vm { +impl Vm> { fn inspect_transaction_with_bytecode_compression( &mut self, missed_storage_invocation_limit: usize, @@ -419,17 +419,16 @@ impl VmSandbox { self, action: impl FnOnce(&mut Vm>, Transaction) -> T, ) -> T { - self.execute_in_vm_with_tracer::(action) + self.execute_in_vm_with_tracer(action) } - fn execute_in_vm_with_tracer< - T, - Tr: vm_fast::interface::Tracer + Default, - Validation: vm_fast::validation_tracer::ValidationMode, - >( + fn execute_in_vm_with_tracer( mut self, - action: impl FnOnce(&mut Vm, Tr, Validation>, Transaction) -> T, - ) -> T { + action: impl FnOnce(&mut Vm, Tr>, Transaction) -> T, + ) -> T + where + vm_fast::Vm>, Tr>: VmInterface, + { Self::setup_storage( &mut self.storage, &self.execution_args, diff --git a/core/tests/vm-benchmark/src/vm.rs b/core/tests/vm-benchmark/src/vm.rs index 3fd690dfb536..5e18bff54ff0 100644 --- a/core/tests/vm-benchmark/src/vm.rs +++ b/core/tests/vm-benchmark/src/vm.rs @@ -114,8 +114,7 @@ impl CountInstructions for Fast { } let (system_env, l1_batch_env) = test_env(); - let mut vm = - vm_fast::Vm::<_, InstructionCount>::custom(l1_batch_env, system_env, &*STORAGE); + let mut vm = vm_fast::Vm::custom(l1_batch_env, system_env, &*STORAGE); vm.push_transaction(tx.clone()); let mut tracer = WithBuiltinTracers::for_sequencer(InstructionCount(0)); vm.inspect(&mut tracer, InspectExecutionMode::OneTx); From 96f1886958dc3b424c5a4b864bc4c22d4f474ff6 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 30 Oct 2024 01:20:41 +0100 Subject: [PATCH 25/46] VmFactory no longer requires VmInterface This allows removing the most horrible trait bounds --- core/lib/multivm/src/versions/shadow/mod.rs | 2 +- core/lib/multivm/src/versions/vm_fast/vm.rs | 1 - core/lib/multivm/src/vm_instance.rs | 5 +---- core/lib/vm_executor/src/oneshot/mod.rs | 7 ++----- core/lib/vm_interface/src/utils/dump.rs | 2 +- core/lib/vm_interface/src/utils/shadow.rs | 19 ++++++++++++------- core/lib/vm_interface/src/vm.rs | 2 +- 7 files changed, 18 insertions(+), 20 deletions(-) diff --git a/core/lib/multivm/src/versions/shadow/mod.rs b/core/lib/multivm/src/versions/shadow/mod.rs index 42a0fbb1b8ba..32de8b8f4307 100644 --- a/core/lib/multivm/src/versions/shadow/mod.rs +++ b/core/lib/multivm/src/versions/shadow/mod.rs @@ -203,7 +203,7 @@ impl Harness { fn sanity_check_vm() -> (Vm, Harness) where - Vm: VmFactory>, + Vm: VmFactory> + VmInterface, { let system_env = default_system_env(); let l1_batch_env = default_l1_batch(L1BatchNumber(1)); diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 7fcf3410ad66..248b6256edfc 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -698,7 +698,6 @@ where impl VmFactory> for Vm, Tr> where S: ReadStorage, - Self: VmInterface, { fn new( batch_env: L1BatchEnv, diff --git a/core/lib/multivm/src/vm_instance.rs b/core/lib/multivm/src/vm_instance.rs index 89838fda3950..b17eb25b921e 100644 --- a/core/lib/multivm/src/vm_instance.rs +++ b/core/lib/multivm/src/vm_instance.rs @@ -320,10 +320,7 @@ where } } -impl FastVmInstance -where - vm_fast::Vm, Tr>: VmInterface, -{ +impl FastVmInstance { /// Creates an isolated fast VM. pub fn fast( l1_batch_env: L1BatchEnv, diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 194cbd771ef4..4069d4d0e49b 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -17,7 +17,7 @@ use once_cell::sync::OnceCell; use zksync_multivm::{ interface::{ executor::{OneshotExecutor, TransactionValidator}, - storage::{ImmutableStorageView, ReadStorage, StorageView, StorageWithOverrides}, + storage::{ReadStorage, StorageView, StorageWithOverrides}, tracer::{ValidationError, ValidationParams}, utils::{DivergenceHandler, ShadowVm}, Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, @@ -425,10 +425,7 @@ impl VmSandbox { fn execute_in_vm_with_tracer( mut self, action: impl FnOnce(&mut Vm, Tr>, Transaction) -> T, - ) -> T - where - vm_fast::Vm>, Tr>: VmInterface, - { + ) -> T { Self::setup_storage( &mut self.storage, &self.execution_args, diff --git a/core/lib/vm_interface/src/utils/dump.rs b/core/lib/vm_interface/src/utils/dump.rs index f23d6f307b89..5777a2fb8171 100644 --- a/core/lib/vm_interface/src/utils/dump.rs +++ b/core/lib/vm_interface/src/utils/dump.rs @@ -66,7 +66,7 @@ impl VmDump { /// Plays back this dump on the specified VM. pub fn play_back(self) -> Vm where - Vm: VmFactory>, + Vm: VmFactory> + VmInterface, { self.play_back_custom(Vm::new) } diff --git a/core/lib/vm_interface/src/utils/shadow.rs b/core/lib/vm_interface/src/utils/shadow.rs index d12d85fa2e3a..c2dd2fcb9ebd 100644 --- a/core/lib/vm_interface/src/utils/shadow.rs +++ b/core/lib/vm_interface/src/utils/shadow.rs @@ -230,7 +230,6 @@ impl ShadowVm where S: ReadStorage, Main: VmTrackingContracts, - Shadow: VmInterface, { /// Sets the divergence handler to be used by this VM. pub fn set_divergence_handler(&mut self, handler: DivergenceHandler) { @@ -239,6 +238,18 @@ where } } + /// Dumps the current VM state. + pub fn dump_state(&self) -> VmDump { + self.main.dump_state() + } +} + +impl ShadowVm +where + S: ReadStorage, + Main: VmTrackingContracts, + Shadow: VmInterface, +{ /// Mutable ref is not necessary, but it automatically drops potential borrows. fn report(&mut self, err: DivergenceErrors) { self.report_shared(err); @@ -252,11 +263,6 @@ where .report(err, self.main.dump_state()); } - /// Dumps the current VM state. - pub fn dump_state(&self) -> VmDump { - self.main.dump_state() - } - /// Gets the specified value from both the main and shadow VM, checking whether it matches on both. pub fn get(&self, name: &str, mut action: impl FnMut(ShadowRef<'_, Main, Shadow>) -> R) -> R where @@ -322,7 +328,6 @@ impl ShadowVm where S: ReadStorage, Main: VmFactory> + VmTrackingContracts, - Shadow: VmInterface, { /// Creates a VM with a custom shadow storage. pub fn with_custom_shadow( diff --git a/core/lib/vm_interface/src/vm.rs b/core/lib/vm_interface/src/vm.rs index 2c25d729e318..d6205e5656d7 100644 --- a/core/lib/vm_interface/src/vm.rs +++ b/core/lib/vm_interface/src/vm.rs @@ -81,7 +81,7 @@ pub trait VmInterfaceExt: VmInterface { impl VmInterfaceExt for T {} /// Encapsulates creating VM instance based on the provided environment. -pub trait VmFactory: VmInterface { +pub trait VmFactory { /// Creates a new VM instance. fn new(batch_env: L1BatchEnv, system_env: SystemEnv, storage: StoragePtr) -> Self; } From bf1f2e6ae672bf90879497f56ac2307e1ec48c32 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 30 Oct 2024 01:57:05 +0100 Subject: [PATCH 26/46] use DefaultTracers type alias --- core/lib/multivm/src/versions/vm_fast/vm.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 248b6256edfc..45a1e831a665 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -29,8 +29,8 @@ use super::{ hook::Hook, initial_bootloader_memory::bootloader_initial_memory, transaction_data::TransactionData, - validation_tracer::{ValidationGasLimitOnly, ValidationMode}, - WithBuiltinTracers, + validation_tracer::ValidationMode, + DefaultTracers, WithBuiltinTracers, }; use crate::{ glue::GlueInto, @@ -91,7 +91,7 @@ impl VmRunResult { /// The wrapper is parametric by the storage and tracer types. Besides the [`Tracer`] trait and implement [`Default`] /// (the latter is necessary to complete batches). [`CircuitsTracer`] is currently always enabled; /// you don't need to specify it explicitly. -pub struct Vm> { +pub struct Vm { pub(crate) world: World, pub(crate) inner: VirtualMachine>, pub(crate) bootloader_state: BootloaderState, From 95f1bbfb67bc68033fcd84bb6566011a05b833b6 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 30 Oct 2024 13:45:28 +0100 Subject: [PATCH 27/46] do not expose ValidationTracer, CircuitsTracer That way it isn't possible to accidentally use them instead of the intended wrappers. --- core/lib/multivm/src/versions/vm_fast/mod.rs | 3 +-- core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index d663bb5fcd74..0df64e1cdc67 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -1,7 +1,6 @@ pub use zksync_vm2::interface; pub use self::{ - circuits_tracer::CircuitsTracer, vm::Vm, with_builtin_tracers::{ DefaultTracers, WithBuiltinTracers, WithBuiltinTracersForApi, @@ -22,6 +21,6 @@ mod refund; mod tests; mod transaction_data; mod utils; -pub mod validation_tracer; +mod validation_tracer; mod vm; mod with_builtin_tracers; diff --git a/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs b/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs index 494a95d2212d..5f55d4787798 100644 --- a/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs +++ b/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs @@ -2,8 +2,8 @@ use zksync_vm2::interface::Tracer; use zksync_vm_interface::tracer::{ValidationParams, ViolatedValidationRule}; use super::{ + circuits_tracer::CircuitsTracer, validation_tracer::{ValidationGasLimitOnly, ValidationMode, ValidationTracer}, - CircuitsTracer, }; #[derive(Default, Debug)] From 9ec83f2fc2c13876ec51bc0897ba48d85b7c69b0 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 30 Oct 2024 14:12:35 +0100 Subject: [PATCH 28/46] try to make CI green --- core/lib/vm_executor/src/oneshot/mod.rs | 4 +++- core/lib/vm_interface/src/types/tracer.rs | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 4069d4d0e49b..522349ca369c 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -281,7 +281,9 @@ where _ => Ok(()), }; - // TODO compare validation to legacy + if fast_result != legacy_result { + unimplemented!("Validation result mismatch, what do I do now? (TODO this is just to make CI happy)") + } legacy_result } diff --git a/core/lib/vm_interface/src/types/tracer.rs b/core/lib/vm_interface/src/types/tracer.rs index ba07772c7f23..eb0c608b4948 100644 --- a/core/lib/vm_interface/src/types/tracer.rs +++ b/core/lib/vm_interface/src/types/tracer.rs @@ -60,7 +60,7 @@ pub struct ValidationParams { } /// Rules that can be violated when validating a transaction. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub enum ViolatedValidationRule { /// The transaction touched disallowed storage slots during validation. TouchedDisallowedStorageSlots(Address, U256), @@ -96,7 +96,7 @@ impl fmt::Display for ViolatedValidationRule { } /// Errors returned when validating a transaction. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub enum ValidationError { /// VM execution was halted during validation. FailedTx(Halt), From 75250746c9c64682590606f8a50c52df08d517d9 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 30 Oct 2024 16:34:50 +0100 Subject: [PATCH 29/46] fix validation test It was breaking because validation and execution would have required different VMs. I fixed that by setting the type of rule violation without actually running a transaction. --- .../testonly/account_validation_rules.rs | 42 +++++++------------ core/lib/multivm/src/versions/testonly/mod.rs | 4 +- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs index 5d2a4f835b50..5fe94a4ffbac 100644 --- a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -1,14 +1,14 @@ use assert_matches::assert_matches; -use ethabi::Token; use zksync_types::{ - fee::Fee, l2::L2Tx, transaction_request::TransactionRequest, Address, Eip712Domain, Execute, - L2ChainId, U256, + fee::Fee, l2::L2Tx, transaction_request::TransactionRequest, AccountTreeId, Address, + Eip712Domain, L2ChainId, StorageKey, U256, }; -use zksync_vm_interface::{tracer::ViolatedValidationRule, InspectExecutionMode, VmInterfaceExt}; +use zksync_utils::u256_to_h256; +use zksync_vm_interface::tracer::ViolatedValidationRule; use super::{ - read_validation_test_contract, tester::VmTesterBuilder, ContractToDeploy, TestedVm, - TestedVmForValidation, + get_empty_storage, read_validation_test_contract, tester::VmTesterBuilder, ContractToDeploy, + TestedVm, TestedVmForValidation, }; use crate::interface::TxExecutionMode; @@ -40,35 +40,25 @@ fn test_rule(rule: u32) -> Option(); - let mut private_account = vm.rich_accounts[0].clone(); - - // Set the type of misbehaviour of the AA contract - let function = contract.function("setTypeOfRuleBreak").unwrap(); - let transaction = private_account.get_l2_tx_for_execute( - Execute { - contract_address: Some(aa_address), - calldata: function.encode_input(&[Token::Uint(rule.into())]).unwrap(), - value: U256::zero(), - factory_deps: vec![], - }, - None, - ); - vm.vm.push_transaction(transaction); - assert!(!vm - .vm - .execute(InspectExecutionMode::OneTx) - .result - .is_failed()); + let private_account = vm.rich_accounts[0].clone(); // Use account abstraction let chain_id: u32 = 270; diff --git a/core/lib/multivm/src/versions/testonly/mod.rs b/core/lib/multivm/src/versions/testonly/mod.rs index b46d687c4612..df3e8701a4ae 100644 --- a/core/lib/multivm/src/versions/testonly/mod.rs +++ b/core/lib/multivm/src/versions/testonly/mod.rs @@ -131,9 +131,9 @@ pub(crate) fn read_simple_transfer_contract() -> Vec { ) } -pub(crate) fn read_validation_test_contract() -> (Vec, Contract) { +pub(crate) fn read_validation_test_contract() -> Vec { let path = "etc/contracts-test-data/artifacts-zk/contracts/custom-account/validation-rule-breaker.sol/ValidationRuleBreaker.json"; - (read_bytecode(path), load_contract(path)) + read_bytecode(path) } pub(crate) fn get_bootloader(test: &str) -> SystemContractCode { From eef1347f07d86b301eb54466ebb833d2c7429163 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 1 Nov 2024 20:56:49 +0100 Subject: [PATCH 30/46] grant access to timestamp to timestampasserter --- core/lib/multivm/src/versions/vm_fast/validation_tracer.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 0a204dd3b8f3..d5ef5d9e8c42 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -268,6 +268,10 @@ impl ValidationTracer { || address == SYSTEM_CONTEXT_ADDRESS && slot == U256::zero() // allow reading code hashes of existing contracts || address == ACCOUNT_CODE_STORAGE_ADDRESS && !value.is_zero() + // allow TimestampAsserter to do its job + || self.timestamp_asserter_params.as_ref() + .map(|p| p.address == caller) + .unwrap_or_default() } fn set_error(&mut self, error: ViolatedValidationRule) { From 0f47e5dae3188c24b256825b38cafbed6bf4b1f6 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 1 Nov 2024 21:41:52 +0100 Subject: [PATCH 31/46] properly compare shadow validation results --- core/lib/vm_executor/src/oneshot/mod.rs | 109 +++++++++++----------- core/lib/vm_interface/src/utils/shadow.rs | 9 ++ 2 files changed, 61 insertions(+), 57 deletions(-) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 42da1b5a3758..d663bdafa6fe 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -19,7 +19,7 @@ use zksync_multivm::{ executor::{OneshotExecutor, TransactionValidator}, storage::{ReadStorage, StorageView, StorageWithOverrides}, tracer::{ValidationError, ValidationParams, ValidationTraces}, - utils::{DivergenceHandler, ShadowVm}, + utils::{DivergenceHandler, ShadowMut, ShadowVm}, Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, OneshotTransactionExecutionResult, StoredL2BlockEnv, TxExecutionArgs, TxExecutionMode, VmFactory, VmInterface, @@ -247,67 +247,62 @@ where Vm::Fast(FastVmInstance::Shadowed(vm)) => { vm.push_transaction(transaction); - let tracer = WithBuiltinTracers::for_validation( - (), - validation_params.clone(), - l1_batch_env.timestamp, - ); - - let validation_tracer = ValidationTracer::::new( - validation_params, - version, - l1_batch_env.timestamp, - ); - let validation_traces = validation_tracer.get_traces(); - let mut validation_result = validation_tracer.get_result(); - let legacy_tracers: Box< - dyn MultiVMTracer>, HistoryEnabled>, - > = validation_tracer.into_tracer_pointer(); - let mut aggregate_tracer = ( - TracerDispatcher::<_, _>::from(legacy_tracers).into(), - tracer, - ); - let result_and_logs = - vm.inspect(&mut aggregate_tracer, InspectExecutionMode::OneTx); - - let fast_result = if let Some(violation) = - aggregate_tracer.1.validation().validation_error() - { - Err(ValidationError::ViolatedRule(violation)) - } else { - match &result_and_logs.result { - ExecutionResult::Halt { reason } => { - Err(ValidationError::FailedTx(reason.clone())) + vm.get_custom_mut("validation result", |vm| match vm { + ShadowMut::Main(vm) => { + let validation_tracer = ValidationTracer::::new( + validation_params.clone(), + version, + l1_batch_env.timestamp, + ); + let mut validation_result = validation_tracer.get_result(); + let validation_traces = validation_tracer.get_traces(); + let tracers: Box> = + validation_tracer.into_tracer_pointer(); + + let exec_result = vm.inspect( + &mut TracerDispatcher::from(tracers).into(), + InspectExecutionMode::OneTx, + ); + + let validation_result = Arc::make_mut(&mut validation_result) + .take() + .map_or(Ok(()), Err); + + match (exec_result.result, validation_result) { + (_, Err(violated_rule)) => { + Err(ValidationError::ViolatedRule(violated_rule)) + } + (ExecutionResult::Halt { reason }, _) => { + Err(ValidationError::FailedTx(reason)) + } + _ => Ok(validation_traces.lock().unwrap().clone()), } - ExecutionResult::Revert { .. } => { - unreachable!("Revert can only happen at the end of a transaction") + } + ShadowMut::Shadow(vm) => { + let mut tracer = WithBuiltinTracers::for_validation( + (), + validation_params.clone(), + l1_batch_env.timestamp, + ); + let result_and_logs = + vm.inspect(&mut tracer, InspectExecutionMode::OneTx); + if let Some(violation) = tracer.validation().validation_error() { + return Err(ValidationError::ViolatedRule(violation)); } - ExecutionResult::Success { .. } => { - Ok(aggregate_tracer.1.validation().traces()) + match result_and_logs.result { + ExecutionResult::Halt { reason } => { + Err(ValidationError::FailedTx(reason)) + } + ExecutionResult::Revert { .. } => { + unreachable!( + "Revert can only happen at the end of a transaction" + ) + } + ExecutionResult::Success { .. } => Ok(tracer.validation().traces()), } } - }; - - let validation_result = Arc::make_mut(&mut validation_result) - .take() - .map_or(Ok(()), Err); - - let legacy_result = match (result_and_logs.result, validation_result) { - (_, Err(violated_rule)) => { - Err(ValidationError::ViolatedRule(violated_rule)) - } - (ExecutionResult::Halt { reason }, _) => { - Err(ValidationError::FailedTx(reason)) - } - _ => Ok(validation_traces.lock().unwrap().clone()), - }; - - if fast_result != legacy_result { - unimplemented!("Validation result mismatch, what do I do now? (TODO this is just to make CI happy)") - } - - legacy_result + }) } }) }) diff --git a/core/lib/vm_interface/src/utils/shadow.rs b/core/lib/vm_interface/src/utils/shadow.rs index 91080f5a6fa1..f0def03e3918 100644 --- a/core/lib/vm_interface/src/utils/shadow.rs +++ b/core/lib/vm_interface/src/utils/shadow.rs @@ -13,6 +13,7 @@ use super::dump::{DumpingVm, VmDump}; use crate::{ pubdata::PubdataBuilder, storage::{ReadStorage, StoragePtr, StorageView}, + tracer::{ValidationError, ValidationTraces}, BytecodeCompressionResult, CurrentExecutionState, FinishedL1Batch, InspectExecutionMode, L1BatchEnv, L2BlockEnv, PushTransactionResult, SystemEnv, VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceHistoryEnabled, VmTrackingContracts, @@ -226,6 +227,14 @@ impl CheckDivergence for FinishedL1Batch { } } +impl CheckDivergence for Result { + fn check_divergence(&self, other: &Self) -> DivergenceErrors { + let mut errors = DivergenceErrors::new(); + errors.check_match("validation result", self, other); + errors + } +} + /// Shadowed VM that executes 2 VMs for each operation and compares their outputs. /// /// If a divergence is detected, the VM state is dumped using [a pluggable handler](Self::set_dump_handler()), From b9fcc3e97afa24154bab119bc197c1ead4ad1ba4 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 13 Nov 2024 13:56:46 +0100 Subject: [PATCH 32/46] fix bug where proxies were not allowed --- core/lib/multivm/src/versions/vm_fast/validation_tracer.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index d5ef5d9e8c42..f3df0b5a59cc 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -6,6 +6,7 @@ use zksync_types::{ KECCAK256_PRECOMPILE_ADDRESS, L2_BASE_TOKEN_ADDRESS, MSG_VALUE_SIMULATOR_ADDRESS, SYSTEM_CONTEXT_ADDRESS, U256, }; +use zksync_utils::u256_to_account_address; use zksync_vm2::interface::{ CallframeInterface, GlobalStateInterface, Opcode::*, OpcodeType, ReturnType::*, ShouldStop, Tracer, @@ -117,7 +118,8 @@ impl Tracer for ValidationTracer { .storage_containing_trusted_addresses .contains(&(address, slot)) { - self.trusted_addresses.insert(address); + self.trusted_addresses + .insert(u256_to_account_address(&state.get_storage(address, slot))); } else if !self.is_valid_storage_read( address, caller, From 938f37ae8cfe80eb404018407938341c1fcddf26 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 13 Nov 2024 14:01:24 +0100 Subject: [PATCH 33/46] remove gasleft test Most likely gasleft will be allowed in the future, it will just return a consistent value between API, sequencer and proving --- .../src/versions/testonly/account_validation_rules.rs | 7 ------- .../contracts/custom-account/validation-rule-breaker.sol | 3 --- 2 files changed, 10 deletions(-) diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs index 57e54bac0085..97ed701d2b4b 100644 --- a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -25,13 +25,6 @@ pub(crate) fn test_account_validation_rules(100), - Some(ViolatedValidationRule::TouchedDisallowedContext) - );*/ - // TODO: test running out of gas but catching the failure. // Can be accomplished via many nested far calls. } diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol index 4b16b7d8396f..fc4ca248361c 100644 --- a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -39,9 +39,6 @@ contract ValidationRuleBreaker is IAccount { address(1234567890).call(""); } else if (typeOfRuleBreak == 3) { // TODO make test that calls upgradeable proxy - } else if (typeOfRuleBreak == 100) { - // Gas may only be queried to pass everything into a far call - gasleft(); } _validateTransaction(_suggestedSignedTxHash, _transaction); From f9c9f777d9b4d0d0aaac4efc3e0889a69e5dc659 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 13 Nov 2024 14:46:12 +0100 Subject: [PATCH 34/46] fix compilation --- .../multivm/src/versions/vm_fast/validation_tracer.rs | 9 ++++----- core/lib/multivm/src/versions/vm_fast/vm.rs | 11 +++-------- core/lib/vm_executor/src/oneshot/mod.rs | 2 +- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index f3df0b5a59cc..7ee25b6412f5 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -2,11 +2,10 @@ use std::collections::HashSet; use zk_evm_1_3_1::address_to_u256; use zksync_types::{ - Address, ACCOUNT_CODE_STORAGE_ADDRESS, BOOTLOADER_ADDRESS, CONTRACT_DEPLOYER_ADDRESS, - KECCAK256_PRECOMPILE_ADDRESS, L2_BASE_TOKEN_ADDRESS, MSG_VALUE_SIMULATOR_ADDRESS, - SYSTEM_CONTEXT_ADDRESS, U256, + u256_to_address, Address, ACCOUNT_CODE_STORAGE_ADDRESS, BOOTLOADER_ADDRESS, + CONTRACT_DEPLOYER_ADDRESS, KECCAK256_PRECOMPILE_ADDRESS, L2_BASE_TOKEN_ADDRESS, + MSG_VALUE_SIMULATOR_ADDRESS, SYSTEM_CONTEXT_ADDRESS, U256, }; -use zksync_utils::u256_to_account_address; use zksync_vm2::interface::{ CallframeInterface, GlobalStateInterface, Opcode::*, OpcodeType, ReturnType::*, ShouldStop, Tracer, @@ -119,7 +118,7 @@ impl Tracer for ValidationTracer { .contains(&(address, slot)) { self.trusted_addresses - .insert(u256_to_account_address(&state.get_storage(address, slot))); + .insert(u256_to_address(&state.get_storage(address, slot))); } else if !self.is_valid_storage_read( address, caller, diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 9ff3d74a23f5..b6084b3897fe 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -52,18 +52,13 @@ use crate::{ refund::compute_refund, version::FastVmVersion, }, - vm_latest::{ - constants::{ - get_result_success_first_slot, get_vm_hook_params_start_position, get_vm_hook_position, - OPERATOR_REFUNDS_OFFSET, TX_GAS_LIMIT_OFFSET, VM_HOOK_PARAMS_COUNT, - }, - MultiVmSubversion, + vm_latest::constants::{ + get_result_success_first_slot, get_vm_hook_params_start_position, get_vm_hook_position, + OPERATOR_REFUNDS_OFFSET, TX_GAS_LIMIT_OFFSET, VM_HOOK_PARAMS_COUNT, }, VmVersion, }; -const VM_VERSION: MultiVmSubversion = MultiVmSubversion::IncreasedBootloaderMemory; - #[derive(Debug)] struct VmRunResult { execution_result: ExecutionResult, diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 7babd9ca526d..b3fcdc7a76da 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -257,7 +257,7 @@ where ); let mut validation_result = validation_tracer.get_result(); let validation_traces = validation_tracer.get_traces(); - let tracers: Box> = + let tracers: Box> = validation_tracer.into_tracer_pointer(); let exec_result = vm.inspect( From 3136879bef597e863d9a4be34b908f67f94a58af Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 13 Nov 2024 17:50:19 +0100 Subject: [PATCH 35/46] test upgradeable proxy --- .../src/versions/testonly/account_validation_rules.rs | 1 + core/lib/multivm/src/versions/testonly/tester/mod.rs | 3 ++- .../contracts/custom-account/validation-rule-breaker.sol | 5 ++++- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs index 6f295051658e..f4253558cdf1 100644 --- a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -23,6 +23,7 @@ pub(crate) fn test_account_validation_rules(2), Some(ViolatedValidationRule::CalledContractWithNoCode(_)) ); + assert_matches!(test_rule::(3), None); // TODO: test running out of gas but catching the failure. // Can be accomplished via many nested far calls. diff --git a/core/lib/multivm/src/versions/testonly/tester/mod.rs b/core/lib/multivm/src/versions/testonly/tester/mod.rs index 4603d66c023f..097d92ac70d4 100644 --- a/core/lib/multivm/src/versions/testonly/tester/mod.rs +++ b/core/lib/multivm/src/versions/testonly/tester/mod.rs @@ -245,7 +245,8 @@ pub(crate) fn validation_params(tx: &L2Tx, system: &SystemEnv) -> ValidationPara paymaster_address, trusted_slots: Default::default(), trusted_addresses: Default::default(), - trusted_address_slots: Default::default(), + // field `trustedAddress` of ValidationRuleBreaker + trusted_address_slots: [(Address::repeat_byte(0x10), 2.into())].into(), computational_gas_limit: system.default_validation_computational_gas_limit, timestamp_asserter_params: None, } diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol index fc4ca248361c..8c0b981e3258 100644 --- a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -13,6 +13,7 @@ contract ValidationRuleBreaker is IAccount { using TransactionHelper for Transaction; uint32 public typeOfRuleBreak; + address public trustedAddress = address(0x800a); constructor() { typeOfRuleBreak = 0; @@ -38,7 +39,9 @@ contract ValidationRuleBreaker is IAccount { // May not call an EOA address(1234567890).call(""); } else if (typeOfRuleBreak == 3) { - // TODO make test that calls upgradeable proxy + // This should succeed because a trustedAddress is marked as a slot that grants access to the address it contains + require(trustedAddress == address(0x800a)); + require(BOOTLOADER_FORMAL_ADDRESS.balance != 0); } _validateTransaction(_suggestedSignedTxHash, _transaction); From 408d9b0228ec74587e0e24e0d7330ea98fb91a41 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 13 Nov 2024 17:59:03 +0100 Subject: [PATCH 36/46] test catching out of gas --- .../src/versions/testonly/account_validation_rules.rs | 7 ++++--- .../contracts/custom-account/validation-rule-breaker.sol | 7 +++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs index f4253558cdf1..2559e4b0c58b 100644 --- a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -24,9 +24,10 @@ pub(crate) fn test_account_validation_rules(3), None); - - // TODO: test running out of gas but catching the failure. - // Can be accomplished via many nested far calls. + assert_matches!( + test_rule::(4), + Some(ViolatedValidationRule::TookTooManyComputationalGas(_)) + ) } fn test_rule(rule: u32) -> Option { diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol index 8c0b981e3258..13d0defbd380 100644 --- a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -42,11 +42,18 @@ contract ValidationRuleBreaker is IAccount { // This should succeed because a trustedAddress is marked as a slot that grants access to the address it contains require(trustedAddress == address(0x800a)); require(BOOTLOADER_FORMAL_ADDRESS.balance != 0); + } else if (typeOfRuleBreak == 4) { + // This should still fail; EIP-4337 defines out of gas as an immediate failure + _runOutOfGasButCatchThePanic(); } _validateTransaction(_suggestedSignedTxHash, _transaction); } + function _runOutOfGasButCatchThePanic() internal { + bool success = _runOutOfGasButCatchThePanic(); + } + function _validateTransaction( bytes32 _suggestedSignedTxHash, Transaction calldata _transaction From 29cfabe8caeccc56f16deeb7e952a94b3e4fcfc9 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 13 Nov 2024 23:03:55 +0100 Subject: [PATCH 37/46] fix test solidity --- .../custom-account/validation-rule-breaker.sol | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol index 13d0defbd380..45961f705be7 100644 --- a/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol +++ b/etc/contracts-test-data/contracts/custom-account/validation-rule-breaker.sol @@ -44,14 +44,18 @@ contract ValidationRuleBreaker is IAccount { require(BOOTLOADER_FORMAL_ADDRESS.balance != 0); } else if (typeOfRuleBreak == 4) { // This should still fail; EIP-4337 defines out of gas as an immediate failure - _runOutOfGasButCatchThePanic(); + address(this).call( + abi.encodeWithSignature("_runOutOfGasButCatchThePanic()") + ); } _validateTransaction(_suggestedSignedTxHash, _transaction); } - function _runOutOfGasButCatchThePanic() internal { - bool success = _runOutOfGasButCatchThePanic(); + function _runOutOfGasButCatchThePanic() external { + address(this).call( + abi.encodeWithSignature("_runOutOfGasButCatchThePanic()") + ); } function _validateTransaction( From 85f33c32202316da3a1c93152fe28357d448c64e Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Wed, 13 Nov 2024 23:14:24 +0100 Subject: [PATCH 38/46] make vm_latest validation fail on out of gas, even if caught --- core/lib/multivm/src/tracers/validator/vm_latest/mod.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/core/lib/multivm/src/tracers/validator/vm_latest/mod.rs b/core/lib/multivm/src/tracers/validator/vm_latest/mod.rs index 8652ece9d4ba..5588dd144e95 100644 --- a/core/lib/multivm/src/tracers/validator/vm_latest/mod.rs +++ b/core/lib/multivm/src/tracers/validator/vm_latest/mod.rs @@ -1,6 +1,6 @@ use zk_evm_1_5_0::{ tracing::{BeforeExecutionData, VmLocalStateData}, - zkevm_opcode_defs::{ContextOpcode, FarCallABI, LogOpcode, Opcode}, + zkevm_opcode_defs::{ContextOpcode, FarCallABI, LogOpcode, Opcode, RetOpcode}, }; use zksync_system_constants::KECCAK256_PRECOMPILE_ADDRESS; use zksync_types::{ @@ -167,6 +167,13 @@ impl ValidationTracer { }); } } + + Opcode::Ret(RetOpcode::Panic) + if state.vm_local_state.callstack.current.ergs_remaining == 0 => + { + // Actual gas limit was reached, not the validation gas limit. + return Err(ViolatedValidationRule::TookTooManyComputationalGas(0)); + } _ => {} } From 9df63c22d598f9103720ee105100b506cf990590 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Thu, 14 Nov 2024 13:18:30 +0100 Subject: [PATCH 39/46] reduce repetition --- .../testonly/account_validation_rules.rs | 49 +++---------------- .../src/versions/testonly/require_eip712.rs | 38 +++++++++----- 2 files changed, 32 insertions(+), 55 deletions(-) diff --git a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs index 2559e4b0c58b..dd72728b5fd9 100644 --- a/core/lib/multivm/src/versions/testonly/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/testonly/account_validation_rules.rs @@ -1,13 +1,10 @@ use assert_matches::assert_matches; -use zksync_types::{ - fee::Fee, l2::L2Tx, transaction_request::TransactionRequest, u256_to_h256, AccountTreeId, - Address, Eip712Domain, L2ChainId, StorageKey, U256, -}; +use zksync_types::{u256_to_h256, AccountTreeId, Address, StorageKey}; use zksync_vm_interface::tracer::ViolatedValidationRule; use super::{ - get_empty_storage, read_validation_test_contract, tester::VmTesterBuilder, ContractToDeploy, - TestedVm, TestedVmForValidation, + get_empty_storage, read_validation_test_contract, require_eip712::make_aa_transaction, + tester::VmTesterBuilder, ContractToDeploy, TestedVm, TestedVmForValidation, }; use crate::interface::TxExecutionMode; @@ -54,40 +51,8 @@ fn test_rule(rule: u32) -> Option() { .with_rich_accounts(1) .build::(); assert_eq!(vm.get_eth_balance(beneficiary_address), U256::from(0)); - let chain_id: u32 = 270; let mut private_account = vm.rich_accounts[0].clone(); // First, let's set the owners of the AA account to the `private_address`. @@ -99,6 +99,29 @@ pub(crate) fn test_require_eip712() { ); // // Now send the 'classic' EIP712 transaction + + let transaction: Transaction = + make_aa_transaction(aa_address, beneficiary_address, &private_account).into(); + vm.vm.push_transaction(transaction); + vm.vm.execute(InspectExecutionMode::OneTx); + + assert_eq!( + vm.get_eth_balance(beneficiary_address), + U256::from(916375026) + ); + assert_eq!( + private_account_balance, + vm.get_eth_balance(private_account.address) + ); +} + +pub(crate) fn make_aa_transaction( + aa_address: Address, + beneficiary_address: Address, + private_account: &Account, +) -> L2Tx { + let chain_id: u32 = 270; + let tx_712 = L2Tx::new( Some(beneficiary_address), vec![], @@ -131,16 +154,5 @@ pub(crate) fn test_require_eip712() { let mut l2_tx = L2Tx::from_request(aa_txn_request, 100000, false).unwrap(); l2_tx.set_input(encoded_tx, aa_hash); - let transaction: Transaction = l2_tx.into(); - vm.vm.push_transaction(transaction); - vm.vm.execute(InspectExecutionMode::OneTx); - - assert_eq!( - vm.get_eth_balance(beneficiary_address), - U256::from(916375026) - ); - assert_eq!( - private_account_balance, - vm.get_eth_balance(private_account.address) - ); + l2_tx } From a33ec8e0235f42a0aa08478e071cefda5eb3f2fe Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Thu, 14 Nov 2024 18:05:34 +0100 Subject: [PATCH 40/46] implement bikeshedding-type review comments --- .../src/versions/testonly/require_eip712.rs | 2 +- ..._builtin_tracers.rs => builtin_tracers.rs} | 20 +++++++++---------- core/lib/multivm/src/versions/vm_fast/mod.rs | 9 ++++----- .../vm_fast/tests/account_validation_rules.rs | 5 +++-- .../multivm/src/versions/vm_fast/tests/mod.rs | 13 +++++------- .../src/versions/vm_fast/validation_tracer.rs | 12 +++++------ core/lib/multivm/src/versions/vm_fast/vm.rs | 15 +++++++------- core/lib/vm_executor/src/batch/factory.rs | 4 ++-- core/lib/vm_executor/src/oneshot/mod.rs | 4 ++-- 9 files changed, 41 insertions(+), 43 deletions(-) rename core/lib/multivm/src/versions/vm_fast/{with_builtin_tracers.rs => builtin_tracers.rs} (74%) diff --git a/core/lib/multivm/src/versions/testonly/require_eip712.rs b/core/lib/multivm/src/versions/testonly/require_eip712.rs index 9862b9277432..55cf7933154f 100644 --- a/core/lib/multivm/src/versions/testonly/require_eip712.rs +++ b/core/lib/multivm/src/versions/testonly/require_eip712.rs @@ -98,7 +98,7 @@ pub(crate) fn test_require_eip712() { vm.get_eth_balance(private_account.address) ); - // // Now send the 'classic' EIP712 transaction + // Now send the 'classic' EIP712 transaction let transaction: Transaction = make_aa_transaction(aa_address, beneficiary_address, &private_account).into(); diff --git a/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs b/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs similarity index 74% rename from core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs rename to core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs index 1d58b7ac16cb..8d5e49872b69 100644 --- a/core/lib/multivm/src/versions/vm_fast/with_builtin_tracers.rs +++ b/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs @@ -4,7 +4,7 @@ use zksync_vm_interface::tracer::ValidationParams; use super::{ circuits_tracer::CircuitsTracer, evm_deploy_tracer::{DynamicBytecodes, EvmDeployTracer}, - validation_tracer::{ValidationGasLimitOnly, ValidationMode, ValidationTracer}, + validation_tracer::{FullValidationTracer, ValidationGasLimitOnly, ValidationTracer}, }; #[derive(Debug, Default)] @@ -12,11 +12,11 @@ pub struct WithBuiltinTracers( (External, (Validation, (CircuitsTracer, EvmDeployTracer))), ); -pub type DefaultTracers = WithBuiltinTracersForSequencer<()>; +pub type DefaultTracers = SequencerTracers<()>; -pub type WithBuiltinTracersForValidation = WithBuiltinTracers; +pub type ValidationTracers = WithBuiltinTracers; -impl WithBuiltinTracersForValidation { +impl ValidationTracers { pub fn for_validation( external: External, validation_params: ValidationParams, @@ -25,24 +25,24 @@ impl WithBuiltinTracersForValidation { Self(( external, ( - ValidationTracer::new(validation_params, timestamp), + FullValidationTracer::new(validation_params, timestamp), Default::default(), ), )) } } -pub type WithBuiltinTracersForApi = WithBuiltinTracers; +pub type ApiTracers = WithBuiltinTracers; -impl WithBuiltinTracersForApi { +impl ApiTracers { pub fn for_api(external: External) -> Self { Self((external, Default::default())) } } -pub type WithBuiltinTracersForSequencer = WithBuiltinTracers; +pub type SequencerTracers = WithBuiltinTracers; -impl WithBuiltinTracersForSequencer { +impl SequencerTracers { pub fn for_sequencer(external: External) -> Self { Self((external, Default::default())) } @@ -70,7 +70,7 @@ impl WithBuiltinTracers { } } -impl Tracer +impl Tracer for WithBuiltinTracers { fn before_instruction< diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index df63258c6f05..fd41bfa2deca 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -2,14 +2,14 @@ pub use zksync_vm2::interface; pub(crate) use self::version::FastVmVersion; pub use self::{ - vm::Vm, - with_builtin_tracers::{ - DefaultTracers, WithBuiltinTracers, WithBuiltinTracersForApi, - WithBuiltinTracersForSequencer, WithBuiltinTracersForValidation, + builtin_tracers::{ + ApiTracers, DefaultTracers, SequencerTracers, ValidationTracers, WithBuiltinTracers, }, + vm::Vm, }; mod bootloader_state; +mod builtin_tracers; mod bytecode; mod circuits_tracer; mod events; @@ -25,4 +25,3 @@ mod utils; mod validation_tracer; mod version; mod vm; -mod with_builtin_tracers; diff --git a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs index 3623ec5c95d5..cc76b20eebfd 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs @@ -1,9 +1,10 @@ +use super::TestedFastVm; use crate::{ versions::testonly::account_validation_rules::test_account_validation_rules, - vm_fast::{Vm, WithBuiltinTracersForValidation}, + vm_fast::ValidationTracers, }; #[test] fn test_account_validation_rules_fast() { - test_account_validation_rules::>>(); + test_account_validation_rules::>>(); } diff --git a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs index 8c511a0e662a..2aa10dcec394 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -10,9 +10,7 @@ use zksync_vm_interface::{ VmExecutionResultAndLogs, VmInterface, }; -use super::{ - validation_tracer::ValidationMode, Vm, WithBuiltinTracers, WithBuiltinTracersForValidation, -}; +use super::{validation_tracer::ValidationTracer, ValidationTracers, Vm, WithBuiltinTracers}; use crate::{ interface::storage::{ImmutableStorageView, InMemoryStorage}, versions::testonly::{validation_params, TestedVm, TestedVmForValidation}, @@ -81,8 +79,9 @@ impl PartialEq for VmStateDump { } } -impl TestedVm - for Vm, WithBuiltinTracers> +pub(crate) type TestedFastVm = Vm, Tr>; + +impl TestedVm for TestedFastVm> where WithBuiltinTracers: std::fmt::Debug + 'static, { @@ -172,9 +171,7 @@ where } } -impl TestedVmForValidation - for Vm, WithBuiltinTracersForValidation<()>> -{ +impl TestedVmForValidation for Vm, ValidationTracers<()>> { fn run_validation(&mut self, tx: L2Tx, timestamp: u64) -> Option { let validation_params = validation_params(&tx, &self.system_env); self.push_transaction(tx.into()); diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index 7ee25b6412f5..c0d8201254a1 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -17,7 +17,7 @@ use zksync_vm_interface::tracer::{ use super::utils::read_fat_pointer; use crate::tracers::TIMESTAMP_ASSERTER_FUNCTION_SELECTOR; -pub trait ValidationMode: Tracer + Default { +pub trait ValidationTracer: Tracer + Default { const STOP_AFTER_VALIDATION: bool; fn account_validation_entered(&mut self); fn validation_exited(&mut self); @@ -26,7 +26,7 @@ pub trait ValidationMode: Tracer + Default { #[derive(Debug, Default)] pub struct ValidationGasLimitOnly; impl Tracer for ValidationGasLimitOnly {} -impl ValidationMode for ValidationGasLimitOnly { +impl ValidationTracer for ValidationGasLimitOnly { const STOP_AFTER_VALIDATION: bool = false; fn account_validation_entered(&mut self) {} fn validation_exited(&mut self) {} @@ -57,7 +57,7 @@ impl ValidationMode for ValidationGasLimitOnly { /// that a contract adheres to the rules ahead of time would be challenging or even impossible, /// considering that contracts that the code depends on may get upgraded. #[derive(Debug, Default)] -pub struct ValidationTracer { +pub struct FullValidationTracer { in_validation: bool, add_return_value_to_allowed_slots: bool, @@ -75,7 +75,7 @@ pub struct ValidationTracer { traces: ValidationTraces, } -impl ValidationMode for ValidationTracer { +impl ValidationTracer for FullValidationTracer { const STOP_AFTER_VALIDATION: bool = true; fn account_validation_entered(&mut self) { @@ -87,7 +87,7 @@ impl ValidationMode for ValidationTracer { } } -impl Tracer for ValidationTracer { +impl Tracer for FullValidationTracer { fn before_instruction(&mut self, state: &mut S) { if !self.in_validation { return; @@ -223,7 +223,7 @@ impl Tracer for ValidationTracer { } } -impl ValidationTracer { +impl FullValidationTracer { pub fn new(params: ValidationParams, l1_batch_timestamp: u64) -> Self { let ValidationParams { user_address, diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 2c5471fb85d7..8e63576b26b7 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -31,7 +31,7 @@ use super::{ hook::Hook, initial_bootloader_memory::bootloader_initial_memory, transaction_data::TransactionData, - validation_tracer::ValidationMode, + validation_tracer::ValidationTracer, DefaultTracers, WithBuiltinTracers, }; use crate::{ @@ -366,7 +366,12 @@ impl Vm { } } -impl Vm> +struct AccountValidationGasSplit { + gas_given: u32, + gas_hidden: u32, +} + +impl Vm> where WithBuiltinTracers: Tracer, { @@ -377,10 +382,6 @@ where track_refunds: bool, pubdata_builder: Option<&dyn PubdataBuilder>, ) -> VmRunResult { - struct AccountValidationGasSplit { - gas_given: u32, - gas_hidden: u32, - } let mut gas_left_for_account_validation = self.system_env.default_validation_computational_gas_limit; let mut account_validation_gas_split = None; @@ -744,7 +745,7 @@ where } } -impl VmInterface +impl VmInterface for Vm> { type TracerDispatcher = WithBuiltinTracers; diff --git a/core/lib/vm_executor/src/batch/factory.rs b/core/lib/vm_executor/src/batch/factory.rs index 0416dee8ac7b..149c6aa4cf87 100644 --- a/core/lib/vm_executor/src/batch/factory.rs +++ b/core/lib/vm_executor/src/batch/factory.rs @@ -16,7 +16,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, pubdata_builders::pubdata_params_to_builder, tracers::CallTracer, - vm_fast::{self, WithBuiltinTracersForSequencer}, + vm_fast::{self, SequencerTracers}, vm_latest::HistoryEnabled, FastVmInstance, LegacyVmInstance, MultiVmTracer, }; @@ -150,7 +150,7 @@ type BytecodeResult = Result, BytecodeCompressionErr #[derive(Debug)] enum BatchVm { Legacy(LegacyVmInstance), - Fast(FastVmInstance>), + Fast(FastVmInstance>), } macro_rules! dispatch_batch_vm { diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index b3fcdc7a76da..e92886a23432 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -27,7 +27,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, tracers::{CallTracer, StorageInvocations, TracerDispatcher, ValidationTracer}, utils::adjust_pubdata_price_for_tx, - vm_fast::{self, WithBuiltinTracers, WithBuiltinTracersForApi}, + vm_fast::{self, ApiTracers, WithBuiltinTracers}, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, FastVmInstance, HistoryMode, LegacyVmInstance, MultiVmTracer, @@ -317,7 +317,7 @@ enum Vm { Fast(FastVmInstance), } -impl Vm> { +impl Vm> { fn inspect_transaction_with_bytecode_compression( &mut self, missed_storage_invocation_limit: usize, From 4798cb594c6607e6d7cc422f76fac4384191c508 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Thu, 14 Nov 2024 18:38:16 +0100 Subject: [PATCH 41/46] crazy solution to .0.1.1 chains --- .../src/versions/vm_fast/builtin_tracers.rs | 36 ++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs b/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs index 8d5e49872b69..6a1ac5b86c5f 100644 --- a/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs +++ b/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs @@ -9,7 +9,10 @@ use super::{ #[derive(Debug, Default)] pub struct WithBuiltinTracers( - (External, (Validation, (CircuitsTracer, EvmDeployTracer))), + ( + External, + (Validation, (CircuitsTracer, (EvmDeployTracer, ()))), + ), ); pub type DefaultTracers = SequencerTracers<()>; @@ -54,19 +57,16 @@ impl WithBuiltinTracers { } pub fn validation(&mut self) -> &mut Validation { - &mut self.0 .1 .0 + self.0.get() } pub(super) fn circuit(&mut self) -> &mut CircuitsTracer { - &mut self.0 .1 .1 .0 + self.0.get() } pub(super) fn insert_dynamic_bytecodes_handle(&mut self, dynamic_bytecodes: DynamicBytecodes) { - self.0 - .1 - .1 - .1 - .insert_dynamic_bytecodes_handle(dynamic_bytecodes) + let deploy_tracer: &mut EvmDeployTracer = self.0.get(); + deploy_tracer.insert_dynamic_bytecodes_handle(dynamic_bytecodes) } } @@ -97,3 +97,23 @@ impl Tracer self.0.on_extra_prover_cycles(stats); } } + +// Heteregeneus list item access + +struct Here; +struct Later(T); +trait ListGet { + fn get(&mut self) -> &mut T; +} + +impl ListGet for (T, U) { + fn get(&mut self) -> &mut T { + &mut self.0 + } +} + +impl> ListGet> for (U, V) { + fn get(&mut self) -> &mut T { + self.1.get() + } +} From 0345875dbe16247ebb8139b4d8d36a73cf57337c Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Fri, 15 Nov 2024 14:35:56 +0100 Subject: [PATCH 42/46] proper hlist library to shut up clippy --- .../src/versions/vm_fast/builtin_tracers.rs | 29 ++++------ .../lib/multivm/src/versions/vm_fast/hlist.rs | 56 +++++++++++++++++++ core/lib/multivm/src/versions/vm_fast/mod.rs | 2 + 3 files changed, 68 insertions(+), 19 deletions(-) create mode 100644 core/lib/multivm/src/versions/vm_fast/hlist.rs diff --git a/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs b/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs index 6a1ac5b86c5f..ef6b3c7d2a45 100644 --- a/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs +++ b/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs @@ -1,18 +1,17 @@ +use std::fmt::{self, Debug, Formatter}; + use zksync_vm2::interface::Tracer; use zksync_vm_interface::tracer::ValidationParams; use super::{ circuits_tracer::CircuitsTracer, evm_deploy_tracer::{DynamicBytecodes, EvmDeployTracer}, + hlist::{debug_hlist, hlist, HListGet}, validation_tracer::{FullValidationTracer, ValidationGasLimitOnly, ValidationTracer}, }; -#[derive(Debug, Default)] pub struct WithBuiltinTracers( - ( - External, - (Validation, (CircuitsTracer, (EvmDeployTracer, ()))), - ), + hlist![External, Validation, CircuitsTracer, EvmDeployTracer], ); pub type DefaultTracers = SequencerTracers<()>; @@ -98,22 +97,14 @@ impl Tracer } } -// Heteregeneus list item access - -struct Here; -struct Later(T); -trait ListGet { - fn get(&mut self) -> &mut T; -} - -impl ListGet for (T, U) { - fn get(&mut self) -> &mut T { - &mut self.0 +impl Default for WithBuiltinTracers { + fn default() -> Self { + Self(Default::default()) } } -impl> ListGet> for (U, V) { - fn get(&mut self) -> &mut T { - self.1.get() +impl Debug for WithBuiltinTracers { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + debug_hlist(&self.0, f) } } diff --git a/core/lib/multivm/src/versions/vm_fast/hlist.rs b/core/lib/multivm/src/versions/vm_fast/hlist.rs new file mode 100644 index 000000000000..f811aaa48659 --- /dev/null +++ b/core/lib/multivm/src/versions/vm_fast/hlist.rs @@ -0,0 +1,56 @@ +//! # Heterogeneous lists +//! A HList is an array that can contain elements of different types. +//! At runtime it works like a struct but at compile time it is more convenient. +//! The tracer interface is automatically implements the `Tracer` trait for tuple constructions like this. + +use std::fmt::{self, DebugList, Formatter}; + +macro_rules! hlist { + [] => (); + [$head:tt] => { + ($head, ()) + }; + [$head:tt, $($tail:tt),*] => { + ($head, hlist![$($tail),*]) + }; +} +pub(crate) use hlist; + +pub(crate) fn debug_hlist(hlist: &T, f: &mut Formatter<'_>) -> fmt::Result { + let mut list = f.debug_list(); + hlist.debug(&mut list); + list.finish() +} + +pub(crate) trait DebugHList { + fn debug(&self, f: &mut DebugList<'_, '_>); +} + +impl DebugHList for (T, W) { + fn debug(&self, list: &mut DebugList<'_, '_>) { + list.entry(&self.0); + self.1.debug(list) + } +} + +impl DebugHList for () { + fn debug(&self, _: &mut DebugList<'_, '_>) {} +} + +pub(crate) struct Here; +pub(crate) struct Later(T); +pub(crate) trait HListGet { + fn get(&mut self) -> &mut T; +} + +impl HListGet for (T, U) { + fn get(&mut self) -> &mut T { + &mut self.0 + } +} + +impl> HListGet> for (U, V) { + fn get(&mut self) -> &mut T { + self.1.get() + } +} diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index fd41bfa2deca..3b87456225f9 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -15,6 +15,8 @@ mod circuits_tracer; mod events; mod evm_deploy_tracer; mod glue; +#[macro_use] +mod hlist; mod hook; mod initial_bootloader_memory; mod refund; From 1343f907bc152101f641d2cfde20b5f10b7080f6 Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Tue, 3 Dec 2024 18:15:35 +0200 Subject: [PATCH 43/46] Simplify tracers in fast VM --- core/lib/multivm/src/versions/shadow/mod.rs | 2 +- .../src/versions/vm_fast/builtin_tracers.rs | 143 +++++++----------- .../multivm/src/versions/vm_fast/bytecode.rs | 2 +- .../src/versions/vm_fast/evm_deploy_tracer.rs | 26 +--- .../lib/multivm/src/versions/vm_fast/hlist.rs | 56 ------- core/lib/multivm/src/versions/vm_fast/mod.rs | 6 +- .../vm_fast/tests/account_validation_rules.rs | 7 +- .../multivm/src/versions/vm_fast/tests/mod.rs | 20 +-- .../src/versions/vm_fast/validation_tracer.rs | 5 +- core/lib/multivm/src/versions/vm_fast/vm.rs | 77 ++++++---- core/lib/multivm/src/vm_instance.rs | 35 +++-- core/lib/vm_executor/src/batch/factory.rs | 4 +- core/lib/vm_executor/src/oneshot/mod.rs | 47 +++--- core/lib/vm_interface/src/vm.rs | 1 + core/tests/vm-benchmark/src/vm.rs | 6 +- 15 files changed, 176 insertions(+), 261 deletions(-) delete mode 100644 core/lib/multivm/src/versions/vm_fast/hlist.rs diff --git a/core/lib/multivm/src/versions/shadow/mod.rs b/core/lib/multivm/src/versions/shadow/mod.rs index 8ff240eda22f..eb55fc107bec 100644 --- a/core/lib/multivm/src/versions/shadow/mod.rs +++ b/core/lib/multivm/src/versions/shadow/mod.rs @@ -25,7 +25,7 @@ use crate::{ mod tests; type ReferenceVm = vm_latest::Vm, HistoryEnabled>; -type ShadowedFastVm = crate::vm_instance::ShadowedFastVm; +type ShadowedFastVm = crate::vm_instance::ShadowedFastVm; fn hash_block(block_env: L2BlockEnv, tx_hashes: &[H256]) -> H256 { let mut hasher = L2BlockHasher::new( diff --git a/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs b/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs index ef6b3c7d2a45..4a02b5b122b7 100644 --- a/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs +++ b/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs @@ -1,110 +1,77 @@ -use std::fmt::{self, Debug, Formatter}; - -use zksync_vm2::interface::Tracer; -use zksync_vm_interface::tracer::ValidationParams; +use zksync_vm2::interface::{CycleStats, GlobalStateInterface, OpcodeType, ShouldStop, Tracer}; use super::{ circuits_tracer::CircuitsTracer, evm_deploy_tracer::{DynamicBytecodes, EvmDeployTracer}, - hlist::{debug_hlist, hlist, HListGet}, - validation_tracer::{FullValidationTracer, ValidationGasLimitOnly, ValidationTracer}, + validation_tracer::ValidationTracer, }; -pub struct WithBuiltinTracers( - hlist![External, Validation, CircuitsTracer, EvmDeployTracer], -); - -pub type DefaultTracers = SequencerTracers<()>; - -pub type ValidationTracers = WithBuiltinTracers; - -impl ValidationTracers { - pub fn for_validation( - external: External, - validation_params: ValidationParams, - timestamp: u64, - ) -> Self { - Self(( - external, - ( - FullValidationTracer::new(validation_params, timestamp), - Default::default(), - ), - )) - } +#[derive(Debug)] +pub(super) struct WithBuiltinTracers { + pub external: Ext, + pub validation: Val, + pub circuits: CircuitsTracer, + pub evm_deploy_tracer: EvmDeployTracer, } -pub type ApiTracers = WithBuiltinTracers; - -impl ApiTracers { - pub fn for_api(external: External) -> Self { - Self((external, Default::default())) - } -} - -pub type SequencerTracers = WithBuiltinTracers; - -impl SequencerTracers { - pub fn for_sequencer(external: External) -> Self { - Self((external, Default::default())) +impl WithBuiltinTracers { + pub(super) fn new(external: Tr, validation: Val, dynamic_bytecodes: DynamicBytecodes) -> Self { + Self { + external, + validation, + circuits: CircuitsTracer::default(), + evm_deploy_tracer: EvmDeployTracer::new(dynamic_bytecodes), + } } } -impl WithBuiltinTracers { - pub fn into_inner(self) -> External { - self.0 .0 - } - - pub fn validation(&mut self) -> &mut Validation { - self.0.get() - } - - pub(super) fn circuit(&mut self) -> &mut CircuitsTracer { - self.0.get() - } - - pub(super) fn insert_dynamic_bytecodes_handle(&mut self, dynamic_bytecodes: DynamicBytecodes) { - let deploy_tracer: &mut EvmDeployTracer = self.0.get(); - deploy_tracer.insert_dynamic_bytecodes_handle(dynamic_bytecodes) +#[cfg(test)] +impl WithBuiltinTracers { + pub(super) fn mock() -> Self { + Self::new(Tr::default(), Val::default(), DynamicBytecodes::default()) } } -impl Tracer - for WithBuiltinTracers -{ - fn before_instruction< - OP: zksync_vm2::interface::OpcodeType, - S: zksync_vm2::interface::GlobalStateInterface, - >( - &mut self, - state: &mut S, - ) { - self.0.before_instruction::(state) +impl Tracer for WithBuiltinTracers { + #[inline(always)] + fn before_instruction(&mut self, state: &mut S) { + self.validation.before_instruction::(state); + self.external.before_instruction::(state); + self.circuits.before_instruction::(state); + self.evm_deploy_tracer.before_instruction::(state); } - fn after_instruction< - OP: zksync_vm2::interface::OpcodeType, - S: zksync_vm2::interface::GlobalStateInterface, - >( + #[inline(always)] + fn after_instruction( &mut self, state: &mut S, - ) -> zksync_vm2::interface::ShouldStop { - self.0.after_instruction::(state) + ) -> ShouldStop { + if matches!( + self.validation.after_instruction::(state), + ShouldStop::Stop + ) { + return ShouldStop::Stop; + } + if matches!( + self.external.after_instruction::(state), + ShouldStop::Stop + ) { + return ShouldStop::Stop; + } + if matches!( + self.circuits.after_instruction::(state), + ShouldStop::Stop + ) { + return ShouldStop::Stop; + } + self.evm_deploy_tracer.after_instruction::(state) } - fn on_extra_prover_cycles(&mut self, stats: zksync_vm2::interface::CycleStats) { - self.0.on_extra_prover_cycles(stats); - } -} - -impl Default for WithBuiltinTracers { - fn default() -> Self { - Self(Default::default()) - } -} - -impl Debug for WithBuiltinTracers { - fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - debug_hlist(&self.0, f) + #[inline(always)] + fn on_extra_prover_cycles(&mut self, stats: CycleStats) { + self.validation.on_extra_prover_cycles(stats); + self.external.on_extra_prover_cycles(stats); + self.circuits.on_extra_prover_cycles(stats); + self.evm_deploy_tracer.on_extra_prover_cycles(stats); } } diff --git a/core/lib/multivm/src/versions/vm_fast/bytecode.rs b/core/lib/multivm/src/versions/vm_fast/bytecode.rs index 4dc52951c16c..aec5ed9ae301 100644 --- a/core/lib/multivm/src/versions/vm_fast/bytecode.rs +++ b/core/lib/multivm/src/versions/vm_fast/bytecode.rs @@ -7,7 +7,7 @@ use crate::{ utils::bytecode, }; -impl Vm { +impl Vm { /// Checks the last transaction has successfully published compressed bytecodes and returns `true` if there is at least one is still unknown. pub(crate) fn has_unpublished_bytecodes(&mut self) -> bool { self.bootloader_state diff --git a/core/lib/multivm/src/versions/vm_fast/evm_deploy_tracer.rs b/core/lib/multivm/src/versions/vm_fast/evm_deploy_tracer.rs index 0ee4310cb1a1..c443c99ccf9a 100644 --- a/core/lib/multivm/src/versions/vm_fast/evm_deploy_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/evm_deploy_tracer.rs @@ -32,22 +32,19 @@ impl DynamicBytecodes { #[derive(Debug)] pub(super) struct EvmDeployTracer { tracked_signature: [u8; 4], - bytecodes: Option, + bytecodes: DynamicBytecodes, } -impl Default for EvmDeployTracer { - fn default() -> Self { +impl EvmDeployTracer { + pub(super) fn new(bytecodes: DynamicBytecodes) -> Self { + let tracked_signature = + ethabi::short_signature("publishEVMBytecode", &[ethabi::ParamType::Bytes]); Self { - tracked_signature: ethabi::short_signature( - "publishEVMBytecode", - &[ethabi::ParamType::Bytes], - ), - bytecodes: None, + tracked_signature, + bytecodes, } } -} -impl EvmDeployTracer { fn handle_far_call(&self, state: &mut impl GlobalStateInterface) { let from = state.current_frame().caller(); let to = state.current_frame().code_address(); @@ -70,18 +67,11 @@ impl EvmDeployTracer { let published_bytecode = decoded.into_iter().next().unwrap().into_bytes().unwrap(); let bytecode_hash = BytecodeHash::for_evm_bytecode(&published_bytecode).value_u256(); - self.bytecodes - .as_ref() - .expect("not initialized") - .insert(bytecode_hash, published_bytecode); + self.bytecodes.insert(bytecode_hash, published_bytecode); } Err(err) => tracing::error!("Unable to decode `publishEVMBytecode` call: {err}"), } } - - pub(super) fn insert_dynamic_bytecodes_handle(&mut self, bytecodes: DynamicBytecodes) { - self.bytecodes = Some(bytecodes); - } } impl Tracer for EvmDeployTracer { diff --git a/core/lib/multivm/src/versions/vm_fast/hlist.rs b/core/lib/multivm/src/versions/vm_fast/hlist.rs deleted file mode 100644 index f811aaa48659..000000000000 --- a/core/lib/multivm/src/versions/vm_fast/hlist.rs +++ /dev/null @@ -1,56 +0,0 @@ -//! # Heterogeneous lists -//! A HList is an array that can contain elements of different types. -//! At runtime it works like a struct but at compile time it is more convenient. -//! The tracer interface is automatically implements the `Tracer` trait for tuple constructions like this. - -use std::fmt::{self, DebugList, Formatter}; - -macro_rules! hlist { - [] => (); - [$head:tt] => { - ($head, ()) - }; - [$head:tt, $($tail:tt),*] => { - ($head, hlist![$($tail),*]) - }; -} -pub(crate) use hlist; - -pub(crate) fn debug_hlist(hlist: &T, f: &mut Formatter<'_>) -> fmt::Result { - let mut list = f.debug_list(); - hlist.debug(&mut list); - list.finish() -} - -pub(crate) trait DebugHList { - fn debug(&self, f: &mut DebugList<'_, '_>); -} - -impl DebugHList for (T, W) { - fn debug(&self, list: &mut DebugList<'_, '_>) { - list.entry(&self.0); - self.1.debug(list) - } -} - -impl DebugHList for () { - fn debug(&self, _: &mut DebugList<'_, '_>) {} -} - -pub(crate) struct Here; -pub(crate) struct Later(T); -pub(crate) trait HListGet { - fn get(&mut self) -> &mut T; -} - -impl HListGet for (T, U) { - fn get(&mut self) -> &mut T { - &mut self.0 - } -} - -impl> HListGet> for (U, V) { - fn get(&mut self) -> &mut T { - self.1.get() - } -} diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index 3b87456225f9..6f165d4e30bd 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -2,9 +2,7 @@ pub use zksync_vm2::interface; pub(crate) use self::version::FastVmVersion; pub use self::{ - builtin_tracers::{ - ApiTracers, DefaultTracers, SequencerTracers, ValidationTracers, WithBuiltinTracers, - }, + validation_tracer::{FullValidationTracer, ValidationTracer}, vm::Vm, }; @@ -15,8 +13,6 @@ mod circuits_tracer; mod events; mod evm_deploy_tracer; mod glue; -#[macro_use] -mod hlist; mod hook; mod initial_bootloader_memory; mod refund; diff --git a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs index cc76b20eebfd..fa4b634a22f6 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/account_validation_rules.rs @@ -1,10 +1,7 @@ use super::TestedFastVm; -use crate::{ - versions::testonly::account_validation_rules::test_account_validation_rules, - vm_fast::ValidationTracers, -}; +use crate::versions::testonly::account_validation_rules::test_account_validation_rules; #[test] fn test_account_validation_rules_fast() { - test_account_validation_rules::>>(); + test_account_validation_rules::>(); } diff --git a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs index 9775a04ec52c..765f2c3160cf 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -12,10 +12,11 @@ use zksync_vm_interface::{ VmExecutionResultAndLogs, VmInterface, }; -use super::{validation_tracer::ValidationTracer, ValidationTracers, Vm, WithBuiltinTracers}; +use super::{validation_tracer::ValidationTracer, Vm}; use crate::{ interface::storage::{ImmutableStorageView, InMemoryStorage}, versions::testonly::{validation_params, TestedVm, TestedVmForValidation}, + vm_fast::{builtin_tracers::WithBuiltinTracers, validation_tracer::FullValidationTracer}, }; mod account_validation_rules; @@ -82,11 +83,12 @@ impl PartialEq for VmStateDump { } } -pub(crate) type TestedFastVm = Vm, Tr>; +pub(crate) type TestedFastVm = Vm, Tr, Val>; -impl TestedVm for TestedFastVm> +impl TestedVm for TestedFastVm where - WithBuiltinTracers: std::fmt::Debug + 'static, + Tr: 'static + Tracer + Default + fmt::Debug, + Val: 'static + ValidationTracer + fmt::Debug, { type StateDump = VmStateDump; @@ -135,7 +137,7 @@ where fn manually_decommit(&mut self, code_hash: H256) -> bool { let (_, is_fresh) = self.inner.world_diff_mut().decommit_opcode( &mut self.world, - &mut Default::default(), + &mut WithBuiltinTracers::mock(), h256_to_u256(code_hash), ); is_fresh @@ -178,14 +180,12 @@ where } } -impl TestedVmForValidation for Vm, ValidationTracers<()>> { +impl TestedVmForValidation for Vm, (), FullValidationTracer> { fn run_validation(&mut self, tx: L2Tx, timestamp: u64) -> Option { let validation_params = validation_params(&tx, &self.system_env); self.push_transaction(tx.into()); - - let mut tracer = WithBuiltinTracers::for_validation((), validation_params, timestamp); + let mut tracer = ((), FullValidationTracer::new(validation_params, timestamp)); self.inspect(&mut tracer, InspectExecutionMode::OneTx); - - tracer.validation().validation_error() + tracer.1.validation_error() } } diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs index c0d8201254a1..33e42661e38d 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs @@ -23,10 +23,7 @@ pub trait ValidationTracer: Tracer + Default { fn validation_exited(&mut self); } -#[derive(Debug, Default)] -pub struct ValidationGasLimitOnly; -impl Tracer for ValidationGasLimitOnly {} -impl ValidationTracer for ValidationGasLimitOnly { +impl ValidationTracer for () { const STOP_AFTER_VALIDATION: bool = false; fn account_validation_entered(&mut self) {} fn validation_exited(&mut self) {} diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 8e63576b26b7..1c47bbd1e479 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, fmt, rc::Rc}; +use std::{collections::HashMap, fmt, mem, rc::Rc}; use zk_evm_1_5_0::{ aux_structures::LogQuery, zkevm_opcode_defs::system_params::INITIAL_FRAME_FORMAL_EH_LOCATION, @@ -26,13 +26,13 @@ use zksync_vm2::{ use super::{ bootloader_state::{BootloaderState, BootloaderStateSnapshot}, + builtin_tracers::WithBuiltinTracers, bytecode::compress_bytecodes, evm_deploy_tracer::DynamicBytecodes, hook::Hook, initial_bootloader_memory::bootloader_initial_memory, transaction_data::TransactionData, validation_tracer::ValidationTracer, - DefaultTracers, WithBuiltinTracers, }; use crate::{ glue::GlueInto, @@ -84,24 +84,27 @@ impl VmRunResult { } } +type InnerVm = + VirtualMachine, World>>; + /// Fast VM wrapper. /// /// The wrapper is parametric by the storage and tracer types. Besides the [`Tracer`] trait and implement [`Default`] /// (the latter is necessary to complete batches). [`CircuitsTracer`] is currently always enabled; /// you don't need to specify it explicitly. -pub struct Vm { - pub(crate) world: World, - pub(crate) inner: VirtualMachine>, - pub(crate) bootloader_state: BootloaderState, - pub(crate) batch_env: L1BatchEnv, - pub(crate) system_env: SystemEnv, +pub struct Vm { + pub(super) world: World>, + pub(super) inner: InnerVm, + pub(super) bootloader_state: BootloaderState, + pub(super) batch_env: L1BatchEnv, + pub(super) system_env: SystemEnv, snapshot: Option, vm_version: FastVmVersion, #[cfg(test)] enforced_state_diffs: Option>, } -impl Vm { +impl Vm { pub fn custom(batch_env: L1BatchEnv, system_env: SystemEnv, storage: S) -> Self { let vm_version: FastVmVersion = VmVersion::from(system_env.version) .try_into() @@ -371,14 +374,16 @@ struct AccountValidationGasSplit { gas_hidden: u32, } -impl Vm> +impl Vm where - WithBuiltinTracers: Tracer, + S: ReadStorage, + Tr: Tracer + Default, + Val: ValidationTracer, { fn run( &mut self, execution_mode: VmExecutionMode, - tracer: &mut WithBuiltinTracers, + tracer: &mut WithBuiltinTracers, track_refunds: bool, pubdata_builder: Option<&dyn PubdataBuilder>, ) -> VmRunResult { @@ -434,7 +439,7 @@ where account_validation_gas_split.is_none(), "Account validation can't be nested" ); - tracer.validation().account_validation_entered(); + tracer.validation.account_validation_entered(); let gas = self.gas_remaining(); let gas_given = gas.min(gas_left_for_account_validation); @@ -449,7 +454,7 @@ where } Hook::ValidationExited => { - tracer.validation().validation_exited(); + tracer.validation.validation_exited(); if let Some(AccountValidationGasSplit { gas_given, @@ -463,7 +468,7 @@ where } Hook::ValidationStepEnded => { - if V::STOP_AFTER_VALIDATION { + if Val::STOP_AFTER_VALIDATION { break (ExecutionResult::Success { output: vec![] }, true); } } @@ -631,7 +636,7 @@ where pub(crate) fn inspect_inner( &mut self, - tracer: &mut WithBuiltinTracers, + tracer: &mut (Tr, Val), execution_mode: VmExecutionMode, pubdata_builder: Option<&dyn PubdataBuilder>, ) -> VmExecutionResultAndLogs { @@ -644,10 +649,17 @@ where let start = self.inner.world_diff().snapshot(); let gas_before = self.gas_remaining(); - - tracer.insert_dynamic_bytecodes_handle(self.world.dynamic_bytecodes.clone()); - - let result = self.run(execution_mode, tracer, track_refunds, pubdata_builder); + let (external, validation) = mem::take(tracer); + let mut full_tracer = + WithBuiltinTracers::new(external, validation, self.world.dynamic_bytecodes.clone()); + + let result = self.run( + execution_mode, + &mut full_tracer, + track_refunds, + pubdata_builder, + ); + *tracer = (full_tracer.external, full_tracer.validation); let ignore_world_diff = matches!(execution_mode, VmExecutionMode::OneTx) && result.should_ignore_vm_logs(); @@ -720,7 +732,7 @@ where gas_remaining, computational_gas_used: gas_used, // since 1.5.0, this always has the same value as `gas_used` pubdata_published: result.pubdata_published, - circuit_statistic: tracer.circuit().circuit_statistic(), + circuit_statistic: full_tracer.circuits.circuit_statistic(), contracts_used: 0, cycles_used: 0, total_log_queries: 0, @@ -731,9 +743,11 @@ where } } -impl VmFactory> for Vm, Tr> +impl VmFactory> for Vm, Tr, Val> where S: ReadStorage, + Tr: Tracer + Default, + Val: ValidationTracer, { fn new( batch_env: L1BatchEnv, @@ -745,10 +759,13 @@ where } } -impl VmInterface - for Vm> +impl VmInterface for Vm +where + S: ReadStorage, + Tr: Tracer + Default, + Val: ValidationTracer, { - type TracerDispatcher = WithBuiltinTracers; + type TracerDispatcher = (Tr, Val); fn push_transaction(&mut self, tx: Transaction) -> PushTransactionResult<'_> { self.push_transaction_inner(tx, 0, true); @@ -771,7 +788,7 @@ impl VmInterface fn inspect_transaction_with_bytecode_compression( &mut self, tracer: &mut Self::TracerDispatcher, - tx: zksync_types::Transaction, + tx: Transaction, with_compression: bool, ) -> (BytecodeCompressionResult<'_>, VmExecutionResultAndLogs) { self.push_transaction_inner(tx, 0, with_compression); @@ -825,9 +842,11 @@ struct VmSnapshot { bootloader_snapshot: BootloaderStateSnapshot, } -impl VmInterfaceHistoryEnabled for Vm +impl VmInterfaceHistoryEnabled for Vm where - Self: VmInterface, + S: ReadStorage, + Tr: Tracer + Default, + Val: ValidationTracer, { fn make_snapshot(&mut self) { assert!( @@ -865,7 +884,7 @@ where } } -impl fmt::Debug for Vm { +impl fmt::Debug for Vm { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Vm") .field("bootloader_state", &self.bootloader_state) diff --git a/core/lib/multivm/src/vm_instance.rs b/core/lib/multivm/src/vm_instance.rs index 1ae58a88cf9c..97af38ea0347 100644 --- a/core/lib/multivm/src/vm_instance.rs +++ b/core/lib/multivm/src/vm_instance.rs @@ -13,7 +13,7 @@ use crate::{ VmMemoryMetrics, }, tracers::TracerDispatcher, - vm_fast::{self, DefaultTracers, FastVmVersion}, + vm_fast::{self, interface::Tracer, FastVmVersion}, vm_latest::{self, HistoryEnabled}, }; @@ -226,19 +226,19 @@ impl LegacyVmInstance { } /// Fast VM shadowed by the latest legacy VM. -pub type ShadowedFastVm = ShadowVm< +pub type ShadowedFastVm = ShadowVm< S, vm_latest::Vm, HistoryEnabled>, - vm_fast::Vm, Tr>, + vm_fast::Vm, Tr, Val>, >; /// Fast VM variants. #[derive(Debug)] -pub enum FastVmInstance { +pub enum FastVmInstance { /// Fast VM running in isolation. - Fast(vm_fast::Vm, Tr>), + Fast(vm_fast::Vm, Tr, Val>), /// Fast VM shadowed by the latest legacy VM. - Shadowed(ShadowedFastVm), + Shadowed(ShadowedFastVm), } macro_rules! dispatch_fast_vm { @@ -250,13 +250,15 @@ macro_rules! dispatch_fast_vm { }; } -impl VmInterface for FastVmInstance +impl VmInterface for FastVmInstance where - vm_fast::Vm, Tr>: VmInterface, + S: ReadStorage, + Tr: Tracer + Default, + Val: vm_fast::ValidationTracer, { type TracerDispatcher = ( vm_latest::TracerDispatcher, HistoryEnabled>, - Tr, + (Tr, Val), ); fn push_transaction(&mut self, tx: Transaction) -> PushTransactionResult<'_> { @@ -301,11 +303,11 @@ where } } -impl VmInterfaceHistoryEnabled - for FastVmInstance +impl VmInterfaceHistoryEnabled for FastVmInstance where - vm_fast::Vm, Tr>: - VmInterface + VmInterfaceHistoryEnabled, + S: ReadStorage, + Tr: Tracer + Default, + Val: vm_fast::ValidationTracer, { fn make_snapshot(&mut self) { dispatch_fast_vm!(self.make_snapshot()); @@ -320,7 +322,12 @@ where } } -impl FastVmInstance { +impl FastVmInstance +where + S: ReadStorage, + Tr: Tracer + Default, + Val: vm_fast::ValidationTracer, +{ /// Creates an isolated fast VM. pub fn fast( l1_batch_env: L1BatchEnv, diff --git a/core/lib/vm_executor/src/batch/factory.rs b/core/lib/vm_executor/src/batch/factory.rs index 149c6aa4cf87..9797e1681032 100644 --- a/core/lib/vm_executor/src/batch/factory.rs +++ b/core/lib/vm_executor/src/batch/factory.rs @@ -16,7 +16,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, pubdata_builders::pubdata_params_to_builder, tracers::CallTracer, - vm_fast::{self, SequencerTracers}, + vm_fast, vm_latest::HistoryEnabled, FastVmInstance, LegacyVmInstance, MultiVmTracer, }; @@ -150,7 +150,7 @@ type BytecodeResult = Result, BytecodeCompressionErr #[derive(Debug)] enum BatchVm { Legacy(LegacyVmInstance), - Fast(FastVmInstance>), + Fast(FastVmInstance), } macro_rules! dispatch_batch_vm { diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index e92886a23432..87e4e993f91e 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -27,7 +27,7 @@ use zksync_multivm::{ is_supported_by_fast_vm, tracers::{CallTracer, StorageInvocations, TracerDispatcher, ValidationTracer}, utils::adjust_pubdata_price_for_tx, - vm_fast::{self, ApiTracers, WithBuiltinTracers}, + vm_fast, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, FastVmInstance, HistoryMode, LegacyVmInstance, MultiVmTracer, @@ -196,7 +196,7 @@ where tokio::task::spawn_blocking(move || { let version = sandbox.env.system.version.into(); - sandbox.execute_in_vm_with_tracer(|vm, transaction| match vm { + sandbox.execute_in_vm(|vm, transaction| match vm { Vm::Legacy(vm) => { let validation_tracer = ValidationTracer::::new( validation_params, @@ -227,21 +227,22 @@ where Vm::Fast(FastVmInstance::Fast(vm)) => { vm.push_transaction(transaction); - let mut tracer = WithBuiltinTracers::for_validation( - (), + let validation = vm_fast::FullValidationTracer::new( validation_params, l1_batch_env.timestamp, ); + let mut tracer = ((), validation); let result_and_logs = vm.inspect(&mut tracer, InspectExecutionMode::OneTx); - if let Some(violation) = tracer.validation().validation_error() { + if let Some(violation) = tracer.1.validation_error() { return Err(ValidationError::ViolatedRule(violation)); } + match result_and_logs.result { ExecutionResult::Halt { reason } => Err(ValidationError::FailedTx(reason)), ExecutionResult::Revert { .. } => { unreachable!("Revert can only happen at the end of a transaction") } - ExecutionResult::Success { .. } => Ok(tracer.validation().traces()), + ExecutionResult::Success { .. } => Ok(tracer.1.traces()), } } @@ -280,14 +281,14 @@ where } } ShadowMut::Shadow(vm) => { - let mut tracer = WithBuiltinTracers::for_validation( - (), + let validation = vm_fast::FullValidationTracer::new( validation_params.clone(), l1_batch_env.timestamp, ); + let mut tracer = ((), validation); let result_and_logs = vm.inspect(&mut tracer, InspectExecutionMode::OneTx); - if let Some(violation) = tracer.validation().validation_error() { + if let Some(violation) = tracer.1.validation_error() { return Err(ValidationError::ViolatedRule(violation)); } match result_and_logs.result { @@ -299,7 +300,7 @@ where "Revert can only happen at the end of a transaction" ) } - ExecutionResult::Success { .. } => Ok(tracer.validation().traces()), + ExecutionResult::Success { .. } => Ok(tracer.1.traces()), } } }) @@ -312,12 +313,12 @@ where } #[derive(Debug)] -enum Vm { +enum Vm { Legacy(LegacyVmInstance), - Fast(FastVmInstance), + Fast(FastVmInstance), } -impl Vm> { +impl Vm { fn inspect_transaction_with_bytecode_compression( &mut self, missed_storage_invocation_limit: usize, @@ -343,7 +344,7 @@ impl Vm> { missed_storage_invocation_limit, None, ); - let mut full_tracer = (legacy_tracers.into(), WithBuiltinTracers::for_api(())); + let mut full_tracer = (legacy_tracers.into(), ((), ())); vm.inspect_transaction_with_bytecode_compression( &mut full_tracer, tx, @@ -433,18 +434,14 @@ impl VmSandbox { } } - /// This method is blocking. - fn execute_in_vm( - self, - action: impl FnOnce(&mut Vm>, Transaction) -> T, - ) -> T { - self.execute_in_vm_with_tracer(action) - } - - fn execute_in_vm_with_tracer( + fn execute_in_vm( mut self, - action: impl FnOnce(&mut Vm, Tr>, Transaction) -> T, - ) -> T { + action: impl FnOnce(&mut Vm, Tr, Val>, Transaction) -> T, + ) -> T + where + Tr: vm_fast::interface::Tracer + Default, + Val: vm_fast::ValidationTracer, + { Self::setup_storage( &mut self.storage, &self.execution_args, diff --git a/core/lib/vm_interface/src/vm.rs b/core/lib/vm_interface/src/vm.rs index d84765eb2d63..77db2a600216 100644 --- a/core/lib/vm_interface/src/vm.rs +++ b/core/lib/vm_interface/src/vm.rs @@ -80,6 +80,7 @@ pub trait VmInterfaceExt: VmInterface { impl VmInterfaceExt for T {} /// Encapsulates creating VM instance based on the provided environment. +// FIXME: revert VmInterface as supertrait? pub trait VmFactory { /// Creates a new VM instance. fn new(batch_env: L1BatchEnv, system_env: SystemEnv, storage: StoragePtr) -> Self; diff --git a/core/tests/vm-benchmark/src/vm.rs b/core/tests/vm-benchmark/src/vm.rs index 2c24dd4fdc01..76b3e0b162c3 100644 --- a/core/tests/vm-benchmark/src/vm.rs +++ b/core/tests/vm-benchmark/src/vm.rs @@ -9,7 +9,7 @@ use zksync_multivm::{ VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceExt, VmInterfaceHistoryEnabled, }, - vm_fast::{self, WithBuiltinTracers}, + vm_fast, vm_latest::{self, constants::BATCH_COMPUTATIONAL_GAS_LIMIT, HistoryEnabled, ToTracerPointer}, zk_evm_latest::ethereum_types::{Address, U256}, }; @@ -115,9 +115,9 @@ impl CountInstructions for Fast { let (system_env, l1_batch_env) = test_env(); let mut vm = vm_fast::Vm::custom(l1_batch_env, system_env, &*STORAGE); vm.push_transaction(tx.clone()); - let mut tracer = WithBuiltinTracers::for_sequencer(InstructionCount(0)); + let mut tracer = (InstructionCount(0), ()); vm.inspect(&mut tracer, InspectExecutionMode::OneTx); - tracer.into_inner().0 + tracer.0 .0 } } From d293f8c601034d02fa9470a12823ecd3ff3c475a Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Tue, 3 Dec 2024 18:32:58 +0200 Subject: [PATCH 44/46] Structure fast VM tracers --- core/lib/multivm/src/versions/vm_fast/mod.rs | 7 ++---- .../multivm/src/versions/vm_fast/tests/mod.rs | 4 ++-- .../circuits.rs} | 0 .../evm_deploy.rs} | 6 ++--- .../{builtin_tracers.rs => tracers/mod.rs} | 23 +++++++++++++------ .../validation.rs} | 3 +-- core/lib/multivm/src/versions/vm_fast/vm.rs | 14 +++++------ 7 files changed, 31 insertions(+), 26 deletions(-) rename core/lib/multivm/src/versions/vm_fast/{circuits_tracer.rs => tracers/circuits.rs} (100%) rename core/lib/multivm/src/versions/vm_fast/{evm_deploy_tracer.rs => tracers/evm_deploy.rs} (94%) rename core/lib/multivm/src/versions/vm_fast/{builtin_tracers.rs => tracers/mod.rs} (82%) rename core/lib/multivm/src/versions/vm_fast/{validation_tracer.rs => tracers/validation.rs} (99%) diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index 6f165d4e30bd..dca575138553 100644 --- a/core/lib/multivm/src/versions/vm_fast/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/mod.rs @@ -2,24 +2,21 @@ pub use zksync_vm2::interface; pub(crate) use self::version::FastVmVersion; pub use self::{ - validation_tracer::{FullValidationTracer, ValidationTracer}, + tracers::{FullValidationTracer, ValidationTracer}, vm::Vm, }; mod bootloader_state; -mod builtin_tracers; mod bytecode; -mod circuits_tracer; mod events; -mod evm_deploy_tracer; mod glue; mod hook; mod initial_bootloader_memory; mod refund; #[cfg(test)] mod tests; +mod tracers; mod transaction_data; mod utils; -mod validation_tracer; mod version; mod vm; diff --git a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs index 765f2c3160cf..e148444922ba 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -12,11 +12,11 @@ use zksync_vm_interface::{ VmExecutionResultAndLogs, VmInterface, }; -use super::{validation_tracer::ValidationTracer, Vm}; +use super::{FullValidationTracer, ValidationTracer, Vm}; use crate::{ interface::storage::{ImmutableStorageView, InMemoryStorage}, versions::testonly::{validation_params, TestedVm, TestedVmForValidation}, - vm_fast::{builtin_tracers::WithBuiltinTracers, validation_tracer::FullValidationTracer}, + vm_fast::tracers::WithBuiltinTracers, }; mod account_validation_rules; diff --git a/core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs b/core/lib/multivm/src/versions/vm_fast/tracers/circuits.rs similarity index 100% rename from core/lib/multivm/src/versions/vm_fast/circuits_tracer.rs rename to core/lib/multivm/src/versions/vm_fast/tracers/circuits.rs diff --git a/core/lib/multivm/src/versions/vm_fast/evm_deploy_tracer.rs b/core/lib/multivm/src/versions/vm_fast/tracers/evm_deploy.rs similarity index 94% rename from core/lib/multivm/src/versions/vm_fast/evm_deploy_tracer.rs rename to core/lib/multivm/src/versions/vm_fast/tracers/evm_deploy.rs index c443c99ccf9a..0fbf9dec7215 100644 --- a/core/lib/multivm/src/versions/vm_fast/evm_deploy_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/tracers/evm_deploy.rs @@ -8,14 +8,14 @@ use zksync_vm2::interface::{ CallframeInterface, CallingMode, GlobalStateInterface, Opcode, OpcodeType, ShouldStop, Tracer, }; -use super::utils::read_fat_pointer; +use crate::vm_fast::utils::read_fat_pointer; /// Container for dynamic bytecodes added by [`EvmDeployTracer`]. #[derive(Debug, Clone, Default)] -pub(super) struct DynamicBytecodes(Rc>>>); +pub(crate) struct DynamicBytecodes(Rc>>>); impl DynamicBytecodes { - pub(super) fn map(&self, hash: U256, f: impl FnOnce(&[u8]) -> R) -> Option { + pub(crate) fn map(&self, hash: U256, f: impl FnOnce(&[u8]) -> R) -> Option { self.0.borrow().get(&hash).map(|code| f(code)) } diff --git a/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs b/core/lib/multivm/src/versions/vm_fast/tracers/mod.rs similarity index 82% rename from core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs rename to core/lib/multivm/src/versions/vm_fast/tracers/mod.rs index 4a02b5b122b7..3d9602536743 100644 --- a/core/lib/multivm/src/versions/vm_fast/builtin_tracers.rs +++ b/core/lib/multivm/src/versions/vm_fast/tracers/mod.rs @@ -1,17 +1,22 @@ +//! Tracers for the fast VM. + use zksync_vm2::interface::{CycleStats, GlobalStateInterface, OpcodeType, ShouldStop, Tracer}; -use super::{ - circuits_tracer::CircuitsTracer, - evm_deploy_tracer::{DynamicBytecodes, EvmDeployTracer}, - validation_tracer::ValidationTracer, -}; +pub(super) use self::evm_deploy::DynamicBytecodes; +pub use self::validation::{FullValidationTracer, ValidationTracer}; +use self::{circuits::CircuitsTracer, evm_deploy::EvmDeployTracer}; +use crate::interface::CircuitStatistic; + +mod circuits; +mod evm_deploy; +mod validation; #[derive(Debug)] pub(super) struct WithBuiltinTracers { pub external: Ext, pub validation: Val, - pub circuits: CircuitsTracer, - pub evm_deploy_tracer: EvmDeployTracer, + circuits: CircuitsTracer, + evm_deploy_tracer: EvmDeployTracer, } impl WithBuiltinTracers { @@ -23,6 +28,10 @@ impl WithBuiltinTracers { evm_deploy_tracer: EvmDeployTracer::new(dynamic_bytecodes), } } + + pub(super) fn circuit_statistic(&self) -> CircuitStatistic { + self.circuits.circuit_statistic() + } } #[cfg(test)] diff --git a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs b/core/lib/multivm/src/versions/vm_fast/tracers/validation.rs similarity index 99% rename from core/lib/multivm/src/versions/vm_fast/validation_tracer.rs rename to core/lib/multivm/src/versions/vm_fast/tracers/validation.rs index 33e42661e38d..3a4d1558bd78 100644 --- a/core/lib/multivm/src/versions/vm_fast/validation_tracer.rs +++ b/core/lib/multivm/src/versions/vm_fast/tracers/validation.rs @@ -14,8 +14,7 @@ use zksync_vm_interface::tracer::{ TimestampAsserterParams, ValidationParams, ValidationTraces, ViolatedValidationRule, }; -use super::utils::read_fat_pointer; -use crate::tracers::TIMESTAMP_ASSERTER_FUNCTION_SELECTOR; +use crate::{tracers::TIMESTAMP_ASSERTER_FUNCTION_SELECTOR, vm_fast::utils::read_fat_pointer}; pub trait ValidationTracer: Tracer + Default { const STOP_AFTER_VALIDATION: bool; diff --git a/core/lib/multivm/src/versions/vm_fast/vm.rs b/core/lib/multivm/src/versions/vm_fast/vm.rs index 1c47bbd1e479..e2310c254e96 100644 --- a/core/lib/multivm/src/versions/vm_fast/vm.rs +++ b/core/lib/multivm/src/versions/vm_fast/vm.rs @@ -26,13 +26,11 @@ use zksync_vm2::{ use super::{ bootloader_state::{BootloaderState, BootloaderStateSnapshot}, - builtin_tracers::WithBuiltinTracers, bytecode::compress_bytecodes, - evm_deploy_tracer::DynamicBytecodes, hook::Hook, initial_bootloader_memory::bootloader_initial_memory, + tracers::{DynamicBytecodes, ValidationTracer, WithBuiltinTracers}, transaction_data::TransactionData, - validation_tracer::ValidationTracer, }; use crate::{ glue::GlueInto, @@ -89,9 +87,10 @@ type InnerVm = /// Fast VM wrapper. /// -/// The wrapper is parametric by the storage and tracer types. Besides the [`Tracer`] trait and implement [`Default`] -/// (the latter is necessary to complete batches). [`CircuitsTracer`] is currently always enabled; -/// you don't need to specify it explicitly. +/// The wrapper is parametric by the storage and tracer types. Besides the [`Tracer`] trait, the tracer must implement [`Default`] +/// (the latter is necessary to complete batches). Validation is encapsulated in a separate type param. It should be set to `()` +/// for "standard" validation (not stopping after validation; no validation-specific checks), or [`FullValidationTracer`](super::FullValidationTracer) +/// for full validation (stopping after validation; validation-specific checks). pub struct Vm { pub(super) world: World>, pub(super) inner: InnerVm, @@ -659,6 +658,7 @@ where track_refunds, pubdata_builder, ); + let circuit_statistic = full_tracer.circuit_statistic(); *tracer = (full_tracer.external, full_tracer.validation); let ignore_world_diff = @@ -732,7 +732,7 @@ where gas_remaining, computational_gas_used: gas_used, // since 1.5.0, this always has the same value as `gas_used` pubdata_published: result.pubdata_published, - circuit_statistic: full_tracer.circuits.circuit_statistic(), + circuit_statistic, contracts_used: 0, cycles_used: 0, total_log_queries: 0, From 9f18a9c9feea170a0e28427e08a985f9d2ced7bb Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Wed, 4 Dec 2024 09:50:29 +0200 Subject: [PATCH 45/46] Revert `VmInterface` as supertrait for `VmFactory` --- core/lib/multivm/src/versions/shadow/mod.rs | 2 +- .../src/versions/vm_fast/tracers/validation.rs | 13 +++++++++++-- core/lib/vm_interface/src/utils/dump.rs | 2 +- core/lib/vm_interface/src/vm.rs | 3 +-- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/core/lib/multivm/src/versions/shadow/mod.rs b/core/lib/multivm/src/versions/shadow/mod.rs index eb55fc107bec..1ad5bdba5a7b 100644 --- a/core/lib/multivm/src/versions/shadow/mod.rs +++ b/core/lib/multivm/src/versions/shadow/mod.rs @@ -195,7 +195,7 @@ impl Harness { fn sanity_check_vm() -> (Vm, Harness) where - Vm: VmFactory> + VmInterface, + Vm: VmFactory>, { let system_env = default_system_env(); let l1_batch_env = default_l1_batch(L1BatchNumber(1)); diff --git a/core/lib/multivm/src/versions/vm_fast/tracers/validation.rs b/core/lib/multivm/src/versions/vm_fast/tracers/validation.rs index 3a4d1558bd78..52b0a4747b7d 100644 --- a/core/lib/multivm/src/versions/vm_fast/tracers/validation.rs +++ b/core/lib/multivm/src/versions/vm_fast/tracers/validation.rs @@ -16,9 +16,16 @@ use zksync_vm_interface::tracer::{ use crate::{tracers::TIMESTAMP_ASSERTER_FUNCTION_SELECTOR, vm_fast::utils::read_fat_pointer}; +/// [`Tracer`] used for account validation per [EIP-4337] and [EIP-7562]. +/// +/// [EIP-4337]: https://eips.ethereum.org/EIPS/eip-4337 +/// [EIP-7562]: https://eips.ethereum.org/EIPS/eip-7562 pub trait ValidationTracer: Tracer + Default { + /// Should the execution stop after validation is complete? const STOP_AFTER_VALIDATION: bool; + /// Hook called when account validation is entered. fn account_validation_entered(&mut self); + /// Hook called when account validation is exited. fn validation_exited(&mut self); } @@ -29,7 +36,7 @@ impl ValidationTracer for () { } /// Account abstraction exposes a chain to denial of service attacks because someone who fails to -/// authenticate does not pay for the failed transaction. Otherwise people could empty other's +/// authenticate does not pay for the failed transaction. Otherwise, people could empty other's /// wallets for free! /// /// If some address repeatedly posts transactions that validate during preliminary checks but fail @@ -46,12 +53,14 @@ impl ValidationTracer for () { /// that, we make sure that the storage slots accessed by different accounts are disjoint by only /// allowing access to storage in the account itself and slots derived from the account's address. /// -/// Our rules are an extension of the rules are outlined in EIP-7562. +/// Our rules are an extension of the rules are outlined in [EIP-7562]. /// /// This tracer enforces the rules by checking what the code does at runtime, even though the /// properties checked are supposed to always hold for a well-written custom account. Proving /// that a contract adheres to the rules ahead of time would be challenging or even impossible, /// considering that contracts that the code depends on may get upgraded. +/// +/// [EIP-7562]: https://eips.ethereum.org/EIPS/eip-7562 #[derive(Debug, Default)] pub struct FullValidationTracer { in_validation: bool, diff --git a/core/lib/vm_interface/src/utils/dump.rs b/core/lib/vm_interface/src/utils/dump.rs index 5777a2fb8171..f23d6f307b89 100644 --- a/core/lib/vm_interface/src/utils/dump.rs +++ b/core/lib/vm_interface/src/utils/dump.rs @@ -66,7 +66,7 @@ impl VmDump { /// Plays back this dump on the specified VM. pub fn play_back(self) -> Vm where - Vm: VmFactory> + VmInterface, + Vm: VmFactory>, { self.play_back_custom(Vm::new) } diff --git a/core/lib/vm_interface/src/vm.rs b/core/lib/vm_interface/src/vm.rs index 77db2a600216..f347bb54f550 100644 --- a/core/lib/vm_interface/src/vm.rs +++ b/core/lib/vm_interface/src/vm.rs @@ -80,8 +80,7 @@ pub trait VmInterfaceExt: VmInterface { impl VmInterfaceExt for T {} /// Encapsulates creating VM instance based on the provided environment. -// FIXME: revert VmInterface as supertrait? -pub trait VmFactory { +pub trait VmFactory: VmInterface { /// Creates a new VM instance. fn new(batch_env: L1BatchEnv, system_env: SystemEnv, storage: StoragePtr) -> Self; } From 64c4b2b7c895fb38bd80b83f9e6a5b5b5bb242bd Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Wed, 4 Dec 2024 10:41:09 +0200 Subject: [PATCH 46/46] Refactor validation in oneshot executor --- core/lib/vm_executor/src/oneshot/mod.rs | 158 ++++++++++-------------- 1 file changed, 63 insertions(+), 95 deletions(-) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 87e4e993f91e..e52a88e3e9c5 100644 --- a/core/lib/vm_executor/src/oneshot/mod.rs +++ b/core/lib/vm_executor/src/oneshot/mod.rs @@ -17,7 +17,7 @@ use once_cell::sync::OnceCell; use zksync_multivm::{ interface::{ executor::{OneshotExecutor, TransactionValidator}, - storage::{ReadStorage, StorageView, StorageWithOverrides}, + storage::{ReadStorage, StorageView, StorageWithOverrides, WriteStorage}, tracer::{ValidationError, ValidationParams, ValidationTraces}, utils::{DivergenceHandler, ShadowMut, ShadowVm}, Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, @@ -30,7 +30,7 @@ use zksync_multivm::{ vm_fast, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, - FastVmInstance, HistoryMode, LegacyVmInstance, MultiVmTracer, + FastVmInstance, HistoryMode, LegacyVmInstance, MultiVmTracer, VmVersion, }; use zksync_types::{ block::pack_block_info, @@ -195,113 +195,30 @@ where tokio::task::spawn_blocking(move || { let version = sandbox.env.system.version.into(); + let batch_timestamp = l1_batch_env.timestamp; sandbox.execute_in_vm(|vm, transaction| match vm { Vm::Legacy(vm) => { - let validation_tracer = ValidationTracer::::new( - validation_params, - version, - l1_batch_env.timestamp, - ); - let mut validation_result = validation_tracer.get_result(); - let validation_traces = validation_tracer.get_traces(); - let tracers = validation_tracer.into_tracer_pointer(); - vm.push_transaction(transaction); - let exec_result = vm.inspect(&mut tracers.into(), InspectExecutionMode::OneTx); - - let validation_result = Arc::make_mut(&mut validation_result) - .take() - .map_or(Ok(()), Err); - - match (exec_result.result, validation_result) { - (_, Err(violated_rule)) => { - Err(ValidationError::ViolatedRule(violated_rule)) - } - (ExecutionResult::Halt { reason }, _) => { - Err(ValidationError::FailedTx(reason)) - } - _ => Ok(validation_traces.lock().unwrap().clone()), - } + validate_legacy(vm, version, validation_params, batch_timestamp) } Vm::Fast(FastVmInstance::Fast(vm)) => { vm.push_transaction(transaction); - let validation = vm_fast::FullValidationTracer::new( - validation_params, - l1_batch_env.timestamp, - ); - let mut tracer = ((), validation); - let result_and_logs = vm.inspect(&mut tracer, InspectExecutionMode::OneTx); - if let Some(violation) = tracer.1.validation_error() { - return Err(ValidationError::ViolatedRule(violation)); - } - - match result_and_logs.result { - ExecutionResult::Halt { reason } => Err(ValidationError::FailedTx(reason)), - ExecutionResult::Revert { .. } => { - unreachable!("Revert can only happen at the end of a transaction") - } - ExecutionResult::Success { .. } => Ok(tracer.1.traces()), - } + validate_fast(vm, validation_params, batch_timestamp) } Vm::Fast(FastVmInstance::Shadowed(vm)) => { vm.push_transaction(transaction); - vm.get_custom_mut("validation result", |vm| match vm { - ShadowMut::Main(vm) => { - let validation_tracer = ValidationTracer::::new( - validation_params.clone(), - version, - l1_batch_env.timestamp, - ); - let mut validation_result = validation_tracer.get_result(); - let validation_traces = validation_tracer.get_traces(); - let tracers: Box> = - validation_tracer.into_tracer_pointer(); - - let exec_result = vm.inspect( - &mut TracerDispatcher::from(tracers).into(), - InspectExecutionMode::OneTx, - ); - - let validation_result = Arc::make_mut(&mut validation_result) - .take() - .map_or(Ok(()), Err); - - match (exec_result.result, validation_result) { - (_, Err(violated_rule)) => { - Err(ValidationError::ViolatedRule(violated_rule)) - } - (ExecutionResult::Halt { reason }, _) => { - Err(ValidationError::FailedTx(reason)) - } - _ => Ok(validation_traces.lock().unwrap().clone()), - } - } + ShadowMut::Main(vm) => validate_legacy::<_, HistoryEnabled>( + vm, + version, + validation_params.clone(), + batch_timestamp, + ), ShadowMut::Shadow(vm) => { - let validation = vm_fast::FullValidationTracer::new( - validation_params.clone(), - l1_batch_env.timestamp, - ); - let mut tracer = ((), validation); - let result_and_logs = - vm.inspect(&mut tracer, InspectExecutionMode::OneTx); - if let Some(violation) = tracer.1.validation_error() { - return Err(ValidationError::ViolatedRule(violation)); - } - match result_and_logs.result { - ExecutionResult::Halt { reason } => { - Err(ValidationError::FailedTx(reason)) - } - ExecutionResult::Revert { .. } => { - unreachable!( - "Revert can only happen at the end of a transaction" - ) - } - ExecutionResult::Success { .. } => Ok(tracer.1.traces()), - } + validate_fast(vm, validation_params.clone(), batch_timestamp) } }) } @@ -374,6 +291,57 @@ impl Vm { } } +fn validate_fast( + vm: &mut vm_fast::Vm, + validation_params: ValidationParams, + batch_timestamp: u64, +) -> Result { + let validation = vm_fast::FullValidationTracer::new(validation_params, batch_timestamp); + let mut tracer = ((), validation); + let result_and_logs = vm.inspect(&mut tracer, InspectExecutionMode::OneTx); + if let Some(violation) = tracer.1.validation_error() { + return Err(ValidationError::ViolatedRule(violation)); + } + + match result_and_logs.result { + ExecutionResult::Halt { reason } => Err(ValidationError::FailedTx(reason)), + ExecutionResult::Revert { .. } => { + unreachable!("Revert can only happen at the end of a transaction") + } + ExecutionResult::Success { .. } => Ok(tracer.1.traces()), + } +} + +fn validate_legacy( + vm: &mut impl VmInterface>>, + version: VmVersion, + validation_params: ValidationParams, + batch_timestamp: u64, +) -> Result +where + S: WriteStorage, + H: 'static + HistoryMode, + ValidationTracer: MultiVmTracer, +{ + let validation_tracer = ValidationTracer::::new(validation_params, version, batch_timestamp); + let mut validation_result = validation_tracer.get_result(); + let validation_traces = validation_tracer.get_traces(); + let validation_tracer: Box> = validation_tracer.into_tracer_pointer(); + let tracers = TracerDispatcher::from(validation_tracer); + + let exec_result = vm.inspect(&mut tracers.into(), InspectExecutionMode::OneTx); + + let validation_result = Arc::make_mut(&mut validation_result) + .take() + .map_or(Ok(()), Err); + + match (exec_result.result, validation_result) { + (_, Err(violated_rule)) => Err(ValidationError::ViolatedRule(violated_rule)), + (ExecutionResult::Halt { reason }, _) => Err(ValidationError::FailedTx(reason)), + _ => Ok(validation_traces.lock().unwrap().clone()), + } +} + /// Full parameters necessary to instantiate a VM for oneshot execution. #[derive(Debug)] struct VmSandbox {