migrations: prevent accidentally using unversioned migrations instead…

… of `VersionedMigration` (paritytech#3835)

closes paritytech#1324 

#### Problem
Currently, it is possible to accidentally use inner unversioned
migration instead of `VersionedMigration` since both implement
`OnRuntimeUpgrade`.

#### Solution

With this change, we make it clear that value of `Inner` is not intended
to be used directly. It is achieved by bounding `Inner` to new trait
`UncheckedOnRuntimeUpgrade`, which has the same interface (except
`unchecked_` prefix) as `OnRuntimeUpgrade`.

#### `try-runtime` functions

Since developers can implement `try-runtime` for `Inner` value in
`VersionedMigration` and have custom logic for it, I added the same
`try-runtime` functions to `UncheckedOnRuntimeUpgrade`. I looked for a
ways to not duplicate functions, but couldn't find anything that doesn't
significantly change the codebase. So I would appreciate If you have any
suggestions to improve this

cc @liamaharon @xlc 

polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

---------

Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>

cumulus/pallets/xcmp-queue/src/migration.rs

@@ -20,7 +20,7 @@ use crate::{Config, OverweightIndex, Pallet, QueueConfig, QueueConfigData, DEFAU
 use cumulus_primitives_core::XcmpMessageFormat;
 use frame_support::{
 	pallet_prelude::*,
-	traits::{EnqueueMessage, OnRuntimeUpgrade, StorageVersion},
+	traits::{EnqueueMessage, StorageVersion, UncheckedOnRuntimeUpgrade},
 	weights::{constants::WEIGHT_REF_TIME_PER_MILLIS, Weight},
 };
 
@@ -96,7 +96,7 @@ pub mod v2 {
 	/// 2D weights).
 	pub struct UncheckedMigrationToV2<T: Config>(PhantomData<T>);
 
-	impl<T: Config> OnRuntimeUpgrade for UncheckedMigrationToV2<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2<T> {
 		#[allow(deprecated)]
 		fn on_runtime_upgrade() -> Weight {
 			let translate = |pre: v1::QueueConfigData| -> v2::QueueConfigData {
@@ -187,7 +187,7 @@ pub mod v3 {
 	/// Migrates the pallet storage to v3.
 	pub struct UncheckedMigrationToV3<T: Config>(PhantomData<T>);
 
-	impl<T: Config> OnRuntimeUpgrade for UncheckedMigrationToV3<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrationToV3<T> {
 		fn on_runtime_upgrade() -> Weight {
 			#[frame_support::storage_alias]
 			type Overweight<T: Config> =
@@ -266,7 +266,7 @@ pub mod v4 {
 	/// thresholds to at least the default values.
 	pub struct UncheckedMigrationToV4<T: Config>(PhantomData<T>);
 
-	impl<T: Config> OnRuntimeUpgrade for UncheckedMigrationToV4<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrationToV4<T> {
 		fn on_runtime_upgrade() -> Weight {
 			let translate = |pre: v2::QueueConfigData| -> QueueConfigData {
 				let pre_default = v2::QueueConfigData::default();
@@ -315,6 +315,7 @@ pub mod v4 {
 mod tests {
 	use super::*;
 	use crate::mock::{new_test_ext, Test};
+	use frame_support::traits::OnRuntimeUpgrade;
 
 	#[test]
 	#[allow(deprecated)]

polkadot/runtime/common/src/assigned_slots/migration.rs

@@ -15,18 +15,17 @@
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
 use super::{Config, MaxPermanentSlots, MaxTemporarySlots, Pallet, LOG_TARGET};
-use frame_support::traits::{Get, GetStorageVersion, OnRuntimeUpgrade};
+use frame_support::traits::{Get, GetStorageVersion, UncheckedOnRuntimeUpgrade};
 
 #[cfg(feature = "try-runtime")]
 use frame_support::ensure;
 #[cfg(feature = "try-runtime")]
 use sp_std::vec::Vec;
 
 pub mod v1 {
-
 	use super::*;
 	pub struct VersionUncheckedMigrateToV1<T>(sp_std::marker::PhantomData<T>);
-	impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateToV1<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateToV1<T> {
 		#[cfg(feature = "try-runtime")]
 		fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
 			let on_chain_version = Pallet::<T>::on_chain_storage_version();

polkadot/runtime/common/src/paras_registrar/migration.rs

@@ -15,7 +15,7 @@
 // along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
 
 use super::*;
-use frame_support::traits::{Contains, OnRuntimeUpgrade};
+use frame_support::traits::{Contains, UncheckedOnRuntimeUpgrade};
 
 #[derive(Encode, Decode)]
 pub struct ParaInfoV1<Account, Balance> {
@@ -27,7 +27,7 @@ pub struct ParaInfoV1<Account, Balance> {
 pub struct VersionUncheckedMigrateToV1<T, UnlockParaIds>(
 	sp_std::marker::PhantomData<(T, UnlockParaIds)>,
 );
-impl<T: Config, UnlockParaIds: Contains<ParaId>> OnRuntimeUpgrade
+impl<T: Config, UnlockParaIds: Contains<ParaId>> UncheckedOnRuntimeUpgrade
 	for VersionUncheckedMigrateToV1<T, UnlockParaIds>
 {
 	fn on_runtime_upgrade() -> Weight {

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

@@ -18,7 +18,7 @@
 use super::*;
 use frame_support::{
 	migrations::VersionedMigration, pallet_prelude::ValueQuery, storage_alias,
-	traits::OnRuntimeUpgrade, weights::Weight,
+	traits::UncheckedOnRuntimeUpgrade, weights::Weight,
 };
 
 mod v0 {
@@ -51,7 +51,7 @@ mod v1 {
 
 	/// Migration to V1
 	pub struct UncheckedMigrateToV1<T>(sp_std::marker::PhantomData<T>);
-	impl<T: Config> OnRuntimeUpgrade for UncheckedMigrateToV1<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrateToV1<T> {
 		fn on_runtime_upgrade() -> Weight {
 			let mut weight: Weight = Weight::zero();
 
@@ -141,7 +141,7 @@ pub type MigrateV0ToV1<T> = VersionedMigration<
 
 #[cfg(test)]
 mod tests {
-	use super::{v0, v1, OnRuntimeUpgrade, Weight};
+	use super::{v0, v1, UncheckedOnRuntimeUpgrade, Weight};
 	use crate::mock::{new_test_ext, MockGenesisConfig, OnDemandAssigner, Test};
 	use primitives::Id as ParaId;
 
@@ -163,7 +163,7 @@ mod tests {
 
 			// For tests, db weight is zero.
 			assert_eq!(
-				<v1::UncheckedMigrateToV1<Test> as OnRuntimeUpgrade>::on_runtime_upgrade(),
+				<v1::UncheckedMigrateToV1<Test> as UncheckedOnRuntimeUpgrade>::on_runtime_upgrade(),
 				Weight::zero()
 			);
 

polkadot/runtime/parachains/src/configuration/migration/v10.rs

@@ -17,7 +17,11 @@
 //! A module that is responsible for migration of storage.
 
 use crate::configuration::{Config, Pallet};
-use frame_support::{pallet_prelude::*, traits::Defensive, weights::Weight};
+use frame_support::{
+	pallet_prelude::*,
+	traits::{Defensive, UncheckedOnRuntimeUpgrade},
+	weights::Weight,
+};
 use frame_system::pallet_prelude::BlockNumberFor;
 use primitives::{
 	AsyncBackingParams, Balance, ExecutorParams, NodeFeatures, SessionIndex,
@@ -26,8 +30,6 @@ use primitives::{
 use sp_runtime::Perbill;
 use sp_std::vec::Vec;
 
-use frame_support::traits::OnRuntimeUpgrade;
-
 use super::v9::V9HostConfiguration;
 // All configuration of the runtime with respect to paras.
 #[derive(Clone, Encode, PartialEq, Decode, Debug)]
@@ -163,7 +165,7 @@ mod v10 {
 }
 
 pub struct VersionUncheckedMigrateToV10<T>(sp_std::marker::PhantomData<T>);
-impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateToV10<T> {
+impl<T: Config> UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateToV10<T> {
 	#[cfg(feature = "try-runtime")]
 	fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
 		log::trace!(target: crate::configuration::LOG_TARGET, "Running pre_upgrade() for HostConfiguration MigrateToV10");

polkadot/runtime/parachains/src/configuration/migration/v11.rs

@@ -18,7 +18,10 @@
 
 use crate::configuration::{self, Config, Pallet};
 use frame_support::{
-	migrations::VersionedMigration, pallet_prelude::*, traits::Defensive, weights::Weight,
+	migrations::VersionedMigration,
+	pallet_prelude::*,
+	traits::{Defensive, UncheckedOnRuntimeUpgrade},
+	weights::Weight,
 };
 use frame_system::pallet_prelude::BlockNumberFor;
 use primitives::{
@@ -27,7 +30,6 @@ use primitives::{
 };
 use sp_std::vec::Vec;
 
-use frame_support::traits::OnRuntimeUpgrade;
 use polkadot_core_primitives::Balance;
 use sp_arithmetic::Perbill;
 
@@ -176,7 +178,7 @@ pub type MigrateToV11<T> = VersionedMigration<
 >;
 
 pub struct UncheckedMigrateToV11<T>(sp_std::marker::PhantomData<T>);
-impl<T: Config> OnRuntimeUpgrade for UncheckedMigrateToV11<T> {
+impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrateToV11<T> {
 	#[cfg(feature = "try-runtime")]
 	fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
 		log::trace!(target: crate::configuration::LOG_TARGET, "Running pre_upgrade() for HostConfiguration MigrateToV11");

polkadot/runtime/parachains/src/configuration/migration/v12.rs

@@ -20,7 +20,7 @@ use crate::configuration::{self, migration::v11::V11HostConfiguration, Config, P
 use frame_support::{
 	migrations::VersionedMigration,
 	pallet_prelude::*,
-	traits::{Defensive, OnRuntimeUpgrade},
+	traits::{Defensive, UncheckedOnRuntimeUpgrade},
 };
 use frame_system::pallet_prelude::BlockNumberFor;
 use primitives::vstaging::SchedulerParams;
@@ -70,7 +70,7 @@ pub type MigrateToV12<T> = VersionedMigration<
 
 pub struct UncheckedMigrateToV12<T>(sp_std::marker::PhantomData<T>);
 
-impl<T: Config> OnRuntimeUpgrade for UncheckedMigrateToV12<T> {
+impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrateToV12<T> {
 	#[cfg(feature = "try-runtime")]
 	fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
 		log::trace!(target: crate::configuration::LOG_TARGET, "Running pre_upgrade() for HostConfiguration MigrateToV12");

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

@@ -73,7 +73,7 @@ mod v1 {
 		CandidatePendingAvailability as V1CandidatePendingAvailability, Config, Pallet,
 		PendingAvailability as V1PendingAvailability,
 	};
-	use frame_support::{traits::OnRuntimeUpgrade, weights::Weight};
+	use frame_support::{traits::UncheckedOnRuntimeUpgrade, weights::Weight};
 	use sp_core::Get;
 	use sp_std::{collections::vec_deque::VecDeque, vec::Vec};
 
@@ -87,7 +87,7 @@ mod v1 {
 
 	pub struct VersionUncheckedMigrateToV1<T>(sp_std::marker::PhantomData<T>);
 
-	impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateToV1<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateToV1<T> {
 		#[cfg(feature = "try-runtime")]
 		fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
 			log::trace!(target: crate::inclusion::LOG_TARGET, "Running pre_upgrade() for inclusion MigrateToV1");
@@ -216,7 +216,7 @@ mod tests {
 		},
 		mock::{new_test_ext, MockGenesisConfig, Test},
 	};
-	use frame_support::traits::OnRuntimeUpgrade;
+	use frame_support::traits::UncheckedOnRuntimeUpgrade;
 	use primitives::{AvailabilityBitfield, Id as ParaId};
 	use test_helpers::{dummy_candidate_commitments, dummy_candidate_descriptor, dummy_hash};
 
@@ -225,7 +225,7 @@ mod tests {
 		new_test_ext(MockGenesisConfig::default()).execute_with(|| {
 			// No data to migrate.
 			assert_eq!(
-				<VersionUncheckedMigrateToV1<Test> as OnRuntimeUpgrade>::on_runtime_upgrade(),
+				<VersionUncheckedMigrateToV1<Test> as UncheckedOnRuntimeUpgrade>::on_runtime_upgrade(),
 				Weight::zero()
 			);
 			assert!(V1PendingAvailability::<Test>::iter().next().is_none());
@@ -299,7 +299,7 @@ mod tests {
 
 			// For tests, db weight is zero.
 			assert_eq!(
-				<VersionUncheckedMigrateToV1<Test> as OnRuntimeUpgrade>::on_runtime_upgrade(),
+				<VersionUncheckedMigrateToV1<Test> as UncheckedOnRuntimeUpgrade>::on_runtime_upgrade(),
 				Weight::zero()
 			);
 

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

@@ -19,7 +19,7 @@
 use super::*;
 use frame_support::{
 	migrations::VersionedMigration, pallet_prelude::ValueQuery, storage_alias,
-	traits::OnRuntimeUpgrade, weights::Weight,
+	traits::UncheckedOnRuntimeUpgrade, weights::Weight,
 };
 
 /// Old/legacy assignment representation (v0).
@@ -105,7 +105,8 @@ mod v0 {
 // - Assignments only consist of `ParaId`, `Assignment` is a concrete type (Same as V0Assignment).
 mod v1 {
 	use frame_support::{
-		pallet_prelude::ValueQuery, storage_alias, traits::OnRuntimeUpgrade, weights::Weight,
+		pallet_prelude::ValueQuery, storage_alias, traits::UncheckedOnRuntimeUpgrade,
+		weights::Weight,
 	};
 	use frame_system::pallet_prelude::BlockNumberFor;
 
@@ -164,7 +165,7 @@ mod v1 {
 
 	/// Migration to V1
 	pub struct UncheckedMigrateToV1<T>(sp_std::marker::PhantomData<T>);
-	impl<T: Config> OnRuntimeUpgrade for UncheckedMigrateToV1<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrateToV1<T> {
 		fn on_runtime_upgrade() -> Weight {
 			let mut weight: Weight = Weight::zero();
 
@@ -302,7 +303,7 @@ mod v2 {
 	/// Migration to V2
 	pub struct UncheckedMigrateToV2<T>(sp_std::marker::PhantomData<T>);
 
-	impl<T: Config> OnRuntimeUpgrade for UncheckedMigrateToV2<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for UncheckedMigrateToV2<T> {
 		fn on_runtime_upgrade() -> Weight {
 			let mut weight: Weight = Weight::zero();
 

polkadot/xcm/pallet-xcm/src/migration.rs

@@ -19,7 +19,7 @@ use crate::{
 };
 use frame_support::{
 	pallet_prelude::*,
-	traits::{OnRuntimeUpgrade, StorageVersion},
+	traits::{OnRuntimeUpgrade, StorageVersion, UncheckedOnRuntimeUpgrade},
 	weights::Weight,
 };
 
@@ -35,7 +35,7 @@ pub mod v1 {
 	///
 	/// Use experimental [`MigrateToV1`] instead.
 	pub struct VersionUncheckedMigrateToV1<T>(sp_std::marker::PhantomData<T>);
-	impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateToV1<T> {
+	impl<T: Config> UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateToV1<T> {
 		fn on_runtime_upgrade() -> Weight {
 			let mut weight = T::DbWeight::get().reads(1);
 

prdoc/pr_3835.prdoc

@@ -0,0 +1,54 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: "migrations: prevent accidentally using inner unversioned migration instead of `VersionedMigration`"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Currently, it is possible to accidentally use inner unversioned migration instead of `VersionedMigration` 
+      since both implement `OnRuntimeUpgrade`. With this change, we make it clear that `Inner` is not intended 
+      to be used directly. It is achieved by bounding `Inner` to new trait `UncheckedOnRuntimeUpgrade`, which 
+      has the same interface as `OnRuntimeUpgrade`, but can not be used directly for runtime upgrade migrations.
+
+      This change will break all existing migrations passed to `VersionedMigration`. Developers should simply change 
+      those migrations to implement `UncheckedOnRuntimeUpgrade` instead of `OnRuntimeUpgrade`.
+
+      Example:
+
+      ```
+      --- a/path/to/migration.rs
+      +++ b/path/to/migration.rs
+      @@ -1,7 +1,7 @@
+      -impl<T: Config> OnRuntimeUpgrade for MigrateVNToVM<T> {
+      +impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateVNToVM<T> {
+          fn on_runtime_upgrade() -> Weight {
+              // Migration logic here
+              // Adjust the migration logic if necessary to align with the expectations
+              // of new `UncheckedOnRuntimeUpgrade` trait.
+              0
+          }
+      }
+      ```
+
+crates:
+  - name: "pallet-example-single-block-migrations"
+    bump: "major"
+  - name: "pallet-xcm"
+    bump: "major"
+  - name: "pallet-grandpa"
+    bump: "major"
+  - name: "pallet-identity"
+    bump: "major"
+  - name: "pallet-nomination-pools"
+    bump: "major"
+  - name: "pallet-society"
+    bump: "major"
+  - name: "frame-support"
+    bump: "major"
+  - name: "pallet-uniques"
+    bump: "major"
+  - name: "polkadot-runtime-parachains"
+    bump: "major"
+  - name: "polkadot-runtime-common"
+    bump: "major"