Bounded Validators / Authorities in New Sessions #8640
Conversation
github-actions
bot
added
A0-please_review
shawntabrizi
changed the title
shawntabrizi
added
B7-runtimenoteworthy
D9-needsaudit 👮
| @@ -332,6 +332,7 @@ parameter_types! { | |||
| pub const ExpectedBlockTime: Moment = MILLISECS_PER_BLOCK; | |||
| pub const ReportLongevity: u64 = | |||
| BondingDuration::get() as u64 * SessionsPerEra::get() as u64 * EpochDuration::get(); | |||
| pub const MaxAuthorities: u32 = 100; | |||
There was a problem hiding this comment.
Probably we should document that this should be the upper bound of what Staking::validator_count is ever going to be.
There was a problem hiding this comment.
Only major issue: We are silently dropping authorities at session listener pallets (aura, babe, grandpa etc.) and I think we should not.
Pushed a truncating_from as you asked for, should help also with my only comment. Basically, we should use this instead of a combination of .take and force_from to ensure we get the logs somewhere at least.
|
Part of paritytech/polkadot-sdk#323 |
| @@ -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> { | |||
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
But the missing point is:
- If session's
MaxAuthorityis 100 - and babe and aura's
MaxAuthorityis 50
I think we should prevent this at compile time but I don't it in the current state of the PR.
There was a problem hiding this comment.
oh no we have this guarantee as well, because of
type SessionHandler: SessionHandler<Self::ValidatorId, Self::MaxValidators>;
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, we should have an uniform number of authorities throughout the whole runtime.
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
There was a problem hiding this comment.
Overall the changes LGTM but I feel like we could maximize the utility of this by spreading the typed sized bound to more places, this is so that we can avoid truncating_from which I think diminishes the value of this change. I believe the two main causes for the back and forth between vec and bounded vec are:
SessionManagerstill uses regular vecs - I didn't look much deeply but I think it's probably not too hard to migrate it toBoundedVecas well.- The
SessionHandler/OneSessionHandlermethods use slices / iterators.SessionHandlermethods are probably not too hard to convert toBoundedVecas well.OneSessionHandlerI'm not sure what we should do. Ideally we would have aBoundedIteratortrait so that we don't lose the size type information. A weaker alternative would be some extension method on iterators that allows us to essentialy do a.collect_bounded<N>()where we get back aBoundedVec<_, N>(basically just hiding thetruncating_from).
| // Note this should never truncate because queued keys is also bounded to `MaxValidators`, | ||
| // but we do so defensively. |
There was a problem hiding this comment.
If BoundedVec add a map method we could use it here and guarantee at compile-time that we're getting a BoundedVec with the same size bound.
| let validators = BoundedVec::<T::ValidatorId, T::MaxValidators>::truncating_from( | ||
| validators, | ||
| Some("Session Rotate Session Maybe Next Validators"), | ||
| ); |
There was a problem hiding this comment.
Wouldn't it make sense that SessionManager is also parameterized on MaxValidators and SessionManager::new_session returns a BoundedVec instead?
| @@ -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> { | |||
There was a problem hiding this comment.
Yeah, we should have an uniform number of authorities throughout the whole runtime.
cwerling
added
D1-audited 👍
|
stale |
This adds a limit to the number of validators and authorities that can be set by the session handler.