From d213e95784e2dd64f924852c86be2321d6c9a258 Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Tue, 6 Sep 2022 08:34:29 +0200 Subject: [PATCH] [Fix] Make sure pool metadata is removed on pool dissolve (#12154) * [Fix] Make sure pool metadata is removed on pool dissolve * add migration * remove_metadata helper removed * fix typo and add a comment * fix pre_upgrade * fix migration * Update frame/nomination-pools/src/migration.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Update frame/nomination-pools/src/migration.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * address comments * fix comments * Update frame/nomination-pools/src/migration.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * address comments * permissions fix Co-authored-by: parity-processbot <> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- substrate/frame/nomination-pools/src/lib.rs | 12 ++-- .../frame/nomination-pools/src/migration.rs | 65 +++++++++++++++++++ substrate/frame/nomination-pools/src/mock.rs | 2 +- substrate/frame/nomination-pools/src/tests.rs | 11 +++- 4 files changed, 83 insertions(+), 7 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 3809a70440..a8c4e50daa 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1141,7 +1141,7 @@ pub mod pallet { use sp_runtime::traits::CheckedAdd; /// The current storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(2); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(3); #[pallet::pallet] #[pallet::generate_store(pub(crate) trait Store)] @@ -2191,8 +2191,8 @@ impl Pallet { } /// Remove everything related to the given bonded pool. /// - /// All sub-pools are also deleted. All accounts are dusted and the leftover of the reward - /// account is returned to the depositor. + /// Metadata and all of the sub-pools are also deleted. All accounts are dusted and the leftover + /// of the reward account is returned to the depositor. pub fn dissolve_pool(bonded_pool: BondedPool) { let reward_account = bonded_pool.reward_account(); let bonded_account = bonded_pool.bonded_account(); @@ -2229,6 +2229,9 @@ impl Pallet { T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero()); Self::deposit_event(Event::::Destroyed { pool_id: bonded_pool.id }); + // Remove bonded pool metadata. + Metadata::::remove(bonded_pool.id); + bonded_pool.remove(); } @@ -2406,8 +2409,7 @@ impl Pallet { let reward_pools = RewardPools::::iter_keys().collect::>(); assert_eq!(bonded_pools, reward_pools); - // TODO: can't check this right now: https://github.com/paritytech/substrate/issues/12077 - // assert!(Metadata::::iter_keys().all(|k| bonded_pools.contains(&k))); + assert!(Metadata::::iter_keys().all(|k| bonded_pools.contains(&k))); assert!(SubPoolsStorage::::iter_keys().all(|k| bonded_pools.contains(&k))); assert!(MaxPools::::get().map_or(true, |max| bonded_pools.len() <= (max as usize))); diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 412c954a2b..bcf23ee863 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -100,6 +100,7 @@ pub mod v1 { fn post_upgrade() -> Result<(), &'static str> { // new version must be set. assert_eq!(Pallet::::on_chain_storage_version(), 1); + Pallet::::try_state(frame_system::Pallet::::block_number())?; Ok(()) } } @@ -384,3 +385,67 @@ pub mod v2 { } } } + +pub mod v3 { + use super::*; + + /// This migration removes stale bonded-pool metadata, if any. + pub struct MigrateToV3(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV3 { + fn on_runtime_upgrade() -> Weight { + let current = Pallet::::current_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + log!( + info, + "Running migration with current storage version {:?} / onchain {:?}", + current, + onchain + ); + + if current > onchain { + let mut metadata_iterated = 0u64; + let mut metadata_removed = 0u64; + Metadata::::iter_keys() + .filter(|id| { + metadata_iterated += 1; + !BondedPools::::contains_key(&id) + }) + .collect::>() + .into_iter() + .for_each(|id| { + metadata_removed += 1; + Metadata::::remove(&id); + }); + current.put::>(); + // metadata iterated + bonded pools read + a storage version read + let total_reads = metadata_iterated * 2 + 1; + // metadata removed + a storage version write + let total_writes = metadata_removed + 1; + T::DbWeight::get().reads_writes(total_reads, total_writes) + } else { + log!(info, "MigrateToV3 should be removed"); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<(), &'static str> { + ensure!( + Pallet::::current_storage_version() > Pallet::::on_chain_storage_version(), + "the on_chain version is equal or more than the current one" + ); + Ok(()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade() -> Result<(), &'static str> { + ensure!( + Metadata::::iter_keys().all(|id| BondedPools::::contains_key(&id)), + "not all of the stale metadata has been removed" + ); + ensure!(Pallet::::on_chain_storage_version() == 3, "wrong storage version"); + Ok(()) + } + } +} diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 3bb260e56f..4428c28558 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -290,7 +290,7 @@ impl ExtBuilder { let amount_to_bond = Pools::depositor_min_bond(); Balances::make_free_balance_be(&10, amount_to_bond * 5); assert_ok!(Pools::create(RawOrigin::Signed(10).into(), amount_to_bond, 900, 901, 902)); - + assert_ok!(Pools::set_metadata(Origin::signed(900), 1, vec![1, 1])); let last_pool = LastPoolId::::get(); for (account_id, bonded) in self.members { Balances::make_free_balance_be(&account_id, bonded * 2); diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 705f8ce3a6..20ba2b76fe 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -47,6 +47,7 @@ fn test_setup_works() { assert_eq!(SubPoolsStorage::::count(), 0); assert_eq!(PoolMembers::::count(), 1); assert_eq!(StakingMock::bonding_duration(), 3); + assert!(Metadata::::contains_key(1)); let last_pool = LastPoolId::::get(); assert_eq!( @@ -1928,6 +1929,7 @@ mod claim_payout { ] ); + assert!(!Metadata::::contains_key(1)); // original ed + ed put into reward account + reward + bond + dust. assert_eq!(Balances::free_balance(&10), 35 + 5 + 13 + 10 + 1); }) @@ -3159,6 +3161,7 @@ mod withdraw_unbonded { Event::Destroyed { pool_id: 1 } ] ); + assert!(!Metadata::::contains_key(1)); assert_eq!( balances_events_since_last_call(), vec![ @@ -3269,6 +3272,10 @@ mod withdraw_unbonded { CurrentEra::set(CurrentEra::get() + 3); + // set metadata to check that it's being removed on dissolve + assert_ok!(Pools::set_metadata(Origin::signed(900), 1, vec![1, 1])); + assert!(Metadata::::contains_key(1)); + // when assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0)); @@ -3287,6 +3294,7 @@ mod withdraw_unbonded { Event::Destroyed { pool_id: 1 } ] ); + assert!(!Metadata::::contains_key(1)); assert_eq!( balances_events_since_last_call(), vec![ @@ -3797,6 +3805,7 @@ mod withdraw_unbonded { Event::Destroyed { pool_id: 1 }, ] ); + assert!(!Metadata::::contains_key(1)); }) } } @@ -4039,7 +4048,7 @@ mod set_state { // Then assert_eq!(BondedPool::::get(1).unwrap().state, PoolState::Destroying); - // If the pool is not ok to be open, it cannot be permissionleslly set to a state that + // If the pool is not ok to be open, it cannot be permissionlessly set to a state that // isn't destroying unsafe_set_state(1, PoolState::Open); assert_noop!(