Make sure that weight for loading code into the sandbox is accounted for (#10448)

substrate/frame/contracts/src/benchmarking/code.rs

@@ -41,6 +41,14 @@ use sp_sandbox::{
 };
 use sp_std::{borrow::ToOwned, prelude::*};
 
+/// The location where to put the genrated code.
+pub enum Location {
+	/// Generate all code into the `call` exported function.
+	Call,
+	/// Generate all code into the `deploy` exported function.
+	Deploy,
+}
+
 /// Pass to `create_code` in order to create a compiled `WasmModule`.
 ///
 /// This exists to have a more declarative way to describe a wasm module than to use
@@ -308,7 +316,8 @@ where
 	/// Creates a wasm module of `target_bytes` size. Used to benchmark the performance of
 	/// `instantiate_with_code` for different sizes of wasm modules. The generated module maximizes
 	/// instrumentation runtime by nesting blocks as deeply as possible given the byte budget.
-	pub fn sized(target_bytes: u32) -> Self {
+	/// `code_location`: Whether to place the code into `deploy` or `call`.
+	pub fn sized(target_bytes: u32, code_location: Location) -> Self {
 		use self::elements::Instruction::{End, I32Const, If, Return};
 		// Base size of a contract is 63 bytes and each expansion adds 6 bytes.
 		// We do one expansion less to account for the code section and function body
@@ -317,12 +326,14 @@ where
 		// because of the maximum code size that is enforced by `instantiate_with_code`.
 		let expansions = (target_bytes.saturating_sub(63) / 6).saturating_sub(1);
 		const EXPANSION: [Instruction; 4] = [I32Const(0), If(BlockType::NoResult), Return, End];
-		ModuleDefinition {
-			call_body: Some(body::repeated(expansions, &EXPANSION)),
-			memory: Some(ImportedMemory::max::<T>()),
-			..Default::default()
+		let mut module =
+			ModuleDefinition { memory: Some(ImportedMemory::max::<T>()), ..Default::default() };
+		let body = Some(body::repeated(expansions, &EXPANSION));
+		match code_location {
+			Location::Call => module.call_body = body,
+			Location::Deploy => module.deploy_body = body,
 		}
-		.into()
+		module.into()
 	}
 
 	/// Creates a wasm module that calls the imported function named `getter_name` `repeat`

substrate/frame/contracts/src/benchmarking/mod.rs

@@ -25,7 +25,7 @@ mod sandbox;
 use self::{
 	code::{
 		body::{self, DynInstr::*},
-		DataSegment, ImportedFunction, ImportedMemory, ModuleDefinition, WasmModule,
+		DataSegment, ImportedFunction, ImportedMemory, Location, ModuleDefinition, WasmModule,
 	},
 	sandbox::Sandbox,
 };
@@ -229,9 +229,9 @@ benchmarks! {
 	// This benchmarks the additional weight that is charged when a contract is executed the
 	// first time after a new schedule was deployed: For every new schedule a contract needs
 	// to re-run the instrumentation once.
-	instrument {
+	reinstrument {
 		let c in 0 .. T::Schedule::get().limits.code_len / 1024;
-		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c * 1024);
+		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c * 1024, Location::Call);
 		Contracts::<T>::store_code_raw(code, whitelisted_caller())?;
 		let schedule = T::Schedule::get();
 		let mut gas_meter = GasMeter::new(Weight::MAX);
@@ -240,21 +240,28 @@ benchmarks! {
 		Contracts::<T>::reinstrument_module(&mut module, &schedule)?;
 	}
 
-	// The weight of loading and decoding of a contract's code per kilobyte.
-	code_load {
+	// This benchmarks the overhead of loading a code of size `c` kb from storage and into
+	// the sandbox. This does **not** include the actual execution for which the gas meter
+	// is responsible. This is achieved by generating all code to the `deploy` function
+	// which is in the wasm module but not executed on `call`.
+	// The results are supposed to be used as `call_with_code_kb(c) - call_with_code_kb(0)`.
+	call_with_code_kb {
 		let c in 0 .. T::Schedule::get().limits.code_len / 1024;
-		let WasmModule { code, hash, .. } = WasmModule::<T>::dummy_with_bytes(c * 1024);
-		Contracts::<T>::store_code_raw(code, whitelisted_caller())?;
-		let schedule = T::Schedule::get();
-		let mut gas_meter = GasMeter::new(Weight::MAX);
-	}: {
-		<PrefabWasmModule<T>>::from_storage(hash, &schedule, &mut gas_meter)?;
-	}
+		let instance = Contract::<T>::with_caller(
+			whitelisted_caller(), WasmModule::sized(c * 1024, Location::Deploy), vec![],
+		)?;
+		let value = T::Currency::minimum_balance();
+		let origin = RawOrigin::Signed(instance.caller.clone());
+		let callee = instance.addr.clone();
+	}: call(origin, callee, value, Weight::MAX, None, vec![])
 
 	// This constructs a contract that is maximal expensive to instrument.
 	// It creates a maximum number of metering blocks per byte.
 	// The size of the salt influences the runtime because is is hashed in order to
-	// determine the contract address.
+	// determine the contract address. All code is generated to the `call` function so that
+	// we don't benchmark the actual execution of this code but merely what it takes to load
+	// a code of that size into the sandbox.
+	//
 	// `c`: Size of the code in kilobytes.
 	// `s`: Size of the salt in kilobytes.
 	//
@@ -269,7 +276,7 @@ benchmarks! {
 		let value = T::Currency::minimum_balance();
 		let caller = whitelisted_caller();
 		T::Currency::make_free_balance_be(&caller, caller_funding::<T>());
-		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c * 1024);
+		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c * 1024, Location::Call);
 		let origin = RawOrigin::Signed(caller.clone());
 		let addr = Contracts::<T>::contract_address(&caller, &hash, &salt);
 	}: _(origin, value, Weight::MAX, None, code, vec![], salt)
@@ -316,7 +323,9 @@ benchmarks! {
 	// The size of the data has no influence on the costs of this extrinsic as long as the contract
 	// won't call `seal_input` in its constructor to copy the data to contract memory.
 	// The dummy contract used here does not do this. The costs for the data copy is billed as
-	// part of `seal_input`.
+	// part of `seal_input`. The costs for invoking a contract of a specific size are not part
+	// of this benchmark because we cannot know the size of the contract when issuing a call
+	// transaction. See `invoke_per_code_kb` for this.
 	call {
 		let data = vec![42u8; 1024];
 		let instance = Contract::<T>::with_caller(
@@ -353,7 +362,7 @@ benchmarks! {
 		let c in 0 .. Perbill::from_percent(50).mul_ceil(T::Schedule::get().limits.code_len / 1024);
 		let caller = whitelisted_caller();
 		T::Currency::make_free_balance_be(&caller, caller_funding::<T>());
-		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c * 1024);
+		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c * 1024, Location::Call);
 		let origin = RawOrigin::Signed(caller.clone());
 	}: _(origin, code, None)
 	verify {

substrate/frame/contracts/src/tests.rs

@@ -1903,7 +1903,7 @@ fn reinstrument_does_charge() {
 		assert!(result2.gas_consumed > result1.gas_consumed);
 		assert_eq!(
 			result2.gas_consumed,
-			result1.gas_consumed + <Test as Config>::WeightInfo::instrument(code_len / 1024),
+			result1.gas_consumed + <Test as Config>::WeightInfo::reinstrument(code_len / 1024),
 		);
 	});
 }

substrate/frame/contracts/src/wasm/code_cache.rs

@@ -158,7 +158,7 @@ where
 	if prefab_module.instruction_weights_version < schedule.instruction_weights.version {
 		// The instruction weights have changed.
 		// We need to re-instrument the code with the new instruction weights.
-		gas_meter.charge(CodeToken::Instrument(estimate_code_size::<T, PristineCode<T>, _>(
+		gas_meter.charge(CodeToken::Reinstrument(estimate_code_size::<T, PristineCode<T>, _>(
 			&code_hash,
 		)?))?;
 		reinstrument(&mut prefab_module, schedule)?;
@@ -201,9 +201,9 @@ where
 #[cfg_attr(test, derive(Debug, PartialEq, Eq))]
 #[derive(Clone, Copy)]
 enum CodeToken {
-	/// Weight for instrumenting a contract contract of the supplied size in bytes.
-	Instrument(u32),
-	/// Weight for loading a contract per kilobyte.
+	/// Weight for reinstrumenting a contract contract of the supplied size in bytes.
+	Reinstrument(u32),
+	/// Weight for loading a contract per byte.
 	Load(u32),
 }
 
@@ -211,14 +211,14 @@ impl<T: Config> Token<T> for CodeToken {
 	fn weight(&self) -> Weight {
 		use self::CodeToken::*;
 		// In case of `Load` we already covered the general costs of
-		// accessing the storage but still need to account for the actual size of the
+		// calling the storage but still need to account for the actual size of the
 		// contract code. This is why we substract `T::*::(0)`. We need to do this at this
-		// point because when charging the general weight we do not know the size of
-		// the contract.
+		// point because when charging the general weight for calling the contract we not know the
+		// size of the contract.
 		match *self {
-			Instrument(len) => T::WeightInfo::instrument(len / 1024),
-			Load(len) =>
-				T::WeightInfo::code_load(len / 1024).saturating_sub(T::WeightInfo::code_load(0)),
+			Reinstrument(len) => T::WeightInfo::reinstrument(len / 1024),
+			Load(len) => T::WeightInfo::call_with_code_kb(len / 1024)
+				.saturating_sub(T::WeightInfo::call_with_code_kb(0)),
 		}
 	}
 }