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

Move type Migrations to Config #14309

Merged
merged 12 commits into from
Jun 9, 2023
6 changes: 6 additions & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ use frame_system::{
pub use node_primitives::{AccountId, Signature};
use node_primitives::{AccountIndex, Balance, BlockNumber, Hash, Index, Moment};
use pallet_asset_conversion::{NativeOrAssetId, NativeOrAssetIdConverter};
#[cfg(feature = "runtime-benchmarks")]
use pallet_contracts::NoopMigration;
use pallet_election_provider_multi_phase::SolutionAccuracyOf;
use pallet_im_online::sr25519::AuthorityId as ImOnlineId;
use pallet_nfts::PalletFeatures;
Expand Down Expand Up @@ -1240,6 +1242,10 @@ impl pallet_contracts::Config for Runtime {
type MaxStorageKeyLen = ConstU32<128>;
type UnsafeUnstableInterface = ConstBool<false>;
type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>;
#[cfg(not(feature = "runtime-benchmarks"))]
type Migrations = ();
#[cfg(feature = "runtime-benchmarks")]
type Migrations = (NoopMigration<1>, NoopMigration<2>);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can add a quick comments to specify that these noop migrations are used for benchmarking the migration framework.

Copy link
Contributor

@agryaznov agryaznov Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the things used for tests & benches should not appear here. It's better to keep them where they were, behind a feature flag. That's being said the docs giving an example how to configure the Migrations seq in runtime Config, would be great to have.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A () should suffice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these NoopMigrations are used by benchmarks, so we should keep them here

#[pov_mode = Measured]
migrate {
StorageVersion::new(0).put::<Pallet<T>>();
<Migration::<T> as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade();
let origin: RawOrigin<<T as frame_system::Config>::AccountId> = RawOrigin::Signed(whitelisted_caller());
}: {
<Contracts<T>>::migrate(origin.into(), Weight::MAX).unwrap()
} verify {
assert_eq!(StorageVersion::get::<Pallet<T>>(), 1);
}

Copy link
Member

@ggwpez ggwpez Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then how does this look like in an actual runtime?
Could be as ugly as this...

Copy link
Contributor

@pgherveou pgherveou Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the things used for tests & benches should not appear here. It's better to keep them where they were, behind a feature flag. That's being said the docs giving an example how to configure the Migrations seq in runtime Config, would be great to have.

@agryaznov I think that's because that is part of the config, we can't avoid having it here. Compile flags worked before because the type was internal to the pallet.

Copy link
Contributor Author

@juangirini juangirini Jun 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then how does this look like in an actual runtime? Could be as ugly as this...

We could do something as ugly as that or as ugly as this (which was the initial approach)

/// Performs all necessary migrations based on `StorageVersion`.
#[cfg(not(feature = "runtime-benchmarks"))]
pub struct Migration<T: Config, M: MigrateSequence = T::Migrations>(PhantomData<(T, M)>);

/// Custom migration for running runtime-benchmarks and tests.
#[cfg(feature = "runtime-benchmarks")]
pub struct Migration<T: Config, M: MigrateSequence = (NoopMigration<1>, NoopMigration<2>)>(
	PhantomData<(T, M)>,
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how it is setup in @ggwpez 's link at least it makes it clear that these noopMigration are intended for benchmarks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have applied PG's suggested fix #14309 (comment)

juangirini marked this conversation as resolved.
Show resolved Hide resolved
}

impl pallet_sudo::Config for Runtime {
Expand Down
12 changes: 11 additions & 1 deletion frame/contracts/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,12 @@ mod address;
mod benchmarking;
mod exec;
mod gas;
mod migration;
mod schedule;
mod storage;
mod wasm;

pub mod chain_extension;
pub mod migration;
pub mod weights;

#[cfg(test)]
Expand Down Expand Up @@ -319,6 +319,16 @@ pub mod pallet {
/// The maximum length of the debug buffer in bytes.
#[pallet::constant]
type MaxDebugBufferLen: Get<u32>;

/// The sequence of migration steps that will be applied during a migration.
///
/// # Examples
/// ```
/// use pallet_contracts::migration::{v9, v10, v11};
/// # struct Runtime {};
/// type Migrations = (v9::Migration<Runtime>, v10::Migration<Runtime>, v11::Migration<Runtime>);
/// ```
type Migrations: MigrateSequence;
}

#[pallet::hooks]
Expand Down
134 changes: 61 additions & 73 deletions frame/contracts/src/migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,10 @@

//! Migration framework for pallets.

/// Macro to include all migration modules.
/// We only want to make these modules public when `runtime-benchmarks` is
/// enabled, so we can access migration code in benchmarks.
macro_rules! use_modules {
($($module:ident),*) => {
$(
#[cfg(feature = "runtime-benchmarks")]
pub mod $module;
#[cfg(not(feature = "runtime-benchmarks"))]
mod $module;
)*
};
}
pub mod v10;
pub mod v11;
pub mod v9;

use_modules!(v9, v10, v11);
use crate::{weights::WeightInfo, Config, Error, MigrationInProgress, Pallet, Weight, LOG_TARGET};
use codec::{Codec, Decode};
use frame_support::{
Expand All @@ -58,10 +47,6 @@ fn invalid_version(version: StorageVersion) -> ! {
/// The cursor used to store the state of the current migration step.
pub type Cursor = BoundedVec<u8, ConstU32<1024>>;

// In benchmark and tests we use noop migrations, to test and bench the migration framework itself.
#[cfg(not(any(feature = "runtime-benchmarks", test)))]
type Migrations<T> = (v9::Migration<T>, v10::Migration<T>, v11::Migration<T>);

/// IsFinished describes whether a migration is finished or not.
pub enum IsFinished {
Yes,
Expand Down Expand Up @@ -206,23 +191,16 @@ pub trait MigrateSequence: private::Sealed {
}

/// Performs all necessary migrations based on `StorageVersion`.
#[cfg(not(any(feature = "runtime-benchmarks", test)))]
pub struct Migration<T: Config, M: MigrateSequence = Migrations<T>>(PhantomData<(T, M)>);

/// Custom migration for running runtime-benchmarks and tests.
#[cfg(any(feature = "runtime-benchmarks", test))]
pub struct Migration<T: Config, M: MigrateSequence = (NoopMigration<1>, NoopMigration<2>)>(
PhantomData<(T, M)>,
);
juangirini marked this conversation as resolved.
Show resolved Hide resolved
juangirini marked this conversation as resolved.
Show resolved Hide resolved
pub struct Migration<T: Config>(PhantomData<T>);

#[cfg(feature = "try-runtime")]
impl<T: Config, M: MigrateSequence> Migration<T, M> {
impl<T: Config> Migration<T> {
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
fn run_all_steps() -> Result<(), TryRuntimeError> {
let mut weight = Weight::zero();
let name = <Pallet<T>>::name();
loop {
let in_progress_version = <Pallet<T>>::on_chain_storage_version() + 1;
let state = M::pre_upgrade_step(in_progress_version)?;
let state = T::Migrations::pre_upgrade_step(in_progress_version)?;
let (status, w) = Self::migrate(Weight::MAX);
weight.saturating_accrue(w);
log::info!(
Expand All @@ -231,7 +209,7 @@ impl<T: Config, M: MigrateSequence> Migration<T, M> {
in_progress_version,
weight
);
M::post_upgrade_step(in_progress_version, state)?;
T::Migrations::post_upgrade_step(in_progress_version, state)?;
if matches!(status, MigrateResult::Completed) {
break
}
Expand All @@ -243,7 +221,7 @@ impl<T: Config, M: MigrateSequence> Migration<T, M> {
}
}

impl<T: Config, M: MigrateSequence> OnRuntimeUpgrade for Migration<T, M> {
impl<T: Config> OnRuntimeUpgrade for Migration<T> {
fn on_runtime_upgrade() -> Weight {
let name = <Pallet<T>>::name();
let latest_version = <Pallet<T>>::current_storage_version();
Expand Down Expand Up @@ -274,7 +252,7 @@ impl<T: Config, M: MigrateSequence> OnRuntimeUpgrade for Migration<T, M> {
"{name}: Upgrading storage from {storage_version:?} to {latest_version:?}.",
);

let cursor = M::new(storage_version + 1);
let cursor = T::Migrations::new(storage_version + 1);
MigrationInProgress::<T>::set(Some(cursor));

#[cfg(feature = "try-runtime")]
Expand All @@ -295,11 +273,14 @@ impl<T: Config, M: MigrateSequence> OnRuntimeUpgrade for Migration<T, M> {
target: LOG_TARGET,
"{}: Range supported {:?}, range requested {:?}",
<Pallet<T>>::name(),
M::VERSION_RANGE,
T::Migrations::VERSION_RANGE,
(storage_version, target_version)
);

ensure!(M::is_upgrade_supported(storage_version, target_version), "Unsupported upgrade");
ensure!(
T::Migrations::is_upgrade_supported(storage_version, target_version),
"Unsupported upgrade"
);
Ok(Default::default())
}
}
Expand All @@ -324,11 +305,11 @@ pub enum StepResult {
Completed { steps_done: u32 },
}

impl<T: Config, M: MigrateSequence> Migration<T, M> {
/// Verify that each migration's step of the MigrateSequence fits into `Cursor`.
impl<T: Config> Migration<T> {
/// Verify that each migration's step of the [`T::Migrations`] sequence fits into `Cursor`.
pub(crate) fn integrity_test() {
let max_weight = <T as frame_system::Config>::BlockWeights::get().max_block;
M::integrity_test(max_weight)
T::Migrations::integrity_test(max_weight)
}

/// Migrate
Expand Down Expand Up @@ -357,33 +338,36 @@ impl<T: Config, M: MigrateSequence> Migration<T, M> {
in_progress_version,
);

let result =
match M::steps(in_progress_version, cursor_before.as_ref(), &mut weight_left) {
StepResult::InProgress { cursor, steps_done } => {
*progress = Some(cursor);
let result = match T::Migrations::steps(
in_progress_version,
cursor_before.as_ref(),
&mut weight_left,
) {
StepResult::InProgress { cursor, steps_done } => {
*progress = Some(cursor);
MigrateResult::InProgress { steps_done }
},
StepResult::Completed { steps_done } => {
in_progress_version.put::<Pallet<T>>();
if <Pallet<T>>::current_storage_version() != in_progress_version {
log::info!(
target: LOG_TARGET,
"{name}: Next migration is {:?},",
in_progress_version + 1
);
*progress = Some(T::Migrations::new(in_progress_version + 1));
MigrateResult::InProgress { steps_done }
},
StepResult::Completed { steps_done } => {
in_progress_version.put::<Pallet<T>>();
if <Pallet<T>>::current_storage_version() != in_progress_version {
log::info!(
target: LOG_TARGET,
"{name}: Next migration is {:?},",
in_progress_version + 1
);
*progress = Some(M::new(in_progress_version + 1));
MigrateResult::InProgress { steps_done }
} else {
log::info!(
target: LOG_TARGET,
"{name}: All migrations done. At version {:?},",
in_progress_version
);
*progress = None;
MigrateResult::Completed
}
},
};
} else {
log::info!(
target: LOG_TARGET,
"{name}: All migrations done. At version {:?},",
in_progress_version
);
*progress = None;
MigrateResult::Completed
}
},
};

(result, weight_limit.saturating_sub(weight_left))
})
Expand Down Expand Up @@ -527,11 +511,14 @@ mod test {

#[test]
fn is_upgrade_supported_works() {
type M = (MockMigration<9>, MockMigration<10>, MockMigration<11>);
type Migrations = (MockMigration<9>, MockMigration<10>, MockMigration<11>);

[(1, 1), (8, 11), (9, 11)].into_iter().for_each(|(from, to)| {
assert!(
M::is_upgrade_supported(StorageVersion::new(from), StorageVersion::new(to)),
Migrations::is_upgrade_supported(
StorageVersion::new(from),
StorageVersion::new(to)
),
"{} -> {} is supported",
from,
to
Expand All @@ -540,7 +527,10 @@ mod test {

[(1, 0), (0, 3), (7, 11), (8, 10)].into_iter().for_each(|(from, to)| {
assert!(
!M::is_upgrade_supported(StorageVersion::new(from), StorageVersion::new(to)),
!Migrations::is_upgrade_supported(
StorageVersion::new(from),
StorageVersion::new(to)
),
"{} -> {} is not supported",
from,
to
Expand All @@ -550,27 +540,26 @@ mod test {

#[test]
fn steps_works() {
type M = (MockMigration<2>, MockMigration<3>);
type Migrations = (MockMigration<2>, MockMigration<3>);
let version = StorageVersion::new(2);
let mut cursor = M::new(version);
let mut cursor = Migrations::new(version);

let mut weight = Weight::from_all(2);
let result = M::steps(version, &cursor, &mut weight);
let result = Migrations::steps(version, &cursor, &mut weight);
cursor = vec![1u8, 0].try_into().unwrap();
assert_eq!(result, StepResult::InProgress { cursor: cursor.clone(), steps_done: 1 });
assert_eq!(weight, Weight::from_all(1));

let mut weight = Weight::from_all(2);
assert_eq!(
M::steps(version, &cursor, &mut weight),
Migrations::steps(version, &cursor, &mut weight),
StepResult::Completed { steps_done: 1 }
);
}

#[test]
fn no_migration_in_progress_works() {
type M = (MockMigration<1>, MockMigration<2>);
type TestMigration = Migration<Test, M>;
type TestMigration = Migration<Test>;

ExtBuilder::default().build().execute_with(|| {
assert_eq!(StorageVersion::get::<Pallet<Test>>(), 2);
Expand All @@ -580,8 +569,7 @@ mod test {

#[test]
fn migration_works() {
type M = (MockMigration<1>, MockMigration<2>);
type TestMigration = Migration<Test, M>;
type TestMigration = Migration<Test>;

ExtBuilder::default().set_storage_version(0).build().execute_with(|| {
assert_eq!(StorageVersion::get::<Pallet<Test>>(), 0);
Expand Down
5 changes: 3 additions & 2 deletions frame/contracts/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ use crate::{
wasm::{Determinism, PrefabWasmModule, ReturnCode as RuntimeReturnCode},
weights::WeightInfo,
BalanceOf, Code, CodeStorage, CollectEvents, Config, ContractInfo, ContractInfoOf, DebugInfo,
DefaultAddressGenerator, DeletionQueueCounter, Error, MigrationInProgress, Origin, Pallet,
Schedule,
DefaultAddressGenerator, DeletionQueueCounter, Error, MigrationInProgress, NoopMigration,
Origin, Pallet, Schedule,
};
use assert_matches::assert_matches;
use codec::Encode;
Expand Down Expand Up @@ -428,6 +428,7 @@ impl Config for Test {
type MaxStorageKeyLen = ConstU32<128>;
type UnsafeUnstableInterface = UnstableInterface;
type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>;
type Migrations = (NoopMigration<1>, NoopMigration<2>);
}

pub const ALICE: AccountId32 = AccountId32::new([1u8; 32]);
Expand Down