From ed7f27918f53286ae689a6f95b31d6b74523becc Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Thu, 21 Sep 2023 11:00:30 +0200 Subject: [PATCH 1/5] pallet epm: add `TrimmingStatus` to solution For tools such that is using the `Miner` it's useful to know whether a solution was trimmed or not. --- .../election-provider-multi-phase/src/lib.rs | 4 +- .../src/signed.rs | 2 +- .../src/unsigned.rs | 90 +++++++++++++------ 3 files changed, 65 insertions(+), 31 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/src/lib.rs b/substrate/frame/election-provider-multi-phase/src/lib.rs index 0d751e3f9cb0..00a3d15e520a 100644 --- a/substrate/frame/election-provider-multi-phase/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/src/lib.rs @@ -2364,7 +2364,7 @@ mod tests { assert_eq!(MultiPhase::desired_targets().unwrap(), 2); // mine seq_phragmen solution with 2 iters. - let (solution, witness) = MultiPhase::mine_solution().unwrap(); + let (solution, witness, _) = MultiPhase::mine_solution().unwrap(); // ensure this solution is valid. assert!(MultiPhase::queued_solution().is_none()); @@ -2646,7 +2646,7 @@ mod tests { // set the solution balancing to get the desired score. crate::mock::Balancing::set(Some(BalancingConfig { iterations: 2, tolerance: 0 })); - let (solution, _) = MultiPhase::mine_solution().unwrap(); + let (solution, _, _) = MultiPhase::mine_solution().unwrap(); // Default solution's score. assert!(matches!(solution.score, ElectionScore { minimal_stake: 50, .. })); diff --git a/substrate/frame/election-provider-multi-phase/src/signed.rs b/substrate/frame/election-provider-multi-phase/src/signed.rs index 76068ba99d36..84a39c94a46e 100644 --- a/substrate/frame/election-provider-multi-phase/src/signed.rs +++ b/substrate/frame/election-provider-multi-phase/src/signed.rs @@ -1269,7 +1269,7 @@ mod tests { roll_to_signed(); assert!(MultiPhase::current_phase().is_signed()); - let (raw, witness) = MultiPhase::mine_solution().unwrap(); + let (raw, witness, _) = MultiPhase::mine_solution().unwrap(); let solution_weight = ::solution_weight( witness.voters, witness.targets, diff --git a/substrate/frame/election-provider-multi-phase/src/unsigned.rs b/substrate/frame/election-provider-multi-phase/src/unsigned.rs index f1c9e92a704e..1ed6039a0954 100644 --- a/substrate/frame/election-provider-multi-phase/src/unsigned.rs +++ b/substrate/frame/election-provider-multi-phase/src/unsigned.rs @@ -108,6 +108,27 @@ impl From for MinerError { } } +/// Reports the trimming result of mined solution +#[derive(Debug, Clone)] +pub struct TrimmingStatus { + weight: usize, + length: usize, +} + +impl TrimmingStatus { + pub fn is_trimmed(&self) -> bool { + self.weight + self.length > 0 + } + + pub fn trimmed_weight(&self) -> usize { + self.weight + } + + pub fn trimmed_length(&self) -> usize { + self.length + } +} + /// Save a given call into OCW storage. fn save_solution(call: &Call) -> Result<(), MinerError> { log!(debug, "saving a call to the offchain storage."); @@ -162,16 +183,21 @@ impl Pallet { /// /// The Npos Solver type, `S`, must have the same AccountId and Error type as the /// [`crate::Config::Solver`] in order to create a unified return type. - pub fn mine_solution( - ) -> Result<(RawSolution>, SolutionOrSnapshotSize), MinerError> { + pub fn mine_solution() -> Result< + (RawSolution>, SolutionOrSnapshotSize, TrimmingStatus), + MinerError, + > { let RoundSnapshot { voters, targets } = Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?; let desired_targets = Self::desired_targets().ok_or(MinerError::SnapshotUnAvailable)?; - let (solution, score, size) = Miner::::mine_solution_with_snapshot::< - T::Solver, - >(voters, targets, desired_targets)?; + let (solution, score, size, is_trimmed) = + Miner::::mine_solution_with_snapshot::( + voters, + targets, + desired_targets, + )?; let round = Self::round(); - Ok((RawSolution { solution, score, round }, size)) + Ok((RawSolution { solution, score, round }, size, is_trimmed)) } /// Attempt to restore a solution from cache. Otherwise, compute it fresh. Either way, submit @@ -232,7 +258,7 @@ impl Pallet { /// Mine a new solution as a call. Performs all checks. pub fn mine_checked_call() -> Result, MinerError> { // get the solution, with a load of checks to ensure if submitted, IT IS ABSOLUTELY VALID. - let (raw_solution, witness) = Self::mine_and_check()?; + let (raw_solution, witness, _) = Self::mine_and_check()?; let score = raw_solution.score; let call: Call = Call::submit_unsigned { raw_solution: Box::new(raw_solution), witness }; @@ -282,11 +308,13 @@ impl Pallet { /// If you want an unchecked solution, use [`Pallet::mine_solution`]. /// If you want a checked solution and submit it at the same time, use /// [`Pallet::mine_check_save_submit`]. - pub fn mine_and_check( - ) -> Result<(RawSolution>, SolutionOrSnapshotSize), MinerError> { - let (raw_solution, witness) = Self::mine_solution()?; + pub fn mine_and_check() -> Result< + (RawSolution>, SolutionOrSnapshotSize, TrimmingStatus), + MinerError, + > { + let (raw_solution, witness, is_trimmed) = Self::mine_solution()?; Self::basic_checks(&raw_solution, "mined")?; - Ok((raw_solution, witness)) + Ok((raw_solution, witness, is_trimmed)) } /// Checks if an execution of the offchain worker is permitted at the given block number, or @@ -408,7 +436,7 @@ impl Miner { voters: Vec<(T::AccountId, VoteWeight, BoundedVec)>, targets: Vec, desired_targets: u32, - ) -> Result<(SolutionOf, ElectionScore, SolutionOrSnapshotSize), MinerError> + ) -> Result<(SolutionOf, ElectionScore, SolutionOrSnapshotSize, TrimmingStatus), MinerError> where S: NposSolver, { @@ -436,7 +464,7 @@ impl Miner { voters: Vec<(T::AccountId, VoteWeight, BoundedVec)>, targets: Vec, desired_targets: u32, - ) -> Result<(SolutionOf, ElectionScore, SolutionOrSnapshotSize), MinerError> { + ) -> Result<(SolutionOf, ElectionScore, SolutionOrSnapshotSize, TrimmingStatus), MinerError> { // now make some helper closures. let cache = helpers::generate_voter_cache::(&voters); let voter_index = helpers::voter_index_fn::(&cache); @@ -495,13 +523,13 @@ impl Miner { // trim assignments list for weight and length. let size = SolutionOrSnapshotSize { voters: voters.len() as u32, targets: targets.len() as u32 }; - Self::trim_assignments_weight( + let weight_trimmed = Self::trim_assignments_weight( desired_targets, size, T::MaxWeight::get(), &mut index_assignments, ); - Self::trim_assignments_length( + let length_trimmed = Self::trim_assignments_length( T::MaxLength::get(), &mut index_assignments, &encoded_size_of, @@ -513,7 +541,9 @@ impl Miner { // re-calc score. let score = solution.clone().score(stake_of, voter_at, target_at)?; - Ok((solution, score, size)) + let is_trimmed = TrimmingStatus { weight: weight_trimmed, length: length_trimmed }; + + Ok((solution, score, size, is_trimmed)) } /// Greedily reduce the size of the solution to fit into the block w.r.t length. @@ -534,7 +564,7 @@ impl Miner { max_allowed_length: u32, assignments: &mut Vec>, encoded_size_of: impl Fn(&[IndexAssignmentOf]) -> Result, - ) -> Result<(), MinerError> { + ) -> Result { // Perform a binary search for the max subset of which can fit into the allowed // length. Having discovered that, we can truncate efficiently. let max_allowed_length: usize = max_allowed_length.saturated_into(); @@ -543,7 +573,7 @@ impl Miner { // not much we can do if assignments are already empty. if high == low { - return Ok(()) + return Ok(0) } while high - low > 1 { @@ -577,16 +607,18 @@ impl Miner { // after this point, we never error. // check before edit. + let remove = assignments.len().saturating_sub(maximum_allowed_voters); + log_no_system!( debug, "from {} assignments, truncating to {} for length, removing {}", assignments.len(), maximum_allowed_voters, - assignments.len().saturating_sub(maximum_allowed_voters), + remove ); assignments.truncate(maximum_allowed_voters); - Ok(()) + Ok(remove) } /// Greedily reduce the size of the solution to fit into the block w.r.t. weight. @@ -609,7 +641,7 @@ impl Miner { size: SolutionOrSnapshotSize, max_weight: Weight, assignments: &mut Vec>, - ) { + ) -> usize { let maximum_allowed_voters = Self::maximum_voter_for_weight(desired_targets, size, max_weight); let removing: usize = @@ -622,6 +654,8 @@ impl Miner { removing, ); assignments.truncate(maximum_allowed_voters as usize); + + removing } /// Find the maximum `len` that a solution can have in order to fit into the block weight. @@ -1230,7 +1264,7 @@ mod tests { assert_eq!(MultiPhase::desired_targets().unwrap(), 2); // mine seq_phragmen solution with 2 iters. - let (solution, witness) = MultiPhase::mine_solution().unwrap(); + let (solution, witness, _) = MultiPhase::mine_solution().unwrap(); // ensure this solution is valid. assert!(MultiPhase::queued_solution().is_none()); @@ -1268,7 +1302,7 @@ mod tests { roll_to_unsigned(); assert!(MultiPhase::current_phase().is_unsigned()); - let (raw, witness) = MultiPhase::mine_solution().unwrap(); + let (raw, witness, _) = MultiPhase::mine_solution().unwrap(); let solution_weight = ::solution_weight( witness.voters, witness.targets, @@ -1282,7 +1316,7 @@ mod tests { // now reduce the max weight ::set(Weight::from_parts(25, u64::MAX)); - let (raw, witness) = MultiPhase::mine_solution().unwrap(); + let (raw, witness, _) = MultiPhase::mine_solution().unwrap(); let solution_weight = ::solution_weight( witness.voters, witness.targets, @@ -1303,7 +1337,7 @@ mod tests { assert!(MultiPhase::current_phase().is_unsigned()); // Force the number of winners to be bigger to fail - let (mut solution, _) = MultiPhase::mine_solution().unwrap(); + let (mut solution, _, _) = MultiPhase::mine_solution().unwrap(); solution.solution.votes1[0].1 = 4; assert_eq!( @@ -1342,7 +1376,7 @@ mod tests { let RoundSnapshot { voters, targets } = MultiPhase::snapshot().unwrap(); let desired_targets = MultiPhase::desired_targets().unwrap(); - let (raw, score, witness) = + let (raw, score, witness, _) = Miner::::prepare_election_result_with_snapshot( result, voters.clone(), @@ -1371,7 +1405,7 @@ mod tests { }, ], }; - let (raw, score, _) = Miner::::prepare_election_result_with_snapshot( + let (raw, score, _, _) = Miner::::prepare_election_result_with_snapshot( result, voters.clone(), targets.clone(), @@ -1400,7 +1434,7 @@ mod tests { }, ], }; - let (raw, score, witness) = + let (raw, score, witness, _) = Miner::::prepare_election_result_with_snapshot( result, voters.clone(), From 9b0321cf9944d363a3ddb3f1832a5ce49eef7903 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Thu, 21 Sep 2023 11:09:07 +0200 Subject: [PATCH 2/5] cargo fmt --- polkadot/xcm/xcm-builder/src/universal_exports.rs | 8 ++++++-- .../frame/election-provider-multi-phase/src/unsigned.rs | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/polkadot/xcm/xcm-builder/src/universal_exports.rs b/polkadot/xcm/xcm-builder/src/universal_exports.rs index d1b8c8c721fe..a24631ffa942 100644 --- a/polkadot/xcm/xcm-builder/src/universal_exports.rs +++ b/polkadot/xcm/xcm-builder/src/universal_exports.rs @@ -171,7 +171,9 @@ impl Miner { voters: Vec<(T::AccountId, VoteWeight, BoundedVec)>, targets: Vec, desired_targets: u32, - ) -> Result<(SolutionOf, ElectionScore, SolutionOrSnapshotSize, TrimmingStatus), MinerError> { + ) -> Result<(SolutionOf, ElectionScore, SolutionOrSnapshotSize, TrimmingStatus), MinerError> + { // now make some helper closures. let cache = helpers::generate_voter_cache::(&voters); let voter_index = helpers::voter_index_fn::(&cache); From 8ac0450f1bb493b6066b49fb5b8171f81a92697f Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Thu, 21 Sep 2023 14:21:00 +0200 Subject: [PATCH 3/5] Update substrate/frame/election-provider-multi-phase/src/unsigned.rs Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com> --- substrate/frame/election-provider-multi-phase/src/unsigned.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/election-provider-multi-phase/src/unsigned.rs b/substrate/frame/election-provider-multi-phase/src/unsigned.rs index a73975431481..a3f774afd0ec 100644 --- a/substrate/frame/election-provider-multi-phase/src/unsigned.rs +++ b/substrate/frame/election-provider-multi-phase/src/unsigned.rs @@ -117,7 +117,7 @@ pub struct TrimmingStatus { impl TrimmingStatus { pub fn is_trimmed(&self) -> bool { - self.weight + self.length > 0 + self.weight > 0 || self.length > 0 } pub fn trimmed_weight(&self) -> usize { From 344b2049845c8c2b7227461b160bc62b313ee445 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Thu, 21 Sep 2023 20:00:05 +0200 Subject: [PATCH 4/5] add tests --- polkadot/tests/common.rs | 4 +--- .../election-provider-multi-phase/src/unsigned.rs | 14 +++++++++----- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/polkadot/tests/common.rs b/polkadot/tests/common.rs index 10859ead5fe8..15721c990e01 100644 --- a/polkadot/tests/common.rs +++ b/polkadot/tests/common.rs @@ -33,9 +33,7 @@ pub async fn wait_n_finalized_blocks(n: usize, url: &str) { let mut interval = tokio::time::interval(Duration::from_secs(6)); loop { - let Ok(rpc) = ws_client(url).await else { - continue; - }; + let Ok(rpc) = ws_client(url).await else { continue }; if let Ok(block) = ChainApi::<(), Hash, Header, Block>::finalized_head(&rpc).await { built_blocks.insert(block); diff --git a/substrate/frame/election-provider-multi-phase/src/unsigned.rs b/substrate/frame/election-provider-multi-phase/src/unsigned.rs index a73975431481..8be9cf2e412a 100644 --- a/substrate/frame/election-provider-multi-phase/src/unsigned.rs +++ b/substrate/frame/election-provider-multi-phase/src/unsigned.rs @@ -108,7 +108,7 @@ impl From for MinerError { } } -/// Reports the trimming result of mined solution +/// Reports the trimming result of a mined solution #[derive(Debug, Clone)] pub struct TrimmingStatus { weight: usize, @@ -1303,7 +1303,7 @@ mod tests { roll_to_unsigned(); assert!(MultiPhase::current_phase().is_unsigned()); - let (raw, witness, _) = MultiPhase::mine_solution().unwrap(); + let (raw, witness, t) = MultiPhase::mine_solution().unwrap(); let solution_weight = ::solution_weight( witness.voters, witness.targets, @@ -1313,11 +1313,12 @@ mod tests { // default solution will have 5 edges (5 * 5 + 10) assert_eq!(solution_weight, Weight::from_parts(35, 0)); assert_eq!(raw.solution.voter_count(), 5); + assert_eq!(t.trimmed_weight(), 0); // now reduce the max weight ::set(Weight::from_parts(25, u64::MAX)); - let (raw, witness, _) = MultiPhase::mine_solution().unwrap(); + let (raw, witness, t) = MultiPhase::mine_solution().unwrap(); let solution_weight = ::solution_weight( witness.voters, witness.targets, @@ -1327,6 +1328,7 @@ mod tests { // default solution will have 5 edges (5 * 5 + 10) assert_eq!(solution_weight, Weight::from_parts(25, 0)); assert_eq!(raw.solution.voter_count(), 3); + assert_eq!(t.trimmed_weight(), 2); }) } @@ -1804,7 +1806,7 @@ mod tests { let solution_clone = solution.clone(); // when - Miner::::trim_assignments_length( + let trimmed_len = Miner::::trim_assignments_length( encoded_len, &mut assignments, encoded_size_of, @@ -1814,6 +1816,7 @@ mod tests { // then let solution = SolutionOf::::try_from(assignments.as_slice()).unwrap(); assert_eq!(solution, solution_clone); + assert_eq!(trimmed_len, 0); }); } @@ -1829,7 +1832,7 @@ mod tests { let solution_clone = solution.clone(); // when - Miner::::trim_assignments_length( + let trimmed_len = Miner::::trim_assignments_length( encoded_len as u32 - 1, &mut assignments, encoded_size_of, @@ -1840,6 +1843,7 @@ mod tests { let solution = SolutionOf::::try_from(assignments.as_slice()).unwrap(); assert_ne!(solution, solution_clone); assert!(solution.encoded_size() < encoded_len); + assert_eq!(trimmed_len, 1); }); } From 148d627cae0e21d97df1d2c2955413dee47faab0 Mon Sep 17 00:00:00 2001 From: Niklas Adolfsson Date: Thu, 21 Sep 2023 20:02:51 +0200 Subject: [PATCH 5/5] cargo fmt --- substrate/frame/election-provider-multi-phase/src/unsigned.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/election-provider-multi-phase/src/unsigned.rs b/substrate/frame/election-provider-multi-phase/src/unsigned.rs index b12ab932378f..e3d0ded97515 100644 --- a/substrate/frame/election-provider-multi-phase/src/unsigned.rs +++ b/substrate/frame/election-provider-multi-phase/src/unsigned.rs @@ -117,7 +117,7 @@ pub struct TrimmingStatus { impl TrimmingStatus { pub fn is_trimmed(&self) -> bool { - self.weight > 0 || self.length > 0 + self.weight > 0 || self.length > 0 } pub fn trimmed_weight(&self) -> usize {