Accept new Phragmén solutions if they are epsilon better + Better pre-inclusion checks. (#6173)

* part1: Accept inly epsilon better solutions

* Fix pre-dispatch check

* Fix build

* review grumbles

* Epsilon -> Threshold

substrate/bin/node/runtime/src/lib.rs

@@ -320,9 +320,11 @@ parameter_types! {
 	pub const BondingDuration: pallet_staking::EraIndex = 24 * 28;
 	pub const SlashDeferDuration: pallet_staking::EraIndex = 24 * 7; // 1/4 the bonding duration.
 	pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE;
-	pub const ElectionLookahead: BlockNumber = EPOCH_DURATION_IN_BLOCKS / 4;
 	pub const MaxNominatorRewardedPerValidator: u32 = 64;
-	pub const MaxIterations: u32 = 5;
+	pub const ElectionLookahead: BlockNumber = EPOCH_DURATION_IN_BLOCKS / 4;
+	pub const MaxIterations: u32 = 10;
+	// 0.05%. The higher the value, the more strict solution acceptance becomes.
+	pub MinSolutionScoreBump: Perbill = Perbill::from_rational_approximation(5u32, 10_000);
 }
 
 impl pallet_staking::Trait for Runtime {
@@ -344,6 +346,7 @@ impl pallet_staking::Trait for Runtime {
 	type ElectionLookahead = ElectionLookahead;
 	type Call = Call;
 	type MaxIterations = MaxIterations;
+	type MinSolutionScoreBump = MinSolutionScoreBump;
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
 	type UnsignedPriority = StakingUnsignedPriority;
 }

substrate/frame/grandpa/src/mock.rs

@@ -229,6 +229,7 @@ impl staking::Trait for Test {
 	type Call = Call;
 	type UnsignedPriority = StakingUnsignedPriority;
 	type MaxIterations = ();
+	type MinSolutionScoreBump = ();
 }
 
 parameter_types! {

substrate/frame/offences/benchmarking/src/mock.rs

@@ -176,6 +176,7 @@ impl pallet_staking::Trait for Test {
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
 	type UnsignedPriority = ();
 	type MaxIterations = ();
+	type MinSolutionScoreBump = ();
 }
 
 impl pallet_im_online::Trait for Test {

substrate/frame/session/benchmarking/src/mock.rs

@@ -183,6 +183,7 @@ impl pallet_staking::Trait for Test {
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
 	type UnsignedPriority = UnsignedPriority;
 	type MaxIterations = ();
+	type MinSolutionScoreBump = ();
 }
 
 impl crate::Trait for Test {}

substrate/frame/staking/fuzzer/src/mock.rs

@@ -185,6 +185,7 @@ impl pallet_staking::Trait for Test {
 	type ElectionLookahead = ();
 	type Call = Call;
 	type MaxIterations = MaxIterations;
+	type MinSolutionScoreBump = ();
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
 	type UnsignedPriority = ();
 }

substrate/frame/staking/src/lib.rs

@@ -293,7 +293,10 @@ use frame_support::{
 	decl_module, decl_event, decl_storage, ensure, decl_error,
 	weights::{Weight, constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}},
 	storage::IterableStorageMap,
-	dispatch::{IsSubType, DispatchResult, DispatchResultWithPostInfo, WithPostDispatchInfo},
+	dispatch::{
+		IsSubType, DispatchResult, DispatchResultWithPostInfo, DispatchErrorWithPostInfo,
+		WithPostDispatchInfo,
+	},
 	traits::{
 		Currency, LockIdentifier, LockableCurrency, WithdrawReasons, OnUnbalanced, Imbalance, Get,
 		UnixTime, EstimateNextNewSession, EnsureOrigin,
@@ -301,7 +304,7 @@ use frame_support::{
 };
 use pallet_session::historical;
 use sp_runtime::{
-	Perbill, PerU16, PerThing, RuntimeDebug,
+	Perbill, PerU16, PerThing, RuntimeDebug, DispatchError,
 	curve::PiecewiseLinear,
 	traits::{
 		Convert, Zero, StaticLookup, CheckedSub, Saturating, SaturatedConversion, AtLeast32Bit,
@@ -891,6 +894,9 @@ pub trait Trait: frame_system::Trait + SendTransactionTypes<Call<Self>> {
 	/// equalize will not be executed at all.
 	type MaxIterations: Get<u32>;
 
+	/// The threshold of improvement that should be provided for a new solution to be accepted.
+	type MinSolutionScoreBump: Get<Perbill>;
+
 	/// The maximum number of nominator rewarded for each validator.
 	///
 	/// For each validator only the `$MaxNominatorRewardedPerValidator` biggest stakers can claim
@@ -2614,7 +2620,7 @@ impl<T: Trait> Module<T> {
 		// assume the given score is valid. Is it better than what we have on-chain, if we have any?
 		if let Some(queued_score) = Self::queued_score() {
 			ensure!(
-				is_score_better(queued_score, score),
+				is_score_better(score, queued_score, T::MinSolutionScoreBump::get()),
 				Error::<T>::PhragmenWeakSubmission.with_weight(T::DbWeight::get().reads(3)),
 			)
 		}
@@ -3506,7 +3512,6 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 			_,
 		) = call {
 			use offchain_election::DEFAULT_LONGEVITY;
-			use sp_runtime::DispatchError;
 
 			// discard solution not coming from the local OCW.
 			match source {
@@ -3518,17 +3523,13 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 			}
 
 			if let Err(error_with_post_info) = Self::pre_dispatch_checks(*score, *era) {
-				let error = error_with_post_info.error;
+				let invalid = to_invalid(error_with_post_info);
-				let error_number = match error {
-					DispatchError::Module { error, ..} => error,
-					_ => 0,
-				};
 				log!(
 					debug,
-					"validate unsigned pre dispatch checks failed due to module error #{:?}.",
+					"validate unsigned pre dispatch checks failed due to error #{:?}.",
-					error,
+					invalid,
 				);
-				return InvalidTransaction::Custom(error_number).into();
+				return invalid .into();
 			}
 
 			log!(debug, "validateUnsigned succeeded for a solution at era {}.", era);
@@ -3556,13 +3557,28 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 		}
 	}
 
-	fn pre_dispatch(_: &Self::Call) -> Result<(), TransactionValidityError> {
+	fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> {
-		// IMPORTANT NOTE: By default, a sane `pre-dispatch` should always do the same checks as
+		if let Call::submit_election_solution_unsigned(
-		// `validate_unsigned` and overriding this should be done with care. this module has only
+			_,
-		// one unsigned entry point, in which we call into `<Module<T>>::pre_dispatch_checks()`
+			_,
-		// which is all the important checks that we do in `validate_unsigned`. Hence, we can safely
+			score,
-		// override this to save some time.
+			era,
-		Ok(())
+			_,
+		) = call {
+			// IMPORTANT NOTE: These checks are performed in the dispatch call itself, yet we need
+			// to duplicate them here to prevent a block producer from putting a previously
+			// validated, yet no longer valid solution on chain.
+			// OPTIMISATION NOTE: we could skip this in the `submit_election_solution_unsigned`
+			// since we already do it here. The signed version needs it though. Yer for now we keep
+			// this duplicate check here so both signed and unsigned can use a singular
+			// `check_and_replace_solution`.
+			Self::pre_dispatch_checks(*score, *era)
+				.map(|_| ())
+				.map_err(to_invalid)
+				.map_err(Into::into)
+		} else {
+			Err(InvalidTransaction::Call.into())
+		}
 	}
 }
 
@@ -3570,3 +3586,14 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 fn is_sorted_and_unique(list: &[u32]) -> bool {
 	list.windows(2).all(|w| w[0] < w[1])
 }
+
+/// convert a DispatchErrorWithPostInfo to a custom InvalidTransaction with the inner code being the
+/// error number.
+fn to_invalid(error_with_post_info: DispatchErrorWithPostInfo) -> InvalidTransaction {
+	let error = error_with_post_info.error;
+	let error_number = match error {
+		DispatchError::Module { error, ..} => error,
+		_ => 0,
+	};
+	InvalidTransaction::Custom(error_number)
+}

substrate/frame/staking/src/mock.rs

@@ -286,6 +286,7 @@ parameter_types! {
 	pub const RewardCurve: &'static PiecewiseLinear<'static> = &I_NPOS;
 	pub const MaxNominatorRewardedPerValidator: u32 = 64;
 	pub const UnsignedPriority: u64 = 1 << 20;
+	pub const MinSolutionScoreBump: Perbill = Perbill::zero();
 }
 
 thread_local! {
@@ -321,6 +322,7 @@ impl Trait for Test {
 	type ElectionLookahead = ElectionLookahead;
 	type Call = Call;
 	type MaxIterations = MaxIterations;
+	type MinSolutionScoreBump = MinSolutionScoreBump;
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
 	type UnsignedPriority = UnsignedPriority;
 }
@@ -853,7 +855,11 @@ pub(crate) fn horrible_phragmen_with_post_processing(
 		let support = build_support_map::<AccountId>(&winners, &staked_assignment).0;
 		let score = evaluate_support(&support);
 
-		assert!(sp_phragmen::is_score_better(score, better_score));
+		assert!(sp_phragmen::is_score_better::<Perbill>(
+			better_score,
+			score,
+			MinSolutionScoreBump::get(),
+		));
 
 		score
 	};

substrate/primitives/arithmetic/src/lib.rs

@@ -41,12 +41,89 @@ mod fixed;
 mod rational128;
 
 pub use fixed::{FixedPointNumber, Fixed64, Fixed128, FixedPointOperand};
-pub use per_things::{PerThing, Percent, PerU16, Permill, Perbill, Perquintill};
+pub use per_things::{PerThing, InnerOf, Percent, PerU16, Permill, Perbill, Perquintill};
 pub use rational128::Rational128;
 
+use sp_std::cmp::Ordering;
+
+/// Trait for comparing two numbers with an threshold.
+///
+/// Returns:
+/// - `Ordering::Greater` if `self` is greater than `other + threshold`.
+/// - `Ordering::Less` if `self` is less than `other - threshold`.
+/// - `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;
+}
+
+impl<T> ThresholdOrd<T> for T
+where
+	T: Ord + PartialOrd + Copy + Clone + traits::Zero + traits::Saturating,
+{
+	fn tcmp(&self, other: &T, threshold: T) -> Ordering {
+		// early exit.
+		if threshold.is_zero() {
+			return self.cmp(&other)
+		}
+
+		let upper_bound = other.saturating_add(threshold);
+		let lower_bound = other.saturating_sub(threshold);
+
+		if upper_bound <= lower_bound {
+			// defensive only. Can never happen.
+			self.cmp(&other)
+		} else {
+			// upper_bound is guaranteed now to be bigger than lower.
+			match (self.cmp(&lower_bound), self.cmp(&upper_bound)) {
+				(Ordering::Greater, Ordering::Greater) => Ordering::Greater,
+				(Ordering::Less, Ordering::Less) => Ordering::Less,
+				_ => Ordering::Equal,
+			}
+		}
+
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;
+	use sp_std::cmp::Ordering;
+
+	#[test]
+	fn epsilon_ord_works() {
+		let b = 115u32;
+		let e = Perbill::from_percent(10).mul_ceil(b);
+
+		// [115 - 11,5 (103,5), 115 + 11,5 (126,5)] is all equal
+		assert_eq!(103u32.tcmp(&b, e), Ordering::Equal);
+		assert_eq!(104u32.tcmp(&b, e), Ordering::Equal);
+		assert_eq!(115u32.tcmp(&b, e), Ordering::Equal);
+		assert_eq!(120u32.tcmp(&b, e), Ordering::Equal);
+		assert_eq!(126u32.tcmp(&b, e), Ordering::Equal);
+		assert_eq!(127u32.tcmp(&b, e), Ordering::Equal);
+
+		assert_eq!(128u32.tcmp(&b, e), Ordering::Greater);
+		assert_eq!(102u32.tcmp(&b, e), Ordering::Less);
+	}
+
+	#[test]
+	fn epsilon_ord_works_with_small_epc() {
+		let b = 115u32;
+		// way less than 1 percent. threshold will be zero. Result should be same as normal ord.
+		let e = Perbill::from_parts(100) * b;
+
+		// [115 - 11,5 (103,5), 115 + 11,5 (126,5)] is all equal
+		assert_eq!(103u32.tcmp(&b, e), 103u32.cmp(&b));
+		assert_eq!(104u32.tcmp(&b, e), 104u32.cmp(&b));
+		assert_eq!(115u32.tcmp(&b, e), 115u32.cmp(&b));
+		assert_eq!(120u32.tcmp(&b, e), 120u32.cmp(&b));
+		assert_eq!(126u32.tcmp(&b, e), 126u32.cmp(&b));
+		assert_eq!(127u32.tcmp(&b, e), 127u32.cmp(&b));
+
+		assert_eq!(128u32.tcmp(&b, e), 128u32.cmp(&b));
+		assert_eq!(102u32.tcmp(&b, e), 102u32.cmp(&b));
+	}
 
 	#[test]
 	fn peru16_rational_does_not_overflow() {

substrate/primitives/arithmetic/src/per_things.rs

@@ -25,6 +25,9 @@ use crate::traits::{
 };
 use sp_debug_derive::RuntimeDebug;
 
+/// Get the inner type of a `PerThing`.
+pub type InnerOf<P> = <P as PerThing>::Inner;
+
 /// Something that implements a fixed point ration with an arbitrary granularity `X`, as _parts per
 /// `X`_.
 pub trait PerThing:
@@ -312,8 +315,7 @@ macro_rules! implement_per_thing {
 		///
 		#[doc = $title]
 		#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
-		#[derive(Encode, Copy, Clone, Default, PartialEq, Eq, PartialOrd, Ord,
+		#[derive(Encode, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, RuntimeDebug, CompactAs)]
-				 RuntimeDebug, CompactAs)]
 		pub struct $name($type);
 
 		impl PerThing for $name {
@@ -566,6 +568,12 @@ macro_rules! implement_per_thing {
 			}
 		}
 
+		impl Default for $name {
+			fn default() -> Self {
+				<Self as PerThing>::zero()
+			}
+		}
+
 		/// Non-overflow multiplication.
 		///
 		/// This is tailored to be used with a balance type.

substrate/primitives/phragmen/fuzzer/src/common.rs

@@ -19,8 +19,8 @@
 
 /// converts x into the range [a, b] in a pseudo-fair way.
 pub fn to_range(x: usize, a: usize, b: usize) -> usize {
-	// does not work correctly if b < 2*a
+	// does not work correctly if b < 2 * a
-	assert!(b > 2 * a);
+	assert!(b >= 2 * a);
 	let collapsed = x % b;
 	if collapsed >= a {
 		collapsed

substrate/primitives/phragmen/fuzzer/src/equalize.rs

@@ -131,7 +131,7 @@ fn main() {
 				return;
 			}
 
-			let enhance = is_score_better(initial_score, final_score);
+			let enhance = is_score_better(final_score, initial_score, Perbill::zero());
 
 			println!(
 				"iter = {} // {:?} -> {:?} [{}]",
@@ -140,6 +140,7 @@ fn main() {
 				final_score,
 				enhance,
 			);
+
 			// if more than one iteration has been done, or they must be equal.
 			assert!(enhance || initial_score == final_score || i == 0)
 		});

substrate/primitives/phragmen/src/lib.rs

@@ -36,7 +36,7 @@
 
 use sp_std::{prelude::*, collections::btree_map::BTreeMap, fmt::Debug, cmp::Ordering, convert::TryFrom};
 use sp_arithmetic::{
-	PerThing, Rational128,
+	PerThing, Rational128, ThresholdOrd,
 	helpers_128bit::multiply_by_rational,
 	traits::{Zero, Saturating, Bounded, SaturatedConversion},
 };
@@ -614,23 +614,36 @@ pub fn evaluate_support<AccountId>(
 	[min_support, sum, sum_squared]
 }
 
-/// Compares two sets of phragmen scores based on desirability and returns true if `that` is
+/// Compares two sets of phragmen scores based on desirability and returns true if `this` is
-/// better than `this`.
+/// better than `that`.
 ///
-/// Evaluation is done in a lexicographic manner.
+/// 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(this: PhragmenScore, that: PhragmenScore) -> bool {
+pub fn is_score_better<P: PerThing>(this: PhragmenScore, that: PhragmenScore, epsilon: P) -> bool
-	match that
+	where ExtendedBalance: From<sp_arithmetic::InnerOf<P>>
+{
+	match this
 		.iter()
 		.enumerate()
-		.map(|(i, e)| e.cmp(&this[i]))
+		.map(|(i, e)| (
-		.collect::<Vec<Ordering>>()
+			e.ge(&that[i]),
+			e.tcmp(&that[i], epsilon.mul_ceil(that[i])),
+		))
+		.collect::<Vec<(bool, Ordering)>>()
 		.as_slice()
 	{
-		[Ordering::Greater, _, _] => true,
+		// epsilon better in the score[0], accept.
-		[Ordering::Equal, Ordering::Greater, _] => true,
+		[(_, Ordering::Greater), _, _] => true,
-		[Ordering::Equal, Ordering::Equal, Ordering::Less] => 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,
 	}
 }

substrate/primitives/phragmen/src/reduce.rs

@@ -640,8 +640,8 @@ fn reduce_all<A: IdentifierT>(assignments: &mut Vec<StakedAssignment<A>>) -> u32
 	num_changed
 }
 
-/// Reduce the given [`Vec<StakedAssignment<IdentifierT>>`]. This removes redundant edges from without changing the
+/// Reduce the given [`Vec<StakedAssignment<IdentifierT>>`]. This removes redundant edges from
-/// overall backing of any of the elected candidates.
+/// without changing the overall backing of any of the elected candidates.
 ///
 /// Returns the number of edges removed.
 ///

substrate/primitives/phragmen/src/tests.rs

@@ -616,28 +616,153 @@ fn assignment_convert_works() {
 }
 
 #[test]
-fn score_comparison_is_lexicographical() {
+fn score_comparison_is_lexicographical_no_epsilon() {
+	let epsilon = Perbill::zero();
 	// only better in the fist parameter, worse in the other two ✅
 	assert_eq!(
-		is_score_better([10, 20, 30], [12, 10, 35]),
+		is_score_better([12, 10, 35], [10, 20, 30], epsilon),
 		true,
 	);
 
 	// worse in the first, better in the other two ❌
 	assert_eq!(
-		is_score_better([10, 20, 30], [9, 30, 10]),
+		is_score_better([9, 30, 10], [10, 20, 30], epsilon),
 		false,
 	);
 
 	// equal in the first, the second one dictates.
 	assert_eq!(
-		is_score_better([10, 20, 30], [10, 25, 40]),
+		is_score_better([10, 25, 40], [10, 20, 30], epsilon),
 		true,
 	);
 
 	// equal in the first two, the last one dictates.
 	assert_eq!(
-		is_score_better([10, 20, 30], [10, 20, 40]),
+		is_score_better([10, 20, 40], [10, 20, 30], epsilon),
+		false,
+	);
+}
+
+#[test]
+fn score_comparison_with_epsilon() {
+	let epsilon = Perbill::from_percent(1);
+
+	{
+		// no more than 1 percent (10) better in the first param.
+		assert_eq!(
+			is_score_better([1009, 5000, 100000], [1000, 5000, 100000], epsilon),
+			false,
+		);
+
+		// now equal, still not better.
+		assert_eq!(
+			is_score_better([1010, 5000, 100000], [1000, 5000, 100000], epsilon),
+			false,
+		);
+
+		// now it is.
+		assert_eq!(
+			is_score_better([1011, 5000, 100000], [1000, 5000, 100000], epsilon),
+			true,
+		);
+	}
+
+	{
+		// First score score is epsilon better, but first score is no longer `ge`. Then this is
+		// still not a good solution.
+		assert_eq!(
+			is_score_better([999, 6000, 100000], [1000, 5000, 100000], epsilon),
+			false,
+		);
+	}
+
+	{
+		// first score is equal or better, but not epsilon. Then second one is the determinant.
+		assert_eq!(
+			is_score_better([1005, 5000, 100000], [1000, 5000, 100000], epsilon),
+			false,
+		);
+
+		assert_eq!(
+			is_score_better([1005, 5050, 100000], [1000, 5000, 100000], epsilon),
+			false,
+		);
+
+		assert_eq!(
+			is_score_better([1005, 5051, 100000], [1000, 5000, 100000], epsilon),
+			true,
+		);
+	}
+
+	{
+		// first score and second are equal or less than epsilon more, third is determinant.
+		assert_eq!(
+			is_score_better([1005, 5025, 100000], [1000, 5000, 100000], epsilon),
+			false,
+		);
+
+		assert_eq!(
+			is_score_better([1005, 5025, 99_000], [1000, 5000, 100000], epsilon),
+			false,
+		);
+
+		assert_eq!(
+			is_score_better([1005, 5025, 98_999], [1000, 5000, 100000], epsilon),
+			true,
+		);
+	}
+}
+
+#[test]
+fn score_comparison_large_value() {
+	// some random value taken from eras in kusama.
+	let initial = [12488167277027543u128, 5559266368032409496, 118749283262079244270992278287436446];
+	// this claim is 0.04090% better in the third component. It should be accepted as better if
+	// epsilon is smaller than 5/10_0000
+	let claim = [12488167277027543u128, 5559266368032409496, 118700736389524721358337889258988054];
+
+	assert_eq!(
+		is_score_better(
+			claim.clone(),
+			initial.clone(),
+			Perbill::from_rational_approximation(1u32, 10_000),
+		),
+		true,
+	);
+
+	assert_eq!(
+		is_score_better(
+			claim.clone(),
+			initial.clone(),
+			Perbill::from_rational_approximation(2u32, 10_000),
+		),
+		true,
+	);
+
+	assert_eq!(
+		is_score_better(
+			claim.clone(),
+			initial.clone(),
+			Perbill::from_rational_approximation(3u32, 10_000),
+		),
+		true,
+	);
+
+	assert_eq!(
+		is_score_better(
+			claim.clone(),
+			initial.clone(),
+			Perbill::from_rational_approximation(4u32, 10_000),
+		),
+		true,
+	);
+
+	assert_eq!(
+		is_score_better(
+			claim.clone(),
+			initial.clone(),
+			Perbill::from_rational_approximation(5u32, 10_000),
+		),
 		false,
 	);
 }