diff --git a/prdoc/pr_4503.prdoc b/prdoc/pr_4503.prdoc
new file mode 100644
index 0000000000000000000000000000000000000000..d95a24cc7d6b6281761d81b4ab2ccdc8b14e4550
--- /dev/null
+++ b/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
diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs
index 0fdb7e3eff5ccca340a5b26f00010e0240377e03..95d23f2280a7e57e76a491c62536174d23f204af 100644
--- a/substrate/frame/nomination-pools/src/lib.rs
+++ b/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()
diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs
index 686402b843492e8f92bb26231a7ed3becf0c0e03..e34719a7b80e87cef65cfc5dd638ee4ca5573803 100644
--- a/substrate/frame/nomination-pools/src/mock.rs
+++ b/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 {
diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs
index f6ef1e6eaac217816e410bc85a837f30bda179f4..535e7537469ecd77a8eb261c895b4d5fd264ecfb 100644
--- a/substrate/frame/nomination-pools/src/tests.rs
+++ b/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 {
diff --git a/substrate/frame/nomination-pools/test-staking/src/lib.rs b/substrate/frame/nomination-pools/test-staking/src/lib.rs
index d84e09e32ba353267e3a2629a372bd189137454d..aa91350259000b810eb63a0036754517eeb8421d 100644
--- a/substrate/frame/nomination-pools/test-staking/src/lib.rs
+++ b/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(|| {