Validator disabling in session enhancements #7663
Conversation
Overkillus
added
T8-polkadot
Overkillus
changed the title
|
Looks good so far. |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| log::trace!( | ||
| target: "runtime::session", | ||
| log!( | ||
| trace, |
There was a problem hiding this comment.
here and elsewhere: would be gud to bring the target: LOG_TARGET back, otherwise, setting trace level logs will be painful
| impl Default for OffenceSeverity { | ||
| fn default() -> Self { | ||
| Self(Perbill::from_percent(100)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
i'm not sure this is intuitive implementation. i would suggest instead create an inherent method named fn most_severe() -> Self to be more explicit
There was a problem hiding this comment.
This is quite intentional but I can change it if you guys think so. The intention was to make sure that if severity is unknown or unclear you default to max. AKA if severity is unknown I'd rather have the offender never re-enabled (max severity) than them at 0% and re-enable them too easily.
Not having this default made it so Ankan mistakenly also assumed that lowest severity is a sensible default when it isn't.
Still thinking I should change it?
There was a problem hiding this comment.
I was about to suggest to have a function max_severity() but @ordian was faster.
What you say makes sense but I still think it's not readable enough. You can add a max_severity()/most_severe() initialiser and call it in default)() if you insist on default behaviour.
That way it will be clear that a) 100% is the highest severity and b) your default value concern: when in doubt - slash them hard.
There was a problem hiding this comment.
Just to be clear this is NOT slashing anyone :D it's purely for purposes of disabling but I like the compromise Tsveto proposes 👍
| @@ -81,7 +83,7 @@ pub mod v17 { | |||
|
|
|||
| #[frame_support::storage_alias] | |||
| pub type DisabledValidators<T: Config> = | |||
| StorageValue<Pallet<T>, BoundedVec<(u32, OffenceSeverity), ConstU32<100>>, ValueQuery>; | |||
| StorageValue<Pallet<T>, BoundedVec<(u32, OffenceSeverity), ConstU32<300>>, ValueQuery>; | |||
There was a problem hiding this comment.
should this be 333 instead? assuming f out of 3f + 1 (1000 eventually)
| DisabledValidators::<T>::mutate(|disabled| { | ||
| if let Ok(index) = disabled.binary_search_by_key(&i, |(index, _)| *index) { | ||
| log!(trace, "reenabling validator {:?}", i); | ||
| Self::deposit_event(Event::ValidatorReenabled { validator: Validators::<T>::get()[index as usize].clone() }); |
There was a problem hiding this comment.
not introduced here, but direct indexing makes me uneasy if index is somehow out of range for Validators (e.g. due to a bug) - this will make the runtime panic!
| impl Default for OffenceSeverity { | ||
| fn default() -> Self { | ||
| Self(Perbill::from_percent(100)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I was about to suggest to have a function max_severity() but @ordian was faster.
What you say makes sense but I still think it's not readable enough. You can add a max_severity()/most_severe() initialiser and call it in default)() if you insist on default behaviour.
That way it will be clear that a) 100% is the highest severity and b) your default value concern: when in doubt - slash them hard.
| @@ -49,14 +49,14 @@ impl<T: Config> MigrateDisabledValidators for InitOffenceSeverity<T> { | |||
| fn peek_disabled() -> Vec<(u32, OffenceSeverity)> { | |||
| DisabledValidators::<T>::get() | |||
| .iter() | |||
| .map(|v| (*v, OffenceSeverity(Perbill::zero()))) | |||
| .map(|v| (*v, OffenceSeverity::default())) | |||
There was a problem hiding this comment.
For example here default() doesn't mean much to the readed in comparison with most_severe().
| @@ -81,7 +83,7 @@ pub mod v17 { | |||
|
|
|||
| #[frame_support::storage_alias] | |||
| pub type DisabledValidators<T: Config> = | |||
| StorageValue<Pallet<T>, BoundedVec<(u32, OffenceSeverity), ConstU32<100>>, ValueQuery>; | |||
| StorageValue<Pallet<T>, BoundedVec<(u32, OffenceSeverity), ConstU32<300>>, ValueQuery>; | |||
There was a problem hiding this comment.
Didn't we wanted to merge v16 and v17?
On top of 7581.
Adds the missing testing cases and adresses some overdue review feedback post merge.