From c710943fb0bbdba67916eb64f48466c08c9512f2 Mon Sep 17 00:00:00 2001 From: William Freudenberger Date: Thu, 6 Jun 2024 10:59:57 +0200 Subject: [PATCH 1/5] fix: add collator selection v2 migration --- runtime/altair/src/migrations.rs | 1 + runtime/centrifuge/src/migrations.rs | 1 + .../src/migrations/collator_selection_v2.rs | 129 ++++++++++++++++++ runtime/common/src/migrations/mod.rs | 1 + runtime/development/src/migrations.rs | 1 + 5 files changed, 133 insertions(+) create mode 100644 runtime/common/src/migrations/collator_selection_v2.rs diff --git a/runtime/altair/src/migrations.rs b/runtime/altair/src/migrations.rs index 6de3291c7f..5aa63640e0 100644 --- a/runtime/altair/src/migrations.rs +++ b/runtime/altair/src/migrations.rs @@ -23,6 +23,7 @@ pub type UpgradeAltair1035 = ( runtime_common::migrations::increase_storage_version::Migration, runtime_common::migrations::increase_storage_version::Migration, pallet_collator_selection::migration::v1::MigrateToV1, + runtime_common::migrations::collator_selection_v2::MigrationToV2, runtime_common::migrations::loans::AddWithLinearPricing, // As of May 2024, the `pallet_balances::Hold` storage was empty. But better be safe. runtime_common::migrations::hold_reason::MigrateTransferAllowListHolds< diff --git a/runtime/centrifuge/src/migrations.rs b/runtime/centrifuge/src/migrations.rs index 38880827ae..0aaa9c6906 100644 --- a/runtime/centrifuge/src/migrations.rs +++ b/runtime/centrifuge/src/migrations.rs @@ -21,6 +21,7 @@ pub type UpgradeCentrifuge1029 = ( runtime_common::migrations::increase_storage_version::Migration, runtime_common::migrations::increase_storage_version::Migration, pallet_collator_selection::migration::v1::MigrateToV1, + runtime_common::migrations::collator_selection_v2::MigrationToV2, runtime_common::migrations::loans::AddWithLinearPricing, runtime_common::migrations::hold_reason::MigrateTransferAllowListHolds< crate::Runtime, diff --git a/runtime/common/src/migrations/collator_selection_v2.rs b/runtime/common/src/migrations/collator_selection_v2.rs new file mode 100644 index 0000000000..1b34e92e61 --- /dev/null +++ b/runtime/common/src/migrations/collator_selection_v2.rs @@ -0,0 +1,129 @@ +// Copyright 2024 Centrifuge Foundation (centrifuge.io). +// +// This file is part of the Centrifuge chain project. +// Centrifuge is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version (see http://www.gnu.org/licenses). +// Centrifuge is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// More info: https://github.com/paritytech/polkadot-sdk/pull/4229#issuecomment-2151690311 +use frame_support::traits::OnRuntimeUpgrade; +use frame_support::{ + pallet_prelude::*, + storage_alias, + traits::{Currency, ReservableCurrency}, +}; + +use pallet_collator_selection::*; +use sp_runtime::traits::{Saturating, Zero}; +#[cfg(feature = "try-runtime")] +use sp_std::vec::Vec; + +const LOG_TARGET: &str = "runtime::collator-selection"; + +/// [`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< + CandidateInfo< + ::AccountId, + <::Currency as Currency<::AccountId>>::Balance, + >, + ::MaxCandidates, + >, + ValueQuery, +>; + +/// Migrate to V2. +pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); +impl OnRuntimeUpgrade 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. + log::info!( + target: LOG_TARGET, + "New candidate list is empty, adding {} previous candidates", + candidates.len(), + ); + 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" + ); + let count_pre: u32 = Decode::decode(&mut number_of_candidates.as_slice()) + .expect("pre_upgrade provides a valid state; qed"); + assert_eq!( + count_pre, + CandidateList::::get().len() as u32, + "after migration, the CandidateList should equal old Candidate storage" + ); + Ok(()) + } +} diff --git a/runtime/common/src/migrations/mod.rs b/runtime/common/src/migrations/mod.rs index dc12bdeb04..9e926ea8a5 100644 --- a/runtime/common/src/migrations/mod.rs +++ b/runtime/common/src/migrations/mod.rs @@ -12,6 +12,7 @@ //! Centrifuge Runtime-Common Migrations +pub mod collator_selection_v2; pub mod hold_reason; pub mod increase_storage_version; pub mod loans; diff --git a/runtime/development/src/migrations.rs b/runtime/development/src/migrations.rs index 87e8267783..4df609e315 100644 --- a/runtime/development/src/migrations.rs +++ b/runtime/development/src/migrations.rs @@ -23,6 +23,7 @@ const IDENTITY_MIGRATION_KEY_LIMIT: u64 = 1000; /// It includes all the migrations that have to be applied on that chain. pub type UpgradeDevelopment1047 = ( pallet_collator_selection::migration::v1::MigrateToV1, + runtime_common::migrations::collator_selection_v2::MigrationToV2, cleanup_foreign_investments::Migration, // v0 -> v1 pallet_multisig::migrations::v1::MigrateToV1, From 1c172827f9aee4d355ec2c10cd3515950168d63e Mon Sep 17 00:00:00 2001 From: William Freudenberger Date: Thu, 6 Jun 2024 11:46:15 +0200 Subject: [PATCH 2/5] fix: clippy --- runtime/common/src/migrations/collator_selection_v2.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/common/src/migrations/collator_selection_v2.rs b/runtime/common/src/migrations/collator_selection_v2.rs index 1b34e92e61..e051d810e7 100644 --- a/runtime/common/src/migrations/collator_selection_v2.rs +++ b/runtime/common/src/migrations/collator_selection_v2.rs @@ -90,7 +90,7 @@ impl OnRuntimeUpgrade for UncheckedMigratio count.saturating_inc(); } weight.saturating_accrue( - <::WeightInfo as pallet_balances::WeightInfo>::force_unreserve().saturating_mul(count.into()), + <::WeightInfo as pallet_balances::WeightInfo>::force_unreserve().saturating_mul(count), ); } From c3d48824f8da73d4ce9cd1ef6cf66ba69dd8bbde Mon Sep 17 00:00:00 2001 From: William Freudenberger Date: Thu, 6 Jun 2024 14:38:12 +0200 Subject: [PATCH 3/5] refactor: use unchecked migration --- runtime/altair/src/migrations.rs | 2 +- runtime/centrifuge/src/migrations.rs | 2 +- .../src/migrations/collator_selection_v2.rs | 34 ++++++------------- runtime/development/src/migrations.rs | 2 +- 4 files changed, 14 insertions(+), 26 deletions(-) diff --git a/runtime/altair/src/migrations.rs b/runtime/altair/src/migrations.rs index 5aa63640e0..1093f5bf8e 100644 --- a/runtime/altair/src/migrations.rs +++ b/runtime/altair/src/migrations.rs @@ -23,7 +23,7 @@ pub type UpgradeAltair1035 = ( runtime_common::migrations::increase_storage_version::Migration, runtime_common::migrations::increase_storage_version::Migration, pallet_collator_selection::migration::v1::MigrateToV1, - runtime_common::migrations::collator_selection_v2::MigrationToV2, + runtime_common::migrations::collator_selection_v2::UncheckedMigrationToV2, runtime_common::migrations::loans::AddWithLinearPricing, // As of May 2024, the `pallet_balances::Hold` storage was empty. But better be safe. runtime_common::migrations::hold_reason::MigrateTransferAllowListHolds< diff --git a/runtime/centrifuge/src/migrations.rs b/runtime/centrifuge/src/migrations.rs index 0aaa9c6906..2d05ce0238 100644 --- a/runtime/centrifuge/src/migrations.rs +++ b/runtime/centrifuge/src/migrations.rs @@ -21,7 +21,7 @@ pub type UpgradeCentrifuge1029 = ( runtime_common::migrations::increase_storage_version::Migration, runtime_common::migrations::increase_storage_version::Migration, pallet_collator_selection::migration::v1::MigrateToV1, - runtime_common::migrations::collator_selection_v2::MigrationToV2, + runtime_common::migrations::collator_selection_v2::UncheckedMigrationToV2, runtime_common::migrations::loans::AddWithLinearPricing, runtime_common::migrations::hold_reason::MigrateTransferAllowListHolds< crate::Runtime, diff --git a/runtime/common/src/migrations/collator_selection_v2.rs b/runtime/common/src/migrations/collator_selection_v2.rs index e051d810e7..38a4e68d9e 100644 --- a/runtime/common/src/migrations/collator_selection_v2.rs +++ b/runtime/common/src/migrations/collator_selection_v2.rs @@ -11,13 +11,11 @@ // GNU General Public License for more details. // More info: https://github.com/paritytech/polkadot-sdk/pull/4229#issuecomment-2151690311 -use frame_support::traits::OnRuntimeUpgrade; use frame_support::{ pallet_prelude::*, storage_alias, - traits::{Currency, ReservableCurrency}, + traits::{Currency, OnRuntimeUpgrade, ReservableCurrency}, }; - use pallet_collator_selection::*; use sp_runtime::traits::{Saturating, Zero}; #[cfg(feature = "try-runtime")] @@ -25,17 +23,6 @@ use sp_std::vec::Vec; const LOG_TARGET: &str = "runtime::collator-selection"; -/// [`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, @@ -49,7 +36,7 @@ pub type Candidates = StorageValue< ValueQuery, >; -/// Migrate to V2. +/// Migrate to storage to V2 without bumping storage version because it is missing in v1.7.2 SDK pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for UncheckedMigrationToV2 { fn on_runtime_upgrade() -> Weight { @@ -58,14 +45,15 @@ impl OnRuntimeUpgrade for UncheckedMigratio // 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`. + // 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. + // The new list is empty, so this is essentially being applied correctly. We + // just put the candidates into the new storage item. log::info!( target: LOG_TARGET, "New candidate list is empty, adding {} previous candidates", @@ -76,8 +64,8 @@ impl OnRuntimeUpgrade for UncheckedMigratio 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. + // `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() { diff --git a/runtime/development/src/migrations.rs b/runtime/development/src/migrations.rs index 4df609e315..9f4222234f 100644 --- a/runtime/development/src/migrations.rs +++ b/runtime/development/src/migrations.rs @@ -23,7 +23,7 @@ const IDENTITY_MIGRATION_KEY_LIMIT: u64 = 1000; /// It includes all the migrations that have to be applied on that chain. pub type UpgradeDevelopment1047 = ( pallet_collator_selection::migration::v1::MigrateToV1, - runtime_common::migrations::collator_selection_v2::MigrationToV2, + runtime_common::migrations::collator_selection_v2::UncheckedMigrationToV2, cleanup_foreign_investments::Migration, // v0 -> v1 pallet_multisig::migrations::v1::MigrateToV1, From 8f64c573222b8777e3db1e95a86f5a79da2787a1 Mon Sep 17 00:00:00 2001 From: William Freudenberger Date: Thu, 6 Jun 2024 14:54:20 +0200 Subject: [PATCH 4/5] fmt --- runtime/common/src/migrations/collator_selection_v2.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/runtime/common/src/migrations/collator_selection_v2.rs b/runtime/common/src/migrations/collator_selection_v2.rs index 38a4e68d9e..54f1721ecf 100644 --- a/runtime/common/src/migrations/collator_selection_v2.rs +++ b/runtime/common/src/migrations/collator_selection_v2.rs @@ -36,7 +36,8 @@ pub type Candidates = StorageValue< ValueQuery, >; -/// Migrate to storage to V2 without bumping storage version because it is missing in v1.7.2 SDK +/// Migrate to storage to V2 without bumping storage version because it is +/// missing in v1.7.2 SDK pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for UncheckedMigrationToV2 { fn on_runtime_upgrade() -> Weight { From 952f8f6f4ea1510cde7d0a7263b2ebe1350654fa Mon Sep 17 00:00:00 2001 From: William Freudenberger Date: Thu, 6 Jun 2024 18:40:56 +0200 Subject: [PATCH 5/5] refactor: use public migration --- runtime/altair/src/migrations.rs | 2 +- runtime/centrifuge/src/migrations.rs | 2 +- .../src/migrations/collator_selection_v2.rs | 118 ------------------ runtime/common/src/migrations/mod.rs | 1 - runtime/development/src/migrations.rs | 2 +- 5 files changed, 3 insertions(+), 122 deletions(-) delete mode 100644 runtime/common/src/migrations/collator_selection_v2.rs diff --git a/runtime/altair/src/migrations.rs b/runtime/altair/src/migrations.rs index 1093f5bf8e..13fff00f13 100644 --- a/runtime/altair/src/migrations.rs +++ b/runtime/altair/src/migrations.rs @@ -23,7 +23,7 @@ pub type UpgradeAltair1035 = ( runtime_common::migrations::increase_storage_version::Migration, runtime_common::migrations::increase_storage_version::Migration, pallet_collator_selection::migration::v1::MigrateToV1, - runtime_common::migrations::collator_selection_v2::UncheckedMigrationToV2, + pallet_collator_selection::migration::v2::MigrationToV2, runtime_common::migrations::loans::AddWithLinearPricing, // As of May 2024, the `pallet_balances::Hold` storage was empty. But better be safe. runtime_common::migrations::hold_reason::MigrateTransferAllowListHolds< diff --git a/runtime/centrifuge/src/migrations.rs b/runtime/centrifuge/src/migrations.rs index 2d05ce0238..f96aa66710 100644 --- a/runtime/centrifuge/src/migrations.rs +++ b/runtime/centrifuge/src/migrations.rs @@ -21,7 +21,7 @@ pub type UpgradeCentrifuge1029 = ( runtime_common::migrations::increase_storage_version::Migration, runtime_common::migrations::increase_storage_version::Migration, pallet_collator_selection::migration::v1::MigrateToV1, - runtime_common::migrations::collator_selection_v2::UncheckedMigrationToV2, + pallet_collator_selection::migration::v2::MigrationToV2, runtime_common::migrations::loans::AddWithLinearPricing, runtime_common::migrations::hold_reason::MigrateTransferAllowListHolds< crate::Runtime, diff --git a/runtime/common/src/migrations/collator_selection_v2.rs b/runtime/common/src/migrations/collator_selection_v2.rs deleted file mode 100644 index 54f1721ecf..0000000000 --- a/runtime/common/src/migrations/collator_selection_v2.rs +++ /dev/null @@ -1,118 +0,0 @@ -// Copyright 2024 Centrifuge Foundation (centrifuge.io). -// -// This file is part of the Centrifuge chain project. -// Centrifuge is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version (see http://www.gnu.org/licenses). -// Centrifuge is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// More info: https://github.com/paritytech/polkadot-sdk/pull/4229#issuecomment-2151690311 -use frame_support::{ - pallet_prelude::*, - storage_alias, - traits::{Currency, OnRuntimeUpgrade, ReservableCurrency}, -}; -use pallet_collator_selection::*; -use sp_runtime::traits::{Saturating, Zero}; -#[cfg(feature = "try-runtime")] -use sp_std::vec::Vec; - -const LOG_TARGET: &str = "runtime::collator-selection"; - -#[storage_alias] -pub type Candidates = StorageValue< - Pallet, - BoundedVec< - CandidateInfo< - ::AccountId, - <::Currency as Currency<::AccountId>>::Balance, - >, - ::MaxCandidates, - >, - ValueQuery, ->; - -/// Migrate to storage to V2 without bumping storage version because it is -/// missing in v1.7.2 SDK -pub struct UncheckedMigrationToV2(sp_std::marker::PhantomData); -impl OnRuntimeUpgrade 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. - log::info!( - target: LOG_TARGET, - "New candidate list is empty, adding {} previous candidates", - candidates.len(), - ); - 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), - ); - } - - 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" - ); - let count_pre: u32 = Decode::decode(&mut number_of_candidates.as_slice()) - .expect("pre_upgrade provides a valid state; qed"); - assert_eq!( - count_pre, - CandidateList::::get().len() as u32, - "after migration, the CandidateList should equal old Candidate storage" - ); - Ok(()) - } -} diff --git a/runtime/common/src/migrations/mod.rs b/runtime/common/src/migrations/mod.rs index 9e926ea8a5..dc12bdeb04 100644 --- a/runtime/common/src/migrations/mod.rs +++ b/runtime/common/src/migrations/mod.rs @@ -12,7 +12,6 @@ //! Centrifuge Runtime-Common Migrations -pub mod collator_selection_v2; pub mod hold_reason; pub mod increase_storage_version; pub mod loans; diff --git a/runtime/development/src/migrations.rs b/runtime/development/src/migrations.rs index 9f4222234f..ab4f700a3e 100644 --- a/runtime/development/src/migrations.rs +++ b/runtime/development/src/migrations.rs @@ -23,7 +23,7 @@ const IDENTITY_MIGRATION_KEY_LIMIT: u64 = 1000; /// It includes all the migrations that have to be applied on that chain. pub type UpgradeDevelopment1047 = ( pallet_collator_selection::migration::v1::MigrateToV1, - runtime_common::migrations::collator_selection_v2::UncheckedMigrationToV2, + pallet_collator_selection::migration::v2::MigrationToV2, cleanup_foreign_investments::Migration, // v0 -> v1 pallet_multisig::migrations::v1::MigrateToV1,