From 50a24bc56ddbedf2657e977b9e3826c956afcf73 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 13 Apr 2021 16:15:30 +0200 Subject: [PATCH 01/34] Refactor election solution trimming for efficiency The previous version always trimmed the `CompactOf` instance, which was intrinsically inefficient: that's a packed data structure, which is naturally expensive to edit. It's much easier to edit the unpacked data structures: the `voters` and `assignments` lists. --- .../src/unsigned.rs | 229 ++++++++++-------- primitives/npos-elections/compact/src/lib.rs | 6 + primitives/npos-elections/src/lib.rs | 44 +++- 3 files changed, 176 insertions(+), 103 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 26e51cf58b34b..da3d01b83fcaa 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -25,7 +25,7 @@ use sp_npos_elections::{ assignment_staked_to_ratio_normalized, }; use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput}; -use sp_std::cmp::Ordering; +use sp_std::{cmp::Ordering, collections::btree_map::BTreeMap}; /// Storage key used to store the persistent offchain worker status. pub(crate) const OFFCHAIN_HEAD_DB: &[u8] = b"parity/multi-phase-unsigned-election"; @@ -34,6 +34,18 @@ pub(crate) const OFFCHAIN_HEAD_DB: &[u8] = b"parity/multi-phase-unsigned-electio /// within a window of 5 blocks. pub(crate) const OFFCHAIN_REPEAT: u32 = 5; +// type helpers for method definitions +// these types are defined elsewhere, but we simplify them here for convenience +pub(crate) type Assignment = sp_npos_elections::Assignment< + ::AccountId, + CompactAccuracyOf, +>; +pub(crate) type Voter = ( + ::AccountId, + sp_npos_elections::VoteWeight, + Vec<::AccountId>, +); + #[derive(Debug, Eq, PartialEq)] pub enum MinerError { /// An internal error in the NPoS elections crate. @@ -140,11 +152,18 @@ impl Pallet { // some point though. // storage items. Note: we have already read this from storage, they must be in cache. - let RoundSnapshot { voters, targets } = + let RoundSnapshot { mut voters, targets } = Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?; let desired_targets = Self::desired_targets().ok_or(MinerError::SnapshotUnAvailable)?; - // closures. + // Both `voters` and `assignments` are vectors. + // They're synchronized: for any arbitrary index `i`, `voters[i]` corresponds to `assignments[i]`. + // However, it turns out to be convenient for us if the assignments are sorted by decreasing + // stake. In order to maintain the correspondence, we have to also sort the voters. + let ElectionResult { mut assignments, winners } = election_result; + Self::sort_by_decreasing_stake(voters.as_mut_slice(), assignments.as_mut_slice()); + + // now make some helper closures. let cache = helpers::generate_voter_cache::(&voters); let voter_index = helpers::voter_index_fn::(&cache); let target_index = helpers::target_index_fn::(&targets); @@ -152,40 +171,28 @@ impl Pallet { let target_at = helpers::target_at_fn::(&targets); let stake_of = helpers::stake_of_fn::(&voters, &cache); - let ElectionResult { assignments, winners } = election_result; - - // convert to staked and reduce. - let mut staked = assignment_ratio_to_staked_normalized(assignments, &stake_of) - .map_err::(Into::into)?; - sp_npos_elections::reduce(&mut staked); - - // convert back to ration and make compact. - let ratio = assignment_staked_to_ratio_normalized(staked)?; - let compact = >::from_assignment(ratio, &voter_index, &target_index)?; - + // trim assignments list for weight and length let size = SolutionOrSnapshotSize { voters: voters.len() as u32, targets: targets.len() as u32 }; - let maximum_allowed_voters = Self::maximum_voter_for_weight::( + Self::trim_assignments_weight( desired_targets, size, T::MinerMaxWeight::get(), + &mut assignments, ); - - log!( - debug, - "initial solution voters = {}, snapshot = {:?}, maximum_allowed(capped) = {}", - compact.voter_count(), - size, - maximum_allowed_voters, + Self::trim_assignments_length( + T::MinerMaxLength::get(), + &mut assignments, ); - // trim length and weight - let compact = Self::trim_compact_weight(maximum_allowed_voters, compact, &voter_index)?; - let compact = Self::trim_compact_length( - T::MinerMaxLength::get(), - compact, - &voter_index, - )?; + // convert to staked and reduce. + let mut staked = assignment_ratio_to_staked_normalized(assignments, &stake_of) + .map_err::(Into::into)?; + sp_npos_elections::reduce(&mut staked); + + // convert back to ratios and make compact. + let ratio = assignment_staked_to_ratio_normalized(staked)?; + let compact = >::from_assignment(ratio, &voter_index, &target_index)?; // re-calc score. let winners = sp_npos_elections::to_without_backing(winners); @@ -212,15 +219,51 @@ impl Pallet { } } - /// Greedily reduce the size of the a solution to fit into the block, w.r.t. weight. + /// Sort the `voters` and `assignments` lists by decreasing voter stake. + /// + /// [`trim_assignments_weight`] and [`trim_assignments_length`] both depend on this property + /// on the `assignments` list for efficient computation. Meanwhile, certain other helper closures + /// depend on `voters` and `assignments` corresponding, so we have to sort both. + fn sort_by_decreasing_stake( + voters: &mut [Voter], + assignments: &mut [Assignment], + ) { + let stakes: BTreeMap<_, _> = voters.iter().map(|(who, stake, _)| { + (who.clone(), *stake) + }).collect(); + + // `Reverse` just reverses the meaning of this key's ordering, so the greatest items come + // first without needing to explicitly call `reverse` afterwards. + // + // Getting an assignment's stake from the `stakes` map should never fail. It will definitely + // never fail for a member of `voters`. However, we use `unwrap_or_default` defensively. In + // case a voter can't be found, it's assumed to have 0 stake, which puts it first on the + // chopping block for removal. + // + // This would be a closure of its own, but rustc doesn't like that and gives E0521, so it's + // simpler to just declare a little macro. The point is that we can show that both lists + // get sorted according to identical rules. + macro_rules! stake_of { + ($who:expr) => { + sp_std::cmp::Reverse( + stakes.get($who).cloned().unwrap_or_default() + ) + }; + } + + // we sort stably so the lists stay synchronized + voters.sort_by_key(|(who, _, _)| stake_of!(who)); + assignments.sort_by_key(|Assignment:: { who, .. }| stake_of!(who)); + } + + /// Greedily reduce the size of the solution to fit into the block w.r.t. weight. /// /// The weight of the solution is foremost a function of the number of voters (i.e. - /// `compact.len()`). Aside from this, the other components of the weight are invariant. The + /// `assignments.len()`). Aside from this, the other components of the weight are invariant. The /// number of winners shall not be changed (otherwise the solution is invalid) and the /// `ElectionSize` is merely a representation of the total number of stakers. /// - /// Thus, we reside to stripping away some voters. This means only changing the `compact` - /// struct. + /// Thus, we reside to stripping away some voters from the `assignments`. /// /// Note that the solution is already computed, and the winners are elected based on the merit /// of the entire stake in the system. Nonetheless, some of the voters will be removed further @@ -228,50 +271,24 @@ impl Pallet { /// /// 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_weight( - maximum_allowed_voters: u32, - mut compact: CompactOf, - voter_index: FN, - ) -> Result, MinerError> - where - for<'r> FN: Fn(&'r T::AccountId) -> Option>, - { - match compact.voter_count().checked_sub(maximum_allowed_voters as usize) { - Some(to_remove) if to_remove > 0 => { - // 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::>(); - voters_sorted.sort_by_key(|(_, y)| *y); - - // start removing from the least stake. Iterate until we know enough have been - // removed. - let mut removed = 0; - for (maybe_index, _stake) in - voters_sorted.iter().map(|(who, stake)| (voter_index(&who), stake)) - { - let index = maybe_index.ok_or(MinerError::SnapshotUnAvailable)?; - if compact.remove_voter(index) { - removed += 1 - } - - if removed >= to_remove { - break; - } - } - - log!(debug, "removed {} voter to meet the max weight limit.", to_remove); - Ok(compact) - } - _ => { - // nada, return as-is - log!(debug, "didn't remove any voter for weight limits."); - Ok(compact) - } - } + fn trim_assignments_weight( + desired_targets: u32, + size: SolutionOrSnapshotSize, + max_weight: Weight, + assignments: &mut Vec>, + ) { + let maximum_allowed_voters = Self::maximum_voter_for_weight::( + desired_targets, + size, + max_weight, + ); + let removing: usize = assignments.len() - maximum_allowed_voters as usize; + log!( + debug, + "from {} assignments, truncating to {} for weight, removing {}", + assignments.len(), maximum_allowed_voters, removing, + ); + assignments.truncate(maximum_allowed_voters as usize); } /// Greedily reduce the size of the solution to fit into the block w.r.t length. @@ -288,34 +305,44 @@ impl Pallet { /// /// 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_length( + fn trim_assignments_length( max_allowed_length: u32, - mut compact: CompactOf, - voter_index: impl Fn(&T::AccountId) -> Option>, - ) -> Result, MinerError> { - // short-circuit to avoid getting the voters if possible - // this involves a redundant encoding, but that should hopefully be relatively cheap - if (compact.encoded_size().saturated_into::()) <= max_allowed_length { - return Ok(compact); + assignments: &mut Vec>, + ) { + // The encoded size of an assignment list is staticly computable, but only in `O(assignments.len())`. + // That makes the naive approach, to loop and pop, somewhat inefficient. + // + // Instead, we perform a binary search for the max subset of which can fit into the allowed + // length. Having discovered that, we can truncate efficiently. + let mut high = assignments.len(); + let mut low = 0; + let avg = move || (high + low) / 2; + while high - low > 1 { + let test = avg(); + if CompactOf::::encoded_size_for(&assignments[..test]) > max_allowed_length.saturated_into() { + high = test; + } else { + low = test; + } } + let maximum_allowed_voters = avg(); - // 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::>(); - voters_sorted.sort_by_key(|(_, y)| *y); - voters_sorted.reverse(); - - 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); - } + // ensure our postconditions are correct + debug_assert!( + CompactOf::::encoded_size_for(&assignments[..maximum_allowed_voters]) <= + max_allowed_length.saturated_into() + ); + debug_assert!( + CompactOf::::encoded_size_for(&assignments[..maximum_allowed_voters + 1]) > + max_allowed_length.saturated_into() + ); - Ok(compact) + log!( + debug, + "from {} assignments, truncating to {} for length, removing {}", + assignments.len(), maximum_allowed_voters, assignments.len() - maximum_allowed_voters, + ); + assignments.truncate(maximum_allowed_voters); } /// Find the maximum `len` that a compact can have in order to fit into the block weight. diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index e558ae89ca93e..88cc6178e32d7 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -255,6 +255,12 @@ fn struct_def( #into_impl Ok(assignments) } + + fn encoded_size_for( + assignments: &[_npos::Assignment], + ) -> usize { + unimplemented!() + } } )) } diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 05505d06f201e..56b49100dadda 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -136,6 +136,42 @@ impl __OrInvalidIndex for Option { } } +/// For types whose `parity_scale_codec::Encode` implementation is guaranteed to occupy a certain +/// fixed number of output bytes. +/// +/// This whole thing is a hack working around the fact that `Encode::size_hint` is unreliable. +pub trait FixedEncodingSize { + const ENCODED_SIZE: usize; +} + +macro_rules! impl_fes { + ($t:ty => $size:literal; $( $rest:tt )* ) => { + impl FixedEncodingSize for $t { + const ENCODED_SIZE: usize = $size; + } + impl_fes!{$($rest)*} + }; + () => {}; +} + +impl_fes! { + () => 0; + bool => 1; + u8 => 1; + u16 => 2; + u32 => 4; + u64 => 8; + u128 => 16; + i8 => 1; + i16 => 2; + i32 => 4; + i64 => 8; + i128 => 16; + f32 => 4; + f64 => 8; + char => 4; +} + /// A common interface for all compact solutions. /// /// See [`sp-npos-elections-compact`] for more info. @@ -150,7 +186,8 @@ pub trait CompactSolution: Sized { + Debug + Copy + Clone - + Bounded; + + Bounded + + FixedEncodingSize; /// The target type. Needs to be an index (convert to usize). type Target: UniqueSaturatedInto @@ -159,7 +196,8 @@ pub trait CompactSolution: Sized { + Debug + Copy + Clone - + Bounded; + + Bounded + + FixedEncodingSize; /// The weight/accuracy type of each vote. type Accuracy: PerThing128; @@ -232,6 +270,8 @@ pub trait CompactSolution: Sized { let supports = to_supports(winners, &staked)?; Ok(supports.evaluate()) } + + fn encoded_size_for(assignments: &[Assignment]) -> usize; } // re-export the compact solution type. From d3cba7bc56877c0c17566d3771c0b1bd89169b90 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 14 Apr 2021 10:05:06 +0200 Subject: [PATCH 02/34] rework length-trim tests to work with the new interface Test suite now compiles. Tests still don't pass because the macro generating the compact structure still generates `unimplemented!()` for the actual `compact_length_of` implementation. --- .../election-provider-multi-phase/src/mock.rs | 55 ++++++++- .../src/unsigned.rs | 110 +++++++++--------- 2 files changed, 106 insertions(+), 59 deletions(-) diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 79e6e952bfec8..b6136b6c760ad 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -17,6 +17,7 @@ use super::*; use crate as multi_phase; +use multi_phase::unsigned::{Voter, Assignment}; pub use frame_support::{assert_noop, assert_ok}; use frame_support::{ parameter_types, @@ -95,18 +96,56 @@ pub fn roll_to_with_ocw(n: u64) { } } -/// Spit out a verifiable raw solution. +/// Compute voters and assignments for this runtime. /// -/// This is a good example of what an offchain miner would do. -pub fn raw_solution() -> RawSolution> { +/// Just a helper for trimming tests. +pub fn voters_and_assignments() -> (Vec>, Vec>) { let RoundSnapshot { voters, targets } = MultiPhase::snapshot().unwrap(); let desired_targets = MultiPhase::desired_targets().unwrap(); + let ElectionResult { assignments, .. } = seq_phragmen::<_, CompactAccuracyOf>( + desired_targets as usize, + targets, + voters.clone(), + None, + ) + .unwrap(); + + (voters, assignments) +} + +/// Convert voters and assignments into a verifiable raw solution. +/// +/// If you don't need to mess with `voters` and `assignments`, [`raw_solution`] is more efficient. +/// +/// It's important that voters and assignments retain their correspondence. +pub fn make_compact_from( + voters: Vec>, + assignments: Vec>, +) -> CompactOf { + // voters and assignments may have different length, but the voter ID must correspond at each + // position. + let voter_ids = voters.iter().map(|(who, _, _)| who).cloned(); + let assignment_ids = assignments.iter().map(|assignment| assignment.who); + assert!(voter_ids + .zip(assignment_ids) + .all(|(voter_id, assignment_id)| voter_id == assignment_id)); + + let RoundSnapshot { targets, .. } = MultiPhase::snapshot().unwrap(); + // closures - let cache = helpers::generate_voter_cache::(&voters); let voter_index = helpers::voter_index_fn_linear::(&voters); let target_index = helpers::target_index_fn_linear::(&targets); - let stake_of = helpers::stake_of_fn::(&voters, &cache); + + >::from_assignment(assignments, &voter_index, &target_index).unwrap() +} + +/// Spit out a verifiable raw solution. +/// +/// This is a good example of what an offchain miner would do. +pub fn raw_solution() -> RawSolution> { + let RoundSnapshot { voters, targets } = MultiPhase::snapshot().unwrap(); + let desired_targets = MultiPhase::desired_targets().unwrap(); let ElectionResult { winners, assignments } = seq_phragmen::<_, CompactAccuracyOf>( desired_targets as usize, @@ -116,6 +155,12 @@ pub fn raw_solution() -> RawSolution> { ) .unwrap(); + // closures + let cache = helpers::generate_voter_cache::(&voters); + let voter_index = helpers::voter_index_fn_linear::(&voters); + let target_index = helpers::target_index_fn_linear::(&targets); + let stake_of = helpers::stake_of_fn::(&voters, &cache); + let winners = to_without_backing(winners); let score = { diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index da3d01b83fcaa..81d7dd2e0e968 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -222,15 +222,18 @@ impl Pallet { /// Sort the `voters` and `assignments` lists by decreasing voter stake. /// /// [`trim_assignments_weight`] and [`trim_assignments_length`] both depend on this property - /// on the `assignments` list for efficient computation. Meanwhile, certain other helper closures - /// depend on `voters` and `assignments` corresponding, so we have to sort both. - fn sort_by_decreasing_stake( - voters: &mut [Voter], - assignments: &mut [Assignment], - ) { - let stakes: BTreeMap<_, _> = voters.iter().map(|(who, stake, _)| { - (who.clone(), *stake) - }).collect(); + /// on the `assignments` list for efficient computation. Meanwhile, certain other helper + /// closures depend on `voters` and `assignments` corresponding, so we have to sort both. + fn sort_by_decreasing_stake(voters: &mut [Voter], assignments: &mut [Assignment]) { + // verify precondition + debug_assert!({ + let voter_ids = voters.iter().map(|(who, _, _)| who).cloned(); + let assignment_ids = assignments.iter().map(|assignment| assignment.who.clone()); + voter_ids.zip(assignment_ids).all(|(voter_id, assignment_id)| voter_id == assignment_id) + }); + + let stakes: BTreeMap<_, _> = + voters.iter().map(|(who, stake, _)| (who.clone(), *stake)).collect(); // `Reverse` just reverses the meaning of this key's ordering, so the greatest items come // first without needing to explicitly call `reverse` afterwards. @@ -254,6 +257,13 @@ impl Pallet { // we sort stably so the lists stay synchronized voters.sort_by_key(|(who, _, _)| stake_of!(who)); assignments.sort_by_key(|Assignment:: { who, .. }| stake_of!(who)); + + // verify postcondition + debug_assert!({ + let voter_ids = voters.iter().map(|(who, _, _)| who).cloned(); + let assignment_ids = assignments.iter().map(|assignment| assignment.who.clone()); + voter_ids.zip(assignment_ids).all(|(voter_id, assignment_id)| voter_id == assignment_id) + }); } /// Greedily reduce the size of the solution to fit into the block w.r.t. weight. @@ -579,12 +589,13 @@ mod max_weight { #[cfg(test)] mod tests { - use super::{ - mock::{Origin, *}, - Call, *, + use super::*; + use crate::mock::{ + assert_noop, assert_ok, ExtBuilder, Extrinsic, make_compact_from, + MinerMaxWeight, MultiPhase, Origin, roll_to_with_ocw, roll_to, Runtime, + TestCompact, voters_and_assignments, witness, }; use frame_support::{dispatch::Dispatchable, traits::OffchainWorker}; - use helpers::voter_index_fn_linear; use mock::Call as OuterCall; use frame_election_provider_support::Assignment; use sp_runtime::{traits::ValidateUnsigned, PerU16}; @@ -970,90 +981,81 @@ mod tests { } #[test] - fn trim_compact_length_does_not_modify_when_short_enough() { + fn trim_assignments_length_does_not_modify_when_short_enough() { let mut ext = ExtBuilder::default().build(); ext.execute_with(|| { roll_to(25); // given - let RoundSnapshot { voters, ..} = MultiPhase::snapshot().unwrap(); - let RawSolution { mut compact, .. } = raw_solution(); + let (mut voters, mut assignments) = voters_and_assignments(); + let compact = make_compact_from(voters.clone(), assignments.clone()); let encoded_len = compact.encode().len() as u32; let compact_clone = compact.clone(); // when - assert!(encoded_len < ::MinerMaxLength::get()); + MultiPhase::sort_by_decreasing_stake(&mut voters, &mut assignments); + MultiPhase::trim_assignments_length(encoded_len, &mut assignments); // then - compact = MultiPhase::trim_compact_length( - encoded_len, - compact, - voter_index_fn_linear::(&voters), - ).unwrap(); + let compact = make_compact_from(voters, assignments); assert_eq!(compact, compact_clone); }); } #[test] - fn trim_compact_length_modifies_when_too_long() { + fn trim_assignments_length_modifies_when_too_long() { 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; + // given + let (mut voters, mut assignments) = voters_and_assignments(); + let compact = make_compact_from(voters.clone(), assignments.clone()); + let encoded_len = compact.encode().len() as u32; let compact_clone = compact.clone(); - compact = MultiPhase::trim_compact_length( - encoded_len - 1, - compact, - voter_index_fn_linear::(&voters), - ).unwrap(); + // when + MultiPhase::sort_by_decreasing_stake(&mut voters, &mut assignments); + MultiPhase::trim_assignments_length(encoded_len - 1, &mut assignments); + // then + let compact = make_compact_from(voters, assignments); assert_ne!(compact, compact_clone); assert!((compact.encoded_size() as u32) < encoded_len); }); } #[test] - fn trim_compact_length_trims_lowest_stake() { + fn trim_assignments_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(); + // given + let (mut voters, mut assignments) = voters_and_assignments(); + let compact = make_compact_from(voters.clone(), assignments.clone()); + let encoded_len = compact.encode().len() as u32; + let voter_count = voters.len(); let min_stake_voter = voters.iter() .map(|(id, weight, _)| (weight, id)) .min() - .map(|(_, id)| id) + .map(|(_, id)| *id) .unwrap(); + // test precondition + assert_eq!(voter_count, compact.voter_count()); + // when + MultiPhase::sort_by_decreasing_stake(&mut voters, &mut assignments); + MultiPhase::trim_assignments_length(encoded_len - 1, &mut assignments); - compact = MultiPhase::trim_compact_length( - encoded_len - 1, - compact, - voter_index_fn_linear::(&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(); + // then assert!( assignments.iter() - .all(|Assignment{ who, ..}| who != min_stake_voter), + .all(|Assignment{ who, ..}| *who != min_stake_voter), "min_stake_voter must no longer be in the set of voters", ); + let compact = make_compact_from(voters, assignments); + assert_eq!(compact.voter_count(), voter_count - 1, "we must have removed exactly 1 voter"); }); } From 919c98dddddfe62ee7a3a281d91d9016df88d023 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 14 Apr 2021 10:13:15 +0200 Subject: [PATCH 03/34] simplify --- frame/election-provider-multi-phase/src/unsigned.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 81d7dd2e0e968..e4c64a2dcb74c 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -1035,27 +1035,24 @@ mod tests { let (mut voters, mut assignments) = voters_and_assignments(); let compact = make_compact_from(voters.clone(), assignments.clone()); let encoded_len = compact.encode().len() as u32; - let voter_count = voters.len(); + let count = assignments.len(); let min_stake_voter = voters.iter() .map(|(id, weight, _)| (weight, id)) .min() .map(|(_, id)| *id) .unwrap(); - // test precondition - assert_eq!(voter_count, compact.voter_count()); // when MultiPhase::sort_by_decreasing_stake(&mut voters, &mut assignments); MultiPhase::trim_assignments_length(encoded_len - 1, &mut assignments); // then + assert_eq!(assignments.len(), count - 1, "we must have removed exactly one assignment"); assert!( assignments.iter() .all(|Assignment{ who, ..}| *who != min_stake_voter), "min_stake_voter must no longer be in the set of voters", ); - let compact = make_compact_from(voters, assignments); - assert_eq!(compact.voter_count(), voter_count - 1, "we must have removed exactly 1 voter"); }); } From 916038790887e64217c6a46e9a6d281386762bfb Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 14 Apr 2021 14:18:33 +0200 Subject: [PATCH 04/34] add a fuzzer which can validate `Compact::encoded_size_for` The `Compact` solution type is generated distinctly for each runtime, and has both three type parameters and a built-in limit to the number of candidates that each voter can vote for. Finally, they have an optional `#[compact]` attribute which changes the encoding behavior. The assignment truncation algorithm we're using depends on the ability to efficiently and accurately determine how much space a `Compact` solution will take once encoded. Together, these two facts imply that simple unit tests are not sufficient to validate the behavior of `Compact::encoded_size_for`. This commit adds such a fuzzer. It is designed such that it is possible to add a new fuzzer to the family by simply adjusting the `generate_solution_type` macro invocation as desired, and making a few minor documentation edits. Of course, the fuzzer still fails for now: the generated implementation for `encoded_size_for` is still `unimplemented!()`. However, once the macro is updated appropriately, this fuzzer family should allow us to gain confidence in the correctness of the generated code. --- Cargo.lock | 12 ++ Cargo.toml | 1 + .../npos-elections/compact/fuzzer/Cargo.toml | 35 ++++ .../compact/fuzzer/src/common.rs | 168 ++++++++++++++++++ .../compact/fuzzer/src/compact_32_16_16.rs | 112 ++++++++++++ 5 files changed, 328 insertions(+) create mode 100644 primitives/npos-elections/compact/fuzzer/Cargo.toml create mode 100644 primitives/npos-elections/compact/fuzzer/src/common.rs create mode 100644 primitives/npos-elections/compact/fuzzer/src/compact_32_16_16.rs diff --git a/Cargo.lock b/Cargo.lock index 14fd4778c639a..3a9c98d8901ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8928,6 +8928,18 @@ dependencies = [ "trybuild", ] +[[package]] +name = "sp-npos-elections-compact-fuzzer" +version = "2.0.0-alpha.5" +dependencies = [ + "honggfuzz", + "parity-scale-codec", + "rand 0.7.3", + "sp-npos-elections", + "sp-runtime", + "structopt", +] + [[package]] name = "sp-npos-elections-fuzzer" version = "2.0.0-alpha.5" diff --git a/Cargo.toml b/Cargo.toml index 1b35c7181d17d..ba1c06a7a4472 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -157,6 +157,7 @@ members = [ "primitives/maybe-compressed-blob", "primitives/npos-elections", "primitives/npos-elections/compact", + "primitives/npos-elections/compact/fuzzer", "primitives/npos-elections/fuzzer", "primitives/offchain", "primitives/panic-handler", diff --git a/primitives/npos-elections/compact/fuzzer/Cargo.toml b/primitives/npos-elections/compact/fuzzer/Cargo.toml new file mode 100644 index 0000000000000..ce54be7fec623 --- /dev/null +++ b/primitives/npos-elections/compact/fuzzer/Cargo.toml @@ -0,0 +1,35 @@ +[package] +name = "sp-npos-elections-compact-fuzzer" +version = "2.0.0-alpha.5" +authors = ["Parity Technologies "] +edition = "2018" +license = "Apache-2.0" +homepage = "https://substrate.dev" +repository = "https://github.com/paritytech/substrate/" +description = "Fuzzer for sp-npos-elections-compact macro" +publish = false + +[dependencies] +codec = { package = "parity-scale-codec", version = "2.0.0", default-features = false, features = ["derive"] } +honggfuzz = "0.5" +rand = { version = "0.7.3", features = ["std", "small_rng"] } +structopt = "0.3.21" + +sp-npos-elections = { version = "3.0.0", path = "../.." } +sp-runtime = { version = "3.0.0", path = "../../../runtime" } + +[[bin]] +name = "compact_32_16_16" +path = "src/compact_32_16_16.rs" + +# [[bin]] +# name = "compact_24" +# path = "src/compact24.rs" + +# [[bin]] +# name = "non_compact_16" +# path = "src/noncompact16.rs" + +# [[bin]] +# name = "non_compact_24" +# path = "src/noncompact24.rs" diff --git a/primitives/npos-elections/compact/fuzzer/src/common.rs b/primitives/npos-elections/compact/fuzzer/src/common.rs new file mode 100644 index 0000000000000..eb3463f8dc895 --- /dev/null +++ b/primitives/npos-elections/compact/fuzzer/src/common.rs @@ -0,0 +1,168 @@ +// This file is part of Substrate. + +// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Common fuzzing utils. + +use rand::{self, seq::SliceRandom, Rng}; +use std::{ + collections::{HashSet, HashMap}, + convert::TryInto, + hash::Hash, +}; + +pub type AccountId = u64; +/// The candidate mask allows easy disambiguation between voters and candidates: accounts +/// for which this bit is set are candidates, and without it, are voters. +pub const CANDIDATE_MASK: AccountId = 1 << ((std::mem::size_of::() * 8) - 1); +pub type CandidateId = AccountId; + +pub type Accuracy = sp_runtime::Perbill; + +pub type Assignment = sp_npos_elections::Assignment; +pub type Voter = (AccountId, sp_npos_elections::VoteWeight, Vec); + +/// converts x into the range [a, b] in a pseudo-fair way. +pub fn to_range(x: usize, a: usize, b: usize) -> usize { + // does not work correctly if b < 2 * a + assert!(b >= 2 * a); + let collapsed = x % b; + if collapsed >= a { + collapsed + } else { + collapsed + a + } +} + +/// Generate voter and assignment lists. Makes no attempt to be realistic about winner or assignment fairness. +/// +/// Maintains these invariants: +/// +/// - candidate ids have `CANDIDATE_MASK` bit set +/// - voter ids do not have `CANDIDATE_MASK` bit set +/// - assignments have the same ordering as voters +/// - `assignments.distribution.iter().map(|(_, frac)| frac).sum() == One::one()` +/// - a coherent set of winners is chosen. +/// - the winner set is a subset of the candidate set. +/// - `assignments.distribution.iter().all(|(who, _)| winners.contains(who))` +pub fn generate_random_votes( + candidate_count: usize, + voter_count: usize, + mut rng: impl Rng, +) -> (Vec, Vec, Vec) { + // cache for fast generation of unique candidate and voter ids + let mut used_ids = HashSet::with_capacity(candidate_count + voter_count); + + // candidates are easy: just a completely random set of IDs + let mut candidates: Vec = Vec::with_capacity(candidate_count); + while candidates.len() < candidate_count { + let mut new = || rng.gen::() | CANDIDATE_MASK; + let mut id = new(); + // insert returns `false` when the value was already present + while !used_ids.insert(id) { + id = new(); + } + candidates.push(id); + } + + // voters are random ids, random weights, random selection from the candidates + let mut voters = Vec::with_capacity(voter_count); + while voters.len() < voter_count { + let mut new = || rng.gen::() & !CANDIDATE_MASK; + let mut id = new(); + // insert returns `false` when the value was already present + while !used_ids.insert(id) { + id = new(); + } + + let vote_weight = rng.gen(); + + // it's not interesting if a voter chooses 0 or all candidates, so rule those cases out. + let n_candidates_chosen = rng.gen_range(1, candidates.len()); + + let mut chosen_candidates = Vec::with_capacity(n_candidates_chosen); + chosen_candidates.extend(candidates.choose_multiple(&mut rng, n_candidates_chosen)); + voters.push((id, vote_weight, chosen_candidates)); + } + + // always generate a sensible number of winners: elections are uninteresting if nobody wins, + // or everybody wins + let num_winners = rng.gen_range(1, candidate_count); + let mut winners: HashSet = HashSet::with_capacity(num_winners); + winners.extend(candidates.choose_multiple(&mut rng, num_winners)); + assert_eq!(winners.len(), num_winners); + + let mut assignments = Vec::with_capacity(voters.len()); + for (voter_id, _, votes) in voters.iter() { + let chosen_winners = votes.iter().filter(|vote| winners.contains(vote)).cloned(); + let num_chosen_winners = chosen_winners.clone().count(); + + // distribute the available stake randomly + let stake_distribution = if num_chosen_winners == 0 { + Vec::new() + } else { + let mut available_stake = 1000; + let mut stake_distribution = Vec::with_capacity(num_chosen_winners); + for _ in 0..num_chosen_winners - 1 { + let stake = rng.gen_range(0, available_stake); + stake_distribution.push(Accuracy::from_perthousand(stake)); + available_stake -= stake; + } + stake_distribution.push(Accuracy::from_perthousand(available_stake)); + stake_distribution.shuffle(&mut rng); + stake_distribution + }; + + assignments.push(Assignment { + who: *voter_id, + distribution: chosen_winners.zip(stake_distribution).collect(), + }); + } + + (voters, assignments, candidates) +} + +fn generate_cache(voters: Voters) -> HashMap +where + Voters: Iterator, + Item: Hash + Eq + Copy, +{ + let mut cache = HashMap::new(); + for (idx, voter_id) in voters.enumerate() { + cache.insert(voter_id, idx); + } + cache +} + +/// Create a function that returns the index of a voter in the voters list. +pub fn make_voter_fn(voters: &[Voter]) -> impl Fn(&AccountId) -> Option +where + usize: TryInto, +{ + let cache = generate_cache(voters.iter().map(|(id, _, _)| *id)); + move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) +} + +/// Create a function that returns the index of a candidate in the candidates list. +pub fn make_target_fn( + candidates: &[CandidateId], +) -> impl Fn(&CandidateId) -> Option +where + usize: TryInto, +{ + let cache = generate_cache(candidates.iter().cloned()); + move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) +} diff --git a/primitives/npos-elections/compact/fuzzer/src/compact_32_16_16.rs b/primitives/npos-elections/compact/fuzzer/src/compact_32_16_16.rs new file mode 100644 index 0000000000000..dc70b793d3710 --- /dev/null +++ b/primitives/npos-elections/compact/fuzzer/src/compact_32_16_16.rs @@ -0,0 +1,112 @@ +// This file is part of Substrate. + +// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Fuzzing which ensures that the generated implementation of `encoded_size_for` always accurately +//! predicts the correct encoded size. +//! +//! ## Running a single iteration +//! +//! Simply run the program without the `fuzzing` configuration to run a single iteration: +//! `cargo run --bin compact_16`. +//! +//! ## Running +//! +//! Run with `cargo hfuzz run compact_16`. +//! +//! ## Debugging a panic +//! +//! Once a panic is found, it can be debugged with +//! `cargo hfuzz run-debug compact_16 hfuzz_workspace/compact_16/*.fuzz`. + +use _npos::CompactSolution; +use codec::Encode; +#[cfg(fuzzing)] +use honggfuzz::fuzz; + +#[cfg(not(fuzzing))] +use structopt::StructOpt; + +mod common; + +use common::{Accuracy, generate_random_votes, make_target_fn, make_voter_fn, to_range}; +use rand::{self, SeedableRng}; + +sp_npos_elections::generate_solution_type!( + #[compact] + pub struct Compact::(16) +); + +const MIN_CANDIDATES: usize = 250; +const MAX_CANDIDATES: usize = 1000; +const MIN_VOTERS: usize = 500; +const MAX_VOTERS: usize = 2500; + +#[cfg(fuzzing)] +fn main() { + loop { + fuzz!(|data: (usize, usize, u64)| { + let (candidate_count, voter_count, seed) = data; + iteration(candidate_count, voter_count, seed); + }); + } +} + +#[cfg(not(fuzzing))] +#[derive(Debug, StructOpt)] +struct Opt { + /// How many candidates participate in this election + #[structopt(short, long)] + candidates: Option, + + /// How many voters participate in this election + #[structopt(short, long)] + voters: Option, + + /// Random seed to use in this election + #[structopt(long)] + seed: Option, +} + +#[cfg(not(fuzzing))] +fn main() { + let opt = Opt::from_args(); + // candidates and voters by default use the maxima, which turn out to be one less than + // the constant. + iteration( + opt.candidates.unwrap_or(MAX_CANDIDATES - 1), + opt.voters.unwrap_or(MAX_VOTERS - 1), + opt.seed.unwrap_or_default(), + ); +} + +fn iteration(mut candidate_count: usize, mut voter_count: usize, seed: u64) { + let rng = rand::rngs::SmallRng::seed_from_u64(seed); + candidate_count = to_range(candidate_count, MIN_CANDIDATES, MAX_CANDIDATES); + voter_count = to_range(voter_count, MIN_VOTERS, MAX_VOTERS); + + let (voters, assignments, candidates) = + generate_random_votes(candidate_count, voter_count, rng); + + let predicted_size = Compact::encoded_size_for(&assignments); + + let compact = + Compact::from_assignment(assignments, make_voter_fn(&voters), make_target_fn(&candidates)) + .unwrap(); + let encoding = compact.encode(); + + assert_eq!(predicted_size, encoding.len()); +} From b7fe186b0ba9ad2180de10d1aaa4c3020f005aba Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 14 Apr 2021 14:34:21 +0200 Subject: [PATCH 05/34] Revert "add a fuzzer which can validate `Compact::encoded_size_for`" This reverts commit 916038790887e64217c6a46e9a6d281386762bfb. The design of `Compact::encoded_size_for` is flawed. When `#[compact]` mode is enabled, every integer in the dataset is encoded using run- length encoding. This means that it is impossible to compute the final length faster than actually encoding the data structure, because the encoded length of every field varies with the actual value stored. Given that we won't be adding that method to the trait, we won't be needing a fuzzer to validate its performance. --- Cargo.lock | 12 -- Cargo.toml | 1 - .../npos-elections/compact/fuzzer/Cargo.toml | 35 ---- .../compact/fuzzer/src/common.rs | 168 ------------------ .../compact/fuzzer/src/compact_32_16_16.rs | 112 ------------ 5 files changed, 328 deletions(-) delete mode 100644 primitives/npos-elections/compact/fuzzer/Cargo.toml delete mode 100644 primitives/npos-elections/compact/fuzzer/src/common.rs delete mode 100644 primitives/npos-elections/compact/fuzzer/src/compact_32_16_16.rs diff --git a/Cargo.lock b/Cargo.lock index 3a9c98d8901ad..14fd4778c639a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8928,18 +8928,6 @@ dependencies = [ "trybuild", ] -[[package]] -name = "sp-npos-elections-compact-fuzzer" -version = "2.0.0-alpha.5" -dependencies = [ - "honggfuzz", - "parity-scale-codec", - "rand 0.7.3", - "sp-npos-elections", - "sp-runtime", - "structopt", -] - [[package]] name = "sp-npos-elections-fuzzer" version = "2.0.0-alpha.5" diff --git a/Cargo.toml b/Cargo.toml index ba1c06a7a4472..1b35c7181d17d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -157,7 +157,6 @@ members = [ "primitives/maybe-compressed-blob", "primitives/npos-elections", "primitives/npos-elections/compact", - "primitives/npos-elections/compact/fuzzer", "primitives/npos-elections/fuzzer", "primitives/offchain", "primitives/panic-handler", diff --git a/primitives/npos-elections/compact/fuzzer/Cargo.toml b/primitives/npos-elections/compact/fuzzer/Cargo.toml deleted file mode 100644 index ce54be7fec623..0000000000000 --- a/primitives/npos-elections/compact/fuzzer/Cargo.toml +++ /dev/null @@ -1,35 +0,0 @@ -[package] -name = "sp-npos-elections-compact-fuzzer" -version = "2.0.0-alpha.5" -authors = ["Parity Technologies "] -edition = "2018" -license = "Apache-2.0" -homepage = "https://substrate.dev" -repository = "https://github.com/paritytech/substrate/" -description = "Fuzzer for sp-npos-elections-compact macro" -publish = false - -[dependencies] -codec = { package = "parity-scale-codec", version = "2.0.0", default-features = false, features = ["derive"] } -honggfuzz = "0.5" -rand = { version = "0.7.3", features = ["std", "small_rng"] } -structopt = "0.3.21" - -sp-npos-elections = { version = "3.0.0", path = "../.." } -sp-runtime = { version = "3.0.0", path = "../../../runtime" } - -[[bin]] -name = "compact_32_16_16" -path = "src/compact_32_16_16.rs" - -# [[bin]] -# name = "compact_24" -# path = "src/compact24.rs" - -# [[bin]] -# name = "non_compact_16" -# path = "src/noncompact16.rs" - -# [[bin]] -# name = "non_compact_24" -# path = "src/noncompact24.rs" diff --git a/primitives/npos-elections/compact/fuzzer/src/common.rs b/primitives/npos-elections/compact/fuzzer/src/common.rs deleted file mode 100644 index eb3463f8dc895..0000000000000 --- a/primitives/npos-elections/compact/fuzzer/src/common.rs +++ /dev/null @@ -1,168 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Common fuzzing utils. - -use rand::{self, seq::SliceRandom, Rng}; -use std::{ - collections::{HashSet, HashMap}, - convert::TryInto, - hash::Hash, -}; - -pub type AccountId = u64; -/// The candidate mask allows easy disambiguation between voters and candidates: accounts -/// for which this bit is set are candidates, and without it, are voters. -pub const CANDIDATE_MASK: AccountId = 1 << ((std::mem::size_of::() * 8) - 1); -pub type CandidateId = AccountId; - -pub type Accuracy = sp_runtime::Perbill; - -pub type Assignment = sp_npos_elections::Assignment; -pub type Voter = (AccountId, sp_npos_elections::VoteWeight, Vec); - -/// converts x into the range [a, b] in a pseudo-fair way. -pub fn to_range(x: usize, a: usize, b: usize) -> usize { - // does not work correctly if b < 2 * a - assert!(b >= 2 * a); - let collapsed = x % b; - if collapsed >= a { - collapsed - } else { - collapsed + a - } -} - -/// Generate voter and assignment lists. Makes no attempt to be realistic about winner or assignment fairness. -/// -/// Maintains these invariants: -/// -/// - candidate ids have `CANDIDATE_MASK` bit set -/// - voter ids do not have `CANDIDATE_MASK` bit set -/// - assignments have the same ordering as voters -/// - `assignments.distribution.iter().map(|(_, frac)| frac).sum() == One::one()` -/// - a coherent set of winners is chosen. -/// - the winner set is a subset of the candidate set. -/// - `assignments.distribution.iter().all(|(who, _)| winners.contains(who))` -pub fn generate_random_votes( - candidate_count: usize, - voter_count: usize, - mut rng: impl Rng, -) -> (Vec, Vec, Vec) { - // cache for fast generation of unique candidate and voter ids - let mut used_ids = HashSet::with_capacity(candidate_count + voter_count); - - // candidates are easy: just a completely random set of IDs - let mut candidates: Vec = Vec::with_capacity(candidate_count); - while candidates.len() < candidate_count { - let mut new = || rng.gen::() | CANDIDATE_MASK; - let mut id = new(); - // insert returns `false` when the value was already present - while !used_ids.insert(id) { - id = new(); - } - candidates.push(id); - } - - // voters are random ids, random weights, random selection from the candidates - let mut voters = Vec::with_capacity(voter_count); - while voters.len() < voter_count { - let mut new = || rng.gen::() & !CANDIDATE_MASK; - let mut id = new(); - // insert returns `false` when the value was already present - while !used_ids.insert(id) { - id = new(); - } - - let vote_weight = rng.gen(); - - // it's not interesting if a voter chooses 0 or all candidates, so rule those cases out. - let n_candidates_chosen = rng.gen_range(1, candidates.len()); - - let mut chosen_candidates = Vec::with_capacity(n_candidates_chosen); - chosen_candidates.extend(candidates.choose_multiple(&mut rng, n_candidates_chosen)); - voters.push((id, vote_weight, chosen_candidates)); - } - - // always generate a sensible number of winners: elections are uninteresting if nobody wins, - // or everybody wins - let num_winners = rng.gen_range(1, candidate_count); - let mut winners: HashSet = HashSet::with_capacity(num_winners); - winners.extend(candidates.choose_multiple(&mut rng, num_winners)); - assert_eq!(winners.len(), num_winners); - - let mut assignments = Vec::with_capacity(voters.len()); - for (voter_id, _, votes) in voters.iter() { - let chosen_winners = votes.iter().filter(|vote| winners.contains(vote)).cloned(); - let num_chosen_winners = chosen_winners.clone().count(); - - // distribute the available stake randomly - let stake_distribution = if num_chosen_winners == 0 { - Vec::new() - } else { - let mut available_stake = 1000; - let mut stake_distribution = Vec::with_capacity(num_chosen_winners); - for _ in 0..num_chosen_winners - 1 { - let stake = rng.gen_range(0, available_stake); - stake_distribution.push(Accuracy::from_perthousand(stake)); - available_stake -= stake; - } - stake_distribution.push(Accuracy::from_perthousand(available_stake)); - stake_distribution.shuffle(&mut rng); - stake_distribution - }; - - assignments.push(Assignment { - who: *voter_id, - distribution: chosen_winners.zip(stake_distribution).collect(), - }); - } - - (voters, assignments, candidates) -} - -fn generate_cache(voters: Voters) -> HashMap -where - Voters: Iterator, - Item: Hash + Eq + Copy, -{ - let mut cache = HashMap::new(); - for (idx, voter_id) in voters.enumerate() { - cache.insert(voter_id, idx); - } - cache -} - -/// Create a function that returns the index of a voter in the voters list. -pub fn make_voter_fn(voters: &[Voter]) -> impl Fn(&AccountId) -> Option -where - usize: TryInto, -{ - let cache = generate_cache(voters.iter().map(|(id, _, _)| *id)); - move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) -} - -/// Create a function that returns the index of a candidate in the candidates list. -pub fn make_target_fn( - candidates: &[CandidateId], -) -> impl Fn(&CandidateId) -> Option -where - usize: TryInto, -{ - let cache = generate_cache(candidates.iter().cloned()); - move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) -} diff --git a/primitives/npos-elections/compact/fuzzer/src/compact_32_16_16.rs b/primitives/npos-elections/compact/fuzzer/src/compact_32_16_16.rs deleted file mode 100644 index dc70b793d3710..0000000000000 --- a/primitives/npos-elections/compact/fuzzer/src/compact_32_16_16.rs +++ /dev/null @@ -1,112 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Fuzzing which ensures that the generated implementation of `encoded_size_for` always accurately -//! predicts the correct encoded size. -//! -//! ## Running a single iteration -//! -//! Simply run the program without the `fuzzing` configuration to run a single iteration: -//! `cargo run --bin compact_16`. -//! -//! ## Running -//! -//! Run with `cargo hfuzz run compact_16`. -//! -//! ## Debugging a panic -//! -//! Once a panic is found, it can be debugged with -//! `cargo hfuzz run-debug compact_16 hfuzz_workspace/compact_16/*.fuzz`. - -use _npos::CompactSolution; -use codec::Encode; -#[cfg(fuzzing)] -use honggfuzz::fuzz; - -#[cfg(not(fuzzing))] -use structopt::StructOpt; - -mod common; - -use common::{Accuracy, generate_random_votes, make_target_fn, make_voter_fn, to_range}; -use rand::{self, SeedableRng}; - -sp_npos_elections::generate_solution_type!( - #[compact] - pub struct Compact::(16) -); - -const MIN_CANDIDATES: usize = 250; -const MAX_CANDIDATES: usize = 1000; -const MIN_VOTERS: usize = 500; -const MAX_VOTERS: usize = 2500; - -#[cfg(fuzzing)] -fn main() { - loop { - fuzz!(|data: (usize, usize, u64)| { - let (candidate_count, voter_count, seed) = data; - iteration(candidate_count, voter_count, seed); - }); - } -} - -#[cfg(not(fuzzing))] -#[derive(Debug, StructOpt)] -struct Opt { - /// How many candidates participate in this election - #[structopt(short, long)] - candidates: Option, - - /// How many voters participate in this election - #[structopt(short, long)] - voters: Option, - - /// Random seed to use in this election - #[structopt(long)] - seed: Option, -} - -#[cfg(not(fuzzing))] -fn main() { - let opt = Opt::from_args(); - // candidates and voters by default use the maxima, which turn out to be one less than - // the constant. - iteration( - opt.candidates.unwrap_or(MAX_CANDIDATES - 1), - opt.voters.unwrap_or(MAX_VOTERS - 1), - opt.seed.unwrap_or_default(), - ); -} - -fn iteration(mut candidate_count: usize, mut voter_count: usize, seed: u64) { - let rng = rand::rngs::SmallRng::seed_from_u64(seed); - candidate_count = to_range(candidate_count, MIN_CANDIDATES, MAX_CANDIDATES); - voter_count = to_range(voter_count, MIN_VOTERS, MAX_VOTERS); - - let (voters, assignments, candidates) = - generate_random_votes(candidate_count, voter_count, rng); - - let predicted_size = Compact::encoded_size_for(&assignments); - - let compact = - Compact::from_assignment(assignments, make_voter_fn(&voters), make_target_fn(&candidates)) - .unwrap(); - let encoding = compact.encode(); - - assert_eq!(predicted_size, encoding.len()); -} From 47dceb8e83f43e72f64e852b5b64fb3d126e84f8 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 14 Apr 2021 14:51:57 +0200 Subject: [PATCH 06/34] revert changes to `trait CompactSolution` If `CompactSolution::encoded_size_for` can't be implemented in the way that we wanted, there's no point in adding it. --- primitives/npos-elections/compact/src/lib.rs | 6 --- primitives/npos-elections/src/lib.rs | 44 +------------------- 2 files changed, 2 insertions(+), 48 deletions(-) diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index 88cc6178e32d7..e558ae89ca93e 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -255,12 +255,6 @@ fn struct_def( #into_impl Ok(assignments) } - - fn encoded_size_for( - assignments: &[_npos::Assignment], - ) -> usize { - unimplemented!() - } } )) } diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 56b49100dadda..05505d06f201e 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -136,42 +136,6 @@ impl __OrInvalidIndex for Option { } } -/// For types whose `parity_scale_codec::Encode` implementation is guaranteed to occupy a certain -/// fixed number of output bytes. -/// -/// This whole thing is a hack working around the fact that `Encode::size_hint` is unreliable. -pub trait FixedEncodingSize { - const ENCODED_SIZE: usize; -} - -macro_rules! impl_fes { - ($t:ty => $size:literal; $( $rest:tt )* ) => { - impl FixedEncodingSize for $t { - const ENCODED_SIZE: usize = $size; - } - impl_fes!{$($rest)*} - }; - () => {}; -} - -impl_fes! { - () => 0; - bool => 1; - u8 => 1; - u16 => 2; - u32 => 4; - u64 => 8; - u128 => 16; - i8 => 1; - i16 => 2; - i32 => 4; - i64 => 8; - i128 => 16; - f32 => 4; - f64 => 8; - char => 4; -} - /// A common interface for all compact solutions. /// /// See [`sp-npos-elections-compact`] for more info. @@ -186,8 +150,7 @@ pub trait CompactSolution: Sized { + Debug + Copy + Clone - + Bounded - + FixedEncodingSize; + + Bounded; /// The target type. Needs to be an index (convert to usize). type Target: UniqueSaturatedInto @@ -196,8 +159,7 @@ pub trait CompactSolution: Sized { + Debug + Copy + Clone - + Bounded - + FixedEncodingSize; + + Bounded; /// The weight/accuracy type of each vote. type Accuracy: PerThing128; @@ -270,8 +232,6 @@ pub trait CompactSolution: Sized { let supports = to_supports(winners, &staked)?; Ok(supports.evaluate()) } - - fn encoded_size_for(assignments: &[Assignment]) -> usize; } // re-export the compact solution type. From 25cd75efa5b898c7126051fa5cd0e4c20abc158f Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 14 Apr 2021 16:59:27 +0200 Subject: [PATCH 07/34] WIP: restructure trim_assignments_length by actually encoding This is not as efficient as what we'd hoped for, but it should still be better than what it's replacing. Overall efficiency of `fn trim_assignments_length` is now `O(edges * lg assignments.len())`. --- .../src/unsigned.rs | 42 ++++++++++++++----- primitives/npos-elections/src/lib.rs | 4 +- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index e4c64a2dcb74c..6f4f61d185a3a 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -183,7 +183,9 @@ impl Pallet { Self::trim_assignments_length( T::MinerMaxLength::get(), &mut assignments, - ); + &voter_at, + &target_at, + )?; // convert to staked and reduce. let mut staked = assignment_ratio_to_staked_normalized(assignments, &stake_of) @@ -192,7 +194,7 @@ impl Pallet { // convert back to ratios and make compact. let ratio = assignment_staked_to_ratio_normalized(staked)?; - let compact = >::from_assignment(ratio, &voter_index, &target_index)?; + let compact = >::from_assignment(&ratio, &voter_index, &target_index)?; // re-calc score. let winners = sp_npos_elections::to_without_backing(winners); @@ -310,15 +312,25 @@ impl Pallet { /// 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 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. + /// 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. fn trim_assignments_length( max_allowed_length: u32, assignments: &mut Vec>, - ) { + voter_index: impl Fn(&T::AccountId) -> Option>, + target_index: impl Fn(&T::AccountId) -> Option>, + ) -> Result<(), MinerError> { + // Compute the size of a compact solution comprised of the selected arguments. + // + // This function completes in `O(edges)`; it's expensive, but linear. + let encoded_size_of = |assignments: &[Assignment]| { + CompactOf::::from_assignment(assignments, voter_index, target_index) + .map(|compact| compact.encoded_size()) + }; + // The encoded size of an assignment list is staticly computable, but only in `O(assignments.len())`. // That makes the naive approach, to loop and pop, somewhat inefficient. // @@ -329,7 +341,7 @@ impl Pallet { let avg = move || (high + low) / 2; while high - low > 1 { let test = avg(); - if CompactOf::::encoded_size_for(&assignments[..test]) > max_allowed_length.saturated_into() { + if encoded_size_of(&assignments[..test])? > max_allowed_length.saturated_into() { high = test; } else { low = test; @@ -339,20 +351,28 @@ impl Pallet { // ensure our postconditions are correct debug_assert!( - CompactOf::::encoded_size_for(&assignments[..maximum_allowed_voters]) <= - max_allowed_length.saturated_into() + encoded_size_of(&assignments[..maximum_allowed_voters]).unwrap() + <= max_allowed_length.saturated_into() ); debug_assert!( - CompactOf::::encoded_size_for(&assignments[..maximum_allowed_voters + 1]) > - max_allowed_length.saturated_into() + encoded_size_of(&assignments[..maximum_allowed_voters + 1]).unwrap() + > max_allowed_length.saturated_into() ); + // NOTE: before this point, every access was immutable. + // after this point, we never error. + // check before edit. + log!( debug, "from {} assignments, truncating to {} for length, removing {}", - assignments.len(), maximum_allowed_voters, assignments.len() - maximum_allowed_voters, + assignments.len(), + maximum_allowed_voters, + assignments.len() - maximum_allowed_voters, ); assignments.truncate(maximum_allowed_voters); + + Ok(()) } /// Find the maximum `len` that a compact can have in order to fit into the block weight. diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 05505d06f201e..4b721d6f90ea1 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -164,9 +164,9 @@ pub trait CompactSolution: Sized { /// The weight/accuracy type of each vote. type Accuracy: PerThing128; - /// Build self from a `assignments: Vec>`. + /// Build self from a list of assignments. fn from_assignment( - assignments: Vec>, + assignments: &[Assignment], voter_index: FV, target_index: FT, ) -> Result From f9055e2339cbe3d76fe18a2e14c2072ffbd0ce79 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 15 Apr 2021 10:09:58 +0200 Subject: [PATCH 08/34] fix compiler errors --- .../src/unsigned.rs | 49 +++++++++---------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 6f4f61d185a3a..6090acf62884a 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -171,7 +171,25 @@ impl Pallet { let target_at = helpers::target_at_fn::(&targets); let stake_of = helpers::stake_of_fn::(&voters, &cache); - // trim assignments list for weight and length + // Compute the size of a compact solution comprised of the selected arguments. + // + // This function completes in `O(edges)`; it's expensive, but linear. + let encoded_size_of = |assignments: &[Assignment]| { + CompactOf::::from_assignment(assignments, &voter_index, &target_index) + .map(|compact| compact.encoded_size()) + }; + + // Reduce (requires round-trip to staked form) + assignments = { + // convert to staked and reduce. + let mut staked = assignment_ratio_to_staked_normalized(assignments, &stake_of)?; + sp_npos_elections::reduce(&mut staked); + + // convert back. + assignment_staked_to_ratio_normalized(staked)? + }; + + // trim assignments list for weight and length. let size = SolutionOrSnapshotSize { voters: voters.len() as u32, targets: targets.len() as u32 }; Self::trim_assignments_weight( @@ -183,18 +201,11 @@ impl Pallet { Self::trim_assignments_length( T::MinerMaxLength::get(), &mut assignments, - &voter_at, - &target_at, + &encoded_size_of, )?; - // convert to staked and reduce. - let mut staked = assignment_ratio_to_staked_normalized(assignments, &stake_of) - .map_err::(Into::into)?; - sp_npos_elections::reduce(&mut staked); - - // convert back to ratios and make compact. - let ratio = assignment_staked_to_ratio_normalized(staked)?; - let compact = >::from_assignment(&ratio, &voter_index, &target_index)?; + // now make compact. + let compact = CompactOf::::from_assignment(&assignments, &voter_index, &target_index)?; // re-calc score. let winners = sp_npos_elections::to_without_backing(winners); @@ -320,21 +331,9 @@ impl Pallet { fn trim_assignments_length( max_allowed_length: u32, assignments: &mut Vec>, - voter_index: impl Fn(&T::AccountId) -> Option>, - target_index: impl Fn(&T::AccountId) -> Option>, + encoded_size_of: impl Fn(&[Assignment]) -> Result, ) -> Result<(), MinerError> { - // Compute the size of a compact solution comprised of the selected arguments. - // - // This function completes in `O(edges)`; it's expensive, but linear. - let encoded_size_of = |assignments: &[Assignment]| { - CompactOf::::from_assignment(assignments, voter_index, target_index) - .map(|compact| compact.encoded_size()) - }; - - // The encoded size of an assignment list is staticly computable, but only in `O(assignments.len())`. - // That makes the naive approach, to loop and pop, somewhat inefficient. - // - // Instead, we perform a binary search for the max subset of which can fit into the allowed + // Perform a binary search for the max subset of which can fit into the allowed // length. Having discovered that, we can truncate efficiently. let mut high = assignments.len(); let mut low = 0; From 24bf2ad66d990fcc6bca23083e3a531002257ad8 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 15 Apr 2021 15:42:09 +0200 Subject: [PATCH 09/34] don't sort voters, just assignments Sorting the `voters` list causes lots of problems; an invariant that we need to maintain is that an index into the voters list has a stable meaning. Luckily, it turns out that there is no need for the assignments list to correspond to the voters list. That isn't an invariant, though previously I'd thought that it was. This simplifies things; we can just leave the voters list alone, and sort the assignments list the way that is convenient. --- .../election-provider-multi-phase/src/lib.rs | 11 +++ .../src/unsigned.rs | 76 +++---------------- 2 files changed, 20 insertions(+), 67 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index c59d68a33adba..af93e2389b4f8 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -267,6 +267,17 @@ pub type CompactTargetIndexOf = as CompactSolution>::Target; pub type CompactAccuracyOf = as CompactSolution>::Accuracy; /// The accuracy of the election, when computed on-chain. Equal to [`Config::OnChainAccuracy`]. pub type OnChainAccuracyOf = ::OnChainAccuracy; +/// The relative distribution of a voter's stake among the winning targets. +pub type Assignment = sp_npos_elections::Assignment< + ::AccountId, + CompactAccuracyOf, +>; +/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they voted. +pub type Voter = ( + ::AccountId, + sp_npos_elections::VoteWeight, + Vec<::AccountId>, +); /// Wrapper type that implements the configurations needed for the on-chain backup. struct OnChainConfig(sp_std::marker::PhantomData); diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 6090acf62884a..6cfdebcdcc2e1 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -34,18 +34,6 @@ pub(crate) const OFFCHAIN_HEAD_DB: &[u8] = b"parity/multi-phase-unsigned-electio /// within a window of 5 blocks. pub(crate) const OFFCHAIN_REPEAT: u32 = 5; -// type helpers for method definitions -// these types are defined elsewhere, but we simplify them here for convenience -pub(crate) type Assignment = sp_npos_elections::Assignment< - ::AccountId, - CompactAccuracyOf, ->; -pub(crate) type Voter = ( - ::AccountId, - sp_npos_elections::VoteWeight, - Vec<::AccountId>, -); - #[derive(Debug, Eq, PartialEq)] pub enum MinerError { /// An internal error in the NPoS elections crate. @@ -152,17 +140,10 @@ impl Pallet { // some point though. // storage items. Note: we have already read this from storage, they must be in cache. - let RoundSnapshot { mut voters, targets } = + let RoundSnapshot { voters, targets } = Self::snapshot().ok_or(MinerError::SnapshotUnAvailable)?; let desired_targets = Self::desired_targets().ok_or(MinerError::SnapshotUnAvailable)?; - // Both `voters` and `assignments` are vectors. - // They're synchronized: for any arbitrary index `i`, `voters[i]` corresponds to `assignments[i]`. - // However, it turns out to be convenient for us if the assignments are sorted by decreasing - // stake. In order to maintain the correspondence, we have to also sort the voters. - let ElectionResult { mut assignments, winners } = election_result; - Self::sort_by_decreasing_stake(voters.as_mut_slice(), assignments.as_mut_slice()); - // now make some helper closures. let cache = helpers::generate_voter_cache::(&voters); let voter_index = helpers::voter_index_fn::(&cache); @@ -179,6 +160,14 @@ impl Pallet { .map(|compact| compact.encoded_size()) }; + let ElectionResult { mut assignments, winners } = election_result; + // Sort the assignments by reversed voter stake. This ensures that we can efficiently truncate the list. + let stakes: BTreeMap<_, _> = + voters.iter().map(|(who, stake, _)| (who.clone(), *stake)).collect(); + assignments.sort_unstable_by_key(|Assignment:: { who, .. }| sp_std::cmp::Reverse( + stakes.get(who).cloned().unwrap_or_default() + )); + // Reduce (requires round-trip to staked form) assignments = { // convert to staked and reduce. @@ -232,53 +221,6 @@ impl Pallet { } } - /// Sort the `voters` and `assignments` lists by decreasing voter stake. - /// - /// [`trim_assignments_weight`] and [`trim_assignments_length`] both depend on this property - /// on the `assignments` list for efficient computation. Meanwhile, certain other helper - /// closures depend on `voters` and `assignments` corresponding, so we have to sort both. - fn sort_by_decreasing_stake(voters: &mut [Voter], assignments: &mut [Assignment]) { - // verify precondition - debug_assert!({ - let voter_ids = voters.iter().map(|(who, _, _)| who).cloned(); - let assignment_ids = assignments.iter().map(|assignment| assignment.who.clone()); - voter_ids.zip(assignment_ids).all(|(voter_id, assignment_id)| voter_id == assignment_id) - }); - - let stakes: BTreeMap<_, _> = - voters.iter().map(|(who, stake, _)| (who.clone(), *stake)).collect(); - - // `Reverse` just reverses the meaning of this key's ordering, so the greatest items come - // first without needing to explicitly call `reverse` afterwards. - // - // Getting an assignment's stake from the `stakes` map should never fail. It will definitely - // never fail for a member of `voters`. However, we use `unwrap_or_default` defensively. In - // case a voter can't be found, it's assumed to have 0 stake, which puts it first on the - // chopping block for removal. - // - // This would be a closure of its own, but rustc doesn't like that and gives E0521, so it's - // simpler to just declare a little macro. The point is that we can show that both lists - // get sorted according to identical rules. - macro_rules! stake_of { - ($who:expr) => { - sp_std::cmp::Reverse( - stakes.get($who).cloned().unwrap_or_default() - ) - }; - } - - // we sort stably so the lists stay synchronized - voters.sort_by_key(|(who, _, _)| stake_of!(who)); - assignments.sort_by_key(|Assignment:: { who, .. }| stake_of!(who)); - - // verify postcondition - debug_assert!({ - let voter_ids = voters.iter().map(|(who, _, _)| who).cloned(); - let assignment_ids = assignments.iter().map(|assignment| assignment.who.clone()); - voter_ids.zip(assignment_ids).all(|(voter_id, assignment_id)| voter_id == assignment_id) - }); - } - /// Greedily reduce the size of the solution to fit into the block w.r.t. weight. /// /// The weight of the solution is foremost a function of the number of voters (i.e. From 983cac13b196c1f38417855dff680ed3c6427d16 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 15 Apr 2021 17:08:47 +0200 Subject: [PATCH 10/34] WIP: add `IndexAssignment` type to speed up repeatedly creating `Compact` Next up: `impl<'a, T> From<&'a [IndexAssignmentOf]> for Compact`, in the proc-macro which makes `Compact`. Should be a pretty straightforward adaptation of `from_assignment`. --- .../election-provider-multi-phase/Cargo.toml | 4 +- .../src/index_assignment.rs | 81 +++++++++++++++++++ .../src/unsigned.rs | 27 ++++--- 3 files changed, 99 insertions(+), 13 deletions(-) create mode 100644 frame/election-provider-multi-phase/src/index_assignment.rs diff --git a/frame/election-provider-multi-phase/Cargo.toml b/frame/election-provider-multi-phase/Cargo.toml index 4b5178faa8e86..ca8b998e735f5 100644 --- a/frame/election-provider-multi-phase/Cargo.toml +++ b/frame/election-provider-multi-phase/Cargo.toml @@ -21,7 +21,8 @@ log = { version = "0.4.14", default-features = false } frame-support = { version = "3.0.0", default-features = false, path = "../support" } frame-system = { version = "3.0.0", default-features = false, path = "../system" } -sp-io ={ version = "3.0.0", default-features = false, path = "../../primitives/io" } +sp-core = { version = "3.0.0", default-features = false, path = "../../primitives/core" } +sp-io = { version = "3.0.0", default-features = false, path = "../../primitives/io" } sp-std = { version = "3.0.0", default-features = false, path = "../../primitives/std" } sp-runtime = { version = "3.0.0", default-features = false, path = "../../primitives/runtime" } sp-npos-elections = { version = "3.0.0", default-features = false, path = "../../primitives/npos-elections" } @@ -39,7 +40,6 @@ rand = { version = "0.7.3" } hex-literal = "0.3.1" substrate-test-utils = { version = "3.0.0", path = "../../test-utils" } sp-io = { version = "3.0.0", path = "../../primitives/io" } -sp-core = { version = "3.0.0", path = "../../primitives/core" } sp-tracing = { version = "3.0.0", path = "../../primitives/tracing" } frame-election-provider-support = { version = "3.0.0", features = ["runtime-benchmarks"], path = "../election-provider-support" } pallet-balances = { version = "3.0.0", path = "../balances" } diff --git a/frame/election-provider-multi-phase/src/index_assignment.rs b/frame/election-provider-multi-phase/src/index_assignment.rs new file mode 100644 index 0000000000000..48ceb7555e894 --- /dev/null +++ b/frame/election-provider-multi-phase/src/index_assignment.rs @@ -0,0 +1,81 @@ +// This file is part of Substrate. + +// Copyright (C) 2020 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! The [`IndexAssignment`] type is an intermediate between the assignments list +//! ([`&[Assignment]`][Assignment]) and [`CompactOf`][crate::CompactOf]. + +use crate::{CompactAccuracyOf, CompactVoterIndexOf, CompactTargetIndexOf}; +use codec::{Encode, Decode}; +use sp_arithmetic::PerThing; +use sp_core::RuntimeDebug; +use sp_npos_elections::{Error, IdentifierT}; + +/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they voted. +pub type Voter = ( + ::AccountId, + sp_npos_elections::VoteWeight, + Vec<::AccountId>, +); + +/// The relative distribution of a voter's stake among the winning targets. +pub type Assignment = sp_npos_elections::Assignment< + ::AccountId, + CompactAccuracyOf, +>; + +// The [`IndexAssignment`] type specialized for a particular runtime `T`. +pub type IndexAssignmentOf = IndexAssignment< + CompactVoterIndexOf, + CompactTargetIndexOf, + CompactAccuracyOf, +>; + +/// The [`IndexAssignment`] type is an intermediate between the assignments list +/// ([`&[Assignment]`][Assignment]) and [`CompactOf`][crate::CompactOf]. +/// +/// The voter and target identifiers have already been replaced with appropriate indices, +/// making it fast to repeatedly encode into a `CompactOf`. This property turns out +/// to be important when trimming for compact length. +#[derive(RuntimeDebug, Clone, Default)] +#[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] +pub struct IndexAssignment { + /// Index of the voter among the voters list. + pub who: VoterIndex, + /// The distribution of the voter's stake among winning targets. + /// + /// Targets are identified by their index in the canonical list. + pub distribution: Vec<(TargetIndex, P)>, +} + +impl IndexAssignment { + pub fn new( + assignment: &sp_npos_elections::Assignment, + voter_index: impl Fn(&AccountId) -> Option, + target_index: impl Fn(&AccountId) -> Option, + ) -> Result { + use sp_npos_elections::__OrInvalidIndex as _; + Ok(Self { + who: voter_index(&assignment.who).or_invalid_index()?, + distribution: assignment + .distribution + .iter() + .map(|(target, proportion)| Some((target_index(target)?, proportion.clone()))) + .collect::>>() + .or_invalid_index()?, + }) + } +} diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 6cfdebcdcc2e1..470b84f02e39d 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -154,19 +154,18 @@ impl Pallet { // Compute the size of a compact solution comprised of the selected arguments. // - // This function completes in `O(edges)`; it's expensive, but linear. - let encoded_size_of = |assignments: &[Assignment]| { - CompactOf::::from_assignment(assignments, &voter_index, &target_index) - .map(|compact| compact.encoded_size()) + // This function completes in `O(edges * log voters.len())`; it's expensive, but linear. + let encoded_size_of = |assignments: &[IndexAssignmentOf]| { + CompactOf::::from(assignments).map(|compact| compact.encoded_size()) }; let ElectionResult { mut assignments, winners } = election_result; // Sort the assignments by reversed voter stake. This ensures that we can efficiently truncate the list. let stakes: BTreeMap<_, _> = voters.iter().map(|(who, stake, _)| (who.clone(), *stake)).collect(); - assignments.sort_unstable_by_key(|Assignment:: { who, .. }| sp_std::cmp::Reverse( - stakes.get(who).cloned().unwrap_or_default() - )); + assignments.sort_unstable_by_key(|Assignment:: { who, .. }| { + sp_std::cmp::Reverse(stakes.get(who).cloned().unwrap_or_default()) + }); // Reduce (requires round-trip to staked form) assignments = { @@ -178,6 +177,12 @@ impl Pallet { assignment_staked_to_ratio_normalized(staked)? }; + // convert to `IndexAssignment`. This improves the runtime complexity of converting to `Compact`. + let mut assignments = assignments + .iter() + .map(|assignment| IndexAssignmentOf::::new(assignment, &voter_index, &target_index)) + .collect::, _>>()?; + // trim assignments list for weight and length. let size = SolutionOrSnapshotSize { voters: voters.len() as u32, targets: targets.len() as u32 }; @@ -194,7 +199,7 @@ impl Pallet { )?; // now make compact. - let compact = CompactOf::::from_assignment(&assignments, &voter_index, &target_index)?; + let compact = CompactOf::::from(assignments)?; // re-calc score. let winners = sp_npos_elections::to_without_backing(winners); @@ -240,7 +245,7 @@ impl Pallet { desired_targets: u32, size: SolutionOrSnapshotSize, max_weight: Weight, - assignments: &mut Vec>, + assignments: &mut Vec>, ) { let maximum_allowed_voters = Self::maximum_voter_for_weight::( desired_targets, @@ -272,8 +277,8 @@ impl Pallet { /// then the solution must be discarded. fn trim_assignments_length( max_allowed_length: u32, - assignments: &mut Vec>, - encoded_size_of: impl Fn(&[Assignment]) -> Result, + assignments: &mut Vec>, + encoded_size_of: impl Fn(&[IndexAssignmentOf]) -> Result, ) -> Result<(), MinerError> { // Perform a binary search for the max subset of which can fit into the allowed // length. Having discovered that, we can truncate efficiently. From e4287c7099ab5f2b1b4346f981b5988ef3ad2619 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 16 Apr 2021 11:48:27 +0200 Subject: [PATCH 11/34] Add IndexAssignment and conversion method to CompactSolution This involves a bit of duplication of types from `election-provider-multi-phase`; we'll clean those up shortly. I'm not entirely happy that we had to add a `from_index_assignments` method to `CompactSolution`, but we couldn't define `trait CompactSolution: TryFrom<&'a [Self::IndexAssignment]` because that made trait lookup recursive, and I didn't want to propagate `CompactSolutionOf + TryFrom<&[IndexAssignmentOf]>` everywhere that compact solutions are specified. --- .../compact/src/index_assignment.rs | 74 +++++++ primitives/npos-elections/compact/src/lib.rs | 29 +++ primitives/npos-elections/src/assignments.rs | 200 ++++++++++++++++++ primitives/npos-elections/src/lib.rs | 152 +------------ 4 files changed, 312 insertions(+), 143 deletions(-) create mode 100644 primitives/npos-elections/compact/src/index_assignment.rs create mode 100644 primitives/npos-elections/src/assignments.rs diff --git a/primitives/npos-elections/compact/src/index_assignment.rs b/primitives/npos-elections/compact/src/index_assignment.rs new file mode 100644 index 0000000000000..35e9ce991020b --- /dev/null +++ b/primitives/npos-elections/compact/src/index_assignment.rs @@ -0,0 +1,74 @@ +// This file is part of Substrate. + +// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Code generation for getting the compact representation from the `IndexAssignment` type. + +use crate::field_name_for; +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; + +pub(crate) fn from_impl(count: usize) -> TokenStream2 { + let from_impl_single = { + let name = field_name_for(1); + quote!(1 => compact.#name.push( + ( + *who, + *distribution[0].0, + ) + ),) + }; + + let from_impl_double = { + let name = field_name_for(2); + quote!(2 => compact.#name.push( + ( + *who, + ( + *distribution[0].0, + *distribution[0].1, + ), + *distribution[1].0, + ) + ),) + }; + + let from_impl_rest = (3..=count).map(|c| { + let inner = (0..c-1).map(|i| + quote!((*distribution[#i].0, *distribution[#i].1),) + ).collect::(); + + let field_name = field_name_for(c); + let last_index = c - 1; + let last = quote!(*distribution[#last_index].0); + + quote!( + #c => compact.#field_name.push( + ( + &who, + [#inner], + #last, + ) + ), + ) + }).collect::(); + + quote!( + #from_impl_single + #from_impl_double + #from_impl_rest + ) +} diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index e558ae89ca93e..198b69967d229 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -25,6 +25,7 @@ use syn::parse::{Parse, ParseStream, Result}; mod assignment; mod codec; +mod index_assignment; // prefix used for struct fields in compact. const PREFIX: &'static str = "votes"; @@ -177,6 +178,7 @@ fn struct_def( let from_impl = assignment::from_impl(count); let into_impl = assignment::into_impl(count, weight_type.clone()); + let from_index_impl = index_assignment::from_impl(count); Ok(quote! ( /// A struct to encode a election assignment in a compact way. @@ -222,6 +224,10 @@ fn struct_def( return false } + fn from_index_assignments(index_assignments: &[__IndexAssignment]) -> Result { + Self::try_from(index_assignments) + } + fn from_assignment( assignments: _npos::sp_std::prelude::Vec<_npos::Assignment>, index_of_voter: FV, @@ -256,6 +262,29 @@ fn struct_def( Ok(assignments) } } + type __IndexAssignment = _npos::IndexAssignment< + <#ident as _npos::CompactSolution>::Voter, + <#ident as _npos::CompactSolution>::Target, + <#ident as _npos::CompactSolution>::Accuracy, + >; + impl<'a> _npos::sp_std::convert::TryFrom<&'a [__IndexAssignment]> for #ident { + type Error = _npos::Error; + fn try_from(index_assignments: &'a [__IndexAssignment]) -> Result { + let mut compact = #ident::default(); + + for _npos::IndexAssignment { who, distribution } in index_assignments { + match distribution.len() { + 0 => {} + #from_index_impl + _ => { + return Err(_npos::Error::CompactTargetOverflow); + } + } + }; + + Ok(compact) + } + } )) } diff --git a/primitives/npos-elections/src/assignments.rs b/primitives/npos-elections/src/assignments.rs new file mode 100644 index 0000000000000..19407fce9d43f --- /dev/null +++ b/primitives/npos-elections/src/assignments.rs @@ -0,0 +1,200 @@ +// This file is part of Substrate. + +// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Structs and helpers for distributing a voter's stake among various winners. + +use crate::{Error, ExtendedBalance, IdentifierT, PerThing128, __OrInvalidIndex}; +use codec::{Encode, Decode}; +use sp_arithmetic::{traits::{Bounded, Zero}, Normalizable, PerThing}; +use sp_core::RuntimeDebug; + +/// A voter's stake assignment among a set of targets, represented as ratios. +#[derive(RuntimeDebug, Clone, Default)] +#[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] +pub struct Assignment { + /// Voter's identifier. + pub who: AccountId, + /// The distribution of the voter's stake. + pub distribution: Vec<(AccountId, P)>, +} + +impl Assignment { + /// Convert from a ratio assignment into one with absolute values aka. [`StakedAssignment`]. + /// + /// It needs `stake` which is the total budget of the voter. + /// + /// Note that this might create _un-normalized_ assignments, due to accuracy loss of `P`. Call + /// site might compensate by calling `try_normalize()` on the returned `StakedAssignment` as a + /// post-precessing. + /// + /// If an edge ratio is [`Bounded::min_value()`], it is dropped. This edge can never mean + /// anything useful. + pub fn into_staked(self, stake: ExtendedBalance) -> StakedAssignment { + let distribution = self + .distribution + .into_iter() + .filter_map(|(target, p)| { + // if this ratio is zero, then skip it. + if p.is_zero() { + None + } else { + // NOTE: this mul impl will always round to the nearest number, so we might both + // overflow and underflow. + let distribution_stake = p * stake; + Some((target, distribution_stake)) + } + }) + .collect::>(); + + StakedAssignment { + who: self.who, + distribution, + } + } + + /// Try and normalize this assignment. + /// + /// If `Ok(())` is returned, then the assignment MUST have been successfully normalized to 100%. + /// + /// ### Errors + /// + /// This will return only if the internal `normalize` fails. This can happen if sum of + /// `self.distribution.map(|p| p.deconstruct())` fails to fit inside `UpperOf

