Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Phragmen solution should submit for current era and be checked against current era #5583

Merged
merged 6 commits into from
Apr 14, 2020
Merged

Phragmen solution should submit for current era and be checked against current era #5583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
28ae548
submit solution for current era
gui1117 Apr 8, 2020
576efef
add test
gui1117 Apr 8, 2020
e2ff7bc
address comment
gui1117 Apr 9, 2020
e8f1aac
Merge remote-tracking branch 'origin/master' into gui-fix-submit-solu…
gui1117 Apr 9, 2020
a066fe8
fix tests
gui1117 Apr 9, 2020
29aba71
Merge remote-tracking branch 'origin/master' into gui-fix-submit-solu…
gui1117 Apr 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frame/staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2327,7 +2327,7 @@ impl<T: Trait> Module<T> {
);

// check current era.
if let Some(current_era) = Self::active_era().map(|e| e.index) {
if let Some(current_era) = Self::current_era() {
ensure!(
current_era == era,
Error::<T>::PhragmenEarlySubmission,
Expand Down
8 changes: 8 additions & 0 deletions frame/staking/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,10 @@ pub type Session = pallet_session::Module<Test>;
pub type Timestamp = pallet_timestamp::Module<Test>;
pub type Staking = Module<Test>;

pub(crate) fn current_era() -> EraIndex {
Staking::current_era().unwrap()
}

fn post_conditions() {
check_nominators();
check_exposures();
Expand Down Expand Up @@ -649,9 +653,13 @@ pub(crate) fn start_session(session_index: SessionIndex) {
assert_eq!(Session::current_index(), session_index);
}

// 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(crate) fn start_era(era_index: EraIndex) {
start_session((era_index * <SessionsPerEra as Get<u32>>::get()).into());
assert_eq!(Staking::current_era().unwrap(), era_index);
assert_eq!(Staking::active_era().unwrap().index, era_index);
}

pub(crate) fn current_total_payout_for_duration(duration: u64) -> Balance {
Expand Down
6 changes: 3 additions & 3 deletions frame/staking/src/offchain_election.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,15 +113,15 @@ pub(crate) fn compute_offchain_election<T: Trait>() -> Result<(), OffchainElecti
// process and prepare it for submission.
let (winners, compact, score) = prepare_submission::<T>(assignments, winners, true)?;

// defensive-only: active era can never be none except genesis.
let era = <Module<T>>::active_era().map(|e| e.index).unwrap_or_default();
// defensive-only: current era can never be none except genesis.
let current_era = <Module<T>>::current_era().unwrap_or_default();
Comment on lines +116 to +117
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// send it.
let call: <T as Trait>::Call = Call::submit_election_solution_unsigned(
winners,
compact,
score,
era,
current_era,
).into();

T::SubmitTransaction::submit_unsigned(call)
Expand Down
70 changes: 35 additions & 35 deletions frame/staking/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1847,20 +1847,20 @@ fn era_is_always_same_length() {
let session_per_era = <SessionsPerEra as Get<SessionIndex>>::get();

mock::start_era(1);
assert_eq!(Staking::eras_start_session_index(active_era()).unwrap(), session_per_era);
assert_eq!(Staking::eras_start_session_index(current_era()).unwrap(), session_per_era);

mock::start_era(2);
assert_eq!(Staking::eras_start_session_index(active_era()).unwrap(), session_per_era * 2u32);
assert_eq!(Staking::eras_start_session_index(current_era()).unwrap(), session_per_era * 2u32);

let session = Session::current_index();
ForceEra::put(Forcing::ForceNew);
advance_session();
advance_session();
assert_eq!(Staking::active_era().unwrap().index, 3);
assert_eq!(Staking::eras_start_session_index(active_era()).unwrap(), session + 2);
assert_eq!(current_era(), 3);
assert_eq!(Staking::eras_start_session_index(current_era()).unwrap(), session + 2);

mock::start_era(4);
assert_eq!(Staking::eras_start_session_index(active_era()).unwrap(), session + 2u32 + session_per_era);
assert_eq!(Staking::eras_start_session_index(current_era()).unwrap(), session + 2u32 + session_per_era);
});
}

Expand Down Expand Up @@ -2912,7 +2912,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));

let queued_result = Staking::queued_elected().unwrap();
Expand Down Expand Up @@ -2955,7 +2955,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that for all those test current_era == active_era anyway.

