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