From ffb2125f4a135906bc2aaddc6f31a04891309a32 Mon Sep 17 00:00:00 2001 From: Ankan <10196091+Ank4n@users.noreply.github.com> Date: Fri, 15 Dec 2023 20:59:39 +0100 Subject: [PATCH] [NPoS] Remove better solution threshold for unsigned submissions (#2694) closes https://github.com/paritytech-secops/srlabs_findings/issues/78. Removes `BetterUnsignedThreshold` from pallet EPM. This will essentially mean any solution submitted by the validator that is strictly better than the current queued solution would be accepted. The reason for having these thresholds is to limit number of solutions submitted on-chain. However for unsigned submissions, the number of solutions that could be submitted on average is limited even without thresholding (calculation shown in the corresponding issue). --- polkadot/runtime/westend/src/lib.rs | 2 - prdoc/pr_2694.prdoc | 14 +++ substrate/bin/node/runtime/src/lib.rs | 3 - .../election-provider-multi-phase/src/lib.rs | 10 +- .../election-provider-multi-phase/src/mock.rs | 7 +- .../src/unsigned.rs | 91 +++++++++++++++---- .../test-staking-e2e/src/mock.rs | 1 - 7 files changed, 92 insertions(+), 36 deletions(-) create mode 100644 prdoc/pr_2694.prdoc diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 0f068b093f..e958a660e6 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -531,7 +531,6 @@ parameter_types! { pub const SignedDepositByte: Balance = deposit(0, 10) / 1024; // Each good submission will get 1 WND as reward pub SignedRewardBase: Balance = 1 * UNITS; - pub BetterUnsignedThreshold: Perbill = Perbill::from_rational(5u32, 10_000); // 1 hour session, 15 minutes unsigned phase, 4 offchain executions. pub OffchainRepeat: BlockNumber = UnsignedPhase::get() / 4; @@ -608,7 +607,6 @@ impl pallet_election_provider_multi_phase::Config for Runtime { type MinerConfig = Self; type SlashHandler = (); // burn slashes type RewardHandler = (); // nothing to do upon rewards - type BetterUnsignedThreshold = BetterUnsignedThreshold; type BetterSignedThreshold = (); type OffchainRepeat = OffchainRepeat; type MinerTxPriority = NposSolutionPriority; diff --git a/prdoc/pr_2694.prdoc b/prdoc/pr_2694.prdoc new file mode 100644 index 0000000000..c393dcfeb9 --- /dev/null +++ b/prdoc/pr_2694.prdoc @@ -0,0 +1,14 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: "pallet-election-provider-multi-phase: Removes `BetterUnsignedThreshold` from pallet config" + +doc: + - audience: Runtime Dev + description: | + Removes thresholding for accepting solutions better than the last queued for unsigned phase. This is unnecessary + as even without thresholding, the number of solutions that can be submitted to on-chain which is better than the + previous one is limited. + +crates: + - name: "pallet-election-provider-multi-phase" diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 4d0b253413..76a5c2bf65 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 41284f6c78..04325a40d0 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 abad7db037..af126f08d5 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 e3d0ded975..9434818133 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 ecb2ae435b..04d218acf8 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;