diff --git a/substrate/frame/nomination-pools/test-staking/src/lib.rs b/substrate/frame/nomination-pools/test-staking/src/lib.rs
index 9108da510a3a5339999046e772fecaa30cd11baa..865b7a71e68840d00b42c7cf11e6916c8debf5ad 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::<Runtime>::get(POOL1_BONDED), RewardDestination::Account(POOL1_REWARD));
+ assert_eq!(
+ Payee::<Runtime>::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 f1159c06aa11da252f4c448e47d8a968183ad93d..7bcc68cdfe6f51c001959ba4f2fe2d28c9c7f0ed 100644
--- a/substrate/frame/staking/src/benchmarking.rs
+++ b/substrate/frame/staking/src/benchmarking.rs
@@ -176,7 +176,7 @@ impl<T: Config> ListScenario<T> {
let (origin_stash1, origin_controller1) = create_stash_controller_with_balance::<T>(
USER_SEED + 2,
origin_weight,
- Default::default(),
+ RewardDestination::Staked,
)?;
Staking::<T>::nominate(
RawOrigin::Signed(origin_controller1.clone()).into(),
@@ -187,7 +187,7 @@ impl<T: Config> ListScenario<T> {
let (_origin_stash2, origin_controller2) = create_stash_controller_with_balance::<T>(
USER_SEED + 3,
origin_weight,
- Default::default(),
+ RewardDestination::Staked,
)?;
Staking::<T>::nominate(
RawOrigin::Signed(origin_controller2).into(),
@@ -207,7 +207,7 @@ impl<T: Config> ListScenario<T> {
let (_dest_stash1, dest_controller1) = create_stash_controller_with_balance::<T>(
USER_SEED + 1,
dest_weight,
- Default::default(),
+ RewardDestination::Staked,
)?;
Staking::<T>::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::<T>(0, 100, Default::default())?;
+ let (stash, controller) = create_stash_controller::<T>(0, 100, RewardDestination::Staked)?;
add_slashing_spans::<T>(&stash, s);
let amount = T::Currency::minimum_balance() * 5u32.into(); // Half of total
Staking::<T>::unbond(RawOrigin::Signed(controller.clone()).into(), amount)?;
@@ -340,7 +340,7 @@ benchmarks! {
let (stash, controller) = create_stash_controller::<T>(
MaxNominationsOf::<T>::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::<T>(
MaxNominationsOf::<T>::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::<T>(
MaxNominationsOf::<T>::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::<T>(
SEED + MaxNominationsOf::<T>::get() + 1, // make sure the account does not conflict with others
origin_weight,
- Default::default(),
+ RewardDestination::Staked,
).unwrap();
assert!(!Nominators::<T>::contains_key(&stash));
@@ -466,11 +466,11 @@ benchmarks! {
set_payee {
let (stash, controller) = create_stash_controller::<T>(USER_SEED, 100, RewardDestination::Staked)?;
- assert_eq!(Payee::<T>::get(&stash), RewardDestination::Staked);
+ assert_eq!(Payee::<T>::get(&stash), Some(RewardDestination::Staked));
whitelist_account!(controller);
}: _(RawOrigin::Signed(controller.clone()), RewardDestination::Account(controller.clone()))
verify {
- assert_eq!(Payee::<T>::get(&stash), RewardDestination::Account(controller));
+ assert_eq!(Payee::<T>::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::<T>::get(&stash), RewardDestination::Account(controller));
+ assert_eq!(Payee::<T>::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::<T>(0, 100, Default::default())?;
+ let (stash, controller) = create_stash_controller::<T>(0, 100, RewardDestination::Staked)?;
let mut staking_ledger = Ledger::<T>::get(controller.clone()).unwrap();
let unlock_chunk = UnlockChunk::<BalanceOf<T>> {
value: 1u32.into(),
diff --git a/substrate/frame/staking/src/ledger.rs b/substrate/frame/staking/src/ledger.rs
index 84bb4d167dcb0118ed0f933d044f76d74bc10560..5947adb9028b3a63b9f5b75eff4190c83749895f 100644
--- a/substrate/frame/staking/src/ledger.rs
+++ b/substrate/frame/staking/src/ledger.rs
@@ -126,7 +126,7 @@ impl<T: Config> StakingLedger<T> {
/// default reward destination.
pub(crate) fn reward_destination(
account: StakingAccount<T::AccountId>,
- ) -> RewardDestination<T::AccountId> {
+ ) -> Option<RewardDestination<T::AccountId>> {
let stash = match account {
StakingAccount::Stash(stash) => Some(stash),
StakingAccount::Controller(controller) =>
@@ -137,7 +137,7 @@ impl<T: Config> StakingLedger<T> {
<Payee<T>>::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 41cb2a12c3a32861c1d9f093c5126e58076221ad..09185a690be1657cc1fe749b00aad1b91baac76f 100644
--- a/substrate/frame/staking/src/lib.rs
+++ b/substrate/frame/staking/src/lib.rs
@@ -407,12 +407,6 @@ pub enum RewardDestination<AccountId> {
None,
}
-impl<AccountId> Default for RewardDestination<AccountId> {
- 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 5332dbfdd5b2d0b1203d71a72fc1e0698becd37f..73b6f8ccf27f67e8382a55da2cf58b4b02c0e4a0 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<Test>;
pub(crate) type TestCall = <Test as frame_system::Config>::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::<Test>::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 80b4079dbd64feabcd53c2833ebb2af035f72137..8b45430c688ec69f975b47b4a079b450716939aa 100644
--- a/substrate/frame/staking/src/pallet/impls.rs
+++ b/substrate/frame/staking/src/pallet/impls.rs
@@ -75,7 +75,7 @@ impl<T: Config> Pallet<T> {
StakingLedger::<T>::get(account)
}
- pub fn payee(account: StakingAccount<T::AccountId>) -> RewardDestination<T::AccountId> {
+ pub fn payee(account: StakingAccount<T::AccountId>) -> Option<RewardDestination<T::AccountId>> {
StakingLedger::<T>::reward_destination(account)
}
@@ -336,11 +336,10 @@ impl<T: Config> Pallet<T> {
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<T: Config> Pallet<T> {
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<T: Config> Pallet<T> {
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<T: Config> Pallet<T> {
"VoterList contains non-staker"
);
+ Self::check_payees()?;
Self::check_nominators()?;
Self::check_exposures()?;
Self::check_paged_exposures()?;
@@ -1833,6 +1832,23 @@ impl<T: Config> Pallet<T> {
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::<T>::iter() {
+ ensure!(Payee::<T>::get(&stash).is_some(), "bonded ledger does not have payee set");
+ }
+
+ ensure!(
+ (Ledger::<T>::iter().count() == Payee::<T>::iter().count()) &&
+ (Ledger::<T>::iter().count() == Bonded::<T>::iter().count()),
+ "number of entries in payee storage items does not match the number of bonded ledgers",
+ );
+
+ Ok(())
+ }
+
fn check_count() -> Result<(), TryRuntimeError> {
ensure!(
<T as Config>::VoterList::count() ==
diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs
index b914545a76b989069476f14360cb0314167ede94..0689418d02bd9c4863f5d5db2e2b8c546d8642cf 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<T: Config> =
- StorageMap<_, Twox64Concat, T::AccountId, RewardDestination<T::AccountId>, ValueQuery>;
+ StorageMap<_, Twox64Concat, T::AccountId, RewardDestination<T::AccountId>, OptionQuery>;
/// The map from (wannabe) validator stash key to the preferences of that validator.
///
@@ -1911,7 +1911,7 @@ pub mod pallet {
ensure!(
(Payee::<T>::get(&ledger.stash) == {
#[allow(deprecated)]
- RewardDestination::Controller
+ Some(RewardDestination::Controller)
}),
Error::<T>::NotController
);
@@ -1948,7 +1948,7 @@ pub mod pallet {
// `Controller` variant, skip deprecating this account.
let payee_deprecated = Payee::<T>::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 f469ace0bc41118608d7ffd3c08c5d5b6703bfeb..85ee7dd3bf59d9d96243f22b0463b465b4754fa7 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::<Test>(
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::<Test>::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::<Test>(
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::<Test>::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::<Test>::set_exposure(
0,
@@ -5016,7 +5017,7 @@ mod election_data_provider {
assert_eq!(MinNominatorBond::<Test>::get(), 1);
assert_eq!(<Test as Config>::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!(<Test as Config>::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::<Test>::new(123456, 20);
+ Ledger::<Test>::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::<Test>::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::<Test>::get(StakingAccount::Stash(42)).is_err());
@@ -6819,7 +6842,7 @@ mod ledger {
assert_ok!(ledger.clone().bond(reward_dest));
assert!(StakingLedger::<Test>::is_bonded(StakingAccount::Stash(42)));
assert!(<Bonded<Test>>::get(&42).is_some());
- assert_eq!(<Payee<Test>>::get(&42), reward_dest);
+ assert_eq!(<Payee<Test>>::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::<Test>::ControllerDeprecated
);
- assert_eq!(Payee::<Test>::get(&11), RewardDestination::Staked);
+ assert_eq!(Payee::<Test>::get(&11), Some(RewardDestination::Staked));
})
}
@@ -6867,18 +6890,18 @@ mod ledger {
ExtBuilder::default().build_and_execute(|| {
// migrate a `Controller` variant to `Account` variant.
Payee::<Test>::insert(11, RewardDestination::Controller);
- assert_eq!(Payee::<Test>::get(&11), RewardDestination::Controller);
+ assert_eq!(Payee::<Test>::get(&11), Some(RewardDestination::Controller));
assert_ok!(Staking::update_payee(RuntimeOrigin::signed(11), 11));
- assert_eq!(Payee::<Test>::get(&11), RewardDestination::Account(11));
+ assert_eq!(Payee::<Test>::get(&11), Some(RewardDestination::Account(11)));
// Do not migrate a variant if not `Controller`.
Payee::<Test>::insert(21, RewardDestination::Stash);
- assert_eq!(Payee::<Test>::get(&21), RewardDestination::Stash);
+ assert_eq!(Payee::<Test>::get(&21), Some(RewardDestination::Stash));
assert_noop!(
Staking::update_payee(RuntimeOrigin::signed(11), 21),
Error::<Test>::NotController
);
- assert_eq!(Payee::<Test>::get(&21), RewardDestination::Stash);
+ assert_eq!(Payee::<Test>::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;