Fix Society v2 migration (#14421)

* fix society v2 migration

* Update frame/society/src/migrations.rs

* Update frame/society/src/migrations.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

* Update frame/society/src/migrations.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

* update for versioned upgrade

* fix society v2 migration

* remove references to members being sorted from commnets

* fix type

* fix can_migrate check

* add sanity log

* fix sanity check

* kick ci

* kick ci

* run tests with --experimental flag

* versioned migration cleanup

* revert pipeline change

* use defensive!

* semicolons

* defensive and doc comment

* address pr comment

* feature gate the versioned migration

* defensive_unwrap_or

* fix test

* fix doc comment

* change defensive to a log warning

* remove can_migrate anti-pattern

* Update frame/society/Cargo.toml

Co-authored-by: Bastian Köcher <git@kchr.de>

* add experimental feature warning to doc comment

* update doc comment

* bump ci

* kick ci

* kick ci

* kick ci

---------

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Bastian Köcher <git@kchr.de>

substrate/frame/society/Cargo.toml

@@ -35,6 +35,10 @@ sp-io = { version = "23.0.0", path = "../../primitives/io" }
 
 [features]
 default = ["std"]
+# Enable `VersionedRuntimeUpgrade` for the migrations that is currently still experimental.
+experimental = [
+	"frame-support/experimental"
+]
 std = [
 	"codec/std",
 	"frame-benchmarking?/std",

substrate/frame/society/src/lib.rs

@@ -466,12 +466,12 @@ pub struct GroupParams<Balance> {
 
 pub type GroupParamsFor<T, I> = GroupParams<BalanceOf<T, I>>;
 
+pub(crate) const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);
+
 #[frame_support::pallet]
 pub mod pallet {
 	use super::*;
 
-	const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);
-
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
 	#[pallet::without_storage_info]
@@ -1681,8 +1681,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		bids.iter().any(|bid| bid.who == *who)
 	}
 
-	/// Add a member to the sorted members list. If the user is already a member, do nothing.
-	/// Can fail when `MaxMember` limit is reached, but in that case it has no side-effects.
+	/// Add a member to the members list. If the user is already a member, do nothing. Can fail when
+	/// `MaxMember` limit is reached, but in that case it has no side-effects.
 	///
 	/// Set the `payouts` for the member. NOTE: This *WILL NOT RESERVE THE FUNDS TO MAKE THE
 	/// PAYOUT*. Only set this to be non-empty if you already have the funds reserved in the Payouts
@@ -1703,7 +1703,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		Ok(())
 	}
 
-	/// Add a member back to the sorted members list, setting their `rank` and `payouts`.
+	/// Add a member back to the members list, setting their `rank` and `payouts`.
 	///
 	/// Can fail when `MaxMember` limit is reached, but in that case it has no side-effects.
 	///
@@ -1713,8 +1713,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		Self::insert_member(who, rank)
 	}
 
-	/// Add a member to the sorted members list. If the user is already a member, do nothing.
-	/// Can fail when `MaxMember` limit is reached, but in that case it has no side-effects.
+	/// Add a member to the members list. If the user is already a member, do nothing. Can fail when
+	/// `MaxMember` limit is reached, but in that case it has no side-effects.
 	fn add_new_member(who: &T::AccountId, rank: Rank) -> DispatchResult {
 		Self::insert_member(who, rank)
 	}

substrate/frame/society/src/migrations.rs

@@ -19,7 +19,7 @@
 
 use super::*;
 use codec::{Decode, Encode};
-use frame_support::traits::{Instance, OnRuntimeUpgrade};
+use frame_support::traits::{Defensive, DefensiveOption, Instance, OnRuntimeUpgrade};
 
 #[cfg(feature = "try-runtime")]
 use sp_runtime::TryRuntimeError;
@@ -28,7 +28,7 @@ use sp_runtime::TryRuntimeError;
 const TARGET: &'static str = "runtime::society::migration";
 
 /// This migration moves all the state to v2 of Society.
