contracts: Remove weight pre charging (#8976)

* Remove pre-charging for code size

* Remove pre charging when reading values of fixed size

* Add new versions of API functions that leave out parameters

* Update CHANGELOG.md

* Apply suggestions from code review

Co-authored-by: Alexander Popiak <[email protected]>

* Add v1 for seal_set_rent_allowance

* Remove unneeded trait bound

Co-authored-by: Guillaume Thiolliere <[email protected]>

Co-authored-by: Alexander Popiak <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>

Author: 3 people

frame/contracts/CHANGELOG.md

@@ -42,6 +42,11 @@ output to an RPC client.
 - Make storage and fields of `Schedule` private to the crate.
 [#8359](https://github.com/paritytech/substrate/pull/8359)
 
+### Fixed
+
+- Remove pre-charging which caused wrongly estimated weights
+[#8976](https://github.com/paritytech/substrate/pull/8976)
+
 ## [v3.0.0] 2021-02-25
 
 This version constitutes the first release that brings any stability guarantees (see above).

frame/contracts/fixtures/dummy.wat

@@ -0,0 +1,5 @@
+;; A valid contract which does nothing at all
+(module
+	(func (export "deploy"))
+	(func (export "call"))
+)

frame/contracts/fixtures/instantiate_return_code.wat

@@ -4,8 +4,8 @@
 ;; The rest of the input is forwarded to the constructor of the callee
 (module
 	(import "seal0" "seal_input" (func $seal_input (param i32 i32)))
-	(import "seal0" "seal_instantiate" (func $seal_instantiate
-		(param i32 i32 i64 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32) (result i32)
+	(import "seal1" "seal_instantiate" (func $seal_instantiate
+		(param i32 i64 i32 i32 i32 i32 i32 i32 i32 i32 i32) (result i32)
 	))
 	(import "seal0" "seal_return" (func $seal_return (param i32 i32 i32)))
 	(import "env" "memory" (memory 1 1))
@@ -29,10 +29,8 @@
 			(i32.const 8)
 			(call $seal_instantiate
 				(i32.const 16) ;; Pointer to the code hash.
-				(i32.const 32) ;; Length of the code hash.
 				(i64.const 0) ;; How much gas to devote for the execution. 0 = all.
 				(i32.const 0) ;; Pointer to the buffer with value to transfer
-				(i32.const 8) ;; Length of the buffer with value to transfer.
 				(i32.const 48) ;; Pointer to input data buffer address
 				(i32.const 4) ;; Length of input data buffer
 				(i32.const 0xffffffff) ;; u32 max sentinel value: do not copy address

frame/contracts/fixtures/ok_trap_revert.wat

@@ -32,4 +32,4 @@
         ;; 2 = trap
         (unreachable)
     )
-)
+)

frame/contracts/fixtures/restoration.wat

@@ -1,9 +1,9 @@
 (module
 	(import "seal0" "seal_set_storage" (func $seal_set_storage (param i32 i32 i32)))
 	(import "seal0" "seal_input" (func $seal_input (param i32 i32)))
-	(import "seal0" "seal_restore_to"
+	(import "seal1" "seal_restore_to"
 		(func $seal_restore_to
-			(param i32 i32 i32 i32 i32 i32 i32 i32)
+			(param i32 i32 i32 i32 i32)
 		)
 	)
 	(import "env" "memory" (memory 1 1))
@@ -27,15 +27,12 @@
 			)
 		)
 		(call $seal_restore_to
-			;; Pointer and length of the encoded dest buffer.
+			;; Pointer to the encoded dest buffer.
 			(i32.const 340)
-			(i32.const 32)
-			;; Pointer and length of the encoded code hash buffer
+			;; Pointer to the encoded code hash buffer
 			(i32.const 308)
-			(i32.const 32)
-			;; Pointer and length of the encoded rent_allowance buffer
+			;; Pointer to the encoded rent_allowance buffer
 			(i32.const 296)
-			(i32.const 8)
 			;; Pointer and number of items in the delta buffer.
 			;; This buffer specifies multiple keys for removal before restoration.
 			(i32.const 100)

frame/contracts/src/benchmarking/code.rs

@@ -258,9 +258,14 @@ where
 
 	/// Same as `dummy` but with maximum sized linear memory and a dummy section of specified size.
 	pub fn dummy_with_bytes(dummy_bytes: u32) -> Self {
+		// We want the module to have the size `dummy_bytes`.
+		// This is not completely correct as the overhead grows when the contract grows
+		// because of variable length integer encoding. However, it is good enough to be that
+		// close for benchmarking purposes.
+		let module_overhead = 65;
 		ModuleDefinition {
 			memory: Some(ImportedMemory::max::<T>()),
-			dummy_section: dummy_bytes,
+			dummy_section: dummy_bytes.saturating_sub(module_overhead),
 			.. Default::default()
 		}
 		.into()

frame/contracts/src/benchmarking/mod.rs

@@ -320,6 +320,25 @@ benchmarks! {
 		Contracts::<T>::reinstrument_module(&mut module, &schedule)?;
 	}
 
+	// The weight of loading and decoding of a contract's code per kilobyte.
+	code_load {
+		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)?;
+	}: {
+		<PrefabWasmModule<T>>::from_storage_noinstr(hash)?;
+	}
+
+	// The weight of changing the refcount of a contract's code per kilobyte.
+	code_refcount {
+		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)?;
+		let mut gas_meter = GasMeter::new(Weight::max_value());
+	}: {
+		<PrefabWasmModule<T>>::add_user(hash, &mut gas_meter)?;
+	}
+
 	// 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
@@ -352,16 +371,14 @@ benchmarks! {
 	}
 
 	// Instantiate uses a dummy contract constructor to measure the overhead of the instantiate.
-	// `c`: Size of the code in kilobytes.
 	// `s`: Size of the salt in kilobytes.
 	instantiate {
-		let c in 0 .. T::Schedule::get().limits.code_len / 1024;
 		let s in 0 .. code::max_pages::<T>() * 64;
 		let salt = vec![42u8; (s * 1024) as usize];
 		let endowment = caller_funding::<T>() / 3u32.into();
 		let caller = whitelisted_caller();
 		T::Currency::make_free_balance_be(&caller, caller_funding::<T>());
-		let WasmModule { code, hash, .. } = WasmModule::<T>::dummy_with_bytes(c * 1024);
+		let WasmModule { code, hash, .. } = WasmModule::<T>::dummy();
 		let origin = RawOrigin::Signed(caller.clone());
 		let addr = Contracts::<T>::contract_address(&caller, &hash, &salt);
 		Contracts::<T>::store_code_raw(code)?;
@@ -380,12 +397,10 @@ benchmarks! {
 	// 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`.
-	// `c`: Size of the code in kilobytes.
 	call {
-		let c in 0 .. T::Schedule::get().limits.code_len / 1024;
 		let data = vec![42u8; 1024];
 		let instance = Contract::<T>::with_caller(
-			whitelisted_caller(), WasmModule::dummy_with_bytes(c * 1024), vec![], Endow::CollectRent
+			whitelisted_caller(), WasmModule::dummy(), vec![], Endow::CollectRent
 		)?;
 		let value = T::Currency::minimum_balance() * 100u32.into();
 		let origin = RawOrigin::Signed(instance.caller.clone());
@@ -720,43 +735,6 @@ benchmarks! {
 		}
 	}
 
-	seal_terminate_per_code_kb {
-		let c in 0 .. T::Schedule::get().limits.code_len / 1024;
-		let beneficiary = account::<T::AccountId>("beneficiary", 0, 0);
-		let beneficiary_bytes = beneficiary.encode();
-		let beneficiary_len = beneficiary_bytes.len();
-		let code = WasmModule::<T>::from(ModuleDefinition {
-			memory: Some(ImportedMemory::max::<T>()),
-			imported_functions: vec![ImportedFunction {
-				module: "seal0",
-				name: "seal_terminate",
-				params: vec![ValueType::I32, ValueType::I32],
-				return_type: None,
-			}],
-			data_segments: vec![
-				DataSegment {
-					offset: 0,
-					value: beneficiary_bytes,
-				},
-			],
-			call_body: Some(body::repeated(1, &[
-				Instruction::I32Const(0), // beneficiary_ptr
-				Instruction::I32Const(beneficiary_len as i32), // beneficiary_len
-				Instruction::Call(0),
-			])),
-			dummy_section: c * 1024,
-			.. Default::default()
-		});
-		let instance = Contract::<T>::new(code, vec![], Endow::Max)?;
-		let origin = RawOrigin::Signed(instance.caller.clone());
-		assert_eq!(T::Currency::total_balance(&beneficiary), 0u32.into());
-		assert_eq!(T::Currency::total_balance(&instance.account_id), Endow::max::<T>());
-	}: call(origin, instance.addr, 0u32.into(), Weight::max_value(), vec![])
-	verify {
-		assert_eq!(T::Currency::total_balance(&instance.account_id), 0u32.into());
-		assert_eq!(T::Currency::total_balance(&beneficiary), Endow::max::<T>());
-	}
-
 	seal_restore_to {
 		let r in 0 .. 1;
 
@@ -836,18 +814,15 @@ benchmarks! {
 		}
 	}
 
-	// `c`: Code size of caller contract
-	// `t`: Code size of tombstone contract
 	// `d`: Number of supplied delta keys
-	seal_restore_to_per_code_kb_delta {
-		let c in 0 .. T::Schedule::get().limits.code_len / 1024;
-		let t in 0 .. T::Schedule::get().limits.code_len / 1024;
+	seal_restore_to_per_delta {
 		let d in 0 .. API_BENCHMARK_BATCHES;
-		let mut tombstone = ContractWithStorage::<T>::with_code(
-			WasmModule::<T>::dummy_with_bytes(t * 1024), 0, 0
-		)?;
+		let mut tombstone = ContractWithStorage::<T>::new(0, 0)?;
 		tombstone.evict()?;
-		let delta = create_storage::<T>(d * API_BENCHMARK_BATCH_SIZE, T::Schedule::get().limits.payload_len)?;
+		let delta = create_storage::<T>(
+			d * API_BENCHMARK_BATCH_SIZE,
+			T::Schedule::get().limits.payload_len,
+		)?;
 
 		let dest = tombstone.contract.account_id.encode();
 		let dest_len = dest.len();
@@ -909,7 +884,6 @@ benchmarks! {
 				Instruction::Call(0),
 				Instruction::End,
 			])),
-			dummy_section: c * 1024,
 			.. Default::default()
 		});
 
@@ -1393,8 +1367,7 @@ benchmarks! {
 		let origin = RawOrigin::Signed(instance.caller.clone());
 	}: call(origin, instance.addr, 0u32.into(), Weight::max_value(), vec![])
 
-	seal_call_per_code_transfer_input_output_kb {
-		let c in 0 .. T::Schedule::get().limits.code_len / 1024;
+	seal_call_per_transfer_input_output_kb {
 		let t in 0 .. 1;
 		let i in 0 .. code::max_pages::<T>() * 64;
 		let o in 0 .. (code::max_pages::<T>() - 1) * 64;
@@ -1417,7 +1390,6 @@ benchmarks! {
 				Instruction::Call(0),
 				Instruction::End,
 			])),
-			dummy_section: c * 1024,
 			.. Default::default()
 		});
 		let callees = (0..API_BENCHMARK_BATCH_SIZE)
@@ -1593,8 +1565,7 @@ benchmarks! {
 		}
 	}
 
-	seal_instantiate_per_code_input_output_salt_kb {
-		let c in 0 .. T::Schedule::get().limits.code_len / 1024;
+	seal_instantiate_per_input_output_salt_kb {
 		let i in 0 .. (code::max_pages::<T>() - 1) * 64;
 		let o in 0 .. (code::max_pages::<T>() - 1) * 64;
 		let s in 0 .. (code::max_pages::<T>() - 1) * 64;
@@ -1617,7 +1588,6 @@ benchmarks! {
 				Instruction::Call(0),
 				Instruction::End,
 			])),
-			dummy_section: c * 1024,
 			.. Default::default()
 		});
 		let hash = callee_code.hash.clone();

frame/contracts/src/chain_extension.rs

@@ -59,7 +59,7 @@ use crate::{
 	wasm::{Runtime, RuntimeCosts},
 };
 use codec::Decode;
-use frame_support::weights::Weight;
+use frame_support::{weights::Weight, traits::MaxEncodedLen};
 use sp_runtime::DispatchError;
 use sp_std::{
 	marker::PhantomData,
@@ -300,18 +300,21 @@ where
 		Ok(())
 	}
 
-	/// Reads `in_len` from contract memory and scale decodes it.
+	/// Reads and decodes a type with a size fixed at compile time from contract memory.
 	///
 	/// This function is secure and recommended for all input types of fixed size
 	/// as long as the cost of reading the memory is included in the overall already charged
 	/// weight of the chain extension. This should usually be the case when fixed input types
-	/// are used. Non fixed size types (like everything using `Vec`) usually need to use
-	/// [`in_len()`](Self::in_len) in order to properly charge the necessary weight.
-	pub fn read_as<T: Decode>(&mut self) -> Result<T> {
-		self.inner.runtime.read_sandbox_memory_as(
-			self.inner.input_ptr,
-			self.inner.input_len,
-		)
+	/// are used.
+	pub fn read_as<T: Decode + MaxEncodedLen>(&mut self) -> Result<T> {
+		self.inner.runtime.read_sandbox_memory_as(self.inner.input_ptr)
+	}
+
+	/// Reads and decodes a type with a dynamic size from contract memory.
+	///
+	/// Make sure to include `len` in your weight calculations.
+	pub fn read_as_unbounded<T: Decode>(&mut self, len: u32) -> Result<T> {
+		self.inner.runtime.read_sandbox_memory_as_unbounded(self.inner.input_ptr, len)
 	}
 
 	/// The length of the input as passed in as `input_len`.