diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 4d0b253413a87..76a5c2bf65f70 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -704,8 +704,6 @@ parameter_types! { pub const SignedDepositIncreaseFactor: Percent = Percent::from_percent(10); pub const SignedDepositByte: Balance = 1 * CENTS; - pub BetterUnsignedThreshold: Perbill = Perbill::from_rational(1u32, 10_000); - // miner configs pub const MultiPhaseUnsignedPriority: TransactionPriority = StakingUnsignedPriority::get() - 1u64; pub MinerMaxWeight: Weight = RuntimeBlockWeights::get() @@ -822,7 +820,6 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type EstimateCallFee = TransactionPayment; type SignedPhase = SignedPhase; type UnsignedPhase = UnsignedPhase; - type BetterUnsignedThreshold = BetterUnsignedThreshold; type BetterSignedThreshold = (); type OffchainRepeat = OffchainRepeat; type MinerTxPriority = MultiPhaseUnsignedPriority; diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 41284f6c78b1c..04325a40d0ada 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -101,9 +101,8 @@ //! unsigned transaction, thus the name _unsigned_ phase. This unsigned transaction can never be //! valid if propagated, and it acts similar to an inherent. //! -//! Validators will only submit solutions if the one that they have computed is sufficiently better -//! than the best queued one (see [`pallet::Config::BetterUnsignedThreshold`]) and will limit the -//! weight of the solution to [`MinerConfig::MaxWeight`]. +//! Validators will only submit solutions if the one that they have computed is strictly better than +//! the best queued one and will limit the weight of the solution to [`MinerConfig::MaxWeight`]. //! //! The unsigned phase can be made passive depending on how the previous signed phase went, by //! setting the first inner value of [`Phase`] to `false`. For now, the signed phase is always @@ -598,11 +597,6 @@ pub mod pallet { #[pallet::constant] type BetterSignedThreshold: Get; - /// The minimum amount of improvement to the solution score that defines a solution as - /// "better" in the Unsigned phase. - #[pallet::constant] - type BetterUnsignedThreshold: Get; - /// The repeat threshold of the offchain worker. /// /// For example, if it is 5, that means that at least 5 blocks will elapse between attempts diff --git a/substrate/frame/election-provider-multi-phase/src/mock.rs b/substrate/frame/election-provider-multi-phase/src/mock.rs index abad7db037e70..af126f08d5103 100644 --- a/substrate/frame/election-provider-multi-phase/src/mock.rs +++ b/substrate/frame/election-provider-multi-phase/src/mock.rs @@ -301,7 +301,6 @@ parameter_types! { pub static SignedMaxWeight: Weight = BlockWeights::get().max_block; pub static MinerTxPriority: u64 = 100; pub static BetterSignedThreshold: Perbill = Perbill::zero(); - pub static BetterUnsignedThreshold: Perbill = Perbill::zero(); pub static OffchainRepeat: BlockNumber = 5; pub static MinerMaxWeight: Weight = BlockWeights::get().max_block; pub static MinerMaxLength: u32 = 256; @@ -399,7 +398,6 @@ impl crate::Config for Runtime { type EstimateCallFee = frame_support::traits::ConstU32<8>; type SignedPhase = SignedPhase; type UnsignedPhase = UnsignedPhase; - type BetterUnsignedThreshold = BetterUnsignedThreshold; type BetterSignedThreshold = BetterSignedThreshold; type OffchainRepeat = OffchainRepeat; type MinerTxPriority = MinerTxPriority; @@ -538,10 +536,7 @@ impl ExtBuilder { ::set(p); self } - pub fn better_unsigned_threshold(self, p: Perbill) -> Self { - ::set(p); - self - } + pub fn phases(self, signed: BlockNumber, unsigned: BlockNumber) -> Self { ::set(signed); ::set(unsigned); diff --git a/substrate/frame/election-provider-multi-phase/src/unsigned.rs b/substrate/frame/election-provider-multi-phase/src/unsigned.rs index e3d0ded97515b..9434818133406 100644 --- a/substrate/frame/election-provider-multi-phase/src/unsigned.rs +++ b/substrate/frame/election-provider-multi-phase/src/unsigned.rs @@ -384,9 +384,8 @@ impl Pallet { // ensure score is being improved. Panic henceforth. ensure!( - Self::queued_solution().map_or(true, |q: ReadySolution<_, _>| raw_solution - .score - .strict_threshold_better(q.score, T::BetterUnsignedThreshold::get())), + Self::queued_solution() + .map_or(true, |q: ReadySolution<_, _>| raw_solution.score > q.score), Error::::PreDispatchWeakSubmission, ); @@ -1025,7 +1024,7 @@ mod tests { bounded_vec, offchain::storage_lock::{BlockAndTime, StorageLock}, traits::{Dispatchable, ValidateUnsigned, Zero}, - ModuleError, PerU16, Perbill, + ModuleError, PerU16, }; type Assignment = crate::unsigned::Assignment; @@ -1360,7 +1359,7 @@ mod tests { .desired_targets(1) .add_voter(7, 2, bounded_vec![10]) .add_voter(8, 5, bounded_vec![10]) - .better_unsigned_threshold(Perbill::from_percent(50)) + .add_voter(9, 1, bounded_vec![10]) .build_and_execute(|| { roll_to_unsigned(); assert!(MultiPhase::current_phase().is_unsigned()); @@ -1368,12 +1367,15 @@ mod tests { // an initial solution let result = ElectionResult { - // note: This second element of backing stake is not important here. - winners: vec![(10, 10)], - assignments: vec![Assignment { - who: 10, - distribution: vec![(10, PerU16::one())], - }], + winners: vec![(10, 12)], + assignments: vec![ + Assignment { who: 10, distribution: vec![(10, PerU16::one())] }, + Assignment { + who: 7, + // note: this percent doesn't even matter, in solution it is 100%. + distribution: vec![(10, PerU16::one())], + }, + ], }; let RoundSnapshot { voters, targets } = MultiPhase::snapshot().unwrap(); @@ -1394,9 +1396,35 @@ mod tests { Box::new(solution), witness )); - assert_eq!(MultiPhase::queued_solution().unwrap().score.minimal_stake, 10); + assert_eq!(MultiPhase::queued_solution().unwrap().score.minimal_stake, 12); - // trial 1: a solution who's score is only 2, i.e. 20% better in the first element. + // trial 1: a solution who's minimal stake is 10, i.e. worse than the first solution + // of 12. + let result = ElectionResult { + winners: vec![(10, 10)], + assignments: vec![Assignment { + who: 10, + distribution: vec![(10, PerU16::one())], + }], + }; + let (raw, score, _, _) = Miner::::prepare_election_result_with_snapshot( + result, + voters.clone(), + targets.clone(), + desired_targets, + ) + .unwrap(); + let solution = RawSolution { solution: raw, score, round: MultiPhase::round() }; + // 10 is not better than 12 + assert_eq!(solution.score.minimal_stake, 10); + // submitting this will actually panic. + assert_noop!( + MultiPhase::unsigned_pre_dispatch_checks(&solution), + Error::::PreDispatchWeakSubmission, + ); + + // trial 2: try resubmitting another solution with same score (12) as the queued + // solution. let result = ElectionResult { winners: vec![(10, 12)], assignments: vec![ @@ -1408,6 +1436,7 @@ mod tests { }, ], }; + let (raw, score, _, _) = Miner::::prepare_election_result_with_snapshot( result, voters.clone(), @@ -1416,15 +1445,45 @@ mod tests { ) .unwrap(); let solution = RawSolution { solution: raw, score, round: MultiPhase::round() }; - // 12 is not 50% more than 10 + // 12 is not better than 12. We need score of atleast 13 to be accepted. assert_eq!(solution.score.minimal_stake, 12); + // submitting this will panic. assert_noop!( MultiPhase::unsigned_pre_dispatch_checks(&solution), Error::::PreDispatchWeakSubmission, ); - // submitting this will actually panic. - // trial 2: a solution who's score is only 7, i.e. 70% better in the first element. + // trial 3: a solution who's minimal stake is 13, i.e. 1 better than the queued + // solution of 12. + let result = ElectionResult { + winners: vec![(10, 12)], + assignments: vec![ + Assignment { who: 10, distribution: vec![(10, PerU16::one())] }, + Assignment { who: 7, distribution: vec![(10, PerU16::one())] }, + Assignment { who: 9, distribution: vec![(10, PerU16::one())] }, + ], + }; + let (raw, score, witness, _) = + Miner::::prepare_election_result_with_snapshot( + result, + voters.clone(), + targets.clone(), + desired_targets, + ) + .unwrap(); + let solution = RawSolution { solution: raw, score, round: MultiPhase::round() }; + assert_eq!(solution.score.minimal_stake, 13); + + // this should work + assert_ok!(MultiPhase::unsigned_pre_dispatch_checks(&solution)); + assert_ok!(MultiPhase::submit_unsigned( + RuntimeOrigin::none(), + Box::new(solution), + witness + )); + + // trial 4: a solution who's minimal stake is 17, i.e. 4 better than the last + // soluton. let result = ElectionResult { winners: vec![(10, 12)], assignments: vec![ diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs index ecb2ae435b8c6..04d218acf8fd1 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs @@ -190,7 +190,6 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type SignedPhase = SignedPhase; type UnsignedPhase = UnsignedPhase; type BetterSignedThreshold = (); - type BetterUnsignedThreshold = (); type OffchainRepeat = OffchainRepeat; type MinerTxPriority = TransactionPriority; type MinerConfig = Self;