EPMB/unsigned: fixed multi-page winner computation by sigurpol · Pull Request #8987 · paritytech/polkadot-sdk

sigurpol · 2025-06-25T21:20:36Z

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 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

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](paritytech/polkadot-staking-miner#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.

sigurpol · 2025-06-25T21:27:13Z

/cmd prdoc --audience runtime_dev --bump patch

…time_dev --bump patch'

prdoc/pr_8987.prdoc

seadanda · 2025-06-30T08:38:14Z

prdoc/pr_8987.prdoc

+    ## 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


Suggested change

## 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

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.

prdoc/pr_8987.prdoc

kianenigma · 2025-06-30T09:23:21Z

substrate/frame/election-provider-multi-block/src/unsigned/miner.rs

 >;

+/// Helper type that computes the maximum total winners across all pages.
+pub struct MaxWinnersTotal<T: MinerConfig>(core::marker::PhantomData<T>);


Elsewhere in the code, we have a similar naming dilemma:

MaxBackersPerWinner: page-local bound

MaxBackersPerWinnerFinal: bound across all pages

I am happy with either naming schemes (or even a new one) but I do suggest them to be consistent.

very fair point, let me use Final here as well

kianenigma · 2025-06-30T09:25:49Z

substrate/frame/election-provider-multi-block/src/unsigned/miner.rs

 	<T as MinerConfig>::AccountId,
-	<T as MinerConfig>::MaxWinnersPerPage,
+	MaxWinnersTotal<T>,
 	<T as MinerConfig>::MaxBackersPerWinnerFinal,


You can actually see the naming convention I mentioned here, noting the Final postfix.

renamed to use Final for consistency, ty

kianenigma

This is a very useful finding! would have been hard to catch later in prod.

The unfortunate note is that we will 99% use configurations in prod that will not trigger any trimming, but in the case of the 1%, it is hard to catch :)

I have a todo in the code about this, which you can now maybe remove and convert to an issue for us as well:

// TODO: we should have a fuzzer for miner that ensures no matter the parameters, it generates a
// valid solution. Esp. for the trimming.

What we did here was a small step towards this! But we can later it push it to the extreme: No matter what is in the snapshot, and what configurations are set in MinerConfig, it MUST produce something that is valid, or fail to produce a solution.

kianenigma · 2025-06-30T09:41:31Z

substrate/frame/election-provider-multi-block/src/unsigned/miner.rs

+				// The solution should still be valid despite aggressive trimming
+				assert!(solution.solution_pages.len() > 0);
+
+				let winner_count = solution


It is not a great name, but I found winner_count_single_page_target_snapshot.

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

yeah it is fine either way. Just noted that we have a helper fn that does the same.

sigurpol · 2025-06-30T09:43:37Z

This is a very useful finding! would have been hard to catch later in prod.

The unfortunate note is that we will 99% use configurations in prod that will not trigger any trimming, but in the case of the 1%, it is hard to catch :)

I have a todo in the code about this, which you can now maybe remove and convert to an issue for us as well:

// TODO: we should have a fuzzer for miner that ensures no matter the parameters, it generates a
// valid solution. Esp. for the trimming.

What we did here was a small step towards this! But we can later it push it to the extreme: No matter what is in the snapshot, and what configurations are set in MinerConfig, it MUST produce something that is valid, or fail to produce a solution.

Kept the TODO for the time being, and added #9038

## 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](paritytech/polkadot-staking-miner#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 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* master: EPMB/unsigned: fixed multi-page winner computation (#8987) Always send full parent header, not only hash, part of collation response (#8939)

