[do not merge] Decouple rewards from ledger

frame/staking/src/benchmarking.rs

@@ -681,7 +681,7 @@ benchmarks! {
 			active: T::Currency::minimum_balance() - One::one(),
 			total: T::Currency::minimum_balance() - One::one(),
 			unlocking: Default::default(),
-			claimed_rewards: Default::default(),
+			legacy_claimed_rewards: Default::default(),
 		};
 		Ledger::<T>::insert(&controller, l);
 

frame/staking/src/lib.rs

@@ -366,12 +366,21 @@ pub struct ActiveEraInfo {
 
 /// Reward points of an era. Used to split era total payout between validators.
 ///
-/// This points will be used to reward validators and their respective nominators.
+/// These points will be used to reward validators and their respective nominators. As the rewards
+/// for `individual` validators are paid out, they are removed from the map to prevent a validator
+/// to claim reward for the same era twice.
 #[derive(PartialEq, Encode, Decode, RuntimeDebug, TypeInfo)]
 pub struct EraRewardPoints<AccountId: Ord> {
-	/// Total number of points. Equals the sum of reward points for each validator.
+	/// Total number of points in an era.
+	///
+	/// For an active era, `total` is strictly equal to the sum of reward points for each
+	/// validator. For any `era < active_era`, `total >= sum(individual)`. As a validator claims
+	/// their era reward in the succeeding eras, individual points are dropped hence sum of
+	/// individual validator reward points reduces but `total` for an era will always remain same.
+	/// This is important, since we rely on `total` to calculate reward payouts for the remaining
+	/// set of validators correctly.
 	pub total: RewardPoint,
-	/// The reward points earned by a given validator.
+	/// The reward points earned by a given validator. Dropped as reward points get claimed.
 	pub individual: BTreeMap<AccountId, RewardPoint>,
 }
 
@@ -465,9 +474,11 @@ pub struct StakingLedger<T: Config> {
 	/// (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>>, 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>,
+	/// Legacy field. Used as a list of eras for which the stakers behind a validator have claimed
+	/// rewards. It should only be used for reading old state.
+	/// TODO: Clean up after `T::HistoryDepth` eras.
+	/// Tracker: <https://github.com/paritytech/substrate/issues/13034>
+	pub legacy_claimed_rewards: BoundedVec<EraIndex, T::HistoryDepth>,
 }
 
 impl<T: Config> StakingLedger<T> {
@@ -478,7 +489,7 @@ impl<T: Config> StakingLedger<T> {
 			total: Zero::zero(),
 			active: Zero::zero(),
 			unlocking: Default::default(),
-			claimed_rewards: Default::default(),
+			legacy_claimed_rewards: Default::default(),
 		}
 	}
 
@@ -508,7 +519,7 @@ impl<T: Config> StakingLedger<T> {
 			total,
 			active: self.active,
 			unlocking,
-			claimed_rewards: self.claimed_rewards,
+			legacy_claimed_rewards: self.legacy_claimed_rewards,
 		}
 	}
 

frame/staking/src/pallet/impls.rs

@@ -25,8 +25,8 @@ use frame_support::{
 	dispatch::WithPostDispatchInfo,
 	pallet_prelude::*,
 	traits::{
-		Currency, CurrencyToVote, Defensive, DefensiveResult, EstimateNextNewSession, Get,
-		Imbalance, LockableCurrency, OnUnbalanced, TryCollect, UnixTime, WithdrawReasons,
+		Currency, CurrencyToVote, Defensive, EstimateNextNewSession, Get, Imbalance,
+		LockableCurrency, OnUnbalanced, TryCollect, UnixTime, WithdrawReasons,
 	},
 	weights::Weight,
 };
