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

addresses #4440 (will
close once we have this in prod runtimes).
related: #2037.

An extra consumer reference is preventing pools to be destroyed. When a
pool is ready to be destroyed, we
can safely clear the consumer references if any. Notably, I only check
for one extra consumer reference since that is a known bug. Anything
more indicates possibly another issue and we probably don't want to
silently absorb those errors as well.

After this change, pools with extra consumer reference should be able to
destroy normally.

Author: Ank4n

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(|| {