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

* Add test to verify unique trie ids

* Rename trie_seed to nonce

* Rename AccountCounter -> Nonce

* fmt

substrate/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);
 		});
 	}
 

substrate/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.
 	///

substrate/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)
+	}
+}

substrate/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()
 	}
 

substrate/frame/contracts/src/tests.rs

@@ -74,17 +74,15 @@ frame_support::construct_runtime!(
 #[macro_use]
 pub mod test_utils {
 	use super::{Balances, Test};
-	use crate::{
-		exec::AccountIdOf, storage::Storage, AccountCounter, CodeHash, Config, ContractInfoOf,
-	};
+	use crate::{exec::AccountIdOf, storage::Storage, CodeHash, Config, ContractInfoOf, Nonce};
 	use frame_support::traits::Currency;
 
 	pub fn place_contract(address: &AccountIdOf<Test>, code_hash: CodeHash<Test>) {
-		let seed = <AccountCounter<Test>>::mutate(|counter| {
+		let nonce = <Nonce<Test>>::mutate(|counter| {
 			*counter += 1;
 			*counter
 		});
-		let trie_id = Storage::<Test>::generate_trie_id(address, seed);
+		let trie_id = Storage::<Test>::generate_trie_id(address, nonce);
 		set_balance(address, <Test as Config>::Currency::minimum_balance() * 10);
 		let contract = Storage::<Test>::new_contract(&address, trie_id, code_hash).unwrap();
 		<ContractInfoOf<Test>>::insert(address, contract);
@@ -349,6 +347,11 @@ where
 	Ok((wasm_binary, code_hash))
 }
 
+fn initialize_block(number: u64) {
+	System::reset_events();
+	System::initialize(&number, &[0u8; 32].into(), &Default::default());
+}
+
 // Perform a call to a plain account.
 // The actual transfer fails because we can only call contracts.
 // Then we check that at least the base costs where charged (no runtime gas costs.)
@@ -540,9 +543,67 @@ fn run_out_of_gas() {
 	});
 }
 
-fn initialize_block(number: u64) {
-	System::reset_events();
-	System::initialize(&number, &[0u8; 32].into(), &Default::default());
+/// Check that contracts with the same account id have different trie ids.
+/// Check the `Nonce` storage item for more information.
+#[test]
+fn instantiate_unique_trie_id() {
+	let (wasm, code_hash) = compile_module::<Test>("self_destruct").unwrap();
+
+	ExtBuilder::default().existential_deposit(500).build().execute_with(|| {
+		let _ = Balances::deposit_creating(&ALICE, 1_000_000);
+		Contracts::upload_code(Origin::signed(ALICE), wasm, None).unwrap();
+		let addr = Contracts::contract_address(&ALICE, &code_hash, &[]);
+
+		// Instantiate the contract and store its trie id for later comparison.
+		assert_ok!(Contracts::instantiate(
+			Origin::signed(ALICE),
+			0,
+			GAS_LIMIT,
+			None,
+			code_hash,
+			vec![],
+			vec![],
+		));
+		let trie_id = ContractInfoOf::<Test>::get(&addr).unwrap().trie_id;
+
+		// Try to instantiate it again without termination should yield an error.
+		assert_err_ignore_postinfo!(
+			Contracts::instantiate(
+				Origin::signed(ALICE),
+				0,
+				GAS_LIMIT,
+				None,
+				code_hash,
+				vec![],
+				vec![],
+			),
+			<Error<Test>>::DuplicateContract,
+		);
+
+		// Terminate the contract.
+		assert_ok!(Contracts::call(
+			Origin::signed(ALICE),
+			addr.clone(),
+			0,
+			GAS_LIMIT,
+			None,
+			vec![]
+		));
+
+		// Re-Instantiate after termination.
+		assert_ok!(Contracts::instantiate(
+			Origin::signed(ALICE),
+			0,
+			GAS_LIMIT,
+			None,
+			code_hash,
+			vec![],
+			vec![],
+		));
+
+		// Trie ids shouldn't match or we might have a collision
+		assert_ne!(trie_id, ContractInfoOf::<Test>::get(&addr).unwrap().trie_id);
+	});
 }
 
 #[test]

substrate/frame/contracts/src/weights.rs

@@ -201,7 +201,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 			.saturating_add(T::DbWeight::get().writes(2 as Weight))
 	}
 	// Storage: Contracts CodeStorage (r:1 w:1)
-	// Storage: Contracts AccountCounter (r:1 w:1)
+	// Storage: Contracts Nonce (r:1 w:1)
 	// Storage: Contracts ContractInfoOf (r:1 w:1)
 	// Storage: Timestamp Now (r:1 w:0)
 	// Storage: System Account (r:1 w:1)
@@ -217,7 +217,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 			.saturating_add(T::DbWeight::get().writes(6 as Weight))
 	}
 	// Storage: Contracts CodeStorage (r:1 w:1)
-	// Storage: Contracts AccountCounter (r:1 w:1)
+	// Storage: Contracts Nonce (r:1 w:1)
 	// Storage: Contracts ContractInfoOf (r:1 w:1)
 	// Storage: Timestamp Now (r:1 w:0)
 	// Storage: System Account (r:1 w:1)
@@ -651,7 +651,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	// Storage: Contracts ContractInfoOf (r:1 w:1)
 	// Storage: Contracts CodeStorage (r:1 w:0)
 	// Storage: Timestamp Now (r:1 w:0)
-	// Storage: Contracts AccountCounter (r:1 w:1)
+	// Storage: Contracts Nonce (r:1 w:1)
 	// Storage: Contracts OwnerInfoOf (r:100 w:100)
 	fn seal_instantiate(r: u32, ) -> Weight {
 		(0 as Weight)
@@ -666,7 +666,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	// Storage: Contracts ContractInfoOf (r:101 w:101)
 	// Storage: Contracts CodeStorage (r:2 w:1)
 	// Storage: Timestamp Now (r:1 w:0)
-	// Storage: Contracts AccountCounter (r:1 w:1)
+	// Storage: Contracts Nonce (r:1 w:1)
 	// Storage: Contracts OwnerInfoOf (r:1 w:1)
 	fn seal_instantiate_per_transfer_salt_kb(t: u32, s: u32, ) -> Weight {
 		(14_790_752_000 as Weight)
@@ -1092,7 +1092,7 @@ impl WeightInfo for () {
 			.saturating_add(RocksDbWeight::get().writes(2 as Weight))
 	}
 	// Storage: Contracts CodeStorage (r:1 w:1)
-	// Storage: Contracts AccountCounter (r:1 w:1)
+	// Storage: Contracts Nonce (r:1 w:1)
 	// Storage: Contracts ContractInfoOf (r:1 w:1)
 	// Storage: Timestamp Now (r:1 w:0)
 	// Storage: System Account (r:1 w:1)
@@ -1108,7 +1108,7 @@ impl WeightInfo for () {
 			.saturating_add(RocksDbWeight::get().writes(6 as Weight))
 	}
 	// Storage: Contracts CodeStorage (r:1 w:1)
-	// Storage: Contracts AccountCounter (r:1 w:1)
+	// Storage: Contracts Nonce (r:1 w:1)
 	// Storage: Contracts ContractInfoOf (r:1 w:1)
 	// Storage: Timestamp Now (r:1 w:0)
 	// Storage: System Account (r:1 w:1)
@@ -1542,7 +1542,7 @@ impl WeightInfo for () {
 	// Storage: Contracts ContractInfoOf (r:1 w:1)
 	// Storage: Contracts CodeStorage (r:1 w:0)
 	// Storage: Timestamp Now (r:1 w:0)
-	// Storage: Contracts AccountCounter (r:1 w:1)
+	// Storage: Contracts Nonce (r:1 w:1)
 	// Storage: Contracts OwnerInfoOf (r:100 w:100)
 	fn seal_instantiate(r: u32, ) -> Weight {
 		(0 as Weight)
@@ -1557,7 +1557,7 @@ impl WeightInfo for () {
 	// Storage: Contracts ContractInfoOf (r:101 w:101)
 	// Storage: Contracts CodeStorage (r:2 w:1)
 	// Storage: Timestamp Now (r:1 w:0)
-	// Storage: Contracts AccountCounter (r:1 w:1)
+	// Storage: Contracts Nonce (r:1 w:1)
 	// Storage: Contracts OwnerInfoOf (r:1 w:1)
 	fn seal_instantiate_per_transfer_salt_kb(t: u32, s: u32, ) -> Weight {
 		(14_790_752_000 as Weight)