Split SolutionImprovementThresholds into two types (#11221)

* Splitting `SolutionImprovementThreshold` in 2
One for Signed phase and one for Unsigned phase.

* Adding some tests

* Fixes after code review.
- Removing `GetDefault`.
- Shorter naming.
- More explicit test.

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

@@ -574,7 +574,7 @@ parameter_types! {
 	pub const SignedDepositBase: Balance = 1 * DOLLARS;
 	pub const SignedDepositByte: Balance = 1 * CENTS;
 
-	pub SolutionImprovementThreshold: Perbill = Perbill::from_rational(1u32, 10_000);
+	pub BetterUnsignedThreshold: Perbill = Perbill::from_rational(1u32, 10_000);
 
 	// miner configs
 	pub const MultiPhaseUnsignedPriority: TransactionPriority = StakingUnsignedPriority::get() - 1u64;
@@ -664,7 +664,8 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
 	type EstimateCallFee = TransactionPayment;
 	type SignedPhase = SignedPhase;
 	type UnsignedPhase = UnsignedPhase;
-	type SolutionImprovementThreshold = SolutionImprovementThreshold;
+	type BetterUnsignedThreshold = BetterUnsignedThreshold;
+	type BetterSignedThreshold = ();
 	type OffchainRepeat = OffchainRepeat;
 	type MinerMaxWeight = MinerMaxWeight;
 	type MinerMaxLength = MinerMaxLength;

substrate/frame/election-provider-multi-phase/src/lib.rs

@@ -102,8 +102,8 @@
 //! 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::SolutionImprovementThreshold`]) and will limit
+//! than the best queued one (see [`pallet::Config::BetterUnsignedThreshold`]) and will limit the
-//! the weight of the solution to [`pallet::Config::MinerMaxWeight`].
+//! weight of the solution to [`pallet::Config::MinerMaxWeight`].
 //!
 //! 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
@@ -585,9 +585,14 @@ pub mod pallet {
 		type SignedPhase: Get<Self::BlockNumber>;
 
 		/// The minimum amount of improvement to the solution score that defines a solution as
-		/// "better" (in any phase).
+		/// "better" in the Signed phase.
 		#[pallet::constant]
-		type SolutionImprovementThreshold: Get<Perbill>;
+		type BetterSignedThreshold: Get<Perbill>;
+
+		/// The minimum amount of improvement to the solution score that defines a solution as
+		/// "better" in the Unsigned phase.
+		#[pallet::constant]
+		type BetterUnsignedThreshold: Get<Perbill>;
 
 		/// The repeat threshold of the offchain worker.
 		///

substrate/frame/election-provider-multi-phase/src/mock.rs

@@ -20,7 +20,7 @@ use crate as multi_phase;
 use frame_election_provider_support::{
 	data_provider, onchain, ElectionDataProvider, NposSolution, SequentialPhragmen,
 };
-pub use frame_support::{assert_noop, assert_ok};
+pub use frame_support::{assert_noop, assert_ok, pallet_prelude::GetDefault};
 use frame_support::{
 	bounded_vec, parameter_types,
 	traits::{ConstU32, Hooks},
@@ -263,7 +263,8 @@ parameter_types! {
 	pub static SignedRewardBase: Balance = 7;
 	pub static SignedMaxWeight: Weight = BlockWeights::get().max_block;
 	pub static MinerTxPriority: u64 = 100;
-	pub static SolutionImprovementThreshold: Perbill = Perbill::zero();
+	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;
@@ -414,7 +415,8 @@ impl crate::Config for Runtime {
 	type EstimateCallFee = frame_support::traits::ConstU32<8>;
 	type SignedPhase = SignedPhase;
 	type UnsignedPhase = UnsignedPhase;
-	type SolutionImprovementThreshold = SolutionImprovementThreshold;
+	type BetterUnsignedThreshold = BetterUnsignedThreshold;
+	type BetterSignedThreshold = BetterSignedThreshold;
 	type OffchainRepeat = OffchainRepeat;
 	type MinerMaxWeight = MinerMaxWeight;
 	type MinerMaxLength = MinerMaxLength;
@@ -537,8 +539,12 @@ impl ExtBuilder {
 		<MinerTxPriority>::set(p);
 		self
 	}
-	pub fn solution_improvement_threshold(self, p: Perbill) -> Self {
+	pub fn better_signed_threshold(self, p: Perbill) -> Self {
-		<SolutionImprovementThreshold>::set(p);
+		<BetterSignedThreshold>::set(p);
+		self
+	}
+	pub fn better_unsigned_threshold(self, p: Perbill) -> Self {
+		<BetterUnsignedThreshold>::set(p);
 		self
 	}
 	pub fn phases(self, signed: BlockNumber, unsigned: BlockNumber) -> Self {

substrate/frame/election-provider-multi-phase/src/signed.rs

@@ -291,7 +291,7 @@ impl<T: Config> SignedSubmissions<T> {
 					None => return InsertResult::NotInserted,
 					Some((score, _)) => *score,
 				};
-				let threshold = T::SolutionImprovementThreshold::get();
+				let threshold = T::BetterSignedThreshold::get();
 
 				// if we haven't improved on the weakest score, don't change anything.
 				if !insert_score.strict_threshold_better(weakest_score, threshold) {
@@ -499,7 +499,7 @@ mod tests {
 			balances, raw_solution, roll_to, ExtBuilder, MultiPhase, Origin, Runtime,
 			SignedMaxSubmissions, SignedMaxWeight,
 		},
-		Error, Phase,
+		Error, Perbill, Phase,
 	};
 	use frame_support::{assert_noop, assert_ok, assert_storage_noop};
 
@@ -632,6 +632,54 @@ mod tests {
 		})
 	}
 
+	#[test]
+	fn cannot_submit_worse_with_full_queue_depends_on_threshold() {
+		ExtBuilder::default()
+			.signed_max_submission(1)
+			.better_signed_threshold(Perbill::from_percent(20))
+			.build_and_execute(|| {
+				roll_to(15);
+				assert!(MultiPhase::current_phase().is_signed());
+
+				let mut solution = RawSolution {
+					score: ElectionScore {
+						minimal_stake: 5u128,
+						sum_stake: 0u128,
+						sum_stake_squared: 10u128,
+					},
+					..Default::default()
+				};
+				assert_ok!(MultiPhase::submit(Origin::signed(99), Box::new(solution)));
+
+				// This is 10% better, so does not meet the 20% threshold and is therefore rejected.
+				solution = RawSolution {
+					score: ElectionScore {
+						minimal_stake: 5u128,
+						sum_stake: 0u128,
+						sum_stake_squared: 9u128,
+					},
+					..Default::default()
+				};
+
+				assert_noop!(
+					MultiPhase::submit(Origin::signed(99), Box::new(solution)),
+					Error::<Runtime>::SignedQueueFull,
+				);
+
+				// This is however 30% better and should therefore be accepted.
+				solution = RawSolution {
+					score: ElectionScore {
+						minimal_stake: 5u128,
+						sum_stake: 0u128,
+						sum_stake_squared: 7u128,
+					},
+					..Default::default()
+				};
+
+				assert_ok!(MultiPhase::submit(Origin::signed(99), Box::new(solution)));
+			})
+	}
+
 	#[test]
 	fn weakest_is_removed_if_better_provided() {
 		ExtBuilder::default().build_and_execute(|| {

substrate/frame/election-provider-multi-phase/src/unsigned.rs

@@ -624,7 +624,7 @@ impl<T: Config> Pallet<T> {
 		ensure!(
 			Self::queued_solution().map_or(true, |q: ReadySolution<_>| raw_solution
 				.score
-				.strict_threshold_better(q.score, T::SolutionImprovementThreshold::get())),
+				.strict_threshold_better(q.score, T::BetterUnsignedThreshold::get())),
 			Error::<T>::PreDispatchWeakSubmission,
 		);
 
@@ -1066,7 +1066,7 @@ mod tests {
 			.desired_targets(1)
 			.add_voter(7, 2, bounded_vec![10])
 			.add_voter(8, 5, bounded_vec![10])
-			.solution_improvement_threshold(Perbill::from_percent(50))
+			.better_unsigned_threshold(Perbill::from_percent(50))
 			.build_and_execute(|| {
 				roll_to(25);
 				assert!(MultiPhase::current_phase().is_unsigned());