contracts: Add test to verify unique trie ids (paritytech#10914)

* Add test to verify unique trie ids

* Rename trie_seed to nonce

* Rename AccountCounter -> Nonce

* fmt

frame/contracts/src/exec.rs

@@ -18,7 +18,7 @@
 use crate::{
 	gas::GasMeter,
 	storage::{self, Storage, WriteOutcome},
-	AccountCounter, BalanceOf, CodeHash, Config, ContractInfo, ContractInfoOf, Error, Event,
+	BalanceOf, CodeHash, Config, ContractInfo, ContractInfoOf, Error, Event, Nonce,
 	Pallet as Contracts, Schedule,
 };
 use frame_support::{
@@ -315,9 +315,10 @@ pub struct Stack<'a, T: Config, E> {
 	timestamp: MomentOf<T>,
 	/// The block number at the time of call stack instantiation.
 	block_number: T::BlockNumber,
-	/// The account counter is cached here when accessed. It is written back when the call stack
-	/// finishes executing.
-	account_counter: Option<u64>,
+	/// The nonce is cached here when accessed. It is written back when the call stack
+	/// finishes executing. Please refer to [`Nonce`] to a description of
+	/// the nonce itself.
+	nonce: Option<u64>,
 	/// The actual call stack. One entry per nested contract called/instantiated.
 	/// This does **not** include the [`Self::first_frame`].
 	frames: SmallVec<T::CallStack>,
@@ -385,8 +386,8 @@ enum FrameArgs<'a, T: Config, E> {
 	Instantiate {
 		/// The contract or signed origin which instantiates the new contract.
 		sender: T::AccountId,
-		/// The seed that should be used to derive a new trie id for the contract.
-		trie_seed: u64,
+		/// The nonce that should be used to derive a new trie id for the contract.
+		nonce: u64,
 		/// The executable whose `deploy` function is run.
 		executable: E,
 		/// A salt used in the contract address deriviation of the new contract.
@@ -571,7 +572,7 @@ where
 		let (mut stack, executable) = Self::new(
 			FrameArgs::Instantiate {
 				sender: origin.clone(),
-				trie_seed: Self::initial_trie_seed(),
+				nonce: Self::initial_nonce(),
 				executable,
 				salt,
 			},
@@ -596,7 +597,7 @@ where
 		value: BalanceOf<T>,
 		debug_message: Option<&'a mut Vec<u8>>,
 	) -> Result<(Self, E), ExecError> {
-		let (first_frame, executable, account_counter) =
+		let (first_frame, executable, nonce) =
 			Self::new_frame(args, value, gas_meter, storage_meter, 0, &schedule)?;
 		let stack = Self {
 			origin,
@@ -605,7 +606,7 @@ where
 			storage_meter,
 			timestamp: T::Time::now(),
 			block_number: <frame_system::Pallet<T>>::block_number(),
-			account_counter,
+			nonce,
 			first_frame,
 			frames: Default::default(),
 			debug_message,
@@ -627,7 +628,7 @@ where
 		gas_limit: Weight,
 		schedule: &Schedule<T>,
 	) -> Result<(Frame<T>, E, Option<u64>), ExecError> {
-		let (account_id, contract_info, executable, delegate_caller, entry_point, account_counter) =
+		let (account_id, contract_info, executable, delegate_caller, entry_point, nonce) =
 			match frame_args {
 				FrameArgs::Call { dest, cached_info, delegated_call } => {
 					let contract = if let Some(contract) = cached_info {
@@ -645,10 +646,10 @@ where
 
 					(dest, contract, executable, delegate_caller, ExportedFunction::Call, None)
 				},
-				FrameArgs::Instantiate { sender, trie_seed, executable, salt } => {
+				FrameArgs::Instantiate { sender, nonce, executable, salt } => {
 					let account_id =
 						<Contracts<T>>::contract_address(&sender, executable.code_hash(), &salt);
-					let trie_id = Storage::<T>::generate_trie_id(&account_id, trie_seed);
+					let trie_id = Storage::<T>::generate_trie_id(&account_id, nonce);
 					let contract = Storage::<T>::new_contract(
 						&account_id,
 						trie_id,
@@ -660,7 +661,7 @@ where
 						executable,
 						None,
 						ExportedFunction::Constructor,
-						Some(trie_seed),
+						Some(nonce),
 					)
 				},
 			};
@@ -676,7 +677,7 @@ where
 			allows_reentry: true,
 		};
 
-		Ok((frame, executable, account_counter))
+		Ok((frame, executable, nonce))
 	}
 
 	/// Create a subsequent nested frame.
@@ -782,9 +783,9 @@ where
 	/// This is called after running the current frame. It commits cached values to storage
 	/// and invalidates all stale references to it that might exist further down the call stack.
 	fn pop_frame(&mut self, persist: bool) {
-		// Revert the account counter in case of a failed instantiation.
+		// Revert changes to the nonce in case of a failed instantiation.
 		if !persist && self.top_frame().entry_point == ExportedFunction::Constructor {
-			self.account_counter.as_mut().map(|c| *c = c.wrapping_sub(1));
+			self.nonce.as_mut().map(|c| *c = c.wrapping_sub(1));
 		}
 
 		// Pop the current frame from the stack and return it in case it needs to interact
@@ -861,8 +862,8 @@ where
 			if let Some(contract) = contract {
 				<ContractInfoOf<T>>::insert(&self.first_frame.account_id, contract);
 			}
-			if let Some(counter) = self.account_counter {
-				<AccountCounter<T>>::set(counter);
+			if let Some(nonce) = self.nonce {
+				<Nonce<T>>::set(nonce);
 			}
 		}
 	}
@@ -920,20 +921,20 @@ where
 		!self.frames().any(|f| &f.account_id == id && !f.allows_reentry)
 	}
 
-	/// Increments the cached account id and returns the value to be used for the trie_id.
-	fn next_trie_seed(&mut self) -> u64 {
-		let next = if let Some(current) = self.account_counter {
+	/// Increments and returns the next nonce. Pulls it from storage if it isn't in cache.
+	fn next_nonce(&mut self) -> u64 {
+		let next = if let Some(current) = self.nonce {
 			current.wrapping_add(1)
 		} else {
-			Self::initial_trie_seed()
+			Self::initial_nonce()
 		};
-		self.account_counter = Some(next);
+		self.nonce = Some(next);
 		next
 	}
 
-	/// The account seed to be used to instantiate the account counter cache.
-	fn initial_trie_seed() -> u64 {
-		<AccountCounter<T>>::get().wrapping_add(1)
+	/// Pull the current nonce from storage.
+	fn initial_nonce() -> u64 {
+		<Nonce<T>>::get().wrapping_add(1)
 	}
 }
 
@@ -1020,11 +1021,11 @@ where
 		salt: &[u8],
 	) -> Result<(AccountIdOf<T>, ExecReturnValue), ExecError> {
 		let executable = E::from_storage(code_hash, &self.schedule, self.gas_meter())?;
-		let trie_seed = self.next_trie_seed();
+		let nonce = self.next_nonce();
 		let executable = self.push_frame(
 			FrameArgs::Instantiate {
 				sender: self.top_frame().account_id.clone(),
-				trie_seed,
+				nonce,
 				executable,
 				salt,
 			},
@@ -2445,7 +2446,7 @@ mod tests {
 	}
 
 	#[test]
-	fn account_counter() {
+	fn nonce() {
 		let fail_code = MockLoader::insert(Constructor, |_, _| exec_trapped());
 		let success_code = MockLoader::insert(Constructor, |_, _| exec_success());
 		let succ_fail_code = MockLoader::insert(Constructor, move |ctx, _| {
@@ -2495,7 +2496,7 @@ mod tests {
 				None,
 			)
 			.ok();
-			assert_eq!(<AccountCounter<Test>>::get(), 0);
+			assert_eq!(<Nonce<Test>>::get(), 0);
 
 			assert_ok!(MockStack::run_instantiate(
 				ALICE,
@@ -2508,7 +2509,7 @@ mod tests {
 				&[],
 				None,
 			));
-			assert_eq!(<AccountCounter<Test>>::get(), 1);
+			assert_eq!(<Nonce<Test>>::get(), 1);
 
 			assert_ok!(MockStack::run_instantiate(
 				ALICE,
@@ -2521,7 +2522,7 @@ mod tests {
 				&[],
 				None,
 			));
-			assert_eq!(<AccountCounter<Test>>::get(), 2);
+			assert_eq!(<Nonce<Test>>::get(), 2);
 
 			assert_ok!(MockStack::run_instantiate(
 				ALICE,
@@ -2534,7 +2535,7 @@ mod tests {
 				&[],
 				None,
 			));
-			assert_eq!(<AccountCounter<Test>>::get(), 4);
+			assert_eq!(<Nonce<Test>>::get(), 4);
 		});
 	}
 

frame/contracts/src/lib.rs

@@ -134,7 +134,7 @@ type BalanceOf<T> =
 	<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
 
 /// The current storage version.
-const STORAGE_VERSION: StorageVersion = StorageVersion::new(6);
+const STORAGE_VERSION: StorageVersion = StorageVersion::new(7);
 
 /// Used as a sentinel value when reading and writing contract memory.
 ///
@@ -665,9 +665,30 @@ pub mod pallet {
 	#[pallet::storage]
 	pub(crate) type OwnerInfoOf<T: Config> = StorageMap<_, Identity, CodeHash<T>, OwnerInfo<T>>;
 
-	/// The subtrie counter.
+	/// This is a **monotonic** counter incremented on contract instantiation.
+	///
+	/// This is used in order to generate unique trie ids for contracts.
+	/// The trie id of a new contract is calculated from hash(account_id, nonce).
+	/// The nonce is required because otherwise the following sequence would lead to
+	/// a possible collision of storage:
+	///
+	/// 1. Create a new contract.
+	/// 2. Terminate the contract.
+	/// 3. Immediately recreate the contract with the same account_id.
+	///
+	/// This is bad because the contents of a trie are deleted lazily and there might be
+	/// storage of the old instantiation still in it when the new contract is created. Please
+	/// note that we can't replace the counter by the block number because the sequence above
+	/// can happen in the same block. We also can't keep the account counter in memory only
+	/// because storage is the only way to communicate across different extrinsics in the
+	/// same block.
+	///
+	/// # Note
+	///
+	/// Do not use it to determine the number of contracts. It won't be decremented if
+	/// a contract is destroyed.
 	#[pallet::storage]
-	pub(crate) type AccountCounter<T: Config> = StorageValue<_, u64, ValueQuery>;
+	pub(crate) type Nonce<T: Config> = StorageValue<_, u64, ValueQuery>;
 
 	/// The code associated with a given account.
 	///

frame/contracts/src/migration.rs

@@ -19,6 +19,7 @@ use crate::{BalanceOf, CodeHash, Config, Pallet, TrieId, Weight};
 use codec::{Decode, Encode};
 use frame_support::{
 	codec, generate_storage_alias,
+	pallet_prelude::*,
 	storage::migration,
 	traits::{Get, PalletInfoAccess},
 	Identity, Twox64Concat,
@@ -47,6 +48,11 @@ pub fn migrate<T: Config>() -> Weight {
 		StorageVersion::new(6).put::<Pallet<T>>();
 	}
 
+	if version < 7 {
+		weight = weight.saturating_add(v7::migrate::<T>());
+		StorageVersion::new(7).put::<Pallet<T>>();
+	}
+
 	weight
 }
 
@@ -249,3 +255,21 @@ mod v6 {
 		weight
 	}
 }
+
+/// Rename `AccountCounter` to `Nonce`.
+mod v7 {
+	use super::*;
+
+	pub fn migrate<T: Config>() -> Weight {
+		generate_storage_alias!(
+			Contracts,
+			AccountCounter => Value<u64, ValueQuery>
+		);
+		generate_storage_alias!(
+			Contracts,
+			Nonce => Value<u64, ValueQuery>
+		);
+		Nonce::set(AccountCounter::take());
+		T::DbWeight::get().reads_writes(1, 2)
+	}
+}

frame/contracts/src/storage.rs

@@ -290,10 +290,9 @@ where
 		weight_limit.saturating_sub(weight_per_key.saturating_mul(remaining_key_budget as Weight))
 	}
 
-	/// This generator uses inner counter for account id and applies the hash over `AccountId +
-	/// accountid_counter`.
-	pub fn generate_trie_id(account_id: &AccountIdOf<T>, seed: u64) -> TrieId {
-		let buf: Vec<_> = account_id.as_ref().iter().chain(&seed.to_le_bytes()).cloned().collect();
+	/// 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()
 	}