diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index b366ceb4da..90c540b85d 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -1346,7 +1346,7 @@ impl Pallet { 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 Pallet { 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 Pallet { 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(::get() - ::get() - ::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(::get() - ::get() - ::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, diff --git a/substrate/frame/election-provider-multi-phase/src/unsigned.rs b/substrate/frame/election-provider-multi-phase/src/unsigned.rs index 3380a61d43..c52a4da22c 100644 --- a/substrate/frame/election-provider-multi-phase/src/unsigned.rs +++ b/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::<::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, diff --git a/substrate/frame/election-provider-support/src/lib.rs b/substrate/frame/election-provider-support/src/lib.rs index ca95eda6b5..e1975a2681 100644 --- a/substrate/frame/election-provider-support/src/lib.rs +++ b/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: fn desired_targets() -> data_provider::Result; /// Provide a best effort prediction about when the next election is about to happen.