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

Bounded Validators / Authorities in New Sessions #8640

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2ae0c01
update pallet
shawntabrizi Apr 20, 2021
fb14e40
update and add tests
shawntabrizi Apr 20, 2021
fcf8809
Fix typo `One` -> `On`
shawntabrizi Apr 20, 2021
0b21a93
fix node build
shawntabrizi Apr 20, 2021
530c4a7
use types in session handler
shawntabrizi Apr 20, 2021
3885e7a
fix compile
shawntabrizi Apr 20, 2021
41e9820
fix test compile
shawntabrizi Apr 20, 2021
307e969
bound authority discovery
shawntabrizi Apr 20, 2021
a8a68cd
bound babe
shawntabrizi Apr 20, 2021
6359f64
some compile fixes
shawntabrizi Apr 20, 2021
8f6ae0c
fix session benchmark compile
shawntabrizi Apr 20, 2021
5d74819
make pallet offences benchmarks compile
shawntabrizi Apr 20, 2021
7879998
fix benchmark
shawntabrizi Apr 20, 2021
5383e00
line width stuff
shawntabrizi Apr 20, 2021
f9491e2
add truncating_from
kianenigma Apr 20, 2021
64bd953
make pub
kianenigma Apr 20, 2021
a9ba4eb
Fix
kianenigma Apr 20, 2021
f643584
Update frame/aura/src/lib.rs
shawntabrizi Apr 21, 2021
ce8d0c3
undo `One` -> `On`
shawntabrizi Apr 21, 2021
8e96900
use truncating_from
shawntabrizi Apr 21, 2021
5514576
integrate max validators into staking
shawntabrizi Apr 21, 2021
b7228bb
add serde to boundedvec, bound invulnerables
shawntabrizi Apr 21, 2021
58756f5
Update lib.rs
shawntabrizi Apr 21, 2021
3ac6283
bound session
shawntabrizi Apr 21, 2021
c32d358
fix im-online tests
shawntabrizi Apr 22, 2021
2ffa620
fix staking tests
shawntabrizi Apr 22, 2021
5e6a814
fix more tests
shawntabrizi Apr 22, 2021
28ca319
std serde
shawntabrizi Apr 22, 2021
b99849b
fix genesis
shawntabrizi Apr 22, 2021
92083f3
undo invulnerables set
shawntabrizi Apr 22, 2021
575294f
Update frame/session/src/lib.rs
shawntabrizi Apr 22, 2021
dadc746
little doc
shawntabrizi Apr 22, 2021
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-template/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,13 @@ impl frame_system::Config for Runtime {
type OnSetCode = ();
}

parameter_types! {
pub const MaxAuthorities: u32 = 100;
}

impl pallet_aura::Config for Runtime {
type AuthorityId = AuraId;
type MaxAuthorities = MaxAuthorities;
}

impl pallet_grandpa::Config for Runtime {
Expand All @@ -221,6 +226,8 @@ impl pallet_grandpa::Config for Runtime {

type HandleEquivocation = ();

type MaxAuthorities = MaxAuthorities;

type WeightInfo = ();
}

Expand Down Expand Up @@ -394,7 +401,7 @@ impl_runtime_apis! {
}

fn authorities() -> Vec<AuraId> {
Aura::authorities()
Aura::authorities().to_vec()
}
}

Expand Down
15 changes: 14 additions & 1 deletion bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,8 +332,14 @@ parameter_types! {
pub const ExpectedBlockTime: Moment = MILLISECS_PER_BLOCK;
pub const ReportLongevity: u64 =
BondingDuration::get() as u64 * SessionsPerEra::get() as u64 * EpochDuration::get();
// The maximum number of authorities / validators for this runtime system.
// This is should be shared across all pallets using session handlers.
pub const MaxAuthorities: u32 = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should document that this should be the upper bound of what Staking::validator_count is ever going to be.

}

// Just a convenient rename...
type MaxValidators = MaxAuthorities;

