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

Use BoundedVec in aura pallet #11617

Merged
merged 2 commits into from
Jun 10, 2022
Merged

Use BoundedVec in aura pallet #11617

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44653c5
Use BoundedVec in aura pallet
KiChjang Jun 7, 2022
58e5662
cargo fmt
KiChjang Jun 7, 2022
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
19 changes: 12 additions & 7 deletions frame/aura/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@

use codec::{Decode, Encode, MaxEncodedLen};
use frame_support::{
log,
traits::{DisabledValidators, FindAuthor, Get, OnTimestampSet, OneSessionHandler},
BoundedSlice, ConsensusEngineId, Parameter, WeakBoundedVec,
BoundedSlice, BoundedVec, ConsensusEngineId, Parameter,
};
use sp_consensus_aura::{AuthorityIndex, ConsensusLog, Slot, AURA_ENGINE_ID};
use sp_runtime::{
Expand Down Expand Up @@ -116,7 +117,7 @@ pub mod pallet {
#[pallet::storage]
#[pallet::getter(fn authorities)]
pub(super) type Authorities<T: Config> =
StorageValue<_, WeakBoundedVec<T::AuthorityId, T::MaxAuthorities>, ValueQuery>;
StorageValue<_, BoundedVec<T::AuthorityId, T::MaxAuthorities>, ValueQuery>;

/// The current slot of this block.
///
Expand Down Expand Up @@ -150,7 +151,7 @@ impl<T: Config> Pallet<T> {
///
/// The storage will be applied immediately.
/// And aura consensus log will be appended to block's log.
pub fn change_authorities(new: WeakBoundedVec<T::AuthorityId, T::MaxAuthorities>) {
pub fn change_authorities(new: BoundedVec<T::AuthorityId, T::MaxAuthorities>) {
<Authorities<T>>::put(&new);

let log = DigestItem::Consensus(
Expand Down Expand Up @@ -219,10 +220,14 @@ impl<T: Config> OneSessionHandler<T::AccountId> for Pallet<T> {
let next_authorities = validators.map(|(_, k)| k).collect::<Vec<_>>();
let last_authorities = Self::authorities();
if last_authorities != next_authorities {
let bounded = <WeakBoundedVec<_, T::MaxAuthorities>>::force_from(
next_authorities,
Some("AuRa new session"),
);
if next_authorities.len() as u32 > T::MaxAuthorities::get() {
Copy link
Member

Choose a reason for hiding this comment

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

The force_from from the WeakBoundedVec does not trunc. So I think since it now truncs we should either do a debug_assert or a defensive failure here, so that we definitely see this.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't a better solution here one that is a bit more comprehensive? Extend the OnSessionHandler to have a const for MaxAuthorities, then we don't use an iterator, but instead expect a bounded vec in the API, and then no truncation is necessary?

I mean, this also does not seem that bad to me to just truncate, but seems to me if we would re-write this API to begin with, it would look different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rewriting the OneSessionHandler API sounds good too, I'll need a bit more context however, but will attempt in another PR.

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 is is very bad because it is unexpected! In other places genesis just panics, so making this fail without a panic is super confusing for the end users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but on_new_session is run whenever there's a new session, so it's not necessarily just on genesis which this function is called, hence we don't panic here. The previous code also did not panic using force_from, but in places where the function is run only on genesis (e.g. initialize_authorities), then we do panic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, sorry I misread the code. Makes sense.

log::warn!(
target: "runtime::aura",
"next authorities list larger than {}, truncating",
T::MaxAuthorities::get(),
);
}
let bounded = <BoundedVec<_, T::MaxAuthorities>>::truncate_from(next_authorities);
Copy link
Contributor

Choose a reason for hiding this comment

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

@KiChjang What does truncate_from mean here? we got an error that after running the node for sometime, the authorities length changed to be zero

Copy link
Member

Choose a reason for hiding this comment

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

truncate_from should be that if next_authorities is 100 people, and the MaxAuthorities is 50, only the first 50 will be placed in storage, and the last 50 will be truncated.

Self::change_authorities(bounded);
}
}
Expand Down