[AHM] Moves disabling logic into pallet-session #7581
Conversation
…into gpestana/epm-mb
Ank4n
requested review from
tdimitrov,
kianenigma,
seadanda and
Overkillus
and removed request for
acatangiu
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| /// Note: This sets the OffenceSeverity to the lowest value. | ||
| pub fn disable_index(i: u32) -> bool { | ||
| if i >= Validators::<T>::decode_len().unwrap_or(0) as u32 { | ||
| if i >= Validators::<T>::decode_len().defensive_unwrap_or(0) as u32 { | ||
| return false | ||
| } | ||
|
|
||
| DisabledValidators::<T>::mutate(|disabled| { | ||
| if let Err(index) = disabled.binary_search(&i) { | ||
| disabled.insert(index, i); | ||
| if let Err(index) = disabled.binary_search_by_key(&i, |(index, _)| *index) { | ||
| disabled.insert(index, (i, OffenceSeverity(Perbill::zero()))); |
There was a problem hiding this comment.
Why are we defaulting to lowest severity? And what is the purpose of this logic? It is not being used for report_offence as you directly modify the DisabledValidators. Just for testing then?
What worries me is that we expose a way of disabling validators that is dangerous (default to lowest priority). At least requiring a severity would make it less prone to mistakes.
And if the argument is to expose it to other pallets then shouldn't we also have the enable_index that you removed a few lines below?
Another option is to make report_offence depend on disable_index / enable_index instead of making direct changes to disabledValidators
| @@ -631,7 +653,7 @@ impl<T: Config> Pallet<T> { | |||
|
|
|||
| /// Public function to access the disabled validators. | |||
| pub fn disabled_validators() -> Vec<u32> { | |||
| DisabledValidators::<T>::get() | |||
| DisabledValidators::<T>::get().iter().map(|(i, _)| *i).collect() | |||
There was a problem hiding this comment.
dont we want to expose the severities as well?
| @@ -740,23 +764,6 @@ impl<T: Config> Pallet<T> { | |||
| }) | |||
| } | |||
|
|
|||
| /// Re-enable the validator of index `i`, returns `false` if the validator was already enabled. | |||
There was a problem hiding this comment.
see comment on disable_index
| @@ -2745,6 +2741,7 @@ impl<T: Config> Pallet<T> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| /* todo(ank4n): move to session try runtime | |||
|
|
||
| // Clear disabled validators. | ||
| <DisabledValidators<T>>::kill(); |
There was a problem hiding this comment.
original impl only killed DisabledValidators on era change, so they persisted between sessions. It is quite important so I'm bringing it to light explicitly. Is this behaviour replicated in the refactor?
| assert!(disabling_decision.disable.is_none() && disabling_decision.reenable.is_none()); | ||
| }); | ||
| } | ||
| // todo(ank4n): Ensure there is a test that for older eras, the disabling strategy does not |
closes #6508.
TODO
DisabledValidatorsboth in pallet-session and pallet-staking.DisabledValidators.