-pub struct MigrateToV2<T: Config<I>, I: 'static, PastPayouts>(
+pub struct VersionUncheckedMigrateToV2<T: Config<I>, I: 'static, PastPayouts>(
 	sp_std::marker::PhantomData<(T, I, PastPayouts)>,
 );
 
@@ -36,12 +36,10 @@ impl<
 		T: Config<I>,
 		I: Instance + 'static,
 		PastPayouts: Get<Vec<(<T as frame_system::Config>::AccountId, BalanceOf<T, I>)>>,
-	> OnRuntimeUpgrade for MigrateToV2<T, I, PastPayouts>
+	> OnRuntimeUpgrade for VersionUncheckedMigrateToV2<T, I, PastPayouts>
 {
 	#[cfg(feature = "try-runtime")]
 	fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
-		ensure!(can_migrate::<T, I>(), "pallet_society: already upgraded");
-
 		let current = Pallet::<T, I>::current_storage_version();
 		let onchain = Pallet::<T, I>::on_chain_storage_version();
 		ensure!(onchain == 0 && current == 2, "pallet_society: invalid version");
@@ -50,16 +48,16 @@ impl<
 	}
 
 	fn on_runtime_upgrade() -> Weight {
-		let current = Pallet::<T, I>::current_storage_version();
 		let onchain = Pallet::<T, I>::on_chain_storage_version();
-		if current == 2 && onchain == 0 {
-			from_original::<T, I>(&mut PastPayouts::get())
-		} else {
+		if onchain < 2 {
 			log::info!(
-				"Running migration with current storage version {:?} / onchain {:?}",
-				current,
+				target: TARGET,
+				"Running migration against onchain version {:?}",
 				onchain
 			);
+			from_original::<T, I>(&mut PastPayouts::get()).defensive_unwrap_or(Weight::MAX)
+		} else {
+			log::warn!("Unexpected onchain version: {:?} (expected 0)", onchain);
 			T::DbWeight::get().reads(1)
 		}
 	}
@@ -95,6 +93,19 @@ impl<
 	}
 }
 
+/// [`VersionUncheckedMigrateToV2`] wrapped in a
+/// [`frame_support::migrations::VersionedRuntimeUpgrade`], ensuring the migration is only performed
+/// when on-chain version is 0.
+#[cfg(feature = "experimental")]
+pub type VersionCheckedMigrateToV2<T, I, PastPayouts> =
+	frame_support::migrations::VersionedRuntimeUpgrade<
+		0,
+		2,
+		VersionUncheckedMigrateToV2<T, I, PastPayouts>,
+		crate::pallet::Pallet<T, I>,
+		<T as frame_system::Config>::DbWeight,
+	>;
+
 pub(crate) mod old {
 	use super::*;
 	use frame_support::storage_alias;
@@ -180,10 +191,6 @@ pub(crate) mod old {
 		StorageMap<Pallet<T, I>, Twox64Concat, <T as frame_system::Config>::AccountId, Vote>;
 }
 
-pub fn can_migrate<T: Config<I>, I: Instance + 'static>() -> bool {
-	old::Members::<T, I>::exists()
-}
-
 /// Will panic if there are any inconsistencies in the pallet's state or old keys remaining.
 pub fn assert_internal_consistency<T: Config<I>, I: Instance + 'static>() {
 	// Check all members are valid data.
@@ -235,15 +242,7 @@ pub fn assert_internal_consistency<T: Config<I>, I: Instance + 'static>() {
 
 pub fn from_original<T: Config<I>, I: Instance + 'static>(
 	past_payouts: &mut [(<T as frame_system::Config>::AccountId, BalanceOf<T, I>)],
-) -> Weight {
-	// First check that this is the original state layout. This is easy since the original layout
-	// contained the Members value, and this value no longer exists in the new layout.
-	if !old::Members::<T, I>::exists() {
-		log::warn!(target: TARGET, "Skipping MigrateToV2 migration since it appears unapplicable");
-		// Already migrated or no data to migrate: Bail.
-		return T::DbWeight::get().reads(1)
-	}
-
+) -> Result<Weight, &'static str> {
 	// Migrate Bids from old::Bids (just a trunctation).
 	Bids::<T, I>::put(BoundedVec::<_, T::MaxBids>::truncate_from(old::Bids::<T, I>::take()));
 
