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 2 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 @@ -2309,7 +2309,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
4 changes: 4 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 @@ -505,6 +505,10 @@ pub fn active_era() -> EraIndex {
Staking::active_era().unwrap().index
}

pub fn current_era() -> EraIndex {
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
Staking::current_era().unwrap()
}

pub fn check_exposure_all(era: EraIndex) {
ErasStakers::<Test>::iter_prefix(era).for_each(check_exposure)
}
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
98 changes: 76 additions & 22 deletions frame/staking/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2963,7 +2963,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));

let queued_result = Staking::queued_elected().unwrap();
Expand All @@ -2990,6 +2990,60 @@ mod offchain_phragmen {
})
}

#[test]
fn signed_result_can_be_submitted_regardless_active_era() {
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
// should check that we have a new validator set normally,
// event says that it comes from offchain.
// active_era is not used for election related stuff.
ExtBuilder::default()
.offchain_phragmen_ext()
.build()
.execute_with(|| {
run_to_block(32);
<Staking as Store>::ActiveEra::put(ActiveEraInfo {
index: 0,
start: Some(0),
});
assert_eq!(Staking::era_election_status(), ElectionStatus::Open(32));
assert!(Staking::snapshot_validators().is_some());

let (compact, winners, score) = prepare_submission_with(true, |_| {});
assert_ok!(Staking::submit_election_solution(
Origin::signed(10),
winners,
compact,
score,
current_era(),
));

let queued_result = Staking::queued_elected().unwrap();
assert_eq!(queued_result.compute, ElectionCompute::Signed);

run_to_block(35);
<Staking as Store>::ActiveEra::put(ActiveEraInfo {
index: 0,
start: Some(0),
});
assert_eq!(Staking::era_election_status(), ElectionStatus::Closed);

assert_eq!(
System::events()
.into_iter()
.map(|r| r.event)
.filter_map(|e| {
if let MetaEvent::staking(inner) = e {
Some(inner)
} else {
None
}
})
.last()
.unwrap(),
RawEvent::StakingElection(ElectionCompute::Signed),
);
})
}

#[test]
fn signed_result_can_be_submitted_later() {
// same as `signed_result_can_be_submitted` but at a later block.
Expand All @@ -3006,7 +3060,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 @@ -3056,7 +3110,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenEarlySubmission,
);
Expand All @@ -3082,7 +3136,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));

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

// a better solution
Expand All @@ -3129,7 +3183,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));
})
}
Expand Down Expand Up @@ -3191,7 +3245,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 @@ -3242,7 +3296,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusWinnerCount,
);
Expand Down Expand Up @@ -3273,7 +3327,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusWinnerCount,
);
Expand Down Expand Up @@ -3302,7 +3356,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),);
})
}
Expand Down Expand Up @@ -3333,7 +3387,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusCompact,
);
Expand Down Expand Up @@ -3366,7 +3420,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusCompact,
);
Expand Down Expand Up @@ -3398,7 +3452,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusWinner,
);
Expand Down Expand Up @@ -3434,7 +3488,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusEdge,
);
Expand Down Expand Up @@ -3470,7 +3524,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusSelfVote,
);
Expand Down Expand Up @@ -3506,7 +3560,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusSelfVote,
);
Expand Down Expand Up @@ -3541,7 +3595,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusCompact,
);
Expand Down Expand Up @@ -3583,7 +3637,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusNomination,
);
Expand Down Expand Up @@ -3646,7 +3700,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
));

// a wrong solution.
Expand All @@ -3665,7 +3719,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenSlashedNomination,
);
Expand Down Expand Up @@ -3693,7 +3747,7 @@ mod offchain_phragmen {
winners,
compact,
score,
active_era(),
current_era(),
),
Error::<Test>::PhragmenBogusScore,
);
Expand Down