Allow pool to be destroyed with an extra (erroneous) consumer reference on the pool account

prdoc/pr_4503.prdoc

@@ -0,0 +1,13 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Patch pool to handle extra consumer ref when destroying.
+
+doc:
+  - audience: Runtime User
+    description: |
+      An erroneous consumer reference on the pool account is preventing pools from being destroyed. This patch removes the extra reference if it exists when the pool account is destroyed.
+
+crates:
+  - name: pallet-nomination-pools
+    bump: patch

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

@@ -2254,6 +2254,7 @@ pub mod pallet {
 				SubPoolsStorage::<T>::get(member.pool_id).ok_or(Error::<T>::SubPoolsNotFound)?;
 
 			bonded_pool.ok_to_withdraw_unbonded_with(&caller, &member_account)?;
+			let pool_account = bonded_pool.bonded_account();
 
 			// NOTE: must do this after we have done the `ok_to_withdraw_unbonded_other_with` check.
 			let withdrawn_points = member.withdraw_unlocked(current_era);
@@ -2262,7 +2263,7 @@ pub mod pallet {
 			// Before calculating the `balance_to_unbond`, we call withdraw unbonded to ensure the
 			// `transferrable_balance` is correct.
 			let stash_killed =
-				T::Staking::withdraw_unbonded(bonded_pool.bonded_account(), num_slashing_spans)?;
+				T::Staking::withdraw_unbonded(pool_account.clone(), num_slashing_spans)?;
 
 			// defensive-only: the depositor puts enough funds into the stash so that it will only
 			// be destroyed when they are leaving.
@@ -2271,6 +2272,20 @@ pub mod pallet {
 				Error::<T>::Defensive(DefensiveError::BondedStashKilledPrematurely)
 			);
 
+			if stash_killed {
+				// Maybe an extra consumer left on the pool account, if so, remove it.
+				if frame_system::Pallet::<T>::consumers(&pool_account) == 1 {
+					frame_system::Pallet::<T>::dec_consumers(&pool_account);
+				}
+
+				// Note: This is not pretty, but we have to do this because of a bug where old pool
+				// accounts might have had an extra consumer increment. We know at this point no
+				// other pallet should depend on pool account so safe to do this.
+				// Refer to following issues:
+				// - https://github.com/paritytech/polkadot-sdk/issues/4440
+				// - https://github.com/paritytech/polkadot-sdk/issues/2037
+			}
+
 			let mut sum_unlocked_points: BalanceOf<T> = Zero::zero();
 			let balance_to_unbond = withdrawn_points
 				.iter()

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

@@ -160,7 +160,8 @@ impl sp_staking::StakingInterface for StakingMock {
 		Pools::on_withdraw(&who, unlocking_before.saturating_sub(unlocking(&staker_map)));
 
 		UnbondingBalanceMap::set(&unbonding_map);
-		Ok(UnbondingBalanceMap::get().is_empty() && BondedBalanceMap::get().is_empty())
+		Ok(UnbondingBalanceMap::get().get(&who).unwrap().is_empty() &&
+			BondedBalanceMap::get().get(&who).unwrap().is_zero())
 	}
 
 	fn bond(stash: &Self::AccountId, value: Self::Balance, _: &Self::AccountId) -> DispatchResult {

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

@@ -4594,6 +4594,92 @@ mod withdraw_unbonded {
 			assert_eq!(ClaimPermissions::<Runtime>::contains_key(20), false);
 		});
 	}
+
+	#[test]
+	fn destroy_works_without_erroneous_extra_consumer() {
+		ExtBuilder::default().ed(1).build_and_execute(|| {
+			// 10 is the depositor for pool 1, with min join bond 10.
+			// set pool to destroying.
+			unsafe_set_state(1, PoolState::Destroying);
+
+			// set current era
+			CurrentEra::set(1);
+			assert_ok!(Pools::unbond(RuntimeOrigin::signed(10), 10, 10));
+
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::Created { depositor: 10, pool_id: 1 },
+					Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
+					Event::Unbonded { member: 10, pool_id: 1, balance: 10, points: 10, era: 4 },
+				]
+			);
+
+			// move to era when unbonded funds can be withdrawn.
+			CurrentEra::set(4);
+			assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(10), 10, 0));
+
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::Withdrawn { member: 10, pool_id: 1, points: 10, balance: 10 },
+					Event::MemberRemoved { pool_id: 1, member: 10 },
+					Event::Destroyed { pool_id: 1 },
+				]
+			);
+
+			// pool is destroyed.
+			assert!(!Metadata::<T>::contains_key(1));
+			// ensure the pool account is reaped.
+			assert!(!frame_system::Account::<T>::contains_key(&Pools::create_bonded_account(1)));
+		})
+	}
+
+	#[test]
+	fn destroy_works_with_erroneous_extra_consumer() {
+		ExtBuilder::default().ed(1).build_and_execute(|| {
+			// 10 is the depositor for pool 1, with min join bond 10.
+			let pool_one = Pools::create_bonded_account(1);
+
+			// set pool to destroying.
+			unsafe_set_state(1, PoolState::Destroying);
+
+			// set current era
+			CurrentEra::set(1);
+			assert_ok!(Pools::unbond(RuntimeOrigin::signed(10), 10, 10));
+
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::Created { depositor: 10, pool_id: 1 },
+					Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
+					Event::Unbonded { member: 10, pool_id: 1, balance: 10, points: 10, era: 4 },
+				]
+			);
+
+			// move to era when unbonded funds can be withdrawn.
+			CurrentEra::set(4);
+
+			// increment consumer by 1 reproducing the erroneous consumer bug.
+			// refer https://github.com/paritytech/polkadot-sdk/issues/4440.
+			assert_ok!(frame_system::Pallet::<T>::inc_consumers(&pool_one));
+			assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(10), 10, 0));
+
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::Withdrawn { member: 10, pool_id: 1, points: 10, balance: 10 },
+					Event::MemberRemoved { pool_id: 1, member: 10 },
+					Event::Destroyed { pool_id: 1 },
+				]
+			);
+
+			// pool is destroyed.
+			assert!(!Metadata::<T>::contains_key(1));
+			// ensure the pool account is reaped.
+			assert!(!frame_system::Account::<T>::contains_key(&pool_one));
+		})
+	}
 }
 
 mod create {

substrate/frame/nomination-pools/test-staking/src/lib.rs

@@ -193,6 +193,95 @@ fn pool_lifecycle_e2e() {
 	})
 }
 