@@ -281,6 +280,36 @@ pub fn from_original<T: Config<I>, I: Instance + 'static>(
 		let record = MemberRecord { index: member_count, rank: 0, strikes, vouching };
 		Members::<T, I>::insert(&member, record);
 		MemberByIndex::<T, I>::insert(member_count, &member);
+
+		// The founder must be the first member in Society V2. If we find the founder not in index
+		// zero, we swap it with the first member.
+		if member == Founder::<T, I>::get().defensive_ok_or("founder must always be set")? &&
+			member_count > 0
+		{
+			let member_to_swap = MemberByIndex::<T, I>::get(0)
+				.defensive_ok_or("member_count > 0, we must have at least 1 member")?;
+			// Swap the founder with the first member in MemberByIndex.
+			MemberByIndex::<T, I>::swap(0, member_count);
+			// Update the indicies of the swapped member MemberRecords.
+			Members::<T, I>::mutate(&member, |m| {
+				if let Some(member) = m {
+					member.index = 0;
+				} else {
+					frame_support::defensive!(
+						"Member somehow disapeared from storage after it was inserted"
+					);
+				}
+			});
+			Members::<T, I>::mutate(&member_to_swap, |m| {
+				if let Some(member) = m {
+					member.index = member_count;
+				} else {
+					frame_support::defensive!(
+						"Member somehow disapeared from storage after it was queried"
+					);
+				}
+			});
+		}
 		member_count.saturating_inc();
 	}
 	MemberCount::<T, I>::put(member_count);
@@ -317,7 +346,7 @@ pub fn from_original<T: Config<I>, I: Instance + 'static>(
 	old::Defender::<T, I>::kill();
 	let _ = old::DefenderVotes::<T, I>::clear(u32::MAX, None);
 
-	T::BlockWeights::get().max_block
+	Ok(T::BlockWeights::get().max_block)
 }
 
 pub fn from_raw_past_payouts<T: Config<I>, I: Instance + 'static>(

substrate/frame/society/src/tests.rs