`. A user of + /// this crate may statically assert that this can never happen and safely `expect` this to + /// return `Ok`. + pub fn try_normalize(&mut self) -> Result<(), &'static str> { + self.distribution + .iter() + .map(|(_, p)| *p) + .collect::>() + .normalize(P::one()) + .map(|normalized_ratios| + self.distribution + .iter_mut() + .zip(normalized_ratios) + .for_each(|((_, old), corrected)| { *old = corrected; }) + ) + } +} + +/// A voter's stake assignment among a set of targets, represented as absolute values in the scale +/// of [`ExtendedBalance`]. +#[derive(RuntimeDebug, Clone, Default)] +#[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] +pub struct StakedAssignment { + /// Voter's identifier + pub who: AccountId, + /// The distribution of the voter's stake. + pub distribution: Vec<(AccountId, ExtendedBalance)>, +} + +impl StakedAssignment { + /// Converts self into the normal [`Assignment`] type. + /// + /// NOTE: This will always round down, and thus the results might be less than a full 100% `P`. + /// Use a normalization post-processing to fix this. The data type returned here will + /// potentially get used to create a compact type; a compact type requires sum of ratios to be + /// less than 100% upon un-compacting. + /// + /// If an edge stake is so small that it cannot be represented in `T`, it is ignored. This edge + /// can never be re-created and does not mean anything useful anymore. + pub fn into_assignment(self) -> Assignment + where + AccountId: IdentifierT, + { + let stake = self.total(); + let distribution = self.distribution + .into_iter() + .filter_map(|(target, w)| { + let per_thing = P::from_rational(w, stake); + if per_thing == Bounded::min_value() { + None + } else { + Some((target, per_thing)) + } + }) + .collect::>(); + + Assignment { + who: self.who, + distribution, + } + } + + /// Try and normalize this assignment. + /// + /// If `Ok(())` is returned, then the assignment MUST have been successfully normalized to + /// `stake`. + /// + /// NOTE: current implementation of `.normalize` is almost safe to `expect()` upon. The only + /// error case is when the input cannot fit in `T`, or the sum of input cannot fit in `T`. + /// Sadly, both of these are dependent upon the implementation of `VoteLimit`, i.e. the limit of + /// edges per voter which is enforced from upstream. Hence, at this crate, we prefer returning a + /// result and a use the name prefix `try_`. + pub fn try_normalize(&mut self, stake: ExtendedBalance) -> Result<(), &'static str> { + self.distribution + .iter() + .map(|(_, ref weight)| *weight) + .collect::>() + .normalize(stake) + .map(|normalized_weights| + self.distribution + .iter_mut() + .zip(normalized_weights.into_iter()) + .for_each(|((_, weight), corrected)| { *weight = corrected; }) + ) + } + + /// Get the total stake of this assignment (aka voter budget). + pub fn total(&self) -> ExtendedBalance { + self.distribution.iter().fold(Zero::zero(), |a, b| a.saturating_add(b.1)) + } +} +/// The [`IndexAssignment`] type is an intermediate between the assignments list +/// ([`&[Assignment]`][Assignment]) and `CompactOf`. +/// +/// The voter and target identifiers have already been replaced with appropriate indices, +/// making it fast to repeatedly encode into a `CompactOf`. This property turns out +/// to be important when trimming for compact length. +#[derive(RuntimeDebug, Clone, Default)] +#[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] +pub struct IndexAssignment { + /// Index of the voter among the voters list. + pub who: VoterIndex, + /// The distribution of the voter's stake among winning targets. + /// + /// Targets are identified by their index in the canonical list. + pub distribution: Vec<(TargetIndex, P)>, +} + +impl IndexAssignment { + pub fn new( + assignment: &Assignment, + voter_index: impl Fn(&AccountId) -> Option, + target_index: impl Fn(&AccountId) -> Option, + ) -> Result { + Ok(Self { + who: voter_index(&assignment.who).or_invalid_index()?, + distribution: assignment + .distribution + .iter() + .map(|(target, proportion)| Some((target_index(target)?, proportion.clone()))) + .collect::>>() + .or_invalid_index()?, + }) + } +} diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 4b721d6f90ea1..24ebe62d09df6 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -99,6 +99,7 @@ mod mock; #[cfg(test)] mod tests; +mod assignments; pub mod phragmen; pub mod balancing; pub mod phragmms; @@ -107,6 +108,7 @@ pub mod reduce; pub mod helpers; pub mod pjr; +pub use assignments::{Assignment, IndexAssignment, StakedAssignment}; pub use reduce::reduce; pub use helpers::*; pub use phragmen::*; @@ -164,6 +166,13 @@ pub trait CompactSolution: Sized { /// The weight/accuracy type of each vote. type Accuracy: PerThing128; + /// Build self from a list of `IndexAssignment`s. + fn from_index_assignments(index_assignments: &[IndexAssignment< + Self::Voter, + Self::Target, + Self::Accuracy, + >]) -> Result; + /// Build self from a list of assignments. fn from_assignment( assignments: &[Assignment], @@ -455,149 +464,6 @@ pub struct ElectionResult { pub assignments: Vec>, } -/// A voter's stake assignment among a set of targets, represented as ratios. -#[derive(RuntimeDebug, Clone, Default)] -#[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] -pub struct Assignment { - /// Voter's identifier. - pub who: AccountId, - /// The distribution of the voter's stake. - pub distribution: Vec<(AccountId, P)>, -} - -impl Assignment { - /// Convert from a ratio assignment into one with absolute values aka. [`StakedAssignment`]. - /// - /// It needs `stake` which is the total budget of the voter. - /// - /// Note that this might create _un-normalized_ assignments, due to accuracy loss of `P`. Call - /// site might compensate by calling `try_normalize()` on the returned `StakedAssignment` as a - /// post-precessing. - /// - /// If an edge ratio is [`Bounded::min_value()`], it is dropped. This edge can never mean - /// anything useful. - pub fn into_staked(self, stake: ExtendedBalance) -> StakedAssignment { - let distribution = self - .distribution - .into_iter() - .filter_map(|(target, p)| { - // if this ratio is zero, then skip it. - if p.is_zero() { - None - } else { - // NOTE: this mul impl will always round to the nearest number, so we might both - // overflow and underflow. - let distribution_stake = p * stake; - Some((target, distribution_stake)) - } - }) - .collect::>(); - - StakedAssignment { - who: self.who, - distribution, - } - } - - /// Try and normalize this assignment. - /// - /// If `Ok(())` is returned, then the assignment MUST have been successfully normalized to 100%. - /// - /// ### Errors - /// - /// This will return only if the internal `normalize` fails. This can happen if sum of - /// `self.distribution.map(|p| p.deconstruct())` fails to fit inside `UpperOf

