From 3f5b01be1767dc37b87d5aa429d4bfba27df16c7 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 | 15 ++++ 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, 205 insertions(+), 1 deletion(-) create mode 100644 prdoc/pr_4503.prdoc diff --git a/prdoc/pr_4503.prdoc b/prdoc/pr_4503.prdoc new file mode 100644 index 000000000000..d95a24cc7d6b --- /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 c2653d8bdabc..c9a98a21886a 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2268,6 +2268,7 @@ pub mod pallet { SubPoolsStorage::::get(member.pool_id).ok_or(Error::::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); @@ -2284,6 +2285,20 @@ pub mod pallet { Error::::Defensive(DefensiveError::BondedStashKilledPrematurely) ); + if stash_killed { + // Maybe an extra consumer left on the pool account, if so, remove it. + if frame_system::Pallet::::consumers(&pool_account) == 1 { + frame_system::Pallet::::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 = 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 84c41a15b201..b8d4c4a1f94b 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -140,7 +140,8 @@ impl sp_staking::StakingInterface for StakingMock { staker_map.retain(|(unlocking_at, _amount)| *unlocking_at > current_era); 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 47f48ba98b7f..60a75f09b75d 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -4598,6 +4598,92 @@ mod withdraw_unbonded { assert_eq!(ClaimPermissions::::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::::contains_key(1)); + // ensure the pool account is reaped. + assert!(!frame_system::Account::::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::::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::::contains_key(1)); + // ensure the pool account is reaped. + assert!(!frame_system::Account::::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 865b7a71e688..067b2edf1450 100644 --- a/substrate/frame/nomination-pools/test-staking/src/lib.rs +++ b/substrate/frame/nomination-pools/test-staking/src/lib.rs @@ -191,6 +191,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::::get(), 1); + + // expect consumers on pool account to be 2 (staking lock and an explicit inc by staking). + assert_eq!(frame_system::Pallet::::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::::inc_consumers(&POOL1_BONDED)); + assert_eq!(frame_system::Pallet::::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::::set(Some(1)); + + // depositor need to chill before unbonding + assert_noop!( + Pools::unbond(RuntimeOrigin::signed(10), 10, 50), + pallet_staking::Error::::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::::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_slash_e2e() { new_test_ext().execute_with(|| {