@@ -43,9 +43,9 @@ use sp_staking::{
 use sp_std::prelude::*;
 
 use crate::{
-	log, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, Exposure, ExposureOf,
-	Forcing, IndividualExposure, MaxWinnersOf, Nominations, PositiveImbalanceOf, RewardDestination,
-	SessionInterface, StakingLedger, ValidatorPrefs,
+	log, slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, EraRewardPoints,
+	Exposure, ExposureOf, Forcing, IndividualExposure, MaxWinnersOf, Nominations,
+	PositiveImbalanceOf, RewardDestination, SessionInterface, StakingLedger, ValidatorPrefs,
 };
 
 use super::{pallet::*, STAKING_ID};
@@ -147,8 +147,6 @@ impl<T: Config> Pallet<T> {
 				.with_weight(T::WeightInfo::payout_stakers_alive_staked(0))
 		);
 
-		// Note: if era has no reward to be claimed, era may be future. better not to update
-		// `ledger.claimed_rewards` in this case.
 		let era_payout = <ErasValidatorReward<T>>::get(&era).ok_or_else(|| {
 			Error::<T>::InvalidEraToReward
 				.with_weight(T::WeightInfo::payout_stakers_alive_staked(0))
@@ -160,48 +158,40 @@ impl<T: Config> Pallet<T> {
 		let mut ledger = <Ledger<T>>::get(&controller).ok_or(Error::<T>::NotController)?;
 
 		ledger
-			.claimed_rewards
+			.legacy_claimed_rewards
 			.retain(|&x| x >= current_era.saturating_sub(history_depth));
 
-		match ledger.claimed_rewards.binary_search(&era) {
-			Ok(_) =>
-				return Err(Error::<T>::AlreadyClaimed
-					.with_weight(T::WeightInfo::payout_stakers_alive_staked(0))),
-			Err(pos) => ledger
-				.claimed_rewards
-				.try_insert(pos, era)
-				// Since we retain era entries in `claimed_rewards` only upto
-				// `HistoryDepth`, following bound is always expected to be
-				// satisfied.
-				.defensive_map_err(|_| Error::<T>::BoundNotMet)?,
-		}
-
-		let exposure = <ErasStakersClipped<T>>::get(&era, &ledger.stash);
-
 		// Input data seems good, no errors allowed after this point
-
 		<Ledger<T>>::insert(&controller, &ledger);
 
+		let exposure = <ErasStakersClipped<T>>::get(&era, &ledger.stash);
+
 		// Get Era reward points. It has TOTAL and INDIVIDUAL
 		// Find the fraction of the era reward that belongs to the validator
 		// Take that fraction of the eras rewards to split to nominator and validator
 		//
 		// Then look at the validator, figure out the proportion of their reward
 		// which goes to them and each of their nominators.
 
-		let era_reward_points = <ErasRewardPoints<T>>::get(&era);
+		let era_reward_points = Self::get_era_reward_points(&era, &ledger);
 		let total_reward_points = era_reward_points.total;
 		let validator_reward_points = era_reward_points
 			.individual
 			.get(&ledger.stash)
 			.copied()
 			.unwrap_or_else(Zero::zero);
 
-		// Nothing to do if they have no reward points.
+		// No reward points to claim. This can also happen if reward has already been claimed.
 		if validator_reward_points.is_zero() {
-			return Ok(Some(T::WeightInfo::payout_stakers_alive_staked(0)).into())
+			return Err(Error::<T>::AlreadyClaimed
+				.with_weight(T::WeightInfo::payout_stakers_alive_staked(0)))
 		}
 
+		// drop reward points for the validator so it cannot be claimed again.
+		<ErasRewardPoints<T>>::mutate(&era, |era_rewards| {
+			era_rewards.individual.remove(&ledger.stash);
+		});
+
 		// This is the fraction of the total reward that the validator and the
 		// nominators will get.
 		let validator_total_reward_part =
@@ -273,6 +263,26 @@ impl<T: Config> Pallet<T> {
 		<Ledger<T>>::insert(controller, ledger);
 	}
 
+	fn get_era_reward_points(
+		era: &EraIndex,
+		ledger: &StakingLedger<T>,
+	) -> EraRewardPoints<T::AccountId> {
+		let mut era_reward_points = <ErasRewardPoints<T>>::get(&era);
+
+		// In the older version of code, ledger maintained the record of `legacy_claimed_rewards`.
+		// Going forward, we don't keep them in the ledger but drop them from
+		// `EraRewardPoints` as they are claimed. Since we maintain `$HistoryDepth` eras in the
+		// `ledger.legacy_claimed_rewards`, once `$HistoryDepth` number of eras have passed, this
+		// code is redundant and can be cleaned up, relying solely on `EraRewardPoints` to prevent
+		// double claim.
+		if ledger.legacy_claimed_rewards.binary_search(&era).is_ok() {
+			// since rewards are already claimed for this era, drop them.
+			era_reward_points.individual.remove(&ledger.stash);
+		}
+
+		era_reward_points
+	}
+
 	/// Chill a stash account.
 	pub(crate) fn chill_stash(stash: &T::AccountId) {
 		let chilled_as_validator = Self::do_remove_validator(stash);
@@ -1044,7 +1054,7 @@ impl<T: Config> ElectionDataProvider for Pallet<T> {
 				active: stake,
 				total: stake,
 				unlocking: Default::default(),
-				claimed_rewards: Default::default(),
+				legacy_claimed_rewards: Default::default(),
 			},
 		);
 
@@ -1062,7 +1072,7 @@ impl<T: Config> ElectionDataProvider for Pallet<T> {
 				active: stake,
 				total: stake,
 				unlocking: Default::default(),
-				claimed_rewards: Default::default(),
+				legacy_claimed_rewards: Default::default(),
 			},
 		);
 		Self::do_add_validator(
@@ -1103,7 +1113,7 @@ impl<T: Config> ElectionDataProvider for Pallet<T> {
 					active: stake,
 					total: stake,
 					unlocking: Default::default(),
-					claimed_rewards: Default::default(),
+					legacy_claimed_rewards: Default::default(),
 				},
 			);
 			Self::do_add_validator(
@@ -1124,7 +1134,7 @@ impl<T: Config> ElectionDataProvider for Pallet<T> {
 					active: stake,
 					total: stake,
 					unlocking: Default::default(),
-					claimed_rewards: Default::default(),
+					legacy_claimed_rewards: Default::default(),
 				},
 			);
 			Self::do_add_nominator(

frame/staking/src/pallet/mod.rs

@@ -24,9 +24,8 @@ use frame_support::{
 	dispatch::Codec,
 	pallet_prelude::*,
 	traits::{
-		Currency, CurrencyToVote, Defensive, DefensiveResult, DefensiveSaturating, EnsureOrigin,
-		EstimateNextNewSession, Get, LockIdentifier, LockableCurrency, OnUnbalanced, TryCollect,
-		UnixTime,
+		Currency, CurrencyToVote, Defensive, DefensiveSaturating, EnsureOrigin,
+		EstimateNextNewSession, Get, LockIdentifier, LockableCurrency, OnUnbalanced, UnixTime,
 	},
 	weights::Weight,
 	BoundedVec,
@@ -136,7 +135,7 @@ pub mod pallet {
 		/// HistoryDepth, current_era]`: `ErasStakers`, `ErasStakersClipped`,
 		/// `ErasValidatorPrefs`, `ErasValidatorReward`, `ErasRewardPoints`,
 		/// `ErasTotalStake`, `ErasStartSessionIndex`,
-		/// `StakingLedger.claimed_rewards`.
+		/// `StakingLedger.legacy_claimed_rewards`.
 		///
 		/// Must be more than the number of eras delayed by session.
 		/// I.e. active era must always be in history. I.e. `active_era >
@@ -146,7 +145,7 @@ pub mod pallet {
 		/// this should be set to same value or greater as in storage.
 		///
 		/// Note: `HistoryDepth` is used as the upper bound for the `BoundedVec`
-		/// item `StakingLedger.claimed_rewards`. Setting this value lower than
+		/// item `StakingLedger.legacy_claimed_rewards`. Setting this value lower than
 		/// 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.
@@ -744,7 +743,7 @@ pub mod pallet {
 		InvalidNumberOfNominations,
 		/// Items are not sorted and unique.
 		NotSortedAndUnique,
-		/// Rewards for this era have already been claimed for this validator.
+		/// No rewards to claim. Either no reward is earned or is already claimed.
 		AlreadyClaimed,
 		/// Incorrect previous history depth input provided.
 		IncorrectHistoryDepth,
@@ -875,10 +874,6 @@ pub mod pallet {
 			<Bonded<T>>::insert(&stash, &controller);
 			<Payee<T>>::insert(&stash, payee);
 
-			let current_era = CurrentEra::<T>::get().unwrap_or(0);
-			let history_depth = T::HistoryDepth::get();
-			let last_reward_era = current_era.saturating_sub(history_depth);
-
 			let stash_balance = T::Currency::free_balance(&stash);
 			let value = value.min(stash_balance);
 			Self::deposit_event(Event::<T>::Bonded { stash: stash.clone(), amount: value });
@@ -887,12 +882,7 @@ pub mod pallet {
 				total: value,
 				active: value,
 				unlocking: Default::default(),
-				claimed_rewards: (last_reward_era..current_era)
-					.try_collect()
-					// Since last_reward_era is calculated as `current_era -
-					// HistoryDepth`, following bound is always expected to be
-					// satisfied.
-					.defensive_map_err(|_| Error::<T>::BoundNotMet)?,
+				legacy_claimed_rewards: Default::default(),
 			};
 			Self::update_ledger(&controller, &item);
 			Ok(())