@@ -77,7 +77,7 @@ fn migration_works() {
 			.collect::<Vec<_>>();
 		old::Bids::<Test, ()>::put(bids);
 
-		migrations::from_original::<Test, ()>(&mut [][..]);
+		migrations::from_original::<Test, ()>(&mut [][..]).expect("migration failed");
 		migrations::assert_internal_consistency::<Test, ()>();
 
 		assert_eq!(

substrate/frame/support/src/migrations.rs

@@ -24,12 +24,8 @@ use sp_core::Get;
 use sp_io::{hashing::twox_128, storage::clear_prefix, KillStorageResult};
 use sp_std::marker::PhantomData;
 
-#[cfg(feature = "try-runtime")]
-use sp_std::vec::Vec;
-
-#[cfg(feature = "experimental")]
-use crate::traits::OnRuntimeUpgrade;
-
+/// EXPERIMENTAL: The API of this feature may change.
+///
 /// Make it easier to write versioned runtime upgrades.
 ///
 /// [`VersionedRuntimeUpgrade`] allows developers to write migrations without worrying about
@@ -57,13 +53,19 @@ use crate::traits::OnRuntimeUpgrade;
 /// 	// OnRuntimeUpgrade implementation...
 /// }
 ///
-/// pub type VersionCheckedMigrateV5ToV6<Runtime, Pallet, DbWeight> =
-/// 	VersionedRuntimeUpgrade<5, 6, VersionUncheckedMigrateV5ToV6<Runtime>, Pallet, DbWeight>;
+/// pub type VersionCheckedMigrateV5ToV6<T, I> =
+/// 	VersionedRuntimeUpgrade<
+/// 		5,
+/// 		6,
+/// 		VersionUncheckedMigrateV5ToV6<T, I>,
+/// 		crate::pallet::Pallet<T, I>,
+/// 		<T as frame_system::Config>::DbWeight
+/// 	>;
 ///
 /// // Migrations tuple to pass to the Executive pallet:
 /// pub type Migrations = (
 /// 	// other migrations...
-/// 	VersionCheckedMigrateV5ToV6<Runtime, Balances, RuntimeDbWeight>,
+/// 	VersionCheckedMigrateV5ToV6<T, ()>,
 /// 	// other migrations...
 /// );
 /// ```
@@ -78,7 +80,7 @@ pub struct VersionedRuntimeUpgrade<const FROM: u16, const TO: u16, Inner, Pallet
 #[derive(codec::Encode, codec::Decode)]
 pub enum VersionedPostUpgradeData {
 	/// The migration ran, inner vec contains pre_upgrade data.
-	MigrationExecuted(Vec<u8>),
+	MigrationExecuted(sp_std::vec::Vec<u8>),
 	/// This migration is a noop, do not run post_upgrade checks.
 	Noop,
 }
@@ -93,16 +95,16 @@ pub enum VersionedPostUpgradeData {
 impl<
 		const FROM: u16,
 		const TO: u16,
-		Inner: OnRuntimeUpgrade,
+		Inner: crate::traits::OnRuntimeUpgrade,
 		Pallet: GetStorageVersion<CurrentStorageVersion = StorageVersion> + PalletInfoAccess,
 		DbWeight: Get<RuntimeDbWeight>,
-	> OnRuntimeUpgrade for VersionedRuntimeUpgrade<FROM, TO, Inner, Pallet, DbWeight>
+	> crate::traits::OnRuntimeUpgrade for VersionedRuntimeUpgrade<FROM, TO, Inner, Pallet, DbWeight>
 {
 	/// Executes pre_upgrade if the migration will run, and wraps the pre_upgrade bytes in
 	/// [`VersionedPostUpgradeData`] before passing them to post_upgrade, so it knows whether the
 	/// migration ran or not.
 	#[cfg(feature = "try-runtime")]
-	fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
+	fn pre_upgrade() -> Result<sp_std::vec::Vec<u8>, sp_runtime::TryRuntimeError> {
 		use codec::Encode;
 		let on_chain_version = Pallet::on_chain_storage_version();
 		if on_chain_version == FROM {
@@ -152,7 +154,7 @@ impl<
 	/// the migration ran, and [`VersionedPostUpgradeData::Noop`] otherwise.
 	#[cfg(feature = "try-runtime")]
 	fn post_upgrade(
-		versioned_post_upgrade_data_bytes: Vec<u8>,
+		versioned_post_upgrade_data_bytes: sp_std::vec::Vec<u8>,
 	) -> Result<(), sp_runtime::TryRuntimeError> {
 		use codec::DecodeAll;
 		match <VersionedPostUpgradeData>::decode_all(&mut &versioned_post_upgrade_data_bytes[..])
@@ -321,7 +323,7 @@ impl<P: Get<&'static str>, DbWeight: Get<RuntimeDbWeight>> frame_support::traits
 	}
 
 	#[cfg(feature = "try-runtime")]
-	fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
+	fn pre_upgrade() -> Result<sp_std::vec::Vec<u8>, sp_runtime::TryRuntimeError> {
 		use crate::storage::unhashed::contains_prefixed_key;
 
 		let hashed_prefix = twox_128(P::get().as_bytes());
@@ -332,11 +334,11 @@ impl<P: Get<&'static str>, DbWeight: Get<RuntimeDbWeight>> frame_support::traits
 				P::get()
 			),
 		};
-		Ok(Vec::new())
+		Ok(sp_std::vec::Vec::new())
 	}
 
 	#[cfg(feature = "try-runtime")]
-	fn post_upgrade(_state: Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
+	fn post_upgrade(_state: sp_std::vec::Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
 		use crate::storage::unhashed::contains_prefixed_key;
 
 		let hashed_prefix = twox_128(P::get().as_bytes());