[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

@@ -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<T: Config> Pallet<T> {
 	}
 	/// 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<T>) {
 		let reward_account = bonded_pool.reward_account();
 		let bonded_account = bonded_pool.bonded_account();
@@ -2229,6 +2229,9 @@ impl<T: Config> Pallet<T> {
 		T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero());
 
 		Self::deposit_event(Event::<T>::Destroyed { pool_id: bonded_pool.id });
+		// Remove bonded pool metadata.
+		Metadata::<T>::remove(bonded_pool.id);
+
 		bonded_pool.remove();
 	}
 
@@ -2406,8 +2409,7 @@ impl<T: Config> Pallet<T> {
 		let reward_pools = RewardPools::<T>::iter_keys().collect::<Vec<_>>();
 		assert_eq!(bonded_pools, reward_pools);
 
-		// TODO: can't check this right now: https://github.com/paritytech/substrate/issues/12077
-		// assert!(Metadata::<T>::iter_keys().all(|k| bonded_pools.contains(&k)));
+		assert!(Metadata::<T>::iter_keys().all(|k| bonded_pools.contains(&k)));
 		assert!(SubPoolsStorage::<T>::iter_keys().all(|k| bonded_pools.contains(&k)));
 
 		assert!(MaxPools::<T>::get().map_or(true, |max| bonded_pools.len() <= (max as usize)));

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::<T>::on_chain_storage_version(), 1);
+			Pallet::<T>::try_state(frame_system::Pallet::<T>::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<T>(sp_std::marker::PhantomData<T>);
+	impl<T: Config> OnRuntimeUpgrade for MigrateToV3<T> {
+		fn on_runtime_upgrade() -> Weight {
+			let current = Pallet::<T>::current_storage_version();
+			let onchain = Pallet::<T>::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::<T>::iter_keys()
+					.filter(|id| {
+						metadata_iterated += 1;
+						!BondedPools::<T>::contains_key(&id)
+					})
+					.collect::<Vec<_>>()
+					.into_iter()
+					.for_each(|id| {
+						metadata_removed += 1;
+						Metadata::<T>::remove(&id);
+					});
+				current.put::<Pallet<T>>();
+				// 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::<T>::current_storage_version() > Pallet::<T>::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::<T>::iter_keys().all(|id| BondedPools::<T>::contains_key(&id)),
+				"not all of the stale metadata has been removed"
+			);
+			ensure!(Pallet::<T>::on_chain_storage_version() == 3, "wrong storage version");
+			Ok(())
+		}
+	}
+}

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::<Runtime>::get();
 			for (account_id, bonded) in self.members {
 				Balances::make_free_balance_be(&account_id, bonded * 2);

substrate/frame/nomination-pools/src/tests.rs

@@ -47,6 +47,7 @@ fn test_setup_works() {
 		assert_eq!(SubPoolsStorage::<Runtime>::count(), 0);
 		assert_eq!(PoolMembers::<Runtime>::count(), 1);
 		assert_eq!(StakingMock::bonding_duration(), 3);
+		assert!(Metadata::<T>::contains_key(1));
 
 		let last_pool = LastPoolId::<Runtime>::get();
 		assert_eq!(
@@ -1928,6 +1929,7 @@ mod claim_payout {
 				]
 			);
 
+			assert!(!Metadata::<T>::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::<T>::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::<T>::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::<T>::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::<T>::contains_key(1));
 		})
 	}
 }
@@ -4039,7 +4048,7 @@ mod set_state {
 			// Then
 			assert_eq!(BondedPool::<Runtime>::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!(