[pallet-staking] Refund unused weight for payout_stakers (paritytec…

…h#8458)

* [pallet-staking] Refund unused weight for `payout_stakers` 

fixes paritytech#8428

* Use periods in comments

* cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_staking --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/staking/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* Address Shawn's Feedback

* Assert monotomic weights && improve test note

* Remove stray new line

* debug_assert payout_count <= max

* Only track payouts to nominators; not validators

* Trivial comment update

Co-authored-by: Parity Benchmarking Bot <admin@parity.io>

frame/staking/src/lib.rs

@@ -290,7 +290,7 @@ use codec::{HasCompact, Encode, Decode};
 use frame_support::{
 	decl_module, decl_event, decl_storage, ensure, decl_error,
 	weights::{
-		Weight,
+		Weight, WithPostDispatchInfo,
 		constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS},
 	},
 	storage::IterableStorageMap,
@@ -1803,7 +1803,7 @@ decl_module! {
 		///   Paying even a dead controller is cheaper weight-wise. We don't do any refunds here.
 		/// # </weight>
 		#[weight = T::WeightInfo::payout_stakers_alive_staked(T::MaxNominatorRewardedPerValidator::get())]
-		fn payout_stakers(origin, validator_stash: T::AccountId, era: EraIndex) -> DispatchResult {
+		fn payout_stakers(origin, validator_stash: T::AccountId, era: EraIndex) -> DispatchResultWithPostInfo {
 			ensure_signed(origin)?;
 			Self::do_payout_stakers(validator_stash, era)
 		}
@@ -1967,24 +1967,35 @@ impl<T: Config> Module<T> {
 		})
 	}
 
-	fn do_payout_stakers(validator_stash: T::AccountId, era: EraIndex) -> DispatchResult {
+	fn do_payout_stakers(validator_stash: T::AccountId, era: EraIndex) -> DispatchResultWithPostInfo {
 		// Validate input data
-		let current_era = CurrentEra::get().ok_or(Error::<T>::InvalidEraToReward)?;
-		ensure!(era <= current_era, Error::<T>::InvalidEraToReward);
+		let current_era = CurrentEra::get().ok_or(
+			Error::<T>::InvalidEraToReward.with_weight(T::WeightInfo::payout_stakers_alive_staked(0))
+		)?;
 		let history_depth = Self::history_depth();
-		ensure!(era >= current_era.saturating_sub(history_depth), Error::<T>::InvalidEraToReward);
+		ensure!(
+			era <= current_era && era >= current_era.saturating_sub(history_depth),
+			Error::<T>::InvalidEraToReward.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)?;
-
-		let controller = Self::bonded(&validator_stash).ok_or(Error::<T>::NotStash)?;
+			.ok_or_else(||
+				Error::<T>::InvalidEraToReward
+					.with_weight(T::WeightInfo::payout_stakers_alive_staked(0))
+			)?;
+
+		let controller = Self::bonded(&validator_stash).ok_or(
+			Error::<T>::NotStash.with_weight(T::WeightInfo::payout_stakers_alive_staked(0))
+		)?;
 		let mut ledger = <Ledger<T>>::get(&controller).ok_or_else(|| Error::<T>::NotController)?;
 
 		ledger.claimed_rewards.retain(|&x| x >= current_era.saturating_sub(history_depth));
 		match ledger.claimed_rewards.binary_search(&era) {
-			Ok(_) => Err(Error::<T>::AlreadyClaimed)?,
+			Ok(_) => Err(
+				Error::<T>::AlreadyClaimed.with_weight(T::WeightInfo::payout_stakers_alive_staked(0))
+			)?,
 			Err(pos) => ledger.claimed_rewards.insert(pos, era),
 		}
 
@@ -2008,7 +2019,9 @@ impl<T: Config> Module<T> {
 			.unwrap_or_else(|| Zero::zero());
 
 		// Nothing to do if they have no reward points.
-		if validator_reward_points.is_zero() { return Ok(())}
+		if validator_reward_points.is_zero() {
+			return Ok(Some(T::WeightInfo::payout_stakers_alive_staked(0)).into())
+		}
 
 		// This is the fraction of the total reward that the validator and the
 		// nominators will get.
@@ -2041,6 +2054,10 @@ impl<T: Config> Module<T> {
 			Self::deposit_event(RawEvent::Reward(ledger.stash, imbalance.peek()));
 		}
 
+		// Track the number of payout ops to nominators. Note: `WeightInfo::payout_stakers_alive_staked`
+		// always assumes at least a validator is paid out, so we do not need to count their payout op.
+		let mut nominator_payout_count: u32 = 0;
+
 		// Lets now calculate how this is split to the nominators.
 		// Reward only the clipped exposures. Note this is not necessarily sorted.
 		for nominator in exposure.others.iter() {
@@ -2052,11 +2069,14 @@ impl<T: Config> Module<T> {
 			let nominator_reward: BalanceOf<T> = nominator_exposure_part * validator_leftover_payout;
 			// We can now make nominator payout:
 			if let Some(imbalance) = Self::make_payout(&nominator.who, nominator_reward) {
+				// Note: this logic does not count payouts for `RewardDestination::None`.
+				nominator_payout_count += 1;
 				Self::deposit_event(RawEvent::Reward(nominator.who.clone(), imbalance.peek()));
 			}
 		}
 
-		Ok(())
+		debug_assert!(nominator_payout_count <= T::MaxNominatorRewardedPerValidator::get());
+		Ok(Some(T::WeightInfo::payout_stakers_alive_staked(nominator_payout_count)).into())
 	}
 
 	/// Update the ledger for a controller.

frame/staking/src/mock.rs

@@ -269,6 +269,8 @@ where
 }
 
 pub type Extrinsic = TestXt<Call, ()>;
+pub(crate) type StakingCall = crate::Call<Test>;
+pub(crate) type TestRuntimeCall = <Test as frame_system::Config>::Call;
 
 pub struct ExtBuilder {
 	validator_pool: bool,

frame/staking/src/tests.rs

@@ -20,12 +20,14 @@
 use super::*;
 use mock::*;
 use sp_runtime::{
-	assert_eq_error_rate, traits::BadOrigin,
+	assert_eq_error_rate,
+	traits::{BadOrigin, Dispatchable},
 };
 use sp_staking::offence::OffenceDetails;
 use frame_support::{
 	assert_ok, assert_noop, StorageMap,
 	traits::{Currency, ReservableCurrency, OnInitialize},
+	weights::{extract_actual_weight, GetDispatchInfo},
 };
 use pallet_balances::Error as BalancesError;
 use substrate_test_utils::assert_eq_uvec;
@@ -2976,6 +2978,9 @@ fn claim_reward_at_the_last_era_and_no_double_claim_and_invalid_claim() {
 	// * an invalid era to claim doesn't update last_reward
 	// * double claim of one era fails
 	ExtBuilder::default().nominate(true).build_and_execute(|| {
+		// Consumed weight for all payout_stakers dispatches that fail
+		let err_weight = weights::SubstrateWeight::<Test>::payout_stakers_alive_staked(0);
+
 		let init_balance_10 = Balances::total_balance(&10);
 		let init_balance_100 = Balances::total_balance(&100);
 
@@ -3021,19 +3026,19 @@ fn claim_reward_at_the_last_era_and_no_double_claim_and_invalid_claim() {
 		assert_noop!(
 			Staking::payout_stakers(Origin::signed(1337), 11, 0),
 			// Fail: Era out of history
-			Error::<Test>::InvalidEraToReward
+			Error::<Test>::InvalidEraToReward.with_weight(err_weight)
 		);
 		assert_ok!(Staking::payout_stakers(Origin::signed(1337), 11, 1));
 		assert_ok!(Staking::payout_stakers(Origin::signed(1337), 11, 2));
 		assert_noop!(
 			Staking::payout_stakers(Origin::signed(1337), 11, 2),
 			// Fail: Double claim
-			Error::<Test>::AlreadyClaimed
+			Error::<Test>::AlreadyClaimed.with_weight(err_weight)
 		);
 		assert_noop!(
 			Staking::payout_stakers(Origin::signed(1337), 11, active_era),
 			// Fail: Era not finished yet
-			Error::<Test>::InvalidEraToReward
+			Error::<Test>::InvalidEraToReward.with_weight(err_weight)
 		);
 
 		// Era 0 can't be rewarded anymore and current era can't be rewarded yet
@@ -3287,6 +3292,9 @@ fn test_payout_stakers() {
 fn payout_stakers_handles_basic_errors() {
 	// Here we will test payouts handle all errors.
 	ExtBuilder::default().has_stakers(false).build_and_execute(|| {
+		// Consumed weight for all payout_stakers dispatches that fail
+		let err_weight = weights::SubstrateWeight::<Test>::payout_stakers_alive_staked(0);
+
 		// Same setup as the test above
 		let balance = 1000;
 		bond_validator(11, 10, balance); // Default(64)
@@ -3305,9 +3313,15 @@ fn payout_stakers_handles_basic_errors() {
 		mock::start_active_era(2);
 
 		// Wrong Era, too big
-		assert_noop!(Staking::payout_stakers(Origin::signed(1337), 11, 2), Error::<Test>::InvalidEraToReward);
+		assert_noop!(
+			Staking::payout_stakers(Origin::signed(1337), 11, 2),
+			Error::<Test>::InvalidEraToReward.with_weight(err_weight)
+		);
 		// Wrong Staker
-		assert_noop!(Staking::payout_stakers(Origin::signed(1337), 10, 1), Error::<Test>::NotStash);
+		assert_noop!(
+			Staking::payout_stakers(Origin::signed(1337), 10, 1),
+			Error::<Test>::NotStash.with_weight(err_weight)
+		);
 
 		for i in 3..100 {
 			Staking::reward_by_ids(vec![(11, 1)]);
@@ -3317,14 +3331,134 @@ fn payout_stakers_handles_basic_errors() {
 		}
 		// We are at era 99, with history depth of 84
 		// We should be able to payout era 15 through 98 (84 total eras), but not 14 or 99.
-		assert_noop!(Staking::payout_stakers(Origin::signed(1337), 11, 14), Error::<Test>::InvalidEraToReward);
-		assert_noop!(Staking::payout_stakers(Origin::signed(1337), 11, 99), Error::<Test>::InvalidEraToReward);
+		assert_noop!(
+			Staking::payout_stakers(Origin::signed(1337), 11, 14),
+			Error::<Test>::InvalidEraToReward.with_weight(err_weight)
+		);
+		assert_noop!(
+			Staking::payout_stakers(Origin::signed(1337), 11, 99),
+			Error::<Test>::InvalidEraToReward.with_weight(err_weight)
+		);
 		assert_ok!(Staking::payout_stakers(Origin::signed(1337), 11, 15));
 		assert_ok!(Staking::payout_stakers(Origin::signed(1337), 11, 98));
 
 		// Can't claim again
-		assert_noop!(Staking::payout_stakers(Origin::signed(1337), 11, 15), Error::<Test>::AlreadyClaimed);
-		assert_noop!(Staking::payout_stakers(Origin::signed(1337), 11, 98), Error::<Test>::AlreadyClaimed);
+		assert_noop!(
+			Staking::payout_stakers(Origin::signed(1337), 11, 15),
+			Error::<Test>::AlreadyClaimed.with_weight(err_weight)
+		);
+		assert_noop!(
+			Staking::payout_stakers(Origin::signed(1337), 11, 98),
+			Error::<Test>::AlreadyClaimed.with_weight(err_weight)
+		);
+	});
+}
+
+#[test]
+fn payout_stakers_handles_weight_refund() {
+	// Note: this test relies on the assumption that `payout_stakers_alive_staked` is solely used by
+	// `payout_stakers` to calculate the weight of each payout op.
+	ExtBuilder::default().has_stakers(false).build_and_execute(|| {
+		let max_nom_rewarded = <Test as Config>::MaxNominatorRewardedPerValidator::get();
+		// Make sure the configured value is meaningful for our use.
+		assert!(max_nom_rewarded >= 4);
+		let half_max_nom_rewarded = max_nom_rewarded / 2;
+		// Sanity check our max and half max nominator quantities.
+		assert!(half_max_nom_rewarded > 0);
+		assert!(max_nom_rewarded > half_max_nom_rewarded);
+
+		let max_nom_rewarded_weight
+			= <Test as Config>::WeightInfo::payout_stakers_alive_staked(max_nom_rewarded);
+		let half_max_nom_rewarded_weight
+			= <Test as Config>::WeightInfo::payout_stakers_alive_staked(half_max_nom_rewarded);
+		let zero_nom_payouts_weight = <Test as Config>::WeightInfo::payout_stakers_alive_staked(0);
+		assert!(zero_nom_payouts_weight > 0);
+		assert!(half_max_nom_rewarded_weight > zero_nom_payouts_weight);
+		assert!(max_nom_rewarded_weight > half_max_nom_rewarded_weight);
+
+		let balance = 1000;
+		bond_validator(11, 10, balance);
+
+		/* Era 1 */
+		start_active_era(1);
+
+		// Reward just the validator.
+		Staking::reward_by_ids(vec![(11, 1)]);
+
+		// Add some `half_max_nom_rewarded` nominators who will start backing the validator in the
+		// next era.
+		for i in 0..half_max_nom_rewarded {
+			bond_nominator((1000 + i).into(), (100 + i).into(), balance + i as Balance, vec![11]);
+		}
+
+		/* Era 2 */
+		start_active_era(2);
+
+		// Collect payouts when there are no nominators
+		let call = TestRuntimeCall::Staking(StakingCall::payout_stakers(11, 1));
+		let info = call.get_dispatch_info();
+		let result = call.dispatch(Origin::signed(20));
+		assert_ok!(result);
+		assert_eq!(
+			extract_actual_weight(&result, &info),
+			zero_nom_payouts_weight
+		);
+
+		// The validator is not rewarded in this era; so there will be zero payouts to claim for this era.
+
+		/* Era 3 */
+		start_active_era(3);
+
+		// Collect payouts for an era where the validator did not receive any points.
+		let call = TestRuntimeCall::Staking(StakingCall::payout_stakers(11, 2));
+		let info = call.get_dispatch_info();
+		let result = call.dispatch(Origin::signed(20));
+		assert_ok!(result);
+		assert_eq!(extract_actual_weight(&result, &info), zero_nom_payouts_weight);
+
+		// Reward the validator and its nominators.
+		Staking::reward_by_ids(vec![(11, 1)]);
+
+		/* Era 4 */
+		start_active_era(4);
+
+		// Collect payouts when the validator has `half_max_nom_rewarded` nominators.
+		let call = TestRuntimeCall::Staking(StakingCall::payout_stakers(11, 3));
+		let info = call.get_dispatch_info();
+		let result = call.dispatch(Origin::signed(20));
+		assert_ok!(result);
+		assert_eq!(extract_actual_weight(&result, &info), half_max_nom_rewarded_weight);
+
+		// Add enough nominators so that we are at the limit. They will be active nominators
+		// in the next era.
+		for i in half_max_nom_rewarded..max_nom_rewarded {
+			bond_nominator((1000 + i).into(), (100 + i).into(), balance + i as Balance, vec![11]);
+		}
+
+		/* Era 5 */
+		start_active_era(5);
+		// We now have `max_nom_rewarded` nominators actively nominating our validator.
+
+		// Reward the validator so we can collect for everyone in the next era.
+		Staking::reward_by_ids(vec![(11, 1)]);
+
+		/* Era 6 */
+		start_active_era(6);
+
+		// Collect payouts when the validator had `half_max_nom_rewarded` nominators.
+		let call = TestRuntimeCall::Staking(StakingCall::payout_stakers(11, 5));
+		let info = call.get_dispatch_info();
+		let result = call.dispatch(Origin::signed(20));
+		assert_ok!(result);
+		assert_eq!(extract_actual_weight(&result, &info), max_nom_rewarded_weight);
+
+		// Try and collect payouts for an era that has already been collected.
+		let call = TestRuntimeCall::Staking(StakingCall::payout_stakers(11, 5));
+		let info = call.get_dispatch_info();
+		let result = call.dispatch(Origin::signed(20));
+		assert!(result.is_err());
+		// When there is an error the consumed weight == weight when there are 0 nominator payouts.
+		assert_eq!(extract_actual_weight(&result, &info), zero_nom_payouts_weight);
 	});
 }