Preventing max unbonding pools slots filled in nomination pools

frame/nomination-pools/src/lib.rs

@@ -1626,10 +1626,10 @@ 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.
+		/// If there the unlocking chunks limit has been reached, using 
+        /// [`StakingInterface::apply_requirements_and_unbound`] should free up slots before
+        /// unbonding without having to explicitly call [`Call::pool_withdraw_unbonded`] or
+        /// handle the [`Error::NoMoreChunks`] error.
 		#[pallet::weight(T::WeightInfo::unbond())]
 		#[transactional]
 		pub fn unbond(
@@ -1655,7 +1655,7 @@ pub mod pallet {
 
 			// Unbond in the actual underlying nominator.
 			let unbonding_balance = bonded_pool.dissolve(unbonding_points);
-			T::StakingInterface::unbond(bonded_pool.bonded_account(), unbonding_balance)?;
+			T::StakingInterface::apply_requirements_and_unbond(bonded_pool.bonded_account(), unbonding_balance)?;
 
 			// Note that we lazily create the unbonding pools here if they don't already exist
 			let mut sub_pools = SubPoolsStorage::<T>::get(member.pool_id)

frame/nomination-pools/src/mock.rs

@@ -91,6 +91,10 @@ impl sp_staking::StakingInterface for StakingMock {
 		Ok(())
 	}
 
+	fn apply_requirements_and_unbond(stash: Self::AccountId, value: Self::Balance) -> sp_runtime::DispatchResult {
+		Self::unbond(stash, value)
+	}
+
 	fn chill(_: Self::AccountId) -> sp_runtime::DispatchResult {
 		Ok(())
 	}

frame/staking/src/pallet/impls.rs

@@ -1514,6 +1514,19 @@ impl<T: Config> StakingInterface for Pallet<T> {
 		Self::unbond(RawOrigin::Signed(controller).into(), value)
 	}
 
+	fn apply_requirements_and_unbond(
+		controller: Self::AccountId,
+		value: Self::Balance,
+	) -> DispatchResult {
+		Self::apply_requirements_and_unbond(
+			RawOrigin::Signed(controller).into(),
+			value,
+			T::MaxUnlockingChunks::get(),
+		)
+		.map(|_| ())
+		.map_err(|with_post| with_post.error.into())
+	}
+
 	fn chill(controller: Self::AccountId) -> DispatchResult {
 		Self::chill(RawOrigin::Signed(controller).into())
 	}

frame/staking/src/pallet/mod.rs

@@ -952,7 +952,6 @@ pub mod pallet {
 				ledger.unlocking.len() < T::MaxUnlockingChunks::get() as usize,
 				Error::<T>::NoMoreChunks,
 			);
-
 			let mut value = value.min(ledger.active);
 
 			if !value.is_zero() {
@@ -1005,6 +1004,36 @@ pub mod pallet {
 			Ok(())
 		}
 
+		/// Same as [`Call::unbond`] but it ensures that there are free chunk slots before applying the
+		/// `unbond` by calling `withdraw_unbonded` if required.
+		#[pallet::weight(T::WeightInfo::withdraw_unbonded_kill(*num_slashing_spans) + T::WeightInfo::unbond())]
+		pub fn apply_requirements_and_unbond(
+			origin: OriginFor<T>,
+			#[pallet::compact] value: BalanceOf<T>,
+			num_slashing_spans: u32,
+		) -> DispatchResultWithPostInfo {
+			let controller = ensure_signed(origin.clone())?;
+			let ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
+
+			let post_weight_info = if ledger.unlocking.len() < T::MaxUnlockingChunks::get() as usize
+			{
+				Self::unbond(origin, value)?;
+
+				Some(T::WeightInfo::unbond())
+			} else {
+				Self::withdraw_unbonded(origin.clone(), num_slashing_spans)
+					.map_err(|with_post| with_post.error)?;
+				Self::unbond(origin, value)?;
+
+				Some(
+					T::WeightInfo::withdraw_unbonded_update(num_slashing_spans)
+						+ T::WeightInfo::unbond(),
+				)
+			};
+
+			Ok(post_weight_info.into())
+		}
+
 		/// Remove any unlocked chunks from the `unlocking` queue from our management.
 		///
 		/// This essentially frees up that balance to be used by the stash account to do

frame/staking/src/tests.rs

@@ -34,7 +34,7 @@ use sp_runtime::{
 };
 use sp_staking::{
 	offence::{DisableStrategy, OffenceDetails, OnOffenceHandler},
-	SessionIndex,
+	SessionIndex, StakingInterface,
 };
 use sp_std::prelude::*;
 use substrate_test_utils::assert_eq_uvec;
@@ -325,9 +325,9 @@ fn rewards_should_work() {
 		assert_eq_error_rate!(Balances::total_balance(&21), init_balance_21, 2);
 		assert_eq_error_rate!(
 			Balances::total_balance(&100),
-			init_balance_100 +
-				part_for_100_from_10 * total_payout_0 * 2 / 3 +
-				part_for_100_from_20 * total_payout_0 * 1 / 3,
+			init_balance_100
+				+ part_for_100_from_10 * total_payout_0 * 2 / 3
+				+ part_for_100_from_20 * total_payout_0 * 1 / 3,
 			2
 		);
 		assert_eq_error_rate!(Balances::total_balance(&101), init_balance_101, 2);
@@ -367,9 +367,9 @@ fn rewards_should_work() {
 		assert_eq_error_rate!(Balances::total_balance(&21), init_balance_21, 2);
 		assert_eq_error_rate!(
 			Balances::total_balance(&100),
-			init_balance_100 +
-				part_for_100_from_10 * (total_payout_0 * 2 / 3 + total_payout_1) +
-				part_for_100_from_20 * total_payout_0 * 1 / 3,
+			init_balance_100
+				+ part_for_100_from_10 * (total_payout_0 * 2 / 3 + total_payout_1)
+				+ part_for_100_from_20 * total_payout_0 * 1 / 3,
 			2
 		);
 		assert_eq_error_rate!(Balances::total_balance(&101), init_balance_101, 2);
@@ -1389,6 +1389,55 @@ fn too_many_unbond_calls_should_not_work() {
 	})
 }
 
+#[test]
+fn apply_requirements_and_unbond_should_always_work() {
+	ExtBuilder::default().build_and_execute(|| {
+		let mut current_era = 0;
+
+		// fill up the chunk slots
+		for i in 0..<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get() - 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));
+		}
+
+		current_era += 1;
+		mock::start_active_era(current_era);
+
+		// confirm that the chunk slots are full.
+		assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
+		assert_eq!(
+			Staking::ledger(&10).unwrap().unlocking.len(),
+			<<Test as Config>::MaxUnlockingChunks as Get<u32>>::get() as usize
+		);
+		// can't do more with `unbond`.
+		assert_noop!(Staking::unbond(RuntimeOrigin::signed(10), 1), Error::<Test>::NoMoreChunks);
+
+		// but with `apply_requirements_and_unbond` it should work, as the chunk slots are freed before
+		// unbonding.
+		assert_ok!(Staking::apply_requirements_and_unbond(RuntimeOrigin::signed(10), 1, 0));
+
+		// only chunks that haven't been through the unbonding period remain in the unlocking queue.
+		assert_eq!(
+			Staking::ledger(&10).unwrap().unlocking.len(),
+			Staking::bonding_duration() as usize
+		);
+
+		current_era += 10;
+		mock::start_active_era(current_era);
+
+		assert_ok!(Staking::apply_requirements_and_unbond(RuntimeOrigin::signed(10), 1, 0));
+
+		// the chunks are unlocked only if the `MaxUnlockingChunks` are filled, otherwise proceed as
+		// if the `unbond` was called.
+		assert_eq!(
+			Staking::ledger(&10).unwrap().unlocking.len(),
+			Staking::bonding_duration() as usize + 1
+		);
+	})
+}
+
 #[test]
 fn rebond_works() {
 	//

primitives/staking/src/lib.rs

@@ -127,6 +127,14 @@ pub trait StakingInterface {
 	/// schedules have reached their unlocking era should allow more calls to this function.
 	fn unbond(stash: Self::AccountId, value: Self::Balance) -> DispatchResult;
 
+	/// Same as [`Self::unbond`] but applies any requirements that may be needed for the unbonding to
+	/// be successful. For example, the implementer may need to explicitly unlock era funds by calling
+	/// [`Self::withdraw_unbonded`] before applying the unbond.
+	fn apply_requirements_and_unbond(
+		stash: Self::AccountId,
+		value: Self::Balance,
+	) -> DispatchResult;
+
 	/// Unlock any funds schedule to unlock before or at the current era.
 	///
 	/// Returns whether the stash was killed because of this withdraw or not.