From 31704ff59d37cfdc86105b88bfc63dd8f0826e5c Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Sun, 21 Apr 2024 19:53:40 +0200 Subject: [PATCH 01/16] draft migration --- cumulus/pallets/collator-selection/src/lib.rs | 2 +- .../collator-selection/src/migration.rs | 49 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/cumulus/pallets/collator-selection/src/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 17bbe2591d48..2fa384367528 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -121,7 +121,7 @@ pub mod pallet { use sp_std::vec::Vec; /// The in-code storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(2); type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 5dc2fba4279a..82432caa1670 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -20,6 +20,55 @@ use super::*; use frame_support::traits::OnRuntimeUpgrade; use log; +/// Migrate to v2. Should have been part of https://github.com/paritytech/polkadot-sdk/pull/1340 +pub mod v2 { + use super::*; + use frame_support::{ + pallet_prelude::*, + storage_alias, + traits::{Currency, ReservableCurrency}, + }; + use sp_runtime::traits::Saturating; + + #[storage_alias] + pub type Candidates = StorageValue< + Pallet, + BoundedVec::AccountId, <::Currency as Currency<::AccountId>>::Balance>, ::MaxCandidates>, + ValueQuery, + >; + + /// Migrate to V2. + pub struct MigrateToV2(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV2 { + fn on_runtime_upgrade() -> Weight { + let on_chain_version = Pallet::::on_chain_storage_version(); + if on_chain_version == 1 { + let mut count: u32 = 0; + let candidates = Candidates::::take(); + for candidate in candidates { + let _err = T::Currency::unreserve(&candidate.who, candidate.deposit); + count.saturating_inc(); + } + + StorageVersion::new(2).put::>(); + log::info!( + target: LOG_TARGET, + "Unreserved locked bond of {} candidates, upgraded storage to version 2", + count, + ); + // weight: todo + T::DbWeight::get().reads_writes(2, 1) + } else { + log::info!( + target: LOG_TARGET, + "Migration did not execute. This probably should be removed" + ); + T::DbWeight::get().reads(1) + } + } + } +} + /// Version 1 Migration /// This migration ensures that any existing `Invulnerables` storage lists are sorted. pub mod v1 { From 92d11e8681b1774eb87379b131671bff1402f9bf Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 22 Apr 2024 12:50:53 +0200 Subject: [PATCH 02/16] add path for correct upgrade --- .../collator-selection/src/migration.rs | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 82432caa1670..acf8bbcef259 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -28,7 +28,7 @@ pub mod v2 { storage_alias, traits::{Currency, ReservableCurrency}, }; - use sp_runtime::traits::Saturating; + use sp_runtime::traits::{Saturating, Zero}; #[storage_alias] pub type Candidates = StorageValue< @@ -43,11 +43,32 @@ pub mod v2 { fn on_runtime_upgrade() -> Weight { let on_chain_version = Pallet::::on_chain_storage_version(); if on_chain_version == 1 { + let mut weight = Weight::zero(); let mut count: u32 = 0; + // candidates who exist under the old `Candidates` key let candidates = Candidates::::take(); - for candidate in candidates { - let _err = T::Currency::unreserve(&candidate.who, candidate.deposit); - count.saturating_inc(); + + // New candidates who have registered since the upgrade. Under normal circumstances, + // this should not exist because the migration should be applied when the upgrade + // happens. But in Polkadot/Kusama we messed this up, and people registered under + // `CandidateList` while their funds were locked in `Candidates`. + let new_candidate_list = CandidateList::::get(); + if new_candidate_list.len().is_zero() { + // The new list is empty, so this is essentially being applied correctly. We + // just put the candidates into the new storage item. + CandidateList::::put(&candidates); + // 1 write for the new list + weight.saturating_accrue(T::DbWeight::get().reads_writes(0, 1)); + } else { + // Oops, the runtime upgraded without the migration. There are new candidates in + // `CandidateList`. So, let's just refund the old ones and assume they have + // already started participating in the new system. + for candidate in candidates { + let _err = T::Currency::unreserve(&candidate.who, candidate.deposit); + count.saturating_inc(); + } + // TODO: set each accrue to weight of `unreserve` + weight.saturating_accrue(T::DbWeight::get().reads_writes(0, count as u64)); } StorageVersion::new(2).put::>(); @@ -56,8 +77,9 @@ pub mod v2 { "Unreserved locked bond of {} candidates, upgraded storage to version 2", count, ); - // weight: todo - T::DbWeight::get().reads_writes(2, 1) + // 1 read/write for storage version + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + weight } else { log::info!( target: LOG_TARGET, From f936850cdc3efb5a74cfc8dce7b067605771f722 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 22 Apr 2024 12:55:57 +0200 Subject: [PATCH 03/16] try runtime --- .../collator-selection/src/migration.rs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index acf8bbcef259..166397e9f214 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -88,6 +88,26 @@ pub mod v2 { T::DbWeight::get().reads(1) } } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, sp_runtime::DispatchError> { + let number_of_candidates = Candidates::::get().to_vec().len(); + Ok((number_of_candidates as u32).encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_number_of_candidates: Vec) -> Result<(), sp_runtime::DispatchError> { + let new_number_of_candidates = Candidates::::get().to_vec().len(); + assert_eq!( + new_number_of_candidates, 0 as usize, + "after migration, the candidates map should be empty" + ); + + let on_chain_version = Pallet::::on_chain_storage_version(); + frame_support::ensure!(on_chain_version >= 1, "must_upgrade"); + + Ok(()) + } } } From b81f0d6b9fe141309f872f6e96404bff0c947366 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 22 Apr 2024 13:23:00 +0200 Subject: [PATCH 04/16] with Vec --- cumulus/pallets/collator-selection/src/migration.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 166397e9f214..51f8c8462369 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -29,6 +29,8 @@ pub mod v2 { traits::{Currency, ReservableCurrency}, }; use sp_runtime::traits::{Saturating, Zero}; + #[cfg(feature = "try-runtime")] + use sp_std::vec::Vec; #[storage_alias] pub type Candidates = StorageValue< From 54627e2ad0b4776253374c0f6af500da11185c96 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 22 Apr 2024 20:10:53 +0200 Subject: [PATCH 05/16] versioned migration plus test --- .../collator-selection/src/migration.rs | 70 ++++++++++++++++++- 1 file changed, 67 insertions(+), 3 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 51f8c8462369..9ed7f2b162d4 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -17,7 +17,7 @@ //! A module that is responsible for migration of storage for Collator Selection. use super::*; -use frame_support::traits::OnRuntimeUpgrade; +use frame_support::traits::{OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade}; use log; /// Migrate to v2. Should have been part of https://github.com/paritytech/polkadot-sdk/pull/1340 @@ -32,6 +32,17 @@ pub mod v2 { #[cfg(feature = "try-runtime")] use sp_std::vec::Vec; + /// [`UncheckedMigrationToV2`] wrapped in a + /// [`VersionedMigration`](frame_support::migrations::VersionedMigration), ensuring the + /// migration is only performed when on-chain version is 1. + pub type MigrationToV2 = frame_support::migrations::VersionedMigration< + 1, + 2, + UncheckedMigrationToV2, + Pallet, + ::DbWeight, + >; + #[storage_alias] pub type Candidates = StorageValue< Pallet, @@ -40,8 +51,10 @@ pub mod v2 { >; /// Migrate to V2. - pub struct MigrateToV2(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for MigrateToV2 { + /// + /// Note: This should have been in https://github.com/paritytech/polkadot-sdk/pull/1340. + pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); + impl UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2 { fn on_runtime_upgrade() -> Weight { let on_chain_version = Pallet::::on_chain_storage_version(); if on_chain_version == 1 { @@ -183,3 +196,54 @@ pub mod v1 { } } } + +#[cfg(all(feature = "try-runtime", test))] +mod tests { + use super::*; + use crate::{ + migration::v2::Candidates, + mock::{new_test_ext, Balances, Test}, + }; + use frame_support::{ + traits::{Currency, ReservableCurrency, StorageVersion}, + BoundedVec, + }; + use sp_runtime::traits::ConstU32; + + #[test] + fn migrate_to_v2() { + new_test_ext().execute_with(|| { + let storage_version = StorageVersion::new(1); + storage_version.put::>(); + + let who = 1u64; + + // Set balance to 100 + Balances::make_free_balance_be(&who, 100u64); + // Reserve 20 -> 10 for the "old" candidacy and 10 for the "new" + Balances::reserve(&who, 20u64).unwrap(); + // Candidate info + let candidate = CandidateInfo { who, deposit: 10u64 }; + let bounded_candidates = + BoundedVec::, ConstU32<20>>::try_from(vec![ + candidate.clone() + ]) + .expect("it works"); + // Candidate is in the old `Candidates` and the new `CandidateList` + Candidates::::put(bounded_candidates.clone()); + CandidateList::::put(bounded_candidates); + // Sanity check + assert_eq!(Balances::free_balance(who), 80); + + // Run migration + v2::MigrationToV2::::on_runtime_upgrade(); + + // 10 should have been unreserved from the old candidacy + assert_eq!(Balances::free_balance(who), 90); + // The storage item should be gone + assert!(Candidates::::get().is_empty()); + // The new storage item should be preserved + assert_eq!(CandidateList::::get(), vec![candidate]); + }); + } +} From 3924c2efbba7981df655bd271536f104797c5f93 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Mon, 22 Apr 2024 20:23:07 +0200 Subject: [PATCH 06/16] add to runtimes --- cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs | 2 +- cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs | 2 +- .../runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs | 2 +- .../runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs | 2 +- .../runtimes/collectives/collectives-westend/src/lib.rs | 2 +- .../parachains/runtimes/contracts/contracts-rococo/src/lib.rs | 2 ++ cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs | 1 + .../parachains/runtimes/coretime/coretime-westend/src/lib.rs | 1 + cumulus/parachains/runtimes/people/people-rococo/src/lib.rs | 1 + cumulus/parachains/runtimes/people/people-westend/src/lib.rs | 1 + 10 files changed, 11 insertions(+), 5 deletions(-) diff --git a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs index 5cb29343a1cf..091b17491390 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs @@ -973,10 +973,10 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. #[allow(deprecated)] pub type Migrations = ( - pallet_collator_selection::migration::v1::MigrateToV1, InitStorageVersions, // unreleased cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4, + pallet_collator_selection::migration::v2::MigrationToV2, // permanent pallet_xcm::migration::MigrateToLatestXcmVersion, ); diff --git a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs index 366fb91723ae..db017bc86382 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs @@ -963,7 +963,7 @@ pub type Migrations = ( // v9420 pallet_nfts::migration::v1::MigrateToV1, // unreleased - pallet_collator_selection::migration::v1::MigrateToV1, + pallet_collator_selection::migration::v2::MigrationToV2, // unreleased pallet_multisig::migrations::v1::MigrateToV1, // unreleased diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs index 1eac813b10ce..109b081f937d 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs @@ -139,7 +139,7 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( - pallet_collator_selection::migration::v1::MigrateToV1, + pallet_collator_selection::migration::v2::MigrationToV2, pallet_multisig::migrations::v1::MigrateToV1, InitStorageVersions, cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4, diff --git a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs index b4ea2c79f64f..cf09a1acc548 100644 --- a/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs @@ -118,7 +118,7 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( - pallet_collator_selection::migration::v1::MigrateToV1, + pallet_collator_selection::migration::v2::MigrationToV2, pallet_multisig::migrations::v1::MigrateToV1, InitStorageVersions, // unreleased diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs index c599ba37f128..7274e9acdcd6 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs @@ -722,7 +722,7 @@ pub type UncheckedExtrinsic = /// `OnRuntimeUpgrade`. Included migrations must be idempotent. type Migrations = ( // unreleased - pallet_collator_selection::migration::v1::MigrateToV1, + pallet_collator_selection::migration::v2::MigrationToV2, // unreleased cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4, // permanent diff --git a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs index efa26fcbc22d..988195d88d87 100644 --- a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs @@ -98,6 +98,8 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( + pallet_collator_selection::migration::v1::MigrateToV1, + pallet_collator_selection::migration::v2::MigrationToV2, cumulus_pallet_parachain_system::migration::Migration, cumulus_pallet_xcmp_queue::migration::v2::MigrationToV2, cumulus_pallet_xcmp_queue::migration::v3::MigrationToV3, diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs index ad065ee34774..895890da7dd6 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs @@ -108,6 +108,7 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( + pallet_collator_selection::migration::v2::MigrationToV2, cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4, pallet_broker::migration::MigrateV0ToV1, // permanent diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs index 0f0742268618..9d080087d5db 100644 --- a/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs @@ -108,6 +108,7 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( + pallet_collator_selection::migration::v2::MigrationToV2, cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4, pallet_broker::migration::MigrateV0ToV1, // permanent diff --git a/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs b/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs index 3cd085fec632..4a57bad01c8c 100644 --- a/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs @@ -102,6 +102,7 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( + pallet_collator_selection::migration::v2::MigrationToV2, // permanent pallet_xcm::migration::MigrateToLatestXcmVersion, ); diff --git a/cumulus/parachains/runtimes/people/people-westend/src/lib.rs b/cumulus/parachains/runtimes/people/people-westend/src/lib.rs index 307ab90a4772..22e8fd57d3ca 100644 --- a/cumulus/parachains/runtimes/people/people-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/people/people-westend/src/lib.rs @@ -102,6 +102,7 @@ pub type UncheckedExtrinsic = /// Migrations to apply on runtime upgrade. pub type Migrations = ( + pallet_collator_selection::migration::v2::MigrationToV2, // permanent pallet_xcm::migration::MigrateToLatestXcmVersion, ); From e770e28b1d9385a2d30d317c62b16b2962f13a42 Mon Sep 17 00:00:00 2001 From: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Date: Mon, 22 Apr 2024 20:51:06 +0200 Subject: [PATCH 07/16] Update cumulus/pallets/collator-selection/src/migration.rs Co-authored-by: georgepisaltu <52418509+georgepisaltu@users.noreply.github.com> --- cumulus/pallets/collator-selection/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 9ed7f2b162d4..9178d5cf7a12 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -93,7 +93,7 @@ pub mod v2 { count, ); // 1 read/write for storage version - weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 1)); + weight.saturating_accrue(T::DbWeight::get().reads_writes(3, 2)); weight } else { log::info!( From 9025472667753a667c156f3824f23c732732dec6 Mon Sep 17 00:00:00 2001 From: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Date: Tue, 23 Apr 2024 06:58:51 +0200 Subject: [PATCH 08/16] Apply suggestions from code review Co-authored-by: Oliver Tale-Yazdi --- cumulus/pallets/collator-selection/src/migration.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 9178d5cf7a12..79f9b1944166 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -79,14 +79,13 @@ pub mod v2 { // `CandidateList`. So, let's just refund the old ones and assume they have // already started participating in the new system. for candidate in candidates { - let _err = T::Currency::unreserve(&candidate.who, candidate.deposit); + let _err = T::Currency::unreserve(&candidate.who, candidate.deposit).defensive(); count.saturating_inc(); } // TODO: set each accrue to weight of `unreserve` weight.saturating_accrue(T::DbWeight::get().reads_writes(0, count as u64)); } - StorageVersion::new(2).put::>(); log::info!( target: LOG_TARGET, "Unreserved locked bond of {} candidates, upgraded storage to version 2", @@ -118,8 +117,6 @@ pub mod v2 { "after migration, the candidates map should be empty" ); - let on_chain_version = Pallet::::on_chain_storage_version(); - frame_support::ensure!(on_chain_version >= 1, "must_upgrade"); Ok(()) } From 30395cd68b5a6926bc88b98b8c3cf0f34b2e2564 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Tue, 23 Apr 2024 07:24:31 +0200 Subject: [PATCH 09/16] add more tests --- .../collator-selection/src/migration.rs | 181 ++++++++++++------ 1 file changed, 124 insertions(+), 57 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 79f9b1944166..ecdf9558c946 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -56,51 +56,48 @@ pub mod v2 { pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); impl UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2 { fn on_runtime_upgrade() -> Weight { - let on_chain_version = Pallet::::on_chain_storage_version(); - if on_chain_version == 1 { - let mut weight = Weight::zero(); - let mut count: u32 = 0; - // candidates who exist under the old `Candidates` key - let candidates = Candidates::::take(); - - // New candidates who have registered since the upgrade. Under normal circumstances, - // this should not exist because the migration should be applied when the upgrade - // happens. But in Polkadot/Kusama we messed this up, and people registered under - // `CandidateList` while their funds were locked in `Candidates`. - let new_candidate_list = CandidateList::::get(); - if new_candidate_list.len().is_zero() { - // The new list is empty, so this is essentially being applied correctly. We - // just put the candidates into the new storage item. - CandidateList::::put(&candidates); - // 1 write for the new list - weight.saturating_accrue(T::DbWeight::get().reads_writes(0, 1)); - } else { - // Oops, the runtime upgraded without the migration. There are new candidates in - // `CandidateList`. So, let's just refund the old ones and assume they have - // already started participating in the new system. - for candidate in candidates { - let _err = T::Currency::unreserve(&candidate.who, candidate.deposit).defensive(); - count.saturating_inc(); - } - // TODO: set each accrue to weight of `unreserve` - weight.saturating_accrue(T::DbWeight::get().reads_writes(0, count as u64)); - } + let mut weight = Weight::zero(); + let mut count: u32 = 0; + // candidates who exist under the old `Candidates` key + let candidates = Candidates::::take(); - log::info!( - target: LOG_TARGET, - "Unreserved locked bond of {} candidates, upgraded storage to version 2", - count, - ); - // 1 read/write for storage version - weight.saturating_accrue(T::DbWeight::get().reads_writes(3, 2)); - weight + // New candidates who have registered since the upgrade. Under normal circumstances, + // this should not exist because the migration should be applied when the upgrade + // happens. But in Polkadot/Kusama we messed this up, and people registered under + // `CandidateList` while their funds were locked in `Candidates`. + let new_candidate_list = CandidateList::::get(); + if new_candidate_list.len().is_zero() { + // The new list is empty, so this is essentially being applied correctly. We just + // put the candidates into the new storage item. + CandidateList::::put(&candidates); + // 1 write for the new list + weight.saturating_accrue(T::DbWeight::get().reads_writes(0, 1)); } else { - log::info!( - target: LOG_TARGET, - "Migration did not execute. This probably should be removed" - ); - T::DbWeight::get().reads(1) + // Oops, the runtime upgraded without the migration. There are new candidates in + // `CandidateList`. So, let's just refund the old ones and assume they have already + // started participating in the new system. + for candidate in candidates { + let err = T::Currency::unreserve(&candidate.who, candidate.deposit); + if err > Zero::zero() { + log::info!( + target: LOG_TARGET, + "{:?} balance was unable to be unreserved from {}", + err, &candidate.who, + ); + } + // weight.saturating_accrue(::WeightInfo::force_unreserve()); + count.saturating_inc(); + } } + + log::info!( + target: LOG_TARGET, + "Unreserved locked bond of {} candidates, upgraded storage to version 2", + count, + ); + + weight.saturating_accrue(T::DbWeight::get().reads_writes(3, 2)); + weight } #[cfg(feature = "try-runtime")] @@ -116,8 +113,6 @@ pub mod v2 { new_number_of_candidates, 0 as usize, "after migration, the candidates map should be empty" ); - - Ok(()) } } @@ -208,39 +203,111 @@ mod tests { use sp_runtime::traits::ConstU32; #[test] - fn migrate_to_v2() { + fn migrate_to_v2_with_new_candidates() { new_test_ext().execute_with(|| { let storage_version = StorageVersion::new(1); storage_version.put::>(); - let who = 1u64; + let one = 1u64; + let two = 2u64; + let three = 3u64; + let deposit = 10u64; // Set balance to 100 - Balances::make_free_balance_be(&who, 100u64); - // Reserve 20 -> 10 for the "old" candidacy and 10 for the "new" - Balances::reserve(&who, 20u64).unwrap(); + Balances::make_free_balance_be(&one, 100u64); + Balances::make_free_balance_be(&two, 100u64); + Balances::make_free_balance_be(&three, 100u64); + // Reservations: 10 for the "old" candidacy and 10 for the "new" + Balances::reserve(&one, 10u64).unwrap(); // old + Balances::reserve(&two, 20u64).unwrap(); // old + new + Balances::reserve(&three, 10u64).unwrap(); // new // Candidate info - let candidate = CandidateInfo { who, deposit: 10u64 }; + let candidate_one = CandidateInfo { who: one, deposit }; + let candidate_two = CandidateInfo { who: two, deposit }; + let candidate_three = CandidateInfo { who: three, deposit }; + // Storage lists let bounded_candidates = BoundedVec::, ConstU32<20>>::try_from(vec![ - candidate.clone() + candidate_one.clone(), + candidate_two.clone(), ]) .expect("it works"); - // Candidate is in the old `Candidates` and the new `CandidateList` - Candidates::::put(bounded_candidates.clone()); - CandidateList::::put(bounded_candidates); + let bounded_candidate_list = + BoundedVec::, ConstU32<20>>::try_from(vec![ + candidate_two.clone(), + candidate_three.clone(), + ]) + .expect("it works"); + // Set storage + Candidates::::put(bounded_candidates); + CandidateList::::put(bounded_candidate_list.clone()); // Sanity check - assert_eq!(Balances::free_balance(who), 80); + assert_eq!(Balances::free_balance(one), 90); + assert_eq!(Balances::free_balance(two), 80); + assert_eq!(Balances::free_balance(three), 90); // Run migration v2::MigrationToV2::::on_runtime_upgrade(); + let new_storage_version = StorageVersion::get::>(); + assert_eq!(new_storage_version, 2); + // 10 should have been unreserved from the old candidacy - assert_eq!(Balances::free_balance(who), 90); + assert_eq!(Balances::free_balance(one), 100); + assert_eq!(Balances::free_balance(two), 90); + assert_eq!(Balances::free_balance(three), 90); // The storage item should be gone assert!(Candidates::::get().is_empty()); // The new storage item should be preserved - assert_eq!(CandidateList::::get(), vec![candidate]); + assert_eq!(CandidateList::::get(), bounded_candidate_list); + }); + } + + #[test] + fn migrate_to_v2_without_new_candidates() { + new_test_ext().execute_with(|| { + let storage_version = StorageVersion::new(1); + storage_version.put::>(); + + let one = 1u64; + let two = 2u64; + let deposit = 10u64; + + // Set balance to 100 + Balances::make_free_balance_be(&one, 100u64); + Balances::make_free_balance_be(&two, 100u64); + // Reservations + Balances::reserve(&one, 10u64).unwrap(); // old + Balances::reserve(&two, 10u64).unwrap(); // old + // Candidate info + let candidate_one = CandidateInfo { who: one, deposit }; + let candidate_two = CandidateInfo { who: two, deposit }; + // Storage lists + let bounded_candidates = + BoundedVec::, ConstU32<20>>::try_from(vec![ + candidate_one.clone(), + candidate_two.clone(), + ]) + .expect("it works"); + // Set storage + Candidates::::put(bounded_candidates.clone()); + // Sanity check + assert_eq!(Balances::free_balance(one), 90); + assert_eq!(Balances::free_balance(two), 90); + + // Run migration + v2::MigrationToV2::::on_runtime_upgrade(); + + let new_storage_version = StorageVersion::get::>(); + assert_eq!(new_storage_version, 2); + + // Nothing changes deposit-wise + assert_eq!(Balances::free_balance(one), 90); + assert_eq!(Balances::free_balance(two), 90); + // The storage item should be gone + assert!(Candidates::::get().is_empty()); + // The new storage item should have the info now + assert_eq!(CandidateList::::get(), bounded_candidates); }); } } From dc4d0a1b4ce45f3452467be0a2256dd77f032678 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Tue, 23 Apr 2024 07:36:37 +0200 Subject: [PATCH 10/16] add real weight --- cumulus/pallets/collator-selection/src/migration.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index ecdf9558c946..3856391f23a4 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -85,9 +85,14 @@ pub mod v2 { err, &candidate.who, ); } - // weight.saturating_accrue(::WeightInfo::force_unreserve()); count.saturating_inc(); } + // conservative benchmarks from `force_unreserve` + weight.saturating_accrue( + Weight::from_parts(20_000_000, 4_000) + .saturating_add(T::DbWeight::get().reads_writes(1, 1)) + .saturating_mul(count.into()), + ); } log::info!( From bb1c60ef68ab1e86b3163c6bf311b54f85e8583f Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Tue, 23 Apr 2024 07:40:15 +0200 Subject: [PATCH 11/16] make fmt happy --- cumulus/pallets/collator-selection/src/migration.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 3856391f23a4..e51be8c36e75 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -81,7 +81,7 @@ pub mod v2 { if err > Zero::zero() { log::info!( target: LOG_TARGET, - "{:?} balance was unable to be unreserved from {}", + "{:?} balance was unable to be unreserved from {:?}", err, &candidate.who, ); } @@ -222,14 +222,17 @@ mod tests { Balances::make_free_balance_be(&one, 100u64); Balances::make_free_balance_be(&two, 100u64); Balances::make_free_balance_be(&three, 100u64); + // Reservations: 10 for the "old" candidacy and 10 for the "new" Balances::reserve(&one, 10u64).unwrap(); // old Balances::reserve(&two, 20u64).unwrap(); // old + new Balances::reserve(&three, 10u64).unwrap(); // new + // Candidate info let candidate_one = CandidateInfo { who: one, deposit }; let candidate_two = CandidateInfo { who: two, deposit }; let candidate_three = CandidateInfo { who: three, deposit }; + // Storage lists let bounded_candidates = BoundedVec::, ConstU32<20>>::try_from(vec![ @@ -243,9 +246,11 @@ mod tests { candidate_three.clone(), ]) .expect("it works"); + // Set storage Candidates::::put(bounded_candidates); CandidateList::::put(bounded_candidate_list.clone()); + // Sanity check assert_eq!(Balances::free_balance(one), 90); assert_eq!(Balances::free_balance(two), 80); @@ -281,12 +286,15 @@ mod tests { // Set balance to 100 Balances::make_free_balance_be(&one, 100u64); Balances::make_free_balance_be(&two, 100u64); + // Reservations Balances::reserve(&one, 10u64).unwrap(); // old Balances::reserve(&two, 10u64).unwrap(); // old + // Candidate info let candidate_one = CandidateInfo { who: one, deposit }; let candidate_two = CandidateInfo { who: two, deposit }; + // Storage lists let bounded_candidates = BoundedVec::, ConstU32<20>>::try_from(vec![ @@ -294,8 +302,10 @@ mod tests { candidate_two.clone(), ]) .expect("it works"); + // Set storage Candidates::::put(bounded_candidates.clone()); + // Sanity check assert_eq!(Balances::free_balance(one), 90); assert_eq!(Balances::free_balance(two), 90); From 2f27ce75ae96d455a635214bffbe7919430d0b69 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Tue, 23 Apr 2024 07:42:36 +0200 Subject: [PATCH 12/16] pr doc --- prdoc/pr_4229.prdoc | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 prdoc/pr_4229.prdoc diff --git a/prdoc/pr_4229.prdoc b/prdoc/pr_4229.prdoc new file mode 100644 index 000000000000..05af8e062a32 --- /dev/null +++ b/prdoc/pr_4229.prdoc @@ -0,0 +1,10 @@ +title: "Fix Stuck Collator Funds" + +doc: + - audience: Runtime Dev + description: | + Fixes stuck collator funds by providing a migration that should have been in PR 1340. + +crates: + - name: pallet-collator-selection + bump: patch From b636ace89d6490a9930d3f09a4c12b8e3420e911 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Tue, 23 Apr 2024 07:56:29 +0200 Subject: [PATCH 13/16] doc fmt --- cumulus/pallets/collator-selection/src/migration.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index e51be8c36e75..122c31af38c5 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -20,7 +20,7 @@ use super::*; use frame_support::traits::{OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade}; use log; -/// Migrate to v2. Should have been part of https://github.com/paritytech/polkadot-sdk/pull/1340 +/// Migrate to v2. Should have been part of . pub mod v2 { use super::*; use frame_support::{ @@ -51,8 +51,6 @@ pub mod v2 { >; /// Migrate to V2. - /// - /// Note: This should have been in https://github.com/paritytech/polkadot-sdk/pull/1340. pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); impl UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2 { fn on_runtime_upgrade() -> Weight { From 9f298d1bbf3a25369a26e4407800f35f10d19a0e Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 23 Apr 2024 10:38:31 +0300 Subject: [PATCH 14/16] Use actual weight for `unreserve` Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/Cargo.toml | 3 ++- cumulus/pallets/collator-selection/src/migration.rs | 7 ++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/cumulus/pallets/collator-selection/Cargo.toml b/cumulus/pallets/collator-selection/Cargo.toml index c04d9e1403ec..25ca2fe057ba 100644 --- a/cumulus/pallets/collator-selection/Cargo.toml +++ b/cumulus/pallets/collator-selection/Cargo.toml @@ -27,6 +27,7 @@ sp-staking = { path = "../../../substrate/primitives/staking", default-features frame-support = { path = "../../../substrate/frame/support", default-features = false } frame-system = { path = "../../../substrate/frame/system", default-features = false } pallet-authorship = { path = "../../../substrate/frame/authorship", default-features = false } +pallet-balances = { path = "../../../substrate/frame/balances", default-features = false } pallet-session = { path = "../../../substrate/frame/session", default-features = false } frame-benchmarking = { path = "../../../substrate/frame/benchmarking", default-features = false, optional = true } @@ -38,7 +39,6 @@ sp-tracing = { path = "../../../substrate/primitives/tracing" } sp-runtime = { path = "../../../substrate/primitives/runtime" } pallet-timestamp = { path = "../../../substrate/frame/timestamp" } sp-consensus-aura = { path = "../../../substrate/primitives/consensus/aura" } -pallet-balances = { path = "../../../substrate/frame/balances" } pallet-aura = { path = "../../../substrate/frame/aura" } [features] @@ -59,6 +59,7 @@ std = [ "frame-system/std", "log/std", "pallet-authorship/std", + "pallet-balances/std", "pallet-session/std", "rand/std", "scale-info/std", diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 122c31af38c5..78d5d4bf0282 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -52,7 +52,7 @@ pub mod v2 { /// Migrate to V2. pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); - impl UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2 { + impl UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2 { fn on_runtime_upgrade() -> Weight { let mut weight = Weight::zero(); let mut count: u32 = 0; @@ -85,11 +85,8 @@ pub mod v2 { } count.saturating_inc(); } - // conservative benchmarks from `force_unreserve` weight.saturating_accrue( - Weight::from_parts(20_000_000, 4_000) - .saturating_add(T::DbWeight::get().reads_writes(1, 1)) - .saturating_mul(count.into()), + <::WeightInfo as pallet_balances::WeightInfo>::force_unreserve(), ); } From 1f4198d71fd8632b43221c8628c2a3d936989d14 Mon Sep 17 00:00:00 2001 From: georgepisaltu Date: Tue, 23 Apr 2024 13:23:45 +0300 Subject: [PATCH 15/16] Fix weight for multiple `unreserve` calls Signed-off-by: georgepisaltu --- cumulus/pallets/collator-selection/src/migration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 78d5d4bf0282..1155f90012cd 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -55,7 +55,7 @@ pub mod v2 { impl UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2 { fn on_runtime_upgrade() -> Weight { let mut weight = Weight::zero(); - let mut count: u32 = 0; + let mut count: u64 = 0; // candidates who exist under the old `Candidates` key let candidates = Candidates::::take(); @@ -86,7 +86,7 @@ pub mod v2 { count.saturating_inc(); } weight.saturating_accrue( - <::WeightInfo as pallet_balances::WeightInfo>::force_unreserve(), + <::WeightInfo as pallet_balances::WeightInfo>::force_unreserve() * count, ); } From ba7b985e998a632a445050642c4b44db9665c5a4 Mon Sep 17 00:00:00 2001 From: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Date: Tue, 23 Apr 2024 13:34:17 +0200 Subject: [PATCH 16/16] Apply suggestions from code review Co-authored-by: Oliver Tale-Yazdi --- cumulus/pallets/collator-selection/src/migration.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs index 1155f90012cd..425acdd8bfb5 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -77,7 +77,7 @@ pub mod v2 { for candidate in candidates { let err = T::Currency::unreserve(&candidate.who, candidate.deposit); if err > Zero::zero() { - log::info!( + log::error!( target: LOG_TARGET, "{:?} balance was unable to be unreserved from {:?}", err, &candidate.who, @@ -86,7 +86,7 @@ pub mod v2 { count.saturating_inc(); } weight.saturating_accrue( - <::WeightInfo as pallet_balances::WeightInfo>::force_unreserve() * count, + <::WeightInfo as pallet_balances::WeightInfo>::force_unreserve().saturating_mul(count.into()), ); }