session, staking: introduce static limit on the number of validators #11967
Conversation
Doordashcon
changed the title
|
@andresilva is this the general direction you envisioned? |
kianenigma
added
A0-please_review
frame/session/src/lib.rs
Outdated
| @@ -647,7 +653,9 @@ impl<T: Config> Pallet<T> { | |||
| let session_keys = <QueuedKeys<T>>::get(); | |||
| let validators = | |||
| session_keys.iter().map(|(validator, _)| validator.clone()).collect::<Vec<_>>(); | |||
| Validators::<T>::put(&validators); | |||
|
|
|||
| let validator2: BoundedVec<T::ValidatorId, T::ValidatorSet> = validators.clone().try_into().expect("Too many current validators"); | |||
There was a problem hiding this comment.
You certainly cannot and should not panic here -- need to handle it defensively, either use WeakBoundedVec, or truncate to fit (better).
There was a problem hiding this comment.
not yet -- you cannot panic in any runtime code path such as on_initialize or a transaction.
There was a problem hiding this comment.
Mapping to a custom error would be ideal?
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
| Validators::<T>::put(&validators); | ||
|
|
||
| let bounded_validator_set: WeakBoundedVec<T::ValidatorId, T::MaxValidators> = | ||
| validators.clone().try_into().expect("Max validators reached"); |
There was a problem hiding this comment.
this panic is still not valid.
There was a problem hiding this comment.
Still creating some panicing code that is not allowed.
The correct way to to do this is actually to bound the interfaces linking staking and session pallet as well, something that is not done yet but is toyed with as a part of #11499 and alike.
For example, the session pallet should be able to ENSURE that its SessionManager does not return more than x validators.
This is all likely to be part of a greater effort to bound and fix all the staking related pallets. We can continue trying it out here, but I can't guarantee that it will be easy 😅
Attempts to resolve #9724
polkadot companion: paritytech/polkadot#5905
cumulus companion: paritytech/cumulus#1558