refactor election score (#10834)

* refactor election score

* Test for ord

* remove reference

* vec -> slice

* change iter to iter_by_significance

* improve doc

* fix typo

* add explanation about [u128; 3]

* consolidate threshold and epsilon

* random fixes

* rename

* remove Into

* make iter_by_sig private

* remove vec

* Fix tests

substrate/primitives/arithmetic/src/lib.rs

@@ -55,7 +55,7 @@ use traits::{BaseArithmetic, One, SaturatedConversion, Unsigned, Zero};
 /// - `Ordering::Equal` otherwise.
 pub trait ThresholdOrd<T> {
 	/// Compare if `self` is `threshold` greater or less than `other`.
-	fn tcmp(&self, other: &T, epsilon: T) -> Ordering;
+	fn tcmp(&self, other: &T, threshold: T) -> Ordering;
 }
 
 impl<T> ThresholdOrd<T> for T

substrate/primitives/npos-elections/fuzzer/src/phragmen_balancing.rs

@@ -23,8 +23,8 @@ use common::*;
 use honggfuzz::fuzz;
 use rand::{self, SeedableRng};
 use sp_npos_elections::{
-	assignment_ratio_to_staked_normalized, is_score_better, seq_phragmen, to_supports,
-	ElectionResult, EvaluateSupport, VoteWeight,
+	assignment_ratio_to_staked_normalized, seq_phragmen, to_supports, ElectionResult,
+	EvaluateSupport, VoteWeight,
 };
 use sp_runtime::Perbill;
 
@@ -60,7 +60,7 @@ fn main() {
 				.unwrap();
 				let score = to_supports(staked.as_ref()).evaluate();
 
-				if score[0] == 0 {
+				if score.minimal_stake == 0 {
 					// such cases cannot be improved by balancing.
 					return
 				}
@@ -80,7 +80,8 @@ fn main() {
 					to_supports(staked.as_ref()).evaluate()
 				};
 
-				let enhance = is_score_better(balanced_score, unbalanced_score, Perbill::zero());
+				let enhance =
+					balanced_score.strict_threshold_better(unbalanced_score, Perbill::zero());
 
 				println!(
 					"iter = {} // {:?} -> {:?} [{}]",
@@ -90,9 +91,9 @@ fn main() {
 				// The only guarantee of balancing is such that the first and third element of the
 				// score cannot decrease.
 				assert!(
-					balanced_score[0] >= unbalanced_score[0] &&
-						balanced_score[1] == unbalanced_score[1] &&
-						balanced_score[2] <= unbalanced_score[2]
+					balanced_score.minimal_stake >= unbalanced_score.minimal_stake &&
+						balanced_score.sum_stake == unbalanced_score.sum_stake &&
+						balanced_score.sum_stake_squared <= unbalanced_score.sum_stake_squared
 				);
 			}
 		});

substrate/primitives/npos-elections/fuzzer/src/phragmms_balancing.rs

@@ -23,8 +23,8 @@ use common::*;
 use honggfuzz::fuzz;
 use rand::{self, SeedableRng};
 use sp_npos_elections::{
-	assignment_ratio_to_staked_normalized, is_score_better, phragmms, to_supports, ElectionResult,
-	EvaluateSupport, VoteWeight,
+	assignment_ratio_to_staked_normalized, phragmms, to_supports, ElectionResult, EvaluateSupport,
+	VoteWeight,
 };
 use sp_runtime::Perbill;
 
@@ -60,7 +60,7 @@ fn main() {
 				.unwrap();
 				let score = to_supports(&staked).evaluate();
 
-				if score[0] == 0 {
+				if score.minimal_stake == 0 {
 					// such cases cannot be improved by balancing.
 					return
 				}
@@ -77,7 +77,7 @@ fn main() {
 				to_supports(staked.as_ref()).evaluate()
 			};
 
-			let enhance = is_score_better(balanced_score, unbalanced_score, Perbill::zero());
+			let enhance = balanced_score.strict_threshold_better(unbalanced_score, Perbill::zero());
 
 			println!(
 				"iter = {} // {:?} -> {:?} [{}]",
@@ -87,9 +87,9 @@ fn main() {
 			// The only guarantee of balancing is such that the first and third element of the score
 			// cannot decrease.
 			assert!(
-				balanced_score[0] >= unbalanced_score[0] &&
-					balanced_score[1] == unbalanced_score[1] &&
-					balanced_score[2] <= unbalanced_score[2]
+				balanced_score.minimal_stake >= unbalanced_score.minimal_stake &&
+					balanced_score.sum_stake == unbalanced_score.sum_stake &&
+					balanced_score.sum_stake_squared <= unbalanced_score.sum_stake_squared
 			);
 		});
 	}

substrate/primitives/npos-elections/src/lib.rs

@@ -74,11 +74,12 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
+use scale_info::TypeInfo;
 use sp_arithmetic::{traits::Zero, Normalizable, PerThing, Rational128, ThresholdOrd};
 use sp_core::RuntimeDebug;
 use sp_std::{cell::RefCell, cmp::Ordering, collections::btree_map::BTreeMap, prelude::*, rc::Rc};
 
-use codec::{Decode, Encode};
+use codec::{Decode, Encode, MaxEncodedLen};
 #[cfg(feature = "std")]
 use serde::{Deserialize, Serialize};
 
@@ -144,9 +145,86 @@ pub type VoteWeight = u64;
 /// A type in which performing operations on vote weights are safe.
 pub type ExtendedBalance = u128;
 
-/// The score of an assignment. This can be computed from the support map via
-/// [`EvaluateSupport::evaluate`].
-pub type ElectionScore = [ExtendedBalance; 3];
+/// The score of an election. This is the main measure of an election's quality.
+///
+/// By definition, the order of significance in [`ElectionScore`] is:
+///
+/// 1. `minimal_stake`.
+/// 2. `sum_stake`.
+/// 3. `sum_stake_squared`.
+#[derive(Clone, Copy, PartialEq, Eq, Encode, Decode, MaxEncodedLen, TypeInfo, Debug, Default)]
+#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
+pub struct ElectionScore {
+	/// The minimal winner, in terms of total backing stake.
+	///
+	/// This parameter should be maximized.
+	pub minimal_stake: ExtendedBalance,
+	/// The sum of the total backing of all winners.
+	///
+	/// This parameter should maximized
+	pub sum_stake: ExtendedBalance,
+	/// The sum squared of the total backing of all winners, aka. the variance.
+	///
+	/// Ths parameter should be minimized.
+	pub sum_stake_squared: ExtendedBalance,
+}
+
+impl ElectionScore {
+	/// Iterate over the inner items, first visiting the most significant one.
+	fn iter_by_significance(self) -> impl Iterator<Item = ExtendedBalance> {
+		[self.minimal_stake, self.sum_stake, self.sum_stake_squared].into_iter()
+	}
+
+	/// Compares two sets of election scores based on desirability, returning true if `self` is
+	/// strictly `threshold` better than `other`. In other words, each element of `self` must be
+	/// `self * threshold` better than `other`.
+	///
+	/// Evaluation is done based on the order of significance of the fields of [`ElectionScore`].
+	pub fn strict_threshold_better(self, other: Self, threshold: impl PerThing) -> bool {
+		match self
+			.iter_by_significance()
+			.zip(other.iter_by_significance())
+			.map(|(this, that)| (this.ge(&that), this.tcmp(&that, threshold.mul_ceil(that))))
+			.collect::<Vec<(bool, Ordering)>>()
+			.as_slice()
+		{
+			// threshold better in the `score.minimal_stake`, accept.
+			[(x, Ordering::Greater), _, _] => {
+				debug_assert!(x);
+				true
+			},
+
+			// less than threshold better in `score.minimal_stake`, but more than threshold better
+			// in `score.sum_stake`.
+			[(true, Ordering::Equal), (_, Ordering::Greater), _] => true,
+
+			// less than threshold better in `score.minimal_stake` and `score.sum_stake`, but more
+			// than threshold better in `score.sum_stake_squared`.
+			[(true, Ordering::Equal), (true, Ordering::Equal), (_, Ordering::Less)] => true,
+
+			// anything else is not a good score.
+			_ => false,
+		}
+	}
+}
+
+impl sp_std::cmp::Ord for ElectionScore {
+	fn cmp(&self, other: &Self) -> Ordering {
+		// we delegate this to the lexicographic cmp of slices`, and to incorporate that we want the
+		// third element to be minimized, we swap them.
+		[self.minimal_stake, self.sum_stake, other.sum_stake_squared].cmp(&[
+			other.minimal_stake,
+			other.sum_stake,
+			self.sum_stake_squared,
+		])
+	}
+}
+
+impl sp_std::cmp::PartialOrd for ElectionScore {
+	fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+		Some(self.cmp(other))
+	}
+}
 
 /// A pointer to a candidate struct with interior mutability.
 pub type CandidatePtr<A> = Rc<RefCell<Candidate<A>>>;
@@ -353,7 +431,7 @@ pub struct ElectionResult<AccountId, P: PerThing> {
 ///
 /// This, at the current version, resembles the `Exposure` defined in the Staking pallet, yet they
 /// do not necessarily have to be the same.
-#[derive(RuntimeDebug, Encode, Decode, Clone, Eq, PartialEq, scale_info::TypeInfo)]
+#[derive(RuntimeDebug, Encode, Decode, Clone, Eq, PartialEq, TypeInfo)]
 #[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
 pub struct Support<AccountId> {
 	/// Total support.
@@ -418,49 +496,22 @@ pub trait EvaluateSupport {
 
 impl<AccountId: IdentifierT> EvaluateSupport for Supports<AccountId> {
 	fn evaluate(&self) -> ElectionScore {
-		let mut min_support = ExtendedBalance::max_value();
-		let mut sum: ExtendedBalance = Zero::zero();
+		let mut minimal_stake = ExtendedBalance::max_value();
+		let mut sum_stake: ExtendedBalance = Zero::zero();
 		// NOTE: The third element might saturate but fine for now since this will run on-chain and
 		// need to be fast.
-		let mut sum_squared: ExtendedBalance = Zero::zero();
+		let mut sum_stake_squared: ExtendedBalance = Zero::zero();
+
 		for (_, support) in self {
-			sum = sum.saturating_add(support.total);
+			sum_stake = sum_stake.saturating_add(support.total);
 			let squared = support.total.saturating_mul(support.total);
-			sum_squared = sum_squared.saturating_add(squared);
-			if support.total < min_support {
-				min_support = support.total;
+			sum_stake_squared = sum_stake_squared.saturating_add(squared);
+			if support.total < minimal_stake {
+				minimal_stake = support.total;
 			}
 		}
-		[min_support, sum, sum_squared]
-	}
-}
 
-/// Compares two sets of election scores based on desirability and returns true if `this` is better
-/// than `that`.
-///
-/// Evaluation is done in a lexicographic manner, and if each element of `this` is `that * epsilon`
-/// greater or less than `that`.
-///
-/// Note that the third component should be minimized.
-pub fn is_score_better<P: PerThing>(this: ElectionScore, that: ElectionScore, epsilon: P) -> bool {
-	match this
-		.iter()
-		.zip(that.iter())
-		.map(|(thi, tha)| (thi.ge(&tha), thi.tcmp(&tha, epsilon.mul_ceil(*tha))))
-		.collect::<Vec<(bool, Ordering)>>()
-		.as_slice()
-	{
-		// epsilon better in the score[0], accept.
-		[(_, Ordering::Greater), _, _] => true,
-
-		// less than epsilon better in score[0], but more than epsilon better in the second.
-		[(true, Ordering::Equal), (_, Ordering::Greater), _] => true,
-
-		// less than epsilon better in score[0, 1], but more than epsilon better in the third
-		[(true, Ordering::Equal), (true, Ordering::Equal), (_, Ordering::Less)] => true,
-
-		// anything else is not a good score.
-		_ => false,
+		ElectionScore { minimal_stake, sum_stake, sum_stake_squared }
 	}
 }
 

substrate/primitives/npos-elections/src/tests.rs

@@ -18,9 +18,9 @@
 //! Tests for npos-elections.
 
 use crate::{
-	balancing, helpers::*, is_score_better, mock::*, seq_phragmen, seq_phragmen_core, setup_inputs,
-	to_support_map, Assignment, ElectionResult, ExtendedBalance, IndexAssignment, NposSolution,
-	StakedAssignment, Support, Voter,
+	balancing, helpers::*, mock::*, seq_phragmen, seq_phragmen_core, setup_inputs, to_support_map,
+	Assignment, ElectionResult, ExtendedBalance, IndexAssignment, NposSolution, StakedAssignment,
+	Support, Voter,
 };
 use rand::{self, SeedableRng};
 use sp_arithmetic::{PerU16, Perbill, Percent, Permill};
@@ -792,6 +792,21 @@ mod assignment_convert_normalize {
 
 mod score {
 	use super::*;
+	use crate::ElectionScore;
+	use sp_arithmetic::PerThing;
+
+	/// NOTE: in tests, we still use the legacy [u128; 3] since it is more compact. Each `u128`
+	/// corresponds to element at the respective field index of `ElectionScore`.
+	impl From<[ExtendedBalance; 3]> for ElectionScore {
+		fn from(t: [ExtendedBalance; 3]) -> Self {
+			Self { minimal_stake: t[0], sum_stake: t[1], sum_stake_squared: t[2] }
+		}
+	}
+
+	fn is_score_better(this: [u128; 3], that: [u128; 3], p: impl PerThing) -> bool {
+		ElectionScore::from(this).strict_threshold_better(ElectionScore::from(that), p)
+	}
+
 	#[test]
 	fn score_comparison_is_lexicographical_no_epsilon() {
 		let epsilon = Perbill::zero();
@@ -883,6 +898,26 @@ mod score {
 			false,
 		);
 	}
+
+	#[test]
+	fn ord_works() {
+		// equal only when all elements are equal
+		assert!(ElectionScore::from([10, 5, 15]) == ElectionScore::from([10, 5, 15]));
+		assert!(ElectionScore::from([10, 5, 15]) != ElectionScore::from([9, 5, 15]));
+		assert!(ElectionScore::from([10, 5, 15]) != ElectionScore::from([10, 5, 14]));
+
+		// first element greater, rest don't matter
+		assert!(ElectionScore::from([10, 5, 15]) > ElectionScore::from([8, 5, 25]));
+		assert!(ElectionScore::from([10, 5, 15]) > ElectionScore::from([9, 20, 5]));
+
+		// second element greater, rest don't matter
+		assert!(ElectionScore::from([10, 5, 15]) > ElectionScore::from([10, 4, 25]));
+		assert!(ElectionScore::from([10, 5, 15]) > ElectionScore::from([10, 4, 5]));
+
+		// second element is less, rest don't matter. Note that this is swapped.
+		assert!(ElectionScore::from([10, 5, 15]) > ElectionScore::from([10, 5, 16]));
+		assert!(ElectionScore::from([10, 5, 15]) > ElectionScore::from([10, 5, 25]));
+	}
 }
 
 mod solution_type {