Fix migrations (#7340)

Okay this was stupid 🤦

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Author: ggwpez

runtime/parachains/src/configuration/migration/v5.rs

@@ -127,27 +127,35 @@ pub struct V5HostConfiguration<BlockNumber> {
 	pub minimum_validation_upgrade_delay: BlockNumber,
 }
 
-#[frame_support::storage_alias]
-pub(crate) type V4ActiveConfig<T: Config> =
-	StorageValue<Pallet<T>, V4HostConfiguration<BlockNumberFor<T>>, OptionQuery>;
-
-#[frame_support::storage_alias]
-pub(crate) type V4PendingConfigs<T: Config> = StorageValue<
-	Pallet<T>,
-	Vec<(SessionIndex, V4HostConfiguration<BlockNumberFor<T>>)>,
-	OptionQuery,
->;
-
-#[frame_support::storage_alias]
-pub(crate) type V5ActiveConfig<T: Config> =
-	StorageValue<Pallet<T>, V5HostConfiguration<BlockNumberFor<T>>, OptionQuery>;
-
-#[frame_support::storage_alias]
-pub(crate) type V5PendingConfigs<T: Config> = StorageValue<
-	Pallet<T>,
-	Vec<(SessionIndex, V5HostConfiguration<BlockNumberFor<T>>)>,
-	OptionQuery,
->;
+mod v4 {
+	use super::*;
+
+	#[frame_support::storage_alias]
+	pub(crate) type ActiveConfig<T: Config> =
+		StorageValue<Pallet<T>, V4HostConfiguration<BlockNumberFor<T>>, OptionQuery>;
+
+	#[frame_support::storage_alias]
+	pub(crate) type PendingConfigs<T: Config> = StorageValue<
+		Pallet<T>,
+		Vec<(SessionIndex, V4HostConfiguration<BlockNumberFor<T>>)>,
+		OptionQuery,
+	>;
+}
+
+mod v5 {
+	use super::*;
+
+	#[frame_support::storage_alias]
+	pub(crate) type ActiveConfig<T: Config> =
+		StorageValue<Pallet<T>, V5HostConfiguration<BlockNumberFor<T>>, OptionQuery>;
+
+	#[frame_support::storage_alias]
+	pub(crate) type PendingConfigs<T: Config> = StorageValue<
+		Pallet<T>,
+		Vec<(SessionIndex, V5HostConfiguration<BlockNumberFor<T>>)>,
+		OptionQuery,
+	>;
+}
 
 impl<BlockNumber: Default + From<u32>> Default for V4HostConfiguration<BlockNumber> {
 	fn default() -> Self {
@@ -266,6 +274,7 @@ impl<T: Config> OnRuntimeUpgrade for MigrateToV5<T> {
 	}
 
 	fn on_runtime_upgrade() -> Weight {
+		log::info!(target: configuration::LOG_TARGET, "MigrateToV5 started");
 		if StorageVersion::get::<Pallet<T>>() == 4 {
 			let weight_consumed = migrate_to_v5::<T>();
 
@@ -352,13 +361,13 @@ executor_params                          : Default::default(),
 		}
 	};
 
-	let v4 = V4ActiveConfig::<T>::get()
+	let v4 = v4::ActiveConfig::<T>::get()
 		.defensive_proof("Could not decode old config")
 		.unwrap_or_default();
 	let v5 = translate(v4);
-	V5ActiveConfig::<T>::set(Some(v5));
+	v5::ActiveConfig::<T>::set(Some(v5));
 
-	let pending_v4 = V4PendingConfigs::<T>::get()
+	let pending_v4 = v4::PendingConfigs::<T>::get()
 		.defensive_proof("Could not decode old pending")
 		.unwrap_or_default();
 	let mut pending_v5 = Vec::new();
@@ -367,7 +376,7 @@ executor_params                          : Default::default(),
 		let v5 = translate(v4);
 		pending_v5.push((session, v5));
 	}
-	V5PendingConfigs::<T>::set(Some(pending_v5.clone()));
+	v5::PendingConfigs::<T>::set(Some(pending_v5.clone()));
 
 	let num_configs = (pending_v5.len() + 1) as u64;
 	T::DbWeight::get().reads_writes(num_configs, num_configs)
@@ -397,8 +406,8 @@ mod tests {
 		// doesn't need to be read and also leaving it as one line allows to easily copy it.
 		let raw_config = hex_literal::hex!["0000a000005000000a00000000c8000000c800000a0000000a000000100e0000580200000000500000c800000700e8764817020040011e00000000000000005039278c0400000000000000000000005039278c0400000000000000000000e8030000009001001e00000000000000009001008070000000000000000000000a0000000a0000000a00000001000000010500000001c80000000600000058020000580200000200000059000000000000001e000000280000000700c817a80402004001010200000014000000"];
 
-		let v4 = v5::V4HostConfiguration::<primitives::BlockNumber>::decode(&mut &raw_config[..])
-			.unwrap();
+		let v4 =
+			V4HostConfiguration::<primitives::BlockNumber>::decode(&mut &raw_config[..]).unwrap();
 
 		// We check only a sample of the values here. If we missed any fields or messed up data types
 		// that would skew all the fields coming after.
@@ -421,7 +430,7 @@ mod tests {
 		// We specify only the picked fields and the rest should be provided by the `Default`
 		// implementation. That implementation is copied over between the two types and should work
 		// fine.
-		let v4 = v5::V4HostConfiguration::<primitives::BlockNumber> {
+		let v4 = V4HostConfiguration::<primitives::BlockNumber> {
 			ump_max_individual_weight: Weight::from_parts(0x71616e6f6e0au64, 0x71616e6f6e0au64),
 			needed_approvals: 69,
 			thread_availability_period: 55,
@@ -438,13 +447,13 @@ mod tests {
 
 		new_test_ext(Default::default()).execute_with(|| {
 			// Implant the v4 version in the state.
-			V4ActiveConfig::<Test>::set(Some(v4));
-			V4PendingConfigs::<Test>::set(Some(pending_configs));
+			v4::ActiveConfig::<Test>::set(Some(v4));
+			v4::PendingConfigs::<Test>::set(Some(pending_configs));
 
 			migrate_to_v5::<Test>();
 
-			let v5 = V5ActiveConfig::<Test>::get().unwrap();
-			let mut configs_to_check = V5PendingConfigs::<Test>::get().unwrap();
+			let v5 = v5::ActiveConfig::<Test>::get().unwrap();
+			let mut configs_to_check = v5::PendingConfigs::<Test>::get().unwrap();
 			configs_to_check.push((0, v5.clone()));
 
 			for (_, v4) in configs_to_check {

runtime/parachains/src/configuration/migration/v6.rs

@@ -34,27 +34,35 @@ use super::v5::V5HostConfiguration;
 // Change this once there is V7.
 type V6HostConfiguration<BlockNumber> = configuration::HostConfiguration<BlockNumber>;
 
-#[frame_support::storage_alias]
-pub(crate) type V5ActiveConfig<T: Config> =
-	StorageValue<Pallet<T>, V5HostConfiguration<BlockNumberFor<T>>, OptionQuery>;
-
-#[frame_support::storage_alias]
-pub(crate) type V5PendingConfigs<T: Config> = StorageValue<
-	Pallet<T>,
-	Vec<(SessionIndex, V5HostConfiguration<BlockNumberFor<T>>)>,
-	OptionQuery,
->;
-
-#[frame_support::storage_alias]
-pub(crate) type V6ActiveConfig<T: Config> =
-	StorageValue<Pallet<T>, V6HostConfiguration<BlockNumberFor<T>>, OptionQuery>;
-
-#[frame_support::storage_alias]
-pub(crate) type V6PendingConfigs<T: Config> = StorageValue<
-	Pallet<T>,
-	Vec<(SessionIndex, V6HostConfiguration<BlockNumberFor<T>>)>,
-	OptionQuery,
->;
+mod v5 {
+	use super::*;
+
+	#[frame_support::storage_alias]
+	pub(crate) type ActiveConfig<T: Config> =
+		StorageValue<Pallet<T>, V5HostConfiguration<BlockNumberFor<T>>, OptionQuery>;
+
+	#[frame_support::storage_alias]
+	pub(crate) type PendingConfigs<T: Config> = StorageValue<
+		Pallet<T>,
+		Vec<(SessionIndex, V5HostConfiguration<BlockNumberFor<T>>)>,
+		OptionQuery,
+	>;
+}
+
+mod v6 {
+	use super::*;
+
+	#[frame_support::storage_alias]
+	pub(crate) type ActiveConfig<T: Config> =
+		StorageValue<Pallet<T>, V6HostConfiguration<BlockNumberFor<T>>, OptionQuery>;
+
+	#[frame_support::storage_alias]
+	pub(crate) type PendingConfigs<T: Config> = StorageValue<
+		Pallet<T>,
+		Vec<(SessionIndex, V6HostConfiguration<BlockNumberFor<T>>)>,
+		OptionQuery,
+	>;
+}
 
 pub struct MigrateToV6<T>(sp_std::marker::PhantomData<T>);
 impl<T: Config> OnRuntimeUpgrade for MigrateToV6<T> {
@@ -65,6 +73,7 @@ impl<T: Config> OnRuntimeUpgrade for MigrateToV6<T> {
 	}
 
 	fn on_runtime_upgrade() -> Weight {
+		log::info!(target: configuration::LOG_TARGET, "MigrateToV6 started");
 		if StorageVersion::get::<Pallet<T>>() == 5 {
 			let weight_consumed = migrate_to_v6::<T>();
 
@@ -145,24 +154,24 @@ executor_params                          : pre.executor_params,
 		}
 	};
 
-	let v5 = V5ActiveConfig::<T>::get()
+	let v5 = v5::ActiveConfig::<T>::get()
 		.defensive_proof("Could not decode old config")
 		.unwrap_or_default();
 	let v6 = translate(v5);
-	V6ActiveConfig::<T>::set(Some(v6));
+	v6::ActiveConfig::<T>::set(Some(v6));
 
-	let pending_v4 = V5PendingConfigs::<T>::get()
+	let pending_v5 = v5::PendingConfigs::<T>::get()
 		.defensive_proof("Could not decode old pending")
 		.unwrap_or_default();
-	let mut pending_v5 = Vec::new();
+	let mut pending_v6 = Vec::new();
 
-	for (session, v5) in pending_v4.into_iter() {
+	for (session, v5) in pending_v5.into_iter() {
 		let v6 = translate(v5);
-		pending_v5.push((session, v6));
+		pending_v6.push((session, v6));
 	}
-	V6PendingConfigs::<T>::set(Some(pending_v5.clone()));
+	v6::PendingConfigs::<T>::set(Some(pending_v6.clone()));
 
-	let num_configs = (pending_v5.len() + 1) as u64;
+	let num_configs = (pending_v6.len() + 1) as u64;
 	T::DbWeight::get().reads_writes(num_configs, num_configs)
 }
 
@@ -201,6 +210,7 @@ mod tests {
 		assert_eq!(v5.n_delay_tranches, 40);
 		assert_eq!(v5.ump_max_individual_weight, Weight::from_parts(20_000_000_000, 5_242_880));
 		assert_eq!(v5.minimum_validation_upgrade_delay, 15); // This is the last field in the struct.
+		assert_eq!(v5.group_rotation_frequency, 10);
 	}
 
 	#[test]
@@ -230,13 +240,13 @@ mod tests {
 
 		new_test_ext(Default::default()).execute_with(|| {
 			// Implant the v5 version in the state.
-			V5ActiveConfig::<Test>::set(Some(v5));
-			V5PendingConfigs::<Test>::set(Some(pending_configs));
+			v5::ActiveConfig::<Test>::set(Some(v5));
+			v5::PendingConfigs::<Test>::set(Some(pending_configs));
 
 			migrate_to_v6::<Test>();
 
-			let v6 = V6ActiveConfig::<Test>::get().unwrap();
-			let mut configs_to_check = V6PendingConfigs::<Test>::get().unwrap();
+			let v6 = v6::ActiveConfig::<Test>::get().unwrap();
+			let mut configs_to_check = v6::PendingConfigs::<Test>::get().unwrap();
 			configs_to_check.push((0, v6.clone()));
 
 			for (_, v5) in configs_to_check {

runtime/parachains/src/configuration/migration_ump.rs

@@ -99,10 +99,29 @@ pub mod latest {
 				"Last pending HostConfig upgrade:\n\n{:#?}\n",
 				pending.last()
 			);
+			let Some(last) = pending.last() else {
+				return Err("There must be a new pending upgrade enqueued".into());
+			};
 			ensure!(
 				pending.len() == old_pending as usize + 1,
-				"There must be a new pending upgrade enqueued"
+				"There must be exactly one new pending upgrade enqueued"
 			);
+			if let Err(err) = last.1.check_consistency() {
+				log::error!(
+					target: LOG_TARGET,
+					"Last PendingConfig is invalidity {:?}", err,
+				);
+
+				return Err("Pending upgrade must be sane but was not".into())
+			}
+			if let Err(err) = ActiveConfig::<T>::get().check_consistency() {
+				log::error!(
+					target: LOG_TARGET,
+					"ActiveConfig is invalid: {:?}", err,
+				);
+
+				return Err("Active upgrade must be sane but was not".into())
+			}
 
 			Ok(())
 		}