-
Notifications
You must be signed in to change notification settings - Fork 1.2k
EPMB/unsigned: fixed multi-page winner computation #8987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8cd71d0
cb7de5d
daf0e62
a55bb36
7ca791b
9bb9e35
5766ea0
e399119
5ce6e1a
eb94c2d
19e9e99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| title: 'EPMB/unsigned: fixed multi-page winner computation' | ||
| doc: | ||
| - audience: Runtime User | ||
| description: |- | ||
| Change the calculation of `MaxWinnersPerPage` in `FullSupportsOfMiner` to `Pages * MaxWinnersPerPage` (instead of the overall maximum number of winners across pages) | ||
| to prevent the computed solution from having a low overall total of winners, which could result in a `WrongWinnerCount` error. | ||
|
|
||
| crates: | ||
| - name: pallet-election-provider-multi-block | ||
| bump: minor |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -221,14 +221,23 @@ pub type PageSupportsOfMiner<T> = frame_election_provider_support::BoundedSuppor | |
| <T as MinerConfig>::MaxBackersPerWinner, | ||
| >; | ||
|
|
||
| /// Helper type that computes the maximum total winners across all pages. | ||
| pub struct MaxWinnersFinal<T: MinerConfig>(core::marker::PhantomData<T>); | ||
|
|
||
| impl<T: MinerConfig> frame_support::traits::Get<u32> for MaxWinnersFinal<T> { | ||
| fn get() -> u32 { | ||
| T::Pages::get().saturating_mul(T::MaxWinnersPerPage::get()) | ||
| } | ||
| } | ||
|
|
||
| /// The full version of [`PageSupportsOfMiner`]. | ||
| /// | ||
| /// This should be used on a support instance that is encapsulating the full solution. | ||
| /// | ||
| /// Another way to look at it, this is never wrapped in a `Vec<_>` | ||
| pub type FullSupportsOfMiner<T> = frame_election_provider_support::BoundedSupports< | ||
| <T as MinerConfig>::AccountId, | ||
| <T as MinerConfig>::MaxWinnersPerPage, | ||
| MaxWinnersFinal<T>, | ||
| <T as MinerConfig>::MaxBackersPerWinnerFinal, | ||
| >; | ||
|
|
||
|
|
@@ -1335,6 +1344,52 @@ mod trimming { | |
| ); | ||
| }) | ||
| } | ||
|
|
||
| #[test] | ||
| fn aggressive_backer_trimming_maintains_winner_count() { | ||
| // Test the scenario where aggressive backer trimming is applied but the solution | ||
| // should still maintain the correct winner count to avoid WrongWinnerCount errors. | ||
| ExtBuilder::unsigned() | ||
| .desired_targets(3) | ||
| .max_winners_per_page(2) | ||
| .pages(2) | ||
| .max_backers_per_winner_final(1) // aggressive final trimming | ||
| .max_backers_per_winner(1) // aggressive per-page trimming | ||
| .build_and_execute(|| { | ||
| // Use default 4 targets to stay within TargetSnapshotPerBlock limit | ||
|
|
||
| // Adjust the voters a bit, such that they are all different backings | ||
| let mut current_voters = Voters::get(); | ||
| current_voters.iter_mut().for_each(|(who, stake, ..)| *stake = *who); | ||
| Voters::set(current_voters); | ||
|
|
||
| roll_to_snapshot_created(); | ||
|
|
||
| let solution = mine_full_solution().unwrap(); | ||
|
|
||
| // The solution should still be valid despite aggressive trimming | ||
| assert!(solution.solution_pages.len() > 0); | ||
|
|
||
| let winner_count = solution | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not a great name, but I found
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not completely sold but also I don't mind to change the name either, I was hoping that the comments were enough to explain what we are doing in the test
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it is fine either way. Just noted that we have a helper fn that does the same. |
||
| .solution_pages | ||
| .iter() | ||
| .flat_map(|page| page.unique_targets()) | ||
| .collect::<std::collections::HashSet<_>>() | ||
| .len(); | ||
|
|
||
| // We should get 3 winners. | ||
| // This demonstrates that FullSupportsOfMiner can accommodate winners from multiple | ||
| // pages and can hold more winners than MaxWinnersPerPage. | ||
| assert_eq!(winner_count, 3); | ||
|
|
||
| // Load and verify the solution passes all checks without WrongWinnerCount error | ||
| load_mock_signed_and_start(solution); | ||
| let _supports = roll_to_full_verification(); | ||
|
|
||
| // A solution should be successfully queued | ||
| assert!(VerifierPallet::queued_score().is_some()); | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can actually see the naming convention I mentioned here, noting the
Finalpostfix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to use
Finalfor consistency, ty