* master: (91 commits) Add extra information to the harmless error logs during validate_transaction (#9047) `sp-tracing`: Remove `test-utils` feature (#9063) add try-state check for staking roles -- staker cannot be nominator a… (#9034) net/discovery: File persistence for `AddrCache` (#8839) dispute-coordinator: handle race with offchain disabling (#9050) Align parameters for `EventEmitter::emit_sent_event` (#9057) Fetch parent block `api_version` (#9059) [XCM Precompile] Rename functions and improve docs in the Solidity interface (#9023) Cleanup and improvements for `ControlledValidatorIndices` (#8896) reenable 0001-parachains-pvf (#9046) Add optional auto-rebag within on-idle (#8684) Fix flaxy 0003-block-building-warp-sync test - one more approach (#8974) [Staking] [AHM] Fixes insufficient slashing of nominators (and some other small issues). (#8937) chore: Bump bounded-collections dep (#9004) XCMP and DMP improvements (#8860) EPMB/unsigned: fixed multi-page winner computation (#8987) Always send full parent header, not only hash, part of collation response (#8939) revive: Precompiles should return dummy code when queried (#9001) Fix confusing log messages in network protocol behaviour (#8819) Fix pallet_migrations benchmark when FailedMigrationHandler emits events (#8694) ...

## 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](paritytech/polkadot-staking-miner#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 --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

sigurpol requested a review from a team as a code owner June 25, 2025 21:20

sigurpol added the T2-pallets This PR/Issue is related to a particular pallet. label Jun 25, 2025

Update from github-actions[bot] running command 'prdoc --audience run…

cb7de5d

…time_dev --bump patch'

sigurpol mentioned this pull request Jun 25, 2025

Miner tests for backer bounding paritytech/polkadot-staking-miner#1074

Closed

Fixed prdoc

daf0e62

sigurpol requested review from Ank4n, kianenigma and seadanda June 25, 2025 21:39

sigurpol added 3 commits June 26, 2025 08:40

Merge branch 'master' into sigurpol/epmb_fix_max_winner_total

a55bb36

Merge branch 'master' into sigurpol/epmb_fix_max_winner_total

7ca791b

Merge branch 'master' into sigurpol/epmb_fix_max_winner_total

9bb9e35

Ank4n approved these changes Jun 30, 2025

View reviewed changes

prdoc/pr_8987.prdoc Outdated Show resolved Hide resolved

Merge branch 'master' into sigurpol/epmb_fix_max_winner_total

5766ea0

seadanda approved these changes Jun 30, 2025

View reviewed changes

sigurpol added 2 commits June 30, 2025 10:45

Improve prdoc

e399119

Updated pr_doc

5ce6e1a

sigurpol enabled auto-merge June 30, 2025 08:50

Cleanup PRdoc

eb94c2d

kianenigma reviewed Jun 30, 2025

View reviewed changes

sigurpol disabled auto-merge June 30, 2025 09:26

kianenigma approved these changes Jun 30, 2025

View reviewed changes

Rename MaxWinnersTotal into MaxWinnersFinal for consistency

19e9e99

kianenigma reviewed Jun 30, 2025

View reviewed changes

sigurpol mentioned this pull request Jun 30, 2025

No matter what is in the snapshot, and what configurations are set in MinerConfig, the miner MUST produce something that is valid, or fail to produce a solution. #9038

Open

sigurpol added this pull request to the merge queue Jun 30, 2025

Merged via the queue into master with commit 75deaab Jun 30, 2025
246 checks passed

sigurpol deleted the sigurpol/epmb_fix_max_winner_total branch June 30, 2025 15:13

ordian added a commit that referenced this pull request Jul 1, 2025

Merge branch 'master' into ao-fix-disabling-race

7951995

* master: EPMB/unsigned: fixed multi-page winner computation (#8987) Always send full parent header, not only hash, part of collation response (#8939)

pgherveou added this to [preview] release tracker Mar 20, 2026

github-project-automation bot moved this to Done in [preview] release tracker Mar 20, 2026

pgherveou removed this from [preview] release tracker Mar 20, 2026

pgherveou added this to [preview] release tracker Mar 23, 2026

github-project-automation bot moved this to Done in [preview] release tracker Mar 23, 2026

Conversation

sigurpol commented Jun 25, 2025

The issue

How to test

Uh oh!

sigurpol commented Jun 25, 2025

Uh oh!

Uh oh!

seadanda Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kianenigma Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

sigurpol Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

sigurpol Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

kianenigma Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

sigurpol Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

kianenigma Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigurpol Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kianenigma Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

sigurpol commented Jun 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kianenigma Jun 30, 2025 •

edited

Loading

sigurpol Jun 30, 2025 •

edited

Loading