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

bin/node/runtime/src/lib.rs

@@ -320,9 +320,10 @@ 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;
+	pub SolutionImprovementThreshold: Perbill = Perbill::from_parts(Perbill::from_percent(1).deconstruct() / 10);
 }
 
 impl pallet_staking::Trait for Runtime {
@@ -344,6 +345,7 @@ impl pallet_staking::Trait for Runtime {
 	type ElectionLookahead = ElectionLookahead;
 	type Call = Call;
 	type MaxIterations = MaxIterations;
+	type SolutionImprovementThreshold = SolutionImprovementThreshold;
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
 	type UnsignedPriority = StakingUnsignedPriority;
 }

frame/grandpa/src/mock.rs

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

frame/offences/benchmarking/src/mock.rs

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

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 SolutionImprovementThreshold = ();
 }
 
 impl crate::Trait for Test {}

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 SolutionImprovementThreshold = ();
 	type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
 	type UnsignedPriority = ();
 }

frame/staking/src/lib.rs

@@ -301,7 +301,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 +891,10 @@ 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.
+	// TODO: fancy a shorter name? my fingers do not like this.
+	type SolutionImprovementThreshold: Get<Perbill>;
+
 	/// The maximum number of nominator rewarded for each validator.
 	///
 	/// For each validator only the `$MaxNominatorRewardedPerValidator` biggest stakers can claim
@@ -2614,7 +2618,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::SolutionImprovementThreshold::get()),
 				Error::<T>::PhragmenWeakSubmission.with_weight(T::DbWeight::get().reads(3)),
 			)
 		}
@@ -3506,7 +3510,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 {
@@ -3556,13 +3559,34 @@ impl<T: Trait> frame_support::unsigned::ValidateUnsigned for Module<T> {
 		}
 	}
 
-	fn pre_dispatch(_: &Self::Call) -> Result<(), TransactionValidityError> {
-		// IMPORTANT NOTE: By default, a sane `pre-dispatch` should always do the same checks as
-		// `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
-		// override this to save some time.
-		Ok(())
+	fn pre_dispatch(call: &Self::Call) -> Result<(), TransactionValidityError> {
+		if let Call::submit_election_solution_unsigned(
+			_,
+			_,
+			score,
+			era,
+			_,
+		) = 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(|error_with_post_info| {
+					let error = error_with_post_info.error;
+					let error_number = match error {
+						DispatchError::Module { error, ..} => error,
+						_ => 0,
+					};
+					InvalidTransaction::Custom(error_number).into()
+				})
+		} else {
+			Err(InvalidTransaction::Call.into())
+		}
 	}
 }
 

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 SolutionImprovementThreshold: Perbill = Perbill::zero();
 }
 
 thread_local! {
@@ -321,6 +322,7 @@ impl Trait for Test {
 	type ElectionLookahead = ElectionLookahead;
 	type Call = Call;
 	type MaxIterations = MaxIterations;
+	type SolutionImprovementThreshold = SolutionImprovementThreshold;
 	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,
+			SolutionImprovementThreshold::get(),
+		));
 
 		score
 	};

primitives/arithmetic/src/lib.rs

