Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
Fix Society v2 migration (#14421)
Browse files Browse the repository at this point in the history 
* 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>
  • Loading branch information
@liamaharon @kianenigma
@bkchr @juangirini
3 people authored and juangirini committed Jul 14, 2023
1 parent de849d2 commit adba2c9
Show file tree
Hide file tree
Showing 5 changed files with 85 additions and 50 deletions.
4 changes: 4 additions & 0 deletions frame/society/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
14 changes: 7 additions & 7 deletions frame/society/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand All @@ -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.
///
Expand All @@ -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)
}
Expand Down
79 changes: 54 additions & 25 deletions frame/society/src/migrations.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -28,20 +28,18 @@ 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)>,
);

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");
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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()));

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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>(
Expand Down
2 changes: 1 addition & 1 deletion frame/society/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
36 changes: 19 additions & 17 deletions frame/support/src/migrations.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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...
/// );
/// ```
Expand All @@ -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,
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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[..])
Expand Down Expand Up @@ -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());
Expand All @@ -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());
Expand Down

0 comments on commit adba2c9

Please sign in to comment.