Backport 4229 Into v1.7.0 (#4288)

Backport of #4229 into the v1.7.0 release in order to support dependent runtimes. This version of the SDK doesn't have `UncheckedOnRuntimeUpgrade`, so I do it the "old way".
paritytech · Apr 26, 2024 · d0c26d8 · d0c26d8
1 parent 7720de4
commit d0c26d8
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 55 deletions.
diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs
@@ -118,7 +118,7 @@ pub mod pallet {
 	use sp_staking::SessionIndex;
 	use sp_std::vec::Vec;
 
-	/// The in-code storage version.
+	/// The current storage version.
 	const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);
 
 	type BalanceOf<T> =

diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs
@@ -17,7 +17,7 @@
 //! A module that is responsible for migration of storage for Collator Selection.
 
 use super::*;
-use frame_support::traits::{OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade};
+use frame_support::traits::OnRuntimeUpgrade;
 use log;
 
 /// Migrate to v2. Should have been part of <https://github.com/paritytech/polkadot-sdk/pull/1340>.
@@ -32,17 +32,6 @@ pub mod v2 {
 	#[cfg(feature = "try-runtime")]
 	use sp_std::vec::Vec;
 
-	/// [`UncheckedMigrationToV2`] wrapped in a
-	/// [`VersionedMigration`](frame_support::migrations::VersionedMigration), ensuring the
-	/// migration is only performed when on-chain version is 1.
-	pub type MigrationToV2<T> = frame_support::migrations::VersionedMigration<
-		1,
-		2,
-		UncheckedMigrationToV2<T>,
-		Pallet<T>,
-		<T as frame_system::Config>::DbWeight,
-	>;
-
 	#[storage_alias]
 	pub type Candidates<T: Config> = StorageValue<
 		Pallet<T>,
@@ -51,53 +40,64 @@ pub mod v2 {
 	>;
 
 	/// Migrate to V2.
-	pub struct UncheckedMigrationToV2<T>(sp_std::marker::PhantomData<T>);
-	impl<T: Config + pallet_balances::Config> UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2<T> {
+	pub struct MigrationToV2<T>(sp_std::marker::PhantomData<T>);
+	impl<T: Config + pallet_balances::Config> OnRuntimeUpgrade for MigrationToV2<T> {
 		fn on_runtime_upgrade() -> Weight {
-			let mut weight = Weight::zero();
-			let mut count: u64 = 0;
-			// candidates who exist under the old `Candidates` key
-			let candidates = Candidates::<T>::take();
-
-			// New candidates who have registered since the upgrade. Under normal circumstances,
-			// this should not exist because the migration should be applied when the upgrade
-			// happens. But in Polkadot/Kusama we messed this up, and people registered under
-			// `CandidateList` while their funds were locked in `Candidates`.
-			let new_candidate_list = CandidateList::<T>::get();
-			if new_candidate_list.len().is_zero() {
-				// The new list is empty, so this is essentially being applied correctly. We just
-				// put the candidates into the new storage item.
-				CandidateList::<T>::put(&candidates);
-				// 1 write for the new list
-				weight.saturating_accrue(T::DbWeight::get().reads_writes(0, 1));
-			} else {
-				// Oops, the runtime upgraded without the migration. There are new candidates in
-				// `CandidateList`. So, let's just refund the old ones and assume they have already
-				// started participating in the new system.
-				for candidate in candidates {
-					let err = T::Currency::unreserve(&candidate.who, candidate.deposit);
-					if err > Zero::zero() {
-						log::error!(
-							target: LOG_TARGET,
-							"{:?} balance was unable to be unreserved from {:?}",
-							err, &candidate.who,
-						);
+			let on_chain_version = Pallet::<T>::on_chain_storage_version();
+			if on_chain_version == 1 {
+				let mut weight = Weight::zero();
+				let mut count: u64 = 0;
+				// candidates who exist under the old `Candidates` key
+				let candidates = Candidates::<T>::take();
+
+				// New candidates who have registered since the upgrade. Under normal circumstances,
+				// this should not exist because the migration should be applied when the upgrade
+				// happens. But in Polkadot/Kusama we messed this up, and people registered under
+				// `CandidateList` while their funds were locked in `Candidates`.
+				let new_candidate_list = CandidateList::<T>::get();
+				if new_candidate_list.len().is_zero() {
+					// The new list is empty, so this is essentially being applied correctly. We
+					// just put the candidates into the new storage item.
+					CandidateList::<T>::put(&candidates);
+					// 1 write for the new list
+					weight.saturating_accrue(T::DbWeight::get().reads_writes(0, 1));
+				} else {
+					// Oops, the runtime upgraded without the migration. There are new candidates in
+					// `CandidateList`. So, let's just refund the old ones and assume they have
+					// already started participating in the new system.
+					for candidate in candidates {
+						let err = T::Currency::unreserve(&candidate.who, candidate.deposit);
+						if err > Zero::zero() {
+							log::error!(
+								target: LOG_TARGET,
+								"{:?} balance was unable to be unreserved from {:?}",
+								err, &candidate.who,
+							);
+						}
+						count.saturating_inc();
 					}
-					count.saturating_inc();
+					weight.saturating_accrue(
+						<<T as pallet_balances::Config>::WeightInfo as pallet_balances::WeightInfo>::force_unreserve().saturating_mul(count.into()),
+					);
 				}
-				weight.saturating_accrue(
-					<<T as pallet_balances::Config>::WeightInfo as pallet_balances::WeightInfo>::force_unreserve().saturating_mul(count.into()),
-				);
-			}
 
-			log::info!(
-				target: LOG_TARGET,
-				"Unreserved locked bond of {} candidates, upgraded storage to version 2",
-				count,
-			);
+				StorageVersion::new(2).put::<Pallet<T>>();
+
+				log::info!(
+					target: LOG_TARGET,
+					"Unreserved locked bond of {} candidates, upgraded storage to version 2",
+					count,
+				);
 
-			weight.saturating_accrue(T::DbWeight::get().reads_writes(3, 2));
-			weight
+				weight.saturating_accrue(T::DbWeight::get().reads_writes(3, 2));
+				weight
+			} else {
+				log::info!(
+					target: LOG_TARGET,
+					"Migration did not execute. This probably should be removed"
+				);
+				T::DbWeight::get().reads(1)
+			}
 		}
 
 		#[cfg(feature = "try-runtime")]

diff --git a/prdoc/pr_4288.prdoc b/prdoc/pr_4288.prdoc
@@ -0,0 +1,9 @@
+title: "Fix Stuck Collator Funds"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Fixes stuck collator funds by providing a migration that should have been in PR 1340.
+
+crates:
+  - name: pallet-collator-selection