Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Staking: Introduce MaxBackersPerWinner #11935

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@

use codec::{Decode, Encode, MaxEncodedLen};
use frame_election_provider_support::{
onchain, BalancingConfig, ElectionDataProvider, SequentialPhragmen, VoteWeight,
onchain, onchain::TruncateIntoBoundedSupportsOf, BalancingConfig, ElectionDataProvider,
SequentialPhragmen, TruncateIntoBoundedSupports, VoteWeight,
};
use frame_support::{
construct_runtime,
Expand Down Expand Up @@ -658,6 +659,10 @@ impl onchain::Config for OnChainSeqPhragmen {
>;
type DataProvider = <Runtime as pallet_election_provider_multi_phase::Config>::DataProvider;
type WeightInfo = frame_election_provider_support::weights::SubstrateWeight<Runtime>;

// TODO no idea what to use here
type MaxBackersPerWinner = ConstU32<{ u32::MAX }>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 1024 or something a few orders larger than MaxNominatorRewardedPerValidator.

type Bounder = TruncateIntoBoundedSupportsOf<Self>;
}

impl onchain::BoundedConfig for OnChainSeqPhragmen {
Expand Down Expand Up @@ -711,6 +716,8 @@ impl pallet_election_provider_multi_phase::Config for Runtime {
type ForceOrigin = EnsureRootOrHalfCouncil;
type MaxElectableTargets = ConstU16<{ u16::MAX }>;
type MaxElectingVoters = MaxElectingVoters;
// TODO what to use here
type MaxBackersPerWinner = ConstU32<{ u32::MAX }>;
type BenchmarkingConfig = ElectionProviderBenchmarkConfig;
type WeightInfo = pallet_election_provider_multi_phase::weights::SubstrateWeight<Self>;
}
Expand Down
7 changes: 6 additions & 1 deletion frame/babe/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@

use crate::{self as pallet_babe, Config, CurrentSlot};
use codec::Encode;
use frame_election_provider_support::{onchain, SequentialPhragmen};
use frame_election_provider_support::{
onchain, onchain::TruncateIntoBoundedSupportsOf, SequentialPhragmen,
};
use frame_support::{
parameter_types,
traits::{ConstU128, ConstU32, ConstU64, GenesisBuild, KeyOwnerProofSystem, OnInitialize},
Expand Down Expand Up @@ -178,6 +180,9 @@ impl onchain::Config for OnChainSeqPhragmen {
type Solver = SequentialPhragmen<DummyValidatorId, Perbill>;
type DataProvider = Staking;
type WeightInfo = ();
// TODO no idea what to use here
type MaxBackersPerWinner = ConstU32<{ u32::MAX }>;
type Bounder = TruncateIntoBoundedSupportsOf<Self>;
}

impl pallet_staking::Config for Test {
Expand Down
5 changes: 3 additions & 2 deletions frame/election-provider-multi-phase/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ frame_benchmarking::benchmarks! {
assert!(<Snapshot<T>>::get().is_some());
assert!(<SnapshotMetadata<T>>::get().is_some());
}: {
assert_ok!(<MultiPhase<T> as ElectionProvider>::elect());
assert!(<MultiPhase<T> as ElectionProvider>::elect().is_ok());
} verify {
assert!(<MultiPhase<T>>::queued_solution().is_none());
assert!(<DesiredTargets<T>>::get().is_none());
Expand Down Expand Up @@ -404,7 +404,8 @@ frame_benchmarking::benchmarks! {
assert_eq!(raw_solution.solution.voter_count() as u32, a);
assert_eq!(raw_solution.solution.unique_targets().len() as u32, d);
}: {
assert_ok!(<MultiPhase<T>>::feasibility_check(raw_solution, ElectionCompute::Unsigned));
// TODO @ggwpez Why does format! work but not assert_ok?
assert!(<MultiPhase<T>>::feasibility_check(raw_solution, ElectionCompute::Unsigned).is_ok());
}

// NOTE: this weight is not used anywhere, but the fact that it should succeed when execution in
Expand Down
101 changes: 78 additions & 23 deletions frame/election-provider-multi-phase/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,15 +229,17 @@

#![cfg_attr(not(feature = "std"), no_std)]

use codec::{Decode, Encode};
use codec::{Decode, Encode, MaxEncodedLen};
use frame_election_provider_support::{
ElectionDataProvider, ElectionProvider, InstantElectionProvider, NposSolution,
BoundedSupport, BoundedSupports, BoundedSupportsOf, ElectionDataProvider, ElectionProvider,
InstantElectionProvider, NposSolution,
};
use frame_support::{
dispatch::DispatchResultWithPostInfo,
ensure,
traits::{Currency, Get, OnUnbalanced, ReservableCurrency},
weights::{DispatchClass, Weight},
DefaultNoBound, PartialEqNoBound,
};
use frame_system::{ensure_none, offchain::SendTransactionTypes};
use scale_info::TypeInfo;
Expand All @@ -249,6 +251,7 @@ use sp_npos_elections::{
assignment_ratio_to_staked_normalized, ElectionScore, EvaluateSupport, Supports, VoteWeight,
};
use sp_runtime::{
traits::Convert,
transaction_validity::{
InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity,
TransactionValidityError, ValidTransaction,
Expand Down Expand Up @@ -317,15 +320,16 @@ impl<T: Config> ElectionProvider for NoFallback<T> {
type BlockNumber = T::BlockNumber;
type DataProvider = T::DataProvider;
type Error = &'static str;
type MaxBackersPerWinner = T::MaxBackersPerWinner;

fn elect() -> Result<Supports<T::AccountId>, Self::Error> {
fn elect() -> Result<BoundedSupportsOf<Self>, Self::Error> {
// Do nothing, this will enable the emergency phase.
Err("NoFallback.")
}
}

impl<T: Config> InstantElectionProvider for NoFallback<T> {
fn elect_with_bounds(_: usize, _: usize) -> Result<Supports<T::AccountId>, Self::Error> {
fn elect_with_bounds(_: usize, _: usize) -> Result<BoundedSupportsOf<Self>, Self::Error> {
Err("NoFallback.")
}
}
Expand Down Expand Up @@ -393,7 +397,7 @@ impl<Bn: PartialEq + Eq> Phase<Bn> {
}

/// The type of `Computation` that provided this election data.
#[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo)]
#[derive(PartialEq, Eq, Clone, Copy, Encode, Decode, Debug, MaxEncodedLen, TypeInfo)]
pub enum ElectionCompute {
/// Election was computed on-chain.
OnChain,
Expand Down Expand Up @@ -437,13 +441,14 @@ impl<C: Default> Default for RawSolution<C> {
}

/// A checked solution, ready to be enacted.
#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default, TypeInfo)]
pub struct ReadySolution<A> {
#[derive(PartialEqNoBound, Clone, Encode, Decode, RuntimeDebug, DefaultNoBound, TypeInfo)]
#[scale_info(skip_type_params(MaxBackersPerWinner))]
pub struct ReadySolution<AccountId: PartialEq, MaxBackersPerWinner: Get<u32>> {
/// The final supports of the solution.
///
/// This is target-major vector, storing each winners, total backing, and each individual
/// backer.
pub supports: Supports<A>,
pub supports: BoundedSupports<AccountId, MaxBackersPerWinner>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally move structs to be <T: Config> once they need more than one generic from T, just a suggestion.

/// The score of the solution.
///
/// This is needed to potentially challenge the solution.
Expand All @@ -452,6 +457,31 @@ pub struct ReadySolution<A> {
pub compute: ElectionCompute,
}

/// Try to convert the supports of a solution into a [`ReadySolution`].
///
/// Errors if any of the supports has more than [`MaxBackersPerWinner`] backers.
impl<AccountId: PartialEq, MaxBackersPerWinner: Get<u32>>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, I would implement just impl TryFrom<Supports<AccountId>> for BoundedSupports<..> in election-provider-support/src.

The other two elements of the tuple seems very out of context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, IIRC a lot of such conversions need to re-allocate in safe rust but with unsafe we can circumvent it.

TryFrom<(Supports<AccountId>, ElectionScore, ElectionCompute)>
for ReadySolution<AccountId, MaxBackersPerWinner>
{
type Error = &'static str;

fn try_from(
supports: (Supports<AccountId>, ElectionScore, ElectionCompute),
) -> Result<Self, Self::Error> {
let mut bounded_supports = BoundedSupports::default();
for (winner, backing) in supports.0.into_iter() {
bounded_supports.push((winner, backing.try_into()?));
}

Ok(Self { supports: bounded_supports, score: supports.1, compute: supports.2 })
}
}

/// Convenience wrapper to create a [`ReadySolution`] from a [`ElectionProvider`].
pub type ReadySolutionOf<E> =
ReadySolution<<E as ElectionProvider>::AccountId, <E as ElectionProvider>::MaxBackersPerWinner>;

/// A snapshot of all the data that is needed for en entire round. They are provided by
/// [`ElectionDataProvider`] and are kept around until the round is finished.
///
Expand Down Expand Up @@ -551,6 +581,8 @@ pub enum FeasibilityError {
InvalidRound,
/// Comparison against `MinimumUntrustedScore` failed.
UntrustedScoreTooLow,
/// There are too many backers.
TooManyBackers,
}

impl From<sp_npos_elections::Error> for FeasibilityError {
Expand Down Expand Up @@ -681,6 +713,7 @@ pub mod pallet {
AccountId = Self::AccountId,
BlockNumber = Self::BlockNumber,
DataProvider = Self::DataProvider,
MaxBackersPerWinner = Self::MaxBackersPerWinner,
>;

/// Configuration of the governance-only fallback.
Expand All @@ -691,11 +724,16 @@ pub mod pallet {
AccountId = Self::AccountId,
BlockNumber = Self::BlockNumber,
DataProvider = Self::DataProvider,
MaxBackersPerWinner = Self::MaxBackersPerWinner,
>;

/// OCW election solution miner algorithm implementation.
type Solver: NposSolver<AccountId = Self::AccountId>;

/// Maximum number of backers per winner that this pallet should, as its implementation of
/// `ElectionProvider` return.
type MaxBackersPerWinner: Get<u32>;

/// Origin that can control this pallet. Note that any action taken by this origin (such)
/// as providing an emergency solution is not checked. Thus, it must be a trusted origin.
type ForceOrigin: EnsureOrigin<Self::Origin>;
Expand Down Expand Up @@ -942,11 +980,10 @@ pub mod pallet {
// Note: we don't `rotate_round` at this point; the next call to
// `ElectionProvider::elect` will succeed and take care of that.

let solution = ReadySolution {
supports,
score: Default::default(),
compute: ElectionCompute::Emergency,
};
let solution: ReadySolution<_, _> =
(supports, Default::default(), ElectionCompute::Emergency)
.try_into()
.map_err(|_| Error::<T>::TooManyBackersPerWinner)?;
Comment on lines +983 to +986
Copy link
Contributor

@kianenigma kianenigma Jul 30, 2022

Choose a reason for hiding this comment

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

Suggested change
let solution: ReadySolution<_, _> =
(supports, Default::default(), ElectionCompute::Emergency)
.try_into()
.map_err(|_| Error::<T>::TooManyBackersPerWinner)?;
ReadySolution {
supports: supports.into_iter().map(|s| s.try_into()).collect::<Result<Vec<_>, _>>().map_err(|_| Error::<T>::TooManyBackersPerWinner)?,
score: Default::default(),
ElectionCompute::Emergency,
};

Supplement of https://github.com/paritytech/substrate/pull/11935/files#r933816968


Self::deposit_event(Event::SolutionStored {
election_compute: ElectionCompute::Emergency,
Expand Down Expand Up @@ -1122,6 +1159,8 @@ pub mod pallet {
CallNotAllowed,
/// The fallback failed
FallbackFailed,
/// The solution has too many backers per winner.
TooManyBackersPerWinner,
}

#[pallet::validate_unsigned]
Expand Down Expand Up @@ -1195,7 +1234,7 @@ pub mod pallet {
/// Current best solution, signed or unsigned, queued to be returned upon `elect`.
#[pallet::storage]
#[pallet::getter(fn queued_solution)]
pub type QueuedSolution<T: Config> = StorageValue<_, ReadySolution<T::AccountId>>;
pub type QueuedSolution<T: Config> = StorageValue<_, ReadySolutionOf<crate::Pallet<T>>>;

/// Snapshot data of the round.
///
Expand Down Expand Up @@ -1429,7 +1468,7 @@ impl<T: Config> Pallet<T> {
pub fn feasibility_check(
raw_solution: RawSolution<SolutionOf<T::MinerConfig>>,
compute: ElectionCompute,
) -> Result<ReadySolution<T::AccountId>, FeasibilityError> {
) -> Result<ReadySolutionOf<Self>, FeasibilityError> {
let RawSolution { solution, score, round } = raw_solution;

// First, check round.
Expand Down Expand Up @@ -1502,7 +1541,9 @@ impl<T: Config> Pallet<T> {
let known_score = supports.evaluate();
ensure!(known_score == score, FeasibilityError::InvalidScore);

Ok(ReadySolution { supports, compute, score })
(supports, score, compute)
.try_into()
.map_err(|_| FeasibilityError::TooManyBackers)
}

/// Perform the tasks to be done after a new `elect` has been triggered:
Expand All @@ -1521,7 +1562,7 @@ impl<T: Config> Pallet<T> {
Self::kill_snapshot();
}

fn do_elect() -> Result<Supports<T::AccountId>, ElectionError<T>> {
fn do_elect() -> Result<BoundedSupportsOf<Self>, ElectionError<T>> {
// We have to unconditionally try finalizing the signed phase here. There are only two
// possibilities:
//
Expand Down Expand Up @@ -1556,7 +1597,7 @@ impl<T: Config> Pallet<T> {
}

/// record the weight of the given `supports`.
fn weigh_supports(supports: &Supports<T::AccountId>) {
fn weigh_supports(supports: &BoundedSupportsOf<Self>) {
let active_voters = supports
.iter()
.map(|(_, x)| x)
Expand All @@ -1570,14 +1611,16 @@ impl<T: Config> ElectionProvider for Pallet<T> {
type AccountId = T::AccountId;
type BlockNumber = T::BlockNumber;
type Error = ElectionError<T>;
type MaxBackersPerWinner = T::MaxBackersPerWinner;
type DataProvider = T::DataProvider;

fn elect() -> Result<Supports<T::AccountId>, Self::Error> {
fn elect() -> Result<BoundedSupportsOf<Self>, Self::Error> {
match Self::do_elect() {
Ok(supports) => {
// All went okay, record the weight, put sign to be Off, clean snapshot, etc.
Self::weigh_supports(&supports);
Self::rotate_round();

Ok(supports)
},
Err(why) => {
Expand Down Expand Up @@ -1807,8 +1850,8 @@ mod tests {
Phase,
};
use frame_election_provider_support::ElectionProvider;
use frame_support::{assert_noop, assert_ok};
use sp_npos_elections::{BalancingConfig, Support};
use frame_support::{assert_noop, assert_ok, bounded_vec};
use sp_npos_elections::BalancingConfig;

#[test]
fn phase_rotation_works() {
Expand Down Expand Up @@ -2030,8 +2073,20 @@ mod tests {
assert_eq!(
supports,
vec![
(30, Support { total: 40, voters: vec![(2, 5), (4, 5), (30, 30)] }),
(40, Support { total: 60, voters: vec![(2, 5), (3, 10), (4, 5), (40, 40)] })
(
30,
BoundedSupport {
total: 40,
voters: bounded_vec![(2, 5), (4, 5), (30, 30)]
}
),
(
40,
BoundedSupport {
total: 60,
voters: bounded_vec![(2, 5), (3, 10), (4, 5), (40, 40)]
}
)
]
)
});
Expand Down
16 changes: 11 additions & 5 deletions frame/election-provider-multi-phase/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use super::*;
use crate::{self as multi_phase, unsigned::MinerConfig};
use frame_election_provider_support::{
data_provider,
onchain::{self, UnboundedExecution},
ElectionDataProvider, NposSolution, SequentialPhragmen,
onchain::{self, TruncateIntoBoundedSupportsOf, UnboundedExecution},
ElectionDataProvider, NposSolution, SequentialPhragmen, TruncateIntoBoundedSupports,
};
pub use frame_support::{assert_noop, assert_ok, pallet_prelude::GetDefault};
use frame_support::{
Expand All @@ -44,7 +44,7 @@ use sp_npos_elections::{
};
use sp_runtime::{
testing::Header,
traits::{BlakeTwo256, IdentityLookup},
traits::{BlakeTwo256, Convert, IdentityLookup},
PerU16,
};
use std::sync::Arc;
Expand Down Expand Up @@ -292,6 +292,9 @@ impl onchain::Config for OnChainSeqPhragmen {
type Solver = SequentialPhragmen<AccountId, SolutionAccuracyOf<Runtime>, Balancing>;
type DataProvider = StakingMock;
type WeightInfo = ();
// TODO no idea what to use here
type MaxBackersPerWinner = ConstU32<{ u32::MAX }>;
type Bounder = TruncateIntoBoundedSupportsOf<Self>;
}

pub struct MockFallback;
Expand All @@ -300,8 +303,9 @@ impl ElectionProvider for MockFallback {
type BlockNumber = u64;
type Error = &'static str;
type DataProvider = StakingMock;
type MaxBackersPerWinner = ConstU32<{ u32::MAX }>;

fn elect() -> Result<Supports<AccountId>, Self::Error> {
fn elect() -> Result<BoundedSupportsOf<Self>, Self::Error> {
Self::elect_with_bounds(Bounded::max_value(), Bounded::max_value())
}
}
Expand All @@ -310,7 +314,7 @@ impl InstantElectionProvider for MockFallback {
fn elect_with_bounds(
max_voters: usize,
max_targets: usize,
) -> Result<Supports<Self::AccountId>, Self::Error> {
) -> Result<BoundedSupportsOf<Self>, Self::Error> {
if OnChainFallback::get() {
onchain::UnboundedExecution::<OnChainSeqPhragmen>::elect_with_bounds(
max_voters,
Expand Down Expand Up @@ -385,6 +389,8 @@ impl crate::Config for Runtime {
type ForceOrigin = frame_system::EnsureRoot<AccountId>;
type MaxElectingVoters = MaxElectingVoters;
type MaxElectableTargets = MaxElectableTargets;
// TODO what to use here
type MaxBackersPerWinner = ConstU32<{ u32::MAX }>;
type MinerConfig = Self;
type Solver = SequentialPhragmen<AccountId, SolutionAccuracyOf<Runtime>, Balancing>;
}
Expand Down
Loading