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

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<Score>)` 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

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));
 			<Pallet<T, I>>::update_member_count(self.member_count)
 				.expect("Number of allowed members exceeded");
 			<Pool<T, I>>::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::<T, I>::TooManyMembers)?;

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