Copy link
Contributor

@kianenigma kianenigma Apr 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed if we want that we need to use a different session-pallet which have more than one session delay.

));

let queued_result = Staking::queued_elected().unwrap();
Expand Down Expand Up @@ -3005,7 +3005,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenEarlySubmission,
);
Expand All @@ -3031,7 +3031,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));

// a bad solution
Expand All @@ -3042,7 +3042,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenWeakSubmission,
);
Expand All @@ -3068,7 +3068,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));

// a better solution
Expand All @@ -3078,7 +3078,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));
})
}
Expand Down Expand Up @@ -3116,7 +3116,7 @@ mod offchain_phragmen {
TransactionValidity::Ok(ValidTransaction {
priority: (1 << 20) + 1125, // the proposed slot stake.
requires: vec![],
provides: vec![("StakingOffchain", active_era()).encode()],
provides: vec![("StakingOffchain", current_era()).encode()],
longevity: 3,
propagate: false,
})
Expand All @@ -3140,7 +3140,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),);

// now run the offchain worker in the same chain state.
Expand Down Expand Up @@ -3191,7 +3191,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusWinnerCount,
);
Expand Down Expand Up @@ -3222,7 +3222,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusWinnerCount,
);
Expand Down Expand Up @@ -3251,7 +3251,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),);
})
}
Expand Down Expand Up @@ -3282,7 +3282,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusCompact,
);
Expand Down Expand Up @@ -3315,7 +3315,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusCompact,
);
Expand Down Expand Up @@ -3347,7 +3347,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusWinner,
);
Expand Down Expand Up @@ -3383,7 +3383,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusEdge,
);
Expand Down Expand Up @@ -3419,7 +3419,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusSelfVote,
);
Expand Down Expand Up @@ -3455,7 +3455,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusSelfVote,
);
Expand Down Expand Up @@ -3490,7 +3490,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusCompact,
);
Expand Down Expand Up @@ -3532,7 +3532,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusNomination,
);
Expand Down Expand Up @@ -3560,7 +3560,7 @@ mod offchain_phragmen {
run_to_block(20);

// slash 10. This must happen outside of the election window.
let offender_expo = Staking::eras_stakers(active_era(), 11);
let offender_expo = Staking::eras_stakers(Staking::active_era().unwrap().index, 11);
on_offence_now(
&[OffenceDetails {
offender: (11, offender_expo.clone()),
Expand Down Expand Up @@ -3595,7 +3595,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));

// a wrong solution.
Expand All @@ -3614,7 +3614,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenSlashedNomination,
);
Expand Down Expand Up @@ -3642,7 +3642,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusScore,
);
Expand Down Expand Up @@ -3729,7 +3729,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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure this is correct and the eras_stakers should be indexed by active era here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// panic from the impl in mock
on_offence_now(
Expand All @@ -3754,8 +3754,8 @@ fn slash_kicks_validators_not_nominators_and_disables_nominator_for_kicked_valid
assert_eq!(Balances::free_balance(101), 2000);

// 11 and 21 both have the support of 100
let exposure_11 = Staking::eras_stakers(active_era(), &11);
let exposure_21 = Staking::eras_stakers(active_era(), &21);
let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11);
let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21);

assert_eq!(exposure_11.total, 1000 + 125);
assert_eq!(exposure_21.total, 1000 + 375);
Expand Down Expand Up @@ -3795,8 +3795,8 @@ fn slash_kicks_validators_not_nominators_and_disables_nominator_for_kicked_valid
assert_ok!(Staking::validate(Origin::signed(10), Default::default()));

mock::start_era(2);
let exposure_11 = Staking::eras_stakers(active_era(), &11);
let exposure_21 = Staking::eras_stakers(active_era(), &21);
let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11);
let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21);

// 10 is re-elected, but without the support of 100
assert_eq!(exposure_11.total, 900);
Expand Down Expand Up @@ -3897,7 +3897,7 @@ fn zero_slash_keeps_nominators() {

assert_eq!(Balances::free_balance(11), 1000);

let exposure = Staking::eras_stakers(active_era(), 11);
let exposure = Staking::eras_stakers(Staking::active_era().unwrap().index, 11);
assert_eq!(Balances::free_balance(101), 2000);

on_offence_now(
Expand Down