From 2e36f571e5c9486819b85561d12fa4001018e953 Mon Sep 17 00:00:00 2001
From: Ankan <10196091+Ank4n@users.noreply.github.com>
Date: Fri, 17 May 2024 14:09:00 +0200
Subject: [PATCH] Allow pool to be destroyed with an extra (erroneous) consumer
reference on the pool account (#4503)
addresses https://github.com/paritytech/polkadot-sdk/issues/4440 (will
close once we have this in prod runtimes).
related: https://github.com/paritytech/polkadot-sdk/issues/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.
---
prdoc/pr_4503.prdoc | 13 +++
substrate/frame/nomination-pools/src/lib.rs | 17 +++-
substrate/frame/nomination-pools/src/mock.rs | 3 +-
substrate/frame/nomination-pools/src/tests.rs | 86 ++++++++++++++++++
.../nomination-pools/test-staking/src/lib.rs | 89 +++++++++++++++++++
5 files changed, 206 insertions(+), 2 deletions(-)
create mode 100644 prdoc/pr_4503.prdoc
diff --git a/prdoc/pr_4503.prdoc b/prdoc/pr_4503.prdoc
new file mode 100644
index 00000000000..d95a24cc7d6
--- /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 0fdb7e3eff5..95d23f2280a 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 686402b8434..e34719a7b80 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 f6ef1e6eaac..535e7537469 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 d84e09e32ba..aa913502590 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(|| {
--
GitLab