From adba2c937084718c53627f25e8cc386eecfe030c Mon Sep 17 00:00:00 2001 From: Liam Aharon Date: Thu, 13 Jul 2023 20:08:45 +1000 Subject: [PATCH] Fix Society v2 migration (#14421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix society v2 migration * Update frame/society/src/migrations.rs * Update frame/society/src/migrations.rs Co-authored-by: Bastian Köcher * Update frame/society/src/migrations.rs Co-authored-by: Bastian Köcher * 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 * 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 --- frame/society/Cargo.toml | 4 ++ frame/society/src/lib.rs | 14 +++--- frame/society/src/migrations.rs | 79 ++++++++++++++++++++++----------- frame/society/src/tests.rs | 2 +- frame/support/src/migrations.rs | 36 ++++++++------- 5 files changed, 85 insertions(+), 50 deletions(-) diff --git a/frame/society/Cargo.toml b/frame/society/Cargo.toml index 2ef5dfad78c6e..7f3e55e339fe0 100644 --- a/frame/society/Cargo.toml +++ b/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", diff --git a/frame/society/src/lib.rs b/frame/society/src/lib.rs index 63d90a0703c4d..3d4b9323246d8 100644 --- a/frame/society/src/lib.rs +++ b/frame/society/src/lib.rs @@ -466,12 +466,12 @@ pub struct GroupParams { pub type GroupParamsFor = GroupParams>; +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, I: 'static> Pallet { 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, I: 'static> Pallet { 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, I: 'static> Pallet { 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) } diff --git a/frame/society/src/migrations.rs b/frame/society/src/migrations.rs index bd590f9b18770..404b1e5fd2cb9 100644 --- a/frame/society/src/migrations.rs +++ b/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, I: 'static, PastPayouts>( +pub struct VersionUncheckedMigrateToV2, I: 'static, PastPayouts>( sp_std::marker::PhantomData<(T, I, PastPayouts)>, ); @@ -36,12 +36,10 @@ impl< T: Config, I: Instance + 'static, PastPayouts: Get::AccountId, BalanceOf)>>, - > OnRuntimeUpgrade for MigrateToV2 + > OnRuntimeUpgrade for VersionUncheckedMigrateToV2 { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, TryRuntimeError> { - ensure!(can_migrate::(), "pallet_society: already upgraded"); - let current = Pallet::::current_storage_version(); let onchain = Pallet::::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::::current_storage_version(); let onchain = Pallet::::on_chain_storage_version(); - if current == 2 && onchain == 0 { - from_original::(&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::(&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 = + frame_support::migrations::VersionedRuntimeUpgrade< + 0, + 2, + VersionUncheckedMigrateToV2, + crate::pallet::Pallet, + ::DbWeight, + >; + pub(crate) mod old { use super::*; use frame_support::storage_alias; @@ -180,10 +191,6 @@ pub(crate) mod old { StorageMap, Twox64Concat, ::AccountId, Vote>; } -pub fn can_migrate, I: Instance + 'static>() -> bool { - old::Members::::exists() -} - /// Will panic if there are any inconsistencies in the pallet's state or old keys remaining. pub fn assert_internal_consistency, I: Instance + 'static>() { // Check all members are valid data. @@ -235,15 +242,7 @@ pub fn assert_internal_consistency, I: Instance + 'static>() { pub fn from_original, I: Instance + 'static>( past_payouts: &mut [(::AccountId, BalanceOf)], -) -> 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::::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 { // Migrate Bids from old::Bids (just a trunctation). Bids::::put(BoundedVec::<_, T::MaxBids>::truncate_from(old::Bids::::take())); @@ -281,6 +280,36 @@ pub fn from_original, I: Instance + 'static>( let record = MemberRecord { index: member_count, rank: 0, strikes, vouching }; Members::::insert(&member, record); MemberByIndex::::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::::get().defensive_ok_or("founder must always be set")? && + member_count > 0 + { + let member_to_swap = MemberByIndex::::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::::swap(0, member_count); + // Update the indicies of the swapped member MemberRecords. + Members::::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::::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::::put(member_count); @@ -317,7 +346,7 @@ pub fn from_original, I: Instance + 'static>( old::Defender::::kill(); let _ = old::DefenderVotes::::clear(u32::MAX, None); - T::BlockWeights::get().max_block + Ok(T::BlockWeights::get().max_block) } pub fn from_raw_past_payouts, I: Instance + 'static>( diff --git a/frame/society/src/tests.rs b/frame/society/src/tests.rs index 4a90ad52112d8..ea2afef3b32b5 100644 --- a/frame/society/src/tests.rs +++ b/frame/society/src/tests.rs @@ -77,7 +77,7 @@ fn migration_works() { .collect::>(); old::Bids::::put(bids); - migrations::from_original::(&mut [][..]); + migrations::from_original::(&mut [][..]).expect("migration failed"); migrations::assert_internal_consistency::(); assert_eq!( diff --git a/frame/support/src/migrations.rs b/frame/support/src/migrations.rs index 889439907efd2..9ba22d3e15404 100644 --- a/frame/support/src/migrations.rs +++ b/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 = -/// VersionedRuntimeUpgrade<5, 6, VersionUncheckedMigrateV5ToV6, Pallet, DbWeight>; +/// pub type VersionCheckedMigrateV5ToV6 = +/// VersionedRuntimeUpgrade< +/// 5, +/// 6, +/// VersionUncheckedMigrateV5ToV6, +/// crate::pallet::Pallet, +/// ::DbWeight +/// >; /// /// // Migrations tuple to pass to the Executive pallet: /// pub type Migrations = ( /// // other migrations... -/// VersionCheckedMigrateV5ToV6, +/// VersionCheckedMigrateV5ToV6, /// // other migrations... /// ); /// ``` @@ -78,7 +80,7 @@ pub struct VersionedRuntimeUpgrade), + MigrationExecuted(sp_std::vec::Vec), /// 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 + PalletInfoAccess, DbWeight: Get, - > OnRuntimeUpgrade for VersionedRuntimeUpgrade + > crate::traits::OnRuntimeUpgrade for VersionedRuntimeUpgrade { /// 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, sp_runtime::TryRuntimeError> { + fn pre_upgrade() -> Result, 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, + versioned_post_upgrade_data_bytes: sp_std::vec::Vec, ) -> Result<(), sp_runtime::TryRuntimeError> { use codec::DecodeAll; match ::decode_all(&mut &versioned_post_upgrade_data_bytes[..]) @@ -321,7 +323,7 @@ impl, DbWeight: Get> frame_support::traits } #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { use crate::storage::unhashed::contains_prefixed_key; let hashed_prefix = twox_128(P::get().as_bytes()); @@ -332,11 +334,11 @@ impl, DbWeight: Get> frame_support::traits P::get() ), }; - Ok(Vec::new()) + Ok(sp_std::vec::Vec::new()) } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { + fn post_upgrade(_state: sp_std::vec::Vec) -> Result<(), sp_runtime::TryRuntimeError> { use crate::storage::unhashed::contains_prefixed_key; let hashed_prefix = twox_128(P::get().as_bytes());