+#[test]
+fn destroy_pool_with_erroneous_consumer() {
+	new_test_ext().execute_with(|| {
+		// create the pool, we know this has id 1.
+		assert_ok!(Pools::create(RuntimeOrigin::signed(10), 50, 10, 10, 10));
+		assert_eq!(LastPoolId::<Runtime>::get(), 1);
+
+		// expect consumers on pool account to be 2 (staking lock and an explicit inc by staking).
+		assert_eq!(frame_system::Pallet::<T>::consumers(&POOL1_BONDED), 2);
+
+		// increment consumer by 1 reproducing the erroneous consumer bug.
+		// refer https://github.com/paritytech/polkadot-sdk/issues/4440.
+		assert_ok!(frame_system::Pallet::<T>::inc_consumers(&POOL1_BONDED));
+		assert_eq!(frame_system::Pallet::<T>::consumers(&POOL1_BONDED), 3);
+
+		// have the pool nominate.
+		assert_ok!(Pools::nominate(RuntimeOrigin::signed(10), 1, vec![1, 2, 3]));
+
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Bonded { stash: POOL1_BONDED, amount: 50 }]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![
+				PoolsEvent::Created { depositor: 10, pool_id: 1 },
+				PoolsEvent::Bonded { member: 10, pool_id: 1, bonded: 50, joined: true },
+			]
+		);
+
+		// pool goes into destroying
+		assert_ok!(Pools::set_state(RuntimeOrigin::signed(10), 1, PoolState::Destroying));
+
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::StateChanged { pool_id: 1, new_state: PoolState::Destroying },]
+		);
+
+		// move to era 1
+		CurrentEra::<Runtime>::set(Some(1));
+
+		// depositor need to chill before unbonding
+		assert_noop!(
+			Pools::unbond(RuntimeOrigin::signed(10), 10, 50),
+			pallet_staking::Error::<Runtime>::InsufficientBond
+		);
+
+		assert_ok!(Pools::chill(RuntimeOrigin::signed(10), 1));
+		assert_ok!(Pools::unbond(RuntimeOrigin::signed(10), 10, 50));
+
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![
+				StakingEvent::Chilled { stash: POOL1_BONDED },
+				StakingEvent::Unbonded { stash: POOL1_BONDED, amount: 50 },
+			]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![PoolsEvent::Unbonded {
+				member: 10,
+				pool_id: 1,
+				points: 50,
+				balance: 50,
+				era: 1 + 3
+			}]
+		);
+
+		// waiting bonding duration:
+		CurrentEra::<Runtime>::set(Some(1 + 3));
+		// this should work even with an extra consumer count on pool account.
+		assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(10), 10, 1));
+
+		// pools is fully destroyed now.
+		assert_eq!(
+			staking_events_since_last_call(),
+			vec![StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 50 },]
+		);
+		assert_eq!(
+			pool_events_since_last_call(),
+			vec![
+				PoolsEvent::Withdrawn { member: 10, pool_id: 1, points: 50, balance: 50 },
+				PoolsEvent::MemberRemoved { pool_id: 1, member: 10 },
+				PoolsEvent::Destroyed { pool_id: 1 }
+			]
+		);
+	})
+}
+
 #[test]
 fn pool_chill_e2e() {
 	new_test_ext().execute_with(|| {