[contracts] Remove stack height limiter wasm instrumentation (parityt…

…ech#12957) * 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 <[email protected]>
gear-tech · Feb 27, 2023 · 3120218 · 3120218
1 parent 38fa012
commit 3120218
Show file tree

Hide file tree

Showing 5 changed files with 12 additions and 69 deletions.
diff --git 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<FuncBody>,
 	/// 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<TableSegment>,
 	/// Create a section named "dummy" of the specified size. This is useful in order to
@@ -238,29 +233,20 @@ impl<T: Config> From<ModuleDefinition> for WasmModule<T> {
 			)));
 		}
 
-		let mut code = contract.build();
-
-		if def.inject_stack_metering {
-			code = inject_stack_metering::<T>(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 }
 	}
 }
 
 impl<T: Config> WasmModule<T> {
 	/// 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::<T>(module);
 			}
-			if inject_stack {
-				module = inject_stack_metering::<T>(module);
-			}
 			module
 		};
 		let limits = *module
@@ -530,11 +516,3 @@ fn inject_gas_metering<T: Config>(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<T: Config>(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
@@ -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::<T>::new(
-			WasmModule::instrumented(code, gas_metering, true), data,
+			WasmModule::instrumented(code, gas_metering), data,
 		)?;
 		let data = {
 			let transfer: ([u8; 4], AccountIdOf<T>, BalanceOf<T>) = (
@@ -3015,7 +3012,7 @@ benchmarks! {
 			new.encode()
 		};
 		let instance = Contract::<T>::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
@@ -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 <https://wiki.parity.io/WebAssembly-StackHeight> 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<u32>,
-
 	/// 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
@@ -339,9 +339,6 @@ impl pallet_utility::Config for Test {
 parameter_types! {
 	pub MySchedule: Schedule<Test> = {
 		let mut schedule = <Schedule<Test>>::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
@@ -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<Self, &'static str> {
-		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))
 	})()