diff --git a/cumulus/pallets/collator-selection/Cargo.toml b/cumulus/pallets/collator-selection/Cargo.toml index f333a889e0af..045d4411f4a3 100644 --- a/cumulus/pallets/collator-selection/Cargo.toml +++ b/cumulus/pallets/collator-selection/Cargo.toml @@ -24,6 +24,7 @@ sp-staking = { path = "../../../substrate/primitives/staking", default-features frame-support = { path = "../../../substrate/frame/support", default-features = false, version = "27.0.0" } frame-system = { path = "../../../substrate/frame/system", default-features = false, version = "27.0.0" } pallet-authorship = { path = "../../../substrate/frame/authorship", default-features = false, version = "27.0.0" } +pallet-balances = { path = "../../../substrate/frame/balances", default-features = false, version = "27.0.0" } pallet-session = { path = "../../../substrate/frame/session", default-features = false, version = "27.0.0" } frame-benchmarking = { path = "../../../substrate/frame/benchmarking", default-features = false, optional = true, version = "27.0.0" } @@ -35,7 +36,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] @@ -54,6 +54,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/lib.rs b/cumulus/pallets/collator-selection/src/lib.rs index 7449f4d68c7e..61a68676466e 100644 --- a/cumulus/pallets/collator-selection/src/lib.rs +++ b/cumulus/pallets/collator-selection/src/lib.rs @@ -118,8 +118,8 @@ pub mod pallet { use sp_staking::SessionIndex; use sp_std::vec::Vec; - /// The current storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + /// The in-code storage version. + 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 58b4cc5b06a1..6ee0b707c442 100644 --- a/cumulus/pallets/collator-selection/src/migration.rs +++ b/cumulus/pallets/collator-selection/src/migration.rs @@ -17,9 +17,107 @@ //! 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 . +pub mod v2 { + use super::*; + use frame_support::{ + pallet_prelude::*, + storage_alias, + traits::{Currency, ReservableCurrency}, + }; + use sp_runtime::traits::{Saturating, Zero}; + #[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, + BoundedVec::AccountId, <::Currency as Currency<::AccountId>>::Balance>, ::MaxCandidates>, + ValueQuery, + >; + + /// Migrate to V2. + pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); + impl UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2 { + fn on_runtime_upgrade() -> Weight { + let mut weight = Weight::zero(); + let mut count: u64 = 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); + if err > Zero::zero() { + log::error!( + target: LOG_TARGET, + "{:?} balance was unable to be unreserved from {:?}", + err, &candidate.who, + ); + } + count.saturating_inc(); + } + weight.saturating_accrue( + <::WeightInfo as pallet_balances::WeightInfo>::force_unreserve().saturating_mul(count.into()), + ); + } + + 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")] + 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" + ); + Ok(()) + } + } +} + /// Version 1 Migration /// This migration ensures that any existing `Invulnerables` storage lists are sorted. pub mod v1 { @@ -90,3 +188,136 @@ 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_with_new_candidates() { + new_test_ext().execute_with(|| { + let storage_version = StorageVersion::new(1); + storage_version.put::>(); + + let one = 1u64; + let two = 2u64; + let three = 3u64; + let deposit = 10u64; + + // Set balance to 100 + 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![ + candidate_one.clone(), + candidate_two.clone(), + ]) + .expect("it works"); + 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(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(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(), 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); + }); + } +} diff --git a/cumulus/parachain-template/runtime/src/lib.rs b/cumulus/parachain-template/runtime/src/lib.rs index 1ef018a8ca34..a1fcb5c74f2b 100644 --- a/cumulus/parachain-template/runtime/src/lib.rs +++ b/cumulus/parachain-template/runtime/src/lib.rs @@ -113,6 +113,13 @@ pub type SignedExtra = ( pub type UncheckedExtrinsic = generic::UncheckedExtrinsic; +/// Migrations to apply on runtime upgrade. +pub type Migrations = ( + pallet_collator_selection::migration::v2::MigrationToV2, + // permanent + pallet_xcm::migration::MigrateToLatestXcmVersion, +); + /// Executive: handles dispatch to the various modules. pub type Executive = frame_executive::Executive< Runtime, 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 daa04d9607d7..c325771666be 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs @@ -945,10 +945,12 @@ pub type UncheckedExtrinsic = generic::UncheckedExtrinsic; /// Migrations to apply on runtime upgrade. 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, ); /// Migration to initialize storage versions for pallets added after genesis. 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 bf0b0a90cd6c..2b14f86d6647 100644 --- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs @@ -922,7 +922,7 @@ pub type Migrations = ( // v9420 pallet_nfts::migration::v1::MigrateToV1, // unreleased - pallet_collator_selection::migration::v1::MigrateToV1, + pallet_collator_selection::migration::v2::MigrationToV2, // unreleased migrations::NativeAssetParents0ToParents1Migration, // 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 2c82a755acaa..0277ecf4458c 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 @@ -115,7 +115,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 c94e0b3f6ac3..8fda85ecf869 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 @@ -115,7 +115,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 d6f8d32628ee..e590fd8db8a2 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs @@ -699,7 +699,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, ); diff --git a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs index 4ab3014cace0..383a18788f9e 100644 --- a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs +++ b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs @@ -99,6 +99,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/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