`. A user of - /// this crate may statically assert that this can never happen and safely `expect` this to - /// return `Ok`. - pub fn try_normalize(&mut self) -> Result<(), &'static str> { - self.distribution - .iter() - .map(|(_, p)| *p) - .collect::>() - .normalize(P::one()) - .map(|normalized_ratios| - self.distribution - .iter_mut() - .zip(normalized_ratios) - .for_each(|((_, old), corrected)| { *old = corrected; }) - ) - } -} - -/// A voter's stake assignment among a set of targets, represented as absolute values in the scale -/// of [`ExtendedBalance`]. -#[derive(RuntimeDebug, Clone, Default)] -#[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] -pub struct StakedAssignment { - /// Voter's identifier - pub who: AccountId, - /// The distribution of the voter's stake. - pub distribution: Vec<(AccountId, ExtendedBalance)>, -} - -impl StakedAssignment { - /// Converts self into the normal [`Assignment`] type. - /// - /// NOTE: This will always round down, and thus the results might be less than a full 100% `P`. - /// Use a normalization post-processing to fix this. The data type returned here will - /// potentially get used to create a compact type; a compact type requires sum of ratios to be - /// less than 100% upon un-compacting. - /// - /// If an edge stake is so small that it cannot be represented in `T`, it is ignored. This edge - /// can never be re-created and does not mean anything useful anymore. - pub fn into_assignment(self) -> Assignment - where - AccountId: IdentifierT, - { - let stake = self.total(); - let distribution = self.distribution - .into_iter() - .filter_map(|(target, w)| { - let per_thing = P::from_rational(w, stake); - if per_thing == Bounded::min_value() { - None - } else { - Some((target, per_thing)) - } - }) - .collect::>(); - - Assignment { - who: self.who, - distribution, - } - } - - /// Try and normalize this assignment. - /// - /// If `Ok(())` is returned, then the assignment MUST have been successfully normalized to - /// `stake`. - /// - /// NOTE: current implementation of `.normalize` is almost safe to `expect()` upon. The only - /// error case is when the input cannot fit in `T`, or the sum of input cannot fit in `T`. - /// Sadly, both of these are dependent upon the implementation of `VoteLimit`, i.e. the limit of - /// edges per voter which is enforced from upstream. Hence, at this crate, we prefer returning a - /// result and a use the name prefix `try_`. - pub fn try_normalize(&mut self, stake: ExtendedBalance) -> Result<(), &'static str> { - self.distribution - .iter() - .map(|(_, ref weight)| *weight) - .collect::>() - .normalize(stake) - .map(|normalized_weights| - self.distribution - .iter_mut() - .zip(normalized_weights.into_iter()) - .for_each(|((_, weight), corrected)| { *weight = corrected; }) - ) - } - - /// Get the total stake of this assignment (aka voter budget). - pub fn total(&self) -> ExtendedBalance { - self.distribution.iter().fold(Zero::zero(), |a, b| a.saturating_add(b.1)) - } -} - /// A structure to demonstrate the election result from the perspective of the candidate, i.e. how /// much support each candidate is receiving. /// From 232ec2bae5e1327f9016bc4f59be41aa4d40235b Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 16 Apr 2021 11:55:19 +0200 Subject: [PATCH 12/34] use `CompactSolution::from_index_assignment` and clean up dead code --- .../src/index_assignment.rs | 81 ------------------- .../election-provider-multi-phase/src/lib.rs | 23 ++++-- .../src/unsigned.rs | 4 +- 3 files changed, 18 insertions(+), 90 deletions(-) delete mode 100644 frame/election-provider-multi-phase/src/index_assignment.rs diff --git a/frame/election-provider-multi-phase/src/index_assignment.rs b/frame/election-provider-multi-phase/src/index_assignment.rs deleted file mode 100644 index 48ceb7555e894..0000000000000 --- a/frame/election-provider-multi-phase/src/index_assignment.rs +++ /dev/null @@ -1,81 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) 2020 Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! The [`IndexAssignment`] type is an intermediate between the assignments list -//! ([`&[Assignment]`][Assignment]) and [`CompactOf`][crate::CompactOf]. - -use crate::{CompactAccuracyOf, CompactVoterIndexOf, CompactTargetIndexOf}; -use codec::{Encode, Decode}; -use sp_arithmetic::PerThing; -use sp_core::RuntimeDebug; -use sp_npos_elections::{Error, IdentifierT}; - -/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they voted. -pub type Voter = ( - ::AccountId, - sp_npos_elections::VoteWeight, - Vec<::AccountId>, -); - -/// The relative distribution of a voter's stake among the winning targets. -pub type Assignment = sp_npos_elections::Assignment< - ::AccountId, - CompactAccuracyOf, ->; - -// The [`IndexAssignment`] type specialized for a particular runtime `T`. -pub type IndexAssignmentOf = IndexAssignment< - CompactVoterIndexOf, - CompactTargetIndexOf, - CompactAccuracyOf, ->; - -/// The [`IndexAssignment`] type is an intermediate between the assignments list -/// ([`&[Assignment]`][Assignment]) and [`CompactOf`][crate::CompactOf]. -/// -/// The voter and target identifiers have already been replaced with appropriate indices, -/// making it fast to repeatedly encode into a `CompactOf`. This property turns out -/// to be important when trimming for compact length. -#[derive(RuntimeDebug, Clone, Default)] -#[cfg_attr(feature = "std", derive(PartialEq, Eq, Encode, Decode))] -pub struct IndexAssignment { - /// Index of the voter among the voters list. - pub who: VoterIndex, - /// The distribution of the voter's stake among winning targets. - /// - /// Targets are identified by their index in the canonical list. - pub distribution: Vec<(TargetIndex, P)>, -} - -impl IndexAssignment { - pub fn new( - assignment: &sp_npos_elections::Assignment, - voter_index: impl Fn(&AccountId) -> Option, - target_index: impl Fn(&AccountId) -> Option, - ) -> Result { - use sp_npos_elections::__OrInvalidIndex as _; - Ok(Self { - who: voter_index(&assignment.who).or_invalid_index()?, - distribution: assignment - .distribution - .iter() - .map(|(target, proportion)| Some((target_index(target)?, proportion.clone()))) - .collect::>>() - .or_invalid_index()?, - }) - } -} diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index af93e2389b4f8..f2d2111156451 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -225,7 +225,7 @@ use frame_system::{ensure_none, offchain::SendTransactionTypes}; use frame_election_provider_support::{ElectionDataProvider, ElectionProvider, onchain}; use sp_npos_elections::{ assignment_ratio_to_staked_normalized, is_score_better, CompactSolution, ElectionScore, - EvaluateSupport, PerThing128, Supports, VoteWeight, + EvaluateSupport, IndexAssignment, PerThing128, Supports, VoteWeight, }; use sp_runtime::{ transaction_validity::{ @@ -267,17 +267,26 @@ pub type CompactTargetIndexOf = as CompactSolution>::Target; pub type CompactAccuracyOf = as CompactSolution>::Accuracy; /// The accuracy of the election, when computed on-chain. Equal to [`Config::OnChainAccuracy`]. pub type OnChainAccuracyOf = ::OnChainAccuracy; + +/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they voted. +pub type Voter = ( + ::AccountId, + sp_npos_elections::VoteWeight, + Vec<::AccountId>, +); + /// The relative distribution of a voter's stake among the winning targets. pub type Assignment = sp_npos_elections::Assignment< ::AccountId, CompactAccuracyOf, >; -/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they voted. -pub type Voter = ( - ::AccountId, - sp_npos_elections::VoteWeight, - Vec<::AccountId>, -); + +// The [`IndexAssignment`] type specialized for a particular runtime `T`. +pub type IndexAssignmentOf = IndexAssignment< + CompactVoterIndexOf, + CompactTargetIndexOf, + CompactAccuracyOf, +>; /// Wrapper type that implements the configurations needed for the on-chain backup. struct OnChainConfig(sp_std::marker::PhantomData); diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 470b84f02e39d..58eda1d1347ad 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -156,7 +156,7 @@ impl Pallet { // // This function completes in `O(edges * log voters.len())`; it's expensive, but linear. let encoded_size_of = |assignments: &[IndexAssignmentOf]| { - CompactOf::::from(assignments).map(|compact| compact.encoded_size()) + CompactOf::::from_index_assignments(assignments).map(|compact| compact.encoded_size()) }; let ElectionResult { mut assignments, winners } = election_result; @@ -199,7 +199,7 @@ impl Pallet { )?; // now make compact. - let compact = CompactOf::::from(assignments)?; + let compact = CompactOf::::from_index_assignments(&assignments)?; // re-calc score. let winners = sp_npos_elections::to_without_backing(winners); From c209b804b5b66f114c9709e70f0baa9273cd6535 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 16 Apr 2021 12:08:33 +0200 Subject: [PATCH 13/34] get rid of `from_index_assignments` in favor of `TryFrom` --- frame/election-provider-multi-phase/src/lib.rs | 1 + frame/election-provider-multi-phase/src/unsigned.rs | 6 +++--- primitives/npos-elections/compact/src/lib.rs | 4 ---- primitives/npos-elections/src/lib.rs | 7 ------- 4 files changed, 4 insertions(+), 14 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index f2d2111156451..588a062ab1bbd 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -577,6 +577,7 @@ pub mod pallet { + Eq + Clone + sp_std::fmt::Debug + + for<'a> sp_std::convert::TryFrom<&'a [IndexAssignmentOf], Error = sp_npos_elections::Error> + CompactSolution; /// Accuracy used for fallback on-chain election. diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 58eda1d1347ad..b40b5af46981e 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -25,7 +25,7 @@ use sp_npos_elections::{ assignment_staked_to_ratio_normalized, }; use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput}; -use sp_std::{cmp::Ordering, collections::btree_map::BTreeMap}; +use sp_std::{cmp::Ordering, collections::btree_map::BTreeMap, convert::TryFrom}; /// Storage key used to store the persistent offchain worker status. pub(crate) const OFFCHAIN_HEAD_DB: &[u8] = b"parity/multi-phase-unsigned-election"; @@ -156,7 +156,7 @@ impl Pallet { // // This function completes in `O(edges * log voters.len())`; it's expensive, but linear. let encoded_size_of = |assignments: &[IndexAssignmentOf]| { - CompactOf::::from_index_assignments(assignments).map(|compact| compact.encoded_size()) + CompactOf::::try_from(assignments).map(|compact| compact.encoded_size()) }; let ElectionResult { mut assignments, winners } = election_result; @@ -199,7 +199,7 @@ impl Pallet { )?; // now make compact. - let compact = CompactOf::::from_index_assignments(&assignments)?; + let compact = CompactOf::::try_from(&assignments)?; // re-calc score. let winners = sp_npos_elections::to_without_backing(winners); diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index 198b69967d229..b3dbde6b815a1 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -224,10 +224,6 @@ fn struct_def( return false } - fn from_index_assignments(index_assignments: &[__IndexAssignment]) -> Result { - Self::try_from(index_assignments) - } - fn from_assignment( assignments: _npos::sp_std::prelude::Vec<_npos::Assignment>, index_of_voter: FV, diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 24ebe62d09df6..b461c81baf7a6 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -166,13 +166,6 @@ pub trait CompactSolution: Sized { /// The weight/accuracy type of each vote. type Accuracy: PerThing128; - /// Build self from a list of `IndexAssignment`s. - fn from_index_assignments(index_assignments: &[IndexAssignment< - Self::Voter, - Self::Target, - Self::Accuracy, - >]) -> Result; - /// Build self from a list of assignments. fn from_assignment( assignments: &[Assignment], From 60ee91e7ef37efe0a2e8d5bb8f15ec50ec2c0755 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 16 Apr 2021 15:16:00 +0200 Subject: [PATCH 14/34] cause `pallet-election-provider-multi-phase` tests to compile successfully Mostly that's just updating the various test functions to keep track of refactorings elsewhere, though in a few places we needed to refactor some test-only helpers as well. --- .../src/benchmarking.rs | 2 +- .../src/helpers.rs | 12 +++ .../election-provider-multi-phase/src/mock.rs | 79 +++++++++++-------- .../src/unsigned.rs | 60 ++++++++------ .../compact/src/index_assignment.rs | 44 ++++++----- primitives/npos-elections/compact/src/lib.rs | 2 +- 6 files changed, 118 insertions(+), 81 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 90e90d427dc6e..f6a3062a2ff9f 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -135,7 +135,7 @@ fn solution_with_size( .collect::>(); let compact = - >::from_assignment(assignments, &voter_index, &target_index).unwrap(); + >::from_assignment(&assignments, &voter_index, &target_index).unwrap(); let score = compact.clone().score(&winners, stake_of, voter_at, target_at).unwrap(); let round = >::round(); diff --git a/frame/election-provider-multi-phase/src/helpers.rs b/frame/election-provider-multi-phase/src/helpers.rs index 7894f71800fdb..bf5b360499cb4 100644 --- a/frame/election-provider-multi-phase/src/helpers.rs +++ b/frame/election-provider-multi-phase/src/helpers.rs @@ -62,6 +62,18 @@ pub fn voter_index_fn( } } +/// Create a function that returns the index of a voter in the snapshot. +/// +/// Same as [`voter_index_fn`] but the returned function owns all its necessary data; nothing is +/// borrowed. +pub fn voter_index_fn_owned( + cache: BTreeMap, +) -> impl Fn(&T::AccountId) -> Option> { + move |who| { + cache.get(who).and_then(|i| >>::try_into(*i).ok()) + } +} + /// Same as [`voter_index_fn`], but the returning index is converted into usize, if possible. /// /// ## Warning diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index b6136b6c760ad..6ec2f182d6554 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -17,7 +17,7 @@ use super::*; use crate as multi_phase; -use multi_phase::unsigned::{Voter, Assignment}; +use multi_phase::Voter; pub use frame_support::{assert_noop, assert_ok}; use frame_support::{ parameter_types, @@ -42,7 +42,7 @@ use sp_runtime::{ traits::{BlakeTwo256, IdentityLookup}, PerU16, }; -use std::sync::Arc; +use std::{convert::TryFrom, sync::Arc}; pub type Block = sp_runtime::generic::Block; pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic; @@ -96,48 +96,61 @@ pub fn roll_to_with_ocw(n: u64) { } } -/// Compute voters and assignments for this runtime. +pub struct TrimHelpers { + pub voters: Vec>, + pub assignments: Vec>, + pub encoded_size_of: + Box]) -> Result>, + pub voter_index: Box< + dyn Fn( + &::AccountId, + ) -> Option>, + >, +} + +/// Helpers for setting up trimming tests. /// -/// Just a helper for trimming tests. -pub fn voters_and_assignments() -> (Vec>, Vec>) { +/// Assignments are pre-sorted in reverse order of stake. +pub fn trim_helpers() -> TrimHelpers { let RoundSnapshot { voters, targets } = MultiPhase::snapshot().unwrap(); + let stakes: std::collections::HashMap<_, _> = + voters.iter().map(|(id, stake, _)| (*id, *stake)).collect(); + + // Compute the size of a compact solution comprised of the selected arguments. + // + // This function completes in `O(edges)`; it's expensive, but linear. + let encoded_size_of = Box::new(|assignments: &[IndexAssignmentOf]| { + CompactOf::::try_from(assignments).map(|compact| compact.encoded_size()) + }); + let cache = helpers::generate_voter_cache::(&voters); + let voter_index = helpers::voter_index_fn_owned::(cache); + let target_index = helpers::target_index_fn::(&targets); + let desired_targets = MultiPhase::desired_targets().unwrap(); - let ElectionResult { assignments, .. } = seq_phragmen::<_, CompactAccuracyOf>( + let ElectionResult { mut assignments, .. } = seq_phragmen::<_, CompactAccuracyOf>( desired_targets as usize, - targets, + targets.clone(), voters.clone(), None, ) .unwrap(); - (voters, assignments) -} + // sort by decreasing order of stake + assignments.sort_unstable_by_key(|assignment| { + std::cmp::Reverse(stakes.get(&assignment.who).cloned().unwrap_or_default()) + }); -/// Convert voters and assignments into a verifiable raw solution. -/// -/// If you don't need to mess with `voters` and `assignments`, [`raw_solution`] is more efficient. -/// -/// It's important that voters and assignments retain their correspondence. -pub fn make_compact_from( - voters: Vec>, - assignments: Vec>, -) -> CompactOf { - // voters and assignments may have different length, but the voter ID must correspond at each - // position. - let voter_ids = voters.iter().map(|(who, _, _)| who).cloned(); - let assignment_ids = assignments.iter().map(|assignment| assignment.who); - assert!(voter_ids - .zip(assignment_ids) - .all(|(voter_id, assignment_id)| voter_id == assignment_id)); - - let RoundSnapshot { targets, .. } = MultiPhase::snapshot().unwrap(); - - // closures - let voter_index = helpers::voter_index_fn_linear::(&voters); - let target_index = helpers::target_index_fn_linear::(&targets); + // convert to IndexAssignment + let assignments = assignments + .iter() + .map(|assignment| { + IndexAssignmentOf::::new(assignment, &voter_index, &target_index) + }) + .collect::, _>>() + .expect("test assignments don't contain any voters with too many votes"); - >::from_assignment(assignments, &voter_index, &target_index).unwrap() + TrimHelpers { voters, assignments, encoded_size_of, voter_index: Box::new(voter_index) } } /// Spit out a verifiable raw solution. @@ -168,7 +181,7 @@ pub fn raw_solution() -> RawSolution> { to_supports(&winners, &staked).unwrap().evaluate() }; let compact = - >::from_assignment(assignments, &voter_index, &target_index).unwrap(); + >::from_assignment(&assignments, &voter_index, &target_index).unwrap(); let round = MultiPhase::round(); RawSolution { compact, score, round } diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index b40b5af46981e..c1332d53908f4 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -154,7 +154,7 @@ impl Pallet { // Compute the size of a compact solution comprised of the selected arguments. // - // This function completes in `O(edges * log voters.len())`; it's expensive, but linear. + // This function completes in `O(edges)`; it's expensive, but linear. let encoded_size_of = |assignments: &[IndexAssignmentOf]| { CompactOf::::try_from(assignments).map(|compact| compact.encoded_size()) }; @@ -557,15 +557,15 @@ mod max_weight { mod tests { use super::*; use crate::mock::{ - assert_noop, assert_ok, ExtBuilder, Extrinsic, make_compact_from, - MinerMaxWeight, MultiPhase, Origin, roll_to_with_ocw, roll_to, Runtime, - TestCompact, voters_and_assignments, witness, + assert_noop, assert_ok, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, Origin, + roll_to_with_ocw, roll_to, Runtime, TestCompact, TrimHelpers, trim_helpers, witness, }; use frame_support::{dispatch::Dispatchable, traits::OffchainWorker}; use mock::Call as OuterCall; - use frame_election_provider_support::Assignment; use sp_runtime::{traits::ValidateUnsigned, PerU16}; + type Assignment = crate::Assignment; + #[test] fn validate_unsigned_retracts_wrong_phase() { ExtBuilder::default().desired_targets(0).build_and_execute(|| { @@ -953,17 +953,20 @@ mod tests { roll_to(25); // given - let (mut voters, mut assignments) = voters_and_assignments(); - let compact = make_compact_from(voters.clone(), assignments.clone()); - let encoded_len = compact.encode().len() as u32; + let TrimHelpers { + mut assignments, + encoded_size_of, + .. + } = trim_helpers(); + let compact = CompactOf::::try_from(assignments.as_slice()).unwrap(); + let encoded_len = compact.encoded_size() as u32; let compact_clone = compact.clone(); // when - MultiPhase::sort_by_decreasing_stake(&mut voters, &mut assignments); - MultiPhase::trim_assignments_length(encoded_len, &mut assignments); + MultiPhase::trim_assignments_length(encoded_len, &mut assignments, encoded_size_of).unwrap(); // then - let compact = make_compact_from(voters, assignments); + let compact = CompactOf::::try_from(assignments.as_slice()).unwrap(); assert_eq!(compact, compact_clone); }); } @@ -975,19 +978,22 @@ mod tests { roll_to(25); // given - let (mut voters, mut assignments) = voters_and_assignments(); - let compact = make_compact_from(voters.clone(), assignments.clone()); - let encoded_len = compact.encode().len() as u32; + let TrimHelpers { + mut assignments, + encoded_size_of, + .. + } = trim_helpers(); + let compact = CompactOf::::try_from(assignments.as_slice()).unwrap(); + let encoded_len = compact.encoded_size(); let compact_clone = compact.clone(); // when - MultiPhase::sort_by_decreasing_stake(&mut voters, &mut assignments); - MultiPhase::trim_assignments_length(encoded_len - 1, &mut assignments); + MultiPhase::trim_assignments_length(encoded_len as u32 - 1, &mut assignments, encoded_size_of).unwrap(); // then - let compact = make_compact_from(voters, assignments); + let compact = CompactOf::::try_from(assignments.as_slice()).unwrap(); assert_ne!(compact, compact_clone); - assert!((compact.encoded_size() as u32) < encoded_len); + assert!(compact.encoded_size() < encoded_len); }); } @@ -998,25 +1004,29 @@ mod tests { roll_to(25); // given - let (mut voters, mut assignments) = voters_and_assignments(); - let compact = make_compact_from(voters.clone(), assignments.clone()); - let encoded_len = compact.encode().len() as u32; + let TrimHelpers { + voters, + mut assignments, + encoded_size_of, + voter_index, + } = trim_helpers(); + let compact = CompactOf::::try_from(assignments.as_slice()).unwrap(); + let encoded_len = compact.encoded_size() as u32; let count = assignments.len(); let min_stake_voter = voters.iter() .map(|(id, weight, _)| (weight, id)) .min() - .map(|(_, id)| *id) + .and_then(|(_, id)| voter_index(id)) .unwrap(); // when - MultiPhase::sort_by_decreasing_stake(&mut voters, &mut assignments); - MultiPhase::trim_assignments_length(encoded_len - 1, &mut assignments); + MultiPhase::trim_assignments_length(encoded_len - 1, &mut assignments, encoded_size_of).unwrap(); // then assert_eq!(assignments.len(), count - 1, "we must have removed exactly one assignment"); assert!( assignments.iter() - .all(|Assignment{ who, ..}| *who != min_stake_voter), + .all(|IndexAssignment{ who, ..}| *who != min_stake_voter), "min_stake_voter must no longer be in the set of voters", ); }); diff --git a/primitives/npos-elections/compact/src/index_assignment.rs b/primitives/npos-elections/compact/src/index_assignment.rs index 35e9ce991020b..6aeef1442236e 100644 --- a/primitives/npos-elections/compact/src/index_assignment.rs +++ b/primitives/npos-elections/compact/src/index_assignment.rs @@ -27,7 +27,7 @@ pub(crate) fn from_impl(count: usize) -> TokenStream2 { quote!(1 => compact.#name.push( ( *who, - *distribution[0].0, + distribution[0].0, ) ),) }; @@ -38,33 +38,35 @@ pub(crate) fn from_impl(count: usize) -> TokenStream2 { ( *who, ( - *distribution[0].0, - *distribution[0].1, + distribution[0].0, + distribution[0].1, ), - *distribution[1].0, + distribution[1].0, ) ),) }; - let from_impl_rest = (3..=count).map(|c| { - let inner = (0..c-1).map(|i| - quote!((*distribution[#i].0, *distribution[#i].1),) - ).collect::(); + let from_impl_rest = (3..=count) + .map(|c| { + let inner = (0..c - 1) + .map(|i| quote!((distribution[#i].0, distribution[#i].1),)) + .collect::(); - let field_name = field_name_for(c); - let last_index = c - 1; - let last = quote!(*distribution[#last_index].0); + let field_name = field_name_for(c); + let last_index = c - 1; + let last = quote!(distribution[#last_index].0); - quote!( - #c => compact.#field_name.push( - ( - &who, - [#inner], - #last, - ) - ), - ) - }).collect::(); + quote!( + #c => compact.#field_name.push( + ( + *who, + [#inner], + #last, + ) + ), + ) + }) + .collect::(); quote!( #from_impl_single diff --git a/primitives/npos-elections/compact/src/lib.rs b/primitives/npos-elections/compact/src/lib.rs index b3dbde6b815a1..e49518cc25cc7 100644 --- a/primitives/npos-elections/compact/src/lib.rs +++ b/primitives/npos-elections/compact/src/lib.rs @@ -225,7 +225,7 @@ fn struct_def( } fn from_assignment( - assignments: _npos::sp_std::prelude::Vec<_npos::Assignment>, + assignments: &[_npos::Assignment], index_of_voter: FV, index_of_target: FT, ) -> Result From 4e42ed4d365b6ff4247602134782d25d02a8c43c Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 16 Apr 2021 16:48:28 +0200 Subject: [PATCH 15/34] fix infinite binary search loop Turns out that moving `low` and `high` into an averager function is a bad idea, because the averager gets copies of those values, which of course are never updated. Can't use mutable references, because we want to read them elsewhere in the code. Just compute the average directly; life is better that way. --- frame/election-provider-multi-phase/src/unsigned.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index c1332d53908f4..65f2945f49637 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -284,16 +284,15 @@ impl Pallet { // length. Having discovered that, we can truncate efficiently. let mut high = assignments.len(); let mut low = 0; - let avg = move || (high + low) / 2; while high - low > 1 { - let test = avg(); + let test = (high + low) / 2; if encoded_size_of(&assignments[..test])? > max_allowed_length.saturated_into() { high = test; } else { low = test; } } - let maximum_allowed_voters = avg(); + let maximum_allowed_voters = (high + low) / 2; // ensure our postconditions are correct debug_assert!( From 859ebf30b60b4839faac50b7fa8bbf16905099eb Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 16 Apr 2021 17:07:54 +0200 Subject: [PATCH 16/34] fix a test failure --- .../src/unsigned.rs | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 65f2945f49637..9909df5b88278 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -282,27 +282,37 @@ impl Pallet { ) -> Result<(), MinerError> { // 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(); let mut high = assignments.len(); let mut low = 0; + let mut maximum_allowed_voters = 0; + let mut size; + while high - low > 1 { - let test = (high + low) / 2; - if encoded_size_of(&assignments[..test])? > max_allowed_length.saturated_into() { - high = test; + maximum_allowed_voters = (high + low) / 2; + size = encoded_size_of(&assignments[..maximum_allowed_voters])?; + if size > max_allowed_length { + high = maximum_allowed_voters; } else { - low = test; + low = maximum_allowed_voters; } } - let maximum_allowed_voters = (high + low) / 2; + if high - low == 1 + && encoded_size_of(&assignments[..maximum_allowed_voters + 1])? <= max_allowed_length + { + maximum_allowed_voters += 1; + } // ensure our postconditions are correct debug_assert!( - encoded_size_of(&assignments[..maximum_allowed_voters]).unwrap() - <= max_allowed_length.saturated_into() + encoded_size_of(&assignments[..maximum_allowed_voters]).unwrap() <= max_allowed_length ); - debug_assert!( + debug_assert!(if maximum_allowed_voters < assignments.len() { encoded_size_of(&assignments[..maximum_allowed_voters + 1]).unwrap() - > max_allowed_length.saturated_into() - ); + > max_allowed_length + } else { + true + }); // NOTE: before this point, every access was immutable. // after this point, we never error. From 1a3987bba7574952d0ad4b1f775309458c9f5ae8 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 16 Apr 2021 17:09:15 +0200 Subject: [PATCH 17/34] fix the rest of test failures --- 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 9909df5b88278..d316acca82e08 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -252,7 +252,7 @@ impl Pallet { size, max_weight, ); - let removing: usize = assignments.len() - maximum_allowed_voters as usize; + let removing: usize = assignments.len().saturating_sub(maximum_allowed_voters.saturated_into()); log!( debug, "from {} assignments, truncating to {} for weight, removing {}", From 9601dd47f0e95018cdac7007d32310e6537c2088 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 16 Apr 2021 17:33:19 +0200 Subject: [PATCH 18/34] remove unguarded subtraction --- 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 d316acca82e08..1da238a0616f0 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -323,7 +323,7 @@ impl Pallet { "from {} assignments, truncating to {} for length, removing {}", assignments.len(), maximum_allowed_voters, - assignments.len() - maximum_allowed_voters, + assignments.len().saturating_sub(maximum_allowed_voters), ); assignments.truncate(maximum_allowed_voters); From b04e16596d6d0d8b857b395939d0ad7df04e9ff5 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 09:26:16 +0200 Subject: [PATCH 19/34] fix npos-elections tests compilation --- primitives/npos-elections/src/tests.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/npos-elections/src/tests.rs b/primitives/npos-elections/src/tests.rs index 6304e50ec5868..b2071e924909b 100644 --- a/primitives/npos-elections/src/tests.rs +++ b/primitives/npos-elections/src/tests.rs @@ -1355,7 +1355,7 @@ mod solution_type { }; let compacted = TestSolutionCompact::from_assignment( - assignments.clone(), + &assignments, voter_index, target_index, ).unwrap(); @@ -1518,7 +1518,7 @@ mod solution_type { ]; let compacted = TestSolutionCompact::from_assignment( - assignments.clone(), + &assignments, voter_index, target_index, ); @@ -1549,7 +1549,7 @@ mod solution_type { }; let compacted = TestSolutionCompact::from_assignment( - assignments.clone(), + &assignments, voter_index, target_index, ).unwrap(); From 31222ca96f28214ef58dade60ad66d78894d8220 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 09:52:58 +0200 Subject: [PATCH 20/34] ensure we use sp_std::vec::Vec in assignments --- primitives/npos-elections/src/assignments.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/primitives/npos-elections/src/assignments.rs b/primitives/npos-elections/src/assignments.rs index 19407fce9d43f..3166e0773e18e 100644 --- a/primitives/npos-elections/src/assignments.rs +++ b/primitives/npos-elections/src/assignments.rs @@ -21,6 +21,7 @@ use crate::{Error, ExtendedBalance, IdentifierT, PerThing128, __OrInvalidIndex}; use codec::{Encode, Decode}; use sp_arithmetic::{traits::{Bounded, Zero}, Normalizable, PerThing}; use sp_core::RuntimeDebug; +use sp_std::vec::Vec; /// A voter's stake assignment among a set of targets, represented as ratios. #[derive(RuntimeDebug, Clone, Default)] From 3dda173e467bd1d2cf6b1e211468fccce5b83544 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 13:35:00 +0200 Subject: [PATCH 21/34] add IndexAssignmentOf to sp_npos_elections --- frame/election-provider-multi-phase/src/lib.rs | 10 +++------- primitives/npos-elections/src/assignments.rs | 7 +++++++ primitives/npos-elections/src/lib.rs | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 588a062ab1bbd..3471e9d4f8ce1 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -225,7 +225,7 @@ use frame_system::{ensure_none, offchain::SendTransactionTypes}; use frame_election_provider_support::{ElectionDataProvider, ElectionProvider, onchain}; use sp_npos_elections::{ assignment_ratio_to_staked_normalized, is_score_better, CompactSolution, ElectionScore, - EvaluateSupport, IndexAssignment, PerThing128, Supports, VoteWeight, + EvaluateSupport, PerThing128, Supports, VoteWeight, }; use sp_runtime::{ transaction_validity::{ @@ -281,12 +281,8 @@ pub type Assignment = sp_npos_elections::Assignment< CompactAccuracyOf, >; -// The [`IndexAssignment`] type specialized for a particular runtime `T`. -pub type IndexAssignmentOf = IndexAssignment< - CompactVoterIndexOf, - CompactTargetIndexOf, - CompactAccuracyOf, ->; +/// The [`IndexAssignment`] type specialized for a particular runtime `T`. +pub type IndexAssignmentOf = sp_npos_elections::IndexAssignmentOf>; /// Wrapper type that implements the configurations needed for the on-chain backup. struct OnChainConfig(sp_std::marker::PhantomData); diff --git a/primitives/npos-elections/src/assignments.rs b/primitives/npos-elections/src/assignments.rs index 3166e0773e18e..aacd01a030692 100644 --- a/primitives/npos-elections/src/assignments.rs +++ b/primitives/npos-elections/src/assignments.rs @@ -199,3 +199,10 @@ impl IndexAssignment = IndexAssignment< + ::Voter, + ::Target, + ::Accuracy, +>; diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index b461c81baf7a6..2adf4760fb874 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -108,7 +108,7 @@ pub mod reduce; pub mod helpers; pub mod pjr; -pub use assignments::{Assignment, IndexAssignment, StakedAssignment}; +pub use assignments::{Assignment, IndexAssignment, StakedAssignment, IndexAssignmentOf}; pub use reduce::reduce; pub use helpers::*; pub use phragmen::*; From 0c76b221d3dd0e1747e5c2db09d5fed781094e42 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 13:44:30 +0200 Subject: [PATCH 22/34] move miner types to `unsigned` --- .../election-provider-multi-phase/src/lib.rs | 18 +------------ .../election-provider-multi-phase/src/mock.rs | 2 +- .../src/unsigned.rs | 27 ++++++++++++++++--- 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 3471e9d4f8ce1..a34d5637179cf 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -268,22 +268,6 @@ pub type CompactAccuracyOf = as CompactSolution>::Accuracy; /// The accuracy of the election, when computed on-chain. Equal to [`Config::OnChainAccuracy`]. pub type OnChainAccuracyOf = ::OnChainAccuracy; -/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they voted. -pub type Voter = ( - ::AccountId, - sp_npos_elections::VoteWeight, - Vec<::AccountId>, -); - -/// The relative distribution of a voter's stake among the winning targets. -pub type Assignment = sp_npos_elections::Assignment< - ::AccountId, - CompactAccuracyOf, ->; - -/// The [`IndexAssignment`] type specialized for a particular runtime `T`. -pub type IndexAssignmentOf = sp_npos_elections::IndexAssignmentOf>; - /// Wrapper type that implements the configurations needed for the on-chain backup. struct OnChainConfig(sp_std::marker::PhantomData); impl onchain::Config for OnChainConfig { @@ -573,7 +557,7 @@ pub mod pallet { + Eq + Clone + sp_std::fmt::Debug - + for<'a> sp_std::convert::TryFrom<&'a [IndexAssignmentOf], Error = sp_npos_elections::Error> + + for<'a> sp_std::convert::TryFrom<&'a [crate::unsigned::IndexAssignmentOf], Error = sp_npos_elections::Error> + CompactSolution; /// Accuracy used for fallback on-chain election. diff --git a/frame/election-provider-multi-phase/src/mock.rs b/frame/election-provider-multi-phase/src/mock.rs index 6ec2f182d6554..f3cf00f2ca0c8 100644 --- a/frame/election-provider-multi-phase/src/mock.rs +++ b/frame/election-provider-multi-phase/src/mock.rs @@ -17,7 +17,7 @@ use super::*; use crate as multi_phase; -use multi_phase::Voter; +use multi_phase::unsigned::{IndexAssignmentOf, Voter}; pub use frame_support::{assert_noop, assert_ok}; use frame_support::{ parameter_types, diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 1da238a0616f0..b2aafea8ed914 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -34,6 +34,22 @@ pub(crate) const OFFCHAIN_HEAD_DB: &[u8] = b"parity/multi-phase-unsigned-electio /// within a window of 5 blocks. pub(crate) const OFFCHAIN_REPEAT: u32 = 5; +/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they voted. +pub type Voter = ( + ::AccountId, + sp_npos_elections::VoteWeight, + Vec<::AccountId>, +); + +/// The relative distribution of a voter's stake among the winning targets. +pub type Assignment = sp_npos_elections::Assignment< + ::AccountId, + CompactAccuracyOf, +>; + +/// The [`IndexAssignment`] type specialized for a particular runtime `T`. +pub type IndexAssignmentOf = sp_npos_elections::IndexAssignmentOf>; + #[derive(Debug, Eq, PartialEq)] pub enum MinerError { /// An internal error in the NPoS elections crate. @@ -565,15 +581,18 @@ mod max_weight { #[cfg(test)] mod tests { use super::*; - use crate::mock::{ - assert_noop, assert_ok, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, Origin, - roll_to_with_ocw, roll_to, Runtime, TestCompact, TrimHelpers, trim_helpers, witness, + use crate::{ + mock::{ + assert_noop, assert_ok, ExtBuilder, Extrinsic, MinerMaxWeight, MultiPhase, Origin, + roll_to_with_ocw, roll_to, Runtime, TestCompact, TrimHelpers, trim_helpers, witness, + }, }; use frame_support::{dispatch::Dispatchable, traits::OffchainWorker}; use mock::Call as OuterCall; + use sp_npos_elections::IndexAssignment; use sp_runtime::{traits::ValidateUnsigned, PerU16}; - type Assignment = crate::Assignment; + type Assignment = crate::unsigned::Assignment; #[test] fn validate_unsigned_retracts_wrong_phase() { From 996034253a8ca33e6e87292b17812e4dc222ace7 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 13:46:04 +0200 Subject: [PATCH 23/34] use stable sort --- 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 b2aafea8ed914..991b0c7e5227a 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -179,7 +179,7 @@ impl Pallet { // Sort the assignments by reversed voter stake. This ensures that we can efficiently truncate the list. let stakes: BTreeMap<_, _> = voters.iter().map(|(who, stake, _)| (who.clone(), *stake)).collect(); - assignments.sort_unstable_by_key(|Assignment:: { who, .. }| { + assignments.sort_by_key(|Assignment:: { who, .. }| { sp_std::cmp::Reverse(stakes.get(who).cloned().unwrap_or_default()) }); From 859555a51c3ce50279e094c5923e548069745c02 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 13:47:22 +0200 Subject: [PATCH 24/34] rewrap some long comments --- frame/election-provider-multi-phase/src/unsigned.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 991b0c7e5227a..f1029b4f3b7cd 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -34,7 +34,8 @@ pub(crate) const OFFCHAIN_HEAD_DB: &[u8] = b"parity/multi-phase-unsigned-electio /// within a window of 5 blocks. pub(crate) const OFFCHAIN_REPEAT: u32 = 5; -/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they voted. +/// A voter's fundamental data: their ID, their stake, and the list of candidates for whom they +/// voted. pub type Voter = ( ::AccountId, sp_npos_elections::VoteWeight, @@ -176,7 +177,8 @@ impl Pallet { }; let ElectionResult { mut assignments, winners } = election_result; - // Sort the assignments by reversed voter stake. This ensures that we can efficiently truncate the list. + // Sort the assignments by reversed voter stake. This ensures that we can efficiently + // truncate the list. let stakes: BTreeMap<_, _> = voters.iter().map(|(who, stake, _)| (who.clone(), *stake)).collect(); assignments.sort_by_key(|Assignment:: { who, .. }| { @@ -193,7 +195,8 @@ impl Pallet { assignment_staked_to_ratio_normalized(staked)? }; - // convert to `IndexAssignment`. This improves the runtime complexity of converting to `Compact`. + // convert to `IndexAssignment`. This improves the runtime complexity of converting to + // `Compact`. let mut assignments = assignments .iter() .map(|assignment| IndexAssignmentOf::::new(assignment, &voter_index, &target_index)) From dd80c95be22d97c7159355c6071817d7845f4bf1 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 13:57:11 +0200 Subject: [PATCH 25/34] use existing cache instead of building a dedicated stake map --- frame/election-provider-multi-phase/src/unsigned.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index f1029b4f3b7cd..de9c1af0de8e6 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -25,7 +25,7 @@ use sp_npos_elections::{ assignment_staked_to_ratio_normalized, }; use sp_runtime::{offchain::storage::StorageValueRef, traits::TrailingZeroInput}; -use sp_std::{cmp::Ordering, collections::btree_map::BTreeMap, convert::TryFrom}; +use sp_std::{cmp::Ordering, convert::TryFrom}; /// Storage key used to store the persistent offchain worker status. pub(crate) const OFFCHAIN_HEAD_DB: &[u8] = b"parity/multi-phase-unsigned-election"; @@ -179,10 +179,12 @@ impl Pallet { let ElectionResult { mut assignments, winners } = election_result; // Sort the assignments by reversed voter stake. This ensures that we can efficiently // truncate the list. - let stakes: BTreeMap<_, _> = - voters.iter().map(|(who, stake, _)| (who.clone(), *stake)).collect(); assignments.sort_by_key(|Assignment:: { who, .. }| { - sp_std::cmp::Reverse(stakes.get(who).cloned().unwrap_or_default()) + let stake = cache.get(who).map(|idx| { + let (_, stake, _) = voters[*idx]; + stake + }).unwrap_or_default(); + sp_std::cmp::Reverse(stake) }); // Reduce (requires round-trip to staked form) From 32ed1ec18be2b78f9259798d9ff66be60f5615c6 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 14:06:32 +0200 Subject: [PATCH 26/34] generalize the TryFrom bound on CompactSolution --- frame/election-provider-multi-phase/src/lib.rs | 1 - primitives/npos-elections/src/lib.rs | 5 ++++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index a34d5637179cf..c59d68a33adba 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -557,7 +557,6 @@ pub mod pallet { + Eq + Clone + sp_std::fmt::Debug - + for<'a> sp_std::convert::TryFrom<&'a [crate::unsigned::IndexAssignmentOf], Error = sp_npos_elections::Error> + CompactSolution; /// Accuracy used for fallback on-chain election. diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 2adf4760fb874..c1cf41a40f2b5 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -141,7 +141,10 @@ impl __OrInvalidIndex for Option { /// A common interface for all compact solutions. /// /// See [`sp-npos-elections-compact`] for more info. -pub trait CompactSolution: Sized { +pub trait CompactSolution +where + Self: Sized + for<'a> sp_std::convert::TryFrom<&'a [IndexAssignmentOf], Error = Error>, +{ /// The maximum number of votes that are allowed. const LIMIT: usize; From 0edfca58ee90a2bd631d0c94285892cedcb8e455 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 14:23:43 +0200 Subject: [PATCH 27/34] undo adding sp-core dependency --- frame/election-provider-multi-phase/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/election-provider-multi-phase/Cargo.toml b/frame/election-provider-multi-phase/Cargo.toml index d1a985e723bb2..6b2219871ab82 100644 --- a/frame/election-provider-multi-phase/Cargo.toml +++ b/frame/election-provider-multi-phase/Cargo.toml @@ -20,7 +20,6 @@ log = { version = "0.4.14", default-features = false } frame-support = { version = "3.0.0", default-features = false, path = "../support" } frame-system = { version = "3.0.0", default-features = false, path = "../system" } -sp-core = { version = "3.0.0", default-features = false, path = "../../primitives/core" } sp-io = { version = "3.0.0", default-features = false, path = "../../primitives/io" } sp-std = { version = "3.0.0", default-features = false, path = "../../primitives/std" } sp-runtime = { version = "3.0.0", default-features = false, path = "../../primitives/runtime" } @@ -38,6 +37,7 @@ parking_lot = "0.11.0" rand = { version = "0.7.3" } hex-literal = "0.3.1" substrate-test-utils = { version = "3.0.0", path = "../../test-utils" } +sp-core = { version = "3.0.0", default-features = false, path = "../../primitives/core" } sp-io = { version = "3.0.0", path = "../../primitives/io" } sp-tracing = { version = "3.0.0", path = "../../primitives/tracing" } frame-election-provider-support = { version = "3.0.0", features = ["runtime-benchmarks"], path = "../election-provider-support" } From e4d2823b47902f05f850f4b398d141873ea9ad53 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 14:29:26 +0200 Subject: [PATCH 28/34] consume assignments to produce index_assignments --- .../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 de9c1af0de8e6..2b3534fe87430 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -197,11 +197,11 @@ impl Pallet { assignment_staked_to_ratio_normalized(staked)? }; - // convert to `IndexAssignment`. This improves the runtime complexity of converting to - // `Compact`. - let mut assignments = assignments - .iter() - .map(|assignment| IndexAssignmentOf::::new(assignment, &voter_index, &target_index)) + // convert to `IndexAssignment`. This improves the runtime complexity of repeatedly + // converting to `Compact`. + let mut index_assignments = assignments + .into_iter() + .map(|assignment| IndexAssignmentOf::::new(&assignment, &voter_index, &target_index)) .collect::, _>>()?; // trim assignments list for weight and length. @@ -211,16 +211,16 @@ impl Pallet { desired_targets, size, T::MinerMaxWeight::get(), - &mut assignments, + &mut index_assignments, ); Self::trim_assignments_length( T::MinerMaxLength::get(), - &mut assignments, + &mut index_assignments, &encoded_size_of, )?; // now make compact. - let compact = CompactOf::::try_from(&assignments)?; + let compact = CompactOf::::try_from(&index_assignments)?; // re-calc score. let winners = sp_npos_elections::to_without_backing(winners); From 73d3fdd6c8e4e1cc837cd073df86efcb19e01d21 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 19 Apr 2021 15:51:02 +0200 Subject: [PATCH 29/34] Add a test of Assignment -> IndexAssignment -> Compact --- Cargo.lock | 2 + primitives/npos-elections/compact/Cargo.toml | 2 + .../npos-elections/compact/tests/compact.rs | 168 ++++++++++++++++++ 3 files changed, 172 insertions(+) create mode 100644 primitives/npos-elections/compact/tests/compact.rs diff --git a/Cargo.lock b/Cargo.lock index 67e08475dc713..da58d8e084570 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8875,8 +8875,10 @@ dependencies = [ "proc-macro-crate 1.0.0", "proc-macro2", "quote", + "rand 0.7.3", "sp-arithmetic", "sp-npos-elections", + "sp-runtime", "syn", "trybuild", ] diff --git a/primitives/npos-elections/compact/Cargo.toml b/primitives/npos-elections/compact/Cargo.toml index 63432a36efc80..e12f7350e9cbf 100644 --- a/primitives/npos-elections/compact/Cargo.toml +++ b/primitives/npos-elections/compact/Cargo.toml @@ -22,6 +22,8 @@ proc-macro-crate = "1.0.0" [dev-dependencies] parity-scale-codec = "2.0.1" +rand = { version = "0.7.3", features = ["small_rng"] } sp-arithmetic = { path = "../../arithmetic" } sp-npos-elections = { path = ".." } +sp-runtime = { path = "../../runtime" } trybuild = "1.0.41" diff --git a/primitives/npos-elections/compact/tests/compact.rs b/primitives/npos-elections/compact/tests/compact.rs new file mode 100644 index 0000000000000..b3c1bcea9994a --- /dev/null +++ b/primitives/npos-elections/compact/tests/compact.rs @@ -0,0 +1,168 @@ +use std::{ + collections::{HashSet, HashMap}, + convert::TryInto, + hash::Hash, +}; +use sp_npos_elections::{CompactSolution, IndexAssignment}; +use rand::{self, Rng, SeedableRng, seq::SliceRandom}; + +const CANDIDATES: usize = 1000; +const VOTERS: usize = 2500; + +sp_npos_elections_compact::generate_solution_type!( + #[compact] + pub struct Compact::(16) +); + +pub type AccountId = u64; +/// The candidate mask allows easy disambiguation between voters and candidates: accounts +/// for which this bit is set are candidates, and without it, are voters. +pub const CANDIDATE_MASK: AccountId = 1 << ((std::mem::size_of::() * 8) - 1); +pub type CandidateId = AccountId; + +pub type Accuracy = sp_runtime::Perbill; + +pub type Assignment = sp_npos_elections::Assignment; +pub type Voter = (AccountId, sp_npos_elections::VoteWeight, Vec); + +/// Generate voter and assignment lists. Makes no attempt to be realistic about winner or assignment fairness. +/// +/// Maintains these invariants: +/// +/// - candidate ids have `CANDIDATE_MASK` bit set +/// - voter ids do not have `CANDIDATE_MASK` bit set +/// - assignments have the same ordering as voters +/// - `assignments.distribution.iter().map(|(_, frac)| frac).sum() == One::one()` +/// - a coherent set of winners is chosen. +/// - the winner set is a subset of the candidate set. +/// - `assignments.distribution.iter().all(|(who, _)| winners.contains(who))` +fn generate_random_votes( + candidate_count: usize, + voter_count: usize, + mut rng: impl Rng, +) -> (Vec, Vec, Vec) { + // cache for fast generation of unique candidate and voter ids + let mut used_ids = HashSet::with_capacity(candidate_count + voter_count); + + // candidates are easy: just a completely random set of IDs + let mut candidates: Vec = Vec::with_capacity(candidate_count); + while candidates.len() < candidate_count { + let mut new = || rng.gen::() | CANDIDATE_MASK; + let mut id = new(); + // insert returns `false` when the value was already present + while !used_ids.insert(id) { + id = new(); + } + candidates.push(id); + } + + // voters are random ids, random weights, random selection from the candidates + let mut voters = Vec::with_capacity(voter_count); + while voters.len() < voter_count { + let mut new = || rng.gen::() & !CANDIDATE_MASK; + let mut id = new(); + // insert returns `false` when the value was already present + while !used_ids.insert(id) { + id = new(); + } + + let vote_weight = rng.gen(); + + // it's not interesting if a voter chooses 0 or all candidates, so rule those cases out. + // also, let's not generate any cases which result in a compact overflow. + let n_candidates_chosen = rng.gen_range(1, candidates.len().min(16)); + + let mut chosen_candidates = Vec::with_capacity(n_candidates_chosen); + chosen_candidates.extend(candidates.choose_multiple(&mut rng, n_candidates_chosen)); + voters.push((id, vote_weight, chosen_candidates)); + } + + // always generate a sensible number of winners: elections are uninteresting if nobody wins, + // or everybody wins + let num_winners = rng.gen_range(1, candidate_count); + let mut winners: HashSet = HashSet::with_capacity(num_winners); + winners.extend(candidates.choose_multiple(&mut rng, num_winners)); + assert_eq!(winners.len(), num_winners); + + let mut assignments = Vec::with_capacity(voters.len()); + for (voter_id, _, votes) in voters.iter() { + let chosen_winners = votes.iter().filter(|vote| winners.contains(vote)).cloned(); + let num_chosen_winners = chosen_winners.clone().count(); + + // distribute the available stake randomly + let stake_distribution = if num_chosen_winners == 0 { + Vec::new() + } else { + let mut available_stake = 1000; + let mut stake_distribution = Vec::with_capacity(num_chosen_winners); + for _ in 0..num_chosen_winners - 1 { + let stake = rng.gen_range(0, available_stake); + stake_distribution.push(Accuracy::from_perthousand(stake)); + available_stake -= stake; + } + stake_distribution.push(Accuracy::from_perthousand(available_stake)); + stake_distribution.shuffle(&mut rng); + stake_distribution + }; + + assignments.push(Assignment { + who: *voter_id, + distribution: chosen_winners.zip(stake_distribution).collect(), + }); + } + + (voters, assignments, candidates) +} + +fn generate_cache(voters: Voters) -> HashMap +where + Voters: Iterator, + Item: Hash + Eq + Copy, +{ + let mut cache = HashMap::new(); + for (idx, voter_id) in voters.enumerate() { + cache.insert(voter_id, idx); + } + cache +} + +/// Create a function that returns the index of a voter in the voters list. +pub fn make_voter_fn(voters: &[Voter]) -> impl Fn(&AccountId) -> Option +where + usize: TryInto, +{ + let cache = generate_cache(voters.iter().map(|(id, _, _)| *id)); + move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) +} + +/// Create a function that returns the index of a candidate in the candidates list. +pub fn make_target_fn( + candidates: &[CandidateId], +) -> impl Fn(&CandidateId) -> Option +where + usize: TryInto, +{ + let cache = generate_cache(candidates.iter().cloned()); + move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) +} + +#[test] +fn index_assignments_generate_same_compact_as_plain_assignments() { + let rng = rand::rngs::SmallRng::seed_from_u64(0); + + let (voters, assignments, candidates) = generate_random_votes(CANDIDATES, VOTERS, rng); + let voter_index = make_voter_fn(&voters); + let target_index = make_target_fn(&candidates); + + let compact = Compact::from_assignment(&assignments, &voter_index, &target_index).unwrap(); + + let index_assignments = assignments + .into_iter() + .map(|assignment| IndexAssignment::new(&assignment, &voter_index, &target_index)) + .collect::, _>>() + .unwrap(); + + let index_compact = index_assignments.as_slice().try_into().unwrap(); + + assert_eq!(compact, index_compact); +} From 1a174aab416aaa7f91042c544bfe058de4f3e45a Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 20 Apr 2021 16:07:04 +0200 Subject: [PATCH 30/34] fix `IndexAssignmentOf` doc --- 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 2b3534fe87430..42f9becbfeb0e 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -48,7 +48,7 @@ pub type Assignment = sp_npos_elections::Assignment< CompactAccuracyOf, >; -/// The [`IndexAssignment`] type specialized for a particular runtime `T`. +/// The [`IndexAssignment`][sp_npos_elections::IndexAssignment] type specialized for a particular runtime `T`. pub type IndexAssignmentOf = sp_npos_elections::IndexAssignmentOf>; #[derive(Debug, Eq, PartialEq)] From eed1c218823794d1977e6e1ac1f4f70b3aa5a6e9 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 20 Apr 2021 17:16:52 +0200 Subject: [PATCH 31/34] move compact test from sp-npos-elections-compact to sp-npos-elections This means that we can put the mocking parts of that into a proper mock package, put the test into a test package among other tests. Having the mocking parts in a mock package enables us to create a benchmark (which is treated as a separate crate) import them. --- Cargo.lock | 2 - primitives/npos-elections/Cargo.toml | 1 + primitives/npos-elections/compact/Cargo.toml | 2 - .../npos-elections/compact/tests/compact.rs | 168 ---------------- primitives/npos-elections/src/mock.rs | 189 +++++++++++++++--- primitives/npos-elections/src/tests.rs | 57 ++++-- 6 files changed, 207 insertions(+), 212 deletions(-) delete mode 100644 primitives/npos-elections/compact/tests/compact.rs diff --git a/Cargo.lock b/Cargo.lock index da58d8e084570..67e08475dc713 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8875,10 +8875,8 @@ dependencies = [ "proc-macro-crate 1.0.0", "proc-macro2", "quote", - "rand 0.7.3", "sp-arithmetic", "sp-npos-elections", - "sp-runtime", "syn", "trybuild", ] diff --git a/primitives/npos-elections/Cargo.toml b/primitives/npos-elections/Cargo.toml index 79d46743cd758..5bca1e0bb859f 100644 --- a/primitives/npos-elections/Cargo.toml +++ b/primitives/npos-elections/Cargo.toml @@ -28,6 +28,7 @@ sp-runtime = { version = "3.0.0", path = "../runtime" } [features] default = ["std"] bench = [] +mocks = [] std = [ "codec/std", "serde", diff --git a/primitives/npos-elections/compact/Cargo.toml b/primitives/npos-elections/compact/Cargo.toml index e12f7350e9cbf..63432a36efc80 100644 --- a/primitives/npos-elections/compact/Cargo.toml +++ b/primitives/npos-elections/compact/Cargo.toml @@ -22,8 +22,6 @@ proc-macro-crate = "1.0.0" [dev-dependencies] parity-scale-codec = "2.0.1" -rand = { version = "0.7.3", features = ["small_rng"] } sp-arithmetic = { path = "../../arithmetic" } sp-npos-elections = { path = ".." } -sp-runtime = { path = "../../runtime" } trybuild = "1.0.41" diff --git a/primitives/npos-elections/compact/tests/compact.rs b/primitives/npos-elections/compact/tests/compact.rs deleted file mode 100644 index b3c1bcea9994a..0000000000000 --- a/primitives/npos-elections/compact/tests/compact.rs +++ /dev/null @@ -1,168 +0,0 @@ -use std::{ - collections::{HashSet, HashMap}, - convert::TryInto, - hash::Hash, -}; -use sp_npos_elections::{CompactSolution, IndexAssignment}; -use rand::{self, Rng, SeedableRng, seq::SliceRandom}; - -const CANDIDATES: usize = 1000; -const VOTERS: usize = 2500; - -sp_npos_elections_compact::generate_solution_type!( - #[compact] - pub struct Compact::(16) -); - -pub type AccountId = u64; -/// The candidate mask allows easy disambiguation between voters and candidates: accounts -/// for which this bit is set are candidates, and without it, are voters. -pub const CANDIDATE_MASK: AccountId = 1 << ((std::mem::size_of::() * 8) - 1); -pub type CandidateId = AccountId; - -pub type Accuracy = sp_runtime::Perbill; - -pub type Assignment = sp_npos_elections::Assignment; -pub type Voter = (AccountId, sp_npos_elections::VoteWeight, Vec); - -/// Generate voter and assignment lists. Makes no attempt to be realistic about winner or assignment fairness. -/// -/// Maintains these invariants: -/// -/// - candidate ids have `CANDIDATE_MASK` bit set -/// - voter ids do not have `CANDIDATE_MASK` bit set -/// - assignments have the same ordering as voters -/// - `assignments.distribution.iter().map(|(_, frac)| frac).sum() == One::one()` -/// - a coherent set of winners is chosen. -/// - the winner set is a subset of the candidate set. -/// - `assignments.distribution.iter().all(|(who, _)| winners.contains(who))` -fn generate_random_votes( - candidate_count: usize, - voter_count: usize, - mut rng: impl Rng, -) -> (Vec, Vec, Vec) { - // cache for fast generation of unique candidate and voter ids - let mut used_ids = HashSet::with_capacity(candidate_count + voter_count); - - // candidates are easy: just a completely random set of IDs - let mut candidates: Vec = Vec::with_capacity(candidate_count); - while candidates.len() < candidate_count { - let mut new = || rng.gen::() | CANDIDATE_MASK; - let mut id = new(); - // insert returns `false` when the value was already present - while !used_ids.insert(id) { - id = new(); - } - candidates.push(id); - } - - // voters are random ids, random weights, random selection from the candidates - let mut voters = Vec::with_capacity(voter_count); - while voters.len() < voter_count { - let mut new = || rng.gen::() & !CANDIDATE_MASK; - let mut id = new(); - // insert returns `false` when the value was already present - while !used_ids.insert(id) { - id = new(); - } - - let vote_weight = rng.gen(); - - // it's not interesting if a voter chooses 0 or all candidates, so rule those cases out. - // also, let's not generate any cases which result in a compact overflow. - let n_candidates_chosen = rng.gen_range(1, candidates.len().min(16)); - - let mut chosen_candidates = Vec::with_capacity(n_candidates_chosen); - chosen_candidates.extend(candidates.choose_multiple(&mut rng, n_candidates_chosen)); - voters.push((id, vote_weight, chosen_candidates)); - } - - // always generate a sensible number of winners: elections are uninteresting if nobody wins, - // or everybody wins - let num_winners = rng.gen_range(1, candidate_count); - let mut winners: HashSet = HashSet::with_capacity(num_winners); - winners.extend(candidates.choose_multiple(&mut rng, num_winners)); - assert_eq!(winners.len(), num_winners); - - let mut assignments = Vec::with_capacity(voters.len()); - for (voter_id, _, votes) in voters.iter() { - let chosen_winners = votes.iter().filter(|vote| winners.contains(vote)).cloned(); - let num_chosen_winners = chosen_winners.clone().count(); - - // distribute the available stake randomly - let stake_distribution = if num_chosen_winners == 0 { - Vec::new() - } else { - let mut available_stake = 1000; - let mut stake_distribution = Vec::with_capacity(num_chosen_winners); - for _ in 0..num_chosen_winners - 1 { - let stake = rng.gen_range(0, available_stake); - stake_distribution.push(Accuracy::from_perthousand(stake)); - available_stake -= stake; - } - stake_distribution.push(Accuracy::from_perthousand(available_stake)); - stake_distribution.shuffle(&mut rng); - stake_distribution - }; - - assignments.push(Assignment { - who: *voter_id, - distribution: chosen_winners.zip(stake_distribution).collect(), - }); - } - - (voters, assignments, candidates) -} - -fn generate_cache(voters: Voters) -> HashMap -where - Voters: Iterator, - Item: Hash + Eq + Copy, -{ - let mut cache = HashMap::new(); - for (idx, voter_id) in voters.enumerate() { - cache.insert(voter_id, idx); - } - cache -} - -/// Create a function that returns the index of a voter in the voters list. -pub fn make_voter_fn(voters: &[Voter]) -> impl Fn(&AccountId) -> Option -where - usize: TryInto, -{ - let cache = generate_cache(voters.iter().map(|(id, _, _)| *id)); - move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) -} - -/// Create a function that returns the index of a candidate in the candidates list. -pub fn make_target_fn( - candidates: &[CandidateId], -) -> impl Fn(&CandidateId) -> Option -where - usize: TryInto, -{ - let cache = generate_cache(candidates.iter().cloned()); - move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) -} - -#[test] -fn index_assignments_generate_same_compact_as_plain_assignments() { - let rng = rand::rngs::SmallRng::seed_from_u64(0); - - let (voters, assignments, candidates) = generate_random_votes(CANDIDATES, VOTERS, rng); - let voter_index = make_voter_fn(&voters); - let target_index = make_target_fn(&candidates); - - let compact = Compact::from_assignment(&assignments, &voter_index, &target_index).unwrap(); - - let index_assignments = assignments - .into_iter() - .map(|assignment| IndexAssignment::new(&assignment, &voter_index, &target_index)) - .collect::, _>>() - .unwrap(); - - let index_compact = index_assignments.as_slice().try_into().unwrap(); - - assert_eq!(compact, index_compact); -} diff --git a/primitives/npos-elections/src/mock.rs b/primitives/npos-elections/src/mock.rs index 14e4139c5d324..363550ed8efcc 100644 --- a/primitives/npos-elections/src/mock.rs +++ b/primitives/npos-elections/src/mock.rs @@ -17,9 +17,15 @@ //! Mock file for npos-elections. -#![cfg(test)] +#![cfg(any(test, mocks))] -use crate::*; +use std::{ + collections::{HashSet, HashMap}, + convert::TryInto, + hash::Hash, +}; + +use rand::{self, Rng, seq::SliceRandom}; use sp_arithmetic::{ traits::{One, SaturatedConversion, Zero}, PerThing, @@ -27,6 +33,24 @@ use sp_arithmetic::{ use sp_runtime::assert_eq_error_rate; use sp_std::collections::btree_map::BTreeMap; +use crate::{Assignment, ElectionResult, ExtendedBalance, PerThing128, VoteWeight, seq_phragmen}; + +sp_npos_elections_compact::generate_solution_type!( + #[compact] + pub struct Compact::(16) +); + +pub type AccountId = u64; +/// The candidate mask allows easy disambiguation between voters and candidates: accounts +/// for which this bit is set are candidates, and without it, are voters. +pub const CANDIDATE_MASK: AccountId = 1 << ((std::mem::size_of::() * 8) - 1); +pub type CandidateId = AccountId; + +pub type Accuracy = sp_runtime::Perbill; + +pub type MockAssignment = crate::Assignment; +pub type Voter = (AccountId, VoteWeight, Vec); + #[derive(Default, Debug)] pub(crate) struct _Candidate { who: A, @@ -60,8 +84,6 @@ pub(crate) struct _Support { pub(crate) type _Assignment = (A, f64); pub(crate) type _SupportMap = BTreeMap>; -pub(crate) type AccountId = u64; - #[derive(Debug, Clone)] pub(crate) struct _ElectionResult { pub winners: Vec<(A, ExtendedBalance)>, @@ -72,14 +94,13 @@ pub(crate) fn auto_generate_self_voters(candidates: &[A]) -> Vec<(A, V candidates.iter().map(|c| (c.clone(), vec![c.clone()])).collect() } -pub(crate) fn elect_float( +pub(crate) fn elect_float( candidate_count: usize, initial_candidates: Vec, initial_voters: Vec<(A, Vec)>, - stake_of: FS, + stake_of: impl Fn(&A) -> VoteWeight, ) -> Option<_ElectionResult> where A: Default + Ord + Copy, - for<'r> FS: Fn(&'r A) -> VoteWeight, { let mut elected_candidates: Vec<(A, ExtendedBalance)>; let mut assigned: Vec<(A, Vec<_Assignment>)>; @@ -299,16 +320,15 @@ pub(crate) fn do_equalize_float( pub(crate) fn create_stake_of(stakes: &[(AccountId, VoteWeight)]) - -> Box VoteWeight> + -> impl Fn(&AccountId) -> VoteWeight { let mut storage = BTreeMap::::new(); stakes.iter().for_each(|s| { storage.insert(s.0, s.1); }); - let stake_of = move |who: &AccountId| -> VoteWeight { storage.get(who).unwrap().to_owned() }; - Box::new(stake_of) + move |who: &AccountId| -> VoteWeight { storage.get(who).unwrap().to_owned() } } -pub fn check_assignments_sum(assignments: Vec>) { +pub fn check_assignments_sum(assignments: &[Assignment]) { for Assignment { distribution, .. } in assignments { let mut sum: u128 = Zero::zero(); distribution.iter().for_each(|(_, p)| sum += p.deconstruct().saturated_into::()); @@ -316,12 +336,16 @@ pub fn check_assignments_sum(assignments: Vec( +pub(crate) fn run_and_compare( candidates: Vec, voters: Vec<(AccountId, Vec)>, - stake_of: &Box VoteWeight>, + stake_of: FS, to_elect: usize, -) { +) +where + Output: PerThing128, + FS: Fn(&AccountId) -> VoteWeight, +{ // run fixed point code. let ElectionResult { winners, assignments } = seq_phragmen::<_, Output>( to_elect, @@ -340,10 +364,10 @@ pub(crate) fn run_and_compare( assert_eq!(winners.iter().map(|(x, _)| x).collect::>(), truth_value.winners.iter().map(|(x, _)| x).collect::>()); - for Assignment { who, distribution } in assignments.clone() { - if let Some(float_assignments) = truth_value.assignments.iter().find(|x| x.0 == who) { + for Assignment { who, distribution } in assignments.iter() { + if let Some(float_assignments) = truth_value.assignments.iter().find(|x| x.0 == *who) { for (candidate, per_thingy) in distribution { - if let Some(float_assignment) = float_assignments.1.iter().find(|x| x.0 == candidate ) { + if let Some(float_assignment) = float_assignments.1.iter().find(|x| x.0 == *candidate ) { assert_eq_error_rate!( Output::from_float(float_assignment.1).deconstruct(), per_thingy.deconstruct(), @@ -362,15 +386,13 @@ pub(crate) fn run_and_compare( } } - check_assignments_sum(assignments); + check_assignments_sum(&assignments); } -pub(crate) fn build_support_map_float( +pub(crate) fn build_support_map_float( result: &mut _ElectionResult, - stake_of: FS, -) -> _SupportMap - where for<'r> FS: Fn(&'r AccountId) -> VoteWeight -{ + stake_of: impl Fn(&AccountId) -> VoteWeight, +) -> _SupportMap { let mut supports = <_SupportMap>::new(); result.winners .iter() @@ -393,3 +415,124 @@ pub(crate) fn build_support_map_float( } supports } + +/// Generate voter and assignment lists. Makes no attempt to be realistic about winner or assignment fairness. +/// +/// Maintains these invariants: +/// +/// - candidate ids have `CANDIDATE_MASK` bit set +/// - voter ids do not have `CANDIDATE_MASK` bit set +/// - assignments have the same ordering as voters +/// - `assignments.distribution.iter().map(|(_, frac)| frac).sum() == One::one()` +/// - a coherent set of winners is chosen. +/// - the winner set is a subset of the candidate set. +/// - `assignments.distribution.iter().all(|(who, _)| winners.contains(who))` +pub fn generate_random_votes( + candidate_count: usize, + voter_count: usize, + mut rng: impl Rng, +) -> (Vec, Vec, Vec) { + // cache for fast generation of unique candidate and voter ids + let mut used_ids = HashSet::with_capacity(candidate_count + voter_count); + + // candidates are easy: just a completely random set of IDs + let mut candidates: Vec = Vec::with_capacity(candidate_count); + while candidates.len() < candidate_count { + let mut new = || rng.gen::() | CANDIDATE_MASK; + let mut id = new(); + // insert returns `false` when the value was already present + while !used_ids.insert(id) { + id = new(); + } + candidates.push(id); + } + + // voters are random ids, random weights, random selection from the candidates + let mut voters = Vec::with_capacity(voter_count); + while voters.len() < voter_count { + let mut new = || rng.gen::() & !CANDIDATE_MASK; + let mut id = new(); + // insert returns `false` when the value was already present + while !used_ids.insert(id) { + id = new(); + } + + let vote_weight = rng.gen(); + + // it's not interesting if a voter chooses 0 or all candidates, so rule those cases out. + // also, let's not generate any cases which result in a compact overflow. + let n_candidates_chosen = rng.gen_range(1, candidates.len().min(16)); + + let mut chosen_candidates = Vec::with_capacity(n_candidates_chosen); + chosen_candidates.extend(candidates.choose_multiple(&mut rng, n_candidates_chosen)); + voters.push((id, vote_weight, chosen_candidates)); + } + + // always generate a sensible number of winners: elections are uninteresting if nobody wins, + // or everybody wins + let num_winners = rng.gen_range(1, candidate_count); + let mut winners: HashSet = HashSet::with_capacity(num_winners); + winners.extend(candidates.choose_multiple(&mut rng, num_winners)); + assert_eq!(winners.len(), num_winners); + + let mut assignments = Vec::with_capacity(voters.len()); + for (voter_id, _, votes) in voters.iter() { + let chosen_winners = votes.iter().filter(|vote| winners.contains(vote)).cloned(); + let num_chosen_winners = chosen_winners.clone().count(); + + // distribute the available stake randomly + let stake_distribution = if num_chosen_winners == 0 { + Vec::new() + } else { + let mut available_stake = 1000; + let mut stake_distribution = Vec::with_capacity(num_chosen_winners); + for _ in 0..num_chosen_winners - 1 { + let stake = rng.gen_range(0, available_stake); + stake_distribution.push(Accuracy::from_perthousand(stake)); + available_stake -= stake; + } + stake_distribution.push(Accuracy::from_perthousand(available_stake)); + stake_distribution.shuffle(&mut rng); + stake_distribution + }; + + assignments.push(MockAssignment { + who: *voter_id, + distribution: chosen_winners.zip(stake_distribution).collect(), + }); + } + + (voters, assignments, candidates) +} + +fn generate_cache(voters: Voters) -> HashMap +where + Voters: Iterator, + Item: Hash + Eq + Copy, +{ + let mut cache = HashMap::new(); + for (idx, voter_id) in voters.enumerate() { + cache.insert(voter_id, idx); + } + cache +} + +/// Create a function that returns the index of a voter in the voters list. +pub fn make_voter_fn(voters: &[Voter]) -> impl Fn(&AccountId) -> Option +where + usize: TryInto, +{ + let cache = generate_cache(voters.iter().map(|(id, _, _)| *id)); + move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) +} + +/// Create a function that returns the index of a candidate in the candidates list. +pub fn make_target_fn( + candidates: &[CandidateId], +) -> impl Fn(&CandidateId) -> Option +where + usize: TryInto, +{ + let cache = generate_cache(candidates.iter().cloned()); + move |who| cache.get(who).cloned().and_then(|i| i.try_into().ok()) +} diff --git a/primitives/npos-elections/src/tests.rs b/primitives/npos-elections/src/tests.rs index b2071e924909b..06505721fd23f 100644 --- a/primitives/npos-elections/src/tests.rs +++ b/primitives/npos-elections/src/tests.rs @@ -19,11 +19,13 @@ use crate::{ balancing, helpers::*, is_score_better, mock::*, seq_phragmen, seq_phragmen_core, setup_inputs, - to_support_map, to_supports, Assignment, ElectionResult, ExtendedBalance, StakedAssignment, - Support, Voter, EvaluateSupport, + to_support_map, to_supports, Assignment, CompactSolution, ElectionResult, ExtendedBalance, + IndexAssignment, StakedAssignment, Support, Voter, EvaluateSupport, }; +use rand::{self, SeedableRng}; use sp_arithmetic::{PerU16, Perbill, Percent, Permill}; use substrate_test_utils::assert_eq_uvec; +use std::convert::TryInto; #[test] fn float_phragmen_poc_works() { @@ -423,10 +425,10 @@ fn phragmen_poc_2_works() { (4, 500), ]); - run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); - run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); - run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); - run_and_compare::(candidates, voters, &stake_of, 2); + run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); + run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); + run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); + run_and_compare::(candidates, voters, &stake_of, 2); } #[test] @@ -444,10 +446,10 @@ fn phragmen_poc_3_works() { (4, 1000), ]); - run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); - run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); - run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); - run_and_compare::(candidates, voters, &stake_of, 2); + run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); + run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); + run_and_compare::(candidates.clone(), voters.clone(), &stake_of, 2); + run_and_compare::(candidates, voters, &stake_of, 2); } #[test] @@ -475,7 +477,7 @@ fn phragmen_accuracy_on_large_scale_only_candidates() { assert_eq_uvec!(winners, vec![(1, 18446744073709551614u128), (5, 18446744073709551613u128)]); assert_eq!(assignments.len(), 2); - check_assignments_sum(assignments); + check_assignments_sum(&assignments); } #[test] @@ -527,7 +529,7 @@ fn phragmen_accuracy_on_large_scale_voters_and_candidates() { ] ); - check_assignments_sum(assignments); + check_assignments_sum(&assignments); } #[test] @@ -549,7 +551,7 @@ fn phragmen_accuracy_on_small_scale_self_vote() { ).unwrap(); assert_eq_uvec!(winners, vec![(20, 2), (10, 1), (30, 1)]); - check_assignments_sum(assignments); + check_assignments_sum(&assignments); } #[test] @@ -580,7 +582,7 @@ fn phragmen_accuracy_on_small_scale_no_self_vote() { ).unwrap(); assert_eq_uvec!(winners, vec![(20, 2), (10, 1), (30, 1)]); - check_assignments_sum(assignments); + check_assignments_sum(&assignments); } @@ -615,7 +617,7 @@ fn phragmen_large_scale_test() { ).unwrap(); assert_eq_uvec!(to_without_backing(winners.clone()), vec![24, 22]); - check_assignments_sum(assignments); + check_assignments_sum(&assignments); } #[test] @@ -663,7 +665,7 @@ fn phragmen_large_scale_test_2() { ], ); - check_assignments_sum(assignments); + check_assignments_sum(&assignments); } #[test] @@ -696,7 +698,7 @@ fn phragmen_linear_equalize() { (130, 1000), ]); - run_and_compare::(candidates, voters, &stake_of, 2); + run_and_compare::(candidates, voters, &stake_of, 2); } #[test] @@ -1564,3 +1566,24 @@ mod solution_type { ); } } + +#[test] +fn index_assignments_generate_same_compact_as_plain_assignments() { + let rng = rand::rngs::SmallRng::seed_from_u64(0); + + let (voters, assignments, candidates) = generate_random_votes(1000, 2500, rng); + let voter_index = make_voter_fn(&voters); + let target_index = make_target_fn(&candidates); + + let compact = Compact::from_assignment(&assignments, &voter_index, &target_index).unwrap(); + + let index_assignments = assignments + .into_iter() + .map(|assignment| IndexAssignment::new(&assignment, &voter_index, &target_index)) + .collect::, _>>() + .unwrap(); + + let index_compact = index_assignments.as_slice().try_into().unwrap(); + + assert_eq!(compact, index_compact); +} From 8f0e9c56023e72e8f5f1996ae1d01ea65bcd69e1 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 21 Apr 2021 14:25:45 +0200 Subject: [PATCH 32/34] rename assignments -> sorted_assignments --- frame/election-provider-multi-phase/src/unsigned.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 42f9becbfeb0e..6fe291a4bc3e2 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -187,10 +187,13 @@ impl Pallet { sp_std::cmp::Reverse(stake) }); + // rename to emphasize that ordering matters + let mut sorted_assignments = assignments; + // Reduce (requires round-trip to staked form) - assignments = { + sorted_assignments = { // convert to staked and reduce. - let mut staked = assignment_ratio_to_staked_normalized(assignments, &stake_of)?; + let mut staked = assignment_ratio_to_staked_normalized(sorted_assignments, &stake_of)?; sp_npos_elections::reduce(&mut staked); // convert back. @@ -199,7 +202,7 @@ impl Pallet { // convert to `IndexAssignment`. This improves the runtime complexity of repeatedly // converting to `Compact`. - let mut index_assignments = assignments + let mut index_assignments = sorted_assignments .into_iter() .map(|assignment| IndexAssignmentOf::::new(&assignment, &voter_index, &target_index)) .collect::, _>>()?; From 26726f86457f7bfc328d6c01a55a7a3c9f2ddbd6 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 21 Apr 2021 16:28:26 +0200 Subject: [PATCH 33/34] sort after reducing to avoid potential re-sort issues --- .../src/unsigned.rs | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 6fe291a4bc3e2..2c068e0805282 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -176,26 +176,30 @@ impl Pallet { CompactOf::::try_from(assignments).map(|compact| compact.encoded_size()) }; - let ElectionResult { mut assignments, winners } = election_result; - // Sort the assignments by reversed voter stake. This ensures that we can efficiently - // truncate the list. - assignments.sort_by_key(|Assignment:: { who, .. }| { - let stake = cache.get(who).map(|idx| { - let (_, stake, _) = voters[*idx]; - stake - }).unwrap_or_default(); - sp_std::cmp::Reverse(stake) - }); - - // rename to emphasize that ordering matters - let mut sorted_assignments = assignments; + let ElectionResult { assignments, winners } = election_result; // Reduce (requires round-trip to staked form) - sorted_assignments = { + let sorted_assignments = { // convert to staked and reduce. - let mut staked = assignment_ratio_to_staked_normalized(sorted_assignments, &stake_of)?; + let mut staked = assignment_ratio_to_staked_normalized(assignments, &stake_of)?; + + // we reduce before sorting in order to ensure that the reduction process doesn't + // accidentally change the sort order sp_npos_elections::reduce(&mut staked); + // Sort the assignments by reversed voter stake. This ensures that we can efficiently + // truncate the list. + staked.sort_by_key(|sp_npos_elections::StakedAssignment:: { who, .. }| { + // though staked assignments are expressed in terms of absolute stake, we'd still + // need to iterate over all votes in order to actually compute the total stake. + // it should be faster to look it up from the cache. + let stake = cache.get(who).map(|idx| { + let (_, stake, _) = voters[*idx]; + stake + }).unwrap_or_default(); + sp_std::cmp::Reverse(stake) + }); + // convert back. assignment_staked_to_ratio_normalized(staked)? }; From c9f25179556fc1b42f5fe59f9ab40d96eaa21307 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 21 Apr 2021 16:30:17 +0200 Subject: [PATCH 34/34] add runtime benchmark, fix critical binary search error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Why don't you add a benchmark?", he said. "It'll be good practice, and can help demonstrate that this isn't blowing up the runtime." He was absolutely right. The biggest discovery is that adding a parametric benchmark means that you get a bunch of new test cases, for free. This is excellent, because those test cases uncovered a binary search bug. Fixing that simplified that part of the code nicely. The other nice thing you get from a parametric benchmark is data about what each parameter does. In this case, `f` is the size factor: what percent of the votes (by size) should be removed. 0 means that we should keep everything, 95 means that we should trim down to 5% of original size or less. ``` Median Slopes Analysis ======== -- Extrinsic Time -- Model: Time ~= 3846 + v 0.015 + t 0 + a 0.192 + d 0 + f 0 µs Min Squares Analysis ======== -- Extrinsic Time -- Data points distribution: v t a d f mean µs sigma µs % 6000 1600 3000 800 0 4385 75.87 1.7% 6000 1600 3000 800 9 4089 46.28 1.1% 6000 1600 3000 800 18 3793 36.45 0.9% 6000 1600 3000 800 27 3365 41.13 1.2% 6000 1600 3000 800 36 3096 7.498 0.2% 6000 1600 3000 800 45 2774 17.96 0.6% 6000 1600 3000 800 54 2057 37.94 1.8% 6000 1600 3000 800 63 1885 2.515 0.1% 6000 1600 3000 800 72 1591 3.203 0.2% 6000 1600 3000 800 81 1219 25.72 2.1% 6000 1600 3000 800 90 859 5.295 0.6% 6000 1600 3000 800 95 684.6 2.969 0.4% Quality and confidence: param error v 0.008 t 0.029 a 0.008 d 0.044 f 0.185 Model: Time ~= 3957 + v 0.009 + t 0 + a 0.185 + d 0 + f 0 µs ``` What's nice about this is the clear negative correlation between amount removed and total time. The more we remove, the less total time things take. --- .../election-provider-multi-phase/Cargo.toml | 2 + .../src/benchmarking.rs | 70 ++++++++++++++++++- .../src/unsigned.rs | 49 +++++++------ primitives/npos-elections/src/helpers.rs | 7 +- 4 files changed, 98 insertions(+), 30 deletions(-) diff --git a/frame/election-provider-multi-phase/Cargo.toml b/frame/election-provider-multi-phase/Cargo.toml index 6b2219871ab82..643c768ce8709 100644 --- a/frame/election-provider-multi-phase/Cargo.toml +++ b/frame/election-provider-multi-phase/Cargo.toml @@ -39,6 +39,7 @@ hex-literal = "0.3.1" substrate-test-utils = { version = "3.0.0", path = "../../test-utils" } sp-core = { version = "3.0.0", default-features = false, path = "../../primitives/core" } sp-io = { version = "3.0.0", path = "../../primitives/io" } +sp-npos-elections = { version = "3.0.0", default-features = false, features = [ "mocks" ], path = "../../primitives/npos-elections" } sp-tracing = { version = "3.0.0", path = "../../primitives/tracing" } frame-election-provider-support = { version = "3.0.0", features = ["runtime-benchmarks"], path = "../election-provider-support" } pallet-balances = { version = "3.0.0", path = "../balances" } @@ -64,5 +65,6 @@ std = [ runtime-benchmarks = [ "frame-benchmarking", "rand", + "sp-npos-elections/mocks", ] try-runtime = ["frame-support/try-runtime"] diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index f6a3062a2ff9f..4eade8e184e75 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -18,15 +18,16 @@ //! Two phase election pallet benchmarking. use super::*; -use crate::Pallet as MultiPhase; +use crate::{Pallet as MultiPhase, unsigned::IndexAssignmentOf}; use frame_benchmarking::impl_benchmark_test_suite; use frame_support::{assert_ok, traits::OnInitialize}; use frame_system::RawOrigin; use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng}; use frame_election_provider_support::Assignment; -use sp_arithmetic::traits::One; +use sp_arithmetic::{per_things::Percent, traits::One}; +use sp_npos_elections::IndexAssignment; use sp_runtime::InnerOf; -use sp_std::convert::TryInto; +use sp_std::convert::{TryFrom, TryInto}; const SEED: u32 = 999; @@ -254,6 +255,69 @@ frame_benchmarking::benchmarks! { assert!(>::queued_solution().is_some()); } + #[extra] + trim_assignments_length { + // number of votes in snapshot. + let v in (T::BenchmarkingConfig::VOTERS[0]) .. T::BenchmarkingConfig::VOTERS[1]; + // number of targets in snapshot. + let t in (T::BenchmarkingConfig::TARGETS[0]) .. 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 (T::BenchmarkingConfig::ACTIVE_VOTERS[0]) .. T::BenchmarkingConfig::ACTIVE_VOTERS[1]; + // number of desired targets. Must be a subset of `t` component. + let d in (T::BenchmarkingConfig::DESIRED_TARGETS[0]) .. T::BenchmarkingConfig::DESIRED_TARGETS[1]; + // Subtract this percentage from the actual encoded size + let f in 0 .. 95; + + // Compute a random solution, then work backwards to get the lists of voters, targets, and assignments + let witness = SolutionOrSnapshotSize { voters: v, targets: t }; + let RawSolution { compact, .. } = solution_with_size::(witness, a, d); + let RoundSnapshot { voters, targets } = MultiPhase::::snapshot().unwrap(); + let voter_at = helpers::voter_at_fn::(&voters); + let target_at = helpers::target_at_fn::(&targets); + let mut assignments = compact.into_assignment(voter_at, target_at).unwrap(); + + // make a voter cache and some helper functions for access + let cache = helpers::generate_voter_cache::(&voters); + let voter_index = helpers::voter_index_fn::(&cache); + let target_index = helpers::target_index_fn::(&targets); + + // sort assignments by decreasing voter stake + assignments.sort_by_key(|crate::unsigned::Assignment:: { who, .. }| { + let stake = cache.get(&who).map(|idx| { + let (_, stake, _) = voters[*idx]; + stake + }).unwrap_or_default(); + sp_std::cmp::Reverse(stake) + }); + + let mut index_assignments = assignments + .into_iter() + .map(|assignment| IndexAssignment::new(&assignment, &voter_index, &target_index)) + .collect::, _>>() + .unwrap(); + + let encoded_size_of = |assignments: &[IndexAssignmentOf]| { + CompactOf::::try_from(assignments).map(|compact| compact.encoded_size()) + }; + + let desired_size = Percent::from_percent(100 - f.saturated_into::()) + .mul_ceil(encoded_size_of(index_assignments.as_slice()).unwrap()); + log!(trace, "desired_size = {}", desired_size); + }: { + MultiPhase::::trim_assignments_length( + desired_size.saturated_into(), + &mut index_assignments, + &encoded_size_of, + ).unwrap(); + } verify { + let compact = CompactOf::::try_from(index_assignments.as_slice()).unwrap(); + let encoding = compact.encode(); + log!(trace, "encoded size prediction = {}", encoded_size_of(index_assignments.as_slice()).unwrap()); + log!(trace, "actual encoded size = {}", encoding.len()); + assert!(encoding.len() <= desired_size); + } + // This is checking a valid solution. The worse case is indeed a valid solution. feasibility_check { // number of votes in snapshot. diff --git a/frame/election-provider-multi-phase/src/unsigned.rs b/frame/election-provider-multi-phase/src/unsigned.rs index 2c068e0805282..8ab3a81aa3d27 100644 --- a/frame/election-provider-multi-phase/src/unsigned.rs +++ b/frame/election-provider-multi-phase/src/unsigned.rs @@ -189,16 +189,21 @@ impl Pallet { // Sort the assignments by reversed voter stake. This ensures that we can efficiently // truncate the list. - staked.sort_by_key(|sp_npos_elections::StakedAssignment:: { who, .. }| { - // though staked assignments are expressed in terms of absolute stake, we'd still - // need to iterate over all votes in order to actually compute the total stake. - // it should be faster to look it up from the cache. - let stake = cache.get(who).map(|idx| { - let (_, stake, _) = voters[*idx]; - stake - }).unwrap_or_default(); - sp_std::cmp::Reverse(stake) - }); + staked.sort_by_key( + |sp_npos_elections::StakedAssignment:: { who, .. }| { + // though staked assignments are expressed in terms of absolute stake, we'd + // still need to iterate over all votes in order to actually compute the total + // stake. it should be faster to look it up from the cache. + let stake = cache + .get(who) + .map(|idx| { + let (_, stake, _) = voters[*idx]; + stake + }) + .unwrap_or_default(); + sp_std::cmp::Reverse(stake) + }, + ); // convert back. assignment_staked_to_ratio_normalized(staked)? @@ -303,7 +308,7 @@ impl Pallet { /// /// The score must be computed **after** this step. If this step reduces the score too much, /// then the solution must be discarded. - fn trim_assignments_length( + pub(crate) fn trim_assignments_length( max_allowed_length: u32, assignments: &mut Vec>, encoded_size_of: impl Fn(&[IndexAssignmentOf]) -> Result, @@ -313,23 +318,21 @@ impl Pallet { let max_allowed_length: usize = max_allowed_length.saturated_into(); let mut high = assignments.len(); let mut low = 0; - let mut maximum_allowed_voters = 0; - let mut size; while high - low > 1 { - maximum_allowed_voters = (high + low) / 2; - size = encoded_size_of(&assignments[..maximum_allowed_voters])?; - if size > max_allowed_length { - high = maximum_allowed_voters; + let test = (high + low) / 2; + if encoded_size_of(&assignments[..test])? <= max_allowed_length { + low = test; } else { - low = maximum_allowed_voters; + high = test; } } - if high - low == 1 - && encoded_size_of(&assignments[..maximum_allowed_voters + 1])? <= max_allowed_length - { - maximum_allowed_voters += 1; - } + let maximum_allowed_voters = + if encoded_size_of(&assignments[..low + 1])? <= max_allowed_length { + low + 1 + } else { + low + }; // ensure our postconditions are correct debug_assert!( diff --git a/primitives/npos-elections/src/helpers.rs b/primitives/npos-elections/src/helpers.rs index 091efdd36ea5f..9fdf76118f89f 100644 --- a/primitives/npos-elections/src/helpers.rs +++ b/primitives/npos-elections/src/helpers.rs @@ -72,10 +72,9 @@ pub fn assignment_staked_to_ratio_normalized( staked: Vec>, ) -> Result>, Error> { let mut ratio = staked.into_iter().map(|a| a.into_assignment()).collect::>(); - ratio - .iter_mut() - .map(|a| a.try_normalize().map_err(|err| Error::ArithmeticError(err))) - .collect::>()?; + for assignment in ratio.iter_mut() { + assignment.try_normalize().map_err(|err| Error::ArithmeticError(err))?; + } Ok(ratio) }