diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 964cf6daf2cee..b1c4ea5e679bf 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -892,12 +892,15 @@ impl Module { voters_and_votes.clone(), None, ).map(|ElectionResult { winners, assignments: _ }| { - let old_members_ids = >::take().into_iter() + // this is already sorted by id. + let old_members_ids_sorted = >::take().into_iter() .map(|(m, _)| m) .collect::>(); - let old_runners_up_ids = >::take().into_iter() + // this one needs a sort by id. + let mut old_runners_up_ids_sorted = >::take().into_iter() .map(|(r, _)| r) .collect::>(); + old_runners_up_ids_sorted.sort(); // filter out those who end up with no backing stake. let new_set_with_stake = winners @@ -912,17 +915,17 @@ impl Module { // split new set into winners and runners up. let split_point = desired_seats.min(new_set_with_stake.len()); - let mut new_members = (&new_set_with_stake[..split_point]).to_vec(); + let mut new_members_sorted_by_id = (&new_set_with_stake[..split_point]).to_vec(); // save the runners up as-is. They are sorted based on desirability. // save the members, sorted based on account id. - new_members.sort_by(|i, j| i.0.cmp(&j.0)); + new_members_sorted_by_id.sort_by(|i, j| i.0.cmp(&j.0)); // Now we select a prime member using a [Borda count](https://en.wikipedia.org/wiki/Borda_count). // We weigh everyone's vote for that new member by a multiplier based on the order // of the votes. i.e. the first person a voter votes for gets a 16x multiplier, // the next person gets a 15x multiplier, an so on... (assuming `MAXIMUM_VOTE` = 16) - let mut prime_votes: Vec<_> = new_members.iter().map(|c| (&c.0, BalanceOf::::zero())).collect(); + let mut prime_votes: Vec<_> = new_members_sorted_by_id.iter().map(|c| (&c.0, BalanceOf::::zero())).collect(); for (_, stake, votes) in voters_and_stakes.into_iter() { for (vote_multiplier, who) in votes.iter() .enumerate() @@ -940,54 +943,58 @@ impl Module { // the person with the "highest" account id based on the sort above. let prime = prime_votes.into_iter().max_by_key(|x| x.1).map(|x| x.0.clone()); - // new_members_ids is sorted by account id. - let new_members_ids = new_members + // new_members_sorted_by_id is sorted by account id. + let new_members_ids_sorted = new_members_sorted_by_id .iter() .map(|(m, _)| m.clone()) .collect::>(); - let new_runners_up = &new_set_with_stake[split_point..] + let new_runners_up_sorted_by_rank = &new_set_with_stake[split_point..] .into_iter() .cloned() .rev() .collect::)>>(); // new_runners_up remains sorted by desirability. - let new_runners_up_ids = new_runners_up + let mut new_runners_up_ids_sorted = new_runners_up_sorted_by_rank .iter() .map(|(r, _)| r.clone()) .collect::>(); + new_runners_up_ids_sorted.sort(); // report member changes. We compute diff because we need the outgoing list. let (incoming, outgoing) = T::ChangeMembers::compute_members_diff( - &new_members_ids, - &old_members_ids, + &new_members_ids_sorted, + &old_members_ids_sorted, ); T::ChangeMembers::change_members_sorted( &incoming, &outgoing, - &new_members_ids, + &new_members_ids_sorted, ); T::ChangeMembers::set_prime(prime); - // outgoing candidates lose their bond. + // outgoing members lose their bond. let mut to_burn_bond = outgoing.to_vec(); // compute the outgoing of runners up as well and append them to the `to_burn_bond` { let (_, outgoing) = T::ChangeMembers::compute_members_diff( - &new_runners_up_ids, - &old_runners_up_ids, + &new_runners_up_ids_sorted, + &old_runners_up_ids_sorted, ); + // none of the ones computed to be outgoing must still be in the list. + debug_assert!(outgoing.iter().all(|o| !new_runners_up_ids_sorted.contains(o))); to_burn_bond.extend(outgoing); } // Burn loser bond. members list is sorted. O(NLogM) (N candidates, M members) - // runner up list is not sorted. O(K*N) given K runner ups. Overall: O(NLogM + N*K) + // runner up list is also sorted. O(NLogK) given K runner ups. Overall: O(NLogM + N*K) // both the member and runner counts are bounded. exposed_candidates.into_iter().for_each(|c| { // any candidate who is not a member and not a runner up. - if new_members.binary_search_by_key(&c, |(m, _)| m.clone()).is_err() - && !new_runners_up_ids.contains(&c) + if + new_members_ids_sorted.binary_search(&c).is_err() && + new_runners_up_ids_sorted.binary_search(&c).is_err() { let (imbalance, _) = T::Currency::slash_reserved(&c, T::CandidacyBond::get()); T::LoserCandidate::on_unbalanced(imbalance); @@ -1000,10 +1007,10 @@ impl Module { T::LoserCandidate::on_unbalanced(imbalance); }); - >::put(&new_members); - >::put(new_runners_up); + >::put(&new_members_sorted_by_id); + >::put(new_runners_up_sorted_by_rank); - Self::deposit_event(RawEvent::NewTerm(new_members.clone().to_vec())); + Self::deposit_event(RawEvent::NewTerm(new_members_sorted_by_id.clone().to_vec())); // clean candidates. >::kill(); @@ -1260,7 +1267,6 @@ mod tests { self.genesis_members = members; self } - #[cfg(feature = "runtime-benchmarks")] pub fn desired_members(mut self, count: u32) -> Self { self.desired_members = count; self @@ -2836,4 +2842,38 @@ mod tests { assert!(Elections::candidates().is_empty()); }) } + + #[test] + fn unsorted_runners_up_are_detected() { + ExtBuilder::default().desired_runners_up(2).desired_members(1).build_and_execute(|| { + assert_ok!(submit_candidacy(Origin::signed(5))); + assert_ok!(submit_candidacy(Origin::signed(4))); + assert_ok!(submit_candidacy(Origin::signed(3))); + + + assert_ok!(vote(Origin::signed(5), vec![5], 50)); + assert_ok!(vote(Origin::signed(4), vec![4], 5)); + assert_ok!(vote(Origin::signed(3), vec![3], 15)); + + System::set_block_number(5); + Elections::end_block(System::block_number()); + + assert_eq!(Elections::members_ids(), vec![5]); + assert_eq!(Elections::runners_up_ids(), vec![4, 3]); + + assert_ok!(submit_candidacy(Origin::signed(2))); + assert_ok!(vote(Origin::signed(2), vec![2], 10)); + + System::set_block_number(10); + Elections::end_block(System::block_number()); + + assert_eq!(Elections::members_ids(), vec![5]); + assert_eq!(Elections::runners_up_ids(), vec![2, 3]); + + // 4 is outgoing runner-up. Slash candidacy bond. + assert_eq!(balances(&4), (35, 2)); + // 3 stays. + assert_eq!(balances(&3), (25, 5)); + }) + } }