From 58f4b6145c732827c242023a044f7908b966d5aa Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 31 Aug 2020 13:28:55 +0200 Subject: [PATCH 01/36] Apply. --- frame/staking/src/lib.rs | 2 +- primitives/npos-elections/src/lib.rs | 39 +++- primitives/npos-elections/src/pjr.rs | 335 +++++++++++++++++++++++++++ 3 files changed, 366 insertions(+), 10 deletions(-) create mode 100644 primitives/npos-elections/src/pjr.rs diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index e5c1a68dfbeae..34cda1d46ba8b 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -2617,8 +2617,8 @@ impl Module { // write new results. >::put(ElectionResult { elected_stashes: winners, - compute, exposures, + compute, }); QueuedScore::put(submitted_score); diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index 11951d2065989..bfc2feb1022e6 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -18,10 +18,10 @@ //! - [`seq_phragmen`]: Implements the Phragmén Sequential Method. An un-ranked, relatively fast //! election method that ensures PJR, but does not provide a constant factor approximation of the //! maximin problem. -//! - [`phragmms`]: Implements a hybrid approach inspired by Phragmén which is executed faster but +//! - [`phragmms`](phragmms::phragmms): Implements a hybrid approach inspired by Phragmén which is executed faster but //! it can achieve a constant factor approximation of the maximin problem, similar to that of the //! MMS algorithm. -//! - [`balance_solution`]: Implements the star balancing algorithm. This iterative process can push +//! - [`balance`](balancing::balance): Implements the star balancing algorithm. This iterative process can push //! a solution toward being more `balances`, which in turn can increase its score. //! //! ### Terminology @@ -92,18 +92,20 @@ mod mock; #[cfg(test)] mod tests; -mod phragmen; -mod balancing; -mod phragmms; -mod node; -mod reduce; -mod helpers; +pub mod phragmen; +pub mod balancing; +pub mod phragmms; +pub mod node; +pub mod reduce; +pub mod helpers; +pub mod pjr; pub use reduce::reduce; pub use helpers::*; pub use phragmen::*; pub use phragmms::*; pub use balancing::*; +pub use pjr::*; // re-export the compact macro, with the dependencies of the macro. #[doc(hidden)] @@ -189,6 +191,12 @@ pub struct Candidate { round: usize, } +impl Candidate { + pub fn to_ptr(self) -> CandidatePtr { + Rc::new(RefCell::new(self)) + } +} + /// A vote being casted by a [`Voter`] to a [`Candidate`] is an `Edge`. #[derive(Clone, Default)] pub struct Edge { @@ -233,6 +241,19 @@ impl std::fmt::Debug for Voter { } impl Voter { + /// Create a new `Voter`. + pub fn new(who: AccountId) -> Self { + Self { who, ..Default::default() } + } + + + /// Returns `true` if `self` votes for `target`. + /// + /// Note that this does not take into account if `target` is elected (i.e. is *active*) or not. + pub fn votes_for(&self, target: &AccountId) -> bool { + self.edges.iter().any(|e| &e.who == target) + } + /// Returns none if this voter does not have any non-zero distributions. /// /// Note that this might create _un-normalized_ assignments, due to accuracy loss of `P`. Call @@ -625,7 +646,7 @@ pub(crate) fn setup_inputs( .enumerate() .map(|(idx, who)| { c_idx_cache.insert(who.clone(), idx); - Rc::new(RefCell::new(Candidate { who, ..Default::default() })) + Candidate { who, ..Default::default() }.to_ptr() }) .collect::>>(); diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs new file mode 100644 index 0000000000000..2a700a276f814 --- /dev/null +++ b/primitives/npos-elections/src/pjr.rs @@ -0,0 +1,335 @@ + // 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. + +//! Implements functions and interfaces to check solutions for being t-PJR. +//! +//! PJR stands for proportional justified representation. PJR is a string absolute measure to make +//! sure an NPoS solution adheres to a minimum standard. +//! +//! See [`pjr_check`] which is the main entry point of the module. + +use crate::*; +use sp_std::rc::Rc; +use sp_std::collections::btree_map::BTreeMap; +use sp_arithmetic::{traits::Zero, Perbill}; + + +/// The type used as the threshold. +/// +/// Just some reading sugar; Must always be same as [`ExtendedBalance`]; +type Threshold = ExtendedBalance; + +/// Convert the data types that the user runtime has into ones that can be used by this module. +/// +/// It is expected that this function's interface might change over time, or multiple variants of it +/// can be provided for different use cases. +/// +/// The ultimate goal, in any case, is to convert the election data into [`Candidate`] and [`Voter`] +/// types defined by this crate, whilst setting correct value for some of their fields, namely: +/// 1. Candidate [`backing_stake`](Candidate::backing_stake) and [`elected`](Candidate::elected) if they are a winner. +/// 2. Voter edge [`weight`](Edge::weight) if they are backing a winner. +/// 3. Voter [`budget`](Voter::budget). +/// +/// None of the `load` or `score` values are used and can be ignored. This is similar to +/// [`setup_inputs`] function of this crate. +/// +/// ### Performance (Weight) Notes +/// +/// Note that the current function is rather unfortunately inefficient. The most significant +/// slowdown is the fact that a typical solution that need to be checked for PJR only contains a +/// subset of the entire NPoS edge graph, encoded as `staked_assignment`. This only encodes the +/// edges that actually contribute to a winner's backing stake and ignores the rest to save space. +/// To check PJR, we need the entire voter set, including those edges that point to non-winners. +/// This could cause the caller runtime to have to read the entire list of voters, which is assumed +/// to be expensive. +/// +/// A sensible user of this module should make sure that the PJR check is executed and checked as +/// little as possible, and take sufficient economical measures to ensure that this function cannot +/// be abused. +pub fn prepare_pjr_input( + winners: Vec, + staked_assignments: Vec>, + supports: &SupportMap, + all_candidates: Vec, + all_voters: Vec<(A, VoteWeight, Vec)>, +) -> (Vec>, Vec>) { + // collect all candidates and winners into a unified `Vec`. + let mut candidates_index: BTreeMap = BTreeMap::new(); + + // dump the staked assignments in a voter-major map for faster access down the road. + let mut assignment_map: BTreeMap> = BTreeMap::new(); + staked_assignments + .into_iter() + .for_each(|StakedAssignment { who, distribution }| { + assignment_map.insert(who, distribution); + }); + + let candidates = all_candidates.into_iter().enumerate().map(|(i, c)| { + candidates_index.insert(c.clone(), i); + + // set the backing value and elected flag if the candidate is among the winners. + let who = c; + let elected = winners.iter().any(|w| w == &who); + let backed_stake = supports.get(&who).map(|s| s.total).unwrap_or_default(); + + debug_assert!( + !(elected ^ (backed_stake > 0)), + "If a candidate is elected, then it must have a positive backing as well." + ); + + Candidate { who, elected, backed_stake, ..Default::default() }.to_ptr() + }).collect::>(); + + // collect all voters into a unified Vec. + let voters = all_voters.into_iter().map(|(v, w, ts)| { + let mut edges: Vec> = Vec::with_capacity(ts.len()); + for t in ts { + if edges.iter().any(|e| e.who == t) { + // duplicate edge. + continue; + } + + if let Some(idx) = candidates_index.get(&t) { + // if this edge is among the assignments, set the weight as well. + let weight = assignment_map + .get(&v) + .and_then(|d| d.iter().find_map(|(x, y)| if x == &t { Some(y) } else { None })) + .cloned() + .unwrap_or_default(); + edges.push(Edge { + who: t, + candidate: Rc::clone(&candidates[*idx]), + weight, + ..Default::default() + }); + } + } + + let who = v; + let budget: ExtendedBalance = w.into(); + Voter { who, budget, edges, ..Default::default() } + }).collect::>(); + + (candidates, voters) +} + +/// Check a solution to be t-PJR. +/// +/// ### Semantics +/// +/// For a solution to be t-PJR, the original condition is as such: If there is a group of `N` voters +/// who have `r` common candidates and can afford to support each of them with backing stake `t` +/// (i.e `sum(stake(v) for all voters ) == r * t`), then this committee need to be represented by at +/// least `r` elected candidates. +/// +/// Section 5 of the NPoS paper shows that this property is equal to: For a feasible solution, if +/// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. +/// +/// In this implementation we use the latter definition due to its simplicity. +/// +/// ### Interface +/// +/// In addition to data that can be computed from the [`ElectionResult`] struct, a PJR check also +/// needs to inspect un-elected candidates and edges, thus `all_candidates` and `all_voters`. +/// +/// See [`prepare_pjr_input`] for more info. +pub fn pjr_check( + winners: Vec, + staked_assignments: Vec>, + supports: &SupportMap, + all_candidates: Vec, + all_voters: Vec<(A, VoteWeight, Vec)>, + t: Threshold, +) -> bool { + // prepare data. + let (candidates, voters) = prepare_pjr_input( + winners, + staked_assignments, + supports, + all_candidates, + all_voters, + ); + // compute with threshold t. + pjr_check_core(candidates.as_ref(), voters.as_ref(), t) +} + +/// The internal implementation of the PJR check after having the data converted. +/// +/// See [`pjr_check`] for more info. +pub fn pjr_check_core( + candidates: &[CandidatePtr], + voters: &[Voter], + t: Threshold, +) -> bool { + let unelected = candidates.iter().filter(|c| !c.borrow().elected); + let maybe_max_pre_score = unelected.map(|c| pre_score(Rc::clone(c), voters, t)).max(); + // if unelected is empty then the solution is indeed PJR. + maybe_max_pre_score.map_or(true, |max_pre_score| max_pre_score < t) +} + +/// The pre-score of an unelected candidate. +/// +/// This is the amount of stake that *all voter* can spare to devote to this candidate without +/// allowing the backing stake of any other elected candidate to fall below `t`. +/// +/// In essence, it is the sum(slack(n, t)) for all `n` who vote for `unelected`. +pub fn pre_score( + unelected: CandidatePtr, + voters: &[Voter], + t: Threshold, +) -> ExtendedBalance { + debug_assert!(!unelected.borrow().elected); + voters + .iter() + .filter(|ref v| v.votes_for(&unelected.borrow().who)) + .fold(Zero::zero(), |acc: ExtendedBalance, voter| acc.saturating_add(slack(voter, t))) +} + + +/// The slack of a voter at a given state. +/// +/// The slack of each voter, with threshold `t` is the total amount of stake that this voter can +/// spare to a new potential member, whilst not dropping the backing stake of any of its currently +/// active members below `t`. In essence, for each of the current active candidates `c`, we assume +/// that we reduce the edge weight of `voter` to `c` from `w` to `w * min(1 / (t / support(c)))`. +/// +/// More accurately: +/// +/// 1. If `c` exactly has `t` backing or less, then we don't generate any slack. +/// 2. If `c` has more than `t`, then we reduce it to `t`. +pub fn slack(voter: &Voter, t: Threshold) -> ExtendedBalance { + let budget = voter.budget; + let leftover = voter.edges.iter().fold(Zero::zero(), |acc: ExtendedBalance, edge| { + let candidate = edge.candidate.borrow(); + if candidate.elected { + // TODO: using perbill here is just going to cause annoyance, why not just subtract? + let extra = + Perbill::one().min(Perbill::from_rational_approximation(t, candidate.backed_stake)) + * edge.weight; + acc.saturating_add(extra) + } else { + // No slack generated here. + acc + } + }); + + // NOTE: candidate for saturating_log_sub() + budget.saturating_sub(leftover) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn setup_voter(who: u32, votes: Vec<(u32, u128, bool)>) -> Voter { + let mut voter = Voter::new(who); + let mut budget = 0u128; + let candidates = votes.into_iter().map(|(t, w, e)| { + budget += w; + Candidate { who: t, elected: e, backed_stake: w, ..Default::default() } + }).collect::>(); + let edges = candidates.into_iter().map(|c| + Edge { who: c.who, weight: c.backed_stake, candidate: c.to_ptr(), ..Default::default() } + ).collect::>(); + voter.edges = edges; + voter.budget = budget; + voter + } + + #[test] + fn slack_works() { + let voter = setup_voter(10, vec![(1, 10, true), (2, 20, true)]); + + assert_eq!(slack(&voter, 15), 5); + assert_eq!(slack(&voter, 17), 3); + assert_eq!(slack(&voter, 10), 10); + assert_eq!(slack(&voter, 5), 20); + + } + + #[test] + fn pre_score_works() { + // will give 5 slack + let v1 = setup_voter(10, vec![(1, 10, true), (2, 20, true), (3, 0, false)]); + // will give no slack + let v2 = setup_voter(20, vec![(1, 5, true), (2, 5, true)]); + // will give 10 slack. + let v3 = setup_voter(30, vec![(1, 20, true), (2, 20, true), (3, 0, false)]); + + let unelected = Candidate { who: 3u32, elected: false, ..Default::default() }.to_ptr(); + let score = pre_score(unelected, &vec![v1, v2, v3], 15); + + assert_eq!(score, 15); + } + + #[test] + fn can_convert_data_from_external_api() { + let winners = vec![20u32, 40]; + let all_candidates = vec![10, 20, 30, 40]; + let staked_assignments = vec![ + StakedAssignment { who: 1, distribution: vec![(20, 5), (40, 5)] }, + StakedAssignment { who: 2, distribution: vec![(20, 10), (40, 10)] }, + ]; + let all_voters = vec![ + (1, 10, vec![10, 20, 30, 40]), + (2, 20, vec![10, 20, 30, 40]), + (3, 30, vec![10, 30]), + ]; + let mut supports = SupportMap::::new(); + supports.insert(20, Support { total: 15, voters: vec![(5, 1), (10, 2)]} ); + supports.insert(40, Support { total: 15, voters: vec![(5, 1), (10, 2)]} ); + + let (candidates, voters) = prepare_pjr_input( + winners, + staked_assignments, + &supports, + all_candidates, + all_voters, + ); + + // elected flag and backing must be set correctly + assert_eq!( + candidates + .iter() + .map(|c| (c.borrow().who.clone(), c.borrow().elected, c.borrow().backed_stake)) + .collect::>(), + vec![(10, false, 0), (20, true, 15), (30, false, 0), (40, true, 15)], + ); + + // edge weight must be set correctly + assert_eq!( + voters + .iter() + .map(|v| ( + v.who, + v.budget, + v.edges.iter().map(|e| (e.who, e.weight)).collect::>(), + )).collect::>(), + vec![ + (1, 10, vec![(10, 0), (20, 5), (30, 0), (40, 5)]), + (2, 20, vec![(10, 0), (20, 10), (30, 0), (40, 10)]), + (3, 30, vec![(10, 0), (30, 0)]), + ], + ); + + // fyi. this is not PJR, obviously because the votes of 3 can bump the stake a lot but they + // are being ignored. + assert!(!pjr_check_core(&candidates, &voters, 1)); + assert!(!pjr_check_core(&candidates, &voters, 10)); + assert!(!pjr_check_core(&candidates, &voters, 20)); + } +} From 12f463665f8f597704d863a4e8fb66ed51306498 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 18 Feb 2021 14:22:54 +0100 Subject: [PATCH 02/36] get rid of glob import --- primitives/npos-elections/src/pjr.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 2a700a276f814..d7aa9e87cc0dd 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -22,7 +22,17 @@ //! //! See [`pjr_check`] which is the main entry point of the module. -use crate::*; +use crate::{ + Candidate, + CandidatePtr, + Edge, + ExtendedBalance, + IdentifierT, + StakedAssignment, + SupportMap, + Voter, + VoteWeight, +}; use sp_std::rc::Rc; use sp_std::collections::btree_map::BTreeMap; use sp_arithmetic::{traits::Zero, Perbill}; From 834e675560e683679d3476a59c5da3f1d22ee549 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 18 Feb 2021 14:26:45 +0100 Subject: [PATCH 03/36] use meaningful generic type name --- primitives/npos-elections/src/pjr.rs | 46 ++++++++++++++-------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index d7aa9e87cc0dd..6035316e305fc 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -70,18 +70,18 @@ type Threshold = ExtendedBalance; /// A sensible user of this module should make sure that the PJR check is executed and checked as /// little as possible, and take sufficient economical measures to ensure that this function cannot /// be abused. -pub fn prepare_pjr_input( - winners: Vec, - staked_assignments: Vec>, - supports: &SupportMap, - all_candidates: Vec, - all_voters: Vec<(A, VoteWeight, Vec)>, -) -> (Vec>, Vec>) { +pub fn prepare_pjr_input( + winners: Vec, + staked_assignments: Vec>, + supports: &SupportMap, + all_candidates: Vec, + all_voters: Vec<(AccountId, VoteWeight, Vec)>, +) -> (Vec>, Vec>) { // collect all candidates and winners into a unified `Vec`. - let mut candidates_index: BTreeMap = BTreeMap::new(); + let mut candidates_index: BTreeMap = BTreeMap::new(); // dump the staked assignments in a voter-major map for faster access down the road. - let mut assignment_map: BTreeMap> = BTreeMap::new(); + let mut assignment_map: BTreeMap> = BTreeMap::new(); staked_assignments .into_iter() .for_each(|StakedAssignment { who, distribution }| { @@ -106,7 +106,7 @@ pub fn prepare_pjr_input( // collect all voters into a unified Vec. let voters = all_voters.into_iter().map(|(v, w, ts)| { - let mut edges: Vec> = Vec::with_capacity(ts.len()); + let mut edges: Vec> = Vec::with_capacity(ts.len()); for t in ts { if edges.iter().any(|e| e.who == t) { // duplicate edge. @@ -157,12 +157,12 @@ pub fn prepare_pjr_input( /// needs to inspect un-elected candidates and edges, thus `all_candidates` and `all_voters`. /// /// See [`prepare_pjr_input`] for more info. -pub fn pjr_check( - winners: Vec, - staked_assignments: Vec>, - supports: &SupportMap, - all_candidates: Vec, - all_voters: Vec<(A, VoteWeight, Vec)>, +pub fn pjr_check( + winners: Vec, + staked_assignments: Vec>, + supports: &SupportMap, + all_candidates: Vec, + all_voters: Vec<(AccountId, VoteWeight, Vec)>, t: Threshold, ) -> bool { // prepare data. @@ -180,9 +180,9 @@ pub fn pjr_check( /// The internal implementation of the PJR check after having the data converted. /// /// See [`pjr_check`] for more info. -pub fn pjr_check_core( - candidates: &[CandidatePtr], - voters: &[Voter], +pub fn pjr_check_core( + candidates: &[CandidatePtr], + voters: &[Voter], t: Threshold, ) -> bool { let unelected = candidates.iter().filter(|c| !c.borrow().elected); @@ -197,9 +197,9 @@ pub fn pjr_check_core( /// allowing the backing stake of any other elected candidate to fall below `t`. /// /// In essence, it is the sum(slack(n, t)) for all `n` who vote for `unelected`. -pub fn pre_score( - unelected: CandidatePtr, - voters: &[Voter], +pub fn pre_score( + unelected: CandidatePtr, + voters: &[Voter], t: Threshold, ) -> ExtendedBalance { debug_assert!(!unelected.borrow().elected); @@ -221,7 +221,7 @@ pub fn pre_score( /// /// 1. If `c` exactly has `t` backing or less, then we don't generate any slack. /// 2. If `c` has more than `t`, then we reduce it to `t`. -pub fn slack(voter: &Voter, t: Threshold) -> ExtendedBalance { +pub fn slack(voter: &Voter, t: Threshold) -> ExtendedBalance { let budget = voter.budget; let leftover = voter.edges.iter().fold(Zero::zero(), |acc: ExtendedBalance, edge| { let candidate = edge.candidate.borrow(); From 963f48cdab9fd5182b9eeff94e044beb9b5a9a42 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 10:41:00 +0100 Subject: [PATCH 04/36] pjr_check operates on `Supports` struct used elsewhere --- primitives/npos-elections/src/pjr.rs | 76 +++++++++++++++------------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 6035316e305fc..c3e4f4938ff04 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -28,8 +28,8 @@ use crate::{ Edge, ExtendedBalance, IdentifierT, - StakedAssignment, - SupportMap, + Support, + Supports, Voter, VoteWeight, }; @@ -70,10 +70,8 @@ type Threshold = ExtendedBalance; /// A sensible user of this module should make sure that the PJR check is executed and checked as /// little as possible, and take sufficient economical measures to ensure that this function cannot /// be abused. -pub fn prepare_pjr_input( - winners: Vec, - staked_assignments: Vec>, - supports: &SupportMap, +fn prepare_pjr_input( + supports: &Supports, all_candidates: Vec, all_voters: Vec<(AccountId, VoteWeight, Vec)>, ) -> (Vec>, Vec>) { @@ -82,19 +80,20 @@ pub fn prepare_pjr_input( // dump the staked assignments in a voter-major map for faster access down the road. let mut assignment_map: BTreeMap> = BTreeMap::new(); - staked_assignments - .into_iter() - .for_each(|StakedAssignment { who, distribution }| { - assignment_map.insert(who, distribution); - }); + for (winner_id, Support { voters, .. }) in supports.iter() { + for (voter_id, support) in voters.iter() { + assignment_map.entry(voter_id.clone()).or_default().push((winner_id.clone(), *support)); + } + } let candidates = all_candidates.into_iter().enumerate().map(|(i, c)| { candidates_index.insert(c.clone(), i); // set the backing value and elected flag if the candidate is among the winners. let who = c; - let elected = winners.iter().any(|w| w == &who); - let backed_stake = supports.get(&who).map(|s| s.total).unwrap_or_default(); + let maybe_support = supports.iter().find(|(winner, _support)| winner == &who); + let elected = maybe_support.is_some(); + let backed_stake = maybe_support.map(|(_id, support)| support.total).unwrap_or_default(); debug_assert!( !(elected ^ (backed_stake > 0)), @@ -141,15 +140,19 @@ pub fn prepare_pjr_input( /// /// ### Semantics /// -/// For a solution to be t-PJR, the original condition is as such: If there is a group of `N` voters +/// The t-PJR property is defined in the paper "Validator Election in Nominated Proof-of-Stake", +/// section 5, definition 1. +/// +/// In plain language, the t-PJR condition is: ff there is a group of `N` voters /// who have `r` common candidates and can afford to support each of them with backing stake `t` -/// (i.e `sum(stake(v) for all voters ) == r * t`), then this committee need to be represented by at +/// (i.e `sum(stake(v) for v in voters) == r * t`), then this committee needs to be represented by at /// least `r` elected candidates. /// /// Section 5 of the NPoS paper shows that this property is equal to: For a feasible solution, if /// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. /// -/// In this implementation we use the latter definition due to its simplicity. +/// The text notes that we can verify this condition by running Algorithm 5: MaxPrescore and validating +/// that MaxPrescore(A, w, t) < t). /// /// ### Interface /// @@ -157,18 +160,26 @@ pub fn prepare_pjr_input( /// needs to inspect un-elected candidates and edges, thus `all_candidates` and `all_voters`. /// /// See [`prepare_pjr_input`] for more info. +// +// ### Implementation Notes +// +// The paper uses mathematical notation, which priorities single-symbol names. For programmer ease, +// we map these to more descriptive names as follows: +// +// C => all_candidates +// N => all_voters +// (A, w) => (candidates, voters) +// +// Note that while the names don't explicitly say so, `candidates` are the winning candidates, and +// `voters` is the set of weighted edges from nominators to winning validators. pub fn pjr_check( - winners: Vec, - staked_assignments: Vec>, - supports: &SupportMap, + supports: &Supports, all_candidates: Vec, all_voters: Vec<(AccountId, VoteWeight, Vec)>, t: Threshold, ) -> bool { - // prepare data. + // First order of business: derive `(candidates, voters)` from `supports` let (candidates, voters) = prepare_pjr_input( - winners, - staked_assignments, supports, all_candidates, all_voters, @@ -180,7 +191,7 @@ pub fn pjr_check( /// The internal implementation of the PJR check after having the data converted. /// /// See [`pjr_check`] for more info. -pub fn pjr_check_core( +fn pjr_check_core( candidates: &[CandidatePtr], voters: &[Voter], t: Threshold, @@ -197,7 +208,7 @@ pub fn pjr_check_core( /// allowing the backing stake of any other elected candidate to fall below `t`. /// /// In essence, it is the sum(slack(n, t)) for all `n` who vote for `unelected`. -pub fn pre_score( +fn pre_score( unelected: CandidatePtr, voters: &[Voter], t: Threshold, @@ -221,7 +232,7 @@ pub fn pre_score( /// /// 1. If `c` exactly has `t` backing or less, then we don't generate any slack. /// 2. If `c` has more than `t`, then we reduce it to `t`. -pub fn slack(voter: &Voter, t: Threshold) -> ExtendedBalance { +fn slack(voter: &Voter, t: Threshold) -> ExtendedBalance { let budget = voter.budget; let leftover = voter.edges.iter().fold(Zero::zero(), |acc: ExtendedBalance, edge| { let candidate = edge.candidate.borrow(); @@ -288,24 +299,19 @@ mod tests { #[test] fn can_convert_data_from_external_api() { - let winners = vec![20u32, 40]; let all_candidates = vec![10, 20, 30, 40]; - let staked_assignments = vec![ - StakedAssignment { who: 1, distribution: vec![(20, 5), (40, 5)] }, - StakedAssignment { who: 2, distribution: vec![(20, 10), (40, 10)] }, - ]; let all_voters = vec![ (1, 10, vec![10, 20, 30, 40]), (2, 20, vec![10, 20, 30, 40]), (3, 30, vec![10, 30]), ]; - let mut supports = SupportMap::::new(); - supports.insert(20, Support { total: 15, voters: vec![(5, 1), (10, 2)]} ); - supports.insert(40, Support { total: 15, voters: vec![(5, 1), (10, 2)]} ); + // tuples in voters vector are (AccountId, Balance) + let supports: Supports = vec![ + (20, Support { total: 15, voters: vec![(1, 5), (2, 10)]}), + (40, Support { total: 15, voters: vec![(1, 5), (2, 10)]}), + ]; let (candidates, voters) = prepare_pjr_input( - winners, - staked_assignments, &supports, all_candidates, all_voters, From 81a788ace33794a4d02cffc0decca606e476ebdf Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 10:53:20 +0100 Subject: [PATCH 05/36] improve algorithmic complexity of `prepare_pjr_input` --- primitives/npos-elections/src/pjr.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index c3e4f4938ff04..2e89d58fff596 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -29,6 +29,7 @@ use crate::{ ExtendedBalance, IdentifierT, Support, + SupportMap, Supports, Voter, VoteWeight, @@ -86,14 +87,27 @@ fn prepare_pjr_input( } } + // Convert Suppports into a SupportMap + // + // As a flat list, we're limited to linear search. That gives the production of `candidates`, + // below, a complexity of `O(s*c)`, where `s == supports.len()` and `c == all_candidates.len()`. + // For large lists, that's pretty bad. + // + // A `SupportMap`, as a `BTreeMap`, has access timing of `O(lg n)`. This means that constructing + // the map and then indexing from it gives us timing of `O((s + c) * lg(s))`. If in the future + // we get access to a deterministic `HashMap`, we can further improve that to `O(s+c)`. + // + // However, it does mean allocating sufficient space to store all the data again. + let supports: SupportMap = supports.iter().cloned().collect(); + let candidates = all_candidates.into_iter().enumerate().map(|(i, c)| { candidates_index.insert(c.clone(), i); // set the backing value and elected flag if the candidate is among the winners. let who = c; - let maybe_support = supports.iter().find(|(winner, _support)| winner == &who); + let maybe_support = supports.get(&who); let elected = maybe_support.is_some(); - let backed_stake = maybe_support.map(|(_id, support)| support.total).unwrap_or_default(); + let backed_stake = maybe_support.map(|support| support.total).unwrap_or_default(); debug_assert!( !(elected ^ (backed_stake > 0)), From dd83c6681cb69b31b960d6d5ec56cb2fc1d97335 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 11:00:53 +0100 Subject: [PATCH 06/36] fix rustdoc warnings --- primitives/npos-elections/src/pjr.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 2e89d58fff596..8d8d89c4b1220 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -170,10 +170,8 @@ fn prepare_pjr_input( /// /// ### Interface /// -/// In addition to data that can be computed from the [`ElectionResult`] struct, a PJR check also +/// In addition to data that can be computed from the [`Supports`] struct, a PJR check also /// needs to inspect un-elected candidates and edges, thus `all_candidates` and `all_voters`. -/// -/// See [`prepare_pjr_input`] for more info. // // ### Implementation Notes // From 0e15efb00d08d024f35f53a3d966224d498a63a2 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 11:02:33 +0100 Subject: [PATCH 07/36] improve module docs --- primitives/npos-elections/src/pjr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 8d8d89c4b1220..f81d69dbce76e 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -17,7 +17,7 @@ //! Implements functions and interfaces to check solutions for being t-PJR. //! -//! PJR stands for proportional justified representation. PJR is a string absolute measure to make +//! PJR stands for proportional justified representation. PJR is an absolute measure to make //! sure an NPoS solution adheres to a minimum standard. //! //! See [`pjr_check`] which is the main entry point of the module. From 46a519d5aeb87632eb608b560b982d2286334ae3 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 11:03:14 +0100 Subject: [PATCH 08/36] typo --- primitives/npos-elections/src/pjr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index f81d69dbce76e..59cd34e3ecd02 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -157,7 +157,7 @@ fn prepare_pjr_input( /// The t-PJR property is defined in the paper "Validator Election in Nominated Proof-of-Stake", /// section 5, definition 1. /// -/// In plain language, the t-PJR condition is: ff there is a group of `N` voters +/// In plain language, the t-PJR condition is: if there is a group of `N` voters /// who have `r` common candidates and can afford to support each of them with backing stake `t` /// (i.e `sum(stake(v) for v in voters) == r * t`), then this committee needs to be represented by at /// least `r` elected candidates. From d63ed937c0f2819983d712a4388fc0ffdeb493a5 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 11:28:26 +0100 Subject: [PATCH 09/36] simplify debug assertion --- primitives/npos-elections/src/pjr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 59cd34e3ecd02..d89e64f05405d 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -110,7 +110,7 @@ fn prepare_pjr_input( let backed_stake = maybe_support.map(|support| support.total).unwrap_or_default(); debug_assert!( - !(elected ^ (backed_stake > 0)), + elected == (backed_stake > 0), "If a candidate is elected, then it must have a positive backing as well." ); From 158d01a97fa3610476bca2f9f9d5b7ba8f51bdb5 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 13:38:57 +0100 Subject: [PATCH 10/36] add test finding the phase-change threshold value for a constructed scenario --- primitives/npos-elections/src/pjr.rs | 69 ++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index d89e64f05405d..df7d9c6d09395 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -360,4 +360,73 @@ mod tests { assert!(!pjr_check_core(&candidates, &voters, 10)); assert!(!pjr_check_core(&candidates, &voters, 20)); } + + // This test ensures that the threshold property holds for us, but that's not it's real purpose. + // It was written to help develop an intuition about what the threshold value actually means + // in layman's terms. + // + // Its results tend to support the intuition that the threshold is the voting power at and below + // which a voter's preferences can simply be ignored. + #[test] + fn find_upper_bound_for_threshold() { + let all_candidates = vec![10, 20, 30, 40]; + let all_voters = vec![ + (1, 10, vec![10, 20, 30, 40]), + (2, 20, vec![10, 20, 30, 40]), + (3, 30, vec![10, 30]), + ]; + // tuples in voters vector are (AccountId, Balance) + let supports: Supports = vec![ + (20, Support { total: 15, voters: vec![(1, 5), (2, 10)]}), + (40, Support { total: 15, voters: vec![(1, 5), (2, 10)]}), + ]; + + let (candidates, voters) = prepare_pjr_input( + &supports, + all_candidates, + all_voters, + ); + + let mut threshold = 1; + let mut prev_threshold = 0; + + // find the binary range containing the threshold beyond which the PJR check succeeds + while !pjr_check_core(&candidates, &voters, threshold) { + prev_threshold = threshold; + threshold = threshold.checked_mul(2).expect("pjr check must fail before we run out of capacity in u128"); + } + + // now binary search within that range to find the phase threshold + let mut high_bound = threshold; + let mut low_bound = prev_threshold; + + while high_bound - low_bound > 1 { + // maintain the invariant that low_bound fails and high_bound passes + let test = low_bound + ((high_bound - low_bound) / 2); + if pjr_check_core(&candidates, &voters, test) { + high_bound = test; + } else { + low_bound = test; + } + } + + println!("highest failing check: {}", low_bound); + println!("lowest succeeding check: {}", high_bound); + + // for a value to be a threshold, it must be the boundary between two conditions + let mut unexpected_failures = Vec::new(); + let mut unexpected_successes = Vec::new(); + for t in 0..=low_bound { + if pjr_check_core(&candidates, &voters, t) { + unexpected_successes.push(t); + } + } + for t in high_bound..(high_bound*2) { + if !pjr_check_core(&candidates, &voters, t) { + unexpected_failures.push(t); + } + } + dbg!(&unexpected_successes, &unexpected_failures); + assert!(unexpected_failures.is_empty() && unexpected_successes.is_empty()); + } } From 902c2df470e637662e856ab9ad5e1ed3b2f15da4 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 13:52:23 +0100 Subject: [PATCH 11/36] add more threshold scenarios to disambiguate plausible interpretations --- primitives/npos-elections/src/pjr.rs | 63 ++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index df7d9c6d09395..d9674f6bf9940 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -361,14 +361,14 @@ mod tests { assert!(!pjr_check_core(&candidates, &voters, 20)); } - // This test ensures that the threshold property holds for us, but that's not it's real purpose. - // It was written to help develop an intuition about what the threshold value actually means + // These next tests ensure that the threshold phase change property holds for us, but that's not their real purpose. + // They were written to help develop an intuition about what the threshold value actually means // in layman's terms. // - // Its results tend to support the intuition that the threshold is the voting power at and below + // The results tend to support the intuition that the threshold is the voting power at and below // which a voter's preferences can simply be ignored. #[test] - fn find_upper_bound_for_threshold() { + fn find_upper_bound_for_threshold_scenario_1() { let all_candidates = vec![10, 20, 30, 40]; let all_voters = vec![ (1, 10, vec![10, 20, 30, 40]), @@ -387,6 +387,59 @@ mod tests { all_voters, ); + find_threshold_phase_change_for_scenario(candidates, voters); + } + + #[test] + fn find_upper_bound_for_threshold_scenario_2() { + let all_candidates = vec![10, 20, 30, 40]; + let all_voters = vec![ + (1, 10, vec![10, 20, 30, 40]), + (2, 20, vec![10, 20, 30, 40]), + (3, 25, vec![10, 30]), + ]; + // tuples in voters vector are (AccountId, Balance) + let supports: Supports = vec![ + (20, Support { total: 15, voters: vec![(1, 5), (2, 10)]}), + (40, Support { total: 15, voters: vec![(1, 5), (2, 10)]}), + ]; + + let (candidates, voters) = prepare_pjr_input( + &supports, + all_candidates, + all_voters, + ); + + find_threshold_phase_change_for_scenario(candidates, voters); + } + + #[test] + fn find_upper_bound_for_threshold_scenario_3() { + let all_candidates = vec![10, 20, 30, 40]; + let all_voters = vec![ + (1, 10, vec![10, 20, 30, 40]), + (2, 20, vec![10, 20, 30, 40]), + (3, 35, vec![10, 30]), + ]; + // tuples in voters vector are (AccountId, Balance) + let supports: Supports = vec![ + (20, Support { total: 15, voters: vec![(1, 5), (2, 10)]}), + (40, Support { total: 15, voters: vec![(1, 5), (2, 10)]}), + ]; + + let (candidates, voters) = prepare_pjr_input( + &supports, + all_candidates, + all_voters, + ); + + find_threshold_phase_change_for_scenario(candidates, voters); + } + + fn find_threshold_phase_change_for_scenario( + candidates: Vec>, + voters: Vec> + ) -> Threshold { let mut threshold = 1; let mut prev_threshold = 0; @@ -428,5 +481,7 @@ mod tests { } dbg!(&unexpected_successes, &unexpected_failures); assert!(unexpected_failures.is_empty() && unexpected_successes.is_empty()); + + high_bound } } From 4cff663b0280f9966b07a215a7158cff287ae936 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 16:48:38 +0100 Subject: [PATCH 12/36] add link to npos paper reference --- primitives/npos-elections/src/pjr.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index d9674f6bf9940..7fe4306cd3851 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -154,7 +154,7 @@ fn prepare_pjr_input( /// /// ### Semantics /// -/// The t-PJR property is defined in the paper "Validator Election in Nominated Proof-of-Stake", +/// The t-PJR property is defined in the paper ["Validator Election in Nominated Proof-of-Stake"][NPoS], /// section 5, definition 1. /// /// In plain language, the t-PJR condition is: if there is a group of `N` voters @@ -172,6 +172,8 @@ fn prepare_pjr_input( /// /// In addition to data that can be computed from the [`Supports`] struct, a PJR check also /// needs to inspect un-elected candidates and edges, thus `all_candidates` and `all_voters`. +/// +/// [NPoS]: https://arxiv.org/pdf/2004.12990v1.pdf // // ### Implementation Notes // From c47eba6b4eac933272fd05ea38fb910f40eb05b3 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Fri, 19 Feb 2021 16:49:47 +0100 Subject: [PATCH 13/36] docs: staked_assignment -> supports Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- primitives/npos-elections/src/pjr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 7fe4306cd3851..f9837f36130ae 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -62,7 +62,7 @@ type Threshold = ExtendedBalance; /// /// Note that the current function is rather unfortunately inefficient. The most significant /// slowdown is the fact that a typical solution that need to be checked for PJR only contains a -/// subset of the entire NPoS edge graph, encoded as `staked_assignment`. This only encodes the +/// subset of the entire NPoS edge graph, encoded as `supports`. This only encodes the /// edges that actually contribute to a winner's backing stake and ignores the rest to save space. /// To check PJR, we need the entire voter set, including those edges that point to non-winners. /// This could cause the caller runtime to have to read the entire list of voters, which is assumed From a12953495122b87dacbb5568f2734a037fb3ad0d Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Feb 2021 11:19:25 +0100 Subject: [PATCH 14/36] add utility method for generating npos inputs --- .../npos-elections/fuzzer/src/common.rs | 58 ++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/primitives/npos-elections/fuzzer/src/common.rs b/primitives/npos-elections/fuzzer/src/common.rs index 29f0247f84f31..162054f122340 100644 --- a/primitives/npos-elections/fuzzer/src/common.rs +++ b/primitives/npos-elections/fuzzer/src/common.rs @@ -21,7 +21,10 @@ #![allow(dead_code)] use sp_npos_elections::{ElectionResult, VoteWeight, phragmms, seq_phragmen}; -use sp_std::collections::btree_map::BTreeMap; +use sp_std::{ + collections::btree_map::BTreeMap, + ops::DerefMut, +}; use sp_runtime::Perbill; use rand::{self, Rng, RngCore}; @@ -44,6 +47,59 @@ pub enum ElectionType { pub type AccountId = u64; +/// Generate a set of inputs suitable for fuzzing an election algorithm +/// +/// Given parameters governing how many candidates and voters should exist, generates a voting +/// scenario suitable for fuzz-testing an election algorithm. +/// +/// Note that the actual counts of candidates may be lower than the requested value due to collisions. +/// The returned candidate list is sorted and deduplicated. This sorting property should not affect +/// the result of the calculation. +/// +/// Note that this does not generate balancing parameters. +pub fn generate_random_npos_inputs( + candidate_count: usize, + voter_count: usize, + mut rng: impl Rng, +) -> (usize, Vec, Vec<(AccountId, VoteWeight, Vec)>) { + // always generate a sensible number of candidates: elections are uninteresting if we desire + // 0 candidates, or a number of candidates >= the actual number of candidates present + let rounds = rng.gen_range(1, candidate_count); + + // candidates are easy: just a completely random set of IDs + let mut candidates: Vec = vec![0; candidate_count]; + rng.fill(candidates.deref_mut()); + candidates.sort(); + candidates.dedup(); + + let mut voters = Vec::with_capacity(voter_count); + + for _ in 0..voter_count { + let mut id = rng.gen(); + while candidates.binary_search(&id).is_ok() { + id = rng.gen(); + } + + 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()); + + // I believe, but am not 100% certain, that this produces a uniform random distribution of + // chosen candidates assuming a uniform RNG. + let mut chosen_candidates = candidates.clone(); + while chosen_candidates.len() > n_candidates_chosen { + chosen_candidates.swap_remove(rng.gen_range(0, chosen_candidates.len())); + } + + chosen_candidates.shrink_to_fit(); + + voters.push((id, vote_weight, chosen_candidates)); + } + + (rounds, candidates, voters) +} + pub fn generate_random_npos_result( voter_count: u64, target_count: u64, From 8049fcfa89bfb7375f467a3f22028fea94e7bfe8 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Feb 2021 14:38:12 +0100 Subject: [PATCH 15/36] add a fuzzer which asserts that all unbalanced seq_phragmen are PJR Note that this currently fails. I hope that this can be rectified by calculating the threshold instead of choosing some arbitrary number. --- Cargo.lock | 1 + primitives/npos-elections/fuzzer/Cargo.toml | 13 ++- .../npos-elections/fuzzer/src/common.rs | 101 +++++++++++------- .../npos-elections/fuzzer/src/phragmen_pjr.rs | 88 +++++++++++++++ 4 files changed, 158 insertions(+), 45 deletions(-) create mode 100644 primitives/npos-elections/fuzzer/src/phragmen_pjr.rs diff --git a/Cargo.lock b/Cargo.lock index 58c6baeb23712..2351c2e9c9e04 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8590,6 +8590,7 @@ dependencies = [ "honggfuzz", "parity-scale-codec", "rand 0.7.3", + "sp-arithmetic", "sp-npos-elections", "sp-runtime", "sp-std", diff --git a/primitives/npos-elections/fuzzer/Cargo.toml b/primitives/npos-elections/fuzzer/Cargo.toml index bac8a165f3947..bba5252d3b6fe 100644 --- a/primitives/npos-elections/fuzzer/Cargo.toml +++ b/primitives/npos-elections/fuzzer/Cargo.toml @@ -14,12 +14,13 @@ publish = false targets = ["x86_64-unknown-linux-gnu"] [dependencies] -sp-npos-elections = { version = "3.0.0", path = ".." } -sp-std = { version = "3.0.0", path = "../../std" } -sp-runtime = { version = "3.0.0", path = "../../runtime" } +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"] } -codec = { package = "parity-scale-codec", version = "2.0.0", default-features = false, features = ["derive"] } +sp-arithmetic = { version = "3.0.0", path = "../../arithmetic" } +sp-npos-elections = { version = "3.0.0", path = ".." } +sp-runtime = { version = "3.0.0", path = "../../runtime" } +sp-std = { version = "3.0.0", path = "../../std" } [[bin]] name = "reduce" @@ -36,3 +37,7 @@ path = "src/phragmms_balancing.rs" [[bin]] name = "compact" path = "src/compact.rs" + +[[bin]] +name = "phragmen_pjr" +path = "src/phragmen_pjr.rs" diff --git a/primitives/npos-elections/fuzzer/src/common.rs b/primitives/npos-elections/fuzzer/src/common.rs index 162054f122340..85b31fa1fc281 100644 --- a/primitives/npos-elections/fuzzer/src/common.rs +++ b/primitives/npos-elections/fuzzer/src/common.rs @@ -20,13 +20,10 @@ // Each function will be used based on which fuzzer binary is being used. #![allow(dead_code)] -use sp_npos_elections::{ElectionResult, VoteWeight, phragmms, seq_phragmen}; -use sp_std::{ - collections::btree_map::BTreeMap, - ops::DerefMut, -}; -use sp_runtime::Perbill; use rand::{self, Rng, RngCore}; +use sp_npos_elections::{phragmms, seq_phragmen, ElectionResult, VoteWeight}; +use sp_runtime::Perbill; +use std::collections::{BTreeMap, HashSet}; /// converts x into the range [a, b] in a pseudo-fair way. pub fn to_range(x: usize, a: usize, b: usize) -> usize { @@ -42,7 +39,7 @@ pub fn to_range(x: usize, a: usize, b: usize) -> usize { pub enum ElectionType { Phragmen(Option<(usize, u128)>), - Phragmms(Option<(usize, u128)>) + Phragmms(Option<(usize, u128)>), } pub type AccountId = u64; @@ -52,31 +49,48 @@ pub type AccountId = u64; /// Given parameters governing how many candidates and voters should exist, generates a voting /// scenario suitable for fuzz-testing an election algorithm. /// -/// Note that the actual counts of candidates may be lower than the requested value due to collisions. -/// The returned candidate list is sorted and deduplicated. This sorting property should not affect -/// the result of the calculation. +/// The returned candidate list is sorted. This sorting property should not affect the result of the +/// calculation. +/// +/// The returned voters list is sorted. This enables binary searching for a particular voter by +/// account id. This sorting property should not affect the results of the calculation. /// /// Note that this does not generate balancing parameters. pub fn generate_random_npos_inputs( candidate_count: usize, voter_count: usize, mut rng: impl Rng, -) -> (usize, Vec, Vec<(AccountId, VoteWeight, Vec)>) { - // always generate a sensible number of candidates: elections are uninteresting if we desire - // 0 candidates, or a number of candidates >= the actual number of candidates present +) -> ( + usize, + Vec, + Vec<(AccountId, VoteWeight, Vec)>, +) { + // cache for fast generation of unique candidate and voter ids + let mut used_ids = HashSet::with_capacity(candidate_count + voter_count); + + // always generate a sensible desired number of candidates: elections are uninteresting if we + // desire 0 candidates, or a number of candidates >= the actual number of candidates present let rounds = rng.gen_range(1, candidate_count); // candidates are easy: just a completely random set of IDs - let mut candidates: Vec = vec![0; candidate_count]; - rng.fill(candidates.deref_mut()); - candidates.sort(); + let mut candidates: Vec = Vec::with_capacity(candidate_count); + for _ in 0..candidate_count { + let mut id = rng.gen(); + // insert returns `false` when the value was already present + while !used_ids.insert(id) { + id = rng.gen(); + } + candidates.push(id); + } + candidates.sort_unstable(); candidates.dedup(); + debug_assert_eq!(candidates.len(), candidate_count); let mut voters = Vec::with_capacity(voter_count); - for _ in 0..voter_count { let mut id = rng.gen(); - while candidates.binary_search(&id).is_ok() { + // insert returns `false` when the value was already present + while !used_ids.insert(id) { id = rng.gen(); } @@ -97,6 +111,10 @@ pub fn generate_random_npos_inputs( voters.push((id, vote_weight, chosen_candidates)); } + voters.sort_unstable(); + voters.dedup_by_key(|(id, _weight, _chosen_candidates)| *id); + debug_assert_eq!(voters.len(), voter_count); + (rounds, candidates, voters) } @@ -127,19 +145,20 @@ pub fn generate_random_npos_result( }); let mut voters = Vec::with_capacity(voter_count as usize); - (prefix ..= (prefix + voter_count)).for_each(|acc| { + (prefix..=(prefix + voter_count)).for_each(|acc| { let edge_per_this_voter = rng.gen_range(1, candidates.len()); // all possible targets let mut all_targets = candidates.clone(); // we remove and pop into `targets` `edge_per_this_voter` times. - let targets = (0..edge_per_this_voter).map(|_| { - let upper = all_targets.len() - 1; - let idx = rng.gen_range(0, upper); - all_targets.remove(idx) - }) - .collect::>(); - - let stake_var = rng.gen_range(ed, 100 * ed) ; + let targets = (0..edge_per_this_voter) + .map(|_| { + let upper = all_targets.len() - 1; + let idx = rng.gen_range(0, upper); + all_targets.remove(idx) + }) + .collect::>(); + + let stake_var = rng.gen_range(ed, 100 * ed); let stake = base_stake + stake_var; stake_of.insert(acc, stake); voters.push((acc, stake, targets)); @@ -147,20 +166,20 @@ pub fn generate_random_npos_result( ( match election_type { - ElectionType::Phragmen(conf) => - seq_phragmen::( - to_elect, - candidates.clone(), - voters.clone(), - conf, - ).unwrap(), - ElectionType::Phragmms(conf) => - phragmms::( - to_elect, - candidates.clone(), - voters.clone(), - conf, - ).unwrap(), + ElectionType::Phragmen(conf) => seq_phragmen::( + to_elect, + candidates.clone(), + voters.clone(), + conf, + ) + .unwrap(), + ElectionType::Phragmms(conf) => phragmms::( + to_elect, + candidates.clone(), + voters.clone(), + conf, + ) + .unwrap(), }, candidates, voters, diff --git a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs new file mode 100644 index 0000000000000..8bc47dce6f319 --- /dev/null +++ b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs @@ -0,0 +1,88 @@ +// 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 running unbalanced sequential phragmen always produces a result +//! which satisfies our PJR checker. +//! +//! # Running +//! +//! Run with `cargo hfuzz run phragmen_pjr`. +//! +//! # Debugging a panic +//! +//! Once a panic is found, it can be debugged with +//! `cargo hfuzz run-debug phragmen_pjr hfuzz_workspace/phragmen_pjr/*.fuzz`. + +use honggfuzz::fuzz; + +mod common; +use common::{generate_random_npos_inputs, to_range}; +use rand::{self, SeedableRng}; +use sp_npos_elections::{ + pjr_check, seq_phragmen, to_supports, ElectionResult, ExtendedBalance, StakedAssignment, + Supports, +}; + +type AccountId = u64; +type PerThing = sp_arithmetic::PerU16; + +/// TODO: This value is currently _entirely arbitrary_. +const PJR_THRESHOLD: ExtendedBalance = 1_000_000_000_000; + +fn main() { + loop { + fuzz!(|data: (usize, usize, u64)| { + let (mut candidate_count, mut voter_count, seed) = data; + let rng = rand::rngs::SmallRng::seed_from_u64(seed); + candidate_count = to_range(candidate_count, 100, 1000); + voter_count = to_range(voter_count, 100, 2000); + + let (rounds, candidates, voters) = + generate_random_npos_inputs(candidate_count, voter_count, rng); + + // Run the NPoS validator selection algorithm + let ElectionResult { + winners, + assignments, + } = seq_phragmen::(rounds, candidates.clone(), voters.clone(), None) + .expect("seq_phragmen must succeed"); + + // pjr_check only cares about the identity of the winner, not its balance + let winners: Vec<_> = winners.into_iter().map(|(id, _stake)| id).collect(); + + // convert assignments into staked assignments + let assignments: Vec> = assignments + .into_iter() + .map(|assignment| { + let voter_idx = voters + .binary_search_by_key(&assignment.who, |(id, _weight, _assignments)| *id) + .expect("voter must be present in voters list"); + let weight: ExtendedBalance = voters[voter_idx].1.into(); + assignment.into_staked(weight) + }) + .collect(); + + let supports: Supports = to_supports(&winners, &assignments) + .expect("election result must be structurally valid"); + + assert!( + pjr_check(&supports, candidates, voters, PJR_THRESHOLD), + "unbalanced sequential phragmen must satisfy PJR", + ); + }); + } +} From 2451a864db12ca5c342affac80e76e53977ef2bb Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Feb 2021 16:36:17 +0100 Subject: [PATCH 16/36] assert in all cases, not just debug --- primitives/npos-elections/fuzzer/src/common.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/primitives/npos-elections/fuzzer/src/common.rs b/primitives/npos-elections/fuzzer/src/common.rs index 85b31fa1fc281..eeefc2e390e8c 100644 --- a/primitives/npos-elections/fuzzer/src/common.rs +++ b/primitives/npos-elections/fuzzer/src/common.rs @@ -84,7 +84,7 @@ pub fn generate_random_npos_inputs( } candidates.sort_unstable(); candidates.dedup(); - debug_assert_eq!(candidates.len(), candidate_count); + assert_eq!(candidates.len(), candidate_count); let mut voters = Vec::with_capacity(voter_count); for _ in 0..voter_count { @@ -113,7 +113,7 @@ pub fn generate_random_npos_inputs( voters.sort_unstable(); voters.dedup_by_key(|(id, _weight, _chosen_candidates)| *id); - debug_assert_eq!(voters.len(), voter_count); + assert_eq!(voters.len(), voter_count); (rounds, candidates, voters) } From 1d4e2dba492f94e829ce39385de1fc5aaed4fabd Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Feb 2021 16:48:03 +0100 Subject: [PATCH 17/36] leverage a native solution to choose candidates --- primitives/npos-elections/fuzzer/src/common.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/primitives/npos-elections/fuzzer/src/common.rs b/primitives/npos-elections/fuzzer/src/common.rs index eeefc2e390e8c..b7c6aa4e7d6f8 100644 --- a/primitives/npos-elections/fuzzer/src/common.rs +++ b/primitives/npos-elections/fuzzer/src/common.rs @@ -20,7 +20,7 @@ // Each function will be used based on which fuzzer binary is being used. #![allow(dead_code)] -use rand::{self, Rng, RngCore}; +use rand::{self, Rng, RngCore, seq::SliceRandom}; use sp_npos_elections::{phragmms, seq_phragmen, ElectionResult, VoteWeight}; use sp_runtime::Perbill; use std::collections::{BTreeMap, HashSet}; @@ -99,15 +99,8 @@ pub fn generate_random_npos_inputs( // 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()); - // I believe, but am not 100% certain, that this produces a uniform random distribution of - // chosen candidates assuming a uniform RNG. - let mut chosen_candidates = candidates.clone(); - while chosen_candidates.len() > n_candidates_chosen { - chosen_candidates.swap_remove(rng.gen_range(0, chosen_candidates.len())); - } - - chosen_candidates.shrink_to_fit(); - + 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)); } From fc64ff572f6f94d3edca5c9181b80b0e30ce8152 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Feb 2021 17:01:00 +0100 Subject: [PATCH 18/36] use existing helper methods --- .../npos-elections/fuzzer/src/phragmen_pjr.rs | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs index 8bc47dce6f319..c495df216f4d9 100644 --- a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs +++ b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs @@ -33,8 +33,8 @@ mod common; use common::{generate_random_npos_inputs, to_range}; use rand::{self, SeedableRng}; use sp_npos_elections::{ - pjr_check, seq_phragmen, to_supports, ElectionResult, ExtendedBalance, StakedAssignment, - Supports, + assignment_ratio_to_staked, pjr_check, seq_phragmen, to_supports, to_without_backing, + ElectionResult, ExtendedBalance, Supports, }; type AccountId = u64; @@ -54,7 +54,7 @@ fn main() { let (rounds, candidates, voters) = generate_random_npos_inputs(candidate_count, voter_count, rng); - // Run the NPoS validator selection algorithm + // Run seq-phragmen let ElectionResult { winners, assignments, @@ -62,19 +62,15 @@ fn main() { .expect("seq_phragmen must succeed"); // pjr_check only cares about the identity of the winner, not its balance - let winners: Vec<_> = winners.into_iter().map(|(id, _stake)| id).collect(); + let winners = to_without_backing(winners); // convert assignments into staked assignments - let assignments: Vec> = assignments - .into_iter() - .map(|assignment| { - let voter_idx = voters - .binary_search_by_key(&assignment.who, |(id, _weight, _assignments)| *id) - .expect("voter must be present in voters list"); - let weight: ExtendedBalance = voters[voter_idx].1.into(); - assignment.into_staked(weight) - }) - .collect(); + let assignments = assignment_ratio_to_staked(assignments, |who| { + let voter_idx = voters + .binary_search_by_key(who, |(id, _weight, _assignments)| *id) + .expect("voter must be present in voters list"); + voters[voter_idx].1 + }); let supports: Supports = to_supports(&winners, &assignments) .expect("election result must be structurally valid"); From f2f11bbb0de37388990b2e1e6fc1580f5d407383 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 22 Feb 2021 17:07:56 +0100 Subject: [PATCH 19/36] add pjr-check and incorporate into the fuzzer We should probably have one of the W3F people look at this to ensure we're not misconstruing any definitions, but this seems like a fairly straightforward implementation. --- .../npos-elections/fuzzer/src/phragmen_pjr.rs | 2 +- primitives/npos-elections/src/pjr.rs | 27 ++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs index c495df216f4d9..849f1c87f381f 100644 --- a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs +++ b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs @@ -76,7 +76,7 @@ fn main() { .expect("election result must be structurally valid"); assert!( - pjr_check(&supports, candidates, voters, PJR_THRESHOLD), + pjr_check(&supports, candidates, voters), "unbalanced sequential phragmen must satisfy PJR", ); }); diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index f9837f36130ae..159cc00b6d13d 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -165,8 +165,11 @@ fn prepare_pjr_input( /// Section 5 of the NPoS paper shows that this property is equal to: For a feasible solution, if /// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. /// -/// The text notes that we can verify this condition by running Algorithm 5: MaxPrescore and validating -/// that MaxPrescore(A, w, t) < t). +/// The text notes the following: +/// +/// - we can verify this condition by running Algorithm 5: MaxPrescore and validating +/// that MaxPrescore(A, w, t) < t). +/// - the property gets stronger as the threshold decreases. /// /// ### Interface /// @@ -186,7 +189,7 @@ fn prepare_pjr_input( // // Note that while the names don't explicitly say so, `candidates` are the winning candidates, and // `voters` is the set of weighted edges from nominators to winning validators. -pub fn pjr_check( +pub fn t_pjr_check( supports: &Supports, all_candidates: Vec, all_voters: Vec<(AccountId, VoteWeight, Vec)>, @@ -266,6 +269,24 @@ fn slack(voter: &Voter, t: Threshold) -> Exte budget.saturating_sub(leftover) } +/// Check a solution to be PJR. +/// +/// The PJR property is true if `t-PJR` is true when `t == sum(stake) / committee_size`. +pub fn pjr_check( + supports: &Supports, + all_candidates: Vec, + all_voters: Vec<(AccountId, VoteWeight, Vec)>, +) -> bool { + use sp_arithmetic::traits::One; + + let t: ExtendedBalance = all_voters + .iter() + .map(|(_id, weight, _allocation)| weight) + .sum() + / supports.len() as ExtendedBalance; + t_pjr_check(supports, all_candidates, all_voters, t) +} + #[cfg(test)] mod tests { use super::*; From 8d63aa4dd9c29653e06f7a389a051016c08583e4 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 23 Feb 2021 10:12:15 +0100 Subject: [PATCH 20/36] fix compilation errors --- primitives/npos-elections/fuzzer/src/phragmen_pjr.rs | 5 +---- primitives/npos-elections/src/pjr.rs | 8 +++----- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs index 849f1c87f381f..ddebe36e0a28e 100644 --- a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs +++ b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs @@ -34,15 +34,12 @@ use common::{generate_random_npos_inputs, to_range}; use rand::{self, SeedableRng}; use sp_npos_elections::{ assignment_ratio_to_staked, pjr_check, seq_phragmen, to_supports, to_without_backing, - ElectionResult, ExtendedBalance, Supports, + ElectionResult, Supports, }; type AccountId = u64; type PerThing = sp_arithmetic::PerU16; -/// TODO: This value is currently _entirely arbitrary_. -const PJR_THRESHOLD: ExtendedBalance = 1_000_000_000_000; - fn main() { loop { fuzz!(|data: (usize, usize, u64)| { diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 159cc00b6d13d..836c9700c16f1 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -277,12 +277,10 @@ pub fn pjr_check( all_candidates: Vec, all_voters: Vec<(AccountId, VoteWeight, Vec)>, ) -> bool { - use sp_arithmetic::traits::One; - - let t: ExtendedBalance = all_voters + let t = all_voters .iter() - .map(|(_id, weight, _allocation)| weight) - .sum() + .map(|(_id, weight, _allocation)| *weight as ExtendedBalance) + .sum::() / supports.len() as ExtendedBalance; t_pjr_check(supports, all_candidates, all_voters, t) } From ba365292f2b0670a5b2ed1ee9195d0c5e8de9ec9 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 23 Feb 2021 11:24:29 +0100 Subject: [PATCH 21/36] Enable manually setting iteration parameters in single run. This gives us the ability to reproducably extract cases where honggfuzz has discovered a panic. For example: $ cargo run --release --bin phragmen_pjr -- --candidates 569 --voters 100 Tue 23 Feb 2021 11:23:39 AM CET Compiling bitflags v1.2.1 Compiling unicode-width v0.1.8 Compiling unicode-segmentation v1.7.1 Compiling ansi_term v0.11.0 Compiling strsim v0.8.0 Compiling vec_map v0.8.2 Compiling proc-macro-error-attr v1.0.4 Compiling proc-macro-error v1.0.4 Compiling textwrap v0.11.0 Compiling atty v0.2.14 Compiling heck v0.3.2 Compiling clap v2.33.3 Compiling structopt-derive v0.4.14 Compiling structopt v0.3.21 Compiling sp-npos-elections-fuzzer v2.0.0-alpha.5 (/home/coriolinus/Documents/Projects/paritytech/substrate/primitives/npos-elections/fuzzer) Finished release [optimized] target(s) in 6.15s Running `/home/coriolinus/Documents/Projects/paritytech/substrate/target/release/phragmen_pjr -c 569 -v 100` thread 'main' panicked at 'unbalanced sequential phragmen must satisfy PJR', primitives/npos-elections/fuzzer/src/phragmen_pjr.rs:133:5 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace This is still not adequate proof that seq_phragmen is broken; it could very well be that our PJR checker is doing the wrong thing, or we've somehow missed a parameter of interest. Still, it's concerning. --- Cargo.lock | 1 + primitives/npos-elections/fuzzer/Cargo.toml | 1 + .../npos-elections/fuzzer/src/phragmen_pjr.rs | 130 +++++++++++++----- 3 files changed, 95 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2351c2e9c9e04..59528ca761eb1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8594,6 +8594,7 @@ dependencies = [ "sp-npos-elections", "sp-runtime", "sp-std", + "structopt", ] [[package]] diff --git a/primitives/npos-elections/fuzzer/Cargo.toml b/primitives/npos-elections/fuzzer/Cargo.toml index bba5252d3b6fe..3154a7861d306 100644 --- a/primitives/npos-elections/fuzzer/Cargo.toml +++ b/primitives/npos-elections/fuzzer/Cargo.toml @@ -21,6 +21,7 @@ sp-arithmetic = { version = "3.0.0", path = "../../arithmetic" } sp-npos-elections = { version = "3.0.0", path = ".." } sp-runtime = { version = "3.0.0", path = "../../runtime" } sp-std = { version = "3.0.0", path = "../../std" } +structopt = "0.3.21" [[bin]] name = "reduce" diff --git a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs index ddebe36e0a28e..d0d400e0783bf 100644 --- a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs +++ b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs @@ -18,17 +18,35 @@ //! Fuzzing which ensures that running unbalanced sequential phragmen always produces a result //! which satisfies our PJR checker. //! -//! # Running +//! ## Running a single iteration //! -//! Run with `cargo hfuzz run phragmen_pjr`. +//! Honggfuzz shuts down each individual loop iteration after a configurable time limit. +//! It can be helpful to run a single iteration on your hardware to help benchmark how long that time +//! limit should reasonably be. Simply run the program without the `fuzzing` configuration to run a +//! single iteration: `cargo run --bin phragmen_pjr`. //! -//! # Debugging a panic +//! On a relatively modern machine (late 2019 era), a single iteration takes ~30 seconds with maximum +//! parameters. +//! +//! ## Running +//! +//! Run with `HFUZZ_RUN_ARGS="-t 60" cargo hfuzz run phragmen_pjr`. +//! +//! Note the environment variable: by default, `cargo hfuzz` shuts down each iteration after 1 second +//! of runtime. We significantly increase that to ensure that the fuzzing gets a chance to complete. +//! +//! ## Debugging a panic //! //! Once a panic is found, it can be debugged with -//! `cargo hfuzz run-debug phragmen_pjr hfuzz_workspace/phragmen_pjr/*.fuzz`. +//! `HFUZZ_RUN_ARGS="-t 60" cargo hfuzz run-debug phragmen_pjr hfuzz_workspace/phragmen_pjr/*.fuzz`. +//! +#[cfg(fuzzing)] use honggfuzz::fuzz; +#[cfg(not(fuzzing))] +use structopt::StructOpt; + mod common; use common::{generate_random_npos_inputs, to_range}; use rand::{self, SeedableRng}; @@ -40,42 +58,80 @@ use sp_npos_elections::{ type AccountId = u64; type PerThing = sp_arithmetic::PerU16; +const MIN_CANDIDATES: usize = 100; +const MAX_CANDIDATES: usize = 1000; +const MIN_VOTERS: usize = 100; +const MAX_VOTERS: usize = 2000; + +#[cfg(fuzzing)] fn main() { loop { fuzz!(|data: (usize, usize, u64)| { - let (mut candidate_count, mut voter_count, seed) = data; - let rng = rand::rngs::SmallRng::seed_from_u64(seed); - candidate_count = to_range(candidate_count, 100, 1000); - voter_count = to_range(voter_count, 100, 2000); - - let (rounds, candidates, voters) = - generate_random_npos_inputs(candidate_count, voter_count, rng); - - // Run seq-phragmen - let ElectionResult { - winners, - assignments, - } = seq_phragmen::(rounds, candidates.clone(), voters.clone(), None) - .expect("seq_phragmen must succeed"); - - // pjr_check only cares about the identity of the winner, not its balance - let winners = to_without_backing(winners); - - // convert assignments into staked assignments - let assignments = assignment_ratio_to_staked(assignments, |who| { - let voter_idx = voters - .binary_search_by_key(who, |(id, _weight, _assignments)| *id) - .expect("voter must be present in voters list"); - voters[voter_idx].1 - }); - - let supports: Supports = to_supports(&winners, &assignments) - .expect("election result must be structurally valid"); - - assert!( - pjr_check(&supports, candidates, voters), - "unbalanced sequential phragmen must satisfy PJR", - ); + 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 (rounds, candidates, voters) = + generate_random_npos_inputs(candidate_count, voter_count, rng); + + // Run seq-phragmen + let ElectionResult { + winners, + assignments, + } = seq_phragmen::(rounds, candidates.clone(), voters.clone(), None) + .expect("seq_phragmen must succeed"); + + // pjr_check only cares about the identity of the winner, not its balance + let winners = to_without_backing(winners); + + // convert assignments into staked assignments + let assignments = assignment_ratio_to_staked(assignments, |who| { + let voter_idx = voters + .binary_search_by_key(who, |(id, _weight, _assignments)| *id) + .expect("voter must be present in voters list"); + voters[voter_idx].1 + }); + + let supports: Supports = to_supports(&winners, &assignments) + .expect("election result must be structurally valid"); + + assert!( + pjr_check(&supports, candidates, voters), + "unbalanced sequential phragmen must satisfy PJR", + ); +} From 32da70df56e892e7e2f3aac8de727f3597f780ad Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 25 Feb 2021 11:04:49 +0100 Subject: [PATCH 22/36] update comment verbiage for accuracy --- primitives/npos-elections/src/pjr.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 836c9700c16f1..845a85b7523f9 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -162,8 +162,10 @@ fn prepare_pjr_input( /// (i.e `sum(stake(v) for v in voters) == r * t`), then this committee needs to be represented by at /// least `r` elected candidates. /// -/// Section 5 of the NPoS paper shows that this property is equal to: For a feasible solution, if -/// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. +/// Section 5 of the NPoS paper shows that this property can be tested by: for a feasible solution, if +/// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. However, +/// this test is incomplete: while every solution which passes this test satisfies t-PJR, not every +/// solution which is t-PJR passes this test. We therefore look to a more accurate test. /// /// The text notes the following: /// From 6419d170c83cb795c83c6162f9835746ecb60202 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 25 Feb 2021 11:25:25 +0100 Subject: [PATCH 23/36] it is valid in PJR for an elected candidate to have 0 support --- primitives/npos-elections/src/pjr.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 845a85b7523f9..ea4d39d771ba5 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -109,11 +109,6 @@ fn prepare_pjr_input( let elected = maybe_support.is_some(); let backed_stake = maybe_support.map(|support| support.total).unwrap_or_default(); - debug_assert!( - elected == (backed_stake > 0), - "If a candidate is elected, then it must have a positive backing as well." - ); - Candidate { who, elected, backed_stake, ..Default::default() }.to_ptr() }).collect::>(); From e582b1449088c29deabeb575059454b1fdbdbe0d Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 1 Mar 2021 12:24:48 +0100 Subject: [PATCH 24/36] Fix phragmen_pjr fuzzer It turns out that the fundamental problem causing previous implementations of the fuzzer to fail wasn't in `seq_phragmen` _or_ in `pjr_check`: it was in the rounding errors introduced in the various conversions between the internal data representation and the external one. Fixing the fuzzer is then simply an issue of using the internal representation and staying in that representation. However, that leaves the issue that `seq_phragmen` occasionally produces an output which is technically not PJR due to rounding errors. In the future we will need to add some kind of "close-enough" threshold. However, that is explicitly out of scope of this PR. --- .../npos-elections/fuzzer/src/common.rs | 5 +- .../npos-elections/fuzzer/src/phragmen_pjr.rs | 49 ++++++------------- primitives/npos-elections/src/lib.rs | 8 ++- primitives/npos-elections/src/phragmen.rs | 3 ++ primitives/npos-elections/src/pjr.rs | 36 ++++++++++---- 5 files changed, 56 insertions(+), 45 deletions(-) diff --git a/primitives/npos-elections/fuzzer/src/common.rs b/primitives/npos-elections/fuzzer/src/common.rs index b7c6aa4e7d6f8..fe237c930de17 100644 --- a/primitives/npos-elections/fuzzer/src/common.rs +++ b/primitives/npos-elections/fuzzer/src/common.rs @@ -20,7 +20,7 @@ // Each function will be used based on which fuzzer binary is being used. #![allow(dead_code)] -use rand::{self, Rng, RngCore, seq::SliceRandom}; +use rand::{self, seq::SliceRandom, Rng, RngCore}; use sp_npos_elections::{phragmms, seq_phragmen, ElectionResult, VoteWeight}; use sp_runtime::Perbill; use std::collections::{BTreeMap, HashSet}; @@ -55,6 +55,8 @@ pub type AccountId = u64; /// The returned voters list is sorted. This enables binary searching for a particular voter by /// account id. This sorting property should not affect the results of the calculation. /// +/// Each voter's selection of candidates to vote for is sorted. +/// /// Note that this does not generate balancing parameters. pub fn generate_random_npos_inputs( candidate_count: usize, @@ -101,6 +103,7 @@ pub fn generate_random_npos_inputs( let mut chosen_candidates = Vec::with_capacity(n_candidates_chosen); chosen_candidates.extend(candidates.choose_multiple(&mut rng, n_candidates_chosen)); + chosen_candidates.sort(); voters.push((id, vote_weight, chosen_candidates)); } diff --git a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs index d0d400e0783bf..cb5b926bc82b3 100644 --- a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs +++ b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs @@ -25,20 +25,18 @@ //! limit should reasonably be. Simply run the program without the `fuzzing` configuration to run a //! single iteration: `cargo run --bin phragmen_pjr`. //! -//! On a relatively modern machine (late 2019 era), a single iteration takes ~30 seconds with maximum -//! parameters. -//! //! ## Running //! -//! Run with `HFUZZ_RUN_ARGS="-t 60" cargo hfuzz run phragmen_pjr`. +//! Run with `HFUZZ_RUN_ARGS="-t 10" cargo hfuzz run phragmen_pjr`. //! //! Note the environment variable: by default, `cargo hfuzz` shuts down each iteration after 1 second //! of runtime. We significantly increase that to ensure that the fuzzing gets a chance to complete. +//! Running a single iteration can help determine an appropriate value for this parameter. //! //! ## Debugging a panic //! //! Once a panic is found, it can be debugged with -//! `HFUZZ_RUN_ARGS="-t 60" cargo hfuzz run-debug phragmen_pjr hfuzz_workspace/phragmen_pjr/*.fuzz`. +//! `HFUZZ_RUN_ARGS="-t 10" cargo hfuzz run-debug phragmen_pjr hfuzz_workspace/phragmen_pjr/*.fuzz`. //! #[cfg(fuzzing)] @@ -50,18 +48,14 @@ use structopt::StructOpt; mod common; use common::{generate_random_npos_inputs, to_range}; use rand::{self, SeedableRng}; -use sp_npos_elections::{ - assignment_ratio_to_staked, pjr_check, seq_phragmen, to_supports, to_without_backing, - ElectionResult, Supports, -}; +use sp_npos_elections::{pjr_check_core, seq_phragmen_core, setup_inputs, standard_threshold}; type AccountId = u64; -type PerThing = sp_arithmetic::PerU16; -const MIN_CANDIDATES: usize = 100; -const MAX_CANDIDATES: usize = 1000; -const MIN_VOTERS: usize = 100; -const MAX_VOTERS: usize = 2000; +const MIN_CANDIDATES: usize = 5; +const MAX_CANDIDATES: usize = 200; +const MIN_VOTERS: usize = 5; +const MAX_VOTERS: usize = 500; #[cfg(fuzzing)] fn main() { @@ -95,8 +89,8 @@ fn main() { // 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.candidates.unwrap_or(MAX_CANDIDATES - 1), + opt.voters.unwrap_or(MAX_VOTERS - 1), opt.seed.unwrap_or_default(), ); } @@ -109,29 +103,16 @@ fn iteration(mut candidate_count: usize, mut voter_count: usize, seed: u64) { let (rounds, candidates, voters) = generate_random_npos_inputs(candidate_count, voter_count, rng); + let (candidates, voters) = setup_inputs(candidates, voters); + // Run seq-phragmen - let ElectionResult { - winners, - assignments, - } = seq_phragmen::(rounds, candidates.clone(), voters.clone(), None) + let (candidates, voters) = seq_phragmen_core::(rounds, candidates, voters) .expect("seq_phragmen must succeed"); - // pjr_check only cares about the identity of the winner, not its balance - let winners = to_without_backing(winners); - - // convert assignments into staked assignments - let assignments = assignment_ratio_to_staked(assignments, |who| { - let voter_idx = voters - .binary_search_by_key(who, |(id, _weight, _assignments)| *id) - .expect("voter must be present in voters list"); - voters[voter_idx].1 - }); - - let supports: Supports = to_supports(&winners, &assignments) - .expect("election result must be structurally valid"); + let threshold = standard_threshold(rounds, voters.iter().map(|voter| voter.budget())); assert!( - pjr_check(&supports, candidates, voters), + pjr_check_core(&candidates, &voters, threshold), "unbalanced sequential phragmen must satisfy PJR", ); } diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index b17265ca7358c..da175180ed30e 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -423,6 +423,12 @@ impl Voter { } }) } + + /// This voter's budget + #[inline] + pub fn budget(&self) -> ExtendedBalance { + self.budget + } } /// Final result of the election. @@ -756,7 +762,7 @@ pub fn is_score_better(this: ElectionScore, that: ElectionScore, ep /// This will perform some cleanup that are most often important: /// - It drops any votes that are pointing to non-candidates. /// - It drops duplicate targets within a voter. -pub(crate) fn setup_inputs( +pub fn setup_inputs( initial_candidates: Vec, initial_voters: Vec<(AccountId, VoteWeight, Vec)>, ) -> (Vec>, Vec>) { diff --git a/primitives/npos-elections/src/phragmen.rs b/primitives/npos-elections/src/phragmen.rs index dad65666738c7..a507dd28666ae 100644 --- a/primitives/npos-elections/src/phragmen.rs +++ b/primitives/npos-elections/src/phragmen.rs @@ -63,6 +63,9 @@ const DEN: ExtendedBalance = ExtendedBalance::max_value(); /// `expect` this to return `Ok`. /// /// This can only fail if the normalization fails. +/// +/// Note that rounding errors can potentially cause the output of this function to fail a PJR check. +/// The underlying algorithm is sound, but the conversions between numeric types can be lossy. pub fn seq_phragmen( rounds: usize, initial_candidates: Vec, diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index ea4d39d771ba5..896da772711f3 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -204,16 +204,16 @@ pub fn t_pjr_check( /// The internal implementation of the PJR check after having the data converted. /// -/// See [`pjr_check`] for more info. -fn pjr_check_core( +/// [`pjr_check`] or [`t_pjr_check`] are typically easier to work with. +pub fn pjr_check_core( candidates: &[CandidatePtr], voters: &[Voter], t: Threshold, ) -> bool { let unelected = candidates.iter().filter(|c| !c.borrow().elected); - let maybe_max_pre_score = unelected.map(|c| pre_score(Rc::clone(c), voters, t)).max(); + let maybe_max_pre_score = unelected.map(|c| (pre_score(Rc::clone(c), voters, t), c.borrow().who.clone())).max(); // if unelected is empty then the solution is indeed PJR. - maybe_max_pre_score.map_or(true, |max_pre_score| max_pre_score < t) + maybe_max_pre_score.map_or(true, |(max_pre_score, _)| max_pre_score < t) } /// The pre-score of an unelected candidate. @@ -266,6 +266,28 @@ fn slack(voter: &Voter, t: Threshold) -> Exte budget.saturating_sub(leftover) } +/// Compute the threshold corresponding to the standard PJR property +/// +/// `t-PJR` checks can check PJR according to an arbitrary threshold. The threshold can be any value, +/// but the property gets stronger as the threshold gets smaller. The strongest possible `t-PJR` property +/// corresponds to `t == 0`. +/// +/// However, standard PJR is less stringent than that. This function returns the threshold whose +/// strength corresponds to the standard PJR property. +/// +/// - `committed_size` is the number of winners of the election. +/// - `weights` is an iterator of voter stakes. If the sum of stakes is already known, +/// `std::iter::once(sum_of_stakes)` is appropriate here. +pub fn standard_threshold( + committeed_size: usize, + weights: impl IntoIterator, +) -> Threshold { + weights + .into_iter() + .sum::() + / committeed_size as Threshold +} + /// Check a solution to be PJR. /// /// The PJR property is true if `t-PJR` is true when `t == sum(stake) / committee_size`. @@ -274,11 +296,7 @@ pub fn pjr_check( all_candidates: Vec, all_voters: Vec<(AccountId, VoteWeight, Vec)>, ) -> bool { - let t = all_voters - .iter() - .map(|(_id, weight, _allocation)| *weight as ExtendedBalance) - .sum::() - / supports.len() as ExtendedBalance; + let t = standard_threshold(supports.len(), all_voters.iter().map(|voter| voter.1 as ExtendedBalance)); t_pjr_check(supports, all_candidates, all_voters, t) } From 2f977df6ddbebf84d519c6afa8f5780fbcef3742 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 1 Mar 2021 16:45:57 +0100 Subject: [PATCH 25/36] restart ci; it appears to be stalled From 8cda3af46c9ada1c6de5d1cb283d41b9b4f2c434 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 1 Mar 2021 17:08:17 +0100 Subject: [PATCH 26/36] use necessary import for no-std --- primitives/npos-elections/src/pjr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 896da772711f3..00e0b010e4983 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -34,7 +34,7 @@ use crate::{ Voter, VoteWeight, }; -use sp_std::rc::Rc; +use sp_std::{rc::Rc, vec::Vec}; use sp_std::collections::btree_map::BTreeMap; use sp_arithmetic::{traits::Zero, Perbill}; From 9f928e3a1e421cedc476e6c32e6006a6173df063 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 2 Mar 2021 09:21:28 +0100 Subject: [PATCH 27/36] use a more realistic distribution of voters and candidates This isn't ideal; more realistic numbers would be about twice these. However, either case generation or voting has nonlinear execution time, and doubling these values brings iteration time from ~20s to ~180s. Fuzzing 6x as fast should make up for fuzzing cases half the size. --- primitives/npos-elections/fuzzer/src/phragmen_pjr.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs index cb5b926bc82b3..9727d1406ad24 100644 --- a/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs +++ b/primitives/npos-elections/fuzzer/src/phragmen_pjr.rs @@ -52,10 +52,10 @@ use sp_npos_elections::{pjr_check_core, seq_phragmen_core, setup_inputs, standar type AccountId = u64; -const MIN_CANDIDATES: usize = 5; -const MAX_CANDIDATES: usize = 200; -const MIN_VOTERS: usize = 5; -const MAX_VOTERS: usize = 500; +const MIN_CANDIDATES: usize = 250; +const MAX_CANDIDATES: usize = 1000; +const MIN_VOTERS: usize = 500; +const MAX_VOTERS: usize = 2500; #[cfg(fuzzing)] fn main() { From 9c7fb95c7aed5491298e822d233ae87264bd12b0 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 2 Mar 2021 09:27:23 +0100 Subject: [PATCH 28/36] identify specifically which PJR check may fail --- primitives/npos-elections/src/phragmen.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/primitives/npos-elections/src/phragmen.rs b/primitives/npos-elections/src/phragmen.rs index a507dd28666ae..a1e632acf5fd3 100644 --- a/primitives/npos-elections/src/phragmen.rs +++ b/primitives/npos-elections/src/phragmen.rs @@ -64,8 +64,9 @@ const DEN: ExtendedBalance = ExtendedBalance::max_value(); /// /// This can only fail if the normalization fails. /// -/// Note that rounding errors can potentially cause the output of this function to fail a PJR check. -/// The underlying algorithm is sound, but the conversions between numeric types can be lossy. +/// Note that rounding errors can potentially cause the output of this function to fail a t-PJR +/// check where t is the standard threshold. The underlying algorithm is sound, but the conversions +/// between numeric types can be lossy. pub fn seq_phragmen( rounds: usize, initial_candidates: Vec, From 06c0e8af8f3056d6a101e7bcae24dca3a2a9d4d3 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 2 Mar 2021 09:30:08 +0100 Subject: [PATCH 29/36] move candidate collection comment into correct place --- primitives/npos-elections/src/pjr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 00e0b010e4983..bd52e5dc31bf5 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -76,7 +76,6 @@ fn prepare_pjr_input( all_candidates: Vec, all_voters: Vec<(AccountId, VoteWeight, Vec)>, ) -> (Vec>, Vec>) { - // collect all candidates and winners into a unified `Vec`. let mut candidates_index: BTreeMap = BTreeMap::new(); // dump the staked assignments in a voter-major map for faster access down the road. @@ -100,6 +99,7 @@ fn prepare_pjr_input( // However, it does mean allocating sufficient space to store all the data again. let supports: SupportMap = supports.iter().cloned().collect(); + // collect all candidates and winners into a unified `Vec`. let candidates = all_candidates.into_iter().enumerate().map(|(i, c)| { candidates_index.insert(c.clone(), i); From cabce8fd7e7fcb0ae26aa20169ff69c916070a18 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 2 Mar 2021 09:43:40 +0100 Subject: [PATCH 30/36] standard_threshold: use a calculation method which cannot overflow --- primitives/npos-elections/src/pjr.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index bd52e5dc31bf5..f0db157bcdaa4 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -284,7 +284,9 @@ pub fn standard_threshold( ) -> Threshold { weights .into_iter() - .sum::() + .fold(Threshold::zero(), |acc, elem| { + acc.saturating_add(elem) + }) / committeed_size as Threshold } From b7616316d8a2e77cc33f32c93a94f67ae2458962 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 2 Mar 2021 09:49:10 +0100 Subject: [PATCH 31/36] Apply suggestions from code review (update comments) Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- primitives/npos-elections/src/pjr.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index f0db157bcdaa4..bbd9532bc2724 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -192,7 +192,7 @@ pub fn t_pjr_check( all_voters: Vec<(AccountId, VoteWeight, Vec)>, t: Threshold, ) -> bool { - // First order of business: derive `(candidates, voters)` from `supports` + // First order of business: derive `(candidates, voters)` from `supports`. let (candidates, voters) = prepare_pjr_input( supports, all_candidates, @@ -251,7 +251,6 @@ fn slack(voter: &Voter, t: Threshold) -> Exte let leftover = voter.edges.iter().fold(Zero::zero(), |acc: ExtendedBalance, edge| { let candidate = edge.candidate.borrow(); if candidate.elected { - // TODO: using perbill here is just going to cause annoyance, why not just subtract? let extra = Perbill::one().min(Perbill::from_rational_approximation(t, candidate.backed_stake)) * edge.weight; @@ -262,7 +261,7 @@ fn slack(voter: &Voter, t: Threshold) -> Exte } }); - // NOTE: candidate for saturating_log_sub() + // NOTE: candidate for saturating_log_sub(). Defensive-only. budget.saturating_sub(leftover) } From eb943752a4d6e09b706d56285baf169325170f7f Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 2 Mar 2021 09:55:10 +0100 Subject: [PATCH 32/36] clarify the effectiveness bounds for t-pjr check --- primitives/npos-elections/src/pjr.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index bbd9532bc2724..e0429a43f56ec 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -157,16 +157,10 @@ fn prepare_pjr_input( /// (i.e `sum(stake(v) for v in voters) == r * t`), then this committee needs to be represented by at /// least `r` elected candidates. /// -/// Section 5 of the NPoS paper shows that this property can be tested by: for a feasible solution, if -/// `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. However, -/// this test is incomplete: while every solution which passes this test satisfies t-PJR, not every -/// solution which is t-PJR passes this test. We therefore look to a more accurate test. -/// -/// The text notes the following: -/// -/// - we can verify this condition by running Algorithm 5: MaxPrescore and validating -/// that MaxPrescore(A, w, t) < t). -/// - the property gets stronger as the threshold decreases. +/// Section 5 of the NPoS paper shows that this property can be tested by: for a feasible solution, +/// if `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. There +/// may exist edge cases which satisfy the formal definition of t-PJR but do not pass this test, but +/// those should be rare enough that we can discount them. /// /// ### Interface /// @@ -261,7 +255,7 @@ fn slack(voter: &Voter, t: Threshold) -> Exte } }); - // NOTE: candidate for saturating_log_sub(). Defensive-only. + // NOTE: candidate for saturating_log_sub(). Defensive-only. budget.saturating_sub(leftover) } From cf6a96b07ea11a8feba6762f2ad55ed3751cd879 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 3 Mar 2021 11:07:56 +0100 Subject: [PATCH 33/36] how to spell "committee" --- primitives/npos-elections/src/pjr.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index e0429a43f56ec..d26215b0005c8 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -268,11 +268,11 @@ fn slack(voter: &Voter, t: Threshold) -> Exte /// However, standard PJR is less stringent than that. This function returns the threshold whose /// strength corresponds to the standard PJR property. /// -/// - `committed_size` is the number of winners of the election. +/// - `committee_size` is the number of winners of the election. /// - `weights` is an iterator of voter stakes. If the sum of stakes is already known, /// `std::iter::once(sum_of_stakes)` is appropriate here. pub fn standard_threshold( - committeed_size: usize, + committee_size: usize, weights: impl IntoIterator, ) -> Threshold { weights @@ -280,7 +280,7 @@ pub fn standard_threshold( .fold(Threshold::zero(), |acc, elem| { acc.saturating_add(elem) }) - / committeed_size as Threshold + / committee_size as Threshold } /// Check a solution to be PJR. From 44c73efa560f7cebdc4fb931e5f8441f02dc0eb3 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 3 Mar 2021 11:10:54 +0100 Subject: [PATCH 34/36] reorganize: high -> low abstraction --- primitives/npos-elections/src/pjr.rs | 205 ++++++++++++++------------- 1 file changed, 103 insertions(+), 102 deletions(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index d26215b0005c8..852d311e45c2b 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -38,12 +38,114 @@ use sp_std::{rc::Rc, vec::Vec}; use sp_std::collections::btree_map::BTreeMap; use sp_arithmetic::{traits::Zero, Perbill}; - /// The type used as the threshold. /// /// Just some reading sugar; Must always be same as [`ExtendedBalance`]; type Threshold = ExtendedBalance; +/// Compute the threshold corresponding to the standard PJR property +/// +/// `t-PJR` checks can check PJR according to an arbitrary threshold. The threshold can be any value, +/// but the property gets stronger as the threshold gets smaller. The strongest possible `t-PJR` property +/// corresponds to `t == 0`. +/// +/// However, standard PJR is less stringent than that. This function returns the threshold whose +/// strength corresponds to the standard PJR property. +/// +/// - `committee_size` is the number of winners of the election. +/// - `weights` is an iterator of voter stakes. If the sum of stakes is already known, +/// `std::iter::once(sum_of_stakes)` is appropriate here. +pub fn standard_threshold( + committee_size: usize, + weights: impl IntoIterator, +) -> Threshold { + weights + .into_iter() + .fold(Threshold::zero(), |acc, elem| { + acc.saturating_add(elem) + }) + / committee_size as Threshold +} + +/// Check a solution to be PJR. +/// +/// The PJR property is true if `t-PJR` is true when `t == sum(stake) / committee_size`. +pub fn pjr_check( + supports: &Supports, + all_candidates: Vec, + all_voters: Vec<(AccountId, VoteWeight, Vec)>, +) -> bool { + let t = standard_threshold(supports.len(), all_voters.iter().map(|voter| voter.1 as ExtendedBalance)); + t_pjr_check(supports, all_candidates, all_voters, t) +} + +/// Check a solution to be t-PJR. +/// +/// ### Semantics +/// +/// The t-PJR property is defined in the paper ["Validator Election in Nominated Proof-of-Stake"][NPoS], +/// section 5, definition 1. +/// +/// In plain language, the t-PJR condition is: if there is a group of `N` voters +/// who have `r` common candidates and can afford to support each of them with backing stake `t` +/// (i.e `sum(stake(v) for v in voters) == r * t`), then this committee needs to be represented by at +/// least `r` elected candidates. +/// +/// Section 5 of the NPoS paper shows that this property can be tested by: for a feasible solution, +/// if `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. There +/// may exist edge cases which satisfy the formal definition of t-PJR but do not pass this test, but +/// those should be rare enough that we can discount them. +/// +/// ### Interface +/// +/// In addition to data that can be computed from the [`Supports`] struct, a PJR check also +/// needs to inspect un-elected candidates and edges, thus `all_candidates` and `all_voters`. +/// +/// [NPoS]: https://arxiv.org/pdf/2004.12990v1.pdf +// +// ### Implementation Notes +// +// The paper uses mathematical notation, which priorities single-symbol names. For programmer ease, +// we map these to more descriptive names as follows: +// +// C => all_candidates +// N => all_voters +// (A, w) => (candidates, voters) +// +// Note that while the names don't explicitly say so, `candidates` are the winning candidates, and +// `voters` is the set of weighted edges from nominators to winning validators. +pub fn t_pjr_check( + supports: &Supports, + all_candidates: Vec, + all_voters: Vec<(AccountId, VoteWeight, Vec)>, + t: Threshold, +) -> bool { + // First order of business: derive `(candidates, voters)` from `supports`. + let (candidates, voters) = prepare_pjr_input( + supports, + all_candidates, + all_voters, + ); + // compute with threshold t. + pjr_check_core(candidates.as_ref(), voters.as_ref(), t) +} + +/// The internal implementation of the PJR check after having the data converted. +/// +/// [`pjr_check`] or [`t_pjr_check`] are typically easier to work with. +pub fn pjr_check_core( + candidates: &[CandidatePtr], + voters: &[Voter], + t: Threshold, +) -> bool { + let unelected = candidates.iter().filter(|c| !c.borrow().elected); + let maybe_max_pre_score = unelected.map(|c| (pre_score(Rc::clone(c), voters, t), c.borrow().who.clone())).max(); + // if unelected is empty then the solution is indeed PJR. + maybe_max_pre_score.map_or(true, |(max_pre_score, _)| max_pre_score < t) +} + + + /// Convert the data types that the user runtime has into ones that can be used by this module. /// /// It is expected that this function's interface might change over time, or multiple variants of it @@ -145,71 +247,6 @@ fn prepare_pjr_input( (candidates, voters) } -/// Check a solution to be t-PJR. -/// -/// ### Semantics -/// -/// The t-PJR property is defined in the paper ["Validator Election in Nominated Proof-of-Stake"][NPoS], -/// section 5, definition 1. -/// -/// In plain language, the t-PJR condition is: if there is a group of `N` voters -/// who have `r` common candidates and can afford to support each of them with backing stake `t` -/// (i.e `sum(stake(v) for v in voters) == r * t`), then this committee needs to be represented by at -/// least `r` elected candidates. -/// -/// Section 5 of the NPoS paper shows that this property can be tested by: for a feasible solution, -/// if `Max {score(c)} < t` where c is every unelected candidate, then this solution is t-PJR. There -/// may exist edge cases which satisfy the formal definition of t-PJR but do not pass this test, but -/// those should be rare enough that we can discount them. -/// -/// ### Interface -/// -/// In addition to data that can be computed from the [`Supports`] struct, a PJR check also -/// needs to inspect un-elected candidates and edges, thus `all_candidates` and `all_voters`. -/// -/// [NPoS]: https://arxiv.org/pdf/2004.12990v1.pdf -// -// ### Implementation Notes -// -// The paper uses mathematical notation, which priorities single-symbol names. For programmer ease, -// we map these to more descriptive names as follows: -// -// C => all_candidates -// N => all_voters -// (A, w) => (candidates, voters) -// -// Note that while the names don't explicitly say so, `candidates` are the winning candidates, and -// `voters` is the set of weighted edges from nominators to winning validators. -pub fn t_pjr_check( - supports: &Supports, - all_candidates: Vec, - all_voters: Vec<(AccountId, VoteWeight, Vec)>, - t: Threshold, -) -> bool { - // First order of business: derive `(candidates, voters)` from `supports`. - let (candidates, voters) = prepare_pjr_input( - supports, - all_candidates, - all_voters, - ); - // compute with threshold t. - pjr_check_core(candidates.as_ref(), voters.as_ref(), t) -} - -/// The internal implementation of the PJR check after having the data converted. -/// -/// [`pjr_check`] or [`t_pjr_check`] are typically easier to work with. -pub fn pjr_check_core( - candidates: &[CandidatePtr], - voters: &[Voter], - t: Threshold, -) -> bool { - let unelected = candidates.iter().filter(|c| !c.borrow().elected); - let maybe_max_pre_score = unelected.map(|c| (pre_score(Rc::clone(c), voters, t), c.borrow().who.clone())).max(); - // if unelected is empty then the solution is indeed PJR. - maybe_max_pre_score.map_or(true, |(max_pre_score, _)| max_pre_score < t) -} - /// The pre-score of an unelected candidate. /// /// This is the amount of stake that *all voter* can spare to devote to this candidate without @@ -259,42 +296,6 @@ fn slack(voter: &Voter, t: Threshold) -> Exte budget.saturating_sub(leftover) } -/// Compute the threshold corresponding to the standard PJR property -/// -/// `t-PJR` checks can check PJR according to an arbitrary threshold. The threshold can be any value, -/// but the property gets stronger as the threshold gets smaller. The strongest possible `t-PJR` property -/// corresponds to `t == 0`. -/// -/// However, standard PJR is less stringent than that. This function returns the threshold whose -/// strength corresponds to the standard PJR property. -/// -/// - `committee_size` is the number of winners of the election. -/// - `weights` is an iterator of voter stakes. If the sum of stakes is already known, -/// `std::iter::once(sum_of_stakes)` is appropriate here. -pub fn standard_threshold( - committee_size: usize, - weights: impl IntoIterator, -) -> Threshold { - weights - .into_iter() - .fold(Threshold::zero(), |acc, elem| { - acc.saturating_add(elem) - }) - / committee_size as Threshold -} - -/// Check a solution to be PJR. -/// -/// The PJR property is true if `t-PJR` is true when `t == sum(stake) / committee_size`. -pub fn pjr_check( - supports: &Supports, - all_candidates: Vec, - all_voters: Vec<(AccountId, VoteWeight, Vec)>, -) -> bool { - let t = standard_threshold(supports.len(), all_voters.iter().map(|voter| voter.1 as ExtendedBalance)); - t_pjr_check(supports, all_candidates, all_voters, t) -} - #[cfg(test)] mod tests { use super::*; From f1721e0f7a10b71452e0b56cfb20a705b484c993 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 3 Mar 2021 11:44:43 +0100 Subject: [PATCH 35/36] ensure standard threshold calc cannot panic Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- primitives/npos-elections/src/pjr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index 852d311e45c2b..c3fe35040df04 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -64,7 +64,7 @@ pub fn standard_threshold( .fold(Threshold::zero(), |acc, elem| { acc.saturating_add(elem) }) - / committee_size as Threshold + / committee_size.max(1) as Threshold } /// Check a solution to be PJR. From a98208faa5b7eef33c87f4e76a5c7b1d8451d525 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 11 Mar 2021 09:26:59 +0100 Subject: [PATCH 36/36] Apply suggestions from code review Co-authored-by: Shawn Tabrizi --- primitives/npos-elections/src/lib.rs | 1 - primitives/npos-elections/src/pjr.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/primitives/npos-elections/src/lib.rs b/primitives/npos-elections/src/lib.rs index da175180ed30e..c87085ef9ff8f 100644 --- a/primitives/npos-elections/src/lib.rs +++ b/primitives/npos-elections/src/lib.rs @@ -340,7 +340,6 @@ impl Voter { Self { who, ..Default::default() } } - /// Returns `true` if `self` votes for `target`. /// /// Note that this does not take into account if `target` is elected (i.e. is *active*) or not. diff --git a/primitives/npos-elections/src/pjr.rs b/primitives/npos-elections/src/pjr.rs index c3fe35040df04..61e0b2deb79fc 100644 --- a/primitives/npos-elections/src/pjr.rs +++ b/primitives/npos-elections/src/pjr.rs @@ -1,6 +1,6 @@ // This file is part of Substrate. -// Copyright (C) 2020 Parity Technologies (UK) Ltd. +// Copyright (C) 2021 Parity Technologies (UK) Ltd. // SPDX-License-Identifier: Apache-2.0 // Licensed under the Apache License, Version 2.0 (the "License");