diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 0f09940a4c..94cdf8bed8 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/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; } diff --git a/substrate/frame/grandpa/src/mock.rs b/substrate/frame/grandpa/src/mock.rs index 08329fbb70..6cd7fddf92 100644 --- a/substrate/frame/grandpa/src/mock.rs +++ b/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! { diff --git a/substrate/frame/offences/benchmarking/src/mock.rs b/substrate/frame/offences/benchmarking/src/mock.rs index 28263a5292..c228acdf40 100644 --- a/substrate/frame/offences/benchmarking/src/mock.rs +++ b/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 { diff --git a/substrate/frame/session/benchmarking/src/mock.rs b/substrate/frame/session/benchmarking/src/mock.rs index 5c0e40096e..bed6509732 100644 --- a/substrate/frame/session/benchmarking/src/mock.rs +++ b/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 {} diff --git a/substrate/frame/staking/fuzzer/src/mock.rs b/substrate/frame/staking/fuzzer/src/mock.rs index 0e3b6cb13f..3783415630 100644 --- a/substrate/frame/staking/fuzzer/src/mock.rs +++ b/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 = (); } diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 535eb4446b..434a5dcf93 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/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> { /// equalize will not be executed at all. type MaxIterations: Get; + /// The threshold of improvement that should be provided for a new solution to be accepted. + type MinSolutionScoreBump: Get; + /// The maximum number of nominator rewarded for each validator. /// /// For each validator only the `$MaxNominatorRewardedPerValidator` biggest stakers can claim @@ -2614,7 +2620,7 @@ impl Module { // 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::::PhragmenWeakSubmission.with_weight(T::DbWeight::get().reads(3)), ) } @@ -3506,7 +3512,6 @@ impl frame_support::unsigned::ValidateUnsigned for Module { _, ) = 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 frame_support::unsigned::ValidateUnsigned for Module { } if let Err(error_with_post_info) = Self::pre_dispatch_checks(*score, *era) { - let error = error_with_post_info.error; - let error_number = match error { - DispatchError::Module { error, ..} => error, - _ => 0, - }; + let invalid = to_invalid(error_with_post_info); log!( debug, - "validate unsigned pre dispatch checks failed due to module error #{:?}.", - error, + "validate unsigned pre dispatch checks failed due to 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 frame_support::unsigned::ValidateUnsigned for Module { } } - 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 `>::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(to_invalid) + .map_err(Into::into) + } else { + Err(InvalidTransaction::Call.into()) + } } } @@ -3570,3 +3586,14 @@ impl frame_support::unsigned::ValidateUnsigned for Module { 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) +} diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 094ab6375c..183196a7c2 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/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::(&winners, &staked_assignment).0; let score = evaluate_support(&support); - assert!(sp_phragmen::is_score_better(score, better_score)); + assert!(sp_phragmen::is_score_better::( + better_score, + score, + MinSolutionScoreBump::get(), + )); score }; diff --git a/substrate/primitives/arithmetic/src/lib.rs b/substrate/primitives/arithmetic/src/lib.rs index c4f95af646..a80fe09238 100644 --- a/substrate/primitives/arithmetic/src/lib.rs +++ b/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 { + /// Compare if `self` is `threshold` greater or less than `other`. + fn tcmp(&self, other: &T, epsilon: T) -> Ordering; +} + +impl ThresholdOrd 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() { diff --git a/substrate/primitives/arithmetic/src/per_things.rs b/substrate/primitives/arithmetic/src/per_things.rs index cf5aa6e4cb..50b87d5076 100644 --- a/substrate/primitives/arithmetic/src/per_things.rs +++ b/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

=

::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 { + ::zero() + } + } + /// Non-overflow multiplication. /// /// This is tailored to be used with a balance type. diff --git a/substrate/primitives/phragmen/fuzzer/src/common.rs b/substrate/primitives/phragmen/fuzzer/src/common.rs index f1aab214de..89fed14cfa 100644 --- a/substrate/primitives/phragmen/fuzzer/src/common.rs +++ b/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 - assert!(b > 2 * a); + // does not work correctly if b < 2 * a + assert!(b >= 2 * a); let collapsed = x % b; if collapsed >= a { collapsed diff --git a/substrate/primitives/phragmen/fuzzer/src/equalize.rs b/substrate/primitives/phragmen/fuzzer/src/equalize.rs index 8ca417f269..a46e479b5a 100644 --- a/substrate/primitives/phragmen/fuzzer/src/equalize.rs +++ b/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) }); diff --git a/substrate/primitives/phragmen/src/lib.rs b/substrate/primitives/phragmen/src/lib.rs index 03bbb279d8..6b2686e543 100644 --- a/substrate/primitives/phragmen/src/lib.rs +++ b/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( [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(this: PhragmenScore, that: PhragmenScore, epsilon: P) -> bool + where ExtendedBalance: From> +{ + match this .iter() .enumerate() - .map(|(i, e)| e.cmp(&this[i])) - .collect::>() + .map(|(i, e)| ( + e.ge(&that[i]), + e.tcmp(&that[i], epsilon.mul_ceil(that[i])), + )) + .collect::>() .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, } } diff --git a/substrate/primitives/phragmen/src/reduce.rs b/substrate/primitives/phragmen/src/reduce.rs index 2878aa78c4..d0b4afe73d 100644 --- a/substrate/primitives/phragmen/src/reduce.rs +++ b/substrate/primitives/phragmen/src/reduce.rs @@ -640,8 +640,8 @@ fn reduce_all(assignments: &mut Vec>) -> u32 num_changed } -/// Reduce the given [`Vec>`]. This removes redundant edges from without changing the -/// overall backing of any of the elected candidates. +/// Reduce the given [`Vec>`]. This removes redundant edges from +/// without changing the overall backing of any of the elected candidates. /// /// Returns the number of edges removed. /// diff --git a/substrate/primitives/phragmen/src/tests.rs b/substrate/primitives/phragmen/src/tests.rs index 0219c35a8b..a8bc069147 100644 --- a/substrate/primitives/phragmen/src/tests.rs +++ b/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, ); }