Skip to content

Commit 3dc463e

Browse files
joepetrowskigeorgepisaltuggwpez
authored andcommitted
Fix Stuck Collator Funds (#4229)
Fixes #4206 In #1340 one of the storage types was changed from `Candidates` to `CandidateList`. Since the actual key includes the hash of this value, all of the candidates stored here are (a) "missing" and (b) unable to unreserve their candidacy bond. This migration kills the storage values and refunds the deposit held for each candidate. --------- Signed-off-by: georgepisaltu <[email protected]> Co-authored-by: georgepisaltu <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: georgepisaltu <[email protected]>
1 parent 300a2ac commit 3dc463e

File tree

11 files changed

+262
-9
lines changed

11 files changed

+262
-9
lines changed

cumulus/pallets/collator-selection/Cargo.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ sp-staking = { path = "../../../substrate/primitives/staking", default-features
2424
frame-support = { path = "../../../substrate/frame/support", default-features = false, version = "27.0.0" }
2525
frame-system = { path = "../../../substrate/frame/system", default-features = false, version = "27.0.0" }
2626
pallet-authorship = { path = "../../../substrate/frame/authorship", default-features = false, version = "27.0.0" }
27+
pallet-balances = { path = "../../../substrate/frame/balances", default-features = false, version = "27.0.0" }
2728
pallet-session = { path = "../../../substrate/frame/session", default-features = false, version = "27.0.0" }
2829

2930
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" }
3536
sp-runtime = { path = "../../../substrate/primitives/runtime" }
3637
pallet-timestamp = { path = "../../../substrate/frame/timestamp" }
3738
sp-consensus-aura = { path = "../../../substrate/primitives/consensus/aura" }
38-
pallet-balances = { path = "../../../substrate/frame/balances" }
3939
pallet-aura = { path = "../../../substrate/frame/aura" }
4040

4141
[features]
@@ -54,6 +54,7 @@ std = [
5454
"frame-system/std",
5555
"log/std",
5656
"pallet-authorship/std",
57+
"pallet-balances/std",
5758
"pallet-session/std",
5859
"rand/std",
5960
"scale-info/std",

cumulus/pallets/collator-selection/src/lib.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,8 @@ pub mod pallet {
118118
use sp_staking::SessionIndex;
119119
use sp_std::vec::Vec;
120120

121-
/// The current storage version.
122-
const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
121+
/// The in-code storage version.
122+
const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);
123123

124124
type BalanceOf<T> =
125125
<<T as Config>::Currency as Currency<<T as SystemConfig>::AccountId>>::Balance;

cumulus/pallets/collator-selection/src/migration.rs

Lines changed: 232 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,107 @@
1717
//! A module that is responsible for migration of storage for Collator Selection.
1818
1919
use super::*;
20-
use frame_support::traits::OnRuntimeUpgrade;
20+
use frame_support::traits::{OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade};
2121
use log;
2222

23+
/// Migrate to v2. Should have been part of <https://github.com/paritytech/polkadot-sdk/pull/1340>.
24+
pub mod v2 {
25+
use super::*;
26+
use frame_support::{
27+
pallet_prelude::*,
28+
storage_alias,
29+
traits::{Currency, ReservableCurrency},
30+
};
31+
use sp_runtime::traits::{Saturating, Zero};
32+
#[cfg(feature = "try-runtime")]
33+
use sp_std::vec::Vec;
34+
35+
/// [`UncheckedMigrationToV2`] wrapped in a
36+
/// [`VersionedMigration`](frame_support::migrations::VersionedMigration), ensuring the
37+
/// migration is only performed when on-chain version is 1.
38+
pub type MigrationToV2<T> = frame_support::migrations::VersionedMigration<
39+
1,
40+
2,
41+
UncheckedMigrationToV2<T>,
42+
Pallet<T>,
43+
<T as frame_system::Config>::DbWeight,
44+
>;
45+
46+
#[storage_alias]
47+
pub type Candidates<T: Config> = StorageValue<
48+
Pallet<T>,
49+
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>,
50+
ValueQuery,
51+
>;
52+
53+
/// Migrate to V2.
54+
pub struct UncheckedMigrationToV2<T>(sp_std::marker::PhantomData<T>);
55+
impl<T: Config + pallet_balances::Config> UncheckedOnRuntimeUpgrade for UncheckedMigrationToV2<T> {
56+
fn on_runtime_upgrade() -> Weight {
57+
let mut weight = Weight::zero();
58+
let mut count: u64 = 0;
59+
// candidates who exist under the old `Candidates` key
60+
let candidates = Candidates::<T>::take();
61+
62+
// New candidates who have registered since the upgrade. Under normal circumstances,
63+
// this should not exist because the migration should be applied when the upgrade
64+
// happens. But in Polkadot/Kusama we messed this up, and people registered under
65+
// `CandidateList` while their funds were locked in `Candidates`.
66+
let new_candidate_list = CandidateList::<T>::get();
67+
if new_candidate_list.len().is_zero() {
68+
// The new list is empty, so this is essentially being applied correctly. We just
69+
// put the candidates into the new storage item.
70+
CandidateList::<T>::put(&candidates);
71+
// 1 write for the new list
72+
weight.saturating_accrue(T::DbWeight::get().reads_writes(0, 1));
73+
} else {
74+
// Oops, the runtime upgraded without the migration. There are new candidates in
75+
// `CandidateList`. So, let's just refund the old ones and assume they have already
76+
// started participating in the new system.
77+
for candidate in candidates {
78+
let err = T::Currency::unreserve(&candidate.who, candidate.deposit);
79+
if err > Zero::zero() {
80+
log::error!(
81+
target: LOG_TARGET,
82+
"{:?} balance was unable to be unreserved from {:?}",
83+
err, &candidate.who,
84+
);
85+
}
86+
count.saturating_inc();
87+
}
88+
weight.saturating_accrue(
89+
<<T as pallet_balances::Config>::WeightInfo as pallet_balances::WeightInfo>::force_unreserve().saturating_mul(count.into()),
90+
);
91+
}
92+
93+
log::info!(
94+
target: LOG_TARGET,
95+
"Unreserved locked bond of {} candidates, upgraded storage to version 2",
96+
count,
97+
);
98+
99+
weight.saturating_accrue(T::DbWeight::get().reads_writes(3, 2));
100+
weight
101+
}
102+
103+
#[cfg(feature = "try-runtime")]
104+
fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::DispatchError> {
105+
let number_of_candidates = Candidates::<T>::get().to_vec().len();
106+
Ok((number_of_candidates as u32).encode())
107+
}
108+
109+
#[cfg(feature = "try-runtime")]
110+
fn post_upgrade(_number_of_candidates: Vec<u8>) -> Result<(), sp_runtime::DispatchError> {
111+
let new_number_of_candidates = Candidates::<T>::get().to_vec().len();
112+
assert_eq!(
113+
new_number_of_candidates, 0 as usize,
114+
"after migration, the candidates map should be empty"
115+
);
116+
Ok(())
117+
}
118+
}
119+
}
120+
23121
/// Version 1 Migration
24122
/// This migration ensures that any existing `Invulnerables` storage lists are sorted.
25123
pub mod v1 {
@@ -90,3 +188,136 @@ pub mod v1 {
90188
}
91189
}
92190
}
191+
192+
#[cfg(all(feature = "try-runtime", test))]
193+
mod tests {
194+
use super::*;
195+
use crate::{
196+
migration::v2::Candidates,
197+
mock::{new_test_ext, Balances, Test},
198+
};
199+
use frame_support::{
200+
traits::{Currency, ReservableCurrency, StorageVersion},
201+
BoundedVec,
202+
};
203+
use sp_runtime::traits::ConstU32;
204+
205+
#[test]
206+
fn migrate_to_v2_with_new_candidates() {
207+
new_test_ext().execute_with(|| {
208+
let storage_version = StorageVersion::new(1);
209+
storage_version.put::<Pallet<Test>>();
210+
211+
let one = 1u64;
212+
let two = 2u64;
213+
let three = 3u64;
214+
let deposit = 10u64;
215+
216+
// Set balance to 100
217+
Balances::make_free_balance_be(&one, 100u64);
218+
Balances::make_free_balance_be(&two, 100u64);
219+
Balances::make_free_balance_be(&three, 100u64);
220+
221+
// Reservations: 10 for the "old" candidacy and 10 for the "new"
222+
Balances::reserve(&one, 10u64).unwrap(); // old
223+
Balances::reserve(&two, 20u64).unwrap(); // old + new
224+
Balances::reserve(&three, 10u64).unwrap(); // new
225+
226+
// Candidate info
227+
let candidate_one = CandidateInfo { who: one, deposit };
228+
let candidate_two = CandidateInfo { who: two, deposit };
229+
let candidate_three = CandidateInfo { who: three, deposit };
230+
231+
// Storage lists
232+
let bounded_candidates =
233+
BoundedVec::<CandidateInfo<u64, u64>, ConstU32<20>>::try_from(vec![
234+
candidate_one.clone(),
235+
candidate_two.clone(),
236+
])
237+
.expect("it works");
238+
let bounded_candidate_list =
239+
BoundedVec::<CandidateInfo<u64, u64>, ConstU32<20>>::try_from(vec![
240+
candidate_two.clone(),
241+
candidate_three.clone(),
242+
])
243+
.expect("it works");
244+
245+
// Set storage
246+
Candidates::<Test>::put(bounded_candidates);
247+
CandidateList::<Test>::put(bounded_candidate_list.clone());
248+
249+
// Sanity check
250+
assert_eq!(Balances::free_balance(one), 90);
251+
assert_eq!(Balances::free_balance(two), 80);
252+
assert_eq!(Balances::free_balance(three), 90);
253+
254+
// Run migration
255+
v2::MigrationToV2::<Test>::on_runtime_upgrade();
256+
257+
let new_storage_version = StorageVersion::get::<Pallet<Test>>();
258+
assert_eq!(new_storage_version, 2);
259+
260+
// 10 should have been unreserved from the old candidacy
261+
assert_eq!(Balances::free_balance(one), 100);
262+
assert_eq!(Balances::free_balance(two), 90);
263+
assert_eq!(Balances::free_balance(three), 90);
264+
// The storage item should be gone
265+
assert!(Candidates::<Test>::get().is_empty());
266+
// The new storage item should be preserved
267+
assert_eq!(CandidateList::<Test>::get(), bounded_candidate_list);
268+
});
269+
}
270+
271+
#[test]
272+
fn migrate_to_v2_without_new_candidates() {
273+
new_test_ext().execute_with(|| {
274+
let storage_version = StorageVersion::new(1);
275+
storage_version.put::<Pallet<Test>>();
276+
277+
let one = 1u64;
278+
let two = 2u64;
279+
let deposit = 10u64;
280+
281+
// Set balance to 100
282+
Balances::make_free_balance_be(&one, 100u64);
283+
Balances::make_free_balance_be(&two, 100u64);
284+
285+
// Reservations
286+
Balances::reserve(&one, 10u64).unwrap(); // old
287+
Balances::reserve(&two, 10u64).unwrap(); // old
288+
289+
// Candidate info
290+
let candidate_one = CandidateInfo { who: one, deposit };
291+
let candidate_two = CandidateInfo { who: two, deposit };
292+
293+
// Storage lists
294+
let bounded_candidates =
295+
BoundedVec::<CandidateInfo<u64, u64>, ConstU32<20>>::try_from(vec![
296+
candidate_one.clone(),
297+
candidate_two.clone(),
298+
])
299+
.expect("it works");
300+
301+
// Set storage
302+
Candidates::<Test>::put(bounded_candidates.clone());
303+
304+
// Sanity check
305+
assert_eq!(Balances::free_balance(one), 90);
306+
assert_eq!(Balances::free_balance(two), 90);
307+
308+
// Run migration
309+
v2::MigrationToV2::<Test>::on_runtime_upgrade();
310+
311+
let new_storage_version = StorageVersion::get::<Pallet<Test>>();
312+
assert_eq!(new_storage_version, 2);
313+
314+
// Nothing changes deposit-wise
315+
assert_eq!(Balances::free_balance(one), 90);
316+
assert_eq!(Balances::free_balance(two), 90);
317+
// The storage item should be gone
318+
assert!(Candidates::<Test>::get().is_empty());
319+
// The new storage item should have the info now
320+
assert_eq!(CandidateList::<Test>::get(), bounded_candidates);
321+
});
322+
}
323+
}

cumulus/parachain-template/runtime/src/lib.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,13 @@ pub type SignedExtra = (
113113
pub type UncheckedExtrinsic =
114114
generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;
115115

116+
/// Migrations to apply on runtime upgrade.
117+
pub type Migrations = (
118+
pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
119+
// permanent
120+
pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>,
121+
);
122+
116123
/// Executive: handles dispatch to the various modules.
117124
pub type Executive = frame_executive::Executive<
118125
Runtime,

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/lib.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -945,10 +945,12 @@ pub type UncheckedExtrinsic =
945945
generic::UncheckedExtrinsic<Address, RuntimeCall, Signature, SignedExtra>;
946946
/// Migrations to apply on runtime upgrade.
947947
pub type Migrations = (
948-
pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
949948
InitStorageVersions,
950949
// unreleased
951950
cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4<Runtime>,
951+
pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
952+
// permanent
953+
pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>,
952954
);
953955

954956
/// Migration to initialize storage versions for pallets added after genesis.

cumulus/parachains/runtimes/assets/asset-hub-westend/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -922,7 +922,7 @@ pub type Migrations = (
922922
// v9420
923923
pallet_nfts::migration::v1::MigrateToV1<Runtime>,
924924
// unreleased
925-
pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
925+
pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
926926
// unreleased
927927
migrations::NativeAssetParents0ToParents1Migration<Runtime>,
928928
// unreleased

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ pub type UncheckedExtrinsic =
115115

116116
/// Migrations to apply on runtime upgrade.
117117
pub type Migrations = (
118-
pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
118+
pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
119119
pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
120120
InitStorageVersions,
121121
cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4<Runtime>,

cumulus/parachains/runtimes/bridge-hubs/bridge-hub-westend/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ pub type UncheckedExtrinsic =
115115

116116
/// Migrations to apply on runtime upgrade.
117117
pub type Migrations = (
118-
pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
118+
pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
119119
pallet_multisig::migrations::v1::MigrateToV1<Runtime>,
120120
InitStorageVersions,
121121
// unreleased

cumulus/parachains/runtimes/collectives/collectives-westend/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -699,7 +699,7 @@ pub type UncheckedExtrinsic =
699699
/// `OnRuntimeUpgrade`. Included migrations must be idempotent.
700700
type Migrations = (
701701
// unreleased
702-
pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
702+
pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
703703
// unreleased
704704
cumulus_pallet_xcmp_queue::migration::v4::MigrationToV4<Runtime>,
705705
);

cumulus/parachains/runtimes/contracts/contracts-rococo/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,8 @@ pub type UncheckedExtrinsic =
9999

100100
/// Migrations to apply on runtime upgrade.
101101
pub type Migrations = (
102+
pallet_collator_selection::migration::v1::MigrateToV1<Runtime>,
103+
pallet_collator_selection::migration::v2::MigrationToV2<Runtime>,
102104
cumulus_pallet_parachain_system::migration::Migration<Runtime>,
103105
cumulus_pallet_xcmp_queue::migration::v2::MigrationToV2<Runtime>,
104106
cumulus_pallet_xcmp_queue::migration::v3::MigrationToV3<Runtime>,

prdoc/pr_4229.prdoc

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
title: "Fix Stuck Collator Funds"
2+
3+
doc:
4+
- audience: Runtime Dev
5+
description: |
6+
Fixes stuck collator funds by providing a migration that should have been in PR 1340.
7+
8+
crates:
9+
- name: pallet-collator-selection
10+
bump: patch

0 commit comments

Comments
 (0)