Migration hook fixes (paritytech#14174)

* fix offences pre_upgrade hook

* identify source of ensure! failures

* stop migration hooks breaking post migration

* add childbounties storage version

* init child bounties version to zero

* Update frame/child-bounties/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

* remove redundant preupgrade version checks

* update test

* fix nom pools v3 migration

* kick ci

* kick ci

---------

Co-authored-by: Bastian Köcher <[email protected]>

Author: 2 people

frame/nomination-pools/src/migration.rs

@@ -416,14 +416,14 @@ pub mod v3 {
 			let current = Pallet::<T>::current_storage_version();
 			let onchain = Pallet::<T>::on_chain_storage_version();
 
-			log!(
-				info,
-				"Running migration with current storage version {:?} / onchain {:?}",
-				current,
-				onchain
-			);
+			if onchain == 2 {
+				log!(
+					info,
+					"Running migration with current storage version {:?} / onchain {:?}",
+					current,
+					onchain
+				);
 
-			if current > onchain {
 				let mut metadata_iterated = 0u64;
 				let mut metadata_removed = 0u64;
 				Metadata::<T>::iter_keys()
@@ -437,7 +437,7 @@ pub mod v3 {
 						metadata_removed += 1;
 						Metadata::<T>::remove(&id);
 					});
-				current.put::<Pallet<T>>();
+				StorageVersion::new(3).put::<Pallet<T>>();
 				// metadata iterated + bonded pools read + a storage version read
 				let total_reads = metadata_iterated * 2 + 1;
 				// metadata removed + a storage version write
@@ -451,10 +451,6 @@ pub mod v3 {
 
 		#[cfg(feature = "try-runtime")]
 		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
-			ensure!(
-				Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
-				"the on_chain version is equal or more than the current one"
-			);
 			Ok(Vec::new())
 		}
 
@@ -464,7 +460,10 @@ pub mod v3 {
 				Metadata::<T>::iter_keys().all(|id| BondedPools::<T>::contains_key(&id)),
 				"not all of the stale metadata has been removed"
 			);
-			ensure!(Pallet::<T>::on_chain_storage_version() == 3, "wrong storage version");
+			ensure!(
+				Pallet::<T>::on_chain_storage_version() >= 3,
+				"nomination-pools::migration::v3: wrong storage version"
+			);
 			Ok(())
 		}
 	}
@@ -551,28 +550,35 @@ pub mod v4 {
 
 		#[cfg(feature = "try-runtime")]
 		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
-			ensure!(
-				Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
-				"the on_chain version is equal or more than the current one"
-			);
 			Ok(Vec::new())
 		}
 
 		#[cfg(feature = "try-runtime")]
 		fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
 			// ensure all BondedPools items now contain an `inner.commission: Commission` field.
 			ensure!(
-				BondedPools::<T>::iter().all(|(_, inner)| inner.commission.current.is_none() &&
-					inner.commission.max.is_none() &&
-					inner.commission.change_rate.is_none() &&
-					inner.commission.throttle_from.is_none()),
-				"a commission value has been incorrectly set"
+				BondedPools::<T>::iter().all(|(_, inner)|
+					// Check current
+					(inner.commission.current.is_none() ||
+					inner.commission.current.is_some()) &&
+					// Check max
+					(inner.commission.max.is_none() || inner.commission.max.is_some()) &&
+					// Check change_rate
+					(inner.commission.change_rate.is_none() ||
+					inner.commission.change_rate.is_some()) &&
+					// Check throttle_from
+					(inner.commission.throttle_from.is_none() ||
+					inner.commission.throttle_from.is_some())),
+				"a commission value has not been set correctly"
 			);
 			ensure!(
 				GlobalMaxCommission::<T>::get() == Some(U::get()),
 				"global maximum commission error"
 			);
-			ensure!(Pallet::<T>::on_chain_storage_version() == 4, "wrong storage version");
+			ensure!(
+				Pallet::<T>::on_chain_storage_version() >= 4,
+				"nomination-pools::migration::v4: wrong storage version"
+			);
 			Ok(())
 		}
 	}
@@ -636,11 +642,6 @@ pub mod v5 {
 
 		#[cfg(feature = "try-runtime")]
 		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
-			ensure!(
-				Pallet::<T>::current_storage_version() > Pallet::<T>::on_chain_storage_version(),
-				"the on_chain version is equal or more than the current one"
-			);
-
 			let rpool_keys = RewardPools::<T>::iter_keys().count();
 			let rpool_values = RewardPools::<T>::iter_values().count();
 			if rpool_keys != rpool_values {
@@ -690,13 +691,16 @@ pub mod v5 {
 			// `total_commission_claimed` field.
 			ensure!(
 				RewardPools::<T>::iter().all(|(_, reward_pool)| reward_pool
-					.total_commission_pending
-					.is_zero() && reward_pool
-					.total_commission_claimed
-					.is_zero()),
+					.total_commission_pending >=
+					Zero::zero() && reward_pool
+					.total_commission_claimed >=
+					Zero::zero()),
 				"a commission value has been incorrectly set"
 			);
-			ensure!(Pallet::<T>::on_chain_storage_version() == 5, "wrong storage version");
+			ensure!(
+				Pallet::<T>::on_chain_storage_version() >= 5,
+				"nomination-pools::migration::v5: wrong storage version"
+			);
 
 			// These should not have been touched - just in case.
 			ensure!(

frame/offences/src/migration.rs

@@ -54,9 +54,6 @@ pub mod v1 {
 	impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
 		#[cfg(feature = "try-runtime")]
 		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
-			let onchain = Pallet::<T>::on_chain_storage_version();
-			ensure!(onchain < 1, "pallet_offences::MigrateToV1 migration can be deleted");
-
 			log::info!(
 				target: LOG_TARGET,
 				"Number of reports to refund and delete: {}",
@@ -67,19 +64,16 @@ pub mod v1 {
 		}
 
 		fn on_runtime_upgrade() -> Weight {
-			let onchain = Pallet::<T>::on_chain_storage_version();
-
-			if onchain > 0 {
+			if Pallet::<T>::on_chain_storage_version() > 0 {
 				log::info!(target: LOG_TARGET, "pallet_offences::MigrateToV1 should be removed");
 				return T::DbWeight::get().reads(1)
 			}
 
 			let keys_removed = v0::ReportsByKindIndex::<T>::clear(u32::MAX, None).unique as u64;
-			let weight = T::DbWeight::get().reads_writes(keys_removed, keys_removed);
-
 			StorageVersion::new(1).put::<Pallet<T>>();
 
-			weight
+			// + 1 for reading/writing the new storage version
+			T::DbWeight::get().reads_writes(keys_removed + 1, keys_removed + 1)
 		}
 
 		#[cfg(feature = "try-runtime")]
@@ -147,7 +141,7 @@ mod test {
 		ext.execute_with(|| {
 			assert_eq!(
 				v1::MigrateToV1::<T>::on_runtime_upgrade(),
-				<T as frame_system::Config>::DbWeight::get().reads_writes(1, 1),
+				<T as frame_system::Config>::DbWeight::get().reads_writes(2, 2),
 			);
 
 			assert!(<v0::ReportsByKindIndex<T>>::iter_values().count() == 0);