In staking rewards go to next validator set

[gui1117]

Issue

session.validators contains the current set of validator validating and producing blocks,
session.queued_keys contains the next set.

in on_session_ending in case of a new era, the staking module elect a new set, put this set in staking::CurrentElected and returns it, session module put this new elected set in the queued keys.

But in staking::reward_by_ids we add reward points for a validator in stakig::CurrentElected.
And at the end of the era we reward according to this points the validators contained in staking::CurrentElected. This seems wrong as staking::CurrentElected at this moment contains the validator set for the next era not the current one.

How to fix

same as session, staking should have information for next_session but also for current_session. Maybe storing the staking information for previous elected set so it can reward them correctly.

CC @jacogr

[kianenigma]

I was also asked about this and it a bug, forgot to issue it actually and thanks for doing that. I think staking should also store 2 eras to always know the current live set. This will also be needed for offchian phragmen, as discussed internally.

[gui1117]

I think all of this boils down to just implementing OnSessionEnding correctly:

currently it is:

substrate/paint/staking/src/lib.rs

Lines 1456 to 1462 in 524413a

	impl<T: Trait> OnSessionEnding<T::AccountId, Exposure<T::AccountId, BalanceOf<T>>> for Module<T> {
	fn on_session_ending(_ending: SessionIndex, start_session: SessionIndex)
	-> Option<(Vec<T::AccountId>, Vec<(T::AccountId, Exposure<T::AccountId, BalanceOf<T>>)>)>
	{
	Self::new_session(start_session - 1)
	}
	}

with OnSessionEnding being defined as:

substrate/paint/session/src/lib.rs

Lines 165 to 184 in 524413a

	/// An event handler for when the session is ending.
	/// TODO [slashing] consider renaming to OnSessionStarting
	pub trait OnSessionEnding<ValidatorId> {
	/// Handle the fact that the session is ending, and optionally provide the new validator set.
	///
	/// Even if the validator-set is the same as before, if any underlying economic
	/// conditions have changed (i.e. stake-weights), the new validator set must be returned.
	/// This is necessary for consensus engines making use of the session module to
	/// issue a validator-set change so misbehavior can be provably associated with the new
	/// economic conditions as opposed to the old.
	///
	/// `ending_index` is the index of the currently ending session.
	/// The returned validator set, if any, will not be applied until `will_apply_at`.
	/// `will_apply_at` is guaranteed to be at least `ending_index + 1`, since session indices don't
	/// repeat, but it could be some time after in case we are staging authority set changes.
	fn on_session_ending(
	ending_index: SessionIndex,
	will_apply_at: SessionIndex
	) -> Option<Vec<ValidatorId>>;
	}

I think staking should store the stake information for the session will_apply_at and use the information stored at session_ending to reward the current era. (and prune this information on session ending)

[gui1117]

I got a wip branch https://github.com/paritytech/substrate/compare/gui-staking-fix-wrong-reward-attribution?expand=1

But it needs to update test, also maybe there is some issue inside, and moreover we need to transition from current logic to this logic and this might need some additional conditions.

EDIT: I'm currently working on prefixed map though

[kianenigma]

https://github.com/paritytech/substrate/compare/gui-staking-fix-wrong-reward-attribution?expand=1#diff-cdf560c4db60ec139dae0470ea9988beR601

I would store just two storage items with known keys and hashes, instead of a map, since we are 100% sure we just want 1 additional entry to store the next + current validators. I think map is not needed here.

[gui1117]

I think the current trait OnSessionEnding doesn't guarantee that there is only one era ahead.

If will_apply_at is 10 session after the current one and there is a forced new era on each session then we should store something like 10 era. to be able to reward for the current era which is 10 era in the past.

[gui1117]

I'm working on this but it takes me time, also I want to inform that this might also affect slashing as maybe this wrong validator set is also register for historical session and so on.