From 8cd71d0cce98ad40e055bef01ea053f9bb11f2a9 Mon Sep 17 00:00:00 2001 From: Paolo La Camera Date: Wed, 25 Jun 2025 21:35:36 +0200 Subject: [PATCH 1/7] EPMB/unsigned: fixed multi-page winner computation In `FullSupportsOfMiner`, we initially considered `MaxWinnersPerPage` as the overall maximum total winners across pages. However, it should be calculated as `Pages * MaxWinnersPerPage`. This bug was identified in [#staking-miner-1074](https://github.com/paritytech/polkadot-staking-miner/issues/1074) while testing the staking miner in a setup with aggressive trimming to evaluate backer bounding. Consequently, the computed solution had a low overall total of winners and was rejected with a `WrongWinnerCount` error. A test has been added to replicate this scenario. --- .../src/mock/mod.rs | 6 ++ .../src/unsigned/miner.rs | 57 ++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/substrate/frame/election-provider-multi-block/src/mock/mod.rs b/substrate/frame/election-provider-multi-block/src/mock/mod.rs index 1c5581a4a8e60..120d76f9d183f 100644 --- a/substrate/frame/election-provider-multi-block/src/mock/mod.rs +++ b/substrate/frame/election-provider-multi-block/src/mock/mod.rs @@ -409,6 +409,12 @@ impl ExtBuilder { SignedMaxSubmissions::set(s); self } + + pub(crate) fn max_winners_per_page(self, w: u32) -> Self { + MaxWinnersPerPage::set(w); + self + } + #[allow(unused)] pub(crate) fn add_voter(self, who: AccountId, stake: Balance, targets: Vec) -> Self { staking::VOTERS.with(|v| v.borrow_mut().push((who, stake, targets.try_into().unwrap()))); diff --git a/substrate/frame/election-provider-multi-block/src/unsigned/miner.rs b/substrate/frame/election-provider-multi-block/src/unsigned/miner.rs index ed099d8b513ee..e232fb7325207 100644 --- a/substrate/frame/election-provider-multi-block/src/unsigned/miner.rs +++ b/substrate/frame/election-provider-multi-block/src/unsigned/miner.rs @@ -221,6 +221,15 @@ pub type PageSupportsOfMiner = frame_election_provider_support::BoundedSuppor ::MaxBackersPerWinner, >; +/// Helper type that computes the maximum total winners across all pages. +pub struct MaxWinnersTotal(core::marker::PhantomData); + +impl frame_support::traits::Get for MaxWinnersTotal { + 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. @@ -228,7 +237,7 @@ pub type PageSupportsOfMiner = frame_election_provider_support::BoundedSuppor /// Another way to look at it, this is never wrapped in a `Vec<_>` pub type FullSupportsOfMiner = frame_election_provider_support::BoundedSupports< ::AccountId, - ::MaxWinnersPerPage, + MaxWinnersTotal, ::MaxBackersPerWinnerFinal, >; @@ -1332,6 +1341,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 + .solution_pages + .iter() + .flat_map(|page| page.unique_targets()) + .collect::>() + .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)] From cb7de5dee46fd1d99697f534638bfc96af0eb74b Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 25 Jun 2025 21:29:39 +0000 Subject: [PATCH 2/7] Update from github-actions[bot] running command 'prdoc --audience runtime_dev --bump patch' --- prdoc/pr_8987.prdoc | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 prdoc/pr_8987.prdoc diff --git a/prdoc/pr_8987.prdoc b/prdoc/pr_8987.prdoc new file mode 100644 index 0000000000000..e3470bceecfe7 --- /dev/null +++ b/prdoc/pr_8987.prdoc @@ -0,0 +1,33 @@ +title: 'EPMB/unsigned: fixed multi-page winner computation' +doc: +- audience: Runtime Dev + description: |- + ## The issue + + In `FullSupportsOfMiner`, we initially considered `MaxWinnersPerPage` as the overall maximum number of winners across pages. However, it should be calculated as `Pages * MaxWinnersPerPage` to prevent the computed solution from having a low overall total of winners, which could result in a `WrongWinnerCount` error. + + This bug was identified in + [#staking-miner-1074](https://github.com/paritytech/polkadot-staking-miner/issues/1074) while testing the staking miner in a setup with aggressive trimming to evaluate backer bounding. + + ## How to test + + A test has been added to replicate this scenario: + + Test configuration: + - `Pages=2` + - `MaxWinnersPerPage=2` + - `desired_targets=3` (3 or 4 doesn't matter here, the key point is that is strictly > `MaxWinnersPerPage`) + - `MaxBackersPerWinner=1` + + Before the fix in this PR: + - `FullSupportsOfMiner` could only hold 2 winners in total (bounded by `MaxWinnersPerPage`) + - But the mining algorithm needed to hold 3 winners across multiple pages + - This would cause a `WrongWinnerCount` error during verification + + With the fix: + - `FullSupportsOfMiner` can now hold `Pages * MaxWinnersPerPage = 2 * 2 = 4` winners + - The test passes with 3 winners across 2 pages (proving it can hold more than `MaxWinnersPerPage=2`) + - No `WrongWinnerCount` errors occur +crates: +- name: pallet-election-provider-multi-block + bump: patch From daf0e622e8af7aa291a5df5c21ce5da76ffa931b Mon Sep 17 00:00:00 2001 From: Paolo La Camera Date: Wed, 25 Jun 2025 23:38:54 +0200 Subject: [PATCH 3/7] Fixed prdoc --- prdoc/pr_8987.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_8987.prdoc b/prdoc/pr_8987.prdoc index e3470bceecfe7..8c9ea46761d47 100644 --- a/prdoc/pr_8987.prdoc +++ b/prdoc/pr_8987.prdoc @@ -30,4 +30,4 @@ doc: - No `WrongWinnerCount` errors occur crates: - name: pallet-election-provider-multi-block - bump: patch + bump: minor From e3991193a5f81255b0dd1f8b1fa7a064171ccfcf Mon Sep 17 00:00:00 2001 From: Paolo La Camera Date: Mon, 30 Jun 2025 10:31:51 +0200 Subject: [PATCH 4/7] Improve prdoc --- prdoc/pr_8987.prdoc | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/prdoc/pr_8987.prdoc b/prdoc/pr_8987.prdoc index 8c9ea46761d47..1bbdebc14423c 100644 --- a/prdoc/pr_8987.prdoc +++ b/prdoc/pr_8987.prdoc @@ -2,32 +2,9 @@ title: 'EPMB/unsigned: fixed multi-page winner computation' doc: - audience: Runtime Dev description: |- - ## The issue + In `FullSupportsOfMiner`, we initially considered `MaxWinnersPerPage` as the overall maximum number of winners across pages. + However, it should be calculated as `Pages * MaxWinnersPerPage` to prevent the computed solution from having a low overall total of winners, which could result in a `WrongWinnerCount` error. - In `FullSupportsOfMiner`, we initially considered `MaxWinnersPerPage` as the overall maximum number of winners across pages. However, it should be calculated as `Pages * MaxWinnersPerPage` to prevent the computed solution from having a low overall total of winners, which could result in a `WrongWinnerCount` error. - - This bug was identified in - [#staking-miner-1074](https://github.com/paritytech/polkadot-staking-miner/issues/1074) while testing the staking miner in a setup with aggressive trimming to evaluate backer bounding. - - ## How to test - - A test has been added to replicate this scenario: - - Test configuration: - - `Pages=2` - - `MaxWinnersPerPage=2` - - `desired_targets=3` (3 or 4 doesn't matter here, the key point is that is strictly > `MaxWinnersPerPage`) - - `MaxBackersPerWinner=1` - - Before the fix in this PR: - - `FullSupportsOfMiner` could only hold 2 winners in total (bounded by `MaxWinnersPerPage`) - - But the mining algorithm needed to hold 3 winners across multiple pages - - This would cause a `WrongWinnerCount` error during verification - - With the fix: - - `FullSupportsOfMiner` can now hold `Pages * MaxWinnersPerPage = 2 * 2 = 4` winners - - The test passes with 3 winners across 2 pages (proving it can hold more than `MaxWinnersPerPage=2`) - - No `WrongWinnerCount` errors occur crates: - name: pallet-election-provider-multi-block bump: minor From 5ce6e1aa75673ffdbe9fac25ad1439c367205256 Mon Sep 17 00:00:00 2001 From: Paolo La Camera Date: Mon, 30 Jun 2025 10:46:29 +0200 Subject: [PATCH 5/7] Updated pr_doc --- prdoc/pr_8987.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_8987.prdoc b/prdoc/pr_8987.prdoc index 1bbdebc14423c..de23fb42577f0 100644 --- a/prdoc/pr_8987.prdoc +++ b/prdoc/pr_8987.prdoc @@ -1,6 +1,6 @@ title: 'EPMB/unsigned: fixed multi-page winner computation' doc: -- audience: Runtime Dev +- audience: Runtime User description: |- In `FullSupportsOfMiner`, we initially considered `MaxWinnersPerPage` as the overall maximum number of winners across pages. However, it should be calculated as `Pages * MaxWinnersPerPage` to prevent the computed solution from having a low overall total of winners, which could result in a `WrongWinnerCount` error. From eb94c2ded2d7eac58971d6698ac86c7ee7a1ed5f Mon Sep 17 00:00:00 2001 From: Paolo La Camera Date: Mon, 30 Jun 2025 10:52:49 +0200 Subject: [PATCH 6/7] Cleanup PRdoc --- prdoc/pr_8987.prdoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prdoc/pr_8987.prdoc b/prdoc/pr_8987.prdoc index de23fb42577f0..46fd6a3be3c98 100644 --- a/prdoc/pr_8987.prdoc +++ b/prdoc/pr_8987.prdoc @@ -2,8 +2,8 @@ title: 'EPMB/unsigned: fixed multi-page winner computation' doc: - audience: Runtime User description: |- - In `FullSupportsOfMiner`, we initially considered `MaxWinnersPerPage` as the overall maximum number of winners across pages. - However, it should be calculated as `Pages * MaxWinnersPerPage` to prevent the computed solution from having a low overall total of winners, which could result in a `WrongWinnerCount` error. + 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 From 19e9e9928e988c5d3ec153296580cccc9e65bc48 Mon Sep 17 00:00:00 2001 From: Paolo La Camera Date: Mon, 30 Jun 2025 11:35:18 +0200 Subject: [PATCH 7/7] Rename MaxWinnersTotal into MaxWinnersFinal for consistency --- .../election-provider-multi-block/src/unsigned/miner.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/election-provider-multi-block/src/unsigned/miner.rs b/substrate/frame/election-provider-multi-block/src/unsigned/miner.rs index bc305853f4c7b..6f425711e9ec0 100644 --- a/substrate/frame/election-provider-multi-block/src/unsigned/miner.rs +++ b/substrate/frame/election-provider-multi-block/src/unsigned/miner.rs @@ -222,9 +222,9 @@ pub type PageSupportsOfMiner = frame_election_provider_support::BoundedSuppor >; /// Helper type that computes the maximum total winners across all pages. -pub struct MaxWinnersTotal(core::marker::PhantomData); +pub struct MaxWinnersFinal(core::marker::PhantomData); -impl frame_support::traits::Get for MaxWinnersTotal { +impl frame_support::traits::Get for MaxWinnersFinal { fn get() -> u32 { T::Pages::get().saturating_mul(T::MaxWinnersPerPage::get()) } @@ -237,7 +237,7 @@ impl frame_support::traits::Get for MaxWinnersTotal { /// Another way to look at it, this is never wrapped in a `Vec<_>` pub type FullSupportsOfMiner = frame_election_provider_support::BoundedSupports< ::AccountId, - MaxWinnersTotal, + MaxWinnersFinal, ::MaxBackersPerWinnerFinal, >;