From cabfbbb6ae2c1524977645a06da882ebb2f887f8 Mon Sep 17 00:00:00 2001 From: gpestana Date: Mon, 17 Oct 2022 10:16:03 +0200 Subject: [PATCH 1/7] [draft] Adds ElectionProvider to handle the elections logic; Pallet is an ElectionDataProvider --- Cargo.lock | 1 + frame/elections-phragmen/Cargo.toml | 1 + frame/elections-phragmen/src/election.rs | 150 ++++++++++++++++++++ frame/elections-phragmen/src/lib.rs | 168 +++++++++++++++++++++-- 4 files changed, 307 insertions(+), 13 deletions(-) create mode 100644 frame/elections-phragmen/src/election.rs diff --git a/Cargo.lock b/Cargo.lock index 6a5b562d0a791..eec5da76b874f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5424,6 +5424,7 @@ name = "pallet-elections-phragmen" version = "5.0.0-dev" dependencies = [ "frame-benchmarking", + "frame-election-provider-support", "frame-support", "frame-system", "log", diff --git a/frame/elections-phragmen/Cargo.toml b/frame/elections-phragmen/Cargo.toml index 2d71a6bed39df..995a8f252b303 100644 --- a/frame/elections-phragmen/Cargo.toml +++ b/frame/elections-phragmen/Cargo.toml @@ -20,6 +20,7 @@ log = { version = "0.4.14", default-features = false } scale-info = { version = "2.0.0", default-features = false, features = ["derive"] } frame-benchmarking = { version = "4.0.0-dev", default-features = false, optional = true, path = "../benchmarking" } frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } +frame-election-provider-support = { version = "4.0.0-dev", default-features = false, path = "../election-provider-support" } frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } sp-core = { version = "6.0.0", default-features = false, path = "../../primitives/core" } sp-io = { version = "6.0.0", default-features = false, path = "../../primitives/io" } diff --git a/frame/elections-phragmen/src/election.rs b/frame/elections-phragmen/src/election.rs new file mode 100644 index 0000000000000..3ad83d956c87f --- /dev/null +++ b/frame/elections-phragmen/src/election.rs @@ -0,0 +1,150 @@ +use sp_std::{marker::PhantomData, prelude::*}; + +use frame_election_provider_support::{ + weights::WeightInfo, BoundedVec, ElectionDataProvider, ElectionProvider, ElectionProviderBase, + InstantElectionProvider, NposSolver, VoterOf, +}; +use frame_support::traits::Get; +use sp_npos_elections::*; + +type AccountId = ::AccountId; +type BlockNumber = ::BlockNumber; +type Balance = u64; // TODO(gpestana): draft for now; abstract + +/// Errors of the on-chain election. +#[derive(Eq, PartialEq, Debug)] +pub enum Error { + /// An internal error in the NPoS elections crate. + NposElections(sp_npos_elections::Error), + /// Errors from the data provider. + DataProvider(&'static str), +} + +impl From for Error { + fn from(e: sp_npos_elections::Error) -> Self { + Error::NposElections(e) + } +} + +pub trait DataProviderConfig { + type System: frame_system::Config; + type MaxVotesPerVoter: Get; + + fn candidates() -> Vec>; + fn votes_with_stake( + ) -> Vec<(AccountId, crate::Voter, Balance>)>; +} + +pub struct DataProvider(PhantomData); +impl ElectionDataProvider for DataProvider { + type AccountId = AccountId; + type BlockNumber = BlockNumber; + type MaxVotesPerVoter = T::MaxVotesPerVoter; + + fn electable_targets( + maybe_max_len: Option, + ) -> frame_election_provider_support::data_provider::Result> { + Ok(T::candidates()) + } + + fn electing_voters( + maybe_max_len: Option, + ) -> frame_election_provider_support::data_provider::Result>> { + let max_votes_per_voter = T::MaxVotesPerVoter::get() as usize; + let mut voters_and_stakes = Vec::new(); + + match T::votes_with_stake().into_iter().try_for_each( + |(voter, crate::Voter { stake, votes, .. })| { + if let Some(max_voters) = maybe_max_len { + if voters_and_stakes.len() > max_voters { + return Err(()); + } + } + let bounded_votes: BoundedVec<_, T::MaxVotesPerVoter> = + BoundedVec::try_from(votes).map_err(|_| ())?; // TODO(gpestana): ref to proper err + + voters_and_stakes.push((voter, stake, bounded_votes)); + Ok(()) + }, + ) { + Ok(_) => (), + Err(_) => return Err(""), // TODO(gpestana): ref proper err + } + + Ok(voters_and_stakes) + } + + fn desired_targets() -> frame_election_provider_support::data_provider::Result { + // TODO(gpestana): fill in + Ok(10) + } + + fn next_election_prediction(now: Self::BlockNumber) -> Self::BlockNumber { + // TODO(gpestana): fill in + >::block_number() + } +} + +pub trait ElectionConfig { + type System: frame_system::Config; + type DataProvider: ElectionDataProvider< + AccountId = AccountId, + BlockNumber = BlockNumber, + >; + type Solver: NposSolver, Error = sp_npos_elections::Error>; + type WeightInfo: WeightInfo; +} + +pub struct BoundedExecution(PhantomData); + +fn elect_with_bounds( + maybe_max_voters: Option, + maybe_max_targets: Option, +) -> Result>, Error> { + // TODO(gpestana): finsh impl + + // TODO(gpestana): handle unwraps() + let num_to_elect = T::DataProvider::desired_targets().unwrap(); + let candidate_ids = T::DataProvider::electable_targets(maybe_max_targets).unwrap(); + let voters_and_votes = T::DataProvider::electing_voters(maybe_max_voters).unwrap(); + + T::Solver::solve(num_to_elect as usize, candidate_ids, voters_and_votes) + .map(|election_result| {}); + + Ok(vec![]) +} + +impl ElectionProviderBase for BoundedExecution { + type AccountId = AccountId; + type BlockNumber = BlockNumber; + type Error = Error; + type DataProvider = T::DataProvider; + + fn ongoing() -> bool { + return false; + } +} + +impl ElectionProvider for BoundedExecution { + fn elect() -> Result, Self::Error> { + // This should not be called if not in `std` mode (and therefore neither in genesis nor in + // testing) + if cfg!(not(feature = "std")) { + frame_support::log::error!( + "Please use `InstantElectionProvider` instead to provide bounds on election if not in \ + genesis or testing mode" + ); + } + + elect_with_bounds::(None, None) + } +} + +impl InstantElectionProvider for BoundedExecution { + fn elect_with_bounds( + max_voters: usize, + max_targets: usize, + ) -> Result, Self::Error> { + elect_with_bounds::(Some(max_voters), Some(max_targets)) + } +} diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 165a8fcab429b..28ae8b4015670 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -98,14 +98,20 @@ #![cfg_attr(not(feature = "std"), no_std)] +use core::marker::PhantomData; + use codec::{Decode, Encode}; +use frame_election_provider_support::{ + ElectionDataProvider, ElectionProvider, ElectionProviderBase, NposSolver, +}; use frame_support::{ traits::{ - defensive_prelude::*, ChangeMembers, Contains, ContainsLengthBound, Currency, + defensive_prelude::*, ChangeMembers, ConstU32, Contains, ContainsLengthBound, Currency, CurrencyToVote, Get, InitializeMembers, LockIdentifier, LockableCurrency, OnUnbalanced, ReservableCurrency, SortedMembers, WithdrawReasons, }, weights::Weight, + BoundedVec, }; use scale_info::TypeInfo; use sp_npos_elections::{ElectionResult, ExtendedBalance}; @@ -122,8 +128,12 @@ pub use weights::WeightInfo; /// All migrations. pub mod migrations; +/// The election module, implementing an ElectionProvider and ElectionDataProvider that can +/// be used by this pallet. +pub mod election; + /// The maximum votes allowed per voter. -pub const MAXIMUM_VOTE: usize = 16; +pub const MAXIMUM_VOTE: u32 = 16; type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; @@ -267,6 +277,14 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: WeightInfo; + + /// Something that will calculate the election results based on the data provided by + /// Self as DataProvider. + type ElectionProvider: ElectionProvider< + AccountId = Self::AccountId, + BlockNumber = Self::BlockNumber, + DataProvider = Pallet, + >; } #[pallet::hooks] @@ -276,8 +294,8 @@ pub mod pallet { /// Checks if an election needs to happen or not. fn on_initialize(n: T::BlockNumber) -> Weight { let term_duration = T::TermDuration::get(); - if !term_duration.is_zero() && (n % term_duration).is_zero() { - Self::do_phragmen() + if !term_duration.is_zero() && Self::next_election_prediction(n) == n { + Self::do_election() } else { Weight::zero() } @@ -322,7 +340,7 @@ pub mod pallet { let who = ensure_signed(origin)?; // votes should not be empty and more than `MAXIMUM_VOTE` in any case. - ensure!(votes.len() <= MAXIMUM_VOTE, Error::::MaximumVotesExceeded); + ensure!(votes.len() as u32 <= MAXIMUM_VOTE, Error::::MaximumVotesExceeded); ensure!(!votes.is_empty(), Error::::NoVotes); let candidates_count = >::decode_len().unwrap_or(0); @@ -518,7 +536,7 @@ pub mod pallet { Self::deposit_event(Event::MemberKicked { member: who }); if rerun_election { - Self::do_phragmen(); + Self::do_election(); } // no refund needed. @@ -905,7 +923,7 @@ impl Pallet { if candidates_and_deposit.len().is_zero() { Self::deposit_event(Event::EmptyTerm); - return T::DbWeight::get().reads(3) + return T::DbWeight::get().reads(3); } // All of the new winners that come out of phragmen will thus have a deposit recorded. @@ -937,7 +955,7 @@ impl Pallet { "Failed to run election. Number of voters exceeded", ); Self::deposit_event(Event::ElectionError); - return T::DbWeight::get().reads(3 + max_voters as u64) + return T::DbWeight::get().reads(3 + max_voters as u64); }, } @@ -954,7 +972,9 @@ impl Pallet { let weight_candidates = candidates_and_deposit.len() as u32; let weight_voters = voters_and_votes.len() as u32; let weight_edges = num_edges; + let _ = + //ElectionProvider >> T::Solver::solve(num_to_elect, candidate_ids, voters_and_votes) sp_npos_elections::seq_phragmen(num_to_elect, candidate_ids, voters_and_votes, None) .map(|ElectionResult:: { winners, assignments: _ }| { // this is already sorted by id. @@ -1009,7 +1029,7 @@ impl Pallet { for (_, stake, votes) in voters_and_stakes.into_iter() { for (vote_multiplier, who) in votes.iter().enumerate().map(|(vote_position, who)| { - ((MAXIMUM_VOTE - vote_position) as u32, who) + (MAXIMUM_VOTE - (vote_position as u32), who) }) { if let Ok(i) = prime_votes.binary_search_by_key(&who, |k| k.0) { prime_votes[i].1 = prime_votes[i] @@ -1044,8 +1064,8 @@ impl Pallet { // All candidates/members/runners-up who are no longer retaining a position as a // seat holder will lose their bond. candidates_and_deposit.iter().for_each(|(c, d)| { - if new_members_ids_sorted.binary_search(c).is_err() && - new_runners_up_ids_sorted.binary_search(c).is_err() + 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, *d); T::LoserCandidate::on_unbalanced(imbalance); @@ -1107,6 +1127,24 @@ impl Pallet { T::WeightInfo::election_phragmen(weight_candidates, weight_voters, weight_edges) } + + /// Run the election with all required side processes and state updates, if election succeeds. + /// Else, it will emit an `ElectionError` event. + /// + /// Calls the appropriate [`ChangeMembers`] function variant internally. + fn do_election() -> Weight { + T::ElectionProvider::elect().map_err(|e| { + log::error!( + target: "runtime::elections-generic", + "Failed to run election [{:?}].", + e, + ); + Self::deposit_event(Event::ElectionError); + }); + + // TODO(gpestana): return correct election weight + Weight::zero() + } } impl Contains for Pallet { @@ -1153,10 +1191,70 @@ impl ContainsLengthBound for Pallet { } } +impl ElectionDataProvider for Pallet { + type AccountId = T::AccountId; + type BlockNumber = T::BlockNumber; + type MaxVotesPerVoter = ConstU32; + + fn electable_targets( + maybe_max_len: Option, + ) -> frame_election_provider_support::data_provider::Result> { + let mut targets: Vec<_> = + Self::candidates().into_iter().map(|(target, _)| target).collect(); + + if let Some(max_candidates) = maybe_max_len { + // TODO(gpestana):: or should return Err instead? + targets.truncate(max_candidates); + } + // TODO(gpestana): this is a self-weighted function + Ok(targets) + } + + fn electing_voters( + maybe_max_len: Option, + ) -> frame_election_provider_support::data_provider::Result< + Vec>, + > { + // TODO(gpestana): this is a self-weighted function + + let max_votes_per_voter: u32 = Self::MaxVotesPerVoter::get(); + let mut voters_and_stakes = Vec::new(); + + match Voting::::iter().try_for_each(|(voter, Voter { stake, votes, .. })| { + if let Some(max_voters) = maybe_max_len { + if voters_and_stakes.len() > max_voters { + return Err(()); + } + } + let bounded_votes: BoundedVec<_, Self::MaxVotesPerVoter> = + BoundedVec::try_from(votes).map_err(|_| ())?; + + voters_and_stakes.push((voter, stake, bounded_votes)); + Ok(()) + }) { + Ok(_) => (), + Err(_) => return Err(""), // TODO(gpestana): ref proper err + } + + //Ok(voters_and_stakes) // expected u64, got AccountId::Balance + Ok(vec![]) + } + + fn desired_targets() -> frame_election_provider_support::data_provider::Result { + Ok(T::DesiredMembers::get()) + } + + fn next_election_prediction(now: Self::BlockNumber) -> Self::BlockNumber { + // TODO(gpestana): verify (and needed?) + now + (now % T::TermDuration::get()) + } +} + #[cfg(test)] mod tests { use super::*; - use crate as elections_phragmen; + use crate::{self as elections_phragmen, election::DataProvider}; + use frame_election_provider_support::{SequentialPhragmen, onchain}; use frame_support::{ assert_noop, assert_ok, dispatch::DispatchResultWithPostInfo, @@ -1164,7 +1262,8 @@ mod tests { traits::{ConstU32, ConstU64, OnInitialize}, }; use frame_system::ensure_signed; - use sp_core::H256; + use pallet_balances::Pallet; +use sp_core::H256; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, @@ -1277,6 +1376,7 @@ mod tests { pub const ElectionsPhragmenPalletId: LockIdentifier = *b"phrelect"; pub const PhragmenMaxVoters: u32 = 1000; pub const PhragmenMaxCandidates: u32 = 100; + pub const MaxVotesPerVoter: u32 = 16; } impl Config for Test { @@ -1297,11 +1397,51 @@ mod tests { type WeightInfo = (); type MaxVoters = PhragmenMaxVoters; type MaxCandidates = PhragmenMaxCandidates; + //type ElectionProvider = onchain::UnboundedExecution::; + type ElectionProvider = ElectionProviderMock; + } + + pub struct ElectionProviderMock(PhantomData); + + impl ElectionProviderBase for ElectionProviderMock { + type AccountId = u64; + type BlockNumber = u64; + type Error = Error; + type DataProvider = Elections; + + fn ongoing() -> bool { + false + } } + impl ElectionProvider for ElectionProviderMock { + fn elect() -> Result, Self::Error> { + Elections::do_election(); + Ok(vec![]) + } + } + + /* + pub struct SeqPhragmenProvider; + + impl onchain::Config for SeqPhragmenProvider{ + type System = Test; + type Solver = SequentialPhragmen; + type DataProvider = Elections; + type WeightInfo = (); + } + + impl onchain::BoundedConfig for SeqPhragmenProvider{ + type TargetsBound = (); + type VotersBound = (); + } + */ + pub type Block = sp_runtime::generic::Block; pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic; + pub type AccountId = u64; + pub type BlockNumber = u64; frame_support::construct_runtime!( pub enum Test where @@ -2230,8 +2370,10 @@ mod tests { assert_ok!(vote(RuntimeOrigin::signed(4), vec![4], 40)); System::set_block_number(5); + Elections::on_initialize(System::block_number()); + // failing here System::assert_last_event(RuntimeEvent::Elections(super::Event::NewTerm { new_members: vec![(4, 35), (5, 45)], })); From 860ea78d901093651bda81284fb2e45a34eac74d Mon Sep 17 00:00:00 2001 From: gpestana Date: Mon, 24 Oct 2022 09:35:20 +0200 Subject: [PATCH 2/7] using ElectionProvider before removing do_phragmen -- tests passing --- frame/elections-phragmen/src/lib.rs | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 28ae8b4015670..78d3f0f752ecf 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -102,7 +102,7 @@ use core::marker::PhantomData; use codec::{Decode, Encode}; use frame_election_provider_support::{ - ElectionDataProvider, ElectionProvider, ElectionProviderBase, NposSolver, + ElectionDataProvider, ElectionProvider, }; use frame_support::{ traits::{ @@ -908,11 +908,11 @@ impl Pallet { debug_assert!(_remainder.is_zero()); } - /// Run the phragmen election with all required side processes and state updates, if election + /// Run the election with all required side processes and state updates, if election /// succeeds. Else, it will emit an `ElectionError` event. /// /// Calls the appropriate [`ChangeMembers`] function variant internally. - fn do_phragmen() -> Weight { + fn do_election() -> Weight { let desired_seats = T::DesiredMembers::get() as usize; let desired_runners_up = T::DesiredRunnersUp::get() as usize; let num_to_elect = desired_runners_up + desired_seats; @@ -1127,24 +1127,6 @@ impl Pallet { T::WeightInfo::election_phragmen(weight_candidates, weight_voters, weight_edges) } - - /// Run the election with all required side processes and state updates, if election succeeds. - /// Else, it will emit an `ElectionError` event. - /// - /// Calls the appropriate [`ChangeMembers`] function variant internally. - fn do_election() -> Weight { - T::ElectionProvider::elect().map_err(|e| { - log::error!( - target: "runtime::elections-generic", - "Failed to run election [{:?}].", - e, - ); - Self::deposit_event(Event::ElectionError); - }); - - // TODO(gpestana): return correct election weight - Weight::zero() - } } impl Contains for Pallet { @@ -1254,7 +1236,7 @@ impl ElectionDataProvider for Pallet { mod tests { use super::*; use crate::{self as elections_phragmen, election::DataProvider}; - use frame_election_provider_support::{SequentialPhragmen, onchain}; + use frame_election_provider_support::{SequentialPhragmen, onchain, ElectionProviderBase}; use frame_support::{ assert_noop, assert_ok, dispatch::DispatchResultWithPostInfo, @@ -1416,7 +1398,6 @@ use sp_core::H256; impl ElectionProvider for ElectionProviderMock { fn elect() -> Result, Self::Error> { - Elections::do_election(); Ok(vec![]) } } From 70272d9c34c4ec31290534777263b0f57c668a60 Mon Sep 17 00:00:00 2001 From: gpestana Date: Mon, 24 Oct 2022 17:29:03 +0200 Subject: [PATCH 3/7] tests using onchain::ElectionProvider implementation --- frame/elections-phragmen/src/election.rs | 150 -------------- frame/elections-phragmen/src/lib.rs | 241 ++++++++++++++++++----- 2 files changed, 191 insertions(+), 200 deletions(-) delete mode 100644 frame/elections-phragmen/src/election.rs diff --git a/frame/elections-phragmen/src/election.rs b/frame/elections-phragmen/src/election.rs deleted file mode 100644 index 3ad83d956c87f..0000000000000 --- a/frame/elections-phragmen/src/election.rs +++ /dev/null @@ -1,150 +0,0 @@ -use sp_std::{marker::PhantomData, prelude::*}; - -use frame_election_provider_support::{ - weights::WeightInfo, BoundedVec, ElectionDataProvider, ElectionProvider, ElectionProviderBase, - InstantElectionProvider, NposSolver, VoterOf, -}; -use frame_support::traits::Get; -use sp_npos_elections::*; - -type AccountId = ::AccountId; -type BlockNumber = ::BlockNumber; -type Balance = u64; // TODO(gpestana): draft for now; abstract - -/// Errors of the on-chain election. -#[derive(Eq, PartialEq, Debug)] -pub enum Error { - /// An internal error in the NPoS elections crate. - NposElections(sp_npos_elections::Error), - /// Errors from the data provider. - DataProvider(&'static str), -} - -impl From for Error { - fn from(e: sp_npos_elections::Error) -> Self { - Error::NposElections(e) - } -} - -pub trait DataProviderConfig { - type System: frame_system::Config; - type MaxVotesPerVoter: Get; - - fn candidates() -> Vec>; - fn votes_with_stake( - ) -> Vec<(AccountId, crate::Voter, Balance>)>; -} - -pub struct DataProvider(PhantomData); -impl ElectionDataProvider for DataProvider { - type AccountId = AccountId; - type BlockNumber = BlockNumber; - type MaxVotesPerVoter = T::MaxVotesPerVoter; - - fn electable_targets( - maybe_max_len: Option, - ) -> frame_election_provider_support::data_provider::Result> { - Ok(T::candidates()) - } - - fn electing_voters( - maybe_max_len: Option, - ) -> frame_election_provider_support::data_provider::Result>> { - let max_votes_per_voter = T::MaxVotesPerVoter::get() as usize; - let mut voters_and_stakes = Vec::new(); - - match T::votes_with_stake().into_iter().try_for_each( - |(voter, crate::Voter { stake, votes, .. })| { - if let Some(max_voters) = maybe_max_len { - if voters_and_stakes.len() > max_voters { - return Err(()); - } - } - let bounded_votes: BoundedVec<_, T::MaxVotesPerVoter> = - BoundedVec::try_from(votes).map_err(|_| ())?; // TODO(gpestana): ref to proper err - - voters_and_stakes.push((voter, stake, bounded_votes)); - Ok(()) - }, - ) { - Ok(_) => (), - Err(_) => return Err(""), // TODO(gpestana): ref proper err - } - - Ok(voters_and_stakes) - } - - fn desired_targets() -> frame_election_provider_support::data_provider::Result { - // TODO(gpestana): fill in - Ok(10) - } - - fn next_election_prediction(now: Self::BlockNumber) -> Self::BlockNumber { - // TODO(gpestana): fill in - >::block_number() - } -} - -pub trait ElectionConfig { - type System: frame_system::Config; - type DataProvider: ElectionDataProvider< - AccountId = AccountId, - BlockNumber = BlockNumber, - >; - type Solver: NposSolver, Error = sp_npos_elections::Error>; - type WeightInfo: WeightInfo; -} - -pub struct BoundedExecution(PhantomData); - -fn elect_with_bounds( - maybe_max_voters: Option, - maybe_max_targets: Option, -) -> Result>, Error> { - // TODO(gpestana): finsh impl - - // TODO(gpestana): handle unwraps() - let num_to_elect = T::DataProvider::desired_targets().unwrap(); - let candidate_ids = T::DataProvider::electable_targets(maybe_max_targets).unwrap(); - let voters_and_votes = T::DataProvider::electing_voters(maybe_max_voters).unwrap(); - - T::Solver::solve(num_to_elect as usize, candidate_ids, voters_and_votes) - .map(|election_result| {}); - - Ok(vec![]) -} - -impl ElectionProviderBase for BoundedExecution { - type AccountId = AccountId; - type BlockNumber = BlockNumber; - type Error = Error; - type DataProvider = T::DataProvider; - - fn ongoing() -> bool { - return false; - } -} - -impl ElectionProvider for BoundedExecution { - fn elect() -> Result, Self::Error> { - // This should not be called if not in `std` mode (and therefore neither in genesis nor in - // testing) - if cfg!(not(feature = "std")) { - frame_support::log::error!( - "Please use `InstantElectionProvider` instead to provide bounds on election if not in \ - genesis or testing mode" - ); - } - - elect_with_bounds::(None, None) - } -} - -impl InstantElectionProvider for BoundedExecution { - fn elect_with_bounds( - max_voters: usize, - max_targets: usize, - ) -> Result, Self::Error> { - elect_with_bounds::(Some(max_voters), Some(max_targets)) - } -} diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 78d3f0f752ecf..c6ebc6e09a9c5 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -114,10 +114,10 @@ use frame_support::{ BoundedVec, }; use scale_info::TypeInfo; -use sp_npos_elections::{ElectionResult, ExtendedBalance}; +use sp_npos_elections::ExtendedBalance; use sp_runtime::{ - traits::{Saturating, StaticLookup, Zero}, - DispatchError, Perbill, RuntimeDebug, + traits::{Saturating, StaticLookup, Zero, UniqueSaturatedInto}, + DispatchError, RuntimeDebug, }; use sp_std::{cmp::Ordering, prelude::*}; @@ -128,10 +128,6 @@ pub use weights::WeightInfo; /// All migrations. pub mod migrations; -/// The election module, implementing an ElectionProvider and ElectionDataProvider that can -/// be used by this pallet. -pub mod election; - /// The maximum votes allowed per voter. pub const MAXIMUM_VOTE: u32 = 16; @@ -635,6 +631,8 @@ pub mod pallet { InvalidRenouncing, /// Prediction regarding replacement after member removal is wrong. InvalidReplacement, + // Errors from the data provider. + DataProvider, } /// The current elected members. @@ -914,8 +912,6 @@ impl Pallet { /// Calls the appropriate [`ChangeMembers`] function variant internally. fn do_election() -> Weight { let desired_seats = T::DesiredMembers::get() as usize; - let desired_runners_up = T::DesiredRunnersUp::get() as usize; - let num_to_elect = desired_runners_up + desired_seats; let mut candidates_and_deposit = Self::candidates(); // add all the previous members and runners-up as candidates as well. @@ -927,8 +923,7 @@ impl Pallet { } // All of the new winners that come out of phragmen will thus have a deposit recorded. - let candidate_ids = - candidates_and_deposit.iter().map(|(x, _)| x).cloned().collect::>(); + let _ = candidates_and_deposit.iter().map(|(x, _)| x).cloned().collect::>(); // helper closures to deal with balance/stake. let total_issuance = T::Currency::total_issuance(); @@ -959,7 +954,7 @@ impl Pallet { }, } - // used for phragmen. + // used for phragmen. TODO(gpestana): Remove with ElectionProvider! let voters_and_votes = voters_and_stakes .iter() .cloned() @@ -973,8 +968,160 @@ impl Pallet { let weight_voters = voters_and_votes.len() as u32; let weight_edges = num_edges; + + let _ = T::ElectionProvider::elect().map(|winners| { + // this is already sorted by id. + let old_members_ids_sorted = >::take() + .into_iter() + .map(|m| m.who) + .collect::>(); + // this one needs a sort by id. + let mut old_runners_up_ids_sorted = >::take() + .into_iter() + .map(|r| r.who) + .collect::>(); + old_runners_up_ids_sorted.sort(); + + // filter out those who end up with no backing stake. + let mut new_set_with_stake = winners + .into_iter() + .filter_map( + |(m, support)| if support.total.is_zero() { None } else { Some((m, to_balance(support.total))) }, + ) + .collect::)>>(); + + // OPTIMIZATION NOTE: we could bail out here if `new_set.len() == 0`. There + // isn't much left to do. Yet, re-arranging the code would require duplicating + // the slashing of exposed candidates, cleaning any previous members, and so on. + // For now, in favor of readability and veracity, we keep it simple. + + // split new set into winners and runners up. + let split_point = desired_seats.min(new_set_with_stake.len()); + let mut new_members_sorted_by_id = + new_set_with_stake.drain(..split_point).collect::>(); + new_members_sorted_by_id.sort_by(|i, j| i.0.cmp(&j.0)); + + // all the rest will be runners-up + new_set_with_stake.reverse(); + let new_runners_up_sorted_by_rank = new_set_with_stake; + 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(); + + // 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 = 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().map(|(vote_position, who)| { + (MAXIMUM_VOTE - (vote_position as u32), who) + }) { + if let Ok(i) = prime_votes.binary_search_by_key(&who, |k| k.0) { + prime_votes[i].1 = prime_votes[i] + .1 + .saturating_add(stake.saturating_mul(vote_multiplier.into())); + } + } + } + + // We then select the new member with the highest weighted stake. In the case of + // a tie, the last person in the list with the tied score is selected. This is + // 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_sorted_by_id is sorted by account id. + let new_members_ids_sorted = new_members_sorted_by_id + .iter() + .map(|(m, _)| m.clone()) + .collect::>(); + + // report member changes. We compute diff because we need the outgoing list. + let (incoming, outgoing) = T::ChangeMembers::compute_members_diff_sorted( + &new_members_ids_sorted, + &old_members_ids_sorted, + ); + T::ChangeMembers::change_members_sorted( + &incoming, + &outgoing, + &new_members_ids_sorted, + ); + T::ChangeMembers::set_prime(prime); + + // All candidates/members/runners-up who are no longer retaining a position as a + // seat holder will lose their bond. + candidates_and_deposit.iter().for_each(|(c, d)| { + 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, *d); + T::LoserCandidate::on_unbalanced(imbalance); + Self::deposit_event(Event::CandidateSlashed { + candidate: c.clone(), + amount: *d, + }); + } + }); + + // write final values to storage. + let deposit_of_candidate = |x: &T::AccountId| -> BalanceOf { + // defensive-only. This closure is used against the new members and new + // runners-up, both of which are phragmen winners and thus must have + // deposit. + candidates_and_deposit + .iter() + .find_map(|(c, d)| if c == x { Some(*d) } else { None }) + .defensive_unwrap_or_default() + }; + // fetch deposits from the one recorded one. This will make sure that a + // candidate who submitted candidacy before a change to candidacy deposit will + // have the correct amount recorded. + >::put( + new_members_sorted_by_id + .iter() + .map(|(who, stake)| SeatHolder { + deposit: deposit_of_candidate(who), + who: who.clone(), + stake: *stake, + }) + .collect::>(), + ); + >::put( + new_runners_up_sorted_by_rank + .into_iter() + .map(|(who, stake)| SeatHolder { + deposit: deposit_of_candidate(&who), + who, + stake, + }) + .collect::>(), + ); + + // clean candidates. + >::kill(); + + Self::deposit_event(Event::NewTerm { new_members: new_members_sorted_by_id }); + >::mutate(|v| *v += 1); + }) + .map_err(|e| { + log::error!( + target: "runtime::elections-phragmen", + "Failed to run election [{:?}].", + e, + ); + Self::deposit_event(Event::ElectionError); + + }); + + /* let _ = - //ElectionProvider >> T::Solver::solve(num_to_elect, candidate_ids, voters_and_votes) sp_npos_elections::seq_phragmen(num_to_elect, candidate_ids, voters_and_votes, None) .map(|ElectionResult:: { winners, assignments: _ }| { // this is already sorted by id. @@ -1124,6 +1271,7 @@ impl Pallet { ); Self::deposit_event(Event::ElectionError); }); + */ T::WeightInfo::election_phragmen(weight_candidates, weight_voters, weight_edges) } @@ -1173,6 +1321,7 @@ impl ContainsLengthBound for Pallet { } } + impl ElectionDataProvider for Pallet { type AccountId = T::AccountId; type BlockNumber = T::BlockNumber; @@ -1181,6 +1330,8 @@ impl ElectionDataProvider for Pallet { fn electable_targets( maybe_max_len: Option, ) -> frame_election_provider_support::data_provider::Result> { + // TODO(gpestana): this is a self-weighted function + let mut targets: Vec<_> = Self::candidates().into_iter().map(|(target, _)| target).collect(); @@ -1188,10 +1339,15 @@ impl ElectionDataProvider for Pallet { // TODO(gpestana):: or should return Err instead? targets.truncate(max_candidates); } - // TODO(gpestana): this is a self-weighted function + + println!(">> electable_targets"); + println!(" {:?}", targets); + println!("---------------------"); + Ok(targets) } + fn electing_voters( maybe_max_len: Option, ) -> frame_election_provider_support::data_provider::Result< @@ -1199,7 +1355,7 @@ impl ElectionDataProvider for Pallet { > { // TODO(gpestana): this is a self-weighted function - let max_votes_per_voter: u32 = Self::MaxVotesPerVoter::get(); + //let max_votes_per_voter: u32 = Self::MaxVotesPerVoter::get(); let mut voters_and_stakes = Vec::new(); match Voting::::iter().try_for_each(|(voter, Voter { stake, votes, .. })| { @@ -1211,19 +1367,27 @@ impl ElectionDataProvider for Pallet { let bounded_votes: BoundedVec<_, Self::MaxVotesPerVoter> = BoundedVec::try_from(votes).map_err(|_| ())?; - voters_and_stakes.push((voter, stake, bounded_votes)); + voters_and_stakes.push((voter, stake.unique_saturated_into(), bounded_votes)); Ok(()) }) { Ok(_) => (), Err(_) => return Err(""), // TODO(gpestana): ref proper err } + println!(">> electing_voters"); + println!(" {:?}", voters_and_stakes); + println!("---------------------"); + + //Ok(voters_and_stakes) // expected u64, got AccountId::Balance - Ok(vec![]) + Ok(voters_and_stakes) + } fn desired_targets() -> frame_election_provider_support::data_provider::Result { - Ok(T::DesiredMembers::get()) + let desired_seats = T::DesiredMembers::get() as usize; + let desired_runners_up = T::DesiredRunnersUp::get() as usize; + Ok((desired_runners_up + desired_seats) as u32) } fn next_election_prediction(now: Self::BlockNumber) -> Self::BlockNumber { @@ -1235,8 +1399,8 @@ impl ElectionDataProvider for Pallet { #[cfg(test)] mod tests { use super::*; - use crate::{self as elections_phragmen, election::DataProvider}; - use frame_election_provider_support::{SequentialPhragmen, onchain, ElectionProviderBase}; + use crate::{self as elections_phragmen}; + use frame_election_provider_support::{SequentialPhragmen, onchain}; use frame_support::{ assert_noop, assert_ok, dispatch::DispatchResultWithPostInfo, @@ -1244,12 +1408,11 @@ mod tests { traits::{ConstU32, ConstU64, OnInitialize}, }; use frame_system::ensure_signed; - use pallet_balances::Pallet; -use sp_core::H256; + use sp_core::H256; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, - BuildStorage, + BuildStorage, Perbill, }; use substrate_test_utils::assert_eq_uvec; @@ -1308,6 +1471,8 @@ use sp_core::H256; pub static TermDuration: u64 = 5; pub static Members: Vec = vec![]; pub static Prime: Option = None; + pub static MaxVoters: u32 = 100; + pub static MaxTargets: u32 = 100; } pub struct TestChangeMembers; @@ -1379,32 +1544,10 @@ use sp_core::H256; type WeightInfo = (); type MaxVoters = PhragmenMaxVoters; type MaxCandidates = PhragmenMaxCandidates; - //type ElectionProvider = onchain::UnboundedExecution::; - type ElectionProvider = ElectionProviderMock; + type ElectionProvider = onchain::BoundedExecution::; } - pub struct ElectionProviderMock(PhantomData); - - impl ElectionProviderBase for ElectionProviderMock { - type AccountId = u64; - type BlockNumber = u64; - type Error = Error; - type DataProvider = Elections; - - fn ongoing() -> bool { - false - } - } - - impl ElectionProvider for ElectionProviderMock { - fn elect() -> Result, Self::Error> { - Ok(vec![]) - } - } - - /* pub struct SeqPhragmenProvider; - impl onchain::Config for SeqPhragmenProvider{ type System = Test; type Solver = SequentialPhragmen; @@ -1413,16 +1556,14 @@ use sp_core::H256; } impl onchain::BoundedConfig for SeqPhragmenProvider{ - type TargetsBound = (); - type VotersBound = (); + type TargetsBound = MaxTargets; + type VotersBound = MaxVoters; } - */ pub type Block = sp_runtime::generic::Block; pub type UncheckedExtrinsic = sp_runtime::generic::UncheckedExtrinsic; pub type AccountId = u64; - pub type BlockNumber = u64; frame_support::construct_runtime!( pub enum Test where From 5dc4ed9bc57eedffb771c10a4be0d67a0dd2363a Mon Sep 17 00:00:00 2001 From: gpestana Date: Tue, 25 Oct 2022 11:18:28 +0200 Subject: [PATCH 4/7] tests passing --- frame/elections-phragmen/src/lib.rs | 525 ++++++------------ frame/elections-phragmen/src/migrations/v4.rs | 2 +- 2 files changed, 179 insertions(+), 348 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index c6ebc6e09a9c5..b4d712d93e7df 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -101,9 +101,7 @@ use core::marker::PhantomData; use codec::{Decode, Encode}; -use frame_election_provider_support::{ - ElectionDataProvider, ElectionProvider, -}; +use frame_election_provider_support::{ElectionDataProvider, ElectionProvider}; use frame_support::{ traits::{ defensive_prelude::*, ChangeMembers, ConstU32, Contains, ContainsLengthBound, Currency, @@ -116,7 +114,7 @@ use frame_support::{ use scale_info::TypeInfo; use sp_npos_elections::ExtendedBalance; use sp_runtime::{ - traits::{Saturating, StaticLookup, Zero, UniqueSaturatedInto}, + traits::{Saturating, StaticLookup, UniqueSaturatedInto, Zero}, DispatchError, RuntimeDebug, }; use sp_std::{cmp::Ordering, prelude::*}; @@ -800,7 +798,7 @@ impl Pallet { // overlap. This can never happen. If so, it seems like our intended replacement // is already a member, so not much more to do. log::error!( - target: "runtime::elections-phragmen", + target: "runtime::elections", "A member seems to also be a runner-up.", ); } @@ -912,6 +910,11 @@ impl Pallet { /// Calls the appropriate [`ChangeMembers`] function variant internally. fn do_election() -> Weight { let desired_seats = T::DesiredMembers::get() as usize; + let max_voters = ::MaxVoters::get() as usize; + + // helper closures to deal with balance/stake. + let total_issuance = T::Currency::total_issuance(); + let to_balance = |e: ExtendedBalance| T::CurrencyToVote::to_currency(e, total_issuance); let mut candidates_and_deposit = Self::candidates(); // add all the previous members and runners-up as candidates as well. @@ -922,18 +925,9 @@ impl Pallet { return T::DbWeight::get().reads(3); } - // All of the new winners that come out of phragmen will thus have a deposit recorded. + // All of the new winners that come out of the election will thus have a deposit recorded. let _ = candidates_and_deposit.iter().map(|(x, _)| x).cloned().collect::>(); - // helper closures to deal with balance/stake. - let total_issuance = T::Currency::total_issuance(); - let to_votes = |b: BalanceOf| T::CurrencyToVote::to_vote(b, total_issuance); - let to_balance = |e: ExtendedBalance| T::CurrencyToVote::to_currency(e, total_issuance); - - let mut num_edges: u32 = 0; - - let max_voters = ::MaxVoters::get() as usize; - // used for prime election. let mut voters_and_stakes = Vec::new(); match Voting::::iter().try_for_each(|(voter, Voter { stake, votes, .. })| { if voters_and_stakes.len() < max_voters { @@ -946,7 +940,7 @@ impl Pallet { Ok(_) => (), Err(_) => { log::error!( - target: "runtime::elections-phragmen", + target: "runtime::elections", "Failed to run election. Number of voters exceeded", ); Self::deposit_event(Event::ElectionError); @@ -954,326 +948,167 @@ impl Pallet { }, } - // used for phragmen. TODO(gpestana): Remove with ElectionProvider! - let voters_and_votes = voters_and_stakes - .iter() - .cloned() - .map(|(voter, stake, votes)| { - num_edges = num_edges.saturating_add(votes.len() as u32); - (voter, to_votes(stake), votes) - }) - .collect::>(); - - let weight_candidates = candidates_and_deposit.len() as u32; - let weight_voters = voters_and_votes.len() as u32; - let weight_edges = num_edges; - - - let _ = T::ElectionProvider::elect().map(|winners| { - // this is already sorted by id. - let old_members_ids_sorted = >::take() - .into_iter() - .map(|m| m.who) - .collect::>(); - // this one needs a sort by id. - let mut old_runners_up_ids_sorted = >::take() - .into_iter() - .map(|r| r.who) - .collect::>(); - old_runners_up_ids_sorted.sort(); - - // filter out those who end up with no backing stake. - let mut new_set_with_stake = winners - .into_iter() - .filter_map( - |(m, support)| if support.total.is_zero() { None } else { Some((m, to_balance(support.total))) }, - ) - .collect::)>>(); - - // OPTIMIZATION NOTE: we could bail out here if `new_set.len() == 0`. There - // isn't much left to do. Yet, re-arranging the code would require duplicating - // the slashing of exposed candidates, cleaning any previous members, and so on. - // For now, in favor of readability and veracity, we keep it simple. - - // split new set into winners and runners up. - let split_point = desired_seats.min(new_set_with_stake.len()); - let mut new_members_sorted_by_id = - new_set_with_stake.drain(..split_point).collect::>(); - new_members_sorted_by_id.sort_by(|i, j| i.0.cmp(&j.0)); - - // all the rest will be runners-up - new_set_with_stake.reverse(); - let new_runners_up_sorted_by_rank = new_set_with_stake; - 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(); - - // 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 = 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().map(|(vote_position, who)| { - (MAXIMUM_VOTE - (vote_position as u32), who) - }) { - if let Ok(i) = prime_votes.binary_search_by_key(&who, |k| k.0) { - prime_votes[i].1 = prime_votes[i] - .1 - .saturating_add(stake.saturating_mul(vote_multiplier.into())); - } + let _ = T::ElectionProvider::elect() + .map(|mut winners| { + // sort winners by stake + winners.sort_by(|i, j| j.1.total.cmp(&i.1.total)); + + // this is already sorted by id. + let old_members_ids_sorted = + >::take().into_iter().map(|m| m.who).collect::>(); + // this one needs a sort by id. + let mut old_runners_up_ids_sorted = >::take() + .into_iter() + .map(|r| r.who) + .collect::>(); + old_runners_up_ids_sorted.sort(); + + // filter out those who end up with no backing stake. + let mut new_set_with_stake = winners + .into_iter() + .filter_map(|(m, support)| { + if support.total.is_zero() { + None + } else { + Some((m, to_balance(support.total))) } - } - - // We then select the new member with the highest weighted stake. In the case of - // a tie, the last person in the list with the tied score is selected. This is - // 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_sorted_by_id is sorted by account id. - let new_members_ids_sorted = new_members_sorted_by_id + }) + .collect::)>>(); + + // OPTIMIZATION NOTE: we could bail out here if `new_set.len() == 0`. There + // isn't much left to do. Yet, re-arranging the code would require duplicating + // the slashing of exposed candidates, cleaning any previous members, and so on. + // For now, in favor of readability and veracity, we keep it simple. + + // split new set into winners and runners up. + let split_point = desired_seats.min(new_set_with_stake.len()); + let mut new_members_sorted_by_id = + new_set_with_stake.drain(..split_point).collect::>(); + + new_members_sorted_by_id.sort_by(|i, j| i.0.cmp(&j.0)); + + // all the rest will be runners-up + new_set_with_stake.reverse(); + let new_runners_up_sorted_by_rank = new_set_with_stake; + 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(); + + // 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 = 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() - .map(|(m, _)| m.clone()) - .collect::>(); - - // report member changes. We compute diff because we need the outgoing list. - let (incoming, outgoing) = T::ChangeMembers::compute_members_diff_sorted( - &new_members_ids_sorted, - &old_members_ids_sorted, - ); - T::ChangeMembers::change_members_sorted( - &incoming, - &outgoing, - &new_members_ids_sorted, - ); - T::ChangeMembers::set_prime(prime); - - // All candidates/members/runners-up who are no longer retaining a position as a - // seat holder will lose their bond. - candidates_and_deposit.iter().for_each(|(c, d)| { - 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, *d); - T::LoserCandidate::on_unbalanced(imbalance); - Self::deposit_event(Event::CandidateSlashed { - candidate: c.clone(), - amount: *d, - }); + .enumerate() + .map(|(vote_position, who)| (MAXIMUM_VOTE - (vote_position as u32), who)) + { + if let Ok(i) = prime_votes.binary_search_by_key(&who, |k| k.0) { + prime_votes[i].1 = prime_votes[i] + .1 + .saturating_add(stake.saturating_mul(vote_multiplier.into())); } - }); - - // write final values to storage. - let deposit_of_candidate = |x: &T::AccountId| -> BalanceOf { - // defensive-only. This closure is used against the new members and new - // runners-up, both of which are phragmen winners and thus must have - // deposit. - candidates_and_deposit - .iter() - .find_map(|(c, d)| if c == x { Some(*d) } else { None }) - .defensive_unwrap_or_default() - }; - // fetch deposits from the one recorded one. This will make sure that a - // candidate who submitted candidacy before a change to candidacy deposit will - // have the correct amount recorded. - >::put( - new_members_sorted_by_id - .iter() - .map(|(who, stake)| SeatHolder { - deposit: deposit_of_candidate(who), - who: who.clone(), - stake: *stake, - }) - .collect::>(), - ); - >::put( - new_runners_up_sorted_by_rank - .into_iter() - .map(|(who, stake)| SeatHolder { - deposit: deposit_of_candidate(&who), - who, - stake, - }) - .collect::>(), - ); - - // clean candidates. - >::kill(); - - Self::deposit_event(Event::NewTerm { new_members: new_members_sorted_by_id }); - >::mutate(|v| *v += 1); - }) - .map_err(|e| { - log::error!( - target: "runtime::elections-phragmen", - "Failed to run election [{:?}].", - e, - ); - Self::deposit_event(Event::ElectionError); - - }); - - /* - let _ = - sp_npos_elections::seq_phragmen(num_to_elect, candidate_ids, voters_and_votes, None) - .map(|ElectionResult:: { winners, assignments: _ }| { - // this is already sorted by id. - let old_members_ids_sorted = >::take() - .into_iter() - .map(|m| m.who) - .collect::>(); - // this one needs a sort by id. - let mut old_runners_up_ids_sorted = >::take() - .into_iter() - .map(|r| r.who) - .collect::>(); - old_runners_up_ids_sorted.sort(); + } + } - // filter out those who end up with no backing stake. - let mut new_set_with_stake = winners - .into_iter() - .filter_map( - |(m, b)| if b.is_zero() { None } else { Some((m, to_balance(b))) }, - ) - .collect::)>>(); - - // OPTIMIZATION NOTE: we could bail out here if `new_set.len() == 0`. There - // isn't much left to do. Yet, re-arranging the code would require duplicating - // the slashing of exposed candidates, cleaning any previous members, and so on. - // For now, in favor of readability and veracity, we keep it simple. - - // split new set into winners and runners up. - let split_point = desired_seats.min(new_set_with_stake.len()); - let mut new_members_sorted_by_id = - new_set_with_stake.drain(..split_point).collect::>(); - new_members_sorted_by_id.sort_by(|i, j| i.0.cmp(&j.0)); - - // all the rest will be runners-up - new_set_with_stake.reverse(); - let new_runners_up_sorted_by_rank = new_set_with_stake; - 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(); - - // 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 = 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().map(|(vote_position, who)| { - (MAXIMUM_VOTE - (vote_position as u32), who) - }) { - if let Ok(i) = prime_votes.binary_search_by_key(&who, |k| k.0) { - prime_votes[i].1 = prime_votes[i] - .1 - .saturating_add(stake.saturating_mul(vote_multiplier.into())); - } - } + // We then select the new member with the highest weighted stake. In the case of + // a tie, the last person in the list with the tied score is selected. This is + // 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_sorted_by_id is sorted by account id. + let new_members_ids_sorted = new_members_sorted_by_id + .iter() + .map(|(m, _)| m.clone()) + .collect::>(); + + // report member changes. We compute diff because we need the outgoing list. + let (incoming, outgoing) = T::ChangeMembers::compute_members_diff_sorted( + &new_members_ids_sorted, + &old_members_ids_sorted, + ); + T::ChangeMembers::change_members_sorted( + &incoming, + &outgoing, + &new_members_ids_sorted, + ); + T::ChangeMembers::set_prime(prime); + + // All candidates/members/runners-up who are no longer retaining a position as a + // seat holder will lose their bond. + candidates_and_deposit.iter().for_each(|(c, d)| { + 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, *d); + T::LoserCandidate::on_unbalanced(imbalance); + Self::deposit_event(Event::CandidateSlashed { + candidate: c.clone(), + amount: *d, + }); } - // We then select the new member with the highest weighted stake. In the case of - // a tie, the last person in the list with the tied score is selected. This is - // 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_sorted_by_id is sorted by account id. - let new_members_ids_sorted = new_members_sorted_by_id + // write final values to storage. + let deposit_of_candidate = |x: &T::AccountId| -> BalanceOf { + // defensive-only. This closure is used against the new members and new + // runners-up, both of which are election winners and thus must have + // deposit. + candidates_and_deposit .iter() - .map(|(m, _)| m.clone()) - .collect::>(); - - // report member changes. We compute diff because we need the outgoing list. - let (incoming, outgoing) = T::ChangeMembers::compute_members_diff_sorted( - &new_members_ids_sorted, - &old_members_ids_sorted, - ); - T::ChangeMembers::change_members_sorted( - &incoming, - &outgoing, - &new_members_ids_sorted, - ); - T::ChangeMembers::set_prime(prime); - - // All candidates/members/runners-up who are no longer retaining a position as a - // seat holder will lose their bond. - candidates_and_deposit.iter().for_each(|(c, d)| { - 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, *d); - T::LoserCandidate::on_unbalanced(imbalance); - Self::deposit_event(Event::CandidateSlashed { - candidate: c.clone(), - amount: *d, - }); - } - }); + .find_map(|(c, d)| if c == x { Some(*d) } else { None }) + .defensive_unwrap_or_default() + }; - // write final values to storage. - let deposit_of_candidate = |x: &T::AccountId| -> BalanceOf { - // defensive-only. This closure is used against the new members and new - // runners-up, both of which are phragmen winners and thus must have - // deposit. - candidates_and_deposit - .iter() - .find_map(|(c, d)| if c == x { Some(*d) } else { None }) - .defensive_unwrap_or_default() - }; - // fetch deposits from the one recorded one. This will make sure that a - // candidate who submitted candidacy before a change to candidacy deposit will - // have the correct amount recorded. - >::put( - new_members_sorted_by_id - .iter() - .map(|(who, stake)| SeatHolder { - deposit: deposit_of_candidate(who), - who: who.clone(), - stake: *stake, - }) - .collect::>(), - ); - >::put( - new_runners_up_sorted_by_rank - .into_iter() - .map(|(who, stake)| SeatHolder { - deposit: deposit_of_candidate(&who), - who, - stake, - }) - .collect::>(), - ); + // fetch deposits from the one recorded one. This will make sure that a + // candidate who submitted candidacy before a change to candidacy deposit will + // have the correct amount recorded. + >::put( + new_members_sorted_by_id + .iter() + .map(|(who, stake)| SeatHolder { + deposit: deposit_of_candidate(who), + who: who.clone(), + stake: *stake, + }) + .collect::>(), + ); + >::put( + new_runners_up_sorted_by_rank + .into_iter() + .map(|(who, stake)| SeatHolder { + deposit: deposit_of_candidate(&who), + who, + stake, + }) + .collect::>(), + ); - // clean candidates. - >::kill(); + // clean candidates. + >::kill(); - Self::deposit_event(Event::NewTerm { new_members: new_members_sorted_by_id }); - >::mutate(|v| *v += 1); - }) - .map_err(|e| { - log::error!( - target: "runtime::elections-phragmen", - "Failed to run election [{:?}].", - e, - ); - Self::deposit_event(Event::ElectionError); - }); - */ + Self::deposit_event(Event::NewTerm { new_members: new_members_sorted_by_id }); + >::mutate(|v| *v += 1); + }) + .map_err(|e| { + log::error!( + target: "runtime::elections", + "Failed to run election [{:?}].", + e, + ); + Self::deposit_event(Event::ElectionError); + }); - T::WeightInfo::election_phragmen(weight_candidates, weight_voters, weight_edges) + // TODO(gpestana): return weight for general election, not phragmen + Weight::zero() } } @@ -1321,7 +1156,6 @@ impl ContainsLengthBound for Pallet { } } - impl ElectionDataProvider for Pallet { type AccountId = T::AccountId; type BlockNumber = T::BlockNumber; @@ -1332,22 +1166,20 @@ impl ElectionDataProvider for Pallet { ) -> frame_election_provider_support::data_provider::Result> { // TODO(gpestana): this is a self-weighted function + let mut candidates_and_deposit = Self::candidates(); + // add all the previous members and runners-up as candidates as well. + candidates_and_deposit.append(&mut Self::implicit_candidates_with_deposit()); let mut targets: Vec<_> = - Self::candidates().into_iter().map(|(target, _)| target).collect(); + candidates_and_deposit.into_iter().map(|(target, _)| target).collect(); if let Some(max_candidates) = maybe_max_len { // TODO(gpestana):: or should return Err instead? targets.truncate(max_candidates); } - println!(">> electable_targets"); - println!(" {:?}", targets); - println!("---------------------"); - Ok(targets) } - fn electing_voters( maybe_max_len: Option, ) -> frame_election_provider_support::data_provider::Result< @@ -1355,12 +1187,11 @@ impl ElectionDataProvider for Pallet { > { // TODO(gpestana): this is a self-weighted function - //let max_votes_per_voter: u32 = Self::MaxVotesPerVoter::get(); let mut voters_and_stakes = Vec::new(); match Voting::::iter().try_for_each(|(voter, Voter { stake, votes, .. })| { if let Some(max_voters) = maybe_max_len { - if voters_and_stakes.len() > max_voters { + if voters_and_stakes.len() >= max_voters { return Err(()); } } @@ -1371,17 +1202,17 @@ impl ElectionDataProvider for Pallet { Ok(()) }) { Ok(_) => (), - Err(_) => return Err(""), // TODO(gpestana): ref proper err + Err(_) => { + log::error!( + target: "runtime::elections", + "Failed to run election. Number of voters exceeded", + ); + Self::deposit_event(Event::ElectionError); + //return T::DbWeight::get().reads(3 + max_voters as u64); // TODO(gpestana)) + }, } - println!(">> electing_voters"); - println!(" {:?}", voters_and_stakes); - println!("---------------------"); - - - //Ok(voters_and_stakes) // expected u64, got AccountId::Balance Ok(voters_and_stakes) - } fn desired_targets() -> frame_election_provider_support::data_provider::Result { @@ -1391,7 +1222,7 @@ impl ElectionDataProvider for Pallet { } fn next_election_prediction(now: Self::BlockNumber) -> Self::BlockNumber { - // TODO(gpestana): verify (and needed?) + // TODO(gpestana): verify now + (now % T::TermDuration::get()) } } @@ -1400,7 +1231,7 @@ impl ElectionDataProvider for Pallet { mod tests { use super::*; use crate::{self as elections_phragmen}; - use frame_election_provider_support::{SequentialPhragmen, onchain}; + use frame_election_provider_support::{onchain, SequentialPhragmen}; use frame_support::{ assert_noop, assert_ok, dispatch::DispatchResultWithPostInfo, @@ -1544,18 +1375,18 @@ mod tests { type WeightInfo = (); type MaxVoters = PhragmenMaxVoters; type MaxCandidates = PhragmenMaxCandidates; - type ElectionProvider = onchain::BoundedExecution::; + type ElectionProvider = onchain::BoundedExecution; } pub struct SeqPhragmenProvider; - impl onchain::Config for SeqPhragmenProvider{ + impl onchain::Config for SeqPhragmenProvider { type System = Test; type Solver = SequentialPhragmen; type DataProvider = Elections; type WeightInfo = (); } - impl onchain::BoundedConfig for SeqPhragmenProvider{ + impl onchain::BoundedConfig for SeqPhragmenProvider { type TargetsBound = MaxTargets; type VotersBound = MaxVoters; } diff --git a/frame/elections-phragmen/src/migrations/v4.rs b/frame/elections-phragmen/src/migrations/v4.rs index 76ef630706c50..4a840a7ab9ce8 100644 --- a/frame/elections-phragmen/src/migrations/v4.rs +++ b/frame/elections-phragmen/src/migrations/v4.rs @@ -38,7 +38,7 @@ pub fn migrate>(new_pallet_name: N) -> Weight { target: "runtime::elections-phragmen", "New pallet name is equal to the old prefix. No migration needs to be done.", ); - return Weight::zero() + return Weight::zero(); } let storage_version = StorageVersion::get::>(); log::info!( From 3fe8093a288f83995b2da3cb143598260c80173a Mon Sep 17 00:00:00 2001 From: gpestana Date: Tue, 25 Oct 2022 12:17:34 +0200 Subject: [PATCH 5/7] fmt; weights (unfinished) --- frame/elections-phragmen/src/lib.rs | 345 ++++++++++++------------ frame/elections-phragmen/src/weights.rs | 40 ++- 2 files changed, 212 insertions(+), 173 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index b4d712d93e7df..d8ff15492dd4b 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -101,7 +101,9 @@ use core::marker::PhantomData; use codec::{Decode, Encode}; -use frame_election_provider_support::{ElectionDataProvider, ElectionProvider}; +use frame_election_provider_support::{ + ElectionDataProvider, ElectionProvider, +}; use frame_support::{ traits::{ defensive_prelude::*, ChangeMembers, ConstU32, Contains, ContainsLengthBound, Currency, @@ -110,11 +112,12 @@ use frame_support::{ }, weights::Weight, BoundedVec, + dispatch::DispatchClass, }; use scale_info::TypeInfo; use sp_npos_elections::ExtendedBalance; use sp_runtime::{ - traits::{Saturating, StaticLookup, UniqueSaturatedInto, Zero}, + traits::{Saturating, StaticLookup, Zero, UniqueSaturatedInto}, DispatchError, RuntimeDebug, }; use sp_std::{cmp::Ordering, prelude::*}; @@ -948,168 +951,178 @@ impl Pallet { }, } - let _ = T::ElectionProvider::elect() - .map(|mut winners| { - // sort winners by stake - winners.sort_by(|i, j| j.1.total.cmp(&i.1.total)); - - // this is already sorted by id. - let old_members_ids_sorted = - >::take().into_iter().map(|m| m.who).collect::>(); - // this one needs a sort by id. - let mut old_runners_up_ids_sorted = >::take() - .into_iter() - .map(|r| r.who) - .collect::>(); - old_runners_up_ids_sorted.sort(); - - // filter out those who end up with no backing stake. - let mut new_set_with_stake = winners - .into_iter() - .filter_map(|(m, support)| { - if support.total.is_zero() { - None - } else { - Some((m, to_balance(support.total))) - } - }) - .collect::)>>(); - - // OPTIMIZATION NOTE: we could bail out here if `new_set.len() == 0`. There - // isn't much left to do. Yet, re-arranging the code would require duplicating - // the slashing of exposed candidates, cleaning any previous members, and so on. - // For now, in favor of readability and veracity, we keep it simple. - - // split new set into winners and runners up. - let split_point = desired_seats.min(new_set_with_stake.len()); - let mut new_members_sorted_by_id = - new_set_with_stake.drain(..split_point).collect::>(); - - new_members_sorted_by_id.sort_by(|i, j| i.0.cmp(&j.0)); - - // all the rest will be runners-up - new_set_with_stake.reverse(); - let new_runners_up_sorted_by_rank = new_set_with_stake; - 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(); - - // 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 = 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 + let _ = T::ElectionProvider::elect().map(|mut winners| { + // sort winners by stake + winners.sort_by(|i, j| j.1.total.cmp(&i.1.total)); + + // this is already sorted by id. + let old_members_ids_sorted = >::take() + .into_iter() + .map(|m| m.who) + .collect::>(); + // this one needs a sort by id. + let mut old_runners_up_ids_sorted = >::take() + .into_iter() + .map(|r| r.who) + .collect::>(); + old_runners_up_ids_sorted.sort(); + + // filter out those who end up with no backing stake. + let mut new_set_with_stake = winners + .into_iter() + .filter_map( + |(m, support)| if support.total.is_zero() { None } else { Some((m, to_balance(support.total))) }, + ) + .collect::)>>(); + + // OPTIMIZATION NOTE: we could bail out here if `new_set.len() == 0`. There + // isn't much left to do. Yet, re-arranging the code would require duplicating + // the slashing of exposed candidates, cleaning any previous members, and so on. + // For now, in favor of readability and veracity, we keep it simple. + + // split new set into winners and runners up. + let split_point = desired_seats.min(new_set_with_stake.len()); + let mut new_members_sorted_by_id = + new_set_with_stake.drain(..split_point).collect::>(); + + new_members_sorted_by_id.sort_by(|i, j| i.0.cmp(&j.0)); + + // all the rest will be runners-up + new_set_with_stake.reverse(); + let new_runners_up_sorted_by_rank = new_set_with_stake; + let mut new_runners_up_ids_sorted = new_runners_up_sorted_by_rank .iter() - .enumerate() - .map(|(vote_position, who)| (MAXIMUM_VOTE - (vote_position as u32), who)) - { - if let Ok(i) = prime_votes.binary_search_by_key(&who, |k| k.0) { - prime_votes[i].1 = prime_votes[i] - .1 - .saturating_add(stake.saturating_mul(vote_multiplier.into())); + .map(|(r, _)| r.clone()) + .collect::>(); + new_runners_up_ids_sorted.sort(); + + // 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 = 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().map(|(vote_position, who)| { + (MAXIMUM_VOTE - (vote_position as u32), who) + }) { + if let Ok(i) = prime_votes.binary_search_by_key(&who, |k| k.0) { + prime_votes[i].1 = prime_votes[i] + .1 + .saturating_add(stake.saturating_mul(vote_multiplier.into())); + } } } - } - // We then select the new member with the highest weighted stake. In the case of - // a tie, the last person in the list with the tied score is selected. This is - // 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_sorted_by_id is sorted by account id. - let new_members_ids_sorted = new_members_sorted_by_id - .iter() - .map(|(m, _)| m.clone()) - .collect::>(); - - // report member changes. We compute diff because we need the outgoing list. - let (incoming, outgoing) = T::ChangeMembers::compute_members_diff_sorted( - &new_members_ids_sorted, - &old_members_ids_sorted, - ); - T::ChangeMembers::change_members_sorted( - &incoming, - &outgoing, - &new_members_ids_sorted, - ); - T::ChangeMembers::set_prime(prime); - - // All candidates/members/runners-up who are no longer retaining a position as a - // seat holder will lose their bond. - candidates_and_deposit.iter().for_each(|(c, d)| { - 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, *d); - T::LoserCandidate::on_unbalanced(imbalance); - Self::deposit_event(Event::CandidateSlashed { - candidate: c.clone(), - amount: *d, - }); - } - }); + // We then select the new member with the highest weighted stake. In the case of + // a tie, the last person in the list with the tied score is selected. This is + // 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()); - // write final values to storage. - let deposit_of_candidate = |x: &T::AccountId| -> BalanceOf { - // defensive-only. This closure is used against the new members and new - // runners-up, both of which are election winners and thus must have - // deposit. - candidates_and_deposit + // new_members_sorted_by_id is sorted by account id. + let new_members_ids_sorted = new_members_sorted_by_id .iter() - .find_map(|(c, d)| if c == x { Some(*d) } else { None }) - .defensive_unwrap_or_default() - }; + .map(|(m, _)| m.clone()) + .collect::>(); - // fetch deposits from the one recorded one. This will make sure that a - // candidate who submitted candidacy before a change to candidacy deposit will - // have the correct amount recorded. - >::put( - new_members_sorted_by_id - .iter() - .map(|(who, stake)| SeatHolder { - deposit: deposit_of_candidate(who), - who: who.clone(), - stake: *stake, - }) - .collect::>(), - ); - >::put( - new_runners_up_sorted_by_rank - .into_iter() - .map(|(who, stake)| SeatHolder { - deposit: deposit_of_candidate(&who), - who, - stake, - }) - .collect::>(), - ); + // report member changes. We compute diff because we need the outgoing list. + let (incoming, outgoing) = T::ChangeMembers::compute_members_diff_sorted( + &new_members_ids_sorted, + &old_members_ids_sorted, + ); + T::ChangeMembers::change_members_sorted( + &incoming, + &outgoing, + &new_members_ids_sorted, + ); + T::ChangeMembers::set_prime(prime); + + // All candidates/members/runners-up who are no longer retaining a position as a + // seat holder will lose their bond. + candidates_and_deposit.iter().for_each(|(c, d)| { + 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, *d); + T::LoserCandidate::on_unbalanced(imbalance); + Self::deposit_event(Event::CandidateSlashed { + candidate: c.clone(), + amount: *d, + }); + } + }); + + // write final values to storage. + let deposit_of_candidate = |x: &T::AccountId| -> BalanceOf { + // defensive-only. This closure is used against the new members and new + // runners-up, both of which are election winners and thus must have + // deposit. + candidates_and_deposit + .iter() + .find_map(|(c, d)| if c == x { Some(*d) } else { None }) + .defensive_unwrap_or_default() + }; + + // fetch deposits from the one recorded one. This will make sure that a + // candidate who submitted candidacy before a change to candidacy deposit will + // have the correct amount recorded. + >::put( + new_members_sorted_by_id + .iter() + .map(|(who, stake)| SeatHolder { + deposit: deposit_of_candidate(who), + who: who.clone(), + stake: *stake, + }) + .collect::>(), + ); + >::put( + new_runners_up_sorted_by_rank + .into_iter() + .map(|(who, stake)| SeatHolder { + deposit: deposit_of_candidate(&who), + who, + stake, + }) + .collect::>(), + ); - // clean candidates. - >::kill(); + // clean candidates. + >::kill(); + + Self::deposit_event(Event::NewTerm { new_members: new_members_sorted_by_id }); + >::mutate(|v| *v += 1); + }) + .map_err(|e| { + log::error!( + target: "runtime::elections", + "Failed to run election [{:?}].", + e, + ); + Self::deposit_event(Event::ElectionError); - Self::deposit_event(Event::NewTerm { new_members: new_members_sorted_by_id }); - >::mutate(|v| *v += 1); - }) - .map_err(|e| { - log::error!( - target: "runtime::elections", - "Failed to run election [{:?}].", - e, - ); - Self::deposit_event(Event::ElectionError); - }); + }); - // TODO(gpestana): return weight for general election, not phragmen + // TODO(gpestana): return weight for general election, not phragmen Weight::zero() + + } + + /// Register some amount of weight directly with the system pallet. + /// + /// This is always mandatory weight. + fn register_weight(weight: Weight) { + >::register_extra_weight_unchecked( + weight, + DispatchClass::Mandatory, + ); + } + } impl Contains for Pallet { @@ -1156,6 +1169,7 @@ impl ContainsLengthBound for Pallet { } } + impl ElectionDataProvider for Pallet { type AccountId = T::AccountId; type BlockNumber = T::BlockNumber; @@ -1164,13 +1178,12 @@ impl ElectionDataProvider for Pallet { fn electable_targets( maybe_max_len: Option, ) -> frame_election_provider_support::data_provider::Result> { - // TODO(gpestana): this is a self-weighted function + let mut candidates_and_deposit = Self::candidates(); + // add all the previous members and runners-up as candidates as well. + candidates_and_deposit.append(&mut Self::implicit_candidates_with_deposit()); + let mut targets: Vec<_> = candidates_and_deposit.into_iter().map(|(target, _)| target).collect(); - let mut candidates_and_deposit = Self::candidates(); - // add all the previous members and runners-up as candidates as well. - candidates_and_deposit.append(&mut Self::implicit_candidates_with_deposit()); - let mut targets: Vec<_> = - candidates_and_deposit.into_iter().map(|(target, _)| target).collect(); + Self::register_weight(T::WeightInfo::electable_candidates(targets.len() as u32)); if let Some(max_candidates) = maybe_max_len { // TODO(gpestana):: or should return Err instead? @@ -1185,8 +1198,6 @@ impl ElectionDataProvider for Pallet { ) -> frame_election_provider_support::data_provider::Result< Vec>, > { - // TODO(gpestana): this is a self-weighted function - let mut voters_and_stakes = Vec::new(); match Voting::::iter().try_for_each(|(voter, Voter { stake, votes, .. })| { @@ -1208,14 +1219,18 @@ impl ElectionDataProvider for Pallet { "Failed to run election. Number of voters exceeded", ); Self::deposit_event(Event::ElectionError); - //return T::DbWeight::get().reads(3 + max_voters as u64); // TODO(gpestana)) + Self::register_weight(T::WeightInfo::electing_voters(voters_and_stakes.len() as u32)); }, } + Self::register_weight(T::WeightInfo::electing_voters(voters_and_stakes.len() as u32)); + Ok(voters_and_stakes) } fn desired_targets() -> frame_election_provider_support::data_provider::Result { + Self::register_weight(T::DbWeight::get().reads(2)); + let desired_seats = T::DesiredMembers::get() as usize; let desired_runners_up = T::DesiredRunnersUp::get() as usize; Ok((desired_runners_up + desired_seats) as u32) @@ -1231,7 +1246,7 @@ impl ElectionDataProvider for Pallet { mod tests { use super::*; use crate::{self as elections_phragmen}; - use frame_election_provider_support::{onchain, SequentialPhragmen}; + use frame_election_provider_support::{SequentialPhragmen, onchain}; use frame_support::{ assert_noop, assert_ok, dispatch::DispatchResultWithPostInfo, @@ -1375,18 +1390,18 @@ mod tests { type WeightInfo = (); type MaxVoters = PhragmenMaxVoters; type MaxCandidates = PhragmenMaxCandidates; - type ElectionProvider = onchain::BoundedExecution; + type ElectionProvider = onchain::BoundedExecution::; } pub struct SeqPhragmenProvider; - impl onchain::Config for SeqPhragmenProvider { + impl onchain::Config for SeqPhragmenProvider{ type System = Test; type Solver = SequentialPhragmen; type DataProvider = Elections; type WeightInfo = (); } - impl onchain::BoundedConfig for SeqPhragmenProvider { + impl onchain::BoundedConfig for SeqPhragmenProvider{ type TargetsBound = MaxTargets; type VotersBound = MaxVoters; } diff --git a/frame/elections-phragmen/src/weights.rs b/frame/elections-phragmen/src/weights.rs index 52a5dedc723d4..0b6e9c561aa72 100644 --- a/frame/elections-phragmen/src/weights.rs +++ b/frame/elections-phragmen/src/weights.rs @@ -57,7 +57,9 @@ pub trait WeightInfo { fn remove_member_without_replacement() -> Weight; fn remove_member_with_replacement() -> Weight; fn clean_defunct_voters(v: u32, d: u32, ) -> Weight; - fn election_phragmen(c: u32, v: u32, e: u32, ) -> Weight; + fn election(c: u32, v: u32, e: u32, ) -> Weight; + fn electable_candidates(c: u32) -> Weight; + fn electing_voters(v: u32) -> Weight; } /// Weights for pallet_elections_phragmen using the Substrate node and recommended hardware. @@ -188,17 +190,28 @@ impl WeightInfo for SubstrateWeight { /// The range of component `c` is `[1, 1000]`. /// The range of component `v` is `[1, 10000]`. /// The range of component `e` is `[10000, 160000]`. - fn election_phragmen(c: u32, v: u32, e: u32, ) -> Weight { + fn election(c: u32, v: u32, e: u32, ) -> Weight { Weight::from_ref_time(0 as u64) // Standard Error: 773_000 .saturating_add(Weight::from_ref_time(81_534_000 as u64).saturating_mul(v as u64)) // Standard Error: 51_000 .saturating_add(Weight::from_ref_time(4_453_000 as u64).saturating_mul(e as u64)) .saturating_add(T::DbWeight::get().reads(280 as u64)) - .saturating_add(T::DbWeight::get().reads((1 as u64).saturating_mul(c as u64))) - .saturating_add(T::DbWeight::get().reads((1 as u64).saturating_mul(v as u64))) .saturating_add(T::DbWeight::get().writes((1 as u64).saturating_mul(c as u64))) } + + // Storage: Elections Candidates (r:1 w:0) + fn electable_candidates(c: u32) -> Weight { + Weight::from_ref_time(0 as u64) + .saturating_add(RocksDbWeight::get().reads(1 as u64).saturating_mul(c as u64)) + } + + // Storage: Elections Voting (r:1 w:0) + fn electing_voters(v: u32) -> Weight { + Weight::from_ref_time(0 as u64) + .saturating_add(RocksDbWeight::get().reads(1 as u64).saturating_mul(v as u64)) + } + } // For backwards compatibility and tests @@ -316,7 +329,7 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads((3 as u64).saturating_mul(v as u64))) .saturating_add(RocksDbWeight::get().writes((3 as u64).saturating_mul(v as u64))) } - // Storage: Elections Candidates (r:1 w:1) + // Storage: Elections Candidates (r:0 w:1) // Storage: Elections Members (r:1 w:1) // Storage: Elections RunnersUp (r:1 w:1) // Storage: Elections Voting (r:10001 w:0) @@ -328,15 +341,26 @@ impl WeightInfo for () { /// The range of component `c` is `[1, 1000]`. /// The range of component `v` is `[1, 10000]`. /// The range of component `e` is `[10000, 160000]`. - fn election_phragmen(c: u32, v: u32, e: u32, ) -> Weight { + fn election(c: u32, v: u32, e: u32, ) -> Weight { Weight::from_ref_time(0 as u64) // Standard Error: 773_000 .saturating_add(Weight::from_ref_time(81_534_000 as u64).saturating_mul(v as u64)) // Standard Error: 51_000 .saturating_add(Weight::from_ref_time(4_453_000 as u64).saturating_mul(e as u64)) .saturating_add(RocksDbWeight::get().reads(280 as u64)) - .saturating_add(RocksDbWeight::get().reads((1 as u64).saturating_mul(c as u64))) - .saturating_add(RocksDbWeight::get().reads((1 as u64).saturating_mul(v as u64))) .saturating_add(RocksDbWeight::get().writes((1 as u64).saturating_mul(c as u64))) } + + // Storage: Elections Candidates (r:1 w:0) + fn electable_candidates(c: u32) -> Weight { + Weight::from_ref_time(0 as u64) + .saturating_add(RocksDbWeight::get().reads(1 as u64).saturating_mul(c as u64)) + } + + // Storage: Elections Voting (r:1 w:0) + fn electing_voters(v: u32) -> Weight { + Weight::from_ref_time(0 as u64) + .saturating_add(RocksDbWeight::get().reads(1 as u64).saturating_mul(v as u64)) + } + } From f5e16e8c0f210565001cc64b9b63f470b9d3f9d7 Mon Sep 17 00:00:00 2001 From: gpestana Date: Tue, 25 Oct 2022 14:44:53 +0200 Subject: [PATCH 6/7] next_election_prediction() and finish weights --- frame/elections-phragmen/src/lib.rs | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index d8ff15492dd4b..438ccf2a505a9 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -951,6 +951,10 @@ impl Pallet { }, } + let weight_candidates = candidates_and_deposit.len() as u32; + let weight_voters = voters_and_stakes.len() as u32; + let weight_edges = 0; // TODO(gpestana): generalize for elections + let _ = T::ElectionProvider::elect().map(|mut winners| { // sort winners by stake winners.sort_by(|i, j| j.1.total.cmp(&i.1.total)); @@ -1107,10 +1111,7 @@ impl Pallet { }); - // TODO(gpestana): return weight for general election, not phragmen - Weight::zero() - - + T::WeightInfo::election(weight_candidates, weight_voters, weight_edges) } /// Register some amount of weight directly with the system pallet. @@ -1237,8 +1238,13 @@ impl ElectionDataProvider for Pallet { } fn next_election_prediction(now: Self::BlockNumber) -> Self::BlockNumber { - // TODO(gpestana): verify - now + (now % T::TermDuration::get()) + let term_duration = T::TermDuration::get(); + let blocks_to_next_election = now % term_duration; + + if blocks_to_next_election == Self::BlockNumber::zero() { + return now; + } + now + term_duration - (blocks_to_next_election) } } @@ -3328,4 +3334,14 @@ mod tests { assert_ok!(Elections::clean_defunct_voters(RuntimeOrigin::root(), 4, 2)); }) } + + #[test] + fn election_data_provider_next_election_prediction() { + ExtBuilder::default().build_and_execute(|| { + assert_eq!(::next_election_prediction(1), 5); + assert_eq!(::next_election_prediction(5), 5); + assert_eq!(::next_election_prediction(7), 10); + assert_eq!(::next_election_prediction(11), 15); + }) + } } From 0a984b8c4278eb9f83ab4fe7500f594b2b673746 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gon=C3=A7alo=20Pestana?= Date: Sun, 30 Oct 2022 17:32:04 +0100 Subject: [PATCH 7/7] Update frame/elections-phragmen/src/lib.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/elections-phragmen/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 438ccf2a505a9..4049e22b706a5 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -276,7 +276,7 @@ pub mod pallet { type WeightInfo: WeightInfo; /// Something that will calculate the election results based on the data provided by - /// Self as DataProvider. + /// `Self as DataProvider`. type ElectionProvider: ElectionProvider< AccountId = Self::AccountId, BlockNumber = Self::BlockNumber,