From 1f35f3932718421b5af1944cf64885cc685d2a01 Mon Sep 17 00:00:00 2001 From: Nnamdi Aninye Date: Wed, 10 Jun 2026 17:27:27 +0100 Subject: [PATCH 1/6] fix invariant bug --- substrate/frame/scored-pool/src/lib.rs | 11 +++++++---- substrate/frame/scored-pool/src/tests.rs | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/substrate/frame/scored-pool/src/lib.rs b/substrate/frame/scored-pool/src/lib.rs index db22c3b1ce75..cad167ca3585 100644 --- a/substrate/frame/scored-pool/src/lib.rs +++ b/substrate/frame/scored-pool/src/lib.rs @@ -276,7 +276,7 @@ pub mod pallet { // Sorts the `Pool` by score in a descending order. Entities which // have a score of `None` are sorted to the end of the bounded vec. - pool.sort_by_key(|(_, maybe_score)| Reverse(maybe_score.unwrap_or_default())); + pool.sort_by_key(|(_, maybe_score)| Reverse(*maybe_score)); >::update_member_count(self.member_count) .expect("Number of allowed members exceeded"); >::put(&pool); @@ -401,14 +401,17 @@ pub mod pallet { pool.remove(index as usize); - // we binary search the pool (which is sorted descending by score). + // 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. // if there is already an element with `score`, we insert // right before that. if not, the search returns a location // where we can insert while maintaining order. let item = (who, Some(score)); let location = pool - .binary_search_by_key(&Reverse(score), |(_, maybe_score)| { - Reverse(maybe_score.unwrap_or_default()) + .binary_search_by_key(&Reverse(Some(score)), |(_, maybe_score)| { + Reverse(*maybe_score) }) .unwrap_or_else(|l| l); pool.try_insert(location, item).map_err(|_| Error::::TooManyMembers)?; diff --git a/substrate/frame/scored-pool/src/tests.rs b/substrate/frame/scored-pool/src/tests.rs index 96c94a6c1c65..2d29f5e5ee53 100644 --- a/substrate/frame/scored-pool/src/tests.rs +++ b/substrate/frame/scored-pool/src/tests.rs @@ -104,6 +104,26 @@ fn scoring_same_element_with_same_score_works() { }); } +#[test] +fn scoring_to_zero_must_sort_before_none_entries() { + new_test_ext().execute_with(|| { + // given a pool with multiple unscored (`None`) candidates after `5` + assert_ok!(ScoredPool::submit_candidacy(RuntimeOrigin::signed(15))); + assert_ok!(ScoredPool::submit_candidacy(RuntimeOrigin::signed(16))); + let who = 5; + let index = find_in_pool(who).expect("entity must be in pool") as u32; + + // when the unscored `who` is scored to `0` + assert_ok!(ScoredPool::score(RuntimeOrigin::signed(ScoreOrigin::get()), who, index, 0)); + + // then `(who, Some(0))` must be placed before all `None` entries, preserving the + // "ordered descending by score, `None` last" invariant. + assert_eq!(fetch_from_pool(who), Some((who, Some(0)))); + assert!(find_in_pool(who).unwrap() < find_in_pool(15).unwrap()); + assert!(find_in_pool(who).unwrap() < find_in_pool(16).unwrap()); + }); +} + #[test] fn kicking_works_only_for_authorized() { new_test_ext().execute_with(|| { From ef914d2fc579d7d04766c8de9f9d0002ac53b891 Mon Sep 17 00:00:00 2001 From: Nnamdi Aninye Date: Wed, 10 Jun 2026 17:31:58 +0100 Subject: [PATCH 2/6] undo update to test --- substrate/frame/scored-pool/src/lib.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/substrate/frame/scored-pool/src/lib.rs b/substrate/frame/scored-pool/src/lib.rs index cad167ca3585..db22c3b1ce75 100644 --- a/substrate/frame/scored-pool/src/lib.rs +++ b/substrate/frame/scored-pool/src/lib.rs @@ -276,7 +276,7 @@ pub mod pallet { // Sorts the `Pool` by score in a descending order. Entities which // have a score of `None` are sorted to the end of the bounded vec. - pool.sort_by_key(|(_, maybe_score)| Reverse(*maybe_score)); + pool.sort_by_key(|(_, maybe_score)| Reverse(maybe_score.unwrap_or_default())); >::update_member_count(self.member_count) .expect("Number of allowed members exceeded"); >::put(&pool); @@ -401,17 +401,14 @@ pub mod pallet { pool.remove(index as usize); - // 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. + // we binary search the pool (which is sorted descending by score). // if there is already an element with `score`, we insert // right before that. if not, the search returns a location // where we can insert while maintaining order. let item = (who, Some(score)); let location = pool - .binary_search_by_key(&Reverse(Some(score)), |(_, maybe_score)| { - Reverse(*maybe_score) + .binary_search_by_key(&Reverse(score), |(_, maybe_score)| { + Reverse(maybe_score.unwrap_or_default()) }) .unwrap_or_else(|l| l); pool.try_insert(location, item).map_err(|_| Error::::TooManyMembers)?; From 42e5a295f81e02523fba70b867c1f2ebbfec9063 Mon Sep 17 00:00:00 2001 From: Nnamdi Aninye Date: Wed, 10 Jun 2026 17:34:21 +0100 Subject: [PATCH 3/6] redo --- substrate/frame/scored-pool/src/lib.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/substrate/frame/scored-pool/src/lib.rs b/substrate/frame/scored-pool/src/lib.rs index db22c3b1ce75..cad167ca3585 100644 --- a/substrate/frame/scored-pool/src/lib.rs +++ b/substrate/frame/scored-pool/src/lib.rs @@ -276,7 +276,7 @@ pub mod pallet { // Sorts the `Pool` by score in a descending order. Entities which // have a score of `None` are sorted to the end of the bounded vec. - pool.sort_by_key(|(_, maybe_score)| Reverse(maybe_score.unwrap_or_default())); + pool.sort_by_key(|(_, maybe_score)| Reverse(*maybe_score)); >::update_member_count(self.member_count) .expect("Number of allowed members exceeded"); >::put(&pool); @@ -401,14 +401,17 @@ pub mod pallet { pool.remove(index as usize); - // we binary search the pool (which is sorted descending by score). + // 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. // if there is already an element with `score`, we insert // right before that. if not, the search returns a location // where we can insert while maintaining order. let item = (who, Some(score)); let location = pool - .binary_search_by_key(&Reverse(score), |(_, maybe_score)| { - Reverse(maybe_score.unwrap_or_default()) + .binary_search_by_key(&Reverse(Some(score)), |(_, maybe_score)| { + Reverse(*maybe_score) }) .unwrap_or_else(|l| l); pool.try_insert(location, item).map_err(|_| Error::::TooManyMembers)?; From 5d6d424fa462ec2f613d84da2c6b55eddfc3b484 Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 10 Jun 2026 16:51:51 +0000 Subject: [PATCH 4/6] Update from github-actions[bot] running command 'prdoc --audience runtime_dev --bump patch' --- prdoc/pr_12333.prdoc | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 prdoc/pr_12333.prdoc diff --git a/prdoc/pr_12333.prdoc b/prdoc/pr_12333.prdoc new file mode 100644 index 000000000000..07b0ff006faf --- /dev/null +++ b/prdoc/pr_12333.prdoc @@ -0,0 +1,16 @@ +title: '[RWF] pallet-scored-pool: fix pool sort logic' +doc: +- audience: Runtime Dev + description: |- + 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)` instead (`Reverse(*maybe_score)` / `Reverse(Some(score))`), since `Option`'s `Ord` already places `None` after all `Some(_)`. + + **Test:** added `scoring_to_zero_must_sort_before_none_entries` covering this case. +crates: +- name: pallet-scored-pool + bump: patch From 69d8ebdee3222f70c4e2e4ff4c8df65c48b9e8e0 Mon Sep 17 00:00:00 2001 From: Nnamdi Aninye Date: Thu, 11 Jun 2026 08:10:08 +0100 Subject: [PATCH 5/6] fix comment description --- prdoc/pr_12333.prdoc | 2 +- substrate/frame/scored-pool/src/lib.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/prdoc/pr_12333.prdoc b/prdoc/pr_12333.prdoc index 07b0ff006faf..af8d4618a350 100644 --- a/prdoc/pr_12333.prdoc +++ b/prdoc/pr_12333.prdoc @@ -8,7 +8,7 @@ doc: `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)` instead (`Reverse(*maybe_score)` / `Reverse(Some(score))`), since `Option`'s `Ord` already places `None` after all `Some(_)`. + **Fix:** key on `Reverse(Option)` instead (`Reverse(*maybe_score)` / `Reverse(Some(score))`), since `Option`'s `Ord` places `None` before `Some(_)` (`None` < `Some(_)`). **Test:** added `scoring_to_zero_must_sort_before_none_entries` covering this case. crates: diff --git a/substrate/frame/scored-pool/src/lib.rs b/substrate/frame/scored-pool/src/lib.rs index cad167ca3585..498b14f29f92 100644 --- a/substrate/frame/scored-pool/src/lib.rs +++ b/substrate/frame/scored-pool/src/lib.rs @@ -402,9 +402,9 @@ pub mod pallet { pool.remove(index as usize); // 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. + // last). `Option`'s `Ord` impl places `None` before `Some(_)` (None < Some(_)) + // regardless of the scored value, so wrapping in `Reverse` keeps `None` entries + // last while sorting `Some(_)` entries by descending score. // if there is already an element with `score`, we insert // right before that. if not, the search returns a location // where we can insert while maintaining order. From 796d4dc7b988e57f67b695e306112fce14264ede Mon Sep 17 00:00:00 2001 From: "cmd[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 23 Jul 2026 08:53:16 +0000 Subject: [PATCH 6/6] Update from github-actions[bot] running command 'fmt' --- substrate/frame/scored-pool/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/scored-pool/src/lib.rs b/substrate/frame/scored-pool/src/lib.rs index 498b14f29f92..327363f2d4e2 100644 --- a/substrate/frame/scored-pool/src/lib.rs +++ b/substrate/frame/scored-pool/src/lib.rs @@ -402,8 +402,8 @@ pub mod pallet { pool.remove(index as usize); // we binary search the pool (which is sorted descending by score, with `None` - // last). `Option`'s `Ord` impl places `None` before `Some(_)` (None < Some(_)) - // regardless of the scored value, so wrapping in `Reverse` keeps `None` entries + // last). `Option`'s `Ord` impl places `None` before `Some(_)` (None < Some(_)) + // regardless of the scored value, so wrapping in `Reverse` keeps `None` entries // last while sorting `Some(_)` entries by descending score. // if there is already an element with `score`, we insert // right before that. if not, the search returns a location