Remove "SolutionImprovementThreshold" logic (#10340)

Fixes [9119](#9119)

The threshold mechanism used by the "election-provider-multi-block"
verifier pallet is no longer relevant because there is no more queued
solution to compare against during the initial verification and
subseuquently, solutions are processed in the order of decreasing score,
with the first one being chosen in all cases.

---------

Signed-off-by: Andrei Trandafir <[email protected]>
Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Author: andreitrand

cumulus/parachains/runtimes/assets/asset-hub-westend/src/staking.rs

@@ -67,9 +67,6 @@ parameter_types! {
 
 	/// Size of the exposures. This should be small enough to make the reward payouts feasible.
 	pub MaxExposurePageSize: u32 = 64;
-
-	/// Each solution is considered "better" if it is 0.01% better.
-	pub SolutionImprovementThreshold: Perbill = Perbill::from_rational(1u32, 10_000);
 }
 
 frame_election_provider_support::generate_solution_type!(
@@ -143,7 +140,6 @@ impl multi_block::verifier::Config for Runtime {
 	type MaxBackersPerWinner = MaxBackersPerWinner;
 	type MaxBackersPerWinnerFinal = MaxBackersPerWinnerFinal;
 	type SolutionDataProvider = MultiBlockElectionSigned;
-	type SolutionImprovementThreshold = SolutionImprovementThreshold;
 	type WeightInfo = weights::pallet_election_provider_multi_block_verifier::WeightInfo<Runtime>;
 }
 

prdoc/pr_10340.prdoc

@@ -0,0 +1,15 @@
+title: Remove "SolutionImprovementThreshold" logic
+doc:
+- audience: Runtime Dev
+  description: |-
+    The threshold mechanism used by the `election-provider-multi-block` verifier pallet is no longer relevant. There are no queued solutions to compare during the initial verification. Solutions are subsequently processed in order of decreasing score, with the first verified solution being selected, while any remaining solutions are not verified.
+
+crates:
+- name: asset-hub-westend-runtime
+  bump: major
+- name: pallet-election-provider-multi-block
+  bump: major
+- name: pallet-election-provider-multi-phase
+  bump: major
+- name: sp-npos-elections
+  bump: major

substrate/frame/election-provider-multi-block/src/mock/mod.rs

@@ -149,7 +149,6 @@ parameter_types! {
 
 	pub static FallbackMode: FallbackModes = FallbackModes::Emergency;
 	pub static MinerTxPriority: u64 = 100;
-	pub static SolutionImprovementThreshold: Perbill = Perbill::zero();
 	pub static OffchainRepeat: BlockNumber = 5;
 	pub static OffchainStorage: bool = true;
 	pub static MinerMaxLength: u32 = 256;
@@ -187,7 +186,6 @@ impl Get<Phase<Runtime>> for AreWeDone {
 }
 
 impl crate::verifier::Config for Runtime {
-	type SolutionImprovementThreshold = SolutionImprovementThreshold;
 	type MaxBackersPerWinnerFinal = MaxBackersPerWinnerFinal;
 	type MaxBackersPerWinner = MaxBackersPerWinner;
 	type MaxWinnersPerPage = MaxWinnersPerPage;
@@ -370,10 +368,6 @@ impl ExtBuilder {
 		MinerTxPriority::set(p);
 		self
 	}
-	pub(crate) fn solution_improvement_threshold(self, p: Perbill) -> Self {
-		SolutionImprovementThreshold::set(p);
-		self
-	}
 	pub(crate) fn election_start(self, at: BlockNumber) -> Self {
 		ElectionStart::set(at);
 		self

substrate/frame/election-provider-multi-block/src/unsigned/mod.rs

@@ -381,64 +381,64 @@ mod validate_unsigned {
 	use crate::{mock::*, types::*, verifier::Verifier};
 
 	#[test]
-	fn retracts_weak_score_accepts_threshold_better() {
-		ExtBuilder::unsigned()
-			.solution_improvement_threshold(sp_runtime::Perbill::from_percent(10))
-			.build_and_execute(|| {
-				roll_to_snapshot_created();
+	fn retracts_weak_score_accepts_better() {
+		ExtBuilder::unsigned().build_and_execute(|| {
+			roll_to_snapshot_created();
 
-				let solution = mine_full_solution().unwrap();
-				load_mock_signed_and_start(solution.clone());
-				roll_to_full_verification();
+			let base_minimal_stake = 55;
+			let solution = mine_full_solution().unwrap();
+			load_mock_signed_and_start(solution.clone());
+			roll_to_full_verification();
 
-				// Some good solution is queued now.
-				assert_eq!(
-					<VerifierPallet as Verifier>::queued_score(),
-					Some(ElectionScore {
-						minimal_stake: 55,
-						sum_stake: 130,
-						sum_stake_squared: 8650
-					})
-				);
+			// Some good solution is queued now.
+			assert_eq!(
+				<VerifierPallet as Verifier>::queued_score(),
+				Some(ElectionScore {
+					minimal_stake: base_minimal_stake,
+					sum_stake: 130,
+					sum_stake_squared: 8650
+				})
+			);
 
-				roll_to_unsigned_open();
+			roll_to_unsigned_open();
 
-				// this is just worse
-				let attempt =
-					fake_solution(ElectionScore { minimal_stake: 20, ..Default::default() });
-				let call = Call::submit_unsigned { paged_solution: Box::new(attempt) };
-				assert_eq!(
-					UnsignedPallet::validate_unsigned(TransactionSource::Local, &call).unwrap_err(),
-					TransactionValidityError::Invalid(InvalidTransaction::Custom(2)),
-				);
+			// This is just worse.
+			let attempt = fake_solution(ElectionScore {
+				minimal_stake: base_minimal_stake - 1,
+				..Default::default()
+			});
+			let call = Call::submit_unsigned { paged_solution: Box::new(attempt) };
+			assert_eq!(
+				UnsignedPallet::validate_unsigned(TransactionSource::Local, &call).unwrap_err(),
+				TransactionValidityError::Invalid(InvalidTransaction::Custom(2)),
+			);
 
-				// this is better, but not enough better.
-				let insufficient_improvement = 55 * 105 / 100;
-				let attempt = fake_solution(ElectionScore {
-					minimal_stake: insufficient_improvement,
-					..Default::default()
-				});
-				let call = Call::submit_unsigned { paged_solution: Box::new(attempt) };
-				assert_eq!(
-					UnsignedPallet::validate_unsigned(TransactionSource::Local, &call).unwrap_err(),
-					TransactionValidityError::Invalid(InvalidTransaction::Custom(2)),
-				);
+			// This is better, but the number of winners is incorrect.
+			let attempt = fake_solution(ElectionScore {
+				minimal_stake: base_minimal_stake + 1,
+				..Default::default()
+			});
+			let call = Call::submit_unsigned { paged_solution: Box::new(attempt) };
+			assert_eq!(
+				UnsignedPallet::validate_unsigned(TransactionSource::Local, &call).unwrap_err(),
+				TransactionValidityError::Invalid(InvalidTransaction::Custom(4)),
+			);
 
-				// note that we now have to use a solution with 2 winners, just to pass all of the
-				// snapshot independent checks.
-				let mut paged = raw_paged_from_supports(
-					vec![vec![
-						(40, Support { total: 10, voters: vec![(3, 5)] }),
-						(30, Support { total: 10, voters: vec![(3, 5)] }),
-					]],
-					0,
-				);
-				let sufficient_improvement = 55 * 115 / 100;
-				paged.score =
-					ElectionScore { minimal_stake: sufficient_improvement, ..Default::default() };
-				let call = Call::submit_unsigned { paged_solution: Box::new(paged) };
-				assert!(UnsignedPallet::validate_unsigned(TransactionSource::Local, &call).is_ok());
-			})
+			// Note that we now have to use a solution with 2 winners, just to pass all of the
+			// snapshot independent checks.
+			let mut paged = raw_paged_from_supports(
+				vec![vec![
+					(40, Support { total: 10, voters: vec![(3, 5)] }),
+					(30, Support { total: 10, voters: vec![(3, 5)] }),
+				]],
+				0,
+			);
+
+			paged.score =
+				ElectionScore { minimal_stake: base_minimal_stake + 1, ..Default::default() };
+			let call = Call::submit_unsigned { paged_solution: Box::new(paged) };
+			assert!(UnsignedPallet::validate_unsigned(TransactionSource::Local, &call).is_ok());
+		})
 	}
 
 	#[test]

substrate/frame/election-provider-multi-block/src/verifier/impls.rs

@@ -38,7 +38,6 @@ use frame_support::{
 use frame_system::pallet_prelude::*;
 use pallet::*;
 use sp_npos_elections::{evaluate_support, ElectionScore};
-use sp_runtime::Perbill;
 use sp_std::{collections::btree_map::BTreeMap, prelude::*};
 
 pub(crate) type SupportsOfVerifier<V> = frame_election_provider_support::BoundedSupports<
@@ -115,11 +114,6 @@ pub(crate) mod pallet {
 	#[pallet::config]
 	#[pallet::disable_frame_system_supertrait_check]
 	pub trait Config: crate::Config {
-		/// The minimum amount of improvement to the solution score that defines a solution as
-		/// "better".
-		#[pallet::constant]
-		type SolutionImprovementThreshold: Get<Perbill>;
-
 		/// Maximum number of backers, per winner, among all pages of an election.
 		///
 		/// This can only be checked at the very final step of verification.
@@ -193,8 +187,7 @@ pub(crate) mod pallet {
 	///
 	/// - `QueuedSolutionScore` must always be correct. In other words, it should correctly be the
 	///   score of `QueuedValidVariant`.
-	/// - `QueuedSolutionScore` must always be [`Config::SolutionImprovementThreshold`] better than
-	///   `MinimumScore`.
+	/// - `QueuedSolutionScore` must always be better than `MinimumScore`.
 	/// - The number of existing keys in `QueuedSolutionBackings` must always match that of the
 	///   INVALID variant.
 	///
@@ -473,8 +466,7 @@ pub(crate) mod pallet {
 			ensure!(
 				Pallet::<T>::minimum_score()
 					.zip(Self::queued_score())
-					.map_or(true, |(min_score, score)| score
-						.strict_threshold_better(min_score, Perbill::zero())),
+					.map_or(true, |(min_score, score)| score.strict_better(min_score)),
 				"queued solution has weak score (min-score)"
 			);
 
@@ -705,7 +697,7 @@ impl<T: Config> Pallet<T> {
 	) -> Result<(), (PageIndex, FeasibilityError)> {
 		let first_page = solution_pages.first().cloned().unwrap_or_default();
 		let last_page = solution_pages.last().cloned().unwrap_or_default();
-		// first, ensure this score will be good enough, even if valid..
+		// first, ensure this score will be good enough, even if valid.
 		let _ = Self::ensure_score_quality(claimed_score).map_err(|fe| (first_page, fe))?;
 		ensure!(
 			partial_solutions.len() == solution_pages.len(),
@@ -817,13 +809,12 @@ impl<T: Config> Pallet<T> {
 	/// - better than the queued solution, if one exists.
 	/// - greater than the minimum untrusted score.
 	pub(crate) fn ensure_score_quality(score: ElectionScore) -> Result<(), FeasibilityError> {
-		let is_improvement = <Self as Verifier>::queued_score().map_or(true, |best_score| {
-			score.strict_threshold_better(best_score, T::SolutionImprovementThreshold::get())
-		});
+		let is_improvement = <Self as Verifier>::queued_score()
+			.map_or(true, |best_score| score.strict_better(best_score));
 		ensure!(is_improvement, FeasibilityError::ScoreTooLow);
 
-		let is_greater_than_min_untrusted = Self::minimum_score()
-			.map_or(true, |min_score| score.strict_threshold_better(min_score, Perbill::zero()));
+		let is_greater_than_min_untrusted =
+			Self::minimum_score().map_or(true, |min_score| score.strict_better(min_score));
 		ensure!(is_greater_than_min_untrusted, FeasibilityError::ScoreTooLow);
 
 		Ok(())

substrate/frame/election-provider-multi-block/src/verifier/tests.rs

@@ -28,7 +28,7 @@ use frame_election_provider_support::Support;
 use frame_support::{assert_noop, assert_ok};
 use sp_core::bounded_vec;
 use sp_npos_elections::ElectionScore;
-use sp_runtime::{traits::Bounded, PerU16, Perbill};
+use sp_runtime::{traits::Bounded, PerU16};
 
 mod feasibility_check {
 	use super::*;
@@ -1420,38 +1420,75 @@ mod single_page_sync_verification {
 	}
 
 	#[test]
-	fn solution_improvement_threshold_respected() {
-		ExtBuilder::verifier()
-			.solution_improvement_threshold(Perbill::from_percent(10))
-			.build_and_execute(|| {
-				roll_to_snapshot_created();
+	fn solution_improvement_respected() {
+		ExtBuilder::verifier().build_and_execute(|| {
+			roll_to_snapshot_created();
 
-				// submit something good.
-				let single_page = mine_solution(1).unwrap();
-				let _ = <VerifierPallet as Verifier>::verify_synchronous(
-					single_page.solution_pages.first().cloned().unwrap(),
-					single_page.score,
+			// submit something good.
+			let single_page = mine_solution(1).unwrap();
+			let _ = <VerifierPallet as Verifier>::verify_synchronous(
+				single_page.solution_pages.first().cloned().unwrap(),
+				single_page.score,
+				MultiBlock::msp(),
+			)
+			.unwrap();
+
+			// The slightly better solution is incorrect in the number of winners.
+			let mut better_score = single_page.score;
+			better_score.minimal_stake += 1;
+			let slightly_better = fake_solution(better_score);
+
+			// The score is checked first; we therefore expect verification to
+			// succeed in score but subsequently fail in winner count.
+			assert_eq!(
+				<VerifierPallet as Verifier>::verify_synchronous(
+					slightly_better.solution_pages.first().cloned().unwrap(),
+					slightly_better.score,
 					MultiBlock::msp(),
 				)
-				.unwrap();
+				.unwrap_err(),
+				FeasibilityError::WrongWinnerCount
+			);
+		});
+	}
 
-				// the slightly better solution need not even be correct. We improve it by 5%, but
-				// we need 10%.
-				let mut better_score = single_page.score;
-				let improvement = Perbill::from_percent(5) * better_score.minimal_stake;
-				better_score.minimal_stake += improvement;
-				let slightly_better = fake_solution(better_score);
+	#[test]
+	fn exact_same_score_is_rejected() {
+		ExtBuilder::verifier().build_and_execute(|| {
+			roll_to_snapshot_created();
 
-				assert_eq!(
-					<VerifierPallet as Verifier>::verify_synchronous(
-						slightly_better.solution_pages.first().cloned().unwrap(),
-						slightly_better.score,
-						MultiBlock::msp(),
-					)
-					.unwrap_err(),
-					FeasibilityError::ScoreTooLow
-				);
-			});
+			// Queue something useful.
+			let single_page = mine_solution(1).unwrap();
+			let _ = <VerifierPallet as Verifier>::verify_synchronous(
+				single_page.solution_pages.first().cloned().unwrap(),
+				single_page.score,
+				MultiBlock::msp(),
+			)
+			.unwrap();
+			assert_eq!(<VerifierPallet as Verifier>::queued_score(), Some(single_page.score));
+
+			// Now try and submit the exact same score again.
+			let same = fake_solution(single_page.score);
+
+			assert_eq!(
+				<VerifierPallet as Verifier>::verify_synchronous(
+					same.solution_pages.first().cloned().unwrap(),
+					same.score,
+					MultiBlock::msp(),
+				)
+				.unwrap_err(),
+				FeasibilityError::ScoreTooLow
+			);
+
+			assert_eq!(
+				verifier_events(),
+				vec![
+					Event::<Runtime>::Verified(2, 2),
+					Event::<Runtime>::Queued(single_page.score, None),
+					Event::<Runtime>::VerificationFailed(2, FeasibilityError::ScoreTooLow),
+				]
+			);
+		});
 	}
 
 	#[test]

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

@@ -1751,7 +1751,7 @@ impl<T: Config> Pallet<T> {
 			if i == 0 {
 				last_score = indice.0
 			} else {
-				if last_score.strict_threshold_better(indice.0, Perbill::zero()) {
+				if last_score.strict_better(indice.0) {
 					return Err(
 						"Signed submission indices vector must be ordered by election score".into()
 					)

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

@@ -838,9 +838,8 @@ impl<T: MinerConfig> Miner<T> {
 		// Ensure that the solution's score can pass absolute min-score.
 		let submitted_score = raw_solution.score;
 		ensure!(
-			minimum_untrusted_score.map_or(true, |min_score| {
-				submitted_score.strict_threshold_better(min_score, sp_runtime::Perbill::zero())
-			}),
+			minimum_untrusted_score
+				.map_or(true, |min_score| { submitted_score.strict_better(min_score) }),
 			FeasibilityError::UntrustedScoreTooLow
 		);
 

substrate/frame/staking-async/ahm-test/src/ah/mock.rs

@@ -283,7 +283,6 @@ impl multi_block::verifier::Config for Runtime {
 	type MaxWinnersPerPage = MaxWinnersPerPage;
 
 	type SolutionDataProvider = MultiBlockSigned;
-	type SolutionImprovementThreshold = ();
 	type WeightInfo = ();
 }