@@ -41,12 +41,95 @@ 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;
+use sp_std::ops;
+use traits::{UniqueSaturatedInto, Saturating};
+
+/// Trait for comparing two numbers with an epsilon coefficient.
+///
+/// Returns:
+/// - `Ordering::Greater` if `self` is greater than `other + other * epsilon`.
+/// - `Ordering::Less` if `self` is less than `other - other * epsilon`.
+/// - `Ordering::Equal` otherwise.
+pub trait EpsilonOrd<T, P> {
+	/// Compare if `self` is `epsilon` greater or less than `other`.
+	fn ecmp(&self, other: &T, epsilon: P) -> Ordering;
+}
+
+impl<T, P> EpsilonOrd<T, P> for T
+where
+	P: PerThing,
+	T: Ord + PartialOrd + Clone + Copy + ops::Rem<T, Output=T> + ops::Mul<T, Output=T> +
+		ops::Div<T, Output=T> + ops::Add<T, Output=T> + UniqueSaturatedInto<InnerOf<P>> +
+		From<InnerOf<P>> + Saturating,
+{
+	fn ecmp(&self, other: &T, epsilon: P) -> Ordering {
+		// early exit.
+		if epsilon.is_zero() {
+			return self.cmp(&other)
+		}
+
+		let threshold = epsilon.mul_ceil(*other);
+		let lower_bound = other.saturating_sub(threshold);
+		let upper_bound = other.saturating_add(threshold);
+
+		if upper_bound <= lower_bound {
+			// threshold is too small to make any difference.
+			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);
+
+		// [115 - 11,5 (103,5), 115 + 11,5 (126,5)] is all equal
+		assert_eq!(103u32.ecmp(&b, e), Ordering::Equal);
+		assert_eq!(104u32.ecmp(&b, e), Ordering::Equal);
+		assert_eq!(115u32.ecmp(&b, e), Ordering::Equal);
+		assert_eq!(120u32.ecmp(&b, e), Ordering::Equal);
+		assert_eq!(126u32.ecmp(&b, e), Ordering::Equal);
+		assert_eq!(127u32.ecmp(&b, e), Ordering::Equal);
+
+		assert_eq!(128u32.ecmp(&b, e), Ordering::Greater);
+		assert_eq!(102u32.ecmp(&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);
+
+		// [115 - 11,5 (103,5), 115 + 11,5 (126,5)] is all equal
+		assert_eq!(103u32.ecmp(&b, e), 103u32.cmp(&b));
+		assert_eq!(104u32.ecmp(&b, e), 104u32.cmp(&b));
+		assert_eq!(115u32.ecmp(&b, e), 115u32.cmp(&b));
+		assert_eq!(120u32.ecmp(&b, e), 120u32.cmp(&b));
+		assert_eq!(126u32.ecmp(&b, e), 126u32.cmp(&b));
+		assert_eq!(127u32.ecmp(&b, e), 127u32.cmp(&b));
+
+		assert_eq!(128u32.ecmp(&b, e), 128u32.cmp(&b));
+		assert_eq!(102u32.ecmp(&b, e), 102u32.cmp(&b));
+	}
 
 	#[test]
 	fn peru16_rational_does_not_overflow() {

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,
-				 RuntimeDebug, CompactAs)]
+		#[derive(Encode, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, 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.

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
-	assert!(b > 2 * a);
+	// does not work correctly if b < 2 * a
+	assert!(b >= 2 * a);
 	let collapsed = x % b;
 	if collapsed >= a {
 		collapsed

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)
 		});

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, EpsilonOrd,
 	helpers_128bit::multiply_by_rational,
 	traits::{Zero, Saturating, Bounded, SaturatedConversion},
 };
@@ -614,23 +614,33 @@ 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
-/// better than `this`.
+/// Compares two sets of phragmen scores based on desirability and returns true if `this` is
+/// 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 {
-	match that
+pub fn is_score_better<P: PerThing>(this: PhragmenScore, that: PhragmenScore, epsilon: P) -> bool
+	where ExtendedBalance: From<sp_arithmetic::InnerOf<P>>
+{
+	match this
 		.iter()
 		.enumerate()
-		.map(|(i, e)| e.cmp(&this[i]))
-		.collect::<Vec<Ordering>>()
+		.map(|(i, e)| (e.ge(&that[i]), e.ecmp(&that[i], epsilon)))
+		.collect::<Vec<(bool, Ordering)>>()
 		.as_slice()
 	{
-		[Ordering::Greater, _, _] => true,
-		[Ordering::Equal, Ordering::Greater, _] => true,
-		[Ordering::Equal, Ordering::Equal, Ordering::Less] => true,
+		// 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,
 	}
 }