Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nomination-pools] Backport of #4503. Fix pools with extra consumer ref to be destroyed. #4524

Merged
merged 1 commit into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions prdoc/pr_4503.prdoc
Original file line number Diff line number Diff line change
@@ -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
15 changes: 15 additions & 0 deletions substrate/frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2268,6 +2268,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);
Expand All @@ -2284,6 +2285,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()
Expand Down
3 changes: 2 additions & 1 deletion substrate/frame/nomination-pools/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
86 changes: 86 additions & 0 deletions substrate/frame/nomination-pools/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4598,6 +4598,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 {
Expand Down
89 changes: 89 additions & 0 deletions substrate/frame/nomination-pools/test-staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<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_slash_e2e() {
new_test_ext().execute_with(|| {
Expand Down
Loading