Phragmen solution should submit for current era and be checked against current era #5583
Conversation
| @@ -3006,7 +3060,7 @@ mod offchain_phragmen { | |||
| winners, | |||
| compact, | |||
| score, | |||
| active_era(), | |||
| current_era(), | |||
There was a problem hiding this comment.
note that for all those test current_era == active_era anyway.
There was a problem hiding this comment.
What would be substantially better is to change the test setup to also have this one session drift. This is probably a lot of work, but I think a good step forward. In that case, I would expect active_era and current_era difference to matter.
Also related #5244
There was a problem hiding this comment.
indeed if we want that we need to use a different session-pallet which have more than one session delay.
gui1117
added
A3-in_progress
frame/staking/src/mock.rs
Outdated
| // This start and activate the era given. | ||
| // Because the mock use pallet-session which delays session by one, this will be one session after | ||
| // the election happened, not the first session after the election has happened. | ||
| pub fn start_era(era_index: EraIndex) { |
There was a problem hiding this comment.
I modified the test to use current_era instead of active_era but actually the tests generally as you pointed in #5244 the tests doesn't test this difference.
Also start_era is ambiguous and used to both start the current era or the active era.
But in the implementation it actually starts the active_era (without taking into account any Forced stuff, maybe would be better to change it with a while loop.
| // defensive-only: current era can never be none except genesis. | ||
| let current_era = <Module<T>>::current_era().unwrap_or_default(); |
There was a problem hiding this comment.
Also I don't think this is defensive, if current_era is none it means there is no current_era, maybe the staking module hasn't started yet, but anyway I think it should return OffchainElectionError, but this is out of scope of the PR
|
I updated the tests but they still doesn't cover this change indeed. should #5244 be part of this PR or should we merge this as is ? |
gui1117
added
A0-please_review
| @@ -3780,7 +3780,7 @@ mod offchain_phragmen { | |||
| run_to_block(12); | |||
| assert_eq!(Staking::era_election_status(), ElectionStatus::Open(12)); | |||
|
|
|||
| let offender_expo = Staking::eras_stakers(active_era(), 10); | |||
| let offender_expo = Staking::eras_stakers(Staking::active_era().unwrap().index, 10); | |||
There was a problem hiding this comment.
are we sure this is correct and the eras_stakers should be indexed by active era here?
There was a problem hiding this comment.
offence can come from active era or previous era (i.e. a validator must be validating or having been validating in order to commit an offence), it reports the exposures of the era of the offence.
There was a problem hiding this comment.
it depends on this:
pub(crate) fn on_offence_now(
offenders: &[OffenceDetails<AccountId, pallet_session::historical::IdentificationTuple<Test>>],
slash_fraction: &[Perbill],
) {
let now = Staking::active_era().unwrap().index;
on_offence_in_era(offenders, slash_fraction, now)
}it is fine for now.
There was a problem hiding this comment.
This does fix the phragmen issue so approve for now, but long term i'd really like to see a revamp of staking tests so that:
1- active_era and current_era are actually different.
2- sessions is actually queuing the receiving validators (same as all of our real chains).
kianenigma
added
A3-needsresolving
and removed
A0-please_review
…tion-for-current-era
phragmen solution should be submitted for the current era. the pallet-session can decide to delay by one or multiple era and dynamically. The active era should only be used for rewards stuff.
Phragmen solution submittion submit solution for the next current era so they should be submitted in the current_era, regardless of the active_era.