From 31202189ba96119f0942edbf1156b0f2d7bae507 Mon Sep 17 00:00:00 2001 From: Sasha Gryaznov Date: Thu, 22 Dec 2022 19:22:57 +0200 Subject: [PATCH] [contracts] Remove stack height limiter wasm instrumentation (#12957) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove stack height limiter from uploaded wasm * fix benchmarks * ".git/.scripts/bench-bot.sh" pallet dev pallet_contracts Co-authored-by: command-bot <> Co-authored-by: Alexander Theißen --- frame/contracts/src/benchmarking/code.rs | 26 ++---------------------- frame/contracts/src/benchmarking/mod.rs | 7 ++----- frame/contracts/src/schedule.rs | 18 +--------------- frame/contracts/src/tests.rs | 15 ++++++-------- frame/contracts/src/wasm/prepare.rs | 15 +------------- 5 files changed, 12 insertions(+), 69 deletions(-) diff --git a/frame/contracts/src/benchmarking/code.rs b/frame/contracts/src/benchmarking/code.rs index 0b09726214260..156b9e2f99945 100644 --- a/frame/contracts/src/benchmarking/code.rs +++ b/frame/contracts/src/benchmarking/code.rs @@ -72,11 +72,6 @@ pub struct ModuleDefinition { pub aux_body: Option, /// The amount of I64 arguments the aux function should have. pub aux_arg_num: u32, - /// If set to true the stack height limiter is injected into the the module. This is - /// needed for instruction debugging because the cost of executing the stack height - /// instrumentation should be included in the costs for the individual instructions - /// that cause more metering code (only call). - pub inject_stack_metering: bool, /// Create a table containing function pointers. pub table: Option, /// Create a section named "dummy" of the specified size. This is useful in order to @@ -238,13 +233,7 @@ impl From for WasmModule { ))); } - let mut code = contract.build(); - - if def.inject_stack_metering { - code = inject_stack_metering::(code); - } - - let code = code.into_bytes().unwrap(); + let code = contract.build().into_bytes().unwrap(); let hash = T::Hashing::hash(&code); Self { code: code.into(), hash, memory: def.memory } } @@ -252,15 +241,12 @@ impl From for WasmModule { impl WasmModule { /// Uses the supplied wasm module and instruments it when requested. - pub fn instrumented(code: &[u8], inject_gas: bool, inject_stack: bool) -> Self { + pub fn instrumented(code: &[u8], inject_gas: bool) -> Self { let module = { let mut module = Module::from_bytes(code).unwrap(); if inject_gas { module = inject_gas_metering::(module); } - if inject_stack { - module = inject_stack_metering::(module); - } module }; let limits = *module @@ -530,11 +516,3 @@ fn inject_gas_metering(module: Module) -> Module { let backend = gas_metering::host_function::Injector::new("seal0", "gas"); gas_metering::inject(module, backend, &gas_rules).unwrap() } - -fn inject_stack_metering(module: Module) -> Module { - if let Some(height) = T::Schedule::get().limits.stack_height { - wasm_instrument::inject_stack_limiter(module, height).unwrap() - } else { - module - } -} diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index da9b53495d077..d46da8469eb41 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -2384,7 +2384,6 @@ benchmarks! { call_body: Some(body::repeated(r * INSTR_BENCHMARK_BATCH_SIZE, &[ Instruction::Call(2), // call aux ])), - inject_stack_metering: true, .. Default::default() })); }: { @@ -2408,7 +2407,6 @@ benchmarks! { RandomI32(0, num_elements as i32), Regular(Instruction::CallIndirect(0, 0)), // we only have one sig: 0 ])), - inject_stack_metering: true, table: Some(TableSegment { num_elements, function_index: 2, // aux @@ -2441,7 +2439,6 @@ benchmarks! { RandomI32(0, num_elements as i32), Regular(Instruction::CallIndirect(p.min(1), 0)), // aux signature: 1 or 0 ])), - inject_stack_metering: true, table: Some(TableSegment { num_elements, function_index: 2, // aux @@ -2968,7 +2965,7 @@ benchmarks! { new.encode() }; let instance = Contract::::new( - WasmModule::instrumented(code, gas_metering, true), data, + WasmModule::instrumented(code, gas_metering), data, )?; let data = { let transfer: ([u8; 4], AccountIdOf, BalanceOf) = ( @@ -3015,7 +3012,7 @@ benchmarks! { new.encode() }; let instance = Contract::::with_caller( - caller, WasmModule::instrumented(code, gas_metering, true), data, + caller, WasmModule::instrumented(code, gas_metering), data, )?; balance[0] = 1; let data = { diff --git a/frame/contracts/src/schedule.rs b/frame/contracts/src/schedule.rs index e7a6b16e23a2b..912e58b048ff4 100644 --- a/frame/contracts/src/schedule.rs +++ b/frame/contracts/src/schedule.rs @@ -99,23 +99,9 @@ pub struct Limits { /// The maximum number of topics supported by an event. pub event_topics: u32, - /// Maximum allowed stack height in number of elements. - /// - /// See to find out - /// how the stack frame cost is calculated. Each element can be of one of the - /// wasm value types. This means the maximum size per element is 64bit. - /// - /// # Note - /// - /// It is safe to disable (pass `None`) the `stack_height` when the execution engine - /// is part of the runtime and hence there can be no indeterminism between different - /// client resident execution engines. - pub stack_height: Option, - /// Maximum number of globals a module is allowed to declare. /// - /// Globals are not limited through the `stack_height` as locals are. Neither does - /// the linear memory limit `memory_pages` applies to them. + /// Globals are not limited through the linear memory limit `memory_pages`. pub globals: u32, /// Maximum number of locals a function can have. @@ -532,8 +518,6 @@ impl Default for Limits { fn default() -> Self { Self { event_topics: 4, - // No stack limit required because we use a runtime resident execution engine. - stack_height: None, globals: 256, locals: 1024, parameters: 128, diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 85fe22ab9a19b..7a0736d1ed61a 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -339,9 +339,6 @@ impl pallet_utility::Config for Test { parameter_types! { pub MySchedule: Schedule = { let mut schedule = >::default(); - // We want stack height to be always enabled for tests so that this - // instrumentation path is always tested implicitly. - schedule.limits.stack_height = Some(512); schedule.instruction_weights.fallback = 1; schedule }; @@ -2964,7 +2961,7 @@ fn upload_code_works() { phase: Phase::Initialization, event: RuntimeEvent::Balances(pallet_balances::Event::Reserved { who: ALICE, - amount: 241, + amount: 173, }), topics: vec![], }, @@ -3054,7 +3051,7 @@ fn remove_code_works() { phase: Phase::Initialization, event: RuntimeEvent::Balances(pallet_balances::Event::Reserved { who: ALICE, - amount: 241, + amount: 173, }), topics: vec![], }, @@ -3067,7 +3064,7 @@ fn remove_code_works() { phase: Phase::Initialization, event: RuntimeEvent::Balances(pallet_balances::Event::Unreserved { who: ALICE, - amount: 241, + amount: 173, }), topics: vec![], }, @@ -3110,7 +3107,7 @@ fn remove_code_wrong_origin() { phase: Phase::Initialization, event: RuntimeEvent::Balances(pallet_balances::Event::Reserved { who: ALICE, - amount: 241, + amount: 173, }), topics: vec![], }, @@ -3246,7 +3243,7 @@ fn instantiate_with_zero_balance_works() { phase: Phase::Initialization, event: RuntimeEvent::Balances(pallet_balances::Event::Reserved { who: ALICE, - amount: 241, + amount: 173, }), topics: vec![], }, @@ -3351,7 +3348,7 @@ fn instantiate_with_below_existential_deposit_works() { phase: Phase::Initialization, event: RuntimeEvent::Balances(pallet_balances::Event::Reserved { who: ALICE, - amount: 241, + amount: 173, }), topics: vec![], }, diff --git a/frame/contracts/src/wasm/prepare.rs b/frame/contracts/src/wasm/prepare.rs index 9bb58bb02b5ea..dfe25bc3af89f 100644 --- a/frame/contracts/src/wasm/prepare.rs +++ b/frame/contracts/src/wasm/prepare.rs @@ -216,16 +216,6 @@ impl<'a, T: Config> ContractModule<'a, T> { Ok(ContractModule { module: contract_module, schedule: self.schedule }) } - fn inject_stack_height_metering(self) -> Result { - if let Some(limit) = self.schedule.limits.stack_height { - let contract_module = wasm_instrument::inject_stack_limiter(self.module, limit) - .map_err(|_| "stack height instrumentation failed")?; - Ok(ContractModule { module: contract_module, schedule: self.schedule }) - } else { - Ok(ContractModule { module: self.module, schedule: self.schedule }) - } - } - /// Check that the module has required exported functions. For now /// these are just entrypoints: /// @@ -447,10 +437,7 @@ where let memory_limits = get_memory_limits(contract_module.scan_imports(&disallowed_imports)?, schedule)?; - let code = contract_module - .inject_gas_metering(determinism)? - .inject_stack_height_metering()? - .into_wasm_code()?; + let code = contract_module.inject_gas_metering(determinism)?.into_wasm_code()?; Ok((code, memory_limits)) })()