[RWF] pallet-scored-pool: fix pool sort logic

[naijauser]

Closes #12259.

Summary

score() used Reverse(maybe_score.unwrap_or_default()) as its binary-search key, which maps both None and Some(0) to Reverse(0). Rescoring a candidate to 0 could then insert it after existing None entries, violating the documented "descending by score, None last" invariant. Same issue existed in the genesis pool.sort_by_key.

Fix: key on Reverse(Option<Score>) instead (Reverse(*maybe_score) / Reverse(Some(score))), since Option's Ord places None before all Some(_) (None < Some(_)).

Test: added scoring_to_zero_must_sort_before_none_entries covering this case.

[naijauser]

/cmd prdoc --audience runtime_dev --bump patch

[github-actions]

Command "prdoc --audience runtime_dev --bump patch" has failed ❌! See logs here

[naijauser]

/cmd prdoc --audience runtime_dev --bump patch

[naijauser]

@xlc, please review.

[xlc]

Code change looks right. One wording nit here

polkadot-sdk/substrate/frame/scored-pool/src/lib.rs

Lines 404 to 407 in 5d6d424

	// we binary search the pool (which is sorted descending by score, with `None`
	// last). `Option`'s `Ord` impl places `None` after `Some(_)` regardless of the
	// scored value, so wrapping in `Reverse` keeps `None` entries last while sorting
	// `Some(_)` entries by descending score.

natural Option order is None < Some(_); Reverse<Option<_>> is what makes None end up last. Sorting fix itself looks good.

[naijauser]

Code change looks right. One wording nit here

polkadot-sdk/substrate/frame/scored-pool/src/lib.rs

Lines 404 to 407 in 5d6d424

	// we binary search the pool (which is sorted descending by score, with `None`
	// last). `Option`'s `Ord` impl places `None` after `Some(_)` regardless of the
	// scored value, so wrapping in `Reverse` keeps `None` entries last while sorting
	// `Some(_)` entries by descending score.

natural Option order is None < Some(_); Reverse<Option<_>> is what makes None end up last. Sorting fix itself looks good.

Oops! You're right. The comment had it the other way round.

I've updated the code comment, the prdoc, and the PR description.

[ggwpez]

Would need to check if this causes issues with the current staking system, if that pallet is still used.

[Ank4n]

Staking does not interact with this pallet.

[Ank4n]

This pallet doesn't seem to be used by any of our runtimes. Any reason to keep it, or should we deprecate and remove?

[naijauser]

/cmd fmt

[naijauser]

/cmd fmt

[sigurpol]

LGTM - +1 on Ankan's question (do we need to keep this pallet around? ) but that's a separate discussion

[Ank4n]

Following up on my question above — I've opened #12746 to remove this pallet.

Rationale: it isn't used by any runtime in the repo, has had no functional changes in years (only mechanical maintenance touches), and the membership/ranking use cases are covered by pallet-membership / pallet-ranked-collective / pallet-core-fellowship.

@naijauser sorry for the wasted effort here — the bug you found is real, and it's part of why the removal came up. If reviewers on #12746 decide to keep the pallet (or to follow the deprecate-then-remove path in the deprecation checklist first), then this PR should go in and I'll rebase mine on top. Let's hold this one until #12746 is resolved either way.

[naijauser]

Following up on my question above — I've opened #12746 to remove this pallet.

Rationale: it isn't used by any runtime in the repo, has had no functional changes in years (only mechanical maintenance touches), and the membership/ranking use cases are covered by pallet-membership / pallet-ranked-collective / pallet-core-fellowship.

@naijauser sorry for the wasted effort here — the bug you found is real, and it's part of why the removal came up. If reviewers on #12746 decide to keep the pallet (or to follow the deprecate-then-remove path in the deprecation checklist first), then this PR should go in and I'll rebase mine on top. Let's hold this one until #12746 is resolved either way.

No worries, thanks.