From e6e4df1cd37ecd7f7aec7ea89d594237deeda263 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Thu, 2 Jun 2022 17:30:16 +0100 Subject: [PATCH 1/6] Implement MaxEncodedLen on pallet-beefy --- frame/beefy-mmr/src/mock.rs | 1 + frame/beefy/src/lib.rs | 58 ++++++++++++++++-------- frame/beefy/src/mock.rs | 3 +- frame/support/src/storage/bounded_vec.rs | 9 ++++ 4 files changed, 52 insertions(+), 19 deletions(-) diff --git a/frame/beefy-mmr/src/mock.rs b/frame/beefy-mmr/src/mock.rs index f6a35f68a4a1f..c9dbdb3b2e16a 100644 --- a/frame/beefy-mmr/src/mock.rs +++ b/frame/beefy-mmr/src/mock.rs @@ -124,6 +124,7 @@ impl pallet_mmr::Config for Test { impl pallet_beefy::Config for Test { type BeefyId = BeefyId; + type MaxAuthorities = ConstU32<100>; } parameter_types! { diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 14e7ac26cdb6e..92a81202e3bc5 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -17,9 +17,9 @@ #![cfg_attr(not(feature = "std"), no_std)] -use codec::Encode; +use codec::{Encode, MaxEncodedLen}; -use frame_support::{traits::OneSessionHandler, Parameter}; +use frame_support::{traits::OneSessionHandler, BoundedSlice, Parameter, WeakBoundedVec}; use sp_runtime::{ generic::DigestItem, @@ -42,28 +42,28 @@ pub use pallet::*; pub mod pallet { use super::*; use frame_support::pallet_prelude::*; - use frame_system::pallet_prelude::*; #[pallet::config] pub trait Config: frame_system::Config { /// Authority identifier type - type BeefyId: Member + Parameter + RuntimeAppPublic + MaybeSerializeDeserialize; + type BeefyId: Member + + Parameter + + RuntimeAppPublic + + MaybeSerializeDeserialize + + MaxEncodedLen; + + /// The maximum number of authorities that can be added. + type MaxAuthorities: Get; } #[pallet::pallet] - #[pallet::without_storage_info] pub struct Pallet(PhantomData); - #[pallet::hooks] - impl Hooks> for Pallet {} - - #[pallet::call] - impl Pallet {} - /// The current authorities set #[pallet::storage] #[pallet::getter(fn authorities)] - pub(super) type Authorities = StorageValue<_, Vec, ValueQuery>; + pub(super) type Authorities = + StorageValue<_, WeakBoundedVec, ValueQuery>; /// The current validator set id #[pallet::storage] @@ -74,7 +74,8 @@ pub mod pallet { /// Authorities set scheduled to be used with the next session #[pallet::storage] #[pallet::getter(fn next_authorities)] - pub(super) type NextAuthorities = StorageValue<_, Vec, ValueQuery>; + pub(super) type NextAuthorities = + StorageValue<_, WeakBoundedVec, ValueQuery>; #[pallet::genesis_config] pub struct GenesisConfig { @@ -99,12 +100,15 @@ pub mod pallet { impl Pallet { /// Return the current active BEEFY validator set. pub fn validator_set() -> Option> { - let validators: Vec = Self::authorities(); + let validators: WeakBoundedVec = Self::authorities(); let id: beefy_primitives::ValidatorSetId = Self::validator_set_id(); ValidatorSet::::new(validators, id) } - fn change_authorities(new: Vec, queued: Vec) { + fn change_authorities( + new: WeakBoundedVec, + queued: WeakBoundedVec, + ) { >::put(&new); let next_id = Self::validator_set_id() + 1u64; @@ -127,10 +131,14 @@ impl Pallet { assert!(>::get().is_empty(), "Authorities are already initialized!"); - >::put(authorities); + let bounded_authorities = + BoundedSlice::::try_from(authorities) + .expect("Authorities vec too big"); + + >::put(bounded_authorities); >::put(0); // Like `pallet_session`, initialize the next validator set as well. - >::put(authorities); + >::put(bounded_authorities); } } @@ -154,11 +162,25 @@ impl OneSessionHandler for Pallet { I: Iterator, { let next_authorities = validators.map(|(_, k)| k).collect::>(); + let bounded_next_authorities = WeakBoundedVec::<_, T::MaxAuthorities>::force_from( + next_authorities, + Some( + "Warning: The session has more validators than expected. \ + A runtime configuration adjustment may be needed.", + ), + ); let next_queued_authorities = queued_validators.map(|(_, k)| k).collect::>(); + let bounded_next_queued_authorities = WeakBoundedVec::<_, T::MaxAuthorities>::force_from( + next_queued_authorities, + Some( + "Warning: The session has more queued validators than expected. \ + A runtime configuration adjustment may be needed.", + ), + ); // Always issue a change on each `session`, even if validator set hasn't changed. // We want to have at least one BEEFY mandatory block per session. - Self::change_authorities(next_authorities, next_queued_authorities); + Self::change_authorities(bounded_next_authorities, bounded_next_queued_authorities); } fn on_disabled(i: u32) { diff --git a/frame/beefy/src/mock.rs b/frame/beefy/src/mock.rs index 7a8f15cd51d29..27796b5b2206c 100644 --- a/frame/beefy/src/mock.rs +++ b/frame/beefy/src/mock.rs @@ -52,7 +52,7 @@ construct_runtime!( UncheckedExtrinsic = UncheckedExtrinsic, { System: frame_system::{Pallet, Call, Config, Storage, Event}, - Beefy: pallet_beefy::{Pallet, Call, Config, Storage}, + Beefy: pallet_beefy::{Pallet, Config, Storage}, Session: pallet_session::{Pallet, Call, Storage, Event, Config}, } ); @@ -86,6 +86,7 @@ impl frame_system::Config for Test { impl pallet_beefy::Config for Test { type BeefyId = BeefyId; + type MaxAuthorities = ConstU32<100>; } parameter_types! { diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index f1f4330ab2960..3d0f70cc181b2 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -111,6 +111,15 @@ where #[scale_info(skip_type_params(S))] pub struct BoundedSlice<'a, T, S>(&'a [T], PhantomData); +impl<'a, T, S> Clone for BoundedSlice<'a, T, S> { + fn clone(&self) -> Self { + BoundedSlice(self.0, PhantomData) + } +} + +// Since a slice `&[]` is always `Copy`, so is `BoundedSlice`. +impl<'a, T, S> Copy for BoundedSlice<'a, T, S> {} + // `BoundedSlice`s encode to something which will always decode into a `BoundedVec`, // `WeakBoundedVec`, or a `Vec`. impl<'a, T: Encode + Decode, S: Get> EncodeLike> for BoundedSlice<'a, T, S> {} From dd0fda1af8bb0752cb05e7a12bfe1b2e4ffc9f7a Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 3 Jun 2022 16:54:26 +0100 Subject: [PATCH 2/6] Return Result in intialize_authorities --- frame/beefy/src/lib.rs | 51 ++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 92a81202e3bc5..22c00b199dc19 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -19,7 +19,7 @@ use codec::{Encode, MaxEncodedLen}; -use frame_support::{traits::OneSessionHandler, BoundedSlice, Parameter, WeakBoundedVec}; +use frame_support::{traits::OneSessionHandler, BoundedSlice, BoundedVec, Parameter}; use sp_runtime::{ generic::DigestItem, @@ -63,7 +63,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn authorities)] pub(super) type Authorities = - StorageValue<_, WeakBoundedVec, ValueQuery>; + StorageValue<_, BoundedVec, ValueQuery>; /// The current validator set id #[pallet::storage] @@ -75,7 +75,7 @@ pub mod pallet { #[pallet::storage] #[pallet::getter(fn next_authorities)] pub(super) type NextAuthorities = - StorageValue<_, WeakBoundedVec, ValueQuery>; + StorageValue<_, BoundedVec, ValueQuery>; #[pallet::genesis_config] pub struct GenesisConfig { @@ -92,7 +92,10 @@ pub mod pallet { #[pallet::genesis_build] impl GenesisBuild for GenesisConfig { fn build(&self) { - Pallet::::initialize_authorities(&self.authorities); + Pallet::::initialize_authorities(&self.authorities) + // we panic here as runtime maintainers can simply reconfigure genesis and restart + // the chain easily + .expect("Authorities vec too big"); } } } @@ -100,14 +103,14 @@ pub mod pallet { impl Pallet { /// Return the current active BEEFY validator set. pub fn validator_set() -> Option> { - let validators: WeakBoundedVec = Self::authorities(); + let validators: BoundedVec = Self::authorities(); let id: beefy_primitives::ValidatorSetId = Self::validator_set_id(); ValidatorSet::::new(validators, id) } fn change_authorities( - new: WeakBoundedVec, - queued: WeakBoundedVec, + new: BoundedVec, + queued: BoundedVec, ) { >::put(&new); @@ -124,21 +127,23 @@ impl Pallet { >::put(&queued); } - fn initialize_authorities(authorities: &[T::BeefyId]) { + fn initialize_authorities(authorities: &[T::BeefyId]) -> Result<(), ()> { if authorities.is_empty() { - return + return Ok(()) } - assert!(>::get().is_empty(), "Authorities are already initialized!"); + if !>::get().is_empty() { + return Err(()) + } let bounded_authorities = - BoundedSlice::::try_from(authorities) - .expect("Authorities vec too big"); + BoundedSlice::::try_from(authorities)?; >::put(bounded_authorities); >::put(0); // Like `pallet_session`, initialize the next validator set as well. >::put(bounded_authorities); + Ok(()) } } @@ -154,7 +159,9 @@ impl OneSessionHandler for Pallet { I: Iterator, { let authorities = validators.map(|(_, k)| k).collect::>(); - Self::initialize_authorities(&authorities); + // we panic here as runtime maintainers can simply reconfigure genesis and restart the + // chain easily + Self::initialize_authorities(&authorities).expect("Authorities vec too big"); } fn on_new_session<'a, I: 'a>(_changed: bool, validators: I, queued_validators: I) @@ -162,21 +169,11 @@ impl OneSessionHandler for Pallet { I: Iterator, { let next_authorities = validators.map(|(_, k)| k).collect::>(); - let bounded_next_authorities = WeakBoundedVec::<_, T::MaxAuthorities>::force_from( - next_authorities, - Some( - "Warning: The session has more validators than expected. \ - A runtime configuration adjustment may be needed.", - ), - ); + let bounded_next_authorities = + BoundedVec::<_, T::MaxAuthorities>::truncate_from(next_authorities); let next_queued_authorities = queued_validators.map(|(_, k)| k).collect::>(); - let bounded_next_queued_authorities = WeakBoundedVec::<_, T::MaxAuthorities>::force_from( - next_queued_authorities, - Some( - "Warning: The session has more queued validators than expected. \ - A runtime configuration adjustment may be needed.", - ), - ); + let bounded_next_queued_authorities = + BoundedVec::<_, T::MaxAuthorities>::truncate_from(next_queued_authorities); // Always issue a change on each `session`, even if validator set hasn't changed. // We want to have at least one BEEFY mandatory block per session. From f14ebe97a4c498ce2bb3d551c97d11f49edf5900 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 3 Jun 2022 17:29:55 +0100 Subject: [PATCH 3/6] Update docs --- frame/support/src/storage/bounded_vec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/src/storage/bounded_vec.rs b/frame/support/src/storage/bounded_vec.rs index 3d0f70cc181b2..78b3b332f98b6 100644 --- a/frame/support/src/storage/bounded_vec.rs +++ b/frame/support/src/storage/bounded_vec.rs @@ -117,7 +117,7 @@ impl<'a, T, S> Clone for BoundedSlice<'a, T, S> { } } -// Since a slice `&[]` is always `Copy`, so is `BoundedSlice`. +// Since a reference `&T` is always `Copy`, so is `BoundedSlice<'a, T, S>`. impl<'a, T, S> Copy for BoundedSlice<'a, T, S> {} // `BoundedSlice`s encode to something which will always decode into a `BoundedVec`, From b9fed5ed3b9c31b0fe11f0b6b0507d365119715e Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Sun, 5 Jun 2022 15:49:32 +0100 Subject: [PATCH 4/6] Log error when authorities list gets truncated --- frame/beefy/src/lib.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 22c00b199dc19..ea95fe8ef48e2 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -19,7 +19,7 @@ use codec::{Encode, MaxEncodedLen}; -use frame_support::{traits::OneSessionHandler, BoundedSlice, BoundedVec, Parameter}; +use frame_support::{log, traits::{Get, OneSessionHandler}, BoundedSlice, BoundedVec, Parameter}; use sp_runtime::{ generic::DigestItem, @@ -169,9 +169,24 @@ impl OneSessionHandler for Pallet { I: Iterator, { let next_authorities = validators.map(|(_, k)| k).collect::>(); + if next_authorities.len() as u32 > T::MaxAuthorities::get() { + log::error!( + target: "runtime::beefy", + "authorities list {:?} truncated to length {}", + next_authorities, T::MaxAuthorities::get(), + ); + } let bounded_next_authorities = BoundedVec::<_, T::MaxAuthorities>::truncate_from(next_authorities); + let next_queued_authorities = queued_validators.map(|(_, k)| k).collect::>(); + if next_queued_authorities.len() as u32 > T::MaxAuthorities::get() { + log::error!( + target: "runtime::beefy", + "authorities list {:?} truncated to length {}", + next_queued_authorities, T::MaxAuthorities::get(), + ); + } let bounded_next_queued_authorities = BoundedVec::<_, T::MaxAuthorities>::truncate_from(next_queued_authorities); From 07617f5a80312d1c3b67d4abf3feb8cbad4d1a3d Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Mon, 6 Jun 2022 15:58:30 +0100 Subject: [PATCH 5/6] Update frame/beefy/src/lib.rs Co-authored-by: Adrian Catangiu --- frame/beefy/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index ea95fe8ef48e2..22f6cef4574e6 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -183,7 +183,7 @@ impl OneSessionHandler for Pallet { if next_queued_authorities.len() as u32 > T::MaxAuthorities::get() { log::error!( target: "runtime::beefy", - "authorities list {:?} truncated to length {}", + "queued authorities list {:?} truncated to length {}", next_queued_authorities, T::MaxAuthorities::get(), ); } From 58c410696fd56c77f044734abdc9f6b2750eea40 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Mon, 6 Jun 2022 16:39:04 +0100 Subject: [PATCH 6/6] cargo fmt --- frame/beefy/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 22f6cef4574e6..a0cba2270b37d 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -19,7 +19,11 @@ use codec::{Encode, MaxEncodedLen}; -use frame_support::{log, traits::{Get, OneSessionHandler}, BoundedSlice, BoundedVec, Parameter}; +use frame_support::{ + log, + traits::{Get, OneSessionHandler}, + BoundedSlice, BoundedVec, Parameter, +}; use sp_runtime::{ generic::DigestItem,