Substrate: In staking rewards go to next validator set

[thiolliere]

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.

[thiolliere]

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

currently it is:

https://github.com/paritytech/substrate/blob/524413a55c026764bc2a70378491ce5752c5b951/paint/staking/src/lib.rs#L1456-L1462

with OnSessionEnding being defined as:

https://github.com/paritytech/substrate/blob/524413a55c026764bc2a70378491ce5752c5b951/paint/session/src/lib.rs#L165-L184

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)

[thiolliere]

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.

[thiolliere]

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.

[thiolliere]

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.

[thiolliere]

close by https://github.com/paritytech/substrate/pull/4474