Prevents max unbonding chunk slots from being filled when unbonding i…

…n the staking pallet

frame/nomination-pools/src/lib.rs

@@ -1608,10 +1608,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(

frame/staking/src/pallet/impls.rs

@@ -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,

frame/staking/src/pallet/mod.rs

@@ -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, 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);
 

frame/staking/src/tests.rs

@@ -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);
@@ -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
+		);
 	})
 }