Allow pallet-election-provider to accept smaller solutions as well (#10905)

* Allow `pallet-election-provider` to accept smaller
solutions, issue #9478

* Fixing a typo

* Adding some more tests
Removing a seemingly outdated comment

* making it a URL

* Updating test name as per suggestion

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Updating documentation to be more explicit

And to follow the general guidelines

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Fixing formatting

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

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

@@ -1346,7 +1346,7 @@ impl<T: Config> Pallet<T> {
 			T::DataProvider::targets(Some(target_limit)).map_err(ElectionError::DataProvider)?;
 		let voters =
 			T::DataProvider::voters(Some(voter_limit)).map_err(ElectionError::DataProvider)?;
-		let desired_targets =
+		let mut desired_targets =
 			T::DataProvider::desired_targets().map_err(ElectionError::DataProvider)?;
 
 		// Defensive-only.
@@ -1355,6 +1355,22 @@ impl<T: Config> Pallet<T> {
 			return Err(ElectionError::DataProvider("Snapshot too big for submission."))
 		}
 
+		// If `desired_targets` > `targets.len()`, cap `desired_targets` to that level and emit a
+		// warning
+		let max_len = targets
+			.len()
+			.try_into()
+			.map_err(|_| ElectionError::DataProvider("Failed to convert usize"))?;
+		if desired_targets > max_len {
+			log!(
+				warn,
+				"desired_targets: {} > targets.len(): {}, capping desired_targets",
+				desired_targets,
+				max_len
+			);
+			desired_targets = max_len;
+		}
+
 		Ok((targets, voters, desired_targets))
 	}
 
@@ -1413,9 +1429,6 @@ impl<T: Config> Pallet<T> {
 		let desired_targets =
 			Self::desired_targets().ok_or(FeasibilityError::SnapshotUnavailable)?;
 
-		// NOTE: this is a bit of duplicate, but we keep it around for veracity. The unsigned path
-		// already checked this in `unsigned_per_dispatch_checks`. The signed path *could* check it
-		// upon arrival, thus we would then remove it here. Given overlay it is cheap anyhow
 		ensure!(winners.len() as u32 == desired_targets, FeasibilityError::WrongWinnerCount);
 
 		// Ensure that the solution's score can pass absolute min-score.
@@ -1588,7 +1601,7 @@ mod feasibility_check {
 		raw_solution, roll_to, EpochLength, ExtBuilder, MultiPhase, Runtime, SignedPhase,
 		TargetIndex, UnsignedPhase, VoterIndex,
 	};
-	use frame_support::assert_noop;
+	use frame_support::{assert_noop, assert_ok};
 
 	const COMPUTE: ElectionCompute = ElectionCompute::OnChain;
 
@@ -1625,7 +1638,7 @@ mod feasibility_check {
 	}
 
 	#[test]
-	fn desired_targets() {
+	fn desired_targets_gets_capped() {
 		ExtBuilder::default().desired_targets(8).build_and_execute(|| {
 			roll_to(<EpochLength>::get() - <SignedPhase>::get() - <UnsignedPhase>::get());
 			assert!(MultiPhase::current_phase().is_signed());
@@ -1633,8 +1646,30 @@ mod feasibility_check {
 			let raw = raw_solution();
 
 			assert_eq!(raw.solution.unique_targets().len(), 4);
-			assert_eq!(MultiPhase::desired_targets().unwrap(), 8);
+			// desired_targets is capped to the number of targets which is 4
+			assert_eq!(MultiPhase::desired_targets().unwrap(), 4);
 
+			// It should succeed
+			assert_ok!(MultiPhase::feasibility_check(raw, COMPUTE));
+		})
+	}
+
+	#[test]
+	fn less_than_desired_targets_fails() {
+		ExtBuilder::default().desired_targets(8).build_and_execute(|| {
+			roll_to(<EpochLength>::get() - <SignedPhase>::get() - <UnsignedPhase>::get());
+			assert!(MultiPhase::current_phase().is_signed());
+
+			let mut raw = raw_solution();
+
+			assert_eq!(raw.solution.unique_targets().len(), 4);
+			// desired_targets is capped to the number of targets which is 4
+			assert_eq!(MultiPhase::desired_targets().unwrap(), 4);
+
+			// Force the number of winners to be bigger to fail
+			raw.solution.votes1[0].1 = 4;
+
+			// It should succeed
 			assert_noop!(
 				MultiPhase::feasibility_check(raw, COMPUTE),
 				FeasibilityError::WrongWinnerCount,

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

@@ -1044,8 +1044,13 @@ mod tests {
 			roll_to(25);
 			assert!(MultiPhase::current_phase().is_unsigned());
 
+			// Force the number of winners to be bigger to fail
+			let (mut solution, _) =
+				MultiPhase::mine_solution::<<Runtime as Config>::Solver>().unwrap();
+			solution.solution.votes1[0].1 = 4;
+
 			assert_eq!(
-				MultiPhase::mine_check_save_submit().unwrap_err(),
+				MultiPhase::basic_checks(&solution, "mined").unwrap_err(),
 				MinerError::PreDispatchChecksFailed(DispatchError::Module(ModuleError {
 					index: 2,
 					error: 1,

substrate/frame/election-provider-support/src/lib.rs

@@ -289,6 +289,12 @@ pub trait ElectionDataProvider {
 	///
 	/// This should be implemented as a self-weighing function. The implementor should register its
 	/// appropriate weight at the end of execution with the system pallet directly.
+	///
+	/// A sensible implementation should use the minimum between this value and
+	/// [`Self::targets().len()`], since desiring a winner set larger than candidates is not
+	/// feasible.
+	///
+	/// This is documented further in issue: <https://github.com/paritytech/substrate/issues/9478>
 	fn desired_targets() -> data_provider::Result<u32>;
 
 	/// Provide a best effort prediction about when the next election is about to happen.