diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 1ea05bba3b579..c7e6936ac75d8 100644 --- a/frame/staking/src/benchmarking.rs +++ b/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::(); @@ -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::(0, 100, Default::default())?; let mut staking_ledger = Ledger::::get(controller.clone()).unwrap(); let unlock_chunk = UnlockChunk::> { diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index df568d6b596ba..e2abf537c9758 100644 --- a/frame/staking/src/lib.rs +++ b/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 = <::Currency as Currency< type AccountIdLookupOf = <::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 { /// 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>, MaxUnlockingChunks>, + pub unlocking: BoundedVec>, T::MaxUnlockingChunks>, /// List of eras for which the stakers behind a validator have claimed rewards. Only updated /// for validators. pub claimed_rewards: BoundedVec, diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 385087f9bec41..3a9351ef4a271 100644 --- a/frame/staking/src/mock.rs +++ b/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, BTreeMap>) = (Zero::zero(), BTreeMap::new()); } @@ -301,7 +302,7 @@ impl crate::pallet::pallet::Config for Test { // NOTE: consider a macro and use `UseNominatorsAndValidatorsMap` as well. type VoterList = VoterBagsList; type TargetList = UseValidatorsMap; - type MaxUnlockingChunks = ConstU32<32>; + type MaxUnlockingChunks = MaxUnlockingChunks; type HistoryDepth = HistoryDepth; type OnStakerSlash = OnStakerSlashMock; type BenchmarkingConfig = TestBenchmarkingConfig; diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 4db3870c62d8b..9f6437b6086ef 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/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; @@ -237,8 +238,16 @@ pub mod pallet { /// VALIDATOR. type TargetList: SortedListProvider>; - /// 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; @@ -943,7 +952,7 @@ pub mod pallet { let controller = ensure_signed(origin)?; let mut ledger = Self::ledger(&controller).ok_or(Error::::NotController)?; ensure!( - ledger.unlocking.len() < MaxUnlockingChunks::get() as usize, + ledger.unlocking.len() < T::MaxUnlockingChunks::get() as usize, Error::::NoMoreChunks, ); @@ -1457,7 +1466,7 @@ pub mod pallet { /// - Bounded by `MaxUnlockingChunks`. /// - Storage changes: Can't increase storage, only decrease it. /// # - #[pallet::weight(T::WeightInfo::rebond(MaxUnlockingChunks::get() as u32))] + #[pallet::weight(T::WeightInfo::rebond(T::MaxUnlockingChunks::get() as u32))] pub fn rebond( origin: OriginFor, #[pallet::compact] value: BalanceOf, diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 6798a78030f9e..b9c4fd04a7b30 100644 --- a/frame/staking/src/tests.rs +++ b/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, @@ -1346,7 +1346,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..<::MaxUnlockingChunks as Get>::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); @@ -1361,7 +1362,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 + <::MaxUnlockingChunks as Get>::get() as usize ); // can't do more. assert_noop!(Staking::unbond(RuntimeOrigin::signed(10), 1), Error::::NoMoreChunks); @@ -5483,7 +5484,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(); @@ -5560,3 +5561,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, 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, 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::::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::::NotController); + assert_noop!(Staking::rebond(RuntimeOrigin::signed(4), 100), Error::::NotController); + + // reset the ledger corruption + MaxUnlockingChunks::set(2); + }) +}