Some review grumbles.

bin/node/runtime/src/lib.rs

@@ -514,7 +514,8 @@ parameter_types! {
 	pub const SignedDepositByte: Balance = 1 * CENTS;
 
 	// fallback: no need to do on-chain phragmen initially.
-	pub const Fallback: pallet_two_phase_election_provider::FallbackStrategy = pallet_two_phase_election_provider::FallbackStrategy::Nothing;
+	pub const Fallback: pallet_two_phase_election_provider::FallbackStrategy =
+		pallet_two_phase_election_provider::FallbackStrategy::Nothing;
 
 	// unsigned configs
 	pub const TwoPhaseUnsignedPriority: TransactionPriority = StakingUnsignedPriority::get() - 1u64;
@@ -549,6 +550,7 @@ impl pallet_two_phase_election_provider::Config for Runtime {
 	type CompactSolution = pallet_staking::CompactAssignments;
 	type Fallback = Fallback;
 	type WeightInfo = pallet_two_phase_election_provider::weights::SubstrateWeight<Runtime>;
+	type BenchmarkingConfig = ();
 }
 
 parameter_types! {

frame/staking/Cargo.toml

@@ -17,7 +17,7 @@ static_assertions = "1.1.0"
 serde = { version = "1.0.101", optional = true }
 codec = { package = "parity-scale-codec", version = "1.3.6", default-features = false, features = ["derive"] }
 sp-std = { version = "2.0.0", default-features = false, path = "../../primitives/std" }
-# TODO: TWO_PHASE:: ideally we should be able to get rid of this.
+# TWO_PHASE_NOTE:: ideally we should be able to get rid of this.
 sp-npos-elections = { version = "2.0.0", default-features = false, path = "../../primitives/npos-elections" }
 sp-io ={ version = "2.0.0", default-features = false, path = "../../primitives/io" }
 sp-runtime = { version = "2.0.0", default-features = false, path = "../../primitives/runtime" }

frame/staking/src/lib.rs

@@ -1077,43 +1077,42 @@ decl_storage! {
 		/// The last planned session scheduled by the session pallet.
 		///
 		/// This is basically in sync with the call to [`SessionManager::new_session`].
-		/// TODO: TWO_PHASE: needs care to set the initial value upon migration.
 		pub CurrentPlannedSession get(fn current_planned_session): SessionIndex;
 
 		/// Snapshot of validators at the beginning of the current election window. This should only
 		/// have a value when [`EraElectionStatus`] == `ElectionStatus::Open(_)`.
 		///
-		/// TODO: TWO_PHASE: should be removed once we switch to two-phase.
+		/// TWO_PHASE_NOTE: should be removed once we switch to two-phase.
 		pub SnapshotValidators get(fn snapshot_validators): Option<Vec<T::AccountId>>;
 
 		/// Snapshot of nominators at the beginning of the current election window. This should only
 		/// have a value when [`EraElectionStatus`] == `ElectionStatus::Open(_)`.
 		///
-		/// TODO: TWO_PHASE: should be removed once we switch to two-phase.
+		/// TWO_PHASE_NOTE: should be removed once we switch to two-phase.
 		pub SnapshotNominators get(fn snapshot_nominators): Option<Vec<T::AccountId>>;
 
 		/// The next validator set. At the end of an era, if this is available (potentially from the
 		/// result of an offchain worker), it is immediately used. Otherwise, the on-chain election
 		/// is executed.
 		///
-		/// TODO: TWO_PHASE: should be removed once we switch to two-phase.
+		/// TWO_PHASE_NOTE: should be removed once we switch to two-phase.
 		pub QueuedElected get(fn queued_elected): Option<ElectionResult<T::AccountId, BalanceOf<T>>>;
 
 		/// The score of the current [`QueuedElected`].
 		///
-		/// TODO: TWO_PHASE: should be removed once we switch to two-phase.
+		/// TWO_PHASE_NOTE: should be removed once we switch to two-phase.
 		pub QueuedScore get(fn queued_score): Option<ElectionScore>;
 
 		/// Flag to control the execution of the offchain election. When `Open(_)`, we accept
 		/// solutions to be submitted.
 		///
-		/// TODO: TWO_PHASE: should be removed once we switch to two-phase.
+		/// TWO_PHASE_NOTE: should be removed once we switch to two-phase.
 		pub EraElectionStatus get(fn era_election_status): ElectionStatus<T::BlockNumber>;
 
 		/// True if the current **planned** session is final. Note that this does not take era
 		/// forcing into account.
 		///
-		/// TODO: TWO_PHASE: should be removed once we switch to two-phase.
+		/// TWO_PHASE_NOTE: should be removed once we switch to two-phase.
 		pub IsCurrentSessionFinal get(fn is_current_session_final): bool = false;
 
 		/// True if network has been upgraded to this version.
@@ -3028,7 +3027,7 @@ impl<T: Config> Module<T> {
 	///
 	/// Returns `Err(())` if less than [`MinimumValidatorCount`] validators have been elected, `Ok`
 	/// otherwise.
-	#[allow(dead_code)] // TODO: TWO_PHASE
+	#[allow(dead_code)] // TWO_PHASE_NOTE
 	pub fn process_election(
 		flat_supports: sp_npos_elections::Supports<T::AccountId>,
 		current_era: EraIndex,
@@ -3076,7 +3075,7 @@ impl<T: Config> Module<T> {
 		}
 
 		// emit event
-		// TODO: TWO_PHASE: remove the inner value.
+		// TWO_PHASE_NOTE: remove the inner value.
 		Self::deposit_event(RawEvent::StakingElection(ElectionCompute::Signed));
 
 		log!(
@@ -3099,7 +3098,7 @@ impl<T: Config> Module<T> {
 			"Experimental election provider outputted {:?}",
 			outcome
 		);
-		// TODO: TWO_PHASE: This code path shall not return anything for now. Later on, redirect the
+		// TWO_PHASE_NOTE: This code path shall not return anything for now. Later on, redirect the
 		// results to `process_election`.
 		None
 	}
@@ -3578,7 +3577,7 @@ where
 	}
 
 	fn can_report() -> bool {
-		// TODO: TWO_PHASE: we can get rid of this API
+		// TWO_PHASE_NOTE: we can get rid of this API
 		Self::era_election_status().is_closed()
 	}
 }

frame/staking/src/tests.rs

@@ -5036,7 +5036,6 @@ mod election_data_provider {
 					45
 				);
 				assert_eq!(staking_events().len(), 1);
-				// TODO: TWO_PHASE: remove the internal value.
 				assert_eq!(*staking_events().last().unwrap(), RawEvent::StakingElection(ElectionCompute::OnChain));
 
 				for b in 21..45 {
@@ -5054,7 +5053,6 @@ mod election_data_provider {
 					70
 				);
 				assert_eq!(staking_events().len(), 3);
-				// TODO: TWO_PHASE: remove the internal value.
 				assert_eq!(*staking_events().last().unwrap(), RawEvent::StakingElection(ElectionCompute::OnChain));
 			})
 	}

frame/two-phase-election-provider/src/benchmarking.rs

@@ -216,7 +216,7 @@ benchmarks! {
 
 	finalize_signed_phase_reject_solution {
 		let receiver = account("receiver", 0, SEED);
-		let initial_balance = T::Currency::minimum_balance() * 10u32.into();
+		let initial_balance = T::Currency::minimum_balance().max(One::one()) * 10u32.into();
 		let deposit: BalanceOf<T> = 10u32.into();
 		T::Currency::make_free_balance_be(&receiver, initial_balance);
 		assert_ok!(T::Currency::reserve(&receiver, deposit));
@@ -243,14 +243,14 @@ benchmarks! {
 		let c in 1 .. (T::MaxSignedSubmissions::get() - 1);
 
 		// the solution will be worse than all of them meaning the score need to be checked against all.
-		let solution = RawSolution { score: [(1000_0000u128 - 1).into(), 0, 0], ..Default::default() };
+		let solution = RawSolution { score: [(1_0000_000u128 - 1).into(), 0, 0], ..Default::default() };
 
 		<CurrentPhase<T>>::put(Phase::Signed);
 		<Round<T>>::put(1);
 
 		for i in 0..c {
 			<SignedSubmissions<T>>::mutate(|queue| {
-				let solution = RawSolution { score: [(1000_0000 + i).into(), 0, 0], ..Default::default() };
+				let solution = RawSolution { score: [(1_0000_000 + i).into(), 0, 0], ..Default::default() };
 				let signed_submission = SignedSubmission { solution, ..Default::default() };
 				// note: this is quite tricky: we know that the queue will stay sorted here. The
 				// last will be best.
@@ -268,14 +268,14 @@ benchmarks! {
 
 	submit_unsigned {
 		// number of votes in snapshot.
-		let v in 2000 .. 3000;
+		let v in 2000 .. T::BenchmarkingConfig::VOTERS[1];
 		// number of targets in snapshot.
-		let t in 500 .. 800;
+		let t in 500 .. T::BenchmarkingConfig::TARGETS[1];
 		// number of assignments, i.e. compact.len(). This means the active nominators, thus must be
 		// a subset of `v` component.
-		let a in 500 .. 1500;
+		let a in 500 .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
 		// number of desired targets. Must be a subset of `t` component.
-		let d in 200 .. 400;
+		let d in 200 .. T::BenchmarkingConfig::DESIRED_TARGETS[1];
 
 		let witness = SolutionSize { voters: v, targets: t };
 		let raw_solution = solution_with_size::<T>(witness, a, d);
@@ -290,14 +290,14 @@ benchmarks! {
 	// This is checking a valid solution. The worse case is indeed a valid solution.
 	feasibility_check {
 		// number of votes in snapshot.
-		let v in 2000 .. 3000;
+		let v in 2000 .. T::BenchmarkingConfig::VOTERS[1];
 		// number of targets in snapshot.
-		let t in 500 .. 800;
+		let t in 500 .. T::BenchmarkingConfig::TARGETS[1];
 		// number of assignments, i.e. compact.len(). This means the active nominators, thus must be
 		// a subset of `v` component.
-		let a in 500 .. 1500;
+		let a in 500 .. T::BenchmarkingConfig::ACTIVE_VOTERS[1];
 		// number of desired targets. Must be a subset of `t` component.
-		let d in 200 .. 400;
+		let d in 200 .. T::BenchmarkingConfig::DESIRED_TARGETS[1];
 
 		let size = SolutionSize { voters: v, targets: t };
 		let raw_solution = solution_with_size::<T>(size, a, d);
@@ -341,15 +341,11 @@ mod test {
 		});
 
 		ExtBuilder::default().build_and_execute(|| {
-			assert_ok!(test_benchmark_finalize_signed_phase_accept_solution::<
-				Runtime,
-			>());
+			assert_ok!(test_benchmark_finalize_signed_phase_accept_solution::<Runtime>());
 		});
 
 		ExtBuilder::default().build_and_execute(|| {
-			assert_ok!(test_benchmark_finalize_signed_phase_reject_solution::<
-				Runtime,
-			>());
+			assert_ok!(test_benchmark_finalize_signed_phase_reject_solution::<Runtime>());
 		});
 
 		ExtBuilder::default().build_and_execute(|| {

frame/two-phase-election-provider/src/lib.rs

@@ -255,6 +255,25 @@ where
 	type DataProvider = T::DataProvider;
 }
 
+/// Configuration for the benchmarks of the pallet.
+pub trait BenchmarkingConfig {
+	/// Range of voters.
+	const VOTERS: [u32; 2];
+	/// Range of targets.
+	const TARGETS: [u32; 2];
+	/// Range of active voters.
+	const ACTIVE_VOTERS: [u32; 2];
+	/// Range of desired targets.
+	const DESIRED_TARGETS: [u32; 2];
+}
+
+impl BenchmarkingConfig for () {
+	const VOTERS: [u32; 2] = [2000, 3000];
+	const TARGETS: [u32; 2] = [500, 800];
+	const ACTIVE_VOTERS: [u32; 2] = [500, 1500];
+	const DESIRED_TARGETS: [u32; 2] = [200, 400];
+}
+
 /// Current phase of the pallet.
 #[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, RuntimeDebug)]
 pub enum Phase<Bn> {
@@ -580,6 +599,9 @@ pub mod pallet {
 		/// Configuration for the fallback
 		type Fallback: Get<FallbackStrategy>;
 
+		/// The configuration of benchmarking.
+		type BenchmarkingConfig: BenchmarkingConfig;
+
 		/// The weight of the pallet.
 		type WeightInfo: WeightInfo;
 	}
@@ -710,10 +732,8 @@ pub mod pallet {
 
 			// ensure solution claims is better.
 			let mut signed_submissions = Self::signed_submissions();
-			let maybe_index =
-				Self::insert_submission(&who, &mut signed_submissions, solution, size);
-			ensure!(maybe_index.is_some(), Error::<T>::QueueFull);
-			let index = maybe_index.expect("Option checked to be `Some`; qed.");
+			let index = Self::insert_submission(&who, &mut signed_submissions, solution, size)
+				.ok_or(Error::<T>::QueueFull)?;
 
 			// collect deposit. Thereafter, the function cannot fail.
 			// Defensive -- index is valid.
@@ -723,8 +743,12 @@ pub mod pallet {
 				.unwrap_or_default();
 			T::Currency::reserve(&who, deposit).map_err(|_| Error::<T>::CannotPayDeposit)?;
 
-			// store the new signed submission.
+			// Remove the weakest, if needed.
+			if signed_submissions.len() as u32 > T::MaxSignedSubmissions::get() {
+				Self::remove_weakest(&mut signed_submissions);
+			}
 			debug_assert!(signed_submissions.len() as u32 <= T::MaxSignedSubmissions::get());
+
 			log!(
 				info,
 				"queued signed solution with (claimed) score {:?}",
@@ -734,6 +758,7 @@ pub mod pallet {
 					.unwrap_or_default()
 			);
 
+			// store the new signed submission.
 			<SignedSubmissions<T>>::put(signed_submissions);
 			Self::deposit_event(Event::SolutionStored(ElectionCompute::Signed));
 			Ok(None.into())

frame/two-phase-election-provider/src/mock.rs

@@ -310,6 +310,7 @@ impl crate::Config for Runtime {
 	type UnsignedPriority = UnsignedPriority;
 	type DataProvider = StakingMock;
 	type WeightInfo = DualMockWeightInfo;
+	type BenchmarkingConfig = ();
 	type OnChainAccuracy = Perbill;
 	type Fallback = Fallback;
 	type CompactSolution = TestCompact;

frame/two-phase-election-provider/src/signed.rs

@@ -136,10 +136,6 @@ where
 	/// solution quality. If insertion was successful, `Some(index)` is returned where index is the
 	/// index of the newly inserted item.
 	///
-	/// The length of the queue will always be kept less than or equal to `T::MaxSignedSubmissions`.
-	///
-	/// If a solution is removed, their bond is unreserved.
-	///
 	/// Invariant: The returned index is always a valid index in `queue` and can safely be used to
 	/// inspect the newly inserted element.
 	pub fn insert_submission(
@@ -184,23 +180,12 @@ where
 					// It is either 0 (in which case `0 <= queue.len()`) or one of the queue indices
 					// + 1. The biggest queue index is `queue.len() - 1`, thus `at <= queue.len()`.
 					queue.insert(at, submission);
-					// if length has exceeded the limit, eject the weakest, return shifted index.
-					if queue.len() as u32 > T::MaxSignedSubmissions::get() {
-						Self::remove_weakest(queue);
-						// Invariant: at > 0
-						// Proof by contradiction: Assume at is 0 and this will underflow. Then the
-						// previous length of the queue must have been `T::MaxSignedSubmissions` so
-						// that `queue.len() + 1` (for the newly inserted one) is more than
-						// `T::MaxSignedSubmissions`. But this would have been detected by the first
-						// if branch; qed.
-						Some(at - 1)
-					} else {
-						Some(at)
-					}
+					Some(at)
 				}
 			});
 
-		debug_assert!(queue.len() as u32 <= T::MaxSignedSubmissions::get());
+		// If the call site is sane and removes the weakest, then this must always be correct.
+		debug_assert!(queue.len() as u32 <= T::MaxSignedSubmissions::get() + 1);
 		outcome
 	}