From 4e1e09c2c4178066d16caa8c09a7f397ab13a0ec Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Wed, 10 Mar 2021 16:33:46 +0100
Subject: [PATCH 01/22] Trim compact solution for length during preparation

Starts addressing #8315
---
 .../election-provider-multi-phase/src/lib.rs  |  9 ++-
 .../src/unsigned.rs                           | 55 +++++++++++++++++++
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs
index 4ee6caae0a641..f7af5e6af78a7 100644
--- a/frame/election-provider-multi-phase/src/lib.rs
+++ b/frame/election-provider-multi-phase/src/lib.rs
@@ -519,12 +519,19 @@ pub mod pallet {
 		/// Maximum number of iteration of balancing that will be executed in the embedded miner of
 		/// the pallet.
 		type MinerMaxIterations: Get<u32>;
+
 		/// Maximum weight that the miner should consume.
 		///
 		/// The miner will ensure that the total weight of the unsigned solution will not exceed
-		/// this values, based on [`WeightInfo::submit_unsigned`].
+		/// this value, based on [`WeightInfo::submit_unsigned`].
 		type MinerMaxWeight: Get<Weight>;
 
+		/// Maximum length (bytes) that the mined solution should consume.
+		///
+		/// The miner will ensure that the total length of the unsigned solution will not exceed
+		/// this value.
+		type MinerMaxLength: Get<u32>;
+
 		/// Something that will provide the election data.
 		type DataProvider: ElectionDataProvider<Self::AccountId, Self::BlockNumber>;
 
diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 2039e5d9f0754..548889daf7b85 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -46,6 +46,8 @@ pub enum MinerError {
 	PreDispatchChecksFailed,
 	/// The solution generated from the miner is not feasible.
 	Feasibility(FeasibilityError),
+	/// There are no more voters to remove to trim the solution.
+	NoMoreVoters,
 }
 
 impl From<sp_npos_elections::Error> for MinerError {
@@ -166,6 +168,11 @@ impl<T: Config> Pallet<T> {
 			maximum_allowed_voters,
 		);
 		let compact = Self::trim_compact(maximum_allowed_voters, compact, &voter_index)?;
+		let compact = Self::trim_compact_for_length(
+			T::MinerMaxLength::get(),
+			compact,
+			&voter_index,
+		)?;
 
 		// re-calc score.
 		let winners = sp_npos_elections::to_without_backing(winners);
@@ -252,6 +259,54 @@ impl<T: Config> Pallet<T> {
 		}
 	}
 
+	/// Greedily reduce the size of the solution to fit into the block w.r.t length.
+	///
+	/// The length of the solution is largely a function of the number of voters. The number of
+	/// winners cannot be changed, and the `ElectionSize` is a proxy for the total number of stakers.
+	///
+	/// Thus, to reduce the solution size, we need to strip voters.
+	///
+	/// Note that this solution is already computed, and winners are elected based on the merit of
+	/// the total stake in the system. Nevertheless, some of the voters may be removed here.
+	///
+	/// The score must be computed **after** this step. If this step reduces the score too much,
+	/// then the solution must be discarded.
+	pub fn trim_compact_for_length(
+		max_allowed_length: u32,
+		mut compact: CompactOf<T>,
+		voter_index: impl Fn(&T::AccountId) -> Option<CompactVoterIndexOf<T>>,
+	) -> Result<CompactOf<T>, MinerError> {
+		// grab all voters and sort them by least stake.
+		let RoundSnapshot { voters, .. } =
+			Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?;
+		let mut voters_sorted = voters
+			.into_iter()
+			.map(|(who, stake, _)| (who.clone(), stake))
+			.collect::<Vec<_>>();
+		voters_sorted.sort_by_key(|(_, y)| *y);
+		voters_sorted.reverse();
+
+		// we run through the reduction process twice: once using the size_hint function, and once
+		// by actually encoding the data. Given a well-behaved size_hint implementation, this will
+		// only actually encode once. Given a misbehaving size_hint implementation, this still ensures
+		// that we accurately trim the solution.
+		let size_fns: [Box<dyn Fn(&CompactOf<T>) -> u32>; 2] = [
+			Box::new(|compact: &CompactOf<T>| compact.size_hint().saturated_into()),
+			Box::new(|compact: &CompactOf<T>| {
+				let encoded = compact.encode();
+				encoded.len().saturated_into()
+			}),
+		];
+		for size_fn in size_fns.iter() {
+			while size_fn(&compact) > max_allowed_length {
+				let (smallest_stake_voter, _) = voters_sorted.pop().ok_or(MinerError::NoMoreVoters)?;
+				let index = voter_index(&smallest_stake_voter).ok_or(MinerError::SnapshotUnAvailable)?;
+				compact.remove_voter(index);
+			}
+		}
+		Ok(compact)
+	}
+
 	/// Find the maximum `len` that a compact can have in order to fit into the block weight.
 	///
 	/// This only returns a value between zero and `size.nominators`.

From 79c339f94c4abdfc0c38a85943ce3c5343fc0a4e Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Fri, 12 Mar 2021 16:35:16 +0100
Subject: [PATCH 02/22] cause tests to compile

---
 frame/election-provider-multi-phase/src/mock.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs
index eb38a4cd52e95..e924f56c49fd1 100644
--- a/frame/election-provider-multi-phase/src/mock.rs
+++ b/frame/election-provider-multi-phase/src/mock.rs
@@ -202,6 +202,7 @@ parameter_types! {
 	pub static MinerTxPriority: u64 = 100;
 	pub static SolutionImprovementThreshold: Perbill = Perbill::zero();
 	pub static MinerMaxWeight: Weight = BlockWeights::get().max_block;
+	pub static MinerMaxLength: u32 = 256;
 	pub static MockWeightInfo: bool = false;
 
 
@@ -267,6 +268,7 @@ impl crate::Config for Runtime {
 	type SolutionImprovementThreshold = SolutionImprovementThreshold;
 	type MinerMaxIterations = MinerMaxIterations;
 	type MinerMaxWeight = MinerMaxWeight;
+	type MinerMaxLength = MinerMaxLength;
 	type MinerTxPriority = MinerTxPriority;
 	type DataProvider = StakingMock;
 	type WeightInfo = DualMockWeightInfo;

From d80a18cda81cd1d8e81f6456503dcfabe027dba7 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Fri, 12 Mar 2021 16:58:44 +0100
Subject: [PATCH 03/22] wip: add test for trim_compact_for_length

---
 .../src/unsigned.rs                           | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 548889daf7b85..7766d7fa4f32c 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -925,4 +925,26 @@ mod tests {
 			assert!(matches!(call, OuterCall::MultiPhase(Call::submit_unsigned(_, _))));
 		})
 	}
+
+	#[test]
+	fn trim_compact_for_length_does_not_modify_when_short_enough() {
+		let RawSolution { mut compact, .. } = raw_solution();
+		let encoded_len = compact.encode().len() as u32;
+		let compact_clone = compact.clone();
+
+		compact = MultiPhase::trim_compact_for_length(
+			encoded_len,
+			compact,
+			// helpers::voter_index_fn_linear appears to need an input we don't have
+			unimplemented!("how can I generate the voter index fn?"),
+		).unwrap();
+
+		assert_eq!(compact, compact_clone);
+	}
+
+	// TODO: trim_compact_for_length_modifies_when_too_long
+	// as above, but must trim at least 1 voter (contains >1 voter to start)
+
+	// TODO: trim_compact_for_length_errs_when_cannot_compact_enough
+	// as above, but returns an error
 }

