Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion primitives/npos-elections/fuzzer/src/phragmen_pjr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ fn iteration(mut candidate_count: usize, mut voter_count: usize, seed: u64) {
let threshold = standard_threshold(rounds, voters.iter().map(|voter| voter.budget()));

assert!(
pjr_check_core(&candidates, &voters, threshold),
pjr_check_core(&candidates, &voters, threshold).is_ok(),
"unbalanced sequential phragmen must satisfy PJR",
);
}
112 changes: 101 additions & 11 deletions primitives/npos-elections/src/pjr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub fn pjr_check<AccountId: IdentifierT>(
supports: &Supports<AccountId>,
all_candidates: Vec<AccountId>,
all_voters: Vec<(AccountId, VoteWeight, Vec<AccountId>)>,
) -> bool {
) -> Result<(), AccountId> {
let t = standard_threshold(supports.len(), all_voters.iter().map(|voter| voter.1 as ExtendedBalance));
t_pjr_check(supports, all_candidates, all_voters, t)
}
Expand Down Expand Up @@ -119,7 +119,7 @@ pub fn t_pjr_check<AccountId: IdentifierT>(
all_candidates: Vec<AccountId>,
all_voters: Vec<(AccountId, VoteWeight, Vec<AccountId>)>,
t: Threshold,
) -> bool {
) -> Result<(), AccountId> {
// First order of business: derive `(candidates, voters)` from `supports`.
let (candidates, voters) = prepare_pjr_input(
supports,
Expand All @@ -133,18 +133,99 @@ pub fn t_pjr_check<AccountId: IdentifierT>(
/// The internal implementation of the PJR check after having the data converted.
///
/// [`pjr_check`] or [`t_pjr_check`] are typically easier to work with.
///
/// This function returns an `AccountId` in the `Err` case. This is the counter_example: the ID of the
/// unelected candidate with the highest prescore, such that `pre_score(counter_example) >= t`.
pub fn pjr_check_core<AccountId: IdentifierT>(
candidates: &[CandidatePtr<AccountId>],
voters: &[Voter<AccountId>],
t: Threshold,
) -> bool {
) -> Result<(), AccountId> {
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)
match maybe_max_pre_score {
Some((max_pre_score, counter_example)) if max_pre_score >= t => Err(counter_example),
_ => Ok(()),
}
}

/// Validate a challenge to an election result.
///
/// A challenge to an election result is valid if there exists some counter_example for which
/// `pre_score(counter_example) >= threshold`. Validating an existing counter_example is computationally
/// cheaper than re-running the PJR check.
///
/// This function uses the standard threshold.
///
/// Returns `true` if the challenge is valid: the proposed solution does not satisfy PJR.
/// Returns `false` if the challenge is invalid: the proposed solution does in fact satisfy PJR.
pub fn validate_pjr_challenge<AccountId: IdentifierT>(
counter_example: AccountId,
supports: &Supports<AccountId>,
all_candidates: Vec<AccountId>,
all_voters: Vec<(AccountId, VoteWeight, Vec<AccountId>)>,
Comment on lines +165 to +167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One random note unrelated to this PR per se: this pallet had quite a bad rap for not having a well defined behaviour with duplicate votes and candidates and targets etc. Now, finally, everything that goes through setup_input (called prior to seq_phragmen and phragmms) gets sanitized. Would be good to ensure these functions have the same standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prepare_pjr_input mentions setup_inputs in its documentation, but doesn't actually delegate. Shall I create an issue to add explicit delegation there to ensure that the behaviors are identical?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An issue would be good, bit I am not sure if delegation is the best way, we have to see. We can maybe mix this into #4593

) -> bool {
let threshold = standard_threshold(supports.len(), all_voters.iter().map(|voter| voter.1 as ExtendedBalance));
validate_t_pjr_challenge(counter_example, supports, all_candidates, all_voters, threshold)
}

/// Validate a challenge to an election result.
///
/// A challenge to an election result is valid if there exists some counter_example for which
/// `pre_score(counter_example) >= threshold`. Validating an existing counter_example is computationally
/// cheaper than re-running the PJR check.
///
/// This function uses a supplied threshold.
///
/// Returns `true` if the challenge is valid: the proposed solution does not satisfy PJR.
/// Returns `false` if the challenge is invalid: the proposed solution does in fact satisfy PJR.
pub fn validate_t_pjr_challenge<AccountId: IdentifierT>(
counter_example: AccountId,
supports: &Supports<AccountId>,
all_candidates: Vec<AccountId>,
all_voters: Vec<(AccountId, VoteWeight, Vec<AccountId>)>,
threshold: Threshold,
) -> bool {
let (candidates, voters) = prepare_pjr_input(
supports,
all_candidates,
all_voters,
);
validate_pjr_challenge_core(counter_example, &candidates, &voters, threshold)
}

/// Validate a challenge to an election result.
///
/// A challenge to an election result is valid if there exists some counter_example for which
/// `pre_score(counter_example) >= threshold`. Validating an existing counter_example is computationally
/// cheaper than re-running the PJR check.
///
/// Returns `true` if the challenge is valid: the proposed solution does not satisfy PJR.
/// Returns `false` if the challenge is invalid: the proposed solution does in fact satisfy PJR.
Comment on lines +198 to +205
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this exact text is copied on all these functions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also any advantage to breaking up the steps into such minimal functions? I feel if anything, this may actually loose context of what other operations are happening between the steps, where optimizations may be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference in the text is

- /// This function uses the standard threshold.
+ /// This function uses a supplied threshold.

The documentation goal is to ensure that each function's documentation completely describes what that function does, even though a similar function exists.

As for why the functions are factored in this way, it comes down to two things. On the one hand, it mirrors the factorization of the pjr_check functions. On the other hand, it means that users can supply a threshold, or not, as desired. We have use cases for both.

fn validate_pjr_challenge_core<AccountId: IdentifierT>(
counter_example: AccountId,
candidates: &[CandidatePtr<AccountId>],
voters: &[Voter<AccountId>],
threshold: Threshold,
) -> bool {
// Performing a linear search of the candidate list is not great, for obvious reasons. However,
// the alternatives are worse:
//
// - we could pre-sort the candidates list in `prepare_pjr_input` (n log n) which would let us
// binary search for the appropriate one here (log n). Overall runtime is `n log n` which is
// worse than the current runtime of `n`.
//
// - we could probably pre-sort the candidates list in `n` in `prepare_pjr_input` using some
// unsafe code leveraging the existing `candidates_index`: allocate an uninitialized vector of
// appropriate length, then copy in all the elements. We'd really prefer to avoid unsafe code
// in the runtime, though.
let candidate = match candidates.iter().find(|candidate| candidate.borrow().who == counter_example) {
None => return false,
Some(candidate) => candidate.clone(),
};
pre_score(candidate, &voters, threshold) >= threshold
}

/// Convert the data types that the user runtime has into ones that can be used by this module.
///
Expand Down Expand Up @@ -315,6 +396,15 @@ mod tests {
voter
}

fn assert_core_failure<AccountId: IdentifierT>(
candidates: &[CandidatePtr<AccountId>],
voters: &[Voter<AccountId>],
t: Threshold,
) {
let counter_example = pjr_check_core(candidates, voters, t).unwrap_err();
assert!(validate_pjr_challenge_core(counter_example, candidates, voters, t));
}

#[test]
fn slack_works() {
let voter = setup_voter(10, vec![(1, 10, true), (2, 20, true)]);
Expand Down Expand Up @@ -388,9 +478,9 @@ mod tests {

// 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));
assert_core_failure(&candidates, &voters, 1);
assert_core_failure(&candidates, &voters, 10);
assert_core_failure(&candidates, &voters, 20);
}

// These next tests ensure that the threshold phase change property holds for us, but that's not their real purpose.
Expand Down Expand Up @@ -476,7 +566,7 @@ mod tests {
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) {
while pjr_check_core(&candidates, &voters, threshold).is_err() {
prev_threshold = threshold;
threshold = threshold.checked_mul(2).expect("pjr check must fail before we run out of capacity in u128");
}
Expand All @@ -488,7 +578,7 @@ mod tests {
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) {
if pjr_check_core(&candidates, &voters, test).is_ok() {
high_bound = test;
} else {
low_bound = test;
Expand All @@ -502,12 +592,12 @@ mod tests {
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) {
if pjr_check_core(&candidates, &voters, t).is_ok() {
unexpected_successes.push(t);
}
}
for t in high_bound..(high_bound*2) {
if !pjr_check_core(&candidates, &voters, t) {
if pjr_check_core(&candidates, &voters, t).is_err() {
unexpected_failures.push(t);
}
}
Expand Down