[fix] Bound staking ledger correctly with MaxUnlockingChunks from con…

…figuration (#12343)

* used maxunlockingchunks from config

* mhl MaxUnlockingChunks

* no migration needed

* changes as per requested

* fmt

* fix tests

* fix benchmark

* warning in the doc for abrupt changes in the config

* less unnecessary details in the test

* fix tests

Co-authored-by: mrisholukamba <[email protected]>
Co-authored-by: parity-processbot <>

frame/staking/src/benchmarking.rs

@@ -613,7 +613,7 @@ benchmarks! {
 	}
 
 	rebond {
-		let l in 1 .. MaxUnlockingChunks::get() as u32;
+		let l in 1 .. T::MaxUnlockingChunks::get() as u32;
 
 		// clean up any existing state.
 		clear_validators_and_nominators::<T>();
@@ -764,7 +764,7 @@ benchmarks! {
 
 	#[extra]
 	do_slash {
-		let l in 1 .. MaxUnlockingChunks::get() as u32;
+		let l in 1 .. T::MaxUnlockingChunks::get() as u32;
 		let (stash, controller) = create_stash_controller::<T>(0, 100, Default::default())?;
 		let mut staking_ledger = Ledger::<T>::get(controller.clone()).unwrap();
 		let unlock_chunk = UnlockChunk::<BalanceOf<T>> {

frame/staking/src/lib.rs

@@ -301,7 +301,6 @@ mod pallet;
 
 use codec::{Decode, Encode, HasCompact, MaxEncodedLen};
 use frame_support::{
-	parameter_types,
 	traits::{Currency, Defensive, Get},
 	weights::Weight,
 	BoundedVec, CloneNoBound, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound,
@@ -349,10 +348,6 @@ type NegativeImbalanceOf<T> = <<T as Config>::Currency as Currency<
 
 type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup>::Source;
 
-parameter_types! {
-	pub MaxUnlockingChunks: u32 = 32;
-}
-
 /// Information regarding the active era (era in used in session).
 #[derive(Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)]
 pub struct ActiveEraInfo {
@@ -465,7 +460,7 @@ pub struct StakingLedger<T: Config> {
 	/// Any balance that is becoming free, which may eventually be transferred out of the stash
 	/// (assuming it doesn't get slashed first). It is assumed that this will be treated as a first
 	/// in, first out queue where the new (higher value) eras get pushed on the back.
-	pub unlocking: BoundedVec<UnlockChunk<BalanceOf<T>>, MaxUnlockingChunks>,
+	pub unlocking: BoundedVec<UnlockChunk<BalanceOf<T>>, T::MaxUnlockingChunks>,
 	/// List of eras for which the stakers behind a validator have claimed rewards. Only updated
 	/// for validators.
 	pub claimed_rewards: BoundedVec<EraIndex, T::HistoryDepth>,

frame/staking/src/mock.rs

@@ -237,6 +237,7 @@ parameter_types! {
 	pub static BagThresholds: &'static [sp_npos_elections::VoteWeight] = &THRESHOLDS;
 	pub static MaxNominations: u32 = 16;
 	pub static HistoryDepth: u32 = 80;
+	pub static MaxUnlockingChunks: u32 = 32;
 	pub static RewardOnUnbalanceWasCalled: bool = false;
 	pub static LedgerSlashPerEra: (BalanceOf<Test>, BTreeMap<EraIndex, BalanceOf<Test>>) = (Zero::zero(), BTreeMap::new());
 }
@@ -301,7 +302,7 @@ impl crate::pallet::pallet::Config for Test {
 	// NOTE: consider a macro and use `UseNominatorsAndValidatorsMap<Self>` as well.
 	type VoterList = VoterBagsList;
 	type TargetList = UseValidatorsMap<Self>;
-	type MaxUnlockingChunks = ConstU32<32>;
+	type MaxUnlockingChunks = MaxUnlockingChunks;
 	type HistoryDepth = HistoryDepth;
 	type OnStakerSlash = OnStakerSlashMock<Test>;
 	type BenchmarkingConfig = TestBenchmarkingConfig;

frame/staking/src/pallet/mod.rs

@@ -43,9 +43,9 @@ pub use impls::*;
 
 use crate::{
 	slashing, weights::WeightInfo, AccountIdLookupOf, ActiveEraInfo, BalanceOf, EraPayout,
-	EraRewardPoints, Exposure, Forcing, MaxUnlockingChunks, NegativeImbalanceOf, Nominations,
-	PositiveImbalanceOf, Releases, RewardDestination, SessionInterface, StakingLedger,
-	UnappliedSlash, UnlockChunk, ValidatorPrefs,
+	EraRewardPoints, Exposure, Forcing, NegativeImbalanceOf, Nominations, PositiveImbalanceOf,
+	Releases, RewardDestination, SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk,
+	ValidatorPrefs,
 };
 
 const STAKING_ID: LockIdentifier = *b"staking ";
@@ -142,8 +142,9 @@ pub mod pallet {
 		///
 		/// Note: `HistoryDepth` is used as the upper bound for the `BoundedVec`
 		/// item `StakingLedger.claimed_rewards`. Setting this value lower than
-		/// the existing value can lead to inconsistencies and will need to be
-		/// handled properly in a migration.
+		/// the existing value can lead to inconsistencies in the
+		/// `StakingLedger` and will need to be handled properly in a migration.
+		/// The test `reducing_history_depth_abrupt` shows this effect.
 		#[pallet::constant]
 		type HistoryDepth: Get<u32>;
 
@@ -237,8 +238,16 @@ pub mod pallet {
 		/// VALIDATOR.
 		type TargetList: SortedListProvider<Self::AccountId, Score = BalanceOf<Self>>;
 
-		/// The maximum number of `unlocking` chunks a [`StakingLedger`] can have. Effectively
-		/// determines how many unique eras a staker may be unbonding in.
+		/// The maximum number of `unlocking` chunks a [`StakingLedger`] can
+		/// have. Effectively determines how many unique eras a staker may be
+		/// unbonding in.
+		///
+		/// Note: `MaxUnlockingChunks` is used as the upper bound for the
+		/// `BoundedVec` item `StakingLedger.unlocking`. Setting this value
+		/// lower than the existing value can lead to inconsistencies in the
+		/// `StakingLedger` and will need to be handled properly in a runtime
+		/// migration. The test `reducing_max_unlocking_chunks_abrupt` shows
+		/// this effect.
 		#[pallet::constant]
 		type MaxUnlockingChunks: Get<u32>;
 
@@ -940,7 +949,7 @@ pub mod pallet {
 			let controller = ensure_signed(origin)?;
 			let mut ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
 			ensure!(
-				ledger.unlocking.len() < MaxUnlockingChunks::get() as usize,
+				ledger.unlocking.len() < T::MaxUnlockingChunks::get() as usize,
 				Error::<T>::NoMoreChunks,
 			);
 
@@ -1454,7 +1463,7 @@ pub mod pallet {
 		/// - Bounded by `MaxUnlockingChunks`.
 		/// - Storage changes: Can't increase storage, only decrease it.
 		/// # </weight>
-		#[pallet::weight(T::WeightInfo::rebond(MaxUnlockingChunks::get() as u32))]
+		#[pallet::weight(T::WeightInfo::rebond(T::MaxUnlockingChunks::get() as u32))]
 		pub fn rebond(
 			origin: OriginFor<T>,
 			#[pallet::compact] value: BalanceOf<T>,

frame/staking/src/tests.rs

@@ -17,7 +17,7 @@
 
 //! Tests for the module.
 
-use super::{ConfigOp, Event, MaxUnlockingChunks, *};
+use super::{ConfigOp, Event, *};
 use frame_election_provider_support::{ElectionProvider, SortedListProvider, Support};
 use frame_support::{
 	assert_noop, assert_ok, assert_storage_noop, bounded_vec,
@@ -1354,7 +1354,8 @@ fn too_many_unbond_calls_should_not_work() {
 	ExtBuilder::default().build_and_execute(|| {
 		let mut current_era = 0;
 		// locked at era MaxUnlockingChunks - 1 until 3
-		for i in 0..MaxUnlockingChunks::get() - 1 {
+
+		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);
@@ -1369,7 +1370,7 @@ fn too_many_unbond_calls_should_not_work() {
 		assert_ok!(Staking::unbond(RuntimeOrigin::signed(10), 1));
 		assert_eq!(
 			Staking::ledger(&10).unwrap().unlocking.len(),
-			MaxUnlockingChunks::get() as usize
+			<<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);
@@ -5494,7 +5495,7 @@ fn pre_bonding_era_cannot_be_claimed() {
 }
 
 #[test]
-fn reducing_history_depth_without_migration() {
+fn reducing_history_depth_abrupt() {
 	// Verifies initial conditions of mock
 	ExtBuilder::default().nominate(false).build_and_execute(|| {
 		let original_history_depth = HistoryDepth::get();
@@ -5571,3 +5572,55 @@ fn reducing_history_depth_without_migration() {
 		HistoryDepth::set(original_history_depth);
 	});
 }
+
+#[test]
+fn reducing_max_unlocking_chunks_abrupt() {
+	// Concern is on validators only
+	// By Default 11, 10 are stash and ctrl and 21,20
+	ExtBuilder::default().build_and_execute(|| {
+		// given a staker at era=10 and MaxUnlockChunks set to 2
+		MaxUnlockingChunks::set(2);
+		start_active_era(10);
+		assert_ok!(Staking::bond(RuntimeOrigin::signed(3), 4, 300, RewardDestination::Staked));
+		assert!(matches!(Staking::ledger(4), Some(_)));
+
+		// when staker unbonds
+		assert_ok!(Staking::unbond(RuntimeOrigin::signed(4), 20));
+
+		// then an unlocking chunk is added at `current_era + bonding_duration`
+		// => 10 + 3 = 13
+		let expected_unlocking: BoundedVec<UnlockChunk<Balance>, MaxUnlockingChunks> =
+			bounded_vec![UnlockChunk { value: 20 as Balance, era: 13 as EraIndex }];
+		assert!(matches!(Staking::ledger(4),
+			Some(StakingLedger {
+				unlocking,
+				..
+			}) if unlocking==expected_unlocking));
+
+		// when staker unbonds at next era
+		start_active_era(11);
+		assert_ok!(Staking::unbond(RuntimeOrigin::signed(4), 50));
+		// then another unlock chunk is added
+		let expected_unlocking: BoundedVec<UnlockChunk<Balance>, MaxUnlockingChunks> =
+			bounded_vec![UnlockChunk { value: 20, era: 13 }, UnlockChunk { value: 50, era: 14 }];
+		assert!(matches!(Staking::ledger(4),
+			Some(StakingLedger {
+				unlocking,
+				..
+			}) if unlocking==expected_unlocking));
+
+		// when staker unbonds further
+		start_active_era(12);
+		// then further unbonding not possible
+		assert_noop!(Staking::unbond(RuntimeOrigin::signed(4), 20), Error::<Test>::NoMoreChunks);
+
+		// when max unlocking chunks is reduced abruptly to a low value
+		MaxUnlockingChunks::set(1);
+		// then unbond, rebond ops are blocked with ledger in corrupt state
+		assert_noop!(Staking::unbond(RuntimeOrigin::signed(4), 20), Error::<Test>::NotController);
+		assert_noop!(Staking::rebond(RuntimeOrigin::signed(4), 100), Error::<Test>::NotController);
+
+		// reset the ledger corruption
+		MaxUnlockingChunks::set(2);
+	})
+}