Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
Prevents max unbonding chunk slots from being filled when unbonding i…
Browse files Browse the repository at this point in the history
…n the staking pallet
  • Loading branch information
gpestana committed Nov 1, 2022
1 parent 6195ef4 commit 4e0e816
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 27 deletions.
6 changes: 2 additions & 4 deletions frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1626,10 +1626,8 @@ pub mod pallet {
///
/// # Note
///
/// If there are too many unlocking chunks to unbond with the pool account,
/// [`Call::pool_withdraw_unbonded`] can be called to try and minimize unlocking chunks. If
/// there are too many unlocking chunks, the result of this call will likely be the
/// `NoMoreChunks` error from the staking system.
/// The [`StakingInterface::unbond`] will implicitly call [`Call::pool_withdraw_unbonded`]
/// if there are no left unlocking chunk slots available.
#[pallet::weight(T::WeightInfo::unbond())]
#[transactional]
pub fn unbond(
Expand Down
5 changes: 5 additions & 0 deletions frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ impl<T: Config> Pallet<T> {
Self::slashable_balance_of_vote_weight(who, issuance)
}

/// Returns the number of filled chunks for an account.
pub fn chunk_slots_filled(who: &T::AccountId) -> Result<usize, Error<T>> {
Ok(Self::ledger(&who).ok_or(Error::<T>::NotController)?.unlocking.len())
}

pub(super) fn do_payout_stakers(
validator_stash: T::AccountId,
era: EraIndex,
Expand Down
28 changes: 20 additions & 8 deletions frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -932,26 +932,36 @@ pub mod pallet {
/// the funds out of management ready for transfer.
///
/// No more than a limited number of unlocking chunks (see `MaxUnlockingChunks`)
/// can co-exists at the same time. In that case, [`Call::withdraw_unbonded`] need
/// to be called first to remove some of the chunks (if possible).
/// can co-exists at the same time. If there are no unlocking chunks slots available
/// [`Call::withdraw_unbonded`] is called to remove some of the chunks (if possible).
///
/// If a user encounters the `InsufficientBond` error when calling this extrinsic,
/// they should call `chill` first in order to free up their bonded funds.
///
/// Emits `Unbonded`.
///
/// See also [`Call::withdraw_unbonded`].
#[pallet::weight(T::WeightInfo::unbond())]
#[pallet::weight(
T::WeightInfo::withdraw_unbonded_kill(T::BondingDuration::get()) +
T::WeightInfo::unbond())
]
pub fn unbond(
origin: OriginFor<T>,
#[pallet::compact] value: BalanceOf<T>,
) -> DispatchResult {
let controller = ensure_signed(origin)?;
let controller = ensure_signed(origin.clone())?;

// ensure that there's chunk slots available by requesting the staking interface to
// withdraw chunks older than `BondingDuration`, if there are no more unlocking chunks
// slots available.
if Self::chunk_slots_filled(&controller)? == T::MaxUnlockingChunks::get() as usize {
let num_slashing_spans = T::BondingDuration::get();

Self::withdraw_unbonded(origin.clone(), num_slashing_spans)
.map_err(|with_post| with_post.error)?;
};

let mut ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
ensure!(
ledger.unlocking.len() < T::MaxUnlockingChunks::get() as usize,
Error::<T>::NoMoreChunks,
);

let mut value = value.min(ledger.active);

Expand All @@ -978,6 +988,7 @@ pub mod pallet {

// Note: in case there is no current era it is fine to bond one era more.
let era = Self::current_era().unwrap_or(0) + T::BondingDuration::get();

if let Some(mut chunk) =
ledger.unlocking.last_mut().filter(|chunk| chunk.era == era)
{
Expand Down Expand Up @@ -1028,6 +1039,7 @@ pub mod pallet {
let controller = ensure_signed(origin)?;
let mut ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
let (stash, old_total) = (ledger.stash.clone(), ledger.total);

if let Some(current_era) = Self::current_era() {
ledger = ledger.consolidate_unlocked(current_era)
}
Expand Down
33 changes: 18 additions & 15 deletions frame/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1350,12 +1350,14 @@ fn bond_extra_and_withdraw_unbonded_works() {
}

#[test]
fn too_many_unbond_calls_should_not_work() {
fn many_unbond_calls_should_work() {
ExtBuilder::default().build_and_execute(|| {
let mut current_era = 0;
// locked at era MaxUnlockingChunks - 1 until 3

for i in 0..<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get() - 1 {
let max_unlocking_chunks = <<Test as Config>::MaxUnlockingChunks as Get<u32>>::get();

for i in 0..max_unlocking_chunks - 1 {
// There is only 1 chunk per era, so we need to be in a new era to create a chunk.
current_era = i as u32;
mock::start_active_era(current_era);
Expand All @@ -1369,23 +1371,24 @@ fn too_many_unbond_calls_should_not_work() {
// == 3).
assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
assert_eq!(
Staking::ledger(&10).unwrap().unlocking.len(),
Staking::chunk_slots_filled(&10).unwrap(),
<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get() as usize
);
// can't do more.
assert_noop!(Staking::unbond(RuntimeOrigin::signed(10), 1), Error::<Test>::NoMoreChunks);

current_era += 2;
mock::start_active_era(current_era);

assert_noop!(Staking::unbond(RuntimeOrigin::signed(10), 1), Error::<Test>::NoMoreChunks);
// free up everything except the most recently added chunk.
assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(10), 0));
assert_eq!(Staking::ledger(&10).unwrap().unlocking.len(), 1);
// even though the number of unlocked chunks is the same as `MaxUnlockingChunks`,
// unbonding works as expected.
for i in current_era..(current_era + max_unlocking_chunks) - 1 {
// There is only 1 chunk per era, so we need to be in a new era to create a chunk.
current_era = i as u32;
mock::start_active_era(current_era);
assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
}

// Can add again.
assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
assert_eq!(Staking::ledger(&10).unwrap().unlocking.len(), 2);
// only slots of unbonds within last BondingDuration are filled.
assert_eq!(
Staking::chunk_slots_filled(&10).unwrap(),
<<Test as Config>::BondingDuration>::get() as usize
);
})
}

Expand Down

0 comments on commit 4e0e816

Please sign in to comment.