From 212de4c627d1a90c827fb0478d77c499a152c8f2 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Mon, 15 Mar 2021 11:23:50 +0100
Subject: [PATCH 04/22] finish fn
 trim_compact_for_length_does_not_modify_when_short_enough

---
 .../src/unsigned.rs                           | 28 ++++++++++++-------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 7766d7fa4f32c..fcdf7a81d3be5 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -541,6 +541,7 @@ mod tests {
 		Call, *,
 	};
 	use frame_support::{dispatch::Dispatchable, traits::OffchainWorker};
+	use helpers::voter_index_fn_linear;
 	use mock::Call as OuterCall;
 	use sp_election_providers::Assignment;
 	use sp_runtime::{traits::ValidateUnsigned, PerU16};
@@ -928,18 +929,25 @@ mod tests {
 
 	#[test]
 	fn trim_compact_for_length_does_not_modify_when_short_enough() {
-		let RawSolution { mut compact, .. } = raw_solution();
-		let encoded_len = compact.encode().len() as u32;
-		let compact_clone = compact.clone();
+		let (mut ext, pool) = ExtBuilder::default().build_offchainify(0);
+		ext.execute_with(|| {
+			roll_to(25);
 
-		compact = MultiPhase::trim_compact_for_length(
-			encoded_len,
-			compact,
-			// helpers::voter_index_fn_linear appears to need an input we don't have
-			unimplemented!("how can I generate the voter index fn?"),
-		).unwrap();
+			let RoundSnapshot { voters, ..} =
+				MultiPhase::snapshot().unwrap();
+
+			let RawSolution { mut compact, .. } = raw_solution();
+			let encoded_len = compact.encode().len() as u32;
+			let compact_clone = compact.clone();
+
+			compact = MultiPhase::trim_compact_for_length(
+				encoded_len,
+				compact,
+				voter_index_fn_linear::<Runtime>(&voters),
+			).unwrap();
 
-		assert_eq!(compact, compact_clone);
+			assert_eq!(compact, compact_clone);
+		});
 	}
 
 	// TODO: trim_compact_for_length_modifies_when_too_long

From 53eef6b4c31ca1b15bed309c41004fb2bdee13d6 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Mon, 15 Mar 2021 11:28:15 +0100
Subject: [PATCH 05/22] add fn trim_compact_for_length_modifies_when_too_long

---
 .../src/unsigned.rs                           | 27 ++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index fcdf7a81d3be5..6bee839830cd0 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -929,7 +929,7 @@ mod tests {
 
 	#[test]
 	fn trim_compact_for_length_does_not_modify_when_short_enough() {
-		let (mut ext, pool) = ExtBuilder::default().build_offchainify(0);
+		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
 		ext.execute_with(|| {
 			roll_to(25);
 
@@ -950,8 +950,29 @@ mod tests {
 		});
 	}
 
-	// TODO: trim_compact_for_length_modifies_when_too_long
-	// as above, but must trim at least 1 voter (contains >1 voter to start)
+	#[test]
+	fn trim_compact_for_length_modifies_when_too_long() {
+		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
+		ext.execute_with(|| {
+			roll_to(25);
+
+			let RoundSnapshot { voters, ..} =
+				MultiPhase::snapshot().unwrap();
+
+			let RawSolution { mut compact, .. } = raw_solution();
+			let encoded_len = compact.encode().len() as u32;
+			let compact_clone = compact.clone();
+
+			compact = MultiPhase::trim_compact_for_length(
+				encoded_len - 1,
+				compact,
+				voter_index_fn_linear::<Runtime>(&voters),
+			).unwrap();
+
+			assert_ne!(compact, compact_clone);
+			assert!((compact.encode().len() as u32) < encoded_len);
+		});
+	}
 
 	// TODO: trim_compact_for_length_errs_when_cannot_compact_enough
 	// as above, but returns an error

From 78ddb641fdf6452b817d82778d45c377cd7d3008 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Mon, 15 Mar 2021 11:38:02 +0100
Subject: [PATCH 06/22] it's impractical to test for MinerError::NoMoreVoters

---
 frame/election-provider-multi-phase/src/unsigned.rs | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 6bee839830cd0..d9b590ddb8bb3 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -973,7 +973,4 @@ mod tests {
 			assert!((compact.encode().len() as u32) < encoded_len);
 		});
 	}
-
-	// TODO: trim_compact_for_length_errs_when_cannot_compact_enough
-	// as above, but returns an error
 }

From 909309c25fbf8f8780acbee47c13d6da80c9a118 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Mon, 15 Mar 2021 11:45:47 +0100
Subject: [PATCH 07/22] trim_compact_ naming consistency

---
 .../src/unsigned.rs                              | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index d9b590ddb8bb3..6925b507c41fb 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -167,8 +167,8 @@ impl<T: Config> Pallet<T> {
 			compact.voter_count(),
 			maximum_allowed_voters,
 		);
-		let compact = Self::trim_compact(maximum_allowed_voters, compact, &voter_index)?;
-		let compact = Self::trim_compact_for_length(
+		let compact = Self::trim_compact_weight(maximum_allowed_voters, compact, &voter_index)?;
+		let compact = Self::trim_compact_length(
 			T::MinerMaxLength::get(),
 			compact,
 			&voter_index,
@@ -215,7 +215,7 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Indeed, the score must be computed **after** this step. If this step reduces the score too
 	/// much or remove a winner, then the solution must be discarded **after** this step.
-	pub fn trim_compact<FN>(
+	pub fn trim_compact_weight<FN>(
 		maximum_allowed_voters: u32,
 		mut compact: CompactOf<T>,
 		voter_index: FN,
@@ -271,7 +271,7 @@ impl<T: Config> Pallet<T> {
 	///
 	/// The score must be computed **after** this step. If this step reduces the score too much,
 	/// then the solution must be discarded.
-	pub fn trim_compact_for_length(
+	pub fn trim_compact_length(
 		max_allowed_length: u32,
 		mut compact: CompactOf<T>,
 		voter_index: impl Fn(&T::AccountId) -> Option<CompactVoterIndexOf<T>>,
@@ -928,7 +928,7 @@ mod tests {
 	}
 
 	#[test]
-	fn trim_compact_for_length_does_not_modify_when_short_enough() {
+	fn trim_compact_length_does_not_modify_when_short_enough() {
 		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
 		ext.execute_with(|| {
 			roll_to(25);
@@ -940,7 +940,7 @@ mod tests {
 			let encoded_len = compact.encode().len() as u32;
 			let compact_clone = compact.clone();
 
-			compact = MultiPhase::trim_compact_for_length(
+			compact = MultiPhase::trim_compact_length(
 				encoded_len,
 				compact,
 				voter_index_fn_linear::<Runtime>(&voters),
@@ -951,7 +951,7 @@ mod tests {
 	}
 
 	#[test]
-	fn trim_compact_for_length_modifies_when_too_long() {
+	fn trim_compact_length_modifies_when_too_long() {
 		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
 		ext.execute_with(|| {
 			roll_to(25);
@@ -963,7 +963,7 @@ mod tests {
 			let encoded_len = compact.encode().len() as u32;
 			let compact_clone = compact.clone();
 
-			compact = MultiPhase::trim_compact_for_length(
+			compact = MultiPhase::trim_compact_length(
 				encoded_len - 1,
 				compact,
 				voter_index_fn_linear::<Runtime>(&voters),

From d05ce23bb1bca9543280e88246026264635ce112 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Mon, 15 Mar 2021 11:48:27 +0100
Subject: [PATCH 08/22] fix trim_compact_length docs copypasta error

---
 frame/election-provider-multi-phase/src/unsigned.rs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 6925b507c41fb..65199e9f0b133 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -262,9 +262,7 @@ impl<T: Config> Pallet<T> {
 	/// Greedily reduce the size of the solution to fit into the block w.r.t length.
 	///
 	/// The length of the solution is largely a function of the number of voters. The number of
-	/// winners cannot be changed, and the `ElectionSize` is a proxy for the total number of stakers.
-	///
-	/// Thus, to reduce the solution size, we need to strip voters.
+	/// winners cannot be changed. Thus, to reduce the solution size, we need to strip voters.
 	///
 	/// Note that this solution is already computed, and winners are elected based on the merit of
 	/// the total stake in the system. Nevertheless, some of the voters may be removed here.

From f0221dfb3631e21a812edf04eff9659a8380a263 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Mon, 15 Mar 2021 15:03:45 +0100
Subject: [PATCH 09/22] short-circuit length trim when known short enough

---
 frame/election-provider-multi-phase/src/unsigned.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 65199e9f0b133..9672b18aefee1 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -274,6 +274,12 @@ impl<T: Config> Pallet<T> {
 		mut compact: CompactOf<T>,
 		voter_index: impl Fn(&T::AccountId) -> Option<CompactVoterIndexOf<T>>,
 	) -> Result<CompactOf<T>, MinerError> {
+		// short-circuit to avoid getting the voters if possible
+		// this involves a redundant encoding, but that should hopefully be relatively cheap
+		if (compact.encode().len().saturated_into::<u32>()) < max_allowed_length {
+			return Ok(compact);
+		}
+
 		// grab all voters and sort them by least stake.
 		let RoundSnapshot { voters, .. } =
 			Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?;

From 99fdfae883433c09c53ad0db5a5feb0d108e5817 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Mon, 15 Mar 2021 16:49:18 +0100
Subject: [PATCH 10/22] set a default MaxMinerLength value for the node

---
 bin/node/runtime/src/lib.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
index 55f42b5723ba8..18a1d579a38a9 100644
--- a/bin/node/runtime/src/lib.rs
+++ b/bin/node/runtime/src/lib.rs
@@ -529,6 +529,7 @@ parameter_types! {
 		.get(DispatchClass::Normal)
 		.max_extrinsic.expect("Normal extrinsics have a weight limit configured; qed")
 		.saturating_sub(BlockExecutionWeight::get());
+	pub MinerMaxLength: u32 = 2 * 1024 * 1024; // 2Mb
 }
 
 impl pallet_election_provider_multi_phase::Config for Runtime {
@@ -539,6 +540,7 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
 	type SolutionImprovementThreshold = MinSolutionScoreBump;
 	type MinerMaxIterations = MinerMaxIterations;
 	type MinerMaxWeight = MinerMaxWeight;
+	type MinerMaxLength = MinerMaxLength;
 	type MinerTxPriority = MultiPhaseUnsignedPriority;
 	type DataProvider = Staking;
 	type OnChainAccuracy = Perbill;

From d41b07f42074e9d45d1cd615b13dcb29891713bf Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Mon, 22 Mar 2021 15:41:04 +0100
Subject: [PATCH 11/22] simplify size calculation for reduction function

The previous implementation had at least two major issues:

- Depending on whether or not `size_hint` was implemented correctly
  for a given type, the impl might fall back to relatively expensive
  full encoding for each iteration. Even with a well-behaved `size_hint`,
  it would perform a full encoding at least once.

  The situation wasn't helped by the fact that the default derived
  implementation of `size_hint` returns 0 unconditionally.

- If the implementation of `size_hint` overreported expected size,
  the implementation might remove more voters than actually necessary.

An ideal implementation would use a sizing function which produced
the exact size from nothing more than addition and multiplication.
In principle, we have all the information we'd need to write such a
function. However, that would take a lot of work.

What this commit does is instead use `encoded_size`, which is a non-
allocating hack which recursively encodes all fields, but then doesn't
actually write them. It should be faster than a full encode, without
sacrificing accuracy.
---
 .../src/unsigned.rs                           | 22 +++++--------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 417f2466ba73a..b3dd794a105ba 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -299,24 +299,12 @@ impl<T: Config> Pallet<T> {
 		voters_sorted.sort_by_key(|(_, y)| *y);
 		voters_sorted.reverse();
 
-		// we run through the reduction process twice: once using the size_hint function, and once
-		// by actually encoding the data. Given a well-behaved size_hint implementation, this will
-		// only actually encode once. Given a misbehaving size_hint implementation, this still ensures
-		// that we accurately trim the solution.
-		let size_fns: [Box<dyn Fn(&CompactOf<T>) -> u32>; 2] = [
-			Box::new(|compact: &CompactOf<T>| compact.size_hint().saturated_into()),
-			Box::new(|compact: &CompactOf<T>| {
-				let encoded = compact.encode();
-				encoded.len().saturated_into()
-			}),
-		];
-		for size_fn in size_fns.iter() {
-			while size_fn(&compact) > max_allowed_length {
-				let (smallest_stake_voter, _) = voters_sorted.pop().ok_or(MinerError::NoMoreVoters)?;
-				let index = voter_index(&smallest_stake_voter).ok_or(MinerError::SnapshotUnAvailable)?;
-				compact.remove_voter(index);
-			}
+		while compact.encoded_size() > max_allowed_length.saturated_into() {
+			let (smallest_stake_voter, _) = voters_sorted.pop().ok_or(MinerError::NoMoreVoters)?;
+			let index = voter_index(&smallest_stake_voter).ok_or(MinerError::SnapshotUnAvailable)?;
+			compact.remove_voter(index);
 		}
+
 		Ok(compact)
 	}
 

From 12519d5bf56f8a350b770a54fc9a21ee49ea996e Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
Date: Tue, 23 Mar 2021 15:06:07 +0100
Subject: [PATCH 12/22] use more efficient short-circuit check

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
---
 frame/election-provider-multi-phase/src/unsigned.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index e5aeb9fa9752a..5bfa91b5501c2 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -285,7 +285,7 @@ impl<T: Config> Pallet<T> {
 	) -> Result<CompactOf<T>, MinerError> {
 		// short-circuit to avoid getting the voters if possible
 		// this involves a redundant encoding, but that should hopefully be relatively cheap
-		if (compact.encode().len().saturated_into::<u32>()) < max_allowed_length {
+		if (compact.encoded_size().saturated_into::<u32>()) <= max_allowed_length {
 			return Ok(compact);
 		}
 

From c230d911c970f9735de5201be4169eeeef084d7e Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Wed, 24 Mar 2021 10:08:26 +0100
Subject: [PATCH 13/22] use a real value for node MinerMaxLength

---
 bin/node/runtime/src/lib.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
index c7a2602add622..b5619150a5bfc 100644
--- a/bin/node/runtime/src/lib.rs
+++ b/bin/node/runtime/src/lib.rs
@@ -513,7 +513,11 @@ parameter_types! {
 		.get(DispatchClass::Normal)
 		.max_extrinsic.expect("Normal extrinsics have a weight limit configured; qed")
 		.saturating_sub(BlockExecutionWeight::get());
-	pub MinerMaxLength: u32 = 2 * 1024 * 1024; // 2Mb
+	// Solution can occupy 90% of normal block size
+	pub MinerMaxLength: u32 = RuntimeBlockLength::get()
+		.max
+		.get(DispatchClass::Normal)
+		.saturating_mul(9) / 10;
 }
 
 impl pallet_election_provider_multi_phase::Config for Runtime {

From da74f0ae365b5ff04b1bc535a317b12ec1d24ca4 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Wed, 24 Mar 2021 11:24:18 +0100
Subject: [PATCH 14/22] add test that trimming compact for length reduces the
 compact solution size

---
 .../src/unsigned.rs                           | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 5bfa91b5501c2..9f5b33de2d0d2 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -976,4 +976,28 @@ mod tests {
 			assert!((compact.encode().len() as u32) < encoded_len);
 		});
 	}
+
+	// all the other solution-generation functions end up delegating to `mine_solution`, so if we
+	// demonstrate that `mine_solution` solutions are all trimmed to an acceptable length, then
+	// we know that higher-level functions will all also have short-enough solutions.
+	#[test]
+	fn mine_solution_solutions_always_within_acceptable_length() {
+		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
+		ext.execute_with(|| {
+			roll_to(25);
+
+			// how long would the default solution be?
+			let solution = MultiPhase::mine_solution(0).unwrap();
+			let max_length = <Runtime as Config>::MinerMaxLength::get();
+			let solution_size = solution.0.compact.encoded_size();
+			assert!(solution_size <= max_length as usize);
+
+			// now set the max size to less than the actual size and regenerate
+			<Runtime as Config>::MinerMaxLength::set(solution_size as u32 - 1);
+			let solution = MultiPhase::mine_solution(0).unwrap();
+			let max_length = <Runtime as Config>::MinerMaxLength::get();
+			let solution_size = solution.0.compact.encoded_size();
+			assert!(solution_size <= max_length as usize);
+		});
+	}
 }

From b39dac102ffc5727df171190ac8fa69bc10d5be4 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Fri, 26 Mar 2021 14:40:20 +0100
Subject: [PATCH 15/22] remove possibility of saturation in MinerMaxLength
 calculation

---
 bin/node/runtime/src/lib.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
index a097965ff37bb..0e9af8a816439 100644
--- a/bin/node/runtime/src/lib.rs
+++ b/bin/node/runtime/src/lib.rs
@@ -514,10 +514,10 @@ parameter_types! {
 		.max_extrinsic.expect("Normal extrinsics have a weight limit configured; qed")
 		.saturating_sub(BlockExecutionWeight::get());
 	// Solution can occupy 90% of normal block size
-	pub MinerMaxLength: u32 = RuntimeBlockLength::get()
+	pub MinerMaxLength: u32 = Perbill::from_rational(9u32, 10) *
+		*RuntimeBlockLength::get()
 		.max
-		.get(DispatchClass::Normal)
-		.saturating_mul(9) / 10;
+		.get(DispatchClass::Normal);
 }
 
 impl pallet_election_provider_multi_phase::Config for Runtime {

From c4fe6b4fbceb1aff0b1b06217b99babbe12cfff5 Mon Sep 17 00:00:00 2001
From: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Date: Mon, 29 Mar 2021 11:16:55 +0200
Subject: [PATCH 16/22] Update
 frame/election-provider-multi-phase/src/unsigned.rs

---
 frame/election-provider-multi-phase/src/unsigned.rs | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 9f5b33de2d0d2..117f37ffcbffb 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -275,6 +275,9 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Note that this solution is already computed, and winners are elected based on the merit of
 	/// the total stake in the system. Nevertheless, some of the voters may be removed here.
+	/// Sometimes, removing a voter can cause a validator to also be implicitely remoted, if 
+	/// that voter was the only backer of that winner. In such cases, this solution is invalid, which
+	/// will be caught prior to submission. 
 	///
 	/// The score must be computed **after** this step. If this step reduces the score too much,
 	/// then the solution must be discarded.

From 89c32c3432b6a73962e1ef51a55592652d9f2ba1 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
Date: Tue, 30 Mar 2021 11:43:36 +0200
Subject: [PATCH 17/22] Apply suggestions from code review

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
---
 frame/election-provider-multi-phase/src/unsigned.rs | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 065ec88a5ac90..6a2dd96c6e93a 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -947,8 +947,7 @@ mod tests {
 		ext.execute_with(|| {
 			roll_to(25);
 
-			let RoundSnapshot { voters, ..} =
-				MultiPhase::snapshot().unwrap();
+			let RoundSnapshot { voters, ..} = MultiPhase::snapshot().unwrap();
 
 			let RawSolution { mut compact, .. } = raw_solution();
 			let encoded_len = compact.encode().len() as u32;

From 56514573b5b7670416264b0ce5d575f84dd31f6e Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Tue, 30 Mar 2021 11:46:55 +0200
Subject: [PATCH 18/22] use encoded_size instead of encode().len() in test

---
 frame/election-provider-multi-phase/src/unsigned.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 6a2dd96c6e93a..85dbd36bc2ba4 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -281,9 +281,9 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Note that this solution is already computed, and winners are elected based on the merit of
 	/// the total stake in the system. Nevertheless, some of the voters may be removed here.
-	/// Sometimes, removing a voter can cause a validator to also be implicitely remoted, if 
+	/// Sometimes, removing a voter can cause a validator to also be implicitely remoted, if
 	/// that voter was the only backer of that winner. In such cases, this solution is invalid, which
-	/// will be caught prior to submission. 
+	/// will be caught prior to submission.
 	///
 	/// The score must be computed **after** this step. If this step reduces the score too much,
 	/// then the solution must be discarded.
@@ -983,7 +983,7 @@ mod tests {
 			).unwrap();
 
 			assert_ne!(compact, compact_clone);
-			assert!((compact.encode().len() as u32) < encoded_len);
+			assert!((compact.encoded_size() as u32) < encoded_len);
 		});
 	}
 

From 5b5257220ad182d02c60b7edc80b229efd6d3a2d Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Tue, 30 Mar 2021 11:50:45 +0200
Subject: [PATCH 19/22] don't unnecessarily offchainify tests

---
 frame/election-provider-multi-phase/src/unsigned.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 85dbd36bc2ba4..17d68f2f03e4e 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -943,7 +943,7 @@ mod tests {
 
 	#[test]
 	fn trim_compact_length_does_not_modify_when_short_enough() {
-		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
+		let mut ext = ExtBuilder::default().build();
 		ext.execute_with(|| {
 			roll_to(25);
 
@@ -965,7 +965,7 @@ mod tests {
 
 	#[test]
 	fn trim_compact_length_modifies_when_too_long() {
-		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
+		let mut ext = ExtBuilder::default().build();
 		ext.execute_with(|| {
 			roll_to(25);
 
@@ -992,7 +992,7 @@ mod tests {
 	// we know that higher-level functions will all also have short-enough solutions.
 	#[test]
 	fn mine_solution_solutions_always_within_acceptable_length() {
-		let (mut ext, _) = ExtBuilder::default().build_offchainify(0);
+		let mut ext = ExtBuilder::default().build();
 		ext.execute_with(|| {
 			roll_to(25);
 

From f86ae653b4c77c37722d4d15168e30e8717b181b Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Tue, 30 Mar 2021 12:03:44 +0200
Subject: [PATCH 20/22] reorganize trim noop test

---
 frame/election-provider-multi-phase/src/unsigned.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 17d68f2f03e4e..7148b958208e7 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -947,18 +947,21 @@ mod tests {
 		ext.execute_with(|| {
 			roll_to(25);
 
+			// given
 			let RoundSnapshot { voters, ..} = MultiPhase::snapshot().unwrap();
-
 			let RawSolution { mut compact, .. } = raw_solution();
 			let encoded_len = compact.encode().len() as u32;
 			let compact_clone = compact.clone();
 
+			// when
+			assert!(encoded_len < <Runtime as Config>::MinerMaxLength::get());
+
+			// then
 			compact = MultiPhase::trim_compact_length(
 				encoded_len,
 				compact,
 				voter_index_fn_linear::<Runtime>(&voters),
 			).unwrap();
-
 			assert_eq!(compact, compact_clone);
 		});
 	}

From 2b68b836344025c31c0ca4c18b62440392e8ef04 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Thu, 1 Apr 2021 15:02:49 +0200
Subject: [PATCH 21/22] typos

---
 frame/election-provider-multi-phase/src/unsigned.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 7148b958208e7..7ce17f4ed55ee 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -281,7 +281,8 @@ impl<T: Config> Pallet<T> {
 	///
 	/// Note that this solution is already computed, and winners are elected based on the merit of
 	/// the total stake in the system. Nevertheless, some of the voters may be removed here.
-	/// Sometimes, removing a voter can cause a validator to also be implicitely remoted, if
+	///
+	/// Sometimes, removing a voter can cause a validator to also be implicitly removed, if
 	/// that voter was the only backer of that winner. In such cases, this solution is invalid, which
 	/// will be caught prior to submission.
 	///

From 4c9f8d7d6d622a0c4d62d6de26afefbbfce10f89 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <peter.r.goodspeedniklaus@gmail.com>
Date: Thu, 1 Apr 2021 16:03:38 +0200
Subject: [PATCH 22/22] add test that trimming by length trims lowest stake

---
 .../src/unsigned.rs                           | 41 ++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs
index 7ce17f4ed55ee..1a88d6eee5e6e 100644
--- a/frame/election-provider-multi-phase/src/unsigned.rs
+++ b/frame/election-provider-multi-phase/src/unsigned.rs
@@ -977,7 +977,7 @@ mod tests {
 				MultiPhase::snapshot().unwrap();
 
 			let RawSolution { mut compact, .. } = raw_solution();
-			let encoded_len = compact.encode().len() as u32;
+			let encoded_len = compact.encoded_size() as u32;
 			let compact_clone = compact.clone();
 
 			compact = MultiPhase::trim_compact_length(
@@ -991,6 +991,45 @@ mod tests {
 		});
 	}
 
+	#[test]
+	fn trim_compact_length_trims_lowest_stake() {
+		let mut ext = ExtBuilder::default().build();
+		ext.execute_with(|| {
+			roll_to(25);
+
+			let RoundSnapshot { voters, ..} =
+				MultiPhase::snapshot().unwrap();
+
+			let RawSolution { mut compact, .. } = raw_solution();
+			let encoded_len = compact.encoded_size() as u32;
+			let voter_count = compact.voter_count();
+			let min_stake_voter = voters.iter()
+				.map(|(id, weight, _)| (weight, id))
+				.min()
+				.map(|(_, id)| id)
+				.unwrap();
+
+
+			compact = MultiPhase::trim_compact_length(
+				encoded_len - 1,
+				compact,
+				voter_index_fn_linear::<Runtime>(&voters),
+			).unwrap();
+
+			assert_eq!(compact.voter_count(), voter_count - 1, "we must have removed exactly 1 voter");
+
+			let assignments = compact.into_assignment(
+				|voter| Some(voter as AccountId),
+				|target| Some(target as AccountId),
+			).unwrap();
+			assert!(
+				assignments.iter()
+					.all(|Assignment{ who, ..}| who != min_stake_voter),
+				"min_stake_voter must no longer be in the set of voters",
+			);
+		});
+	}
+
 	// all the other solution-generation functions end up delegating to `mine_solution`, so if we
 	// demonstrate that `mine_solution` solutions are all trimmed to an acceptable length, then
 	// we know that higher-level functions will all also have short-enough solutions.