wip

frame/contracts/src/benchmarking/mod.rs

@@ -219,6 +219,7 @@ benchmarks! {
 		ContractInfo::<T>::process_deletion_queue_batch(Weight::MAX)
 	}
 
+	// TODO simplify this
 	#[pov_mode = Measured]
 	on_initialize_per_queue_item {
 		let q in 0..1024.min(T::DeletionQueueDepth::get());

frame/contracts/src/lib.rs

@@ -906,76 +906,94 @@ 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, u64, DeletedContract>;
 
+	/// A pair of monotonic counters used to index the latest contract marked for deletion
+	/// and the latest deleted contract
 	#[pallet::storage]
 	pub(crate) type DeletionQueueNonce<T: Config> =
 		StorageValue<_, DeletionQueueNonces, ValueQuery>;
 }
 
 #[derive(Encode, Decode, TypeInfo, MaxEncodedLen, Clone, 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: u64,
+	/// the index used to read the next element to be deleted
+	/// the nonce is incremented after each deletion
 	delete_nonce: u64,
 }
 
 impl DeletionQueueNonces {
-	fn delete_plus_one(&mut self) -> &mut Self {
-		self.delete_nonce.saturating_plus_one();
-		self
+	/// increment the delete nonce and return the new value
+	fn delete_inc(&mut self) -> Self {
+		self.delete_nonce = self.delete_nonce.wrapping_add(1);
+		self.clone()
 	}
 
-	fn insert_plus_one(&mut self) -> &mut Self {
-		self.insert_nonce.saturating_plus_one();
-		self
+	/// increment the insert nonce and return the new value
+	fn insert_inc(&mut self) -> Self {
+		self.insert_nonce = self.insert_nonce.wrapping_add(1);
+		self.clone()
 	}
 }
 
+/// Utility struct to manage the deletion queue
 struct DeletionQueue<T: Config> {
 	nonces: DeletionQueueNonces,
 	_phantom: PhantomData<T>,
 }
-//
+
+/// View on a contract that is marked for deletion
+/// The struct takes a mutable reference to the deletion queue so that the contract can be removed,
+/// and none can be added in the meantime.
 struct DeletionQueueEntry<'a, T: Config> {
 	contract: DeletedContract,
 	queue: &'a mut DeletionQueue<T>,
 }
 
 impl<'a, T: Config> DeletionQueueEntry<'a, T> {
-	fn get(&self) -> &DeletedContract {
+	/// 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_plus_one().clone());
+		<DeletionQueueNonce<T>>::set(self.queue.nonces.delete_inc());
 	}
 }
 
 impl<T: Config> DeletionQueue<T> {
+	/// Load the Deletion Queue nonces.
 	fn load() -> Self {
 		let nonces = <DeletionQueueNonce<T>>::get();
 		Self { nonces, _phantom: PhantomData }
 	}
 
+	/// The number of contracts marked for deletion.
 	fn len(&self) -> u64 {
-		self.nonces.insert_nonce - self.nonces.delete_nonce
-	}
-
-	fn is_empty(&self) -> bool {
-		self.nonces.insert_nonce == self.nonces.delete_nonce
+		self.nonces.insert_nonce.wrapping_sub(self.nonces.insert_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_plus_one().clone());
+		<DeletionQueueNonce<T>>::set(self.nonces.insert_inc());
 	}
 
-	fn next(&mut self) -> DeletionQueueEntry<T> {
-		let contract = <DeletionQueueMap<T>>::get(self.nonces.delete_nonce).unwrap(); // TODO
-		DeletionQueueEntry { contract, queue: self }
+	/// 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 })
 	}
 }
 

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

@@ -209,7 +209,6 @@ impl<T: Config> ContractInfo<T> {
 		Ok(())
 	}
 
-	/// TODO revisit this
 	/// 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.
@@ -246,33 +245,33 @@ impl<T: Config> ContractInfo<T> {
 		let (weight_per_key, mut remaining_key_budget) =
 			Self::deletion_budget(queue_len, weight_limit);
 
-		// TODO revisit this
 		// We want to check whether we have enough weight to decode the queue before
 		// proceeding. Too little weight for decoding might happen during runtime upgrades
 		// which consume the whole block before the other `on_initialize` blocks are called.
 		if remaining_key_budget == 0 {
 			return weight_limit
 		}
 
-		while !queue.is_empty() && remaining_key_budget > 0 {
-			let entry = queue.next();
+		while remaining_key_budget > 0 {
+			let Some(entry) = queue.next() else { break };
+
 			#[allow(deprecated)]
 			let outcome = child::kill_storage(
-				&ChildInfo::new_default(&entry.get().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) => {
-					// TODO revisit this ensure that we break, after this since
-					c
+				KillStorageResult::SomeRemaining(keys_removed) => {
+					remaining_key_budget = remaining_key_budget.saturating_sub(keys_removed);
+					break
 				},
-				KillStorageResult::AllRemoved(c) => {
+				KillStorageResult::AllRemoved(keys_removed) => {
 					entry.remove();
-					c
+					remaining_key_budget = remaining_key_budget.saturating_sub(keys_removed);
 				},
 			};
-			remaining_key_budget = remaining_key_budget.saturating_sub(keys_removed);
 		}
 
 		weight_limit.saturating_sub(weight_per_key.saturating_mul(u64::from(remaining_key_budget)))