diff --git a/bin/node/cli/src/chain_spec.rs b/bin/node/cli/src/chain_spec.rs index a56c4e7b4016e..a06f4b5c8f067 100644 --- a/bin/node/cli/src/chain_spec.rs +++ b/bin/node/cli/src/chain_spec.rs @@ -326,7 +326,11 @@ pub fn testnet_genesis( .take((num_endowed_accounts + 1) / 2) .cloned() .map(|member| (member, STASH)) - .collect(), + .collect::>() + .try_into() + .expect( + "Elections-Phragmen: Cannot accept more than DesiredMembers genesis member.", + ), }, council: CouncilConfig::default(), technical_committee: TechnicalCommitteeConfig { diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 56ea19578c8f5..1418e0b301ab8 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -84,9 +84,9 @@ fn submit_candidates_with_self_vote( ) -> Result, &'static str> { let candidates = submit_candidates::(c, prefix)?; let stake = default_stake::(c); - let _ = candidates - .iter() - .try_for_each(|c| submit_voter::(c.clone(), vec![c.clone()], stake).map(|_| ()))?; + let _ = candidates.iter().try_for_each(|c| { + submit_voter::(c.clone(), vec![c.clone()].try_into().unwrap(), stake).map(|_| ()) + })?; Ok(candidates) } @@ -96,7 +96,7 @@ fn submit_voter( votes: Vec, stake: BalanceOf, ) -> DispatchResultWithPostInfo { - >::vote(RawOrigin::Signed(caller).into(), votes, stake) + >::vote(RawOrigin::Signed(caller).into(), votes.try_into().unwrap(), stake) } /// create `num_voter` voters who randomly vote for at most `votes` of `all_candidates` if @@ -110,7 +110,13 @@ fn distribute_voters( for i in 0..num_voters { // to ensure that votes are different all_candidates.rotate_left(1); - let votes = all_candidates.iter().cloned().take(votes).collect::>(); + let votes = all_candidates + .iter() + .cloned() + .take(votes) + .collect::>() + .try_into() + .unwrap(); let voter = endowed_account::("voter", i); submit_voter::(voter, votes, stake)?; } @@ -165,7 +171,7 @@ benchmarks! { votes.rotate_left(1); whitelist!(caller); - }: vote(RawOrigin::Signed(caller), votes, stake) + }: vote(RawOrigin::Signed(caller), votes.try_into().unwrap(), stake) vote_more { let v in 2 .. T::MaxVotesPerVoter::get(); @@ -187,7 +193,7 @@ benchmarks! { assert!(votes.len() > >::get(caller.clone()).votes.len()); whitelist!(caller); - }: vote(RawOrigin::Signed(caller), votes, stake / >::from(10u32)) + }: vote(RawOrigin::Signed(caller), votes.try_into().unwrap(), stake / >::from(10u32)) vote_less { let v in 2 .. T::MaxVotesPerVoter::get(); @@ -208,7 +214,7 @@ benchmarks! { assert!(votes.len() < >::get(caller.clone()).votes.len()); whitelist!(caller); - }: vote(RawOrigin::Signed(caller), votes, stake) + }: vote(RawOrigin::Signed(caller), votes.try_into().unwrap(), stake) remove_voter { // we fix the number of voted candidates to max @@ -228,7 +234,7 @@ benchmarks! { submit_candidacy { // number of already existing candidates. - let c in 1 .. T::MaxCandidates::get(); + let c in 1 .. T::MaxCandidates::get() - 1; // we fix the number of members to the number of desired members and runners-up. We'll be in // this state almost always. let m = T::DesiredMembers::get() + T::DesiredRunnersUp::get(); diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index b23ddda4e8d1c..1ef6770983f0d 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -98,7 +98,7 @@ #![cfg_attr(not(feature = "std"), no_std)] -use codec::{Decode, Encode}; +use codec::{Decode, Encode, MaxEncodedLen}; use frame_support::{ traits::{ defensive_prelude::*, ChangeMembers, Contains, ContainsLengthBound, Currency, Get, @@ -106,6 +106,7 @@ use frame_support::{ SortedMembers, WithdrawReasons, }, weights::Weight, + BoundedVec, }; use scale_info::TypeInfo; use sp_npos_elections::{ElectionResult, ExtendedBalance}; @@ -146,10 +147,11 @@ pub enum Renouncing { } /// An active voter. -#[derive(Encode, Decode, Clone, RuntimeDebug, PartialEq, TypeInfo)] -pub struct Voter { +#[derive(Encode, Decode, Clone, RuntimeDebug, PartialEq, TypeInfo, MaxEncodedLen)] +#[scale_info(skip_type_params(MaxVotesPerVoter))] +pub struct Voter> { /// The members being backed. - pub votes: Vec, + pub votes: BoundedVec, /// The amount of stake placed on this vote. pub stake: Balance, /// The amount of deposit reserved for this vote. @@ -158,14 +160,20 @@ pub struct Voter { pub deposit: Balance, } -impl Default for Voter { +impl> Default + for Voter +{ fn default() -> Self { - Self { votes: vec![], stake: Default::default(), deposit: Default::default() } + Self { + votes: BoundedVec::default(), + stake: Default::default(), + deposit: Default::default(), + } } } /// A holder of a seat as either a member or a runner-up. -#[derive(Encode, Decode, Clone, Default, RuntimeDebug, PartialEq, TypeInfo)] +#[derive(Encode, Decode, Clone, Default, RuntimeDebug, PartialEq, TypeInfo, MaxEncodedLen)] pub struct SeatHolder { /// The holder. pub who: AccountId, @@ -192,7 +200,6 @@ pub mod pallet { #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] - #[pallet::without_storage_info] pub struct Pallet(_); #[pallet::config] @@ -365,15 +372,11 @@ pub mod pallet { )] pub fn vote( origin: OriginFor, - votes: Vec, + votes: BoundedVec, #[pallet::compact] value: BalanceOf, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - ensure!( - votes.len() <= T::MaxVotesPerVoter::get() as usize, - Error::::MaximumVotesExceeded - ); ensure!(!votes.is_empty(), Error::::NoVotes); let candidates_count = >::decode_len().unwrap_or(0); @@ -456,10 +459,6 @@ pub mod pallet { let actual_count = >::decode_len().unwrap_or(0) as u32; ensure!(actual_count <= candidate_count, Error::::InvalidWitnessData); - ensure!( - actual_count <= ::MaxCandidates::get(), - Error::::TooManyCandidates - ); let index = Self::is_candidate(&who).err().ok_or(Error::::DuplicatedCandidate)?; @@ -469,8 +468,11 @@ pub mod pallet { T::Currency::reserve(&who, T::CandidacyBond::get()) .map_err(|_| Error::::InsufficientCandidateFunds)?; - >::mutate(|c| c.insert(index, (who, T::CandidacyBond::get()))); - Ok(()) + >::mutate(|c| -> Result<(), DispatchError> { + c.try_insert(index, (who, T::CandidacyBond::get())) + .map_err(|_| Error::::TooManyCandidates)?; + Ok(()) + }) } /// Renounce one's intention to be a candidate for the next election round. 3 potential @@ -679,8 +681,11 @@ pub mod pallet { /// Invariant: Always sorted based on account id. #[pallet::storage] #[pallet::getter(fn members)] - pub type Members = - StorageValue<_, Vec>>, ValueQuery>; + pub type Members = StorageValue< + _, + BoundedVec>, T::DesiredMembers>, + ValueQuery, + >; /// The current reserved runners-up. /// @@ -688,8 +693,11 @@ pub mod pallet { /// last (i.e. _best_) runner-up will be replaced. #[pallet::storage] #[pallet::getter(fn runners_up)] - pub type RunnersUp = - StorageValue<_, Vec>>, ValueQuery>; + pub type RunnersUp = StorageValue< + _, + BoundedVec>, T::DesiredRunnersUp>, + ValueQuery, + >; /// The present candidate list. A current member or runner-up can never enter this vector /// and is always implicitly assumed to be a candidate. @@ -699,7 +707,8 @@ pub mod pallet { /// Invariant: Always sorted based on account id. #[pallet::storage] #[pallet::getter(fn candidates)] - pub type Candidates = StorageValue<_, Vec<(T::AccountId, BalanceOf)>, ValueQuery>; + pub type Candidates = + StorageValue<_, BoundedVec<(T::AccountId, BalanceOf), T::MaxCandidates>, ValueQuery>; /// The total number of vote rounds that have happened, excluding the upcoming one. #[pallet::storage] @@ -711,13 +720,18 @@ pub mod pallet { /// TWOX-NOTE: SAFE as `AccountId` is a crypto hash. #[pallet::storage] #[pallet::getter(fn voting)] - pub type Voting = - StorageMap<_, Twox64Concat, T::AccountId, Voter>, ValueQuery>; + pub type Voting = StorageMap< + _, + Twox64Concat, + T::AccountId, + Voter, T::MaxVotesPerVoter>, + ValueQuery, + >; #[pallet::genesis_config] #[derive(frame_support::DefaultNoBound)] pub struct GenesisConfig { - pub members: Vec<(T::AccountId, BalanceOf)>, + pub members: BoundedVec<(T::AccountId, BalanceOf), T::DesiredMembers>, } #[pallet::genesis_build] @@ -749,14 +763,16 @@ pub mod pallet { member ) }, - Err(pos) => members.insert( - pos, - SeatHolder { - who: member.clone(), - stake: *stake, - deposit: Zero::zero(), - }, - ), + Err(pos) => members + .try_insert( + pos, + SeatHolder { + who: member.clone(), + stake: *stake, + deposit: Zero::zero(), + }, + ) + .expect("Cannot accept more than DesiredMembers genesis member."), } }); @@ -766,7 +782,13 @@ pub mod pallet { // remove_lock is noop if lock is not there. >::insert( &member, - Voter { votes: vec![member.clone()], stake: *stake, deposit: Zero::zero() }, + Voter { + votes: vec![member.clone()] + .try_into() + .expect("Cannot expect more votes than there are candidates."), + stake: *stake, + deposit: Zero::zero(), + }, ); member.clone() @@ -831,7 +853,13 @@ impl Pallet { // defensive-only: Members and runners-up are disjoint. This will always be err and // give us an index to insert. if let Err(index) = members.binary_search_by(|m| m.who.cmp(&next_best.who)) { - members.insert(index, next_best.clone()); + let _ = members.try_insert(index, next_best.clone()).map_err(|_| { + // This can never happen. + log::error!( + target: "runtime::elections-phragmen", + "There must be an empty place for a new member since we just removed one.", + ); + }); } else { // overlap. This can never happen. If so, it seems like our intended replacement // is already a member, so not much more to do. @@ -950,7 +978,13 @@ impl Pallet { 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()); + match candidates_and_deposit + .try_append(&mut Self::implicit_candidates_with_deposit()) + .map_err(|_| T::DbWeight::get().reads(3)) + { + Ok(_) => (), + Err(weight) => return weight, + } if candidates_and_deposit.len().is_zero() { Self::deposit_event(Event::EmptyTerm); @@ -1118,25 +1152,35 @@ impl Pallet { // 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( + >::put::>( new_members_sorted_by_id .iter() + .take(T::DesiredMembers::get() as usize) .map(|(who, stake)| SeatHolder { deposit: deposit_of_candidate(who), who: who.clone(), stake: *stake, }) - .collect::>(), + .take(T::DesiredMembers::get() as usize) + .collect::>() + .try_into() + .expect("number members will never exceed T::DesiredMembers. qed."), ); - >::put( + >::put::>( new_runners_up_sorted_by_rank .into_iter() + .take(T::DesiredRunnersUp::get() as usize) .map(|(who, stake)| SeatHolder { deposit: deposit_of_candidate(&who), who, stake, }) - .collect::>(), + .take(T::DesiredRunnersUp::get() as usize) + .collect::>() + .try_into() + .expect( + "number runners up will never exceed T::DesiredRunnersUp. qed.", + ), ); // clean candidates. @@ -1181,7 +1225,7 @@ impl SortedMembers for Pallet { stake: Default::default(), deposit: Default::default(), }; - members.insert(pos, s) + members.try_insert(pos, s).expect("Cannot accept more than DesiredMembers.") }, }) } @@ -1512,7 +1556,10 @@ mod tests { ], }, elections: elections_phragmen::GenesisConfig:: { - members: self.genesis_members, + members: self + .genesis_members + .try_into() + .expect("Cannot accept more than DesiredMembers genesis member."), }, } .build_storage() @@ -1601,10 +1648,10 @@ mod tests { // call was changing. Currently it is a wrapper for the original call and does not do much. // Nonetheless, totally harmless. ensure_signed(origin.clone()).expect("vote origin must be signed"); - Elections::vote(origin, votes, stake) + Elections::vote(origin, votes.try_into().unwrap(), stake) } - fn votes_of(who: &u64) -> Vec { + fn votes_of(who: &u64) -> BoundedVec::MaxVotesPerVoter> { Voting::::get(who).votes } @@ -1647,11 +1694,11 @@ mod tests { assert_eq!( Elections::voting(1), - Voter { stake: 10u64, votes: vec![1], deposit: 0 } + Voter { stake: 10u64, votes: vec![1].try_into().unwrap(), deposit: 0 } ); assert_eq!( Elections::voting(2), - Voter { stake: 20u64, votes: vec![2], deposit: 0 } + Voter { stake: 20u64, votes: vec![2].try_into().unwrap(), deposit: 0 } ); // they will persist since they have self vote. @@ -1671,11 +1718,11 @@ mod tests { assert_eq!( Elections::voting(1), - Voter { stake: 10u64, votes: vec![1], deposit: 0 } + Voter { stake: 10u64, votes: vec![1].try_into().unwrap(), deposit: 0 } ); assert_eq!( Elections::voting(2), - Voter { stake: 20u64, votes: vec![2], deposit: 0 } + Voter { stake: 20u64, votes: vec![2].try_into().unwrap(), deposit: 0 } ); assert_ok!(Elections::remove_voter(RuntimeOrigin::signed(1))); @@ -1702,11 +1749,11 @@ mod tests { assert_eq!( Elections::voting(1), - Voter { stake: 10u64, votes: vec![1], deposit: 0 } + Voter { stake: 10u64, votes: vec![1].try_into().unwrap(), deposit: 0 } ); assert_eq!( Elections::voting(2), - Voter { stake: 20u64, votes: vec![2], deposit: 0 } + Voter { stake: 20u64, votes: vec![2].try_into().unwrap(), deposit: 0 } ); // they will persist since they have self vote. diff --git a/frame/elections-phragmen/src/migrations/mod.rs b/frame/elections-phragmen/src/migrations/mod.rs index f5403284126a0..892a44a9d6b02 100644 --- a/frame/elections-phragmen/src/migrations/mod.rs +++ b/frame/elections-phragmen/src/migrations/mod.rs @@ -25,3 +25,5 @@ pub mod v3; pub mod v4; /// Version 5. pub mod v5; +/// Version 6. +pub mod v6; diff --git a/frame/elections-phragmen/src/migrations/v6.rs b/frame/elections-phragmen/src/migrations/v6.rs new file mode 100644 index 0000000000000..6e0a06da2bada --- /dev/null +++ b/frame/elections-phragmen/src/migrations/v6.rs @@ -0,0 +1,173 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use crate::{BalanceOf, Config, Pallet, SeatHolder, Weight}; +use codec::{Decode, Encode}; +use frame_support::{ + pallet_prelude::StorageVersion, traits::OnRuntimeUpgrade, BoundedVec, RuntimeDebug, +}; +use sp_core::Get; +use sp_runtime::Saturating; +use sp_std::prelude::*; + +#[cfg(feature = "try-runtime")] +use frame_support::traits::GetStorageVersion; + +#[derive(Encode, Decode, Clone, Default, RuntimeDebug, PartialEq)] +struct DeprecatedVoter { + votes: Vec, + stake: Balance, + deposit: Balance, +} + +pub fn apply() -> Weight { + let storage_version = StorageVersion::get::>(); + log::info!( + target: "runtime::elections-phragmen", + "Running migration for elections-phragmen with storage version {:?}", + storage_version, + ); + + if storage_version <= 5 { + let weight = migrate_voting::(); + weight.saturating_add(migrate_members::()); + weight.saturating_add(migrate_runners_up::()); + weight.saturating_add(migrate_candidates::()); + + StorageVersion::new(6).put::>(); + + weight + } else { + log::warn!( + target: "runtime::elections-phragmen", + "Attempted to apply migration to V6 but failed because storage version is {:?}", + storage_version, + ); + Weight::zero() + } +} + +pub fn migrate_voting() -> Weight { + let mut translated = 0u64; + crate::Voting::::translate::>, _>(|_, voter| { + translated.saturating_inc(); + let bounded_votes: BoundedVec = + voter.votes.try_into().unwrap(); + Some(crate::Voter { votes: bounded_votes, stake: voter.stake, deposit: voter.deposit }) + }); + T::DbWeight::get().reads_writes(translated, translated) +} + +pub fn migrate_members() -> Weight { + let mut translated = 0u64; + let _ = crate::Members::::translate::>>, _>( + |maybe_members| { + translated.saturating_inc(); + maybe_members.map(|members| { + let bounded_members: BoundedVec< + SeatHolder>, + T::DesiredMembers, + > = members.try_into().unwrap(); + bounded_members + }) + }, + ); + T::DbWeight::get().reads_writes(translated, translated) +} + +pub fn migrate_runners_up() -> Weight { + let mut translated = 0u64; + let _ = crate::RunnersUp::::translate::>>, _>( + |maybe_runners_up| { + translated.saturating_inc(); + maybe_runners_up.map(|runners_up| { + let bounded_runners_up: BoundedVec< + SeatHolder>, + T::DesiredRunnersUp, + > = runners_up.try_into().unwrap(); + bounded_runners_up + }) + }, + ); + T::DbWeight::get().reads_writes(translated, translated) +} + +pub fn migrate_candidates() -> Weight { + let mut translated = 0u64; + let _ = crate::Candidates::::translate::)>, _>( + |maybe_candidates| { + translated.saturating_inc(); + maybe_candidates.map(|candidates| { + let bounded_candidates: BoundedVec<(T::AccountId, BalanceOf), T::MaxCandidates> = + candidates.try_into().unwrap(); + bounded_candidates + }) + }, + ); + T::DbWeight::get().reads_writes(translated, translated) +} + +pub struct MigrateToV6(sp_std::marker::PhantomData); +impl OnRuntimeUpgrade for MigrateToV6 { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, &'static str> { + let members_count = crate::Members::::get().len(); + log::info!("{} members will be migrated.", members_count); + + let runners_up_count = crate::RunnersUp::::get().len(); + log::info!("{} runners-up will be migrated.", runners_up_count); + + let candidates_count = crate::Candidates::::get().len(); + log::info!("{} candidates will be migrated.", candidates_count); + + Ok((members_count as u32, runners_up_count as u32, candidates_count as u32).encode()) + } + + fn on_runtime_upgrade() -> Weight { + apply::() + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), &'static str> { + let (old_members_count, old_runners_up_count, old_candidates_count): (u32, u32, u32) = + Decode::decode(&mut &state[..]).expect("pre_upgrade provides a valid state; qed"); + + let post_members_count = crate::Members::::get().len() as u32; + let post_runners_up_count = crate::RunnersUp::::get().len() as u32; + let post_candidates_count = crate::Candidates::::get().len() as u32; + + assert_eq!( + old_members_count.min(T::DesiredMembers::get()), + post_members_count, + "The members count should be the same or less." + ); + assert_eq!( + old_runners_up_count.min(T::DesiredRunnersUp::get()), + post_runners_up_count, + "The runners-up count should be the same or less." + ); + assert_eq!( + old_candidates_count.min(T::MaxCandidates::get()), + post_candidates_count, + "The runners-up count should be the same or less." + ); + + // new version must be set. + assert_eq!(Pallet::::on_chain_storage_version(), 6); + Ok(()) + } +}