diff --git a/prdoc/pr_12333.prdoc b/prdoc/pr_12333.prdoc new file mode 100644 index 000000000000..af8d4618a350 --- /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` places `None` before `Some(_)` (`None` < `Some(_)`). + + **Test:** added `scoring_to_zero_must_sort_before_none_entries` covering this case. +crates: +- name: pallet-scored-pool + bump: patch diff --git a/substrate/frame/scored-pool/src/lib.rs b/substrate/frame/scored-pool/src/lib.rs index db22c3b1ce75..327363f2d4e2 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` 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. 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(|| {