From 1bd3de00596e96a144916af45250e867680e71d7 Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Wed, 29 Jan 2025 12:57:42 +0200 Subject: [PATCH 1/5] Sketch storage limiting tracer --- core/lib/multivm/src/versions/vm_fast/mod.rs | 2 +- .../src/versions/vm_fast/tracers/mod.rs | 2 + .../src/versions/vm_fast/tracers/storage.rs | 44 +++++++++++++++++++ core/lib/vm_executor/src/oneshot/mod.rs | 28 ++++++------ 4 files changed, 62 insertions(+), 14 deletions(-) create mode 100644 core/lib/multivm/src/versions/vm_fast/tracers/storage.rs diff --git a/core/lib/multivm/src/versions/vm_fast/mod.rs b/core/lib/multivm/src/versions/vm_fast/mod.rs index 291961d3312a..38a0794c2ad8 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(crate) use self::version::FastVmVersion; pub use self::{ - tracers::{CallTracer, FullValidationTracer, ValidationTracer}, + tracers::{CallTracer, FullValidationTracer, StorageInvocationsTracer, ValidationTracer}, vm::Vm, }; diff --git a/core/lib/multivm/src/versions/vm_fast/tracers/mod.rs b/core/lib/multivm/src/versions/vm_fast/tracers/mod.rs index db527bdbaceb..26d5d690e244 100644 --- a/core/lib/multivm/src/versions/vm_fast/tracers/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tracers/mod.rs @@ -5,6 +5,7 @@ use zksync_vm2::interface::{CycleStats, GlobalStateInterface, OpcodeType, Should pub(super) use self::evm_deploy::DynamicBytecodes; pub use self::{ calls::CallTracer, + storage::StorageInvocationsTracer, validation::{FullValidationTracer, ValidationTracer}, }; use self::{circuits::CircuitsTracer, evm_deploy::EvmDeployTracer}; @@ -13,6 +14,7 @@ use crate::interface::CircuitStatistic; mod calls; mod circuits; mod evm_deploy; +mod storage; mod validation; #[derive(Debug)] diff --git a/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs b/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs new file mode 100644 index 000000000000..2f2baa71a242 --- /dev/null +++ b/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs @@ -0,0 +1,44 @@ +use zksync_vm2::interface::{GlobalStateInterface, Opcode, OpcodeType, ShouldStop, Tracer}; + +use crate::interface::storage::{StoragePtr, WriteStorage}; + +#[derive(Debug)] +pub struct StorageInvocationsTracer { + // FIXME: this is very awkward; logically, storage should be obtainable from `GlobalStateInterface` (but how?) + storage: Option>, + limit: usize, +} + +impl Default for StorageInvocationsTracer { + fn default() -> Self { + Self { + storage: None, + limit: usize::MAX, + } + } +} + +impl StorageInvocationsTracer { + pub fn new(storage: StoragePtr, limit: usize) -> Self { + Self { + storage: Some(storage), + limit, + } + } +} + +impl Tracer for StorageInvocationsTracer { + #[inline(always)] + fn after_instruction( + &mut self, + _state: &mut S, + ) -> ShouldStop { + if matches!(OP::VALUE, Opcode::StorageRead | Opcode::StorageWrite) { + let storage = self.storage.as_ref().unwrap(); + if storage.borrow().missed_storage_invocations() > self.limit { + return ShouldStop::Stop; + } + } + ShouldStop::Continue + } +} diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index e52a88e3e9c5..38c500ab6b6b 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, WriteStorage}, + storage::{ReadStorage, StoragePtr, StorageView, StorageWithOverrides, WriteStorage}, tracer::{ValidationError, ValidationParams, ValidationTraces}, utils::{DivergenceHandler, ShadowMut, ShadowVm}, Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, @@ -28,6 +28,7 @@ use zksync_multivm::{ tracers::{CallTracer, StorageInvocations, TracerDispatcher, ValidationTracer}, utils::adjust_pubdata_price_for_tx, vm_fast, + vm_fast::StorageInvocationsTracer, vm_latest::{HistoryDisabled, HistoryEnabled}, zk_evm_latest::ethereum_types::U256, FastVmInstance, HistoryMode, LegacyVmInstance, MultiVmTracer, VmVersion, @@ -203,12 +204,12 @@ where validate_legacy(vm, version, validation_params, batch_timestamp) } - Vm::Fast(FastVmInstance::Fast(vm)) => { + Vm::Fast(_, FastVmInstance::Fast(vm)) => { vm.push_transaction(transaction); validate_fast(vm, validation_params, batch_timestamp) } - Vm::Fast(FastVmInstance::Shadowed(vm)) => { + Vm::Fast(_, FastVmInstance::Shadowed(vm)) => { vm.push_transaction(transaction); vm.get_custom_mut("validation result", |vm| match vm { ShadowMut::Main(vm) => validate_legacy::<_, HistoryEnabled>( @@ -232,10 +233,10 @@ where #[derive(Debug)] enum Vm { Legacy(LegacyVmInstance), - Fast(FastVmInstance), + Fast(StoragePtr>, FastVmInstance), } -impl Vm { +impl Vm>, ()> { fn inspect_transaction_with_bytecode_compression( &mut self, missed_storage_invocation_limit: usize, @@ -252,7 +253,7 @@ impl Vm { ); vm.inspect_transaction_with_bytecode_compression(&mut tracers, tx, with_compression) } - Self::Fast(vm) => { + Self::Fast(storage, vm) => { assert!( !params.trace_calls, "Call tracing is not supported by fast VM yet" @@ -261,7 +262,9 @@ impl Vm { missed_storage_invocation_limit, None, ); - let mut full_tracer = (legacy_tracers.into(), ((), ())); + let tracer = + StorageInvocationsTracer::new(storage.clone(), missed_storage_invocation_limit); + let mut full_tracer = (legacy_tracers.into(), (tracer, ())); vm.inspect_transaction_with_bytecode_compression( &mut full_tracer, tx, @@ -442,11 +445,10 @@ impl VmSandbox { storage_view.clone(), protocol_version.into_api_vm_version(), )), - FastVmMode::New => Vm::Fast(FastVmInstance::fast( - self.env.l1_batch, - self.env.system, + FastVmMode::New => Vm::Fast( storage_view.clone(), - )), + FastVmInstance::fast(self.env.l1_batch, self.env.system, storage_view.clone()), + ), FastVmMode::Shadow => { let mut vm = ShadowVm::new(self.env.l1_batch, self.env.system, storage_view.clone()); @@ -457,7 +459,7 @@ impl VmSandbox { }); vm.set_divergence_handler(handler); } - Vm::Fast(FastVmInstance::Shadowed(vm)) + Vm::Fast(storage_view.clone(), FastVmInstance::Shadowed(vm)) } }; @@ -479,7 +481,7 @@ impl VmSandbox { &storage_view.borrow().stats(), ); } - Vm::Fast(_) => { + Vm::Fast(..) => { // The new VM implementation doesn't have the same memory model as old ones, so it doesn't report memory metrics, // only storage-related ones. metrics::report_vm_storage_metrics( From 374082e55a3d8d556b928d1e5dade21817b8ce4d Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Wed, 29 Jan 2025 12:57:52 +0200 Subject: [PATCH 2/5] Test storage limiting tracer --- core/lib/multivm/src/versions/testonly/mod.rs | 4 +- .../multivm/src/versions/testonly/storage.rs | 104 +++++++++++++++++- .../src/versions/testonly/tester/mod.rs | 4 + .../multivm/src/versions/vm_fast/tests/mod.rs | 28 +++-- .../src/versions/vm_fast/tests/storage.rs | 21 +++- .../src/versions/vm_latest/tests/mod.rs | 11 +- .../src/versions/vm_latest/tests/storage.rs | 21 +++- core/lib/vm_interface/src/storage/view.rs | 5 + 8 files changed, 179 insertions(+), 19 deletions(-) diff --git a/core/lib/multivm/src/versions/testonly/mod.rs b/core/lib/multivm/src/versions/testonly/mod.rs index c1a603bfeefc..34e5873d7894 100644 --- a/core/lib/multivm/src/versions/testonly/mod.rs +++ b/core/lib/multivm/src/versions/testonly/mod.rs @@ -22,8 +22,8 @@ use zksync_types::{ }; pub(super) use self::tester::{ - validation_params, TestedVm, TestedVmForValidation, TestedVmWithCallTracer, VmTester, - VmTesterBuilder, + validation_params, TestedVm, TestedVmForValidation, TestedVmWithCallTracer, + TestedVmWithStorageLimit, VmTester, VmTesterBuilder, }; use crate::{ interface::{ diff --git a/core/lib/multivm/src/versions/testonly/storage.rs b/core/lib/multivm/src/versions/testonly/storage.rs index d57acc37944a..6e07ca2b9433 100644 --- a/core/lib/multivm/src/versions/testonly/storage.rs +++ b/core/lib/multivm/src/versions/testonly/storage.rs @@ -1,9 +1,15 @@ +use assert_matches::assert_matches; use ethabi::Token; -use zksync_test_contracts::TestContract; -use zksync_types::{Address, Execute, U256}; +use zksync_test_contracts::{LoadnextContractExecutionParams, TestContract, TxType}; +use zksync_types::{AccountTreeId, Address, Execute, StorageKey, H256, U256}; -use super::{tester::VmTesterBuilder, ContractToDeploy, TestedVm}; -use crate::interface::{InspectExecutionMode, TxExecutionMode, VmInterfaceExt}; +use super::{ + get_empty_storage, tester::VmTesterBuilder, ContractToDeploy, TestedVm, + TestedVmWithStorageLimit, VmTester, +}; +use crate::interface::{ + ExecutionResult, Halt, InspectExecutionMode, TxExecutionMode, VmInterfaceExt, +}; fn test_storage(first_tx_calldata: Vec, second_tx_calldata: Vec) -> u32 { let bytecode = TestContract::storage_test().bytecode.to_vec(); @@ -107,3 +113,93 @@ pub(crate) fn test_transient_storage_behavior() { test_storage::(first_tstore_test, second_tstore_test); } + +pub(crate) fn test_limiting_storage_writes(should_stop: bool) { + let bytecode = TestContract::expensive().bytecode.to_vec(); + let test_address = Address::repeat_byte(1); + + let mut vm: VmTester = VmTesterBuilder::new() + .with_empty_in_memory_storage() + .with_execution_mode(TxExecutionMode::VerifyExecute) + .with_rich_accounts(1) + .with_custom_contracts(vec![ContractToDeploy::new(bytecode, test_address)]) + .build(); + + let account = &mut vm.rich_accounts[0]; + let test_fn = TestContract::expensive().function("expensive"); + let (executed_writes, limit) = if should_stop { + (1_000, 100) + } else { + (100, 1_000) + }; + let tx = account.get_l2_tx_for_execute( + Execute { + contract_address: Some(test_address), + calldata: test_fn + .encode_input(&[Token::Uint(executed_writes.into())]) + .unwrap(), + value: 0.into(), + factory_deps: vec![], + }, + None, + ); + vm.vm.push_transaction(tx); + + let result = vm.vm.execute_with_storage_limit(limit); + if should_stop { + assert_matches!( + &result.result, + ExecutionResult::Halt { + reason: Halt::TracerCustom(_) + } + ); + } else { + assert!(!result.result.is_failed(), "{:?}", result.result); + } +} + +pub(crate) fn test_limiting_storage_reads(should_stop: bool) { + let bytecode = TestContract::load_test().bytecode.to_vec(); + let test_address = Address::repeat_byte(1); + let (executed_reads, limit) = if should_stop { + (1_000, 100) + } else { + (100, 1_000) + }; + let mut storage = get_empty_storage(); + // Set the read array length in the load test contract. + storage.set_value( + StorageKey::new(AccountTreeId::new(test_address), H256::zero()), + H256::from_low_u64_be(executed_reads), + ); + + let mut vm: VmTester = VmTesterBuilder::new() + .with_storage(storage) + .with_execution_mode(TxExecutionMode::VerifyExecute) + .with_rich_accounts(1) + .with_custom_contracts(vec![ContractToDeploy::new(bytecode, test_address)]) + .build(); + + let account = &mut vm.rich_accounts[0]; + let tx = account.get_loadnext_transaction( + test_address, + LoadnextContractExecutionParams { + reads: executed_reads as usize, + ..LoadnextContractExecutionParams::empty() + }, + TxType::L2, + ); + vm.vm.push_transaction(tx); + + let result = vm.vm.execute_with_storage_limit(limit); + if should_stop { + assert_matches!( + &result.result, + ExecutionResult::Halt { + reason: Halt::TracerCustom(_) + } + ); + } else { + assert!(!result.result.is_failed(), "{:?}", result.result); + } +} diff --git a/core/lib/multivm/src/versions/testonly/tester/mod.rs b/core/lib/multivm/src/versions/testonly/tester/mod.rs index a25909525f11..229f98a5b8be 100644 --- a/core/lib/multivm/src/versions/testonly/tester/mod.rs +++ b/core/lib/multivm/src/versions/testonly/tester/mod.rs @@ -257,3 +257,7 @@ pub(crate) fn validation_params(tx: &L2Tx, system: &SystemEnv) -> ValidationPara pub(crate) trait TestedVmWithCallTracer: TestedVm { fn inspect_with_call_tracer(&mut self) -> (VmExecutionResultAndLogs, Vec); } + +pub(crate) trait TestedVmWithStorageLimit: TestedVm { + fn execute_with_storage_limit(&mut self, limit: usize) -> VmExecutionResultAndLogs; +} 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 89edd85b86f1..308611d2663d 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/mod.rs @@ -4,21 +4,21 @@ use zksync_types::{ h256_to_u256, l2::L2Tx, writes::StateDiffRecord, StorageKey, Transaction, H160, H256, U256, }; use zksync_vm2::interface::{Event, HeapId, StateInterface, Tracer}; -use zksync_vm_interface::{ - pubdata::{PubdataBuilder, PubdataInput}, - storage::ReadStorage, - tracer::ViolatedValidationRule, - Call, CurrentExecutionState, InspectExecutionMode, L2BlockEnv, VmExecutionMode, - VmExecutionResultAndLogs, VmInterface, -}; use super::{FullValidationTracer, ValidationTracer, Vm}; use crate::{ - interface::storage::{ImmutableStorageView, InMemoryStorage}, + interface::{ + pubdata::{PubdataBuilder, PubdataInput}, + storage::{ImmutableStorageView, InMemoryStorage, ReadStorage, StorageView}, + tracer::ViolatedValidationRule, + Call, CurrentExecutionState, InspectExecutionMode, L2BlockEnv, VmExecutionMode, + VmExecutionResultAndLogs, VmInterface, + }, versions::testonly::{ validation_params, TestedVm, TestedVmForValidation, TestedVmWithCallTracer, + TestedVmWithStorageLimit, }, - vm_fast::{tracers::WithBuiltinTracers, CallTracer}, + vm_fast::{tracers::WithBuiltinTracers, CallTracer, StorageInvocationsTracer}, }; mod account_validation_rules; @@ -200,3 +200,13 @@ impl TestedVmWithCallTracer for TestedFastVm { (result, tracer.0.into_result()) } } + +type TestStorageLimiter = StorageInvocationsTracer>; + +impl TestedVmWithStorageLimit for TestedFastVm { + fn execute_with_storage_limit(&mut self, limit: usize) -> VmExecutionResultAndLogs { + let storage = self.world.storage.to_rc_ptr(); + let mut tracer = (StorageInvocationsTracer::new(storage, limit), ()); + self.inspect(&mut tracer, InspectExecutionMode::OneTx) + } +} diff --git a/core/lib/multivm/src/versions/vm_fast/tests/storage.rs b/core/lib/multivm/src/versions/vm_fast/tests/storage.rs index 54a38814d3b5..c725e323bcf9 100644 --- a/core/lib/multivm/src/versions/vm_fast/tests/storage.rs +++ b/core/lib/multivm/src/versions/vm_fast/tests/storage.rs @@ -1,5 +1,8 @@ use crate::{ - versions::testonly::storage::{test_storage_behavior, test_transient_storage_behavior}, + versions::testonly::storage::{ + test_limiting_storage_reads, test_limiting_storage_writes, test_storage_behavior, + test_transient_storage_behavior, + }, vm_fast::Vm, }; @@ -12,3 +15,19 @@ fn storage_behavior() { fn transient_storage_behavior() { test_transient_storage_behavior::>(); } + +#[test] +fn limiting_storage_writes() { + println!("Sanity check"); + test_limiting_storage_writes::>(false); + println!("Storage limiter check"); + test_limiting_storage_writes::>(true); +} + +#[test] +fn limiting_storage_reads() { + println!("Sanity check"); + test_limiting_storage_reads::>(false); + println!("Storage limiter check"); + test_limiting_storage_reads::>(true); +} 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 260b1a4eeef3..0459fa0bac95 100644 --- a/core/lib/multivm/src/versions/vm_latest/tests/mod.rs +++ b/core/lib/multivm/src/versions/vm_latest/tests/mod.rs @@ -24,11 +24,11 @@ use crate::{ tracer::ViolatedValidationRule, CurrentExecutionState, L2BlockEnv, VmExecutionMode, VmExecutionResultAndLogs, }, - tracers::{CallTracer, ValidationTracer}, + tracers::{CallTracer, StorageInvocations, ValidationTracer}, utils::bytecode::bytes_to_be_words, versions::testonly::{ filter_out_base_system_contracts, validation_params, TestedVm, TestedVmForValidation, - TestedVmWithCallTracer, + TestedVmWithCallTracer, TestedVmWithStorageLimit, }, vm_latest::{ constants::BOOTLOADER_HEAP_PAGE, @@ -350,3 +350,10 @@ impl TestedVmWithCallTracer for TestedLatestVm { (res, traces) } } + +impl TestedVmWithStorageLimit for TestedLatestVm { + fn execute_with_storage_limit(&mut self, limit: usize) -> VmExecutionResultAndLogs { + let tracer = StorageInvocations::new(limit).into_tracer_pointer(); + self.inspect(&mut tracer.into(), InspectExecutionMode::OneTx) + } +} diff --git a/core/lib/multivm/src/versions/vm_latest/tests/storage.rs b/core/lib/multivm/src/versions/vm_latest/tests/storage.rs index 4cb03875a0f0..e80d655d1a11 100644 --- a/core/lib/multivm/src/versions/vm_latest/tests/storage.rs +++ b/core/lib/multivm/src/versions/vm_latest/tests/storage.rs @@ -1,5 +1,8 @@ use crate::{ - versions::testonly::storage::{test_storage_behavior, test_transient_storage_behavior}, + versions::testonly::storage::{ + test_limiting_storage_reads, test_limiting_storage_writes, test_storage_behavior, + test_transient_storage_behavior, + }, vm_latest::{HistoryEnabled, Vm}, }; @@ -12,3 +15,19 @@ fn storage_behavior() { fn transient_storage_behavior() { test_transient_storage_behavior::>(); } + +#[test] +fn limiting_storage_writes() { + println!("Sanity check"); + test_limiting_storage_writes::>(false); + println!("Storage limiter check"); + test_limiting_storage_writes::>(true); +} + +#[test] +fn limiting_storage_reads() { + println!("Sanity check"); + test_limiting_storage_reads::>(false); + println!("Storage limiter check"); + test_limiting_storage_reads::>(true); +} diff --git a/core/lib/vm_interface/src/storage/view.rs b/core/lib/vm_interface/src/storage/view.rs index 249d584c9f6c..c2ab678df325 100644 --- a/core/lib/vm_interface/src/storage/view.rs +++ b/core/lib/vm_interface/src/storage/view.rs @@ -271,6 +271,11 @@ impl ImmutableStorageView { pub fn new(ptr: StoragePtr>) -> Self { Self(ptr) } + + #[doc(hidden)] // can easily break invariants if not used carefully + pub fn to_rc_ptr(&self) -> StoragePtr> { + self.0.clone() + } } // All methods other than `read_value()` do not read back modified storage slots, so we proxy them as-is. From 488deb11c7c6f325cd3c0228e0b87f0fa6429fe0 Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Wed, 29 Jan 2025 14:14:13 +0200 Subject: [PATCH 3/5] Benchmark fast VM with storage limiter --- .../src/versions/vm_fast/tracers/storage.rs | 4 +- core/tests/vm-benchmark/benches/batch.rs | 4 +- core/tests/vm-benchmark/src/lib.rs | 5 +- core/tests/vm-benchmark/src/vm.rs | 94 ++++++++++++++----- 4 files changed, 83 insertions(+), 24 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs b/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs index 2f2baa71a242..8b743502b8e9 100644 --- a/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs +++ b/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs @@ -2,6 +2,8 @@ use zksync_vm2::interface::{GlobalStateInterface, Opcode, OpcodeType, ShouldStop use crate::interface::storage::{StoragePtr, WriteStorage}; +/// Tracer that stops VM execution once too many storage slots were read from the underlying storage. +/// Useful as an anti-DoS measure. #[derive(Debug)] pub struct StorageInvocationsTracer { // FIXME: this is very awkward; logically, storage should be obtainable from `GlobalStateInterface` (but how?) @@ -35,7 +37,7 @@ impl Tracer for StorageInvocationsTracer { ) -> ShouldStop { if matches!(OP::VALUE, Opcode::StorageRead | Opcode::StorageWrite) { let storage = self.storage.as_ref().unwrap(); - if storage.borrow().missed_storage_invocations() > self.limit { + if storage.borrow().missed_storage_invocations() >= self.limit { return ShouldStop::Stop; } } diff --git a/core/tests/vm-benchmark/benches/batch.rs b/core/tests/vm-benchmark/benches/batch.rs index f4151c39a6f8..531a25fda343 100644 --- a/core/tests/vm-benchmark/benches/batch.rs +++ b/core/tests/vm-benchmark/benches/batch.rs @@ -20,7 +20,8 @@ use vm_benchmark::{ criterion::{is_test_mode, BenchmarkGroup, BenchmarkId, CriterionExt, MeteredTime}, get_deploy_tx_with_gas_limit, get_erc20_deploy_tx, get_erc20_transfer_tx, get_heavy_load_test_tx, get_load_test_deploy_tx, get_load_test_tx, get_realistic_load_test_tx, - get_transfer_tx, BenchmarkingVm, BenchmarkingVmFactory, Bytecode, Fast, Legacy, LoadTestParams, + get_transfer_tx, BenchmarkingVm, BenchmarkingVmFactory, Bytecode, Fast, FastWithStorageLimit, + Legacy, LoadTestParams, }; use zksync_types::Transaction; @@ -197,6 +198,7 @@ criterion_group!( .with_measurement(MeteredTime::new("fill_bootloader")); targets = bench_fill_bootloader::, bench_fill_bootloader::, + bench_fill_bootloader::, bench_fill_bootloader:: ); criterion_main!(benches); diff --git a/core/tests/vm-benchmark/src/lib.rs b/core/tests/vm-benchmark/src/lib.rs index 8f43f61b28b6..1d404fcf86b0 100644 --- a/core/tests/vm-benchmark/src/lib.rs +++ b/core/tests/vm-benchmark/src/lib.rs @@ -6,7 +6,10 @@ pub use crate::{ get_heavy_load_test_tx, get_load_test_deploy_tx, get_load_test_tx, get_realistic_load_test_tx, get_transfer_tx, LoadTestParams, }, - vm::{BenchmarkingVm, BenchmarkingVmFactory, CountInstructions, Fast, Legacy, VmLabel}, + vm::{ + BenchmarkingVm, BenchmarkingVmFactory, CountInstructions, Fast, FastWithStorageLimit, + Legacy, VmLabel, + }, }; pub mod criterion; diff --git a/core/tests/vm-benchmark/src/vm.rs b/core/tests/vm-benchmark/src/vm.rs index a4b61ee54809..d99a4d040a0e 100644 --- a/core/tests/vm-benchmark/src/vm.rs +++ b/core/tests/vm-benchmark/src/vm.rs @@ -4,12 +4,12 @@ use once_cell::sync::Lazy; use zksync_contracts::BaseSystemContracts; use zksync_multivm::{ interface::{ - storage::{InMemoryStorage, StorageView}, + storage::{ImmutableStorageView, InMemoryStorage, StoragePtr, StorageView}, ExecutionResult, InspectExecutionMode, L1BatchEnv, L2BlockEnv, SystemEnv, TxExecutionMode, - VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceExt, - VmInterfaceHistoryEnabled, + VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceHistoryEnabled, }, vm_fast, + vm_fast::StorageInvocationsTracer, vm_latest::{self, constants::BATCH_COMPUTATIONAL_GAS_LIMIT, HistoryEnabled, ToTracerPointer}, zk_evm_latest::ethereum_types::{Address, U256}, }; @@ -36,6 +36,7 @@ static STORAGE: Lazy = Lazy::new(|| { #[derive(Debug, Clone, Copy)] pub enum VmLabel { Fast, + FastWithStorageLimit, Legacy, } @@ -44,6 +45,7 @@ impl VmLabel { pub const fn as_str(self) -> &'static str { match self { Self::Fast => "fast", + Self::FastWithStorageLimit => "fast/storage_limit", Self::Legacy => "legacy", } } @@ -52,13 +54,14 @@ impl VmLabel { pub const fn as_suffix(self) -> &'static str { match self { Self::Fast => "", + Self::FastWithStorageLimit => "/storage_limit", Self::Legacy => "/legacy", } } } /// Factory for VMs used in benchmarking. -pub trait BenchmarkingVmFactory { +pub trait BenchmarkingVmFactory: Sized { /// VM label used to name `criterion` benchmarks. const LABEL: VmLabel; @@ -70,7 +73,11 @@ pub trait BenchmarkingVmFactory { batch_env: L1BatchEnv, system_env: SystemEnv, storage: &'static InMemoryStorage, - ) -> Self::Instance; + ) -> (Self, Self::Instance); + + fn create_tracer(&self) -> ::TracerDispatcher { + ::TracerDispatcher::default() + } } pub trait CountInstructions { @@ -91,8 +98,8 @@ impl BenchmarkingVmFactory for Fast { batch_env: L1BatchEnv, system_env: SystemEnv, storage: &'static InMemoryStorage, - ) -> Self::Instance { - vm_fast::Vm::custom(batch_env, system_env, storage) + ) -> (Self, Self::Instance) { + (Self, vm_fast::Vm::custom(batch_env, system_env, storage)) } } @@ -121,6 +128,41 @@ impl CountInstructions for Fast { } } +#[derive(Debug)] +pub struct FastWithStorageLimit { + storage: StoragePtr>, +} + +impl BenchmarkingVmFactory for FastWithStorageLimit { + const LABEL: VmLabel = VmLabel::FastWithStorageLimit; + + type Instance = vm_fast::Vm< + ImmutableStorageView<&'static InMemoryStorage>, + StorageInvocationsTracer>, + >; + + fn create( + batch_env: L1BatchEnv, + system_env: SystemEnv, + storage: &'static InMemoryStorage, + ) -> (Self, Self::Instance) { + let storage = StorageView::new(storage).to_rc_ptr(); + let this = Self { + storage: storage.clone(), + }; + (this, vm_fast::Vm::new(batch_env, system_env, storage)) + } + + fn create_tracer(&self) -> ::TracerDispatcher { + // Set some large limit so that tx execution isn't affected by it. + let limit = u32::MAX as usize / 2; + ( + StorageInvocationsTracer::new(self.storage.clone(), limit), + (), + ) + } +} + /// Factory for the legacy VM (latest version). #[derive(Debug)] pub struct Legacy; @@ -134,18 +176,18 @@ impl BenchmarkingVmFactory for Legacy { batch_env: L1BatchEnv, system_env: SystemEnv, storage: &'static InMemoryStorage, - ) -> Self::Instance { + ) -> (Self, Self::Instance) { let storage = StorageView::new(storage).to_rc_ptr(); - vm_latest::Vm::new(batch_env, system_env, storage) + (Self, vm_latest::Vm::new(batch_env, system_env, storage)) } } impl CountInstructions for Legacy { fn count_instructions(tx: &Transaction) -> usize { let mut vm = BenchmarkingVm::::default(); - vm.0.push_transaction(tx.clone()); + vm.vm.push_transaction(tx.clone()); let count = Rc::new(RefCell::new(0)); - vm.0.inspect( + vm.vm.inspect( &mut InstructionCounter::new(count.clone()) .into_tracer_pointer() .into(), @@ -187,32 +229,42 @@ fn test_env() -> (SystemEnv, L1BatchEnv) { } #[derive(Debug)] -pub struct BenchmarkingVm(VM::Instance); +pub struct BenchmarkingVm { + factory: VM, + vm: VM::Instance, +} impl Default for BenchmarkingVm { fn default() -> Self { let (system_env, l1_batch_env) = test_env(); - Self(VM::create(l1_batch_env, system_env, &STORAGE)) + let (factory, vm) = VM::create(l1_batch_env, system_env, &STORAGE); + Self { factory, vm } } } impl BenchmarkingVm { pub fn run_transaction(&mut self, tx: &Transaction) -> VmExecutionResultAndLogs { - self.0.push_transaction(tx.clone()); - self.0.execute(InspectExecutionMode::OneTx) + self.vm.push_transaction(tx.clone()); + self.vm.inspect( + &mut self.factory.create_tracer(), + InspectExecutionMode::OneTx, + ) } pub fn run_transaction_full(&mut self, tx: &Transaction) -> VmExecutionResultAndLogs { - self.0.make_snapshot(); - let (compression_result, tx_result) = self - .0 - .execute_transaction_with_bytecode_compression(tx.clone(), true); + self.vm.make_snapshot(); + let (compression_result, tx_result) = + self.vm.inspect_transaction_with_bytecode_compression( + &mut self.factory.create_tracer(), + tx.clone(), + true, + ); compression_result.expect("compressing bytecodes failed"); if matches!(tx_result.result, ExecutionResult::Halt { .. }) { - self.0.rollback_to_the_latest_snapshot(); + self.vm.rollback_to_the_latest_snapshot(); } else { - self.0.pop_snapshot_no_rollback(); + self.vm.pop_snapshot_no_rollback(); } tx_result } From 155e1b70015f098d35d0dca47631174c8b9da347 Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Wed, 29 Jan 2025 15:17:20 +0200 Subject: [PATCH 4/5] Test storage limiter in API server --- core/lib/vm_executor/src/oneshot/mod.rs | 17 +++++++++--- core/lib/vm_interface/src/utils/shadow.rs | 23 +++++++++++++--- .../api_server/src/tx_sender/tests/call.rs | 17 ++++++++++++ .../src/tx_sender/tests/gas_estimation.rs | 27 +++++++++++++++++++ .../api_server/src/tx_sender/tests/mod.rs | 15 +++++++++-- 5 files changed, 91 insertions(+), 8 deletions(-) diff --git a/core/lib/vm_executor/src/oneshot/mod.rs b/core/lib/vm_executor/src/oneshot/mod.rs index 38c500ab6b6b..2705783ece61 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, StoragePtr, StorageView, StorageWithOverrides, WriteStorage}, tracer::{ValidationError, ValidationParams, ValidationTraces}, utils::{DivergenceHandler, ShadowMut, ShadowVm}, - Call, ExecutionResult, InspectExecutionMode, OneshotEnv, OneshotTracingParams, + Call, ExecutionResult, Halt, InspectExecutionMode, OneshotEnv, OneshotTracingParams, OneshotTransactionExecutionResult, StoredL2BlockEnv, TxExecutionArgs, TxExecutionMode, VmFactory, VmInterface, }, @@ -265,11 +265,22 @@ impl Vm>, ()> { let tracer = StorageInvocationsTracer::new(storage.clone(), missed_storage_invocation_limit); let mut full_tracer = (legacy_tracers.into(), (tracer, ())); - vm.inspect_transaction_with_bytecode_compression( + let mut result = vm.inspect_transaction_with_bytecode_compression( &mut full_tracer, tx, with_compression, - ) + ); + + if let ExecutionResult::Halt { + reason: Halt::TracerCustom(msg), + } = &mut result.1.result + { + // Patch the halt message to be more specific; the fast VM provides a generic one since it doesn't know + // which tracer(s) are run. Here, we do know that the only tracer capable of stopping VM execution is the storage limiter. + *msg = "Storage invocations limit reached".to_owned(); + } + + result } }; diff --git a/core/lib/vm_interface/src/utils/shadow.rs b/core/lib/vm_interface/src/utils/shadow.rs index e4a7aa51f78c..de18ccdfe3df 100644 --- a/core/lib/vm_interface/src/utils/shadow.rs +++ b/core/lib/vm_interface/src/utils/shadow.rs @@ -16,9 +16,9 @@ use crate::{ pubdata::PubdataBuilder, storage::{ReadStorage, StoragePtr, StorageView}, tracer::{ValidationError, ValidationTraces}, - BytecodeCompressionResult, Call, CallType, CurrentExecutionState, FinishedL1Batch, - InspectExecutionMode, L1BatchEnv, L2BlockEnv, PushTransactionResult, SystemEnv, - VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceHistoryEnabled, + BytecodeCompressionResult, Call, CallType, CurrentExecutionState, ExecutionResult, + FinishedL1Batch, Halt, InspectExecutionMode, L1BatchEnv, L2BlockEnv, PushTransactionResult, + SystemEnv, VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceHistoryEnabled, VmTrackingContracts, }; @@ -152,6 +152,23 @@ impl CheckDivergence for CurrentExecutionState { impl CheckDivergence for VmExecutionResultAndLogs { fn check_divergence(&self, other: &Self) -> DivergenceErrors { let mut errors = DivergenceErrors::new(); + + // Special case: the execution is stopped by a tracer. Because of significant differences between how tracers + // work in legacy and fast VMs, we only require the general stop reason to be the same, and ignore anything else. + if matches!( + (&self.result, &other.result), + ( + ExecutionResult::Halt { + reason: Halt::TracerCustom(_) + }, + ExecutionResult::Halt { + reason: Halt::TracerCustom(_) + } + ) + ) { + return errors; + } + errors.check_match("result", &self.result, &other.result); errors.check_match("logs.events", &self.logs.events, &other.logs.events); errors.check_match( diff --git a/core/node/api_server/src/tx_sender/tests/call.rs b/core/node/api_server/src/tx_sender/tests/call.rs index 08571790e8eb..cf3d2af7429e 100644 --- a/core/node/api_server/src/tx_sender/tests/call.rs +++ b/core/node/api_server/src/tx_sender/tests/call.rs @@ -3,6 +3,7 @@ use std::collections::HashMap; use assert_matches::assert_matches; +use test_casing::test_casing; use zksync_multivm::interface::ExecutionResult; use zksync_node_test_utils::create_l2_transaction; use zksync_types::{ @@ -252,3 +253,19 @@ async fn eth_call_with_load_test_transactions() { .unwrap(); } } + +#[test_casing(3, ALL_VM_MODES)] +#[tokio::test] +async fn limiting_storage_access_during_call(vm_mode: FastVmMode) { + let alice = K256PrivateKey::random(); + let state_override = StateBuilder::default().with_expensive_contract().build(); + + let tx = alice.create_expensive_tx(1_000); + let pool = ConnectionPool::::constrained_test_pool(1).await; + let tx_sender = create_real_tx_sender_with_options(pool, vm_mode, 100).await; + + let err = test_call(&tx_sender, state_override, tx.into()) + .await + .unwrap_err(); + assert_matches!(err, SubmitTxError::ExecutionReverted(msg, _) if msg.contains("limit reached")); +} diff --git a/core/node/api_server/src/tx_sender/tests/gas_estimation.rs b/core/node/api_server/src/tx_sender/tests/gas_estimation.rs index 954792f915cc..5f4db71f4f52 100644 --- a/core/node/api_server/src/tx_sender/tests/gas_estimation.rs +++ b/core/node/api_server/src/tx_sender/tests/gas_estimation.rs @@ -513,3 +513,30 @@ async fn estimating_gas_for_infinite_loop_tx() { assert_matches!(err, SubmitTxError::ExecutionReverted(msg, _) if msg.is_empty()); } } + +#[test_casing(3, ALL_VM_MODES)] +#[tokio::test] +async fn limiting_storage_access_during_gas_estimation(vm_mode: FastVmMode) { + let alice = K256PrivateKey::random(); + let state_override = StateBuilder::default().with_expensive_contract().build(); + + let tx = alice.create_expensive_tx(1_000); + let pool = ConnectionPool::::constrained_test_pool(1).await; + let tx_sender = create_real_tx_sender_with_options(pool, vm_mode, 100).await; + let block_args = pending_block_args(&tx_sender).await; + + let fee_scale_factor = 1.0; + let acceptable_overestimation = 0; + let err = tx_sender + .get_txs_fee_in_wei( + tx.into(), + block_args, + fee_scale_factor, + acceptable_overestimation, + Some(state_override), + BinarySearchKind::Full, + ) + .await + .unwrap_err(); + assert_matches!(err, SubmitTxError::ExecutionReverted(msg, _) if msg.contains("limit reached")); +} diff --git a/core/node/api_server/src/tx_sender/tests/mod.rs b/core/node/api_server/src/tx_sender/tests/mod.rs index 014bc5636c2d..193cc9e379f5 100644 --- a/core/node/api_server/src/tx_sender/tests/mod.rs +++ b/core/node/api_server/src/tx_sender/tests/mod.rs @@ -14,6 +14,8 @@ mod call; mod gas_estimation; mod send_tx; +const ALL_VM_MODES: [FastVmMode; 3] = [FastVmMode::Old, FastVmMode::New, FastVmMode::Shadow]; + const LOAD_TEST_CASES: TestCases = test_casing::cases! {[ LoadnextContractExecutionParams::default(), // No storage modification @@ -138,6 +140,14 @@ async fn getting_nonce_for_account_after_snapshot_recovery() { } async fn create_real_tx_sender(pool: ConnectionPool) -> TxSender { + create_real_tx_sender_with_options(pool, FastVmMode::Shadow, usize::MAX).await +} + +async fn create_real_tx_sender_with_options( + pool: ConnectionPool, + vm_mode: FastVmMode, + storage_invocations_limit: usize, +) -> TxSender { let mut storage = pool.connection().await.unwrap(); let genesis_params = GenesisParams::mock(); insert_genesis_batch(&mut storage, &genesis_params) @@ -153,10 +163,11 @@ async fn create_real_tx_sender(pool: ConnectionPool) -> TxSender { ) .await .unwrap(); - executor_options.set_fast_vm_mode(FastVmMode::Shadow); + executor_options.set_fast_vm_mode(vm_mode); let pg_caches = PostgresStorageCaches::new(1, 1); - let tx_executor = SandboxExecutor::real(executor_options, pg_caches, usize::MAX, None); + let tx_executor = + SandboxExecutor::real(executor_options, pg_caches, storage_invocations_limit, None); create_test_tx_sender(pool, genesis_params.config().l2_chain_id, tx_executor) .await .0 From b3c435c1d1d71389dafd630f4d32ba11f86d7eb0 Mon Sep 17 00:00:00 2001 From: Alex Ostrovski Date: Mon, 3 Feb 2025 14:23:36 +0200 Subject: [PATCH 5/5] Remove FIXME --- core/lib/multivm/src/versions/vm_fast/tracers/storage.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs b/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs index 8b743502b8e9..4e1f9c71ca6a 100644 --- a/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs +++ b/core/lib/multivm/src/versions/vm_fast/tracers/storage.rs @@ -6,7 +6,6 @@ use crate::interface::storage::{StoragePtr, WriteStorage}; /// Useful as an anti-DoS measure. #[derive(Debug)] pub struct StorageInvocationsTracer { - // FIXME: this is very awkward; logically, storage should be obtainable from `GlobalStateInterface` (but how?) storage: Option>, limit: usize, } @@ -36,9 +35,10 @@ impl Tracer for StorageInvocationsTracer { _state: &mut S, ) -> ShouldStop { if matches!(OP::VALUE, Opcode::StorageRead | Opcode::StorageWrite) { - let storage = self.storage.as_ref().unwrap(); - if storage.borrow().missed_storage_invocations() >= self.limit { - return ShouldStop::Stop; + if let Some(storage) = self.storage.as_ref() { + if storage.borrow().missed_storage_invocations() >= self.limit { + return ShouldStop::Stop; + } } } ShouldStop::Continue