Removes the Default implementation for RewardDestination (#2402)

This PR removes current default for `RewardDestination`, which may cause
confusion since a ledger should not have a default reward destination:
either it has a reward destination, or something is wrong. It also
changes the `Payee`'s reward destination in storage from `ValueQuery` to
`OptionQuery`.

In addition, it adds a `try_state` check to make sure each bonded ledger
have a valid reward destination.

Closes #2063

---------

Co-authored-by: command-bot <>
Co-authored-by: Ross Bulat <ross@parity.io>

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

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(),

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
 		}
 	}
 

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 {

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

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,13 +1824,31 @@ impl<T: Config> Pallet<T> {
 			"VoterList contains non-staker"
 		);
 
+		Self::check_payees()?;
 		Self::check_nominators()?;
 		Self::check_exposures()?;
 		Self::check_paged_exposures()?;
 		Self::check_ledgers()?;
 		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() ==

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 {