From 5c1452917a195413ce28137ef9176937865e72da Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 6 Jun 2023 13:13:05 +0200 Subject: [PATCH 01/42] move migrate sequence to config --- bin/node/runtime/src/lib.rs | 4 +++- frame/contracts/src/benchmarking/mod.rs | 10 ++++----- frame/contracts/src/lib.rs | 29 ++++++++++++++----------- frame/contracts/src/migration.rs | 25 +++++++-------------- frame/contracts/src/tests.rs | 5 +++-- 5 files changed, 35 insertions(+), 38 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 6dc9841f6b44f..33ffe6d4f32cc 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -55,6 +55,7 @@ use frame_system::{ pub use node_primitives::{AccountId, Signature}; use node_primitives::{AccountIndex, Balance, BlockNumber, Hash, Index, Moment}; use pallet_asset_conversion::{NativeOrAssetId, NativeOrAssetIdConverter}; +use pallet_contracts::migration::{v10, v11, v9}; use pallet_election_provider_multi_phase::SolutionAccuracyOf; use pallet_im_online::sr25519::AuthorityId as ImOnlineId; use pallet_nfts::PalletFeatures; @@ -1240,6 +1241,7 @@ impl pallet_contracts::Config for Runtime { type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = ConstBool; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; + type Migrations = (v9::Migration, v10::Migration, v11::Migration); } impl pallet_sudo::Config for Runtime { @@ -1981,7 +1983,7 @@ pub type Executive = frame_executive::Executive< type Migrations = ( pallet_nomination_pools::migration::v2::MigrateToV2, pallet_alliance::migration::Migration, - pallet_contracts::Migration, + pallet_contracts::Migration::Migrations>, ); type EventRecord = frame_system::EventRecord< diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index b98c05f2503c2..27b9aadeed675 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -272,7 +272,7 @@ benchmarks! { migration_noop { assert_eq!(StorageVersion::get::>(), 2); }: { - Migration::::migrate(Weight::MAX) + Migration::::migrate(Weight::MAX) } verify { assert_eq!(StorageVersion::get::>(), 2); } @@ -281,7 +281,7 @@ benchmarks! { #[pov_mode = Measured] migrate { StorageVersion::new(0).put::>(); - as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade(); + as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade(); let origin: RawOrigin<::AccountId> = RawOrigin::Signed(whitelisted_caller()); }: { >::migrate(origin.into(), Weight::MAX).unwrap() @@ -294,7 +294,7 @@ benchmarks! { on_runtime_upgrade_noop { assert_eq!(StorageVersion::get::>(), 2); }: { - as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() + as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() } verify { assert!(MigrationInProgress::::get().is_none()); } @@ -306,7 +306,7 @@ benchmarks! { let v = vec![42u8].try_into().ok(); MigrationInProgress::::set(v.clone()); }: { - as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() + as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() } verify { assert!(MigrationInProgress::::get().is_some()); assert_eq!(MigrationInProgress::::get(), v); @@ -317,7 +317,7 @@ benchmarks! { on_runtime_upgrade { StorageVersion::new(0).put::>(); }: { - as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() + as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() } verify { assert!(MigrationInProgress::::get().is_some()); } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 779b713795b64..fa0a95d2ffebb 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -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)] @@ -319,6 +319,9 @@ pub mod pallet { /// The maximum length of the debug buffer in bytes. #[pallet::constant] type MaxDebugBufferLen: Get; + + /// The sequence of migrations applied. + type Migrations: MigrateSequence; } #[pallet::hooks] @@ -326,7 +329,7 @@ pub mod pallet { fn on_idle(_block: T::BlockNumber, remaining_weight: Weight) -> Weight { use migration::MigrateResult::*; - let (result, weight) = Migration::::migrate(remaining_weight); + let (result, weight) = Migration::::migrate(remaining_weight); let remaining_weight = remaining_weight.saturating_sub(weight); if !matches!(result, Completed | NoMigrationInProgress) { @@ -338,7 +341,7 @@ pub mod pallet { } fn integrity_test() { - Migration::::integrity_test(); + Migration::::integrity_test(); // Total runtime memory limit let max_runtime_mem: u32 = T::Schedule::get().limits.runtime_memory; @@ -518,7 +521,7 @@ pub mod pallet { storage_deposit_limit: Option< as codec::HasCompact>::Type>, determinism: Determinism, ) -> DispatchResult { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let origin = ensure_signed(origin)?; Self::bare_upload_code(origin, code, storage_deposit_limit.map(Into::into), determinism) .map(|_| ()) @@ -534,7 +537,7 @@ pub mod pallet { origin: OriginFor, code_hash: CodeHash, ) -> DispatchResultWithPostInfo { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let origin = ensure_signed(origin)?; >::remove(&origin, code_hash)?; // we waive the fee because removing unused code is beneficial @@ -558,7 +561,7 @@ pub mod pallet { dest: AccountIdLookupOf, code_hash: CodeHash, ) -> DispatchResult { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; ensure_root(origin)?; let dest = T::Lookup::lookup(dest)?; >::try_mutate(&dest, |contract| { @@ -608,7 +611,7 @@ pub mod pallet { storage_deposit_limit: Option< as codec::HasCompact>::Type>, data: Vec, ) -> DispatchResultWithPostInfo { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let common = CommonInput { origin: Origin::from_runtime_origin(origin)?, value, @@ -668,7 +671,7 @@ pub mod pallet { data: Vec, salt: Vec, ) -> DispatchResultWithPostInfo { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let code_len = code.len() as u32; let data_len = data.len() as u32; let salt_len = salt.len() as u32; @@ -711,7 +714,7 @@ pub mod pallet { data: Vec, salt: Vec, ) -> DispatchResultWithPostInfo { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let data_len = data.len() as u32; let salt_len = salt.len() as u32; let common = CommonInput { @@ -746,7 +749,7 @@ pub mod pallet { ensure_signed(origin)?; let weight_limit = weight_limit.saturating_add(T::WeightInfo::migrate()); - let (result, weight) = Migration::::migrate(weight_limit); + let (result, weight) = Migration::::migrate(weight_limit); match result { Completed => @@ -1272,7 +1275,7 @@ impl Invokable for InstantiateInput { macro_rules! ensure_no_migration_in_progress { () => { - if Migration::::in_progress() { + if Migration::::in_progress() { return ContractResult { gas_consumed: Zero::zero(), gas_required: Zero::zero(), @@ -1412,7 +1415,7 @@ impl Pallet { storage_deposit_limit: Option>, determinism: Determinism, ) -> CodeUploadResult, BalanceOf> { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let schedule = T::Schedule::get(); let module = PrefabWasmModule::from_code( code, @@ -1433,7 +1436,7 @@ impl Pallet { /// Query storage of a specified contract under a specified key. pub fn get_storage(address: T::AccountId, key: Vec) -> GetStorageResult { - if Migration::::in_progress() { + if Migration::::in_progress() { return Err(ContractAccessError::MigrationInProgress) } let contract_info = diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index a75d5cc1a1f4d..c13c5f4681f50 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -18,15 +18,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; )* }; } @@ -58,10 +53,6 @@ fn invalid_version(version: StorageVersion) -> ! { /// The cursor used to store the state of the current migration step. pub type Cursor = BoundedVec>; -// 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 = (v9::Migration, v10::Migration, v11::Migration); - /// IsFinished describes whether a migration is finished or not. pub enum IsFinished { Yes, @@ -206,14 +197,14 @@ pub trait MigrateSequence: private::Sealed { } /// Performs all necessary migrations based on `StorageVersion`. -#[cfg(not(any(feature = "runtime-benchmarks", test)))] -pub struct Migration>(PhantomData<(T, M)>); - -/// Custom migration for running runtime-benchmarks and tests. -#[cfg(any(feature = "runtime-benchmarks", test))] -pub struct Migration, NoopMigration<2>)>( - PhantomData<(T, M)>, -); +// #[cfg(not(any(feature = "runtime-benchmarks", test)))] +pub struct Migration(PhantomData<(T, M)>); + +// /// Custom migration for running runtime-benchmarks and tests. +// #[cfg(any(feature = "runtime-benchmarks", test))] +// pub struct Migration, NoopMigration<2>)>( +// PhantomData<(T, M)>, +// ); #[cfg(feature = "try-runtime")] impl Migration { diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index d23a238dcd158..9425f25adf4b8 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -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; @@ -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]); From e8c493cecdef218476a5a6ce78ce678140996132 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 6 Jun 2023 13:19:31 +0200 Subject: [PATCH 02/42] remove commented out code --- frame/contracts/src/migration.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index c13c5f4681f50..ec8a67c755c48 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -197,15 +197,8 @@ pub trait MigrateSequence: private::Sealed { } /// Performs all necessary migrations based on `StorageVersion`. -// #[cfg(not(any(feature = "runtime-benchmarks", test)))] pub struct Migration(PhantomData<(T, M)>); -// /// Custom migration for running runtime-benchmarks and tests. -// #[cfg(any(feature = "runtime-benchmarks", test))] -// pub struct Migration, NoopMigration<2>)>( -// PhantomData<(T, M)>, -// ); - #[cfg(feature = "try-runtime")] impl Migration { fn run_all_steps() -> Result<(), TryRuntimeError> { From 0e629301500114775c71e7e01cf039ed0c5ebb2e Mon Sep 17 00:00:00 2001 From: Juan Date: Tue, 6 Jun 2023 13:41:50 +0200 Subject: [PATCH 03/42] Update frame/contracts/src/lib.rs Co-authored-by: PG Herveou --- frame/contracts/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index fa0a95d2ffebb..118ccca94206c 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -320,7 +320,7 @@ pub mod pallet { #[pallet::constant] type MaxDebugBufferLen: Get; - /// The sequence of migrations applied. + /// The sequence of migration steps that will be applied during a migration. type Migrations: MigrateSequence; } From 14a71755cfb5546c4e78f5fdd6cf54ac1134d9b7 Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 6 Jun 2023 14:01:59 +0200 Subject: [PATCH 04/42] remove Migrations generic --- bin/node/runtime/src/lib.rs | 2 +- frame/contracts/src/benchmarking/mod.rs | 10 +- frame/contracts/src/lib.rs | 24 ++--- frame/contracts/src/migration.rs | 118 ++++++++++++------------ 4 files changed, 79 insertions(+), 75 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 33ffe6d4f32cc..dd2accb086a43 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1983,7 +1983,7 @@ pub type Executive = frame_executive::Executive< type Migrations = ( pallet_nomination_pools::migration::v2::MigrateToV2, pallet_alliance::migration::Migration, - pallet_contracts::Migration::Migrations>, + pallet_contracts::Migration, ); type EventRecord = frame_system::EventRecord< diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index 27b9aadeed675..b98c05f2503c2 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -272,7 +272,7 @@ benchmarks! { migration_noop { assert_eq!(StorageVersion::get::>(), 2); }: { - Migration::::migrate(Weight::MAX) + Migration::::migrate(Weight::MAX) } verify { assert_eq!(StorageVersion::get::>(), 2); } @@ -281,7 +281,7 @@ benchmarks! { #[pov_mode = Measured] migrate { StorageVersion::new(0).put::>(); - as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade(); + as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade(); let origin: RawOrigin<::AccountId> = RawOrigin::Signed(whitelisted_caller()); }: { >::migrate(origin.into(), Weight::MAX).unwrap() @@ -294,7 +294,7 @@ benchmarks! { on_runtime_upgrade_noop { assert_eq!(StorageVersion::get::>(), 2); }: { - as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() + as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() } verify { assert!(MigrationInProgress::::get().is_none()); } @@ -306,7 +306,7 @@ benchmarks! { let v = vec![42u8].try_into().ok(); MigrationInProgress::::set(v.clone()); }: { - as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() + as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() } verify { assert!(MigrationInProgress::::get().is_some()); assert_eq!(MigrationInProgress::::get(), v); @@ -317,7 +317,7 @@ benchmarks! { on_runtime_upgrade { StorageVersion::new(0).put::>(); }: { - as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() + as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade() } verify { assert!(MigrationInProgress::::get().is_some()); } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 118ccca94206c..59c9ea23ed38b 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -329,7 +329,7 @@ pub mod pallet { fn on_idle(_block: T::BlockNumber, remaining_weight: Weight) -> Weight { use migration::MigrateResult::*; - let (result, weight) = Migration::::migrate(remaining_weight); + let (result, weight) = Migration::::migrate(remaining_weight); let remaining_weight = remaining_weight.saturating_sub(weight); if !matches!(result, Completed | NoMigrationInProgress) { @@ -341,7 +341,7 @@ pub mod pallet { } fn integrity_test() { - Migration::::integrity_test(); + Migration::::integrity_test(); // Total runtime memory limit let max_runtime_mem: u32 = T::Schedule::get().limits.runtime_memory; @@ -521,7 +521,7 @@ pub mod pallet { storage_deposit_limit: Option< as codec::HasCompact>::Type>, determinism: Determinism, ) -> DispatchResult { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let origin = ensure_signed(origin)?; Self::bare_upload_code(origin, code, storage_deposit_limit.map(Into::into), determinism) .map(|_| ()) @@ -537,7 +537,7 @@ pub mod pallet { origin: OriginFor, code_hash: CodeHash, ) -> DispatchResultWithPostInfo { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let origin = ensure_signed(origin)?; >::remove(&origin, code_hash)?; // we waive the fee because removing unused code is beneficial @@ -561,7 +561,7 @@ pub mod pallet { dest: AccountIdLookupOf, code_hash: CodeHash, ) -> DispatchResult { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; ensure_root(origin)?; let dest = T::Lookup::lookup(dest)?; >::try_mutate(&dest, |contract| { @@ -611,7 +611,7 @@ pub mod pallet { storage_deposit_limit: Option< as codec::HasCompact>::Type>, data: Vec, ) -> DispatchResultWithPostInfo { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let common = CommonInput { origin: Origin::from_runtime_origin(origin)?, value, @@ -671,7 +671,7 @@ pub mod pallet { data: Vec, salt: Vec, ) -> DispatchResultWithPostInfo { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let code_len = code.len() as u32; let data_len = data.len() as u32; let salt_len = salt.len() as u32; @@ -714,7 +714,7 @@ pub mod pallet { data: Vec, salt: Vec, ) -> DispatchResultWithPostInfo { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let data_len = data.len() as u32; let salt_len = salt.len() as u32; let common = CommonInput { @@ -749,7 +749,7 @@ pub mod pallet { ensure_signed(origin)?; let weight_limit = weight_limit.saturating_add(T::WeightInfo::migrate()); - let (result, weight) = Migration::::migrate(weight_limit); + let (result, weight) = Migration::::migrate(weight_limit); match result { Completed => @@ -1275,7 +1275,7 @@ impl Invokable for InstantiateInput { macro_rules! ensure_no_migration_in_progress { () => { - if Migration::::in_progress() { + if Migration::::in_progress() { return ContractResult { gas_consumed: Zero::zero(), gas_required: Zero::zero(), @@ -1415,7 +1415,7 @@ impl Pallet { storage_deposit_limit: Option>, determinism: Determinism, ) -> CodeUploadResult, BalanceOf> { - Migration::::ensure_migrated()?; + Migration::::ensure_migrated()?; let schedule = T::Schedule::get(); let module = PrefabWasmModule::from_code( code, @@ -1436,7 +1436,7 @@ impl Pallet { /// Query storage of a specified contract under a specified key. pub fn get_storage(address: T::AccountId, key: Vec) -> GetStorageResult { - if Migration::::in_progress() { + if Migration::::in_progress() { return Err(ContractAccessError::MigrationInProgress) } let contract_info = diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index ec8a67c755c48..ad85da0477148 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -17,16 +17,10 @@ //! Migration framework for pallets. -/// Macro to include all migration modules. -macro_rules! use_modules { - ($($module:ident),*) => { - $( - pub 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::{ @@ -197,16 +191,16 @@ pub trait MigrateSequence: private::Sealed { } /// Performs all necessary migrations based on `StorageVersion`. -pub struct Migration(PhantomData<(T, M)>); +pub struct Migration(PhantomData); #[cfg(feature = "try-runtime")] -impl Migration { +impl Migration { fn run_all_steps() -> Result<(), TryRuntimeError> { let mut weight = Weight::zero(); let name = >::name(); loop { let in_progress_version = >::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!( @@ -215,7 +209,7 @@ impl Migration { 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 } @@ -227,7 +221,7 @@ impl Migration { } } -impl OnRuntimeUpgrade for Migration { +impl OnRuntimeUpgrade for Migration { fn on_runtime_upgrade() -> Weight { let name = >::name(); let latest_version = >::current_storage_version(); @@ -258,7 +252,7 @@ impl OnRuntimeUpgrade for Migration { "{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::::set(Some(cursor)); #[cfg(feature = "try-runtime")] @@ -279,11 +273,14 @@ impl OnRuntimeUpgrade for Migration { target: LOG_TARGET, "{}: Range supported {:?}, range requested {:?}", >::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()) } } @@ -308,11 +305,11 @@ pub enum StepResult { Completed { steps_done: u32 }, } -impl Migration { - /// Verify that each migration's step of the MigrateSequence fits into `Cursor`. +impl Migration { + /// Verify that each migration's step of the [`T::Migrations`] sequence fits into `Cursor`. pub(crate) fn integrity_test() { let max_weight = ::BlockWeights::get().max_block; - M::integrity_test(max_weight) + T::Migrations::integrity_test(max_weight) } /// Migrate @@ -341,33 +338,36 @@ impl Migration { 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::>(); + if >::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::>(); - if >::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)) }) @@ -511,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 @@ -524,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 @@ -534,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; + type TestMigration = Migration; ExtBuilder::default().build().execute_with(|| { assert_eq!(StorageVersion::get::>(), 2); @@ -564,8 +569,7 @@ mod test { #[test] fn migration_works() { - type M = (MockMigration<1>, MockMigration<2>); - type TestMigration = Migration; + type TestMigration = Migration; ExtBuilder::default().set_storage_version(0).build().execute_with(|| { assert_eq!(StorageVersion::get::>(), 0); From 2601d79a37c70fde3d81e891957d9b5d0f946a1c Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 6 Jun 2023 17:11:59 +0200 Subject: [PATCH 05/42] make runtime use noop migrations --- bin/node/runtime/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index dd2accb086a43..dd4b14b1889b5 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -55,7 +55,7 @@ use frame_system::{ pub use node_primitives::{AccountId, Signature}; use node_primitives::{AccountIndex, Balance, BlockNumber, Hash, Index, Moment}; use pallet_asset_conversion::{NativeOrAssetId, NativeOrAssetIdConverter}; -use pallet_contracts::migration::{v10, v11, v9}; +use pallet_contracts::NoopMigration; use pallet_election_provider_multi_phase::SolutionAccuracyOf; use pallet_im_online::sr25519::AuthorityId as ImOnlineId; use pallet_nfts::PalletFeatures; @@ -1241,7 +1241,7 @@ impl pallet_contracts::Config for Runtime { type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = ConstBool; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; - type Migrations = (v9::Migration, v10::Migration, v11::Migration); + type Migrations = (NoopMigration<1>, NoopMigration<2>); } impl pallet_sudo::Config for Runtime { From e88854a39dbbd133b647b0b5ca9ede0a525b0bbe Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Tue, 6 Jun 2023 17:30:01 +0200 Subject: [PATCH 06/42] restrict is_upgrade_supported --- frame/contracts/src/migration.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index ad85da0477148..ce0e018f0ac3d 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -186,7 +186,7 @@ pub trait MigrateSequence: private::Sealed { return false }; - in_storage >= first_supported && target == high + in_storage == first_supported && target == high } } @@ -513,7 +513,7 @@ mod test { fn is_upgrade_supported_works() { type Migrations = (MockMigration<9>, MockMigration<10>, MockMigration<11>); - [(1, 1), (8, 11), (9, 11)].into_iter().for_each(|(from, to)| { + [(1, 1), (8, 11)].into_iter().for_each(|(from, to)| { assert!( Migrations::is_upgrade_supported( StorageVersion::new(from), @@ -525,7 +525,7 @@ mod test { ) }); - [(1, 0), (0, 3), (7, 11), (8, 10)].into_iter().for_each(|(from, to)| { + [(1, 0), (0, 3), (7, 11), (8, 10), (9, 11)].into_iter().for_each(|(from, to)| { assert!( !Migrations::is_upgrade_supported( StorageVersion::new(from), From a8caaf2f492bb46db9de8c11a0df70d91692a898 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 6 Jun 2023 23:53:59 +0200 Subject: [PATCH 07/42] Update contract multi-block migration Ensure that we do as many steps as possible given the weight limit passed to on_idle --- frame/contracts/src/lib.rs | 22 ++++++++++++++++------ frame/contracts/src/tests.rs | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 59c9ea23ed38b..79de7fbf6ab75 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -328,12 +328,22 @@ pub mod pallet { impl Hooks> for Pallet { fn on_idle(_block: T::BlockNumber, remaining_weight: Weight) -> Weight { use migration::MigrateResult::*; - - let (result, weight) = Migration::::migrate(remaining_weight); - let remaining_weight = remaining_weight.saturating_sub(weight); - - if !matches!(result, Completed | NoMigrationInProgress) { - return weight + let mut remaining_weight = remaining_weight; + + loop { + let (result, weight) = Migration::::migrate(remaining_weight); + remaining_weight.saturating_reduce(weight); + + match result { + // There is not enough weight to perform a migration, or make any progress, we + // just return the remaining weight. + NoMigrationPerformed | InProgress { steps_done: 0 } => return remaining_weight, + // Migration is still in progress, we can start the next step. + InProgress { .. } => {}, + // There are no migration in progress, or we are done with all migrations, we + // can do some more work with the remaining weight. + Completed | NoMigrationInProgress => break, + } } ContractInfo::::process_deletion_queue_batch(remaining_weight) diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 9425f25adf4b8..48b56128d5298 100644 --- a/frame/contracts/src/tests.rs +++ b/frame/contracts/src/tests.rs @@ -559,6 +559,24 @@ fn calling_plain_account_fails() { }); } +#[test] +fn migration_on_idle_hooks_works() { + // Defines expectations of how many migration steps can be done given the weight limit. + let tests = [ + (Weight::zero(), 0), + (::WeightInfo::migrate() + 1.into(), 1), + (Weight::MAX, 2), + ]; + + for (weight, expected_version) in tests { + ExtBuilder::default().set_storage_version(0).build().execute_with(|| { + MigrationInProgress::::set(Some(Default::default())); + Contracts::on_idle(System::block_number(), weight); + assert_eq!(StorageVersion::get::>(), expected_version); + }); + } +} + #[test] fn migration_in_progress_works() { let (wasm, code_hash) = compile_module::("dummy").unwrap(); From 1400ab05eb3452b534d5999e82dd8f4fc528913b Mon Sep 17 00:00:00 2001 From: Juan Girini Date: Wed, 7 Jun 2023 11:51:57 +0200 Subject: [PATCH 08/42] undo is_upgrade_supported change --- frame/contracts/src/migration.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index ce0e018f0ac3d..ad85da0477148 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -186,7 +186,7 @@ pub trait MigrateSequence: private::Sealed { return false }; - in_storage == first_supported && target == high + in_storage >= first_supported && target == high } } @@ -513,7 +513,7 @@ mod test { fn is_upgrade_supported_works() { type Migrations = (MockMigration<9>, MockMigration<10>, MockMigration<11>); - [(1, 1), (8, 11)].into_iter().for_each(|(from, to)| { + [(1, 1), (8, 11), (9, 11)].into_iter().for_each(|(from, to)| { assert!( Migrations::is_upgrade_supported( StorageVersion::new(from), @@ -525,7 +525,7 @@ mod test { ) }); - [(1, 0), (0, 3), (7, 11), (8, 10), (9, 11)].into_iter().for_each(|(from, to)| { + [(1, 0), (0, 3), (7, 11), (8, 10)].into_iter().for_each(|(from, to)| { assert!( !Migrations::is_upgrade_supported( StorageVersion::new(from), From cc7609c9275d1fa03fde4d5b139100777bc4b96b Mon Sep 17 00:00:00 2001 From: Juan Date: Wed, 7 Jun 2023 14:14:43 +0200 Subject: [PATCH 09/42] Update bin/node/runtime/src/lib.rs Co-authored-by: PG Herveou --- bin/node/runtime/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index dd4b14b1889b5..a5fe7465a3818 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1241,6 +1241,9 @@ impl pallet_contracts::Config for Runtime { type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = ConstBool; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; + #[cfg(not(feature = "runtime-benchmarks"))] + type Migrations = (); + #[cfg(feature = "runtime-benchmarks")] type Migrations = (NoopMigration<1>, NoopMigration<2>); } From 92360cde3f35a794f038410f7b8043a9e4d58f26 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Wed, 7 Jun 2023 15:17:05 +0200 Subject: [PATCH 10/42] wip --- frame/contracts/src/migration.rs | 84 +++++++++++++++++----------- frame/contracts/src/migration/v10.rs | 4 +- frame/contracts/src/migration/v11.rs | 4 +- frame/contracts/src/migration/v9.rs | 4 +- 4 files changed, 57 insertions(+), 39 deletions(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index ce0e018f0ac3d..f2122d1baf5c5 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -15,7 +15,45 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Migration framework for pallets. +//! Multi-block Migration framework for pallet-contracts. +//! +//! This module allows us to define a migration as a sequence of [`MigrationStep`]s that can be +//! executed across multiple blocks. +//! +//! # Usage +//! +//! A migration step is defined under `src/migration/vX.rs`, where `X` is the version number. +//! For example, `vX.rs` defines a migration from version X to version X + 1. +//! +//! ## Example: +//! +//! To configure a migration to v12 for a runtime using v9 of `pallet-contracts` on the chain, you +//! would set the `Migrations` trait as follows: +//! +//! ```rust +//! use pallet_contracts::migration::{v10, v11, v12}; +//! type Migrations = (v10, v11, v12); +//! ``` +//! +//! ## Notes: +//! +//! - Migrations should always be tested with `try-runtime` before being deployed. +//! - By testing with `try-runtime` against a live network, you ensure that all migration steps work +//! and that you have included the required steps. +//! +//! ## Low Level / Implementation Details +//! +//! When a migration starts and [`OnRuntimeUpgrade::on_runtime_upgrade`] is called, instead of +//! performing the actual migration, we set a custom storage item, `MigrationInProgress`. This +//! storage item defines a [`Cursor`] for the current migration step. +//! +//! If the `MigrationInProgress` storage item exists, it means a migration is in progress, and its +//! value holds a cursor for the current migration step. These migration steps are executed during +//! [`Hooks::on_idle`] or when the [`Pallet::migrate`] dispatchable is +//! called. +//! +//! While the migration is in progress, all dispatchables except `migrate`, are blocked, and returns +//! a `MigrationInProgress` error. pub mod v10; pub mod v11; @@ -57,7 +95,7 @@ pub enum IsFinished { /// /// The migration is done in steps. The migration is finished when /// `step()` returns `IsFinished::Yes`. -pub trait Migrate: Codec + MaxEncodedLen + Default { +pub trait MigrationStep: Codec + MaxEncodedLen + Default { /// Returns the version of the migration. const VERSION: u16; @@ -110,7 +148,7 @@ pub trait Migrate: Codec + MaxEncodedLen + Default { #[derive(frame_support::DefaultNoBound, Encode, Decode, MaxEncodedLen)] pub struct NoopMigration; -impl Migrate for NoopMigration { +impl MigrationStep for NoopMigration { const VERSION: u16 = N; fn max_step_weight() -> Weight { Weight::zero() @@ -122,10 +160,10 @@ impl Migrate for NoopMigration { } mod private { - use crate::migration::Migrate; + use crate::migration::MigrationStep; pub trait Sealed {} #[impl_trait_for_tuples::impl_for_tuples(10)] - #[tuple_types_custom_trait_bound(Migrate)] + #[tuple_types_custom_trait_bound(MigrationStep)] impl Sealed for Tuple {} } @@ -172,21 +210,10 @@ pub trait MigrateSequence: private::Sealed { /// Returns whether migrating from `in_storage` to `target` is supported. /// - /// A migration is supported if (in_storage + 1, target) is contained by `VERSION_RANGE`. + /// A migration is supported if `VERSION_RANGE` is (in_storage + 1, target). fn is_upgrade_supported(in_storage: StorageVersion, target: StorageVersion) -> bool { - if in_storage == target { - return true - } - if in_storage > target { - return false - } - let (low, high) = Self::VERSION_RANGE; - let Some(first_supported) = low.checked_sub(1) else { - return false - }; - - in_storage == first_supported && target == high + target == high && in_storage + 1 == low } } @@ -387,7 +414,7 @@ impl Migration { } #[impl_trait_for_tuples::impl_for_tuples(10)] -#[tuple_types_custom_trait_bound(Migrate)] +#[tuple_types_custom_trait_bound(MigrationStep)] impl MigrateSequence for Tuple { const VERSION_RANGE: (u16, u16) = { let mut versions: (u16, u16) = (0, 0); @@ -487,7 +514,7 @@ mod test { count: u16, } - impl Migrate for MockMigration { + impl MigrationStep for MockMigration { const VERSION: u16 = N; fn max_step_weight() -> Weight { Weight::from_all(1) @@ -512,20 +539,11 @@ mod test { #[test] fn is_upgrade_supported_works() { type Migrations = (MockMigration<9>, MockMigration<10>, MockMigration<11>); + assert!(Migrations::is_upgrade_supported(StorageVersion::new(8), StorageVersion::new(11))); + assert!(!Migrations::is_upgrade_supported(StorageVersion::new(9), StorageVersion::new(11))); + assert!(!Migrations::is_upgrade_supported(StorageVersion::new(8), StorageVersion::new(12))); - [(1, 1), (8, 11)].into_iter().for_each(|(from, to)| { - assert!( - Migrations::is_upgrade_supported( - StorageVersion::new(from), - StorageVersion::new(to) - ), - "{} -> {} is supported", - from, - to - ) - }); - - [(1, 0), (0, 3), (7, 11), (8, 10), (9, 11)].into_iter().for_each(|(from, to)| { + [(1, 0), (0, 3), (7, 11), (8, 10)].into_iter().for_each(|(from, to)| { assert!( !Migrations::is_upgrade_supported( StorageVersion::new(from), diff --git a/frame/contracts/src/migration/v10.rs b/frame/contracts/src/migration/v10.rs index cddf67a53c4f8..8d5897d15caee 100644 --- a/frame/contracts/src/migration/v10.rs +++ b/frame/contracts/src/migration/v10.rs @@ -21,7 +21,7 @@ use crate::{ address::AddressGenerator, exec::AccountIdOf, - migration::{IsFinished, Migrate}, + migration::{IsFinished, MigrationStep}, weights::WeightInfo, BalanceOf, CodeHash, Config, Pallet, TrieId, Weight, LOG_TARGET, }; @@ -117,7 +117,7 @@ pub struct Migration { type ContractInfoOf = StorageMap, Twox64Concat, ::AccountId, ContractInfo>; -impl Migrate for Migration { +impl MigrationStep for Migration { const VERSION: u16 = 10; fn max_step_weight() -> Weight { diff --git a/frame/contracts/src/migration/v11.rs b/frame/contracts/src/migration/v11.rs index 27a4b96431e06..59e4f5b2862c4 100644 --- a/frame/contracts/src/migration/v11.rs +++ b/frame/contracts/src/migration/v11.rs @@ -19,7 +19,7 @@ //! See . use crate::{ - migration::{IsFinished, Migrate}, + migration::{IsFinished, MigrationStep}, weights::WeightInfo, Config, Pallet, TrieId, Weight, LOG_TARGET, }; @@ -69,7 +69,7 @@ pub struct Migration { _phantom: PhantomData, } -impl Migrate for Migration { +impl MigrationStep for Migration { const VERSION: u16 = 11; // It would be more correct to make our use the now removed [DeletionQueueDepth](https://github.com/paritytech/substrate/pull/13702/files#diff-70e9723e9db62816e35f6f885b6770a8449c75a6c2733e9fa7a245fe52c4656c) diff --git a/frame/contracts/src/migration/v9.rs b/frame/contracts/src/migration/v9.rs index 3dd86a89cf06d..2aed926e632bf 100644 --- a/frame/contracts/src/migration/v9.rs +++ b/frame/contracts/src/migration/v9.rs @@ -18,7 +18,7 @@ //! Update `CodeStorage` with the new `determinism` field. use crate::{ - migration::{IsFinished, Migrate}, + migration::{IsFinished, MigrationStep}, weights::WeightInfo, CodeHash, Config, Determinism, Pallet, Weight, LOG_TARGET, }; @@ -83,7 +83,7 @@ pub struct Migration { _phantom: PhantomData, } -impl Migrate for Migration { +impl MigrationStep for Migration { const VERSION: u16 = 9; fn max_step_weight() -> Weight { From 137ecc8bd0f1070eaae89896dc26c4fd3ac4a497 Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Wed, 7 Jun 2023 15:18:13 +0200 Subject: [PATCH 11/42] fix comment (#14316) --- frame/contracts/src/benchmarking/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index b98c05f2503c2..85a5df6eed9ab 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -277,7 +277,7 @@ benchmarks! { assert_eq!(StorageVersion::get::>(), 2); } - // This benchmarks the weight of executing Migration::migrate when there are no migration in progress. + // This benchmarks the weight of executing 1 `NoopMigraton` #[pov_mode = Measured] migrate { StorageVersion::new(0).put::>(); From 27e5cf85540703660ef8a5a17926a9acd1b23ef3 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Wed, 7 Jun 2023 15:22:23 +0200 Subject: [PATCH 12/42] fix test --- frame/contracts/src/migration.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index f2122d1baf5c5..9c6d83b8b3435 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -542,18 +542,6 @@ mod test { assert!(Migrations::is_upgrade_supported(StorageVersion::new(8), StorageVersion::new(11))); assert!(!Migrations::is_upgrade_supported(StorageVersion::new(9), StorageVersion::new(11))); assert!(!Migrations::is_upgrade_supported(StorageVersion::new(8), StorageVersion::new(12))); - - [(1, 0), (0, 3), (7, 11), (8, 10)].into_iter().for_each(|(from, to)| { - assert!( - !Migrations::is_upgrade_supported( - StorageVersion::new(from), - StorageVersion::new(to) - ), - "{} -> {} is not supported", - from, - to - ) - }); } #[test] From 2935e99998e644b4f60c4298b203ed7b30b83ec1 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Wed, 7 Jun 2023 15:28:08 +0200 Subject: [PATCH 13/42] fix --- frame/contracts/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 9c6d83b8b3435..7a5e79ef09eca 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -44,7 +44,7 @@ //! ## Low Level / Implementation Details //! //! When a migration starts and [`OnRuntimeUpgrade::on_runtime_upgrade`] is called, instead of -//! performing the actual migration, we set a custom storage item, `MigrationInProgress`. This +//! performing the actual migration, we set a custom storage item `MigrationInProgress`. This //! storage item defines a [`Cursor`] for the current migration step. //! //! If the `MigrationInProgress` storage item exists, it means a migration is in progress, and its From a4bb30251991840effdae3aade647ecb02fb7fcb Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Wed, 7 Jun 2023 15:55:30 +0200 Subject: [PATCH 14/42] Update frame/contracts/src/migration.rs Co-authored-by: Juan --- frame/contracts/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 7a5e79ef09eca..9f17d99c6be2d 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -32,7 +32,7 @@ //! //! ```rust //! use pallet_contracts::migration::{v10, v11, v12}; -//! type Migrations = (v10, v11, v12); +//! type Migrations = (v9::Migration, v10::Migration, v11::Migration); //! ``` //! //! ## Notes: From f3e81ce8c5a673fb98a7ec70ca602d270036f7d1 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Wed, 7 Jun 2023 21:32:35 +0200 Subject: [PATCH 15/42] fix test doc --- bin/node/runtime/src/lib.rs | 4 ++-- frame/contracts/src/migration.rs | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index a5fe7465a3818..86c2758db3751 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1241,9 +1241,9 @@ impl pallet_contracts::Config for Runtime { type MaxStorageKeyLen = ConstU32<128>; type UnsafeUnstableInterface = ConstBool; type MaxDebugBufferLen = ConstU32<{ 2 * 1024 * 1024 }>; - #[cfg(not(feature = "runtime-benchmarks"))] + #[cfg(not(feature = "runtime-benchmarks"))] type Migrations = (); - #[cfg(feature = "runtime-benchmarks")] + #[cfg(feature = "runtime-benchmarks")] type Migrations = (NoopMigration<1>, NoopMigration<2>); } diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 9f17d99c6be2d..81fac2cdb94ad 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -30,8 +30,9 @@ //! To configure a migration to v12 for a runtime using v9 of `pallet-contracts` on the chain, you //! would set the `Migrations` trait as follows: //! -//! ```rust -//! use pallet_contracts::migration::{v10, v11, v12}; +//! ``` +//! use pallet_contracts::migration::{v9, v10, v11}; +//! # pub enum Runtime {}; //! type Migrations = (v9::Migration, v10::Migration, v11::Migration); //! ``` //! From f10f5bd8dc845b539d11e7006bda9ebd7ecc16cf Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Thu, 8 Jun 2023 15:09:14 +0200 Subject: [PATCH 16/42] Apply suggestions from code review Co-authored-by: Sasha Gryaznov --- frame/contracts/src/lib.rs | 6 +++--- frame/contracts/src/migration.rs | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 79de7fbf6ab75..5b46dd1df4725 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -339,9 +339,9 @@ pub mod pallet { // just return the remaining weight. NoMigrationPerformed | InProgress { steps_done: 0 } => return remaining_weight, // Migration is still in progress, we can start the next step. - InProgress { .. } => {}, - // There are no migration in progress, or we are done with all migrations, we - // can do some more work with the remaining weight. + InProgress { .. } => continue, + // Either no migration is in progress, or we are done with all migrations, we + // can do some more other work with the remaining weight. Completed | NoMigrationInProgress => break, } } diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 81fac2cdb94ad..e4eff0f263ec0 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -23,12 +23,12 @@ //! # Usage //! //! A migration step is defined under `src/migration/vX.rs`, where `X` is the version number. -//! For example, `vX.rs` defines a migration from version X to version X + 1. +//! For example, `vX.rs` defines a migration from version `X - 1` to version `X`. //! //! ## Example: //! -//! To configure a migration to v12 for a runtime using v9 of `pallet-contracts` on the chain, you -//! would set the `Migrations` trait as follows: +//! To configure a migration to `v12` for a runtime using `v9` of pallet-contracts on the chain, you +//! would set the `Migrations` type as follows: //! //! ``` //! use pallet_contracts::migration::{v9, v10, v11}; From dc556df320705862cbf01bcdbbe6acb799a0d666 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Thu, 8 Jun 2023 15:15:01 +0200 Subject: [PATCH 17/42] Fix compilation with feature runtime-benchmarks --- frame/contracts/src/benchmarking/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index 85a5df6eed9ab..dcfb3f841c02a 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -30,7 +30,7 @@ use self::{ }; use crate::{ exec::{AccountIdOf, Key}, - migration::{v10, v11, v9, Migrate}, + migration::{v10, v11, v9, MigrationStep}, wasm::CallFlags, Pallet as Contracts, *, }; From a7ef99728e3b79a38eda90831197165495bcc2d7 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Thu, 8 Jun 2023 15:18:46 +0200 Subject: [PATCH 18/42] fix example --- frame/contracts/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index e4eff0f263ec0..eb330882aa82d 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -27,7 +27,7 @@ //! //! ## Example: //! -//! To configure a migration to `v12` for a runtime using `v9` of pallet-contracts on the chain, you +//! To configure a migration to `v11` for a runtime using `v8` of pallet-contracts on the chain, you //! would set the `Migrations` type as follows: //! //! ``` From 6a89133090d77002224d3aa865aa85f57e835acd Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 11:59:12 +0200 Subject: [PATCH 19/42] fix cargo doc --document-private-items --- frame/contracts/src/migration.rs | 3 ++- frame/contracts/src/storage/meter.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index eb330882aa82d..fe63f5ef57347 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -334,7 +334,8 @@ pub enum StepResult { } impl Migration { - /// Verify that each migration's step of the [`T::Migrations`] sequence fits into `Cursor`. + /// Verify that each migration's step of the [`Config::Migrations`] sequence fits into + /// `Cursor`. pub(crate) fn integrity_test() { let max_weight = ::BlockWeights::get().max_block; T::Migrations::integrity_test(max_weight) diff --git a/frame/contracts/src/storage/meter.rs b/frame/contracts/src/storage/meter.rs index 506f4f0d86649..e5caa68a88482 100644 --- a/frame/contracts/src/storage/meter.rs +++ b/frame/contracts/src/storage/meter.rs @@ -90,7 +90,7 @@ pub trait Ext { /// This [`Ext`] is used for actual on-chain execution when balance needs to be charged. /// -/// It uses [`ReservableCurrency`] in order to do accomplish the reserves. +/// It uses [`frame_support::traits::ReservableCurrency`] in order to do accomplish the reserves. pub enum ReservingExt {} /// Used to implement a type state pattern for the meter. From 86e66960deffa601e290074ece42a3ebcbc0d662 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 13:46:59 +0200 Subject: [PATCH 20/42] private links --- frame/contracts/src/lib.rs | 1 + frame/contracts/src/migration.rs | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 5b46dd1df4725..924ccf5a8633c 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -80,6 +80,7 @@ //! an [`eDSL`](https://wiki.haskell.org/Embedded_domain_specific_language) that enables writing //! WebAssembly based smart contracts in the Rust programming language. +#![allow(rustdoc::private_intra_doc_links)] #![cfg_attr(not(feature = "std"), no_std)] #![cfg_attr(feature = "runtime-benchmarks", recursion_limit = "1024")] diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index fe63f5ef57347..4b5f95df5ec81 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -45,10 +45,10 @@ //! ## Low Level / Implementation Details //! //! When a migration starts and [`OnRuntimeUpgrade::on_runtime_upgrade`] is called, instead of -//! performing the actual migration, we set a custom storage item `MigrationInProgress`. This -//! storage item defines a [`Cursor`] for the current migration step. +//! performing the actual migration, we set a custom storage item [`MigrationInProgress`]. +//! This storage item defines a [`Cursor`] for the current migration step. //! -//! If the `MigrationInProgress` storage item exists, it means a migration is in progress, and its +//! If the [`MigrationInProgress`] storage item exists, it means a migration is in progress, and its //! value holds a cursor for the current migration step. These migration steps are executed during //! [`Hooks::on_idle`] or when the [`Pallet::migrate`] dispatchable is //! called. From 6417dde5bd3b96b764aac92fcad014f51e17b330 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 13:54:17 +0200 Subject: [PATCH 21/42] Remove dup comment --- frame/contracts/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 4b5f95df5ec81..d3c475df27434 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -175,9 +175,9 @@ mod private { pub trait MigrateSequence: private::Sealed { /// Returns the range of versions that this migration can handle. /// Migrations must be ordered by their versions with no gaps. - /// The following code will fail to compile: /// /// The following code will fail to compile: + /// /// ```compile_fail /// # use pallet_contracts::{NoopMigration, MigrateSequence}; /// let _ = <(NoopMigration<1>, NoopMigration<3>)>::VERSION_RANGE; From e6bb63de1b7468c3652436f987eb534c63b2e25a Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 14:04:52 +0200 Subject: [PATCH 22/42] add doc for MigrationInProgress --- frame/contracts/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 924ccf5a8633c..e008183cf1774 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -991,6 +991,8 @@ pub mod pallet { pub(crate) type DeletionQueueCounter = StorageValue<_, DeletionQueueManager, ValueQuery>; + /// A migration can span across multiple blocks. This storage defines a cursor to track the + /// progress of the migration, enabling us to resume from the last completed position. #[pallet::storage] pub(crate) type MigrationInProgress = StorageValue<_, migration::Cursor, OptionQuery>; From 1841f2348bda131ad644f33ee1ac0b9d47cd361e Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 14:15:53 +0200 Subject: [PATCH 23/42] PR review remove duplicate asserts --- frame/contracts/src/migration/v9.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/frame/contracts/src/migration/v9.rs b/frame/contracts/src/migration/v9.rs index 2aed926e632bf..9f00f47da3886 100644 --- a/frame/contracts/src/migration/v9.rs +++ b/frame/contracts/src/migration/v9.rs @@ -140,8 +140,6 @@ impl MigrationStep for Migration { ensure!(module.initial == old.initial, "invalid initial"); ensure!(module.maximum == old.maximum, "invalid maximum"); ensure!(module.code == old.code, "invalid code"); - ensure!(module.maximum == old.maximum, "invalid maximum"); - ensure!(module.code == old.code, "invalid code"); } Ok(()) From 0236ca0d75fbe655fe4df32d130e4cefcfaea7c0 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 14:16:18 +0200 Subject: [PATCH 24/42] simplify upper bound --- frame/contracts/src/benchmarking/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index dcfb3f841c02a..3e0540512e70b 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -237,7 +237,7 @@ benchmarks! { // This benchmarks the v9 migration step. (update codeStorage) #[pov_mode = Measured] v9_migration_step { - let c in 0 .. Perbill::from_percent(49).mul_ceil(T::MaxCodeLen::get()); + let c in 0 .. T::MaxCodeLen::get(); v9::store_old_dummy_code::(c as usize); let mut m = v9::Migration::::default(); }: { From 4686c0300be2c61b8f6103c64f8f50dc16292976 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 14:18:08 +0200 Subject: [PATCH 25/42] fix link --- frame/contracts/src/migration/v10.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration/v10.rs b/frame/contracts/src/migration/v10.rs index 8d5897d15caee..90bb12047d24f 100644 --- a/frame/contracts/src/migration/v10.rs +++ b/frame/contracts/src/migration/v10.rs @@ -16,7 +16,7 @@ // limitations under the License. //! Don't rely on reserved balances keeping an account alive -//! See . +//! See . use crate::{ address::AddressGenerator, From 5e029e1e29fe9f5e1f24f06576a65c6084eeabf6 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 14:18:41 +0200 Subject: [PATCH 26/42] typo --- frame/contracts/src/migration/v10.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration/v10.rs b/frame/contracts/src/migration/v10.rs index 90bb12047d24f..e138f5736ae73 100644 --- a/frame/contracts/src/migration/v10.rs +++ b/frame/contracts/src/migration/v10.rs @@ -188,7 +188,7 @@ impl MigrationStep for Migration { // Calculate the new base_deposit to store in the contract: // Ideally: it should be the same as the old one - // Ideally, it should be at least 2xED (for the contract and deposit account). + // Ideally, it should be at least 2xED (for the contract and deposit accounts). // It can't be more than the `new_deposit`. let new_base_deposit = min( max(contract.storage_base_deposit, min_balance.saturating_add(min_balance)), From c7846be2dcae9fc784e3c2dcc23b85955b9e8b62 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 14:19:10 +0200 Subject: [PATCH 27/42] typo --- frame/contracts/src/migration/v10.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration/v10.rs b/frame/contracts/src/migration/v10.rs index e138f5736ae73..ffe4c1d590f8a 100644 --- a/frame/contracts/src/migration/v10.rs +++ b/frame/contracts/src/migration/v10.rs @@ -187,7 +187,7 @@ impl MigrationStep for Migration { }); // Calculate the new base_deposit to store in the contract: - // Ideally: it should be the same as the old one + // Ideally, it should be the same as the old one // Ideally, it should be at least 2xED (for the contract and deposit accounts). // It can't be more than the `new_deposit`. let new_base_deposit = min( From d4349fb7a28142fd3dbdac92deef23b126f72c4a Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 14:24:05 +0200 Subject: [PATCH 28/42] no unwrap() --- frame/contracts/src/migration/v10.rs | 4 ++-- frame/contracts/src/migration/v11.rs | 3 ++- frame/contracts/src/migration/v9.rs | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/migration/v10.rs b/frame/contracts/src/migration/v10.rs index ffe4c1d590f8a..d3545340e43bf 100644 --- a/frame/contracts/src/migration/v10.rs +++ b/frame/contracts/src/migration/v10.rs @@ -240,8 +240,8 @@ impl MigrationStep for Migration { #[cfg(feature = "try-runtime")] fn post_upgrade_step(state: Vec) -> Result<(), TryRuntimeError> { - let sample = - )> as Decode>::decode(&mut &state[..]).unwrap(); + let sample = )> as Decode>::decode(&mut &state[..]) + .expect("pre_upgrade_step provides a valid state; qed"); log::debug!(target: LOG_TARGET, "Validating sample of {} contracts", sample.len()); for (account, old_contract) in sample { diff --git a/frame/contracts/src/migration/v11.rs b/frame/contracts/src/migration/v11.rs index 59e4f5b2862c4..67740cfaf6c80 100644 --- a/frame/contracts/src/migration/v11.rs +++ b/frame/contracts/src/migration/v11.rs @@ -121,7 +121,8 @@ impl MigrationStep for Migration { #[cfg(feature = "try-runtime")] fn post_upgrade_step(state: Vec) -> Result<(), TryRuntimeError> { - let len = ::decode(&mut &state[..]).unwrap(); + let len = ::decode(&mut &state[..]) + .expect("pre_upgrade_step provides a valid state; qed"); let counter = >::get(); ensure!(counter.insert_counter == len, "invalid insert counter"); ensure!(counter.delete_counter == 0, "invalid delete counter"); diff --git a/frame/contracts/src/migration/v9.rs b/frame/contracts/src/migration/v9.rs index 9f00f47da3886..d50f79c72c3be 100644 --- a/frame/contracts/src/migration/v9.rs +++ b/frame/contracts/src/migration/v9.rs @@ -126,8 +126,8 @@ impl MigrationStep for Migration { #[cfg(feature = "try-runtime")] fn post_upgrade_step(state: Vec) -> Result<(), TryRuntimeError> { - let sample = - , old::PrefabWasmModule)> as Decode>::decode(&mut &state[..]).unwrap(); + let sample = , old::PrefabWasmModule)> as Decode>::decode(&mut &state[..]) + .expect("pre_upgrade_step provides a valid state; qed"); log::debug!(target: LOG_TARGET, "Validating sample of {} contract codes", sample.len()); for (code_hash, old) in sample { From 7511bac4ba870ab8f9d721eec89da4b303585d1d Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 15:17:09 +0200 Subject: [PATCH 29/42] correct log message --- frame/contracts/src/migration/v10.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration/v10.rs b/frame/contracts/src/migration/v10.rs index d3545340e43bf..61ff362e7c96b 100644 --- a/frame/contracts/src/migration/v10.rs +++ b/frame/contracts/src/migration/v10.rs @@ -181,7 +181,7 @@ impl MigrationStep for Migration { }) // If it fails we fallback to minting the ED. .unwrap_or_else(|err| { - log::error!(target: LOG_TARGET, "Failed to transfer ED, reason: {:?}", err); + log::error!(target: LOG_TARGET, "Failed to transfer the base deposit, reason: {:?}", err); T::Currency::deposit_creating(&deposit_account, min_balance); min_balance }); From 3545f04265a616c339142528e7488736d3782f75 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Fri, 9 Jun 2023 15:23:36 +0200 Subject: [PATCH 30/42] missing --- frame/contracts/src/migration.rs | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index bb21741edd549..d3c475df27434 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -541,36 +541,9 @@ mod test { #[test] fn is_upgrade_supported_works() { type Migrations = (MockMigration<9>, MockMigration<10>, MockMigration<11>); -<<<<<<< HEAD assert!(Migrations::is_upgrade_supported(StorageVersion::new(8), StorageVersion::new(11))); assert!(!Migrations::is_upgrade_supported(StorageVersion::new(9), StorageVersion::new(11))); assert!(!Migrations::is_upgrade_supported(StorageVersion::new(8), StorageVersion::new(12))); -======= - - [(1, 1), (8, 11), (9, 11)].into_iter().for_each(|(from, to)| { - assert!( - Migrations::is_upgrade_supported( - StorageVersion::new(from), - StorageVersion::new(to) - ), - "{} -> {} is supported", - from, - to - ) - }); - - [(1, 0), (0, 3), (7, 11), (8, 10)].into_iter().for_each(|(from, to)| { - assert!( - !Migrations::is_upgrade_supported( - StorageVersion::new(from), - StorageVersion::new(to) - ), - "{} -> {} is not supported", - from, - to - ) - }); ->>>>>>> master } #[test] From 08f84fbba78f4c76ae3c8402acdd9471ab4fdd68 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 12 Jun 2023 10:57:05 +0200 Subject: [PATCH 31/42] fix typo --- frame/contracts/src/benchmarking/mod.rs | 2 +- frame/contracts/src/migration/v10.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index 3e0540512e70b..f671826dd1398 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -251,7 +251,7 @@ benchmarks! { whitelisted_caller(), WasmModule::dummy(), vec![], )?; - v10::store_old_contrat_info::(contract.account_id.clone(), contract.info()?); + v10::store_old_contract_info::(contract.account_id.clone(), contract.info()?); let mut m = v10::Migration::::default(); }: { m.step(); diff --git a/frame/contracts/src/migration/v10.rs b/frame/contracts/src/migration/v10.rs index 61ff362e7c96b..d58a9479f2fb9 100644 --- a/frame/contracts/src/migration/v10.rs +++ b/frame/contracts/src/migration/v10.rs @@ -69,7 +69,7 @@ mod old { } #[cfg(feature = "runtime-benchmarks")] -pub fn store_old_contrat_info(account: T::AccountId, info: crate::ContractInfo) { +pub fn store_old_contract_info(account: T::AccountId, info: crate::ContractInfo) { let info = old::ContractInfo { trie_id: info.trie_id, code_hash: info.code_hash, From 85f334f6d95214460445c9100ae2aa66a3ec62e5 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Mon, 12 Jun 2023 12:21:00 +0200 Subject: [PATCH 32/42] PR comment --- frame/contracts/src/benchmarking/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index f671826dd1398..be95294835a0e 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -277,15 +277,15 @@ benchmarks! { assert_eq!(StorageVersion::get::>(), 2); } - // This benchmarks the weight of executing 1 `NoopMigraton` + // This benchmarks the weight of dispatching migrate to executing 1 `NoopMigraton` #[pov_mode = Measured] migrate { StorageVersion::new(0).put::>(); as frame_support::traits::OnRuntimeUpgrade>::on_runtime_upgrade(); - let origin: RawOrigin<::AccountId> = RawOrigin::Signed(whitelisted_caller()); - }: { - >::migrate(origin.into(), Weight::MAX).unwrap() - } verify { + let caller: T::AccountId = whitelisted_caller(); + let origin = RawOrigin::Signed(caller.clone()); + }: _(origin, Weight::MAX) + verify { assert_eq!(StorageVersion::get::>(), 1); } From 103d50c2a7dd38e0b168b3436ec93c3ae3167061 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 13 Jun 2023 13:17:05 +0200 Subject: [PATCH 33/42] Add example with single element tuple --- frame/contracts/src/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 3df33ada47ea3..9a798c1a824dd 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -329,6 +329,13 @@ pub mod pallet { /// # struct Runtime {}; /// type Migrations = (v9::Migration, v10::Migration, v11::Migration); /// ``` + /// + /// If you have a single migration step, you can use a tuple with a single element: + /// ``` + /// use pallet_contracts::migration::v9; + /// # struct Runtime {}; + /// type Migrations = (v9::Migration,); + /// ``` type Migrations: MigrateSequence; } From dbb20d3accc0331eb40af0c842427bfeeb430240 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 13 Jun 2023 17:31:01 +0200 Subject: [PATCH 34/42] Improve migration message --- frame/contracts/src/migration.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index d3c475df27434..a8a559196e286 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -299,15 +299,13 @@ impl OnRuntimeUpgrade for Migration { log::debug!( target: LOG_TARGET, - "{}: Range supported {:?}, range requested {:?}", - >::name(), - T::Migrations::VERSION_RANGE, - (storage_version, target_version) + "Requested migration of {} from {:?}(on-chain storage version) to {:?}(current storage version)", + >::name(), storage_version, target_version ); ensure!( T::Migrations::is_upgrade_supported(storage_version, target_version), - "Unsupported upgrade" + "Unsupported upgrade: VERSION_RANGE should be (on-chain storage version + 1, current storage version)" ); Ok(Default::default()) } From 3b3c179fd0fb3323aaf961443611300210a54c00 Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Thu, 15 Jun 2023 17:40:00 +0200 Subject: [PATCH 35/42] Update frame/contracts/src/benchmarking/mod.rs Co-authored-by: Sasha Gryaznov --- frame/contracts/src/benchmarking/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index be95294835a0e..0233405d10f08 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -277,7 +277,7 @@ benchmarks! { assert_eq!(StorageVersion::get::>(), 2); } - // This benchmarks the weight of dispatching migrate to executing 1 `NoopMigraton` + // This benchmarks the weight of dispatching migrate to execute 1 `NoopMigraton` #[pov_mode = Measured] migrate { StorageVersion::new(0).put::>(); From fc977cb2203ea68f00171f3841444328f73de1f3 Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Thu, 15 Jun 2023 17:40:44 +0200 Subject: [PATCH 36/42] Update frame/contracts/src/migration.rs Co-authored-by: Sasha Gryaznov --- frame/contracts/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index a8a559196e286..4348c2e290765 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -46,7 +46,7 @@ //! //! When a migration starts and [`OnRuntimeUpgrade::on_runtime_upgrade`] is called, instead of //! performing the actual migration, we set a custom storage item [`MigrationInProgress`]. -//! This storage item defines a [`Cursor`] for the current migration step. +//! This storage item defines a [`Cursor`] for the current migration. //! //! If the [`MigrationInProgress`] storage item exists, it means a migration is in progress, and its //! value holds a cursor for the current migration step. These migration steps are executed during From d45d5700cf2fecac7c9f099270cea77bd52a5bed Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Thu, 15 Jun 2023 17:46:23 +0200 Subject: [PATCH 37/42] Update frame/contracts/src/migration.rs Co-authored-by: Sasha Gryaznov --- frame/contracts/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 4348c2e290765..65ddc2b439d21 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -173,7 +173,7 @@ mod private { /// The sequence must be defined by a tuple of migrations, each of which must implement the /// `Migrate` trait. Migrations must be ordered by their versions with no gaps. pub trait MigrateSequence: private::Sealed { - /// Returns the range of versions that this migration can handle. + /// Returns the range of versions that this migrations sequence can handle. /// Migrations must be ordered by their versions with no gaps. /// /// The following code will fail to compile: From 9d5b6b8afc1bb2a6c1f2fee1185cdafba70ca4ba Mon Sep 17 00:00:00 2001 From: pgherveou Date: Thu, 15 Jun 2023 20:47:07 +0200 Subject: [PATCH 38/42] use saturating_accrue instead of += --- frame/contracts/src/migration.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 65ddc2b439d21..641a059e14695 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -67,6 +67,7 @@ use frame_support::{ pallet_prelude::*, traits::{ConstU32, OnRuntimeUpgrade}, }; +use sp_runtime::Saturating; use sp_std::marker::PhantomData; #[cfg(feature = "try-runtime")] @@ -481,7 +482,7 @@ impl MigrateSequence for Tuple { let mut steps_done = 0; while weight_left.all_gt(max_weight) { let (finished, weight) = migration.step(); - steps_done += 1; + steps_done.saturating_accrue(1); weight_left.saturating_reduce(weight); if matches!(finished, IsFinished::Yes) { return StepResult::Completed{ steps_done } From fe412a7c63712599d465460edf50c4816977e6a2 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Thu, 15 Jun 2023 20:58:03 +0200 Subject: [PATCH 39/42] add more doc --- frame/contracts/src/migration.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 641a059e14695..00f9bf354be0a 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -84,7 +84,8 @@ fn invalid_version(version: StorageVersion) -> ! { panic!("Required migration {version:?} not supported by this runtime. This is a bug."); } -/// The cursor used to store the state of the current migration step. +/// The cursor used to encode the position (usually the last iterated key) of the current migration +/// step. pub type Cursor = BoundedVec>; /// IsFinished describes whether a migration is finished or not. From 8643fcdbeade3022056454751af0b6de4d746c51 Mon Sep 17 00:00:00 2001 From: PG Herveou Date: Tue, 20 Jun 2023 11:45:39 +0200 Subject: [PATCH 40/42] Contracts: Better migration types (#14418) --- frame/contracts/src/migration/v10.rs | 18 ++++++++++-------- frame/contracts/src/migration/v9.rs | 15 ++++++--------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/frame/contracts/src/migration/v10.rs b/frame/contracts/src/migration/v10.rs index d58a9479f2fb9..75d794a6d8167 100644 --- a/frame/contracts/src/migration/v10.rs +++ b/frame/contracts/src/migration/v10.rs @@ -42,7 +42,7 @@ use sp_core::hexdisplay::HexDisplay; #[cfg(feature = "try-runtime")] use sp_runtime::TryRuntimeError; use sp_runtime::{traits::Zero, Perbill, Saturating}; -use sp_std::{marker::PhantomData, ops::Deref, prelude::*}; +use sp_std::{ops::Deref, prelude::*}; mod old { use super::*; @@ -109,8 +109,7 @@ pub struct ContractInfo { #[derive(Encode, Decode, MaxEncodedLen, DefaultNoBound)] pub struct Migration { - last_key: Option>>, - _phantom: PhantomData, + last_account: Option, } #[storage_alias] @@ -125,8 +124,10 @@ impl MigrationStep for Migration { } fn step(&mut self) -> (IsFinished, Weight) { - let mut iter = if let Some(last_key) = self.last_key.take() { - old::ContractInfoOf::::iter_from(last_key.to_vec()) + let mut iter = if let Some(last_account) = self.last_account.take() { + old::ContractInfoOf::::iter_from(old::ContractInfoOf::::hashed_key_for( + last_account, + )) } else { old::ContractInfoOf::::iter() }; @@ -135,9 +136,6 @@ impl MigrationStep for Migration { let min_balance = Pallet::::min_balance(); log::debug!(target: LOG_TARGET, "Account: 0x{} ", HexDisplay::from(&account.encode())); - // Store last key for next migration step - self.last_key = Some(iter.last_raw_key().to_vec().try_into().unwrap()); - // Get the new deposit account address let deposit_account: DepositAccount = DepositAccount(T::AddressGenerator::deposit_address(&account)); @@ -223,6 +221,10 @@ impl MigrationStep for Migration { }; ContractInfoOf::::insert(&account, new_contract_info); + + // Store last key for next migration step + self.last_account = Some(account); + (IsFinished::No, T::WeightInfo::v10_migration_step()) } else { log::debug!(target: LOG_TARGET, "Done Migrating contract info"); diff --git a/frame/contracts/src/migration/v9.rs b/frame/contracts/src/migration/v9.rs index d50f79c72c3be..e6c6642955642 100644 --- a/frame/contracts/src/migration/v9.rs +++ b/frame/contracts/src/migration/v9.rs @@ -23,12 +23,10 @@ use crate::{ CodeHash, Config, Determinism, Pallet, Weight, LOG_TARGET, }; use codec::{Decode, Encode}; -use frame_support::{ - codec, pallet_prelude::*, storage_alias, BoundedVec, DefaultNoBound, Identity, -}; +use frame_support::{codec, pallet_prelude::*, storage_alias, DefaultNoBound, Identity}; #[cfg(feature = "try-runtime")] use sp_runtime::TryRuntimeError; -use sp_std::{marker::PhantomData, prelude::*}; +use sp_std::prelude::*; mod old { use super::*; @@ -79,8 +77,7 @@ type CodeStorage = StorageMap, Identity, CodeHash, Prefa #[derive(Encode, Decode, MaxEncodedLen, DefaultNoBound)] pub struct Migration { - last_key: Option>>, - _phantom: PhantomData, + last_code_hash: Option>, } impl MigrationStep for Migration { @@ -91,8 +88,8 @@ impl MigrationStep for Migration { } fn step(&mut self) -> (IsFinished, Weight) { - let mut iter = if let Some(last_key) = self.last_key.take() { - old::CodeStorage::::iter_from(last_key.to_vec()) + let mut iter = if let Some(last_key) = self.last_code_hash.take() { + old::CodeStorage::::iter_from(old::CodeStorage::::hashed_key_for(last_key)) } else { old::CodeStorage::::iter() }; @@ -108,7 +105,7 @@ impl MigrationStep for Migration { determinism: Determinism::Enforced, }; CodeStorage::::insert(key, module); - self.last_key = Some(iter.last_raw_key().to_vec().try_into().unwrap()); + self.last_code_hash = Some(key); (IsFinished::No, T::WeightInfo::v9_migration_step(len)) } else { log::debug!(target: LOG_TARGET, "No more contracts code to migrate"); From 7a2c272bd9a4e133f60215a9b7fc0aa79898c9b9 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 20 Jun 2023 11:56:56 +0200 Subject: [PATCH 41/42] Add explicit error, if try-runtime runs a noop migration --- frame/contracts/src/migration.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frame/contracts/src/migration.rs b/frame/contracts/src/migration.rs index 272ea7fad280d..5da1bb70a6204 100644 --- a/frame/contracts/src/migration.rs +++ b/frame/contracts/src/migration.rs @@ -306,6 +306,11 @@ impl OnRuntimeUpgrade for Migration>::on_chain_storage_version(); let target_version = >::current_storage_version(); + ensure!( + storage_version != target_version, + "No upgrade: Please remove this migration from your runtime upgrade configuration." + ); + log::debug!( target: LOG_TARGET, "Requested migration of {} from {:?}(on-chain storage version) to {:?}(current storage version)", From 030554d5b5c8664b7a79e1fea678142578d48b04 Mon Sep 17 00:00:00 2001 From: pgherveou Date: Tue, 20 Jun 2023 12:01:52 +0200 Subject: [PATCH 42/42] use mut remaining_weight --- frame/contracts/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 2d5cba082ec6e..0b765a2e89956 100644 --- a/frame/contracts/src/lib.rs +++ b/frame/contracts/src/lib.rs @@ -341,9 +341,8 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { - fn on_idle(_block: T::BlockNumber, remaining_weight: Weight) -> Weight { + fn on_idle(_block: T::BlockNumber, mut remaining_weight: Weight) -> Weight { use migration::MigrateResult::*; - let mut remaining_weight = remaining_weight; loop { let (result, weight) = Migration::::migrate(remaining_weight);