diff --git a/substrate/frame/nomination-pools/test-staking/src/lib.rs b/substrate/frame/nomination-pools/test-staking/src/lib.rs index 9108da510a3a..865b7a71e688 100644 --- a/substrate/frame/nomination-pools/test-staking/src/lib.rs +++ b/substrate/frame/nomination-pools/test-staking/src/lib.rs @@ -214,7 +214,10 @@ fn pool_slash_e2e() { ] ); - assert_eq!(Payee::::get(POOL1_BONDED), RewardDestination::Account(POOL1_REWARD)); + assert_eq!( + Payee::::get(POOL1_BONDED), + Some(RewardDestination::Account(POOL1_REWARD)) + ); // have two members join assert_ok!(Pools::join(RuntimeOrigin::signed(20), 20, 1)); diff --git a/substrate/frame/staking/src/benchmarking.rs b/substrate/frame/staking/src/benchmarking.rs index f1159c06aa11..7bcc68cdfe6f 100644 --- a/substrate/frame/staking/src/benchmarking.rs +++ b/substrate/frame/staking/src/benchmarking.rs @@ -176,7 +176,7 @@ impl ListScenario { let (origin_stash1, origin_controller1) = create_stash_controller_with_balance::( USER_SEED + 2, origin_weight, - Default::default(), + RewardDestination::Staked, )?; Staking::::nominate( RawOrigin::Signed(origin_controller1.clone()).into(), @@ -187,7 +187,7 @@ impl ListScenario { let (_origin_stash2, origin_controller2) = create_stash_controller_with_balance::( USER_SEED + 3, origin_weight, - Default::default(), + RewardDestination::Staked, )?; Staking::::nominate( RawOrigin::Signed(origin_controller2).into(), @@ -207,7 +207,7 @@ impl ListScenario { let (_dest_stash1, dest_controller1) = create_stash_controller_with_balance::( USER_SEED + 1, dest_weight, - Default::default(), + RewardDestination::Staked, )?; Staking::::nominate( RawOrigin::Signed(dest_controller1).into(), @@ -291,7 +291,7 @@ benchmarks! { withdraw_unbonded_update { // Slashing Spans let s in 0 .. MAX_SPANS; - let (stash, controller) = create_stash_controller::(0, 100, Default::default())?; + let (stash, controller) = create_stash_controller::(0, 100, RewardDestination::Staked)?; add_slashing_spans::(&stash, s); let amount = T::Currency::minimum_balance() * 5u32.into(); // Half of total Staking::::unbond(RawOrigin::Signed(controller.clone()).into(), amount)?; @@ -340,7 +340,7 @@ benchmarks! { let (stash, controller) = create_stash_controller::( MaxNominationsOf::::get() - 1, 100, - Default::default(), + RewardDestination::Staked, )?; // because it is chilled. assert!(!T::VoterList::contains(&stash)); @@ -368,7 +368,7 @@ benchmarks! { let (stash, controller) = create_stash_controller::( MaxNominationsOf::::get() - 1, 100, - Default::default(), + RewardDestination::Staked, )?; let stash_lookup = T::Lookup::unlookup(stash.clone()); @@ -383,7 +383,7 @@ benchmarks! { let (n_stash, n_controller) = create_stash_controller::( MaxNominationsOf::::get() + i, 100, - Default::default(), + RewardDestination::Staked, )?; // bake the nominations; we first clone them from the rest of the validators. @@ -431,7 +431,7 @@ benchmarks! { let (stash, controller) = create_stash_controller_with_balance::( SEED + MaxNominationsOf::::get() + 1, // make sure the account does not conflict with others origin_weight, - Default::default(), + RewardDestination::Staked, ).unwrap(); assert!(!Nominators::::contains_key(&stash)); @@ -466,11 +466,11 @@ benchmarks! { set_payee { let (stash, controller) = create_stash_controller::(USER_SEED, 100, RewardDestination::Staked)?; - assert_eq!(Payee::::get(&stash), RewardDestination::Staked); + assert_eq!(Payee::::get(&stash), Some(RewardDestination::Staked)); whitelist_account!(controller); }: _(RawOrigin::Signed(controller.clone()), RewardDestination::Account(controller.clone())) verify { - assert_eq!(Payee::::get(&stash), RewardDestination::Account(controller)); + assert_eq!(Payee::::get(&stash), Some(RewardDestination::Account(controller))); } update_payee { @@ -482,7 +482,7 @@ benchmarks! { whitelist_account!(controller); }: _(RawOrigin::Signed(controller.clone()), controller.clone()) verify { - assert_eq!(Payee::::get(&stash), RewardDestination::Account(controller)); + assert_eq!(Payee::::get(&stash), Some(RewardDestination::Account(controller))); } set_controller { @@ -784,7 +784,7 @@ benchmarks! { #[extra] do_slash { let l in 1 .. T::MaxUnlockingChunks::get() as u32; - let (stash, controller) = create_stash_controller::(0, 100, Default::default())?; + let (stash, controller) = create_stash_controller::(0, 100, RewardDestination::Staked)?; let mut staking_ledger = Ledger::::get(controller.clone()).unwrap(); let unlock_chunk = UnlockChunk::> { value: 1u32.into(), diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs index 84bb4d167dcb..5947adb9028b 100644 --- a/substrate/frame/staking/src/ledger.rs +++ b/substrate/frame/staking/src/ledger.rs @@ -126,7 +126,7 @@ impl StakingLedger { /// default reward destination. pub(crate) fn reward_destination( account: StakingAccount, - ) -> RewardDestination { + ) -> Option> { let stash = match account { StakingAccount::Stash(stash) => Some(stash), StakingAccount::Controller(controller) => @@ -137,7 +137,7 @@ impl StakingLedger { >::get(stash) } else { defensive!("fetched reward destination from unbonded stash {}", stash); - RewardDestination::default() + None } } diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 41cb2a12c3a3..09185a690be1 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -407,12 +407,6 @@ pub enum RewardDestination { None, } -impl Default for RewardDestination { - fn default() -> Self { - RewardDestination::Staked - } -} - /// Preference of what happens regarding validation. #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, TypeInfo, Default, MaxEncodedLen)] pub struct ValidatorPrefs { diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 5332dbfdd5b2..73b6f8ccf27f 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -345,6 +345,11 @@ where pub(crate) type StakingCall = crate::Call; pub(crate) type TestCall = ::RuntimeCall; +parameter_types! { + // if true, skips the try-state for the test running. + pub static SkipTryStateCheck: bool = false; +} + pub struct ExtBuilder { nominate: bool, validator_count: u32, @@ -454,6 +459,10 @@ impl ExtBuilder { self.balance_factor = factor; self } + pub fn try_state(self, enable: bool) -> Self { + SkipTryStateCheck::set(!enable); + self + } fn build(self) -> sp_io::TestExternalities { sp_tracing::try_init_simple(); let mut storage = frame_system::GenesisConfig::::default().build_storage().unwrap(); @@ -582,7 +591,9 @@ impl ExtBuilder { let mut ext = self.build(); ext.execute_with(test); ext.execute_with(|| { - Staking::do_try_state(System::block_number()).unwrap(); + if !SkipTryStateCheck::get() { + Staking::do_try_state(System::block_number()).unwrap(); + } }); } } diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 80b4079dbd64..8b45430c688e 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -75,7 +75,7 @@ impl Pallet { StakingLedger::::get(account) } - pub fn payee(account: StakingAccount) -> RewardDestination { + pub fn payee(account: StakingAccount) -> Option> { StakingLedger::::reward_destination(account) } @@ -336,11 +336,10 @@ impl Pallet { if amount.is_zero() { return None } + let dest = Self::payee(StakingAccount::Stash(stash.clone()))?; - let dest = Self::payee(StakingAccount::Stash(stash.clone())); let maybe_imbalance = match dest { - RewardDestination::Stash => - T::Currency::deposit_into_existing(stash, amount).ok(), + RewardDestination::Stash => T::Currency::deposit_into_existing(stash, amount).ok(), RewardDestination::Staked => Self::ledger(Stash(stash.clone())) .and_then(|mut ledger| { ledger.active += amount; @@ -354,7 +353,7 @@ impl Pallet { Ok(r) }) .unwrap_or_default(), - RewardDestination::Account(dest_account) => + RewardDestination::Account(ref dest_account) => Some(T::Currency::deposit_creating(&dest_account, amount)), RewardDestination::None => None, #[allow(deprecated)] @@ -366,8 +365,7 @@ impl Pallet { T::Currency::deposit_creating(&controller, amount) }), }; - maybe_imbalance - .map(|imbalance| (imbalance, Self::payee(StakingAccount::Stash(stash.clone())))) + maybe_imbalance.map(|imbalance| (imbalance, dest)) } /// Plan a new session potentially trigger a new era. @@ -1826,6 +1824,7 @@ impl Pallet { "VoterList contains non-staker" ); + Self::check_payees()?; Self::check_nominators()?; Self::check_exposures()?; Self::check_paged_exposures()?; @@ -1833,6 +1832,23 @@ impl Pallet { Self::check_count() } + /// Invariants: + /// * A bonded ledger should always have an assigned `Payee`. + /// * The number of entries in `Payee` and of bonded staking ledgers *must* match. + fn check_payees() -> Result<(), TryRuntimeError> { + for (stash, _) in Bonded::::iter() { + ensure!(Payee::::get(&stash).is_some(), "bonded ledger does not have payee set"); + } + + ensure!( + (Ledger::::iter().count() == Payee::::iter().count()) && + (Ledger::::iter().count() == Bonded::::iter().count()), + "number of entries in payee storage items does not match the number of bonded ledgers", + ); + + Ok(()) + } + fn check_count() -> Result<(), TryRuntimeError> { ensure!( ::VoterList::count() == diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index b914545a76b9..0689418d02bd 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -339,7 +339,7 @@ pub mod pallet { /// TWOX-NOTE: SAFE since `AccountId` is a secure hash. #[pallet::storage] pub type Payee = - StorageMap<_, Twox64Concat, T::AccountId, RewardDestination, ValueQuery>; + StorageMap<_, Twox64Concat, T::AccountId, RewardDestination, OptionQuery>; /// The map from (wannabe) validator stash key to the preferences of that validator. /// @@ -1911,7 +1911,7 @@ pub mod pallet { ensure!( (Payee::::get(&ledger.stash) == { #[allow(deprecated)] - RewardDestination::Controller + Some(RewardDestination::Controller) }), Error::::NotController ); @@ -1948,7 +1948,7 @@ pub mod pallet { // `Controller` variant, skip deprecating this account. let payee_deprecated = Payee::::get(&ledger.stash) == { #[allow(deprecated)] - RewardDestination::Controller + Some(RewardDestination::Controller) }; if ledger.stash != *controller && !payee_deprecated { diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index f469ace0bc41..85ee7dd3bf59 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -771,12 +771,12 @@ fn double_staking_should_fail() { // * an account already bonded as stash cannot be be stashed again. // * an account already bonded as stash cannot nominate. // * an account already bonded as controller can nominate. - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { let arbitrary_value = 5; let (stash, controller) = testing_utils::create_unique_stash_controller::( 0, arbitrary_value, - RewardDestination::default(), + RewardDestination::Staked, false, ) .unwrap(); @@ -786,7 +786,7 @@ fn double_staking_should_fail() { Staking::bond( RuntimeOrigin::signed(stash), arbitrary_value.into(), - RewardDestination::default() + RewardDestination::Staked, ), Error::::AlreadyBonded, ); @@ -805,12 +805,12 @@ fn double_controlling_attempt_should_fail() { // should test (in the same order): // * an account already bonded as controller CANNOT be reused as the controller of another // account. - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { let arbitrary_value = 5; let (stash, _) = testing_utils::create_unique_stash_controller::( 0, arbitrary_value, - RewardDestination::default(), + RewardDestination::Staked, false, ) .unwrap(); @@ -820,7 +820,7 @@ fn double_controlling_attempt_should_fail() { Staking::bond( RuntimeOrigin::signed(stash), arbitrary_value.into(), - RewardDestination::default() + RewardDestination::Staked, ), Error::::AlreadyBonded, ); @@ -1068,8 +1068,8 @@ fn reward_destination_works() { mock::start_active_era(1); mock::make_all_reward_payment(0); - // Check that RewardDestination is Staked (default) - assert_eq!(Staking::payee(11.into()), RewardDestination::Staked); + // Check that RewardDestination is Staked + assert_eq!(Staking::payee(11.into()), Some(RewardDestination::Staked)); // Check that reward went to the stash account of validator assert_eq!(Balances::free_balance(11), 1000 + total_payout_0); // Check that amount at stake increased accordingly @@ -1098,7 +1098,7 @@ fn reward_destination_works() { mock::make_all_reward_payment(1); // Check that RewardDestination is Stash - assert_eq!(Staking::payee(11.into()), RewardDestination::Stash); + assert_eq!(Staking::payee(11.into()), Some(RewardDestination::Stash)); // Check that reward went to the stash account assert_eq!(Balances::free_balance(11), 1000 + total_payout_0 + total_payout_1); // Record this value @@ -1132,7 +1132,7 @@ fn reward_destination_works() { mock::make_all_reward_payment(2); // Check that RewardDestination is Account(11) - assert_eq!(Staking::payee(11.into()), RewardDestination::Account(11)); + assert_eq!(Staking::payee(11.into()), Some(RewardDestination::Account(11))); // Check that reward went to the controller account assert_eq!(Balances::free_balance(11), recorded_stash_balance + total_payout_2); // Check that amount at stake is NOT increased @@ -1746,6 +1746,7 @@ fn reward_to_stake_works() { .set_status(31, StakerStatus::Idle) .set_status(41, StakerStatus::Idle) .set_stake(21, 2000) + .try_state(false) .build_and_execute(|| { assert_eq!(Staking::validator_count(), 2); // Confirm account 10 and 20 are validators @@ -2200,7 +2201,7 @@ fn reward_validator_slashing_validator_does_not_overflow() { let _ = Balances::make_free_balance_be(&2, stake); // only slashes out of bonded stake are applied. without this line, it is 0. - Staking::bond(RuntimeOrigin::signed(2), stake - 1, RewardDestination::default()).unwrap(); + Staking::bond(RuntimeOrigin::signed(2), stake - 1, RewardDestination::Staked).unwrap(); // Override exposure of 11 EraInfo::::set_exposure( 0, @@ -5016,7 +5017,7 @@ mod election_data_provider { assert_eq!(MinNominatorBond::::get(), 1); assert_eq!(::VoterList::count(), 4); - assert_ok!(Staking::bond(RuntimeOrigin::signed(4), 5, Default::default(),)); + assert_ok!(Staking::bond(RuntimeOrigin::signed(4), 5, RewardDestination::Staked,)); assert_ok!(Staking::nominate(RuntimeOrigin::signed(4), vec![1])); assert_eq!(::VoterList::count(), 5); @@ -5415,6 +5416,28 @@ fn count_check_works() { }) } +#[test] +#[should_panic = "called `Result::unwrap()` on an `Err` value: Other(\"number of entries in payee storage items does not match the number of bonded ledgers\")"] +fn check_payee_invariant1_works() { + // A bonded ledger should always have an assigned `Payee` This test should panic as we verify + // that a bad state will panic due to the `try_state` checks in the `post_checks` in `mock`. + ExtBuilder::default().build_and_execute(|| { + let rogue_ledger = StakingLedger::::new(123456, 20); + Ledger::::insert(123456, rogue_ledger); + }) +} + +#[test] +#[should_panic = "called `Result::unwrap()` on an `Err` value: Other(\"number of entries in payee storage items does not match the number of bonded ledgers\")"] +fn check_payee_invariant2_works() { + // The number of entries in both `Payee` and of bonded staking ledgers should match. This test + // should panic as we verify that a bad state will panic due to the `try_state` checks in the + // `post_checks` in `mock`. + ExtBuilder::default().build_and_execute(|| { + Payee::::insert(1111, RewardDestination::Staked); + }) +} + #[test] fn min_bond_checks_work() { ExtBuilder::default() @@ -6734,7 +6757,7 @@ mod ledger { #[test] fn paired_account_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { assert_ok!(Staking::bond( RuntimeOrigin::signed(10), 100, @@ -6769,7 +6792,7 @@ mod ledger { #[test] fn get_ledger_works() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { // stash does not exist assert!(StakingLedger::::get(StakingAccount::Stash(42)).is_err()); @@ -6819,7 +6842,7 @@ mod ledger { assert_ok!(ledger.clone().bond(reward_dest)); assert!(StakingLedger::::is_bonded(StakingAccount::Stash(42))); assert!(>::get(&42).is_some()); - assert_eq!(>::get(&42), reward_dest); + assert_eq!(>::get(&42), Some(reward_dest)); // cannot bond again. assert!(ledger.clone().bond(reward_dest).is_err()); @@ -6857,7 +6880,7 @@ mod ledger { Staking::set_payee(RuntimeOrigin::signed(11), RewardDestination::Controller), Error::::ControllerDeprecated ); - assert_eq!(Payee::::get(&11), RewardDestination::Staked); + assert_eq!(Payee::::get(&11), Some(RewardDestination::Staked)); }) } @@ -6867,18 +6890,18 @@ mod ledger { ExtBuilder::default().build_and_execute(|| { // migrate a `Controller` variant to `Account` variant. Payee::::insert(11, RewardDestination::Controller); - assert_eq!(Payee::::get(&11), RewardDestination::Controller); + assert_eq!(Payee::::get(&11), Some(RewardDestination::Controller)); assert_ok!(Staking::update_payee(RuntimeOrigin::signed(11), 11)); - assert_eq!(Payee::::get(&11), RewardDestination::Account(11)); + assert_eq!(Payee::::get(&11), Some(RewardDestination::Account(11))); // Do not migrate a variant if not `Controller`. Payee::::insert(21, RewardDestination::Stash); - assert_eq!(Payee::::get(&21), RewardDestination::Stash); + assert_eq!(Payee::::get(&21), Some(RewardDestination::Stash)); assert_noop!( Staking::update_payee(RuntimeOrigin::signed(11), 21), Error::::NotController ); - assert_eq!(Payee::::get(&21), RewardDestination::Stash); + assert_eq!(Payee::::get(&21), Some(RewardDestination::Stash)); }) } @@ -7016,7 +7039,7 @@ mod ledger { #[test] fn deprecate_controller_batch_skips_unmigrated_controller_payees() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().try_state(false).build_and_execute(|| { // Given: let stash: u64 = 1000;