contracts: Get rid of #[pallet::without_storage_info] (paritytech#1…

…1414)

* Implement `MaxEncodeLen` for pallet-contracts storage

* Remove redundant debug println

* Move code len check to PrefabWasmModule::from_code

bin/node/runtime/src/lib.rs

@@ -1118,6 +1118,8 @@ impl pallet_contracts::Config for Runtime {
 	type Schedule = Schedule;
 	type AddressGenerator = pallet_contracts::DefaultAddressGenerator;
 	type ContractAccessWeight = pallet_contracts::DefaultContractAccessWeight<RuntimeBlockWeights>;
+	type MaxCodeLen = ConstU32<{ 128 * 1024 }>;
+	type RelaxedMaxCodeLen = ConstU32<{ 256 * 1024 }>;
 }
 
 impl pallet_sudo::Config for Runtime {

frame/contracts/src/benchmarking/mod.rs

@@ -231,7 +231,7 @@ benchmarks! {
 	// first time after a new schedule was deployed: For every new schedule a contract needs
 	// to re-run the instrumentation once.
 	reinstrument {
-		let c in 0 .. T::Schedule::get().limits.code_len;
+		let c in 0 .. Perbill::from_percent(49).mul_ceil(T::MaxCodeLen::get());
 		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c, Location::Call);
 		Contracts::<T>::store_code_raw(code, whitelisted_caller())?;
 		let schedule = T::Schedule::get();
@@ -247,7 +247,7 @@ benchmarks! {
 	// 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_per_byte {
-		let c in 0 .. T::Schedule::get().limits.code_len;
+		let c in 0 .. T::MaxCodeLen::get();
 		let instance = Contract::<T>::with_caller(
 			whitelisted_caller(), WasmModule::sized(c, Location::Deploy), vec![],
 		)?;
@@ -271,7 +271,7 @@ benchmarks! {
 	// We cannot let `c` grow to the maximum code size because the code is not allowed
 	// to be larger than the maximum size **after instrumentation**.
 	instantiate_with_code {
-		let c in 0 .. Perbill::from_percent(49).mul_ceil(T::Schedule::get().limits.code_len);
+		let c in 0 .. Perbill::from_percent(49).mul_ceil(T::MaxCodeLen::get());
 		let s in 0 .. code::max_pages::<T>() * 64 * 1024;
 		let salt = vec![42u8; s as usize];
 		let value = T::Currency::minimum_balance();
@@ -360,7 +360,7 @@ benchmarks! {
 	// We cannot let `c` grow to the maximum code size because the code is not allowed
 	// to be larger than the maximum size **after instrumentation**.
 	upload_code {
-		let c in 0 .. Perbill::from_percent(50).mul_ceil(T::Schedule::get().limits.code_len);
+		let c in 0 .. Perbill::from_percent(49).mul_ceil(T::MaxCodeLen::get());
 		let caller = whitelisted_caller();
 		T::Currency::make_free_balance_be(&caller, caller_funding::<T>());
 		let WasmModule { code, hash, .. } = WasmModule::<T>::sized(c, Location::Call);

frame/contracts/src/lib.rs

@@ -114,8 +114,9 @@ use codec::{Encode, HasCompact};
 use frame_support::{
 	dispatch::Dispatchable,
 	ensure,
-	traits::{Contains, Currency, Get, Randomness, ReservableCurrency, Time},
+	traits::{ConstU32, Contains, Currency, Get, Randomness, ReservableCurrency, Time},
 	weights::{DispatchClass, GetDispatchInfo, Pays, PostDispatchInfo, Weight},
+	BoundedVec,
 };
 use frame_system::{limits::BlockWeights, Pallet as System};
 use pallet_contracts_primitives::{
@@ -129,9 +130,11 @@ use sp_runtime::traits::{Convert, Hash, Saturating, StaticLookup};
 use sp_std::{fmt::Debug, marker::PhantomData, prelude::*};
 
 type CodeHash<T> = <T as frame_system::Config>::Hash;
-type TrieId = Vec<u8>;
+type TrieId = BoundedVec<u8, ConstU32<128>>;
 type BalanceOf<T> =
 	<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
+type CodeVec<T> = BoundedVec<u8, <T as Config>::MaxCodeLen>;
+type RelaxedCodeVec<T> = BoundedVec<u8, <T as Config>::RelaxedMaxCodeLen>;
 
 /// Used as a sentinel value when reading and writing contract memory.
 ///
@@ -224,7 +227,6 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
-	#[pallet::without_storage_info]
 	pub struct Pallet<T>(PhantomData<T>);
 
 	#[pallet::config]
@@ -356,6 +358,20 @@ pub mod pallet {
 
 		/// The address generator used to generate the addresses of contracts.
 		type AddressGenerator: AddressGenerator<Self>;
+
+		/// The maximum length of a contract code in bytes. This limit applies to the instrumented
+		/// version of the code. Therefore `instantiate_with_code` can fail even when supplying
+		/// a wasm binary below this maximum size.
+		type MaxCodeLen: Get<u32>;
+
+		/// The maximum length of a contract code after reinstrumentation.
+		///
+		/// When uploading a new contract the size defined by [`Self::MaxCodeLen`] is used for both
+		/// the pristine **and** the instrumented version. When a existing contract needs to be
+		/// reinstrumented after a runtime upgrade we apply this bound. The reason is that if the
+		/// new instrumentation increases the size beyond the limit it would make that contract
+		/// inaccessible until rectified by another runtime upgrade.
+		type RelaxedMaxCodeLen: Get<u32>;
 	}
 
 	#[pallet::hooks]
@@ -698,7 +714,7 @@ pub mod pallet {
 
 	/// A mapping from an original code hash to the original code, untouched by instrumentation.
 	#[pallet::storage]
-	pub(crate) type PristineCode<T: Config> = StorageMap<_, Identity, CodeHash<T>, Vec<u8>>;
+	pub(crate) type PristineCode<T: Config> = StorageMap<_, Identity, CodeHash<T>, CodeVec<T>>;
 
 	/// A mapping between an original code hash and instrumented wasm code, ready for execution.
 	#[pallet::storage]
@@ -746,7 +762,8 @@ pub mod pallet {
 	/// Child trie deletion is a heavy operation depending on the amount of storage items
 	/// stored in said trie. Therefore this operation is performed lazily in `on_initialize`.
 	#[pallet::storage]
-	pub(crate) type DeletionQueue<T: Config> = StorageValue<_, Vec<DeletedContract>, ValueQuery>;
+	pub(crate) type DeletionQueue<T: Config> =
+		StorageValue<_, BoundedVec<DeletedContract, T::DeletionQueueDepth>, ValueQuery>;
 }
 
 /// Return type of the private [`Pallet::internal_call`] function.
@@ -864,8 +881,8 @@ where
 		storage_deposit_limit: Option<BalanceOf<T>>,
 	) -> CodeUploadResult<CodeHash<T>, BalanceOf<T>> {
 		let schedule = T::Schedule::get();
-		let module = PrefabWasmModule::from_code(code, &schedule, origin)
-			.map_err(|_| <Error<T>>::CodeRejected)?;
+		let module =
+			PrefabWasmModule::from_code(code, &schedule, origin).map_err(|(err, _)| err)?;
 		let deposit = module.open_deposit();
 		if let Some(storage_deposit_limit) = storage_deposit_limit {
 			ensure!(storage_deposit_limit >= deposit, <Error<T>>::StorageDepositLimitExhausted);
@@ -971,19 +988,11 @@ where
 			let schedule = T::Schedule::get();
 			let (extra_deposit, executable) = match code {
 				Code::Upload(Bytes(binary)) => {
-					ensure!(
-						binary.len() as u32 <= schedule.limits.code_len,
-						<Error<T>>::CodeTooLarge
-					);
 					let executable = PrefabWasmModule::from_code(binary, &schedule, origin.clone())
-						.map_err(|msg| {
+						.map_err(|(err, msg)| {
 							debug_message.as_mut().map(|buffer| buffer.extend(msg.as_bytes()));
-							<Error<T>>::CodeRejected
+							err
 						})?;
-					ensure!(
-						executable.code_len() <= schedule.limits.code_len,
-						<Error<T>>::CodeTooLarge
-					);
 					// The open deposit will be charged during execution when the
 					// uploaded module does not already exist. This deposit is not part of the
 					// storage meter because it is not transfered to the contract but

frame/contracts/src/schedule.rs

@@ -33,7 +33,7 @@ use wasm_instrument::{gas_metering, parity_wasm::elements};
 /// How many API calls are executed in a single batch. The reason for increasing the amount
 /// of API calls in batches (per benchmark component increase) is so that the linear regression
 /// has an easier time determining the contribution of that component.
-pub const API_BENCHMARK_BATCH_SIZE: u32 = 100;
+pub const API_BENCHMARK_BATCH_SIZE: u32 = 80;
 
 /// How many instructions are executed in a single batch. The reasoning is the same
 /// as for `API_BENCHMARK_BATCH_SIZE`.
@@ -147,11 +147,6 @@ pub struct Limits {
 
 	/// The maximum size of a storage value and event payload in bytes.
 	pub payload_len: u32,
-
-	/// The maximum length of a contract code in bytes. This limit applies to the instrumented
-	/// version of the code. Therefore `instantiate_with_code` can fail even when supplying
-	/// a wasm binary below this maximum size.
-	pub code_len: u32,
 }
 
 impl Limits {
@@ -522,7 +517,6 @@ impl Default for Limits {
 			subject_len: 32,
 			call_depth: 32,
 			payload_len: 16 * 1024,
-			code_len: 128 * 1024,
 		}
 	}
 }

frame/contracts/src/storage.rs

@@ -24,11 +24,10 @@ use crate::{
 	weights::WeightInfo,
 	BalanceOf, CodeHash, Config, ContractInfoOf, DeletionQueue, Error, TrieId, SENTINEL,
 };
-use codec::{Decode, Encode};
+use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::{
 	dispatch::{DispatchError, DispatchResult},
 	storage::child::{self, ChildInfo, KillStorageResult},
-	traits::Get,
 	weights::Weight,
 };
 use scale_info::TypeInfo;
@@ -44,7 +43,7 @@ pub type ContractInfo<T> = RawContractInfo<CodeHash<T>, BalanceOf<T>>;
 
 /// Information for managing an account and its sub trie abstraction.
 /// This is the required info to cache for an account.
-#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo)]
+#[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug, TypeInfo, MaxEncodedLen)]
 pub struct RawContractInfo<CodeHash, Balance> {
 	/// Unique ID for the subtree encoded as a bytes vector.
 	pub trie_id: TrieId,
@@ -67,7 +66,7 @@ fn child_trie_info(trie_id: &[u8]) -> ChildInfo {
 	ChildInfo::new_default(trie_id)
 }
 
-#[derive(Encode, Decode, TypeInfo)]
+#[derive(Encode, Decode, TypeInfo, MaxEncodedLen)]
 pub struct DeletedContract {
 	pub(crate) trie_id: TrieId,
 }
@@ -217,12 +216,8 @@ where
 	///
 	/// You must make sure that the contract is also removed when queuing the trie for deletion.
 	pub fn queue_trie_for_deletion(contract: &ContractInfo<T>) -> DispatchResult {
-		if <DeletionQueue<T>>::decode_len().unwrap_or(0) >= T::DeletionQueueDepth::get() as usize {
-			Err(Error::<T>::DeletionQueueFull.into())
-		} else {
-			<DeletionQueue<T>>::append(DeletedContract { trie_id: contract.trie_id.clone() });
-			Ok(())
-		}
+		<DeletionQueue<T>>::try_append(DeletedContract { trie_id: contract.trie_id.clone() })
+			.map_err(|_| <Error<T>>::DeletionQueueFull.into())
 	}
 
 	/// Calculates the weight that is necessary to remove one key from the trie and how many
@@ -293,7 +288,11 @@ where
 	/// Generates a unique trie id by returning  `hash(account_id ++ nonce)`.
 	pub fn generate_trie_id(account_id: &AccountIdOf<T>, nonce: u64) -> TrieId {
 		let buf: Vec<_> = account_id.as_ref().iter().chain(&nonce.to_le_bytes()).cloned().collect();
-		T::Hashing::hash(&buf).as_ref().into()
+		T::Hashing::hash(&buf)
+			.as_ref()
+			.to_vec()
+			.try_into()
+			.expect("Runtime uses a reasonable hash size. Hence sizeof(T::Hash) <= 128; qed")
 	}
 
 	/// Returns the code hash of the contract specified by `account` ID.
@@ -305,9 +304,11 @@ where
 	/// Fill up the queue in order to exercise the limits during testing.
 	#[cfg(test)]
 	pub fn fill_queue_with_dummies() {
-		let queue: Vec<_> = (0..T::DeletionQueueDepth::get())
-			.map(|_| DeletedContract { trie_id: vec![] })
+		use frame_support::{traits::Get, BoundedVec};
+		let queue: Vec<DeletedContract> = (0..T::DeletionQueueDepth::get())
+			.map(|_| DeletedContract { trie_id: TrieId::default() })
 			.collect();
-		<DeletionQueue<T>>::put(queue);
+		let bounded: BoundedVec<_, _> = queue.try_into().unwrap();
+		<DeletionQueue<T>>::put(bounded);
 	}
 }

frame/contracts/src/tests.rs

@@ -288,6 +288,8 @@ impl Config for Test {
 	type DepositPerItem = DepositPerItem;
 	type AddressGenerator = DefaultAddressGenerator;
 	type ContractAccessWeight = DefaultContractAccessWeight<BlockWeights>;
+	type MaxCodeLen = ConstU32<{ 128 * 1024 }>;
+	type RelaxedMaxCodeLen = ConstU32<{ 256 * 1024 }>;
 }
 
 pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]);

frame/contracts/src/wasm/code_cache.rs

@@ -163,7 +163,8 @@ pub fn load<T: Config>(
 where
 	T::AccountId: UncheckedFrom<T::Hash> + AsRef<[u8]>,
 {
-	let charged = gas_meter.charge(CodeToken::Load(schedule.limits.code_len))?;
+	let max_code_len = T::MaxCodeLen::get();
+	let charged = gas_meter.charge(CodeToken::Load(max_code_len))?;
 
 	let mut prefab_module = <CodeStorage<T>>::get(code_hash).ok_or(Error::<T>::CodeNotFound)?;
 	gas_meter.adjust_gas(charged, CodeToken::Load(prefab_module.code.len() as u32));
@@ -172,7 +173,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.
-		let charged = gas_meter.charge(CodeToken::Reinstrument(schedule.limits.code_len))?;
+		let charged = gas_meter.charge(CodeToken::Reinstrument(max_code_len))?;
 		let code_size = reinstrument(&mut prefab_module, schedule)?;
 		gas_meter.adjust_gas(charged, CodeToken::Reinstrument(code_size));
 	}
@@ -190,7 +191,10 @@ pub fn reinstrument<T: Config>(
 	let original_code =
 		<PristineCode<T>>::get(&prefab_module.code_hash).ok_or(Error::<T>::CodeNotFound)?;
 	let original_code_len = original_code.len();
-	prefab_module.code = prepare::reinstrument_contract::<T>(original_code, schedule)?;
+	prefab_module.code = prepare::reinstrument_contract::<T>(&original_code, schedule)
+		.map_err(|_| <Error<T>>::CodeRejected)?
+		.try_into()
+		.map_err(|_| <Error<T>>::CodeTooLarge)?;
 	prefab_module.instruction_weights_version = schedule.instruction_weights.version;
 	<CodeStorage<T>>::insert(&prefab_module.code_hash, &*prefab_module);
 	Ok(original_code_len as u32)

frame/contracts/src/wasm/mod.rs

@@ -31,10 +31,15 @@ use crate::{
 	exec::{ExecResult, Executable, ExportedFunction, Ext},
 	gas::GasMeter,
 	wasm::env_def::FunctionImplProvider,
-	AccountIdOf, BalanceOf, CodeHash, CodeStorage, Config, Schedule,
+	AccountIdOf, BalanceOf, CodeHash, CodeStorage, CodeVec, Config, Error, RelaxedCodeVec,
+	Schedule,
 };
 use codec::{Decode, Encode, MaxEncodedLen};
-use frame_support::dispatch::{DispatchError, DispatchResult};
+use frame_support::{
+	dispatch::{DispatchError, DispatchResult},
+	ensure,
+	traits::Get,
+};
 use sp_core::crypto::UncheckedFrom;
 use sp_sandbox::{SandboxEnvironmentBuilder, SandboxInstance, SandboxMemory};
 use sp_std::prelude::*;
@@ -50,7 +55,8 @@ pub use tests::MockExt;
 /// `instruction_weights_version` and `code` change when a contract with an outdated instrumentation
 /// is called. Therefore one must be careful when holding any in-memory representation of this
 /// type while calling into a contract as those fields can get out of date.
-#[derive(Clone, Encode, Decode, scale_info::TypeInfo)]
+#[derive(Clone, Encode, Decode, scale_info::TypeInfo, MaxEncodedLen)]
+#[codec(mel_bound())]
 #[scale_info(skip_type_params(T))]
 pub struct PrefabWasmModule<T: Config> {
 	/// Version of the instruction weights with which the code was instrumented.
@@ -63,14 +69,14 @@ pub struct PrefabWasmModule<T: Config> {
 	#[codec(compact)]
 	maximum: u32,
 	/// Code instrumented with the latest schedule.
-	code: Vec<u8>,
+	code: RelaxedCodeVec<T>,
 	/// The uninstrumented, pristine version of the code.
 	///
 	/// It is not stored because the pristine code has its own storage item. The value
 	/// is only `Some` when this module was created from an `original_code` and `None` if
 	/// it was loaded from storage.
 	#[codec(skip)]
-	original_code: Option<Vec<u8>>,
+	original_code: Option<CodeVec<T>>,
 	/// The code hash of the stored code which is defined as the hash over the `original_code`.
 	///
 	/// As the map key there is no need to store the hash in the value, too. It is set manually
@@ -122,8 +128,19 @@ where
 		original_code: Vec<u8>,
 		schedule: &Schedule<T>,
 		owner: AccountIdOf<T>,
-	) -> Result<Self, &'static str> {
-		prepare::prepare_contract(original_code, schedule, owner)
+	) -> Result<Self, (DispatchError, &'static str)> {
+		let module = prepare::prepare_contract(
+			original_code.try_into().map_err(|_| (<Error<T>>::CodeTooLarge.into(), ""))?,
+			schedule,
+			owner,
+		)?;
+		// When instrumenting a new code we apply a stricter limit than enforced by the
+		// `RelaxedCodeVec` in order to leave some headroom for reinstrumentation.
+		ensure!(
+			module.code.len() as u32 <= T::MaxCodeLen::get(),
+			(<Error<T>>::CodeTooLarge.into(), ""),
+		);
+		Ok(module)
 	}
 
 	/// Store the code without instantiating it.