Fix para-scheduler migration on Rococo (#1921)

Closes #1916

Changes:
- Trivially wrap the migration into a version migration to enforce
idempotency.
- Opinionated logging nits

@liamaharon maybe we can add a check to the `try-runtime-cli` that
migrations are idempotent? It should be possible to check that the
storage root is identical after executing a second time (and that it
does not panic like it did here 😆).

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>

polkadot/runtime/parachains/src/scheduler/migration.rs

@@ -18,7 +18,8 @@
 
 use super::*;
 use frame_support::{
-	pallet_prelude::ValueQuery, storage_alias, traits::OnRuntimeUpgrade, weights::Weight,
+	migrations::VersionedMigration, pallet_prelude::ValueQuery, storage_alias,
+	traits::OnRuntimeUpgrade, weights::Weight,
 };
 
 mod v0 {
@@ -83,22 +84,26 @@ mod v0 {
 pub mod v1 {
 	use super::*;
 	use crate::scheduler;
-	use frame_support::traits::StorageVersion;
 
-	pub struct MigrateToV1<T>(sp_std::marker::PhantomData<T>);
-	impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
+	#[allow(deprecated)]
+	pub type MigrateToV1<T> = VersionedMigration<
+		0,
+		1,
+		UncheckedMigrateToV1<T>,
+		Pallet<T>,
+		<T as frame_system::Config>::DbWeight,
+	>;
+
+	#[deprecated(note = "Use MigrateToV1 instead")]
+	pub struct UncheckedMigrateToV1<T>(sp_std::marker::PhantomData<T>);
+	#[allow(deprecated)]
+	impl<T: Config> OnRuntimeUpgrade for UncheckedMigrateToV1<T> {
 		fn on_runtime_upgrade() -> Weight {
-			if StorageVersion::get::<Pallet<T>>() == 0 {
-				let weight_consumed = migrate_to_v1::<T>();
+			let weight_consumed = migrate_to_v1::<T>();
 
-				log::info!(target: scheduler::LOG_TARGET, "Migrating para scheduler storage to v1");
-				StorageVersion::new(1).put::<Pallet<T>>();
+			log::info!(target: scheduler::LOG_TARGET, "Migrating para scheduler storage to v1");
 
-				weight_consumed
-			} else {
-				log::warn!(target: scheduler::LOG_TARGET, "Para scheduler v1 migration should be removed.");
-				T::DbWeight::get().reads(1)
-			}
+			weight_consumed
 		}
 
 		#[cfg(feature = "try-runtime")]
@@ -117,10 +122,7 @@ pub mod v1 {
 		#[cfg(feature = "try-runtime")]
 		fn post_upgrade(state: Vec<u8>) -> Result<(), sp_runtime::DispatchError> {
 			log::trace!(target: crate::scheduler::LOG_TARGET, "Running post_upgrade()");
-			ensure!(
-				StorageVersion::get::<Pallet<T>>() >= 1,
-				"Storage version should be at least `1` after the migration"
-			);
+
 			ensure!(
 				v0::Scheduled::<T>::get().len() == 0,
 				"Scheduled should be empty after the migration"

prdoc/pr_1921.prdoc

@@ -0,0 +1,19 @@
+title: Fix para-scheduler migration
+
+doc:
+  - audience: Core Dev
+    description: |
+      Changing the `MigrateToV1` migration in the `ParachainScheduler` pallet to be truly idempotent. It is achieved by wrapping it in a `VersionedMigration`.
+
+migrations:
+  db: []
+
+  runtime:
+    - pallet: "ParachainScheduler"
+      description: Non-critical fixup for `MigrateToV1`.
+
+crates:
+  - name: "polkadot-runtime-parachains"
+    semver: patch
+
+host_functions: []

substrate/frame/support/src/migrations.rs

@@ -119,7 +119,7 @@ impl<
 		let on_chain_version = Pallet::on_chain_storage_version();
 		if on_chain_version == FROM {
 			log::info!(
-				"Running {} VersionedOnRuntimeUpgrade: version {:?} to {:?}.",
+				"🚚 Pallet {:?} migrating storage version from {:?} to {:?}.",
 				Pallet::name(),
 				FROM,
 				TO
@@ -134,9 +134,10 @@ impl<
 			weight.saturating_add(DbWeight::get().reads_writes(1, 1))
 		} else {
 			log::warn!(
-				"{} VersionedOnRuntimeUpgrade for version {:?} skipped because current on-chain version is {:?}.",
+				"🚚 Pallet {:?} migration {}->{} can be removed; on-chain is already at {:?}.",
 				Pallet::name(),
 				FROM,
+				TO,
 				on_chain_version
 			);
 			DbWeight::get().reads(1)