[Contracts] Overflowing bounded DeletionQueue

bin/node/runtime/src/lib.rs

@@ -1198,7 +1198,6 @@ impl pallet_tips::Config for Runtime {
 parameter_types! {
 	pub const DepositPerItem: Balance = deposit(1, 0);
 	pub const DepositPerByte: Balance = deposit(0, 1);
-	pub const DeletionQueueDepth: u32 = 128;
 	// The lazy deletion runs inside on_initialize.
 	pub DeletionWeightLimit: Weight = RuntimeBlockWeights::get()
 		.per_class
@@ -1227,7 +1226,6 @@ impl pallet_contracts::Config for Runtime {
 	type WeightPrice = pallet_transaction_payment::Pallet<Self>;
 	type WeightInfo = pallet_contracts::weights::SubstrateWeight<Self>;
 	type ChainExtension = ();
-	type DeletionQueueDepth = DeletionQueueDepth;
 	type DeletionWeightLimit = DeletionWeightLimit;
 	type Schedule = Schedule;
 	type AddressGenerator = pallet_contracts::DefaultAddressGenerator;

frame/contracts/src/benchmarking/mod.rs

@@ -221,7 +221,7 @@ benchmarks! {
 
 	#[pov_mode = Measured]
 	on_initialize_per_queue_item {
-		let q in 0..1024.min(T::DeletionQueueDepth::get());
+		let q in 0..1024;
 		for i in 0 .. q {
 			let instance = Contract::<T>::with_index(i, WasmModule::dummy(), vec![])?;
 			instance.info()?.queue_trie_for_deletion()?;
@@ -3021,9 +3021,8 @@ benchmarks! {
 		#[cfg(feature = "std")]
 		{
 			let weight_limit = T::DeletionWeightLimit::get();
-			let max_queue_depth = T::DeletionQueueDepth::get() as usize;
 			let empty_queue_throughput = ContractInfo::<T>::deletion_budget(0, weight_limit);
-			let full_queue_throughput = ContractInfo::<T>::deletion_budget(max_queue_depth, weight_limit);
+			let full_queue_throughput = ContractInfo::<T>::deletion_budget(1024, weight_limit);
 			println!("{:#?}", Schedule::<T>::default());
 			println!("###############################################");
 			println!("Lazy deletion weight per key: {}", empty_queue_throughput.0);

frame/contracts/src/lib.rs

@@ -106,7 +106,7 @@ use crate::{
 	wasm::{OwnerInfo, PrefabWasmModule, TryInstantiate},
 	weights::WeightInfo,
 };
-use codec::{Codec, Encode, HasCompact};
+use codec::{Codec, Decode, Encode, HasCompact, MaxEncodedLen};
 use environmental::*;
 use frame_support::{
 	dispatch::{Dispatchable, GetDispatchInfo, Pays, PostDispatchInfo},
@@ -245,30 +245,11 @@ pub mod pallet {
 		/// memory usage of your runtime.
 		type CallStack: Array<Item = Frame<Self>>;
 
-		/// The maximum number of contracts that can be pending for deletion.
-		///
-		/// When a contract is deleted by calling `seal_terminate` it becomes inaccessible
-		/// immediately, but the deletion of the storage items it has accumulated is performed
-		/// later. The contract is put into the deletion queue. This defines how many
-		/// contracts can be queued up at the same time. If that limit is reached `seal_terminate`
-		/// will fail. The action must be retried in a later block in that case.
-		///
-		/// The reasons for limiting the queue depth are:
-		///
-		/// 1. The queue is in storage in order to be persistent between blocks. We want to limit
-		/// 	the amount of storage that can be consumed.
-		/// 2. The queue is stored in a vector and needs to be decoded as a whole when reading
-		///		it at the end of each block. Longer queues take more weight to decode and hence
-		///		limit the amount of items that can be deleted per block.
-		#[pallet::constant]
-		type DeletionQueueDepth: Get<u32>;
-
 		/// The maximum amount of weight that can be consumed per block for lazy trie removal.
 		///
 		/// The amount of weight that is dedicated per block to work on the deletion queue. Larger
 		/// values allow more trie keys to be deleted in each block but reduce the amount of
-		/// weight that is left for transactions. See [`Self::DeletionQueueDepth`] for more
-		/// information about the deletion queue.
+		/// weight that is left for transactions.
 		#[pallet::constant]
 		type DeletionWeightLimit: Get<Weight>;
 
@@ -329,25 +310,6 @@ pub mod pallet {
 				.saturating_add(T::WeightInfo::on_process_deletion_queue_batch())
 		}
 
-		fn on_initialize(_block: T::BlockNumber) -> Weight {
-			// We want to process the deletion_queue in the on_idle hook. Only in the case
-			// that the queue length has reached its maximal depth, we process it here.
-			let max_len = T::DeletionQueueDepth::get() as usize;
-			let queue_len = <DeletionQueue<T>>::decode_len().unwrap_or(0);
-			if queue_len >= max_len {
-				// We do not want to go above the block limit and rather avoid lazy deletion
-				// in that case. This should only happen on runtime upgrades.
-				let weight_limit = T::BlockWeights::get()
-					.max_block
-					.saturating_sub(System::<T>::block_weight().total())
-					.min(T::DeletionWeightLimit::get());
-				ContractInfo::<T>::process_deletion_queue_batch(weight_limit)
-					.saturating_add(T::WeightInfo::on_process_deletion_queue_batch())
-			} else {
-				T::WeightInfo::on_process_deletion_queue_batch()
-			}
-		}
-
 		fn integrity_test() {
 			// Total runtime memory is expected to have 128Mb upper limit
 			const MAX_RUNTIME_MEM: u32 = 1024 * 1024 * 128;
@@ -860,12 +822,6 @@ pub mod pallet {
 		/// in this error. Note that this usually  shouldn't happen as deploying such contracts
 		/// is rejected.
 		NoChainExtension,
-		/// Removal of a contract failed because the deletion queue is full.
-		///
-		/// This can happen when calling `seal_terminate`.
-		/// The queue is filled by deleting contracts and emptied by a fixed amount each block.
-		/// Trying again during another block is the only way to resolve this issue.
-		DeletionQueueFull,
 		/// A contract with the same AccountId already exists.
 		DuplicateContract,
 		/// A contract self destructed in its constructor.
@@ -949,10 +905,102 @@ pub mod pallet {
 	/// Evicted contracts that await child trie deletion.
 	///
 	/// 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`.
+	/// stored in said trie. Therefore this operation is performed lazily in `on_idle`.
+	#[pallet::storage]
+	pub(crate) type DeletionQueueMap<T: Config> = StorageMap<_, Twox64Concat, u32, DeletedContract>;
+
+	/// A pair of monotonic counters used to index the latest contract marked for deletion
+	/// and the latest deleted contract in DeletionQueueMap.
 	#[pallet::storage]
-	pub(crate) type DeletionQueue<T: Config> =
-		StorageValue<_, BoundedVec<DeletedContract, T::DeletionQueueDepth>, ValueQuery>;
+	pub(crate) type DeletionQueueNonce<T: Config> =
+		StorageValue<_, DeletionQueueNonces, ValueQuery>;
+}
+
+/// DeletionQueueNonces is a pair of monotonic counters used to index contracts marked for deletion,
+/// and the latest deleted contract.
+#[derive(Encode, Decode, TypeInfo, MaxEncodedLen, Clone, Copy, Default)]
+struct DeletionQueueNonces {
+	/// Monotonic counter used as a key for inserting new deleted contract in the DeletionQueueMap.
+	/// The nonce is incremented after each insertion.
+	insert_nonce: u32,
+	/// The index used to read the next element to be deleted in the DeletionQueueMap.
+	/// The nonce is incremented after each deletion.
+	delete_nonce: u32,
+}
+
+impl DeletionQueueNonces {
+	/// Increment the delete nonce and return the new value.
+	fn delete_inc(&mut self) -> &mut Self {
+		self.delete_nonce = self.delete_nonce.wrapping_add(1);
+		self
+	}
+
+	/// Increment the insert nonce and return the new value.
+	fn insert_inc(&mut self) -> &mut Self {
+		self.insert_nonce = self.insert_nonce.wrapping_add(1);
+		self
+	}
+}
+
+/// DeletionQueue manage the removal of contracts storage that are marked for deletion.
+///
+/// When a contract is deleted by calling `seal_terminate` it becomes inaccessible
+/// immediately, but the deletion of the storage items it has accumulated is performed
+/// later by pulling the contract from the deletion queue in the `on_idle` hook.
+struct DeletionQueue<T: Config> {
+	nonces: DeletionQueueNonces,
+	_phantom: PhantomData<T>,
+}
+
+/// View on a contract that is marked for deletion
+/// The struct takes a mutable reference on the deletion queue so that the contract can be removed,
+/// and none can be added or read in the meantime.
+struct DeletionQueueEntry<'a, T: Config> {
+	contract: DeletedContract,
+	queue: &'a mut DeletionQueue<T>,
+}
+
+impl<'a, T: Config> DeletionQueueEntry<'a, T> {
+	/// Get a reference to the contract that is marked for deletion.
+	fn contract(&self) -> &DeletedContract {
+		&self.contract
+	}
+
+	/// Remove the contract from the deletion queue.
+	fn remove(self) {
+		<DeletionQueueMap<T>>::remove(self.queue.nonces.delete_nonce);
+		<DeletionQueueNonce<T>>::set(*self.queue.nonces.delete_inc());
+	}
+}
+
+impl<T: Config> DeletionQueue<T> {
+	/// Load the Deletion Queue nonces, so we can perform read or write operations on the
+	/// DeletionQueueMap storage
+	fn load() -> Self {
+		let nonces = <DeletionQueueNonce<T>>::get();
+		Self { nonces, _phantom: PhantomData }
+	}
+
+	/// The number of contracts marked for deletion.
+	fn len(&self) -> u32 {
+		self.nonces.insert_nonce.wrapping_sub(self.nonces.delete_nonce)
+	}
+
+	/// Insert a contract in the deletion queue.
+	fn insert(&mut self, contract: DeletedContract) {
+		<DeletionQueueMap<T>>::insert(self.nonces.insert_nonce, contract);
+		<DeletionQueueNonce<T>>::set(*self.nonces.insert_inc());
+	}
+
+	/// Fetch the next contract to be deleted.
+	fn next(&mut self) -> Option<DeletionQueueEntry<T>> {
+		if self.len() == 0 {
+			return None
+		}
+
+		<DeletionQueueMap<T>>::get(self.nonces.delete_nonce)
+			.map(|contract| DeletionQueueEntry { contract, queue: self })
+	}
 }
 
 /// Context of a contract invocation.

frame/contracts/src/migration.rs

@@ -59,7 +59,11 @@ impl<T: Config> OnRuntimeUpgrade for Migration<T> {
 			v9::migrate::<T>(&mut weight);
 		}
 
-		StorageVersion::new(9).put::<Pallet<T>>();
+		if version < 10 {
+			v10::migrate::<T>(&mut weight);
+		}
+
+		StorageVersion::new(10).put::<Pallet<T>>();
 		weight.saturating_accrue(T::DbWeight::get().writes(1));
 
 		weight
@@ -400,6 +404,30 @@ mod v9 {
 	}
 }
 
+mod v10 {
+
+	use super::*;
+	use crate::storage::DeletedContract;
+
+	#[storage_alias]
+	type DeletionQueue<T: Config> = StorageValue<Pallet<T>, Vec<DeletedContract>>;
+
+	pub fn migrate<T: Config>(weight: &mut Weight) {
+		let Some(contracts) = DeletionQueue::<T>::take() else { return };
+		weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1));
+
+		let mut queue = crate::DeletionQueue::<T>::load();
+		weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 0));
+
+		let queue_len = contracts.len() as u64;
+		for contract in contracts {
+			queue.insert(contract);
+		}
+
+		weight.saturating_accrue(T::DbWeight::get().reads_writes(0, queue_len + 1));
+	}
+}
+
 // Post checks always need to be run against the latest storage version. This is why we
 // do not scope them in the per version modules. They always need to be ported to the latest
 // version.

frame/contracts/src/storage.rs

@@ -205,14 +205,14 @@ impl<T: Config> ContractInfo<T> {
 	///
 	/// You must make sure that the contract is also removed when queuing the trie for deletion.
 	pub fn queue_trie_for_deletion(&self) -> DispatchResult {
-		<DeletionQueue<T>>::try_append(DeletedContract { trie_id: self.trie_id.clone() })
-			.map_err(|_| <Error<T>>::DeletionQueueFull.into())
+		DeletionQueue::<T>::load().insert(DeletedContract { trie_id: self.trie_id.clone() });
+		Ok(())
 	}
 
 	/// Calculates the weight that is necessary to remove one key from the trie and how many
 	/// of those keys can be deleted from the deletion queue given the supplied queue length
 	/// and weight limit.
-	pub fn deletion_budget(queue_len: usize, weight_limit: Weight) -> (Weight, u32) {
+	pub fn deletion_budget(queue_len: u32, weight_limit: Weight) -> (Weight, u32) {
 		let base_weight = T::WeightInfo::on_process_deletion_queue_batch();
 		let weight_per_queue_item = T::WeightInfo::on_initialize_per_queue_item(1) -
 			T::WeightInfo::on_initialize_per_queue_item(0);
@@ -235,7 +235,9 @@ impl<T: Config> ContractInfo<T> {
 	///
 	/// It returns the amount of weight used for that task.
 	pub fn process_deletion_queue_batch(weight_limit: Weight) -> Weight {
-		let queue_len = <DeletionQueue<T>>::decode_len().unwrap_or(0);
+		let mut queue = <DeletionQueue<T>>::load();
+		let queue_len = queue.len();
+
 		if queue_len == 0 {
 			return Weight::zero()
 		}
@@ -250,48 +252,35 @@ impl<T: Config> ContractInfo<T> {
 			return weight_limit
 		}
 
-		let mut queue = <DeletionQueue<T>>::get();
+		while remaining_key_budget > 0 {
+			let Some(entry) = queue.next() else { break };
 
-		while !queue.is_empty() && remaining_key_budget > 0 {
-			// Cannot panic due to loop condition
-			let trie = &mut queue[0];
 			#[allow(deprecated)]
 			let outcome = child::kill_storage(
-				&ChildInfo::new_default(&trie.trie_id),
+				&ChildInfo::new_default(&entry.contract().trie_id),
 				Some(remaining_key_budget),
 			);
-			let keys_removed = match outcome {
+
+			match outcome {
 				// This happens when our budget wasn't large enough to remove all keys.
-				KillStorageResult::SomeRemaining(c) => c,
-				KillStorageResult::AllRemoved(c) => {
-					// We do not care to preserve order. The contract is deleted already and
-					// no one waits for the trie to be deleted.
-					queue.swap_remove(0);
-					c
+				KillStorageResult::SomeRemaining(keys_removed) => {
+					remaining_key_budget = remaining_key_budget.saturating_sub(keys_removed);
+					break
+				},
+				KillStorageResult::AllRemoved(keys_removed) => {
+					entry.remove();
+					remaining_key_budget = remaining_key_budget.saturating_sub(keys_removed);
 				},
 			};
-			remaining_key_budget = remaining_key_budget.saturating_sub(keys_removed);
 		}
 
-		<DeletionQueue<T>>::put(queue);
 		weight_limit.saturating_sub(weight_per_key.saturating_mul(u64::from(remaining_key_budget)))
 	}
 
 	/// Returns the code hash of the contract specified by `account` ID.
 	pub fn load_code_hash(account: &AccountIdOf<T>) -> Option<CodeHash<T>> {
 		<ContractInfoOf<T>>::get(account).map(|i| i.code_hash)
 	}
-
-	/// Fill up the queue in order to exercise the limits during testing.
-	#[cfg(test)]
-	pub fn fill_queue_with_dummies() {
-		use frame_support::{traits::Get, BoundedVec};
-		let queue: Vec<DeletedContract> = (0..T::DeletionQueueDepth::get())
-			.map(|_| DeletedContract { trie_id: TrieId::default() })
-			.collect();
-		let bounded: BoundedVec<_, _> = queue.try_into().map_err(|_| ()).unwrap();
-		<DeletionQueue<T>>::put(bounded);
-	}
 }
 
 #[derive(Encode, Decode, TypeInfo, MaxEncodedLen)]