impl pallet_babe::Config for Runtime {
type EpochDuration = EpochDuration;
type ExpectedBlockTime = ExpectedBlockTime;
Expand All @@ -354,6 +360,8 @@ impl pallet_babe::Config for Runtime {
type HandleEquivocation =
pallet_babe::EquivocationHandler<Self::KeyOwnerIdentification, Offences, ReportLongevity>;

type MaxAuthorities = MaxAuthorities;

type WeightInfo = ();
}

Expand Down Expand Up @@ -446,6 +454,7 @@ impl pallet_session::Config for Runtime {
type SessionHandler = <SessionKeys as OpaqueKeys>::KeyTypeIdProviders;
type Keys = SessionKeys;
type DisabledValidatorsThreshold = DisabledValidatorsThreshold;
type MaxValidators = MaxValidators;
type WeightInfo = pallet_session::weights::SubstrateWeight<Runtime>;
}

Expand Down Expand Up @@ -496,6 +505,7 @@ impl pallet_staking::Config for Runtime {
type NextNewSession = Session;
type MaxNominatorRewardedPerValidator = MaxNominatorRewardedPerValidator;
type ElectionProvider = ElectionProviderMultiPhase;
type MaxValidators = MaxValidators;
type WeightInfo = pallet_staking::weights::SubstrateWeight<Runtime>;
}

Expand Down Expand Up @@ -891,6 +901,7 @@ impl pallet_im_online::Config for Runtime {
type ValidatorSet = Historical;
type ReportUnresponsiveness = Offences;
type UnsignedPriority = ImOnlineUnsignedPriority;
type MaxAuthorityKeys = MaxAuthorities;
type WeightInfo = pallet_im_online::weights::SubstrateWeight<Runtime>;
}

Expand Down Expand Up @@ -925,6 +936,8 @@ impl pallet_grandpa::Config for Runtime {
type HandleEquivocation =
pallet_grandpa::EquivocationHandler<Self::KeyOwnerIdentification, Offences, ReportLongevity>;

type MaxAuthorities = MaxAuthorities;

type WeightInfo = ();
}

Expand Down Expand Up @@ -1294,7 +1307,7 @@ impl_runtime_apis! {
slot_duration: Babe::slot_duration(),
epoch_length: EpochDuration::get(),
c: BABE_GENESIS_EPOCH_CONFIG.c,
genesis_authorities: Babe::authorities(),
genesis_authorities: Babe::authorities().to_vec(),
randomness: Babe::randomness(),
allowed_slots: BABE_GENESIS_EPOCH_CONFIG.allowed_slots,
}
Expand Down
46 changes: 37 additions & 9 deletions frame/aura/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,14 @@

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

use sp_std::prelude::*;
use sp_std::{
prelude::*,
convert::TryInto,
};
use codec::{Encode, Decode};
use frame_support::{
Parameter, traits::{Get, FindAuthor, OneSessionHandler, OnTimestampSet}, ConsensusEngineId,
Parameter, BoundedVec, ConsensusEngineId,
traits::{Get, FindAuthor, OneSessionHandler, OnTimestampSet},
};
use sp_runtime::{
RuntimeAppPublic,
Expand All @@ -64,11 +68,21 @@ pub mod pallet {
pub trait Config: pallet_timestamp::Config + frame_system::Config {
/// The identifier type for an authority.
type AuthorityId: Member + Parameter + RuntimeAppPublic + Default + MaybeSerializeDeserialize;

/// The maximum number of authorities that can be registered in this pallet.
type MaxAuthorities: Get<u32>;
}

#[pallet::pallet]
pub struct Pallet<T>(sp_std::marker::PhantomData<T>);

// Errors inform users that something went wrong.
#[pallet::error]
pub enum Error<T> {
/// You are trying to add more authorities than allowed in the pallet configuration.
TooManyAuthorities,
}

#[pallet::hooks]
impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
fn on_initialize(_: T::BlockNumber) -> Weight {
Expand All @@ -93,7 +107,11 @@ pub mod pallet {
/// The current authority set.
#[pallet::storage]
#[pallet::getter(fn authorities)]
pub(super) type Authorities<T: Config> = StorageValue<_, Vec<T::AuthorityId>, ValueQuery>;
pub(super) type Authorities<T: Config> = StorageValue<
_,
BoundedVec<T::AuthorityId, T::MaxAuthorities>,
ValueQuery,
>;

/// The current slot of this block.
///
Expand Down Expand Up @@ -123,20 +141,24 @@ pub mod pallet {
}

impl<T: Config> Pallet<T> {
fn change_authorities(new: Vec<T::AuthorityId>) {
fn change_authorities(new: BoundedVec<T::AuthorityId, T::MaxAuthorities>) {
<Authorities<T>>::put(&new);

let log: DigestItem<T::Hash> = DigestItem::Consensus(
AURA_ENGINE_ID,
ConsensusLog::AuthoritiesChange(new).encode()
ConsensusLog::AuthoritiesChange(new.to_vec()).encode()
);
<frame_system::Pallet<T>>::deposit_log(log.into());
}

fn initialize_authorities(authorities: &[T::AuthorityId]) {
if !authorities.is_empty() {
assert!(<Authorities<T>>::get().is_empty(), "Authorities are already initialized!");
<Authorities<T>>::put(authorities);
let bounded_authorities: BoundedVec<T::AuthorityId, T::MaxAuthorities> = authorities
.to_vec()
.try_into()
.expect("Too many initial authorities!");
<Authorities<T>>::put(&bounded_authorities);
}
}

Expand Down Expand Up @@ -165,7 +187,7 @@ impl<T: Config> sp_runtime::BoundToRuntimeAppPublic for Pallet<T> {
type Public = T::AuthorityId;
}

impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
impl<T: Config> OneSessionHandler<T::AccountId, T::MaxAuthorities> for Pallet<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is very very good because now OneSessionHandler<_, X> is a different trait than OneSessionHandler<_, Y> and we should get some good compile time checks, ensuring that we can't faff with them:

if babe implements OneSessionHandler<_, 10> and aura implements OneSessionHandler<_, 20>, I think if you then say impl session::Config { type SessionHandlers = (babe, aura) } we should get an error 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

But the missing point is:

  • If session's MaxAuthority is 100
  • and babe and aura's MaxAuthority is 50

I think we should prevent this at compile time but I don't it in the current state of the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh no we have this guarantee as well, because of

type SessionHandler: SessionHandler<Self::ValidatorId, Self::MaxValidators>;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the point here is that there should be a single global config of MaxAuthorities / MaxValidators in the runtime, and this should be shared with all of the pallets which are using a session handler.

Anything else would result in a compile error, which ensures that these systems are working together correctly.

I dont think it would ever be that session and babe would have different max authorities, but maybe @andresilva could give insight

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we should have an uniform number of authorities throughout the whole runtime.

type Key = T::AuthorityId;

fn on_genesis_session<'a, I: 'a>(validators: I)
Expand All @@ -181,9 +203,15 @@ impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
// instant changes
if changed {
let next_authorities = validators.map(|(_, k)| k).collect::<Vec<_>>();
// Truncate any extra that would not fit into storage...
let bounded_next_authorities = BoundedVec::<T::AuthorityId, T::MaxAuthorities>::truncating_from(
next_authorities,
Some("Aura New Session"),
);

let last_authorities = Self::authorities();
if next_authorities != last_authorities {
Self::change_authorities(next_authorities);
if bounded_next_authorities != last_authorities {
Self::change_authorities(bounded_next_authorities);
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions frame/aura/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,13 @@ impl pallet_timestamp::Config for Test {
type WeightInfo = ();
}

parameter_types! {
pub const MaxAuthorities: u32 = 10;
}

impl pallet_aura::Config for Test {
type AuthorityId = AuthorityId;
type MaxAuthorities = MaxAuthorities;
}

pub fn new_test_ext(authorities: Vec<u64>) -> sp_io::TestExternalities {
Expand Down
29 changes: 29 additions & 0 deletions frame/aura/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#![cfg(test)]

use crate::mock::{Aura, new_test_ext};
use sp_runtime::testing::UintAuthorityId;
use frame_support::traits::OneSessionHandler;

#[test]
fn initial_values() {
Expand All @@ -28,3 +30,30 @@ fn initial_values() {
assert_eq!(Aura::authorities().len(), 4);
});
}

// Should not be able to put more authorities than allowed in genesis.
#[test]
#[should_panic(expected = "Too many initial authorities!")]
fn too_many_initial_fails() {
new_test_ext((0..100).collect::<Vec<_>>());
}

// Session change should truncate the new authorities to fit into the limits
// of the Aura pallet.
#[test]
fn session_change_truncates() {
new_test_ext(vec![0, 1, 2, 3]).execute_with(|| {
Aura::on_new_session(
true,
(0..100)
.map(|x| {
let auth_id = UintAuthorityId(x).to_public_key();
(&0, auth_id)
})
.collect::<Vec<_>>()
.into_iter(),
vec![].into_iter(),
);
assert_eq!(Aura::authorities().len(), 10);
});
}
55 changes: 37 additions & 18 deletions frame/authority-discovery/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
// Ensure we're `no_std` when compiling for Wasm.
#![cfg_attr(not(feature = "std"), no_std)]

use sp_std::prelude::*;
use frame_support::traits::OneSessionHandler;
use sp_std::{prelude::*, convert::TryInto};
use frame_support::{traits::OneSessionHandler, BoundedVec};
#[cfg(feature = "std")]
use frame_support::traits::GenesisBuild;
use sp_authority_discovery::AuthorityId;
Expand All @@ -50,16 +50,16 @@ pub mod pallet {
/// Keys of the current authority set.
pub(super) type Keys<T: Config> = StorageValue<
_,
Vec<AuthorityId>,
BoundedVec<AuthorityId, T::MaxValidators>,
ValueQuery,
>;

#[pallet::storage]
#[pallet::getter(fn next_keys)]
/// Keys of the next authority set.
pub(super) type NextKeys<T: Config> = StorageValue<
_,
Vec<AuthorityId>,
BoundedVec<AuthorityId, T::MaxValidators>,
ValueQuery,
>;

Expand All @@ -79,7 +79,9 @@ pub mod pallet {
#[pallet::genesis_build]
impl<T: Config> GenesisBuild<T> for GenesisConfig {
fn build(&self) {
Pallet::<T>::initialize_keys(&self.keys)
let bounded_keys: BoundedVec::<AuthorityId, T::MaxValidators> =
self.keys.clone().try_into().expect("Too many genesis keys!");
Pallet::<T>::initialize_keys(&bounded_keys)
}
}

Expand All @@ -94,8 +96,8 @@ impl<T: Config> Pallet<T> {
/// Retrieve authority identifiers of the current and next authority set
/// sorted and deduplicated.
pub fn authorities() -> Vec<AuthorityId> {
let mut keys = Keys::<T>::get();
let next = NextKeys::<T>::get();
let mut keys = Keys::<T>::get().to_vec();
let next = NextKeys::<T>::get().to_vec();

keys.extend(next);
keys.sort();
Expand All @@ -106,19 +108,21 @@ impl<T: Config> Pallet<T> {

/// Retrieve authority identifiers of the current authority set in the original order.
pub fn current_authorities() -> Vec<AuthorityId> {
Keys::<T>::get()
Keys::<T>::get().to_vec()
}

/// Retrieve authority identifiers of the next authority set in the original order.
pub fn next_authorities() -> Vec<AuthorityId> {
NextKeys::<T>::get()
NextKeys::<T>::get().to_vec()
}

fn initialize_keys(keys: &[AuthorityId]) {
if !keys.is_empty() {
assert!(Keys::<T>::get().is_empty(), "Keys are already initialized!");
Keys::<T>::put(keys);
NextKeys::<T>::put(keys);
let bounded_keys: BoundedVec<AuthorityId, T::MaxValidators> =
keys.to_vec().try_into().expect("Too many initial keys!");
Keys::<T>::put(&bounded_keys);
NextKeys::<T>::put(&bounded_keys);
}
}
}
Expand All @@ -127,7 +131,7 @@ impl<T: Config> sp_runtime::BoundToRuntimeAppPublic for Pallet<T> {
type Public = AuthorityId;
}

impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
impl<T: Config> OneSessionHandler<T::AccountId, T::MaxValidators> for Pallet<T> {
type Key = AuthorityId;

fn on_genesis_session<'a, I: 'a>(authorities: I)
Expand All @@ -143,10 +147,20 @@ impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
{
// Remember who the authorities are for the new and next session.
if changed {
let keys = validators.map(|x| x.1);
Keys::<T>::put(keys.collect::<Vec<_>>());
let next_keys = queued_validators.map(|x| x.1);
NextKeys::<T>::put(next_keys.collect::<Vec<_>>());
let keys = validators.map(|x| x.1).collect::<Vec<_>>();
// Truncate any extra that would not fit in storage...
let bounded_keys = BoundedVec::<AuthorityId, T::MaxValidators>::truncating_from(
keys,
Some("Authority Discovery New Session Keys"),
);
Keys::<T>::put(bounded_keys);
let next_keys = queued_validators.map(|x| x.1).collect::<Vec<_>>();
// Truncate any extra that would not fit in storage...
let bounded_next_keys = BoundedVec::<AuthorityId, T::MaxValidators>::truncating_from(
next_keys,
Some("Authority Discovery New Session Next Keys"),
);
NextKeys::<T>::put(bounded_next_keys);
}
}

Expand Down Expand Up @@ -211,6 +225,7 @@ mod tests {
type ValidatorIdOf = ConvertInto;
type DisabledValidatorsThreshold = DisabledValidatorsThreshold;
type NextSessionRotation = pallet_session::PeriodicSessions<Period, Offset>;
type MaxValidators = MaxValidators;
type WeightInfo = ();
}

Expand Down Expand Up @@ -256,8 +271,12 @@ mod tests {
type OnSetCode = ();
}

parameter_types! {
pub const MaxValidators: u32 = 10;
}

pub struct TestSessionHandler;
impl pallet_session::SessionHandler<AuthorityId> for TestSessionHandler {
impl pallet_session::SessionHandler<AuthorityId, MaxValidators> for TestSessionHandler {
const KEY_TYPE_IDS: &'static [KeyTypeId] = &[key_types::DUMMY];

fn on_new_session<Ks: OpaqueKeys>(
Expand Down
Loading