Validator Re-Enabling #5724
Validator Re-Enabling #5724
Conversation
Overkillus
added
I1-security
There was a problem hiding this comment.
I had a quick pass by focusing mainly on the approach. It looks good, nice work @Overkillus!
I've left some thoughts about a corner case with the re-enabling.
| // Find the smallest offender to re-enable that is not higher than offender_slash_severity | ||
| if let Some((smallest_idx, _)) = currently_disabled | ||
| .iter() | ||
| .filter(|(_, perbill)| *perbill <= offender_slash_severity) |
There was a problem hiding this comment.
Just to clarify - even if there is no offender with a smaller offence we will re-enable the first guy with an offence equal to the new offender. Which is a good thing.
However I think we are biased here towards disabling the validator with the smallest index, which is wrong. If there are multiple elements with the same min value min_by_key returns the first one. If I am not mistaken DisabledValidators are sorted by key (here) so this way we will always reenable the guy with the smallest index.
It's a corner case but it would be nice to handle it if it is not too complicated.
There was a problem hiding this comment.
You are definitely right. I simply don't think this is anything to worry about. This will only kick in and matter only when we have offender of the same level of severity. Slashes will be dispensed and the newest offender will be disabled. From our perspective as long as lowest severity is re-enabled this is the lowest possible risk.
We will mention it to SRLabs on audit and see what the think. We can remedy as you mentioned with a random selection for a given severity level instead of arbitrary.
|
The CI pipeline was cancelled due to failure one of the required jobs. |
| @@ -727,7 +727,7 @@ pub mod pallet { | |||
| /// offended using binary search. | |||
| #[pallet::storage] | |||
| #[pallet::unbounded] | |||
| pub type DisabledValidators<T: Config> = StorageValue<_, Vec<u32>, ValueQuery>; | |||
| pub type DisabledValidators<T: Config> = StorageValue<_, Vec<(u32, Perbill)>, ValueQuery>; | |||
There was a problem hiding this comment.
I think we should create a separate type OffenceSeverity that implements Ord and at the moment is only a wrapper around PerBill, but could be changed in the future to compare two severities with equal slash in the different way.
There was a problem hiding this comment.
Also, we need to account for #64 here by changing the runtime API like
polkadot-sdk/polkadot/runtime/parachains/src/runtime_api_impl/v11.rs
Lines 459 to 462 in f7119e4
| // Offender is already disabled, update severity if the new one is higher | ||
| Ok(index) => { | ||
| let (_, old_severity) = &mut disabled[index]; | ||
| if params.slash > *old_severity { |
There was a problem hiding this comment.
is this a cumulitive slash or only the one coming from the latest offence? are we relying on the fact that we only slash for the max in the era and do not accumulate slashes here? all implicit assumptions would need to be documented
| // Remove the validator from `DisabledValidators` and re-enable it. | ||
| if let Ok(index) = disabled.binary_search_by_key(&reenable_idx, |(index, _)| *index) { | ||
| disabled.remove(index); | ||
| // Propagate re-enablement to session level |
There was a problem hiding this comment.
i got confused by having the same storage name DisabledValidators in slashing and in session. maybe we could rename the one in staking to DisabledValidatorsWithSeverity or something like that
Co-authored-by: ordian <write@reusable.software>
Aims to implement Stage 3 of Validator Disbling as outlined here: #4359
Features:
Testing & Security:
Closes #4745
Closes #2418