From 924c7b6ee7cae260967b83e36c251d77e79ae9ec Mon Sep 17 00:00:00 2001
From: Morgan Adamiec <morgan@parity.io>
Date: Wed, 24 Apr 2024 19:40:03 +0100
Subject: [PATCH] backport: Fix Stuck Collator Funds
---
cumulus/pallets/collator-selection/Cargo.toml | 3 +-
cumulus/pallets/collator-selection/src/lib.rs | 2 +-
.../collator-selection/src/migration.rs | 233 +++++++++++++++++-
.../assets/asset-hub-rococo/src/lib.rs | 2 +-
.../assets/asset-hub-westend/src/lib.rs | 2 +-
.../bridge-hubs/bridge-hub-rococo/src/lib.rs | 2 +-
.../bridge-hubs/bridge-hub-westend/src/lib.rs | 2 +-
.../collectives-westend/src/lib.rs | 2 +-
.../contracts/contracts-rococo/src/lib.rs | 2 +
.../coretime/coretime-rococo/src/lib.rs | 1 +
.../coretime/coretime-westend/src/lib.rs | 1 +
.../runtimes/people/people-rococo/src/lib.rs | 1 +
.../runtimes/people/people-westend/src/lib.rs | 1 +
prdoc/pr_4229.prdoc | 10 +
14 files changed, 256 insertions(+), 8 deletions(-)
create mode 100644 prdoc/pr_4229.prdoc
diff --git a/cumulus/pallets/collator-selection/Cargo.toml b/cumulus/pallets/collator-selection/Cargo.toml
index 025a089b3c3..34e0d94b976 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, version = "32.0.0" }
frame-system = { path = "../../../substrate/frame/system", default-features = false, version = "32.0.0" }
pallet-authorship = { path = "../../../substrate/frame/authorship", default-features = false, version = "32.0.0" }
+pallet-balances = { path = "../../../substrate/frame/balances", default-features = false }
pallet-session = { path = "../../../substrate/frame/session", default-features = false, version = "32.0.0" }
frame-benchmarking = { path = "../../../substrate/frame/benchmarking", default-features = false, optional = true, version = "32.0.0" }
@@ -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]
@@ -57,6 +57,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 84bde5c9fac..296033f4444 100644
--- a/cumulus/pallets/collator-selection/src/lib.rs
+++ b/cumulus/pallets/collator-selection/src/lib.rs
@@ -119,7 +119,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<T> =
<<T as Config>::Currency as Currency<<T as SystemConfig>::AccountId>>::Balance;
diff --git a/cumulus/pallets/collator-selection/src/migration.rs b/cumulus/pallets/collator-selection/src/migration.rs
index 5dc2fba4279..425acdd8bfb 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 <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, 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<T> = frame_support::migrations::VersionedMigration<
+ 1,
+ 2,
+ UncheckedMigrationToV2<T>,
+ Pallet<T>,
+ <T as frame_system::Config>::DbWeight,
+ >;
+
+ #[storage_alias]
+ pub type Candidates<T: Config> = StorageValue<
+ Pallet<T>,
+ BoundedVec<CandidateInfo<<T as frame_system::Config>::AccountId, <<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance>, <T as Config>::MaxCandidates>,
+ ValueQuery,
+ >;
+
+ /// Migrate to V2.
+ pub struct UncheckedMigrationToV2<T>(sp_std::marker::PhantomData<T>);
+ impl<T: Config + pallet_balances::Config> UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2<T> {
+ 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::<T>::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::<T>::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::<T>::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(
+ <<T as pallet_balances::Config>::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<Vec<u8>, sp_runtime::DispatchError> {
+ let number_of_candidates = Candidates::<T>::get().to_vec().len();
+ Ok((number_of_candidates as u32).encode())
+ }
+
+ #[cfg(feature = "try-runtime")]
+ fn post_upgrade(_number_of_candidates: Vec<u8>) -> Result<(), sp_runtime::DispatchError> {
+ let new_number_of_candidates = Candidates::<T>::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::<Pallet<Test>>();
+
+ 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::<CandidateInfo<u64, u64>, ConstU32<20>>::try_from(vec![
+ candidate_one.clone(),
+ candidate_two.clone(),
+ ])
+ .expect("it works");
+ let bounded_candidate_list =
+ BoundedVec::<CandidateInfo<u64, u64>, ConstU32<20>>::try_from(vec![
+ candidate_two.clone(),
+ candidate_three.clone(),
+ ])
+ .expect("it works");
+
+ // Set storage
+ Candidates::<Test>::put(bounded_candidates);
+ CandidateList::<Test>::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::<Test>::on_runtime_upgrade();
+
+ let new_storage_version = StorageVersion::get::<Pallet<Test>>();
+ 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::<Test>::get().is_empty());
+ // The new storage item should be preserved
+ assert_eq!(CandidateList::<Test>::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::<Pallet<Test>>();
+
+ 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::<CandidateInfo<u64, u64>, ConstU32<20>>::try_from(vec![
+ candidate_one.clone(),
+ candidate_two.clone(),
+ ])
+ .expect("it works");
+
+ // Set storage
+ Candidates::<Test>::put(bounded_candidates.clone());
+
+ // Sanity check
+ assert_eq!(Balances::free_balance(one), 90);
+ assert_eq!(Balances::free_balance(two), 90);
+
+ // Run migration
+ v2::MigrationToV2::<Test>::on_runtime_upgrade();
+
+ let new_storage_version = StorageVersion::get::<Pallet<Test>>();
+ 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::<Test>::get().is_empty());
+ // The new storage item should have the info now
+ assert_eq!(CandidateList::<Test>::get(), bounded_candidates);
+ });
+ }
+}
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 f68d142fd5b..4f3162d9c26 100644
--- a/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs
+++ b/cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs
@@ -962,10 +962,10 @@ pub type UncheckedExtrinsic =
generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;
/// Migrations to apply on runtime upgrade.
pub type Migrations = (
- pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
InitStorageVersions,
// unreleased
cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4<Runtime>,
+ pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
// permanent
pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>,
);
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 4c5da99c8ea..3d5c5bd4272 100644
--- a/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs
+++ b/cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs
@@ -938,7 +938,7 @@ pub type Migrations = (
// v9420
pallet_nfts::migration::v1::MigrateToV1<Runtime>,
// unreleased
- pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
+ pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
// unreleased
pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
// 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 e46820f5041..7b6af611c91 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<Runtime>,
+ pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
InitStorageVersions,
cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4<Runtime>,
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 71a41f6d8ed..abc9c05f18d 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<Runtime>,
+ pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
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 ab3d83fe435..93ff506de97 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<Runtime>,
+ pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
// unreleased
cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4<Runtime>,
// permanent
diff --git a/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs b/cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs
index 8ac88476cd5..f5313f892e0 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<Runtime>,
+ pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
cumulus_pallet_parachain_system::migration::Migration<Runtime>,
cumulus_pallet_xcmp_queue::migration::v2::MigrationToV2<Runtime>,
cumulus_pallet_xcmp_queue::migration::v3::MigrationToV3<Runtime>,
diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs
index 571c1d6bcb9..bce3e6e8059 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<Runtime>,
cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4<Runtime>,
// permanent
pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>,
diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs
index 7c6baa02ca6..5c26055e586 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<Runtime>,
cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4<Runtime>,
// permanent
pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>,
diff --git a/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs b/cumulus/parachains/runtimes/people/people-rococo/src/lib.rs
index af8ef0bf138..c49854257c9 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<Runtime>,
// permanent
pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>,
);
diff --git a/cumulus/parachains/runtimes/people/people-westend/src/lib.rs b/cumulus/parachains/runtimes/people/people-westend/src/lib.rs
index 04bddadc595..31f5e4882ad 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<Runtime>,
// permanent
pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>,
);
diff --git a/prdoc/pr_4229.prdoc b/prdoc/pr_4229.prdoc
new file mode 100644
index 00000000000..05af8e062a3
--- /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
--
GitLab