From 3d438dec2ba261d801411346e1ab650b93fd657e Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 27 Aug 2024 13:03:58 +0200 Subject: [PATCH 01/48] expand DisabledValidators with severity --- substrate/frame/staking/src/lib.rs | 97 ++++++++++++++++++--- substrate/frame/staking/src/pallet/impls.rs | 4 +- substrate/frame/staking/src/pallet/mod.rs | 2 +- substrate/frame/staking/src/slashing.rs | 61 +++++++++---- 4 files changed, 134 insertions(+), 30 deletions(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 9e59cbd3d0cb..8a17948ba474 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1270,13 +1270,17 @@ impl BenchmarkingConfig for TestBenchmarkingConfig { /// Controls validator disabling pub trait DisablingStrategy { - /// Make a disabling decision. Returns the index of the validator to disable or `None` if no new - /// validator should be disabled. + /// Make a disabling decision. Potentially returns 2 validators indices. + /// First index is to be disabled, second is to be re-enabled. + /// Options: + /// (None, None) no changes needed + /// (Some(x), None) just disable x + /// (Some(x), Some(y)) disable x instead of y fn decision( offender_stash: &T::AccountId, slash_era: EraIndex, - currently_disabled: &Vec, - ) -> Option; + currently_disabled: &Vec<(u32, Perbill)>, + ) -> (Option, Option); } /// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a @@ -1306,8 +1310,8 @@ impl DisablingStrategy fn decision( offender_stash: &T::AccountId, slash_era: EraIndex, - currently_disabled: &Vec, - ) -> Option { + currently_disabled: &Vec<(u32, Perbill)>, + ) -> (Option, Option) { let active_set = T::SessionInterface::validators(); // We don't disable more than the limit @@ -1317,7 +1321,7 @@ impl DisablingStrategy "Won't disable: reached disabling limit {:?}", Self::disable_limit(active_set.len()) ); - return None + return (None, None) } // We don't disable for offences in previous eras @@ -1328,18 +1332,91 @@ impl DisablingStrategy Pallet::::current_era().unwrap_or_default(), slash_era ); - return None + return (None, None) } let offender_idx = if let Some(idx) = active_set.iter().position(|i| i == offender_stash) { idx as u32 } else { log!(debug, "Won't disable: offender not in active set",); - return None + return (None, None) + }; + + log!(debug, "Will disable {:?}", offender_idx); + + (Some(offender_idx), None) + } +} + +/// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a +/// limit and if the limit is reached and the new offender is higher (bigger punishment/severity) +/// then it re-enables to lowest offender to free up space for the new offender. +/// +/// An extension of [`UpToLimitDisablingStrategy`]. +pub struct UpToLimitWithReEnablingDisablingStrategy; + +impl UpToLimitWithReEnablingDisablingStrategy { + /// Disabling limit calculated from the total number of validators in the active set. When + /// reached re-enabling logic might kick in. + pub fn disable_limit(validators_len: usize) -> usize { + validators_len + .saturating_sub(1) + .checked_div(DISABLING_LIMIT_FACTOR) + .unwrap_or_else(|| { + defensive!("DISABLING_LIMIT_FACTOR should not be 0"); + 0 + }) + } +} + +impl DisablingStrategy + for UpToLimitWithReEnablingDisablingStrategy +{ + fn decision( + offender_stash: &T::AccountId, + slash_era: EraIndex, + currently_disabled: &Vec<(u32, Perbill)>, + ) -> (Option, Option) { + let active_set = T::SessionInterface::validators(); + + // We don't disable for offences in previous eras + if ActiveEra::::get().map(|e| e.index).unwrap_or_default() > slash_era { + log!( + debug, + "Won't disable: current_era {:?} > slash_era {:?}", + Pallet::::current_era().unwrap_or_default(), + slash_era + ); + return (None, None) + } + + let offender_idx = if let Some(idx) = active_set.iter().position(|i| i == offender_stash) { + idx as u32 + } else { + log!(debug, "Won't disable: offender not in active set",); + return (None, None) }; + // We don't disable more than the limit + if currently_disabled.len() >= Self::disable_limit(active_set.len()) { + log!( + debug, + "Reached disabling limit {:?}, checking for re-enabling", + Self::disable_limit(active_set.len()) + ); + + // Find the smallest offender to re-enable + if let Some((smallest_idx, _)) = currently_disabled.iter().min_by_key(|(_, perbill)| *perbill) { + log!(debug, "Will disable {:?} and re-enable {:?}", offender_idx, smallest_idx); + return (Some(offender_idx), Some(*smallest_idx)); + } else { + log!(debug, "No smaller offender found to re-enable"); + return (None, None); + } + } + log!(debug, "Will disable {:?}", offender_idx); - Some(offender_idx) + (Some(offender_idx), None) } } diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 2df3bc084eb0..b232f74a3846 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -508,7 +508,7 @@ impl Pallet { } // disable all offending validators that have been disabled for the whole era - for index in >::get() { + for (index, _) in >::get() { T::SessionInterface::disable_validator(index); } } @@ -2299,7 +2299,7 @@ impl Pallet { fn ensure_disabled_validators_sorted() -> Result<(), TryRuntimeError> { ensure!( - DisabledValidators::::get().windows(2).all(|pair| pair[0] <= pair[1]), + DisabledValidators::::get().windows(2).all(|pair| pair[0].0 <= pair[1].0), "DisabledValidators is not sorted" ); Ok(()) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 79f9d298ada7..9e39b72f61db 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -727,7 +727,7 @@ pub mod pallet { /// offended using binary search. #[pallet::storage] #[pallet::unbounded] - pub type DisabledValidators = StorageValue<_, Vec, ValueQuery>; + pub type DisabledValidators = StorageValue<_, Vec<(u32, Perbill)>, ValueQuery>; /// The threshold for when users can start calling `chill_other` for other validators / /// nominators. The threshold is compared to the actual number of validators / nominators diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 9bc8197c50b3..bb6bc801743c 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -52,7 +52,7 @@ use crate::{ BalanceOf, Config, DisabledValidators, DisablingStrategy, Error, Exposure, NegativeImbalanceOf, NominatorSlashInEra, Pallet, Perbill, SessionInterface, SpanSlash, UnappliedSlash, - ValidatorSlashInEra, + ValidatorSlashInEra, log, }; use alloc::vec::Vec; use codec::{Decode, Encode, MaxEncodedLen}; @@ -321,23 +321,50 @@ fn kick_out_if_recent(params: SlashParams) { } /// Inform the [`DisablingStrategy`] implementation about the new offender and disable the list of -/// validators provided by [`make_disabling_decision`]. +/// validators provided by [`decision`]. fn add_offending_validator(params: &SlashParams) { - DisabledValidators::::mutate(|disabled| { - if let Some(offender) = - T::DisablingStrategy::decision(params.stash, params.slash_era, &disabled) - { - // Add the validator to `DisabledValidators` and disable it. Do nothing if it is - // already disabled. - if let Err(index) = disabled.binary_search_by_key(&offender, |index| *index) { - disabled.insert(index, offender); - T::SessionInterface::disable_validator(offender); - } - } - }); - - // `DisabledValidators` should be kept sorted - debug_assert!(DisabledValidators::::get().windows(2).all(|pair| pair[0] < pair[1])); + DisabledValidators::::mutate(|disabled| { + let (offender, reenable) = + T::DisablingStrategy::decision(params.stash, params.slash_era, &disabled); + + match (offender, reenable) { + (None, None) => { + // Do nothing + } + (Some(offender_idx), None) => { + // Add the validator to `DisabledValidators` and disable it. Do nothing if it is + // already disabled. + if let Err(index) = disabled.binary_search_by_key(&offender_idx, |index| *index) { + disabled.insert(index, offender_idx); + // Propagate disablement to session level + T::SessionInterface::disable_validator(offender_idx); + } + } + (Some(offender_idx), Some(reenable_idx)) => { + // 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 + T::SessionInterface::enable_validator(reenable_idx); + } + + // Add the validator to `DisabledValidators` and disable it. Do nothing if it is + // already disabled. + if let Err(index) = disabled.binary_search_by_key(&offender_idx, |index| *index) { + disabled.insert(index, offender_idx); + // Propagate disablement to session level + T::SessionInterface::disable_validator(offender_idx); + } + } + _ => { + // This case should not happen, but we handle it defensively + log!(error, "Unexpected decision result: {:?}", (offender, reenable)); + } + } + }); + + // `DisabledValidators` should be kept sorted + debug_assert!(DisabledValidators::::get().windows(2).all(|pair| pair[0] < pair[1])); } /// Slash nominators. Accepts general parameters and the prior slash percentage of the validator. From b2d21dd675fb28ac0327e3f726e076bc4164118c Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 26 Aug 2024 13:15:15 +0200 Subject: [PATCH 02/48] comment --- substrate/frame/staking/src/pallet/impls.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index b232f74a3846..15ac3fd2c9d9 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -2297,6 +2297,7 @@ impl Pallet { Ok(()) } + // Sorted by index fn ensure_disabled_validators_sorted() -> Result<(), TryRuntimeError> { ensure!( DisabledValidators::::get().windows(2).all(|pair| pair[0].0 <= pair[1].0), From 4c01a5678898607bd703f1364d02b6bcc2550763 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 28 Aug 2024 09:55:49 +0200 Subject: [PATCH 03/48] migration to disabling with severity --- substrate/frame/staking/src/migrations.rs | 34 +++++++++++++++++++++++ substrate/frame/staking/src/slashing.rs | 15 ++++++---- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index 5c9cf8613213..6bc62009206b 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -60,6 +60,37 @@ impl Default for ObsoleteReleases { #[storage_alias] type StorageVersion = StorageValue, ObsoleteReleases, ValueQuery>; +/// Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>` to track offense severity for re-enabling purposes. +pub mod v16 { + use super::*; + + // The disabling strategy used by staking pallet + type DefaultDisablingStrategy = UpToLimitWithReEnablingDisablingStrategy; + pub struct VersionUncheckedMigrateV15ToV16(core::marker::PhantomData); + impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV15ToV16 { + fn on_runtime_upgrade() -> Weight { + // Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>`. + // Using max severity (PerBill) for the migration which effectively makes it so offenders before the migration will never be re-enabled. + let max_perbill = Perbill::from_percent(100); + // Inject severity + let migrated = v15::DisabledValidators::::take().into_iter().map(|v| (v, max_perbill)).collect::>(); + + DisabledValidators::::set(migrated); + + log!(info, "v16 applied successfully."); + T::DbWeight::get().reads_writes(1, 1) + } + } + + pub type MigrateV14ToV15 = VersionedMigration< + 15, + 16, + VersionUncheckedMigrateV15ToV16, + Pallet, + ::DbWeight, + >; +} + /// Migrating `OffendingValidators` from `Vec<(u32, bool)>` to `Vec` pub mod v15 { use super::*; @@ -67,6 +98,9 @@ pub mod v15 { // The disabling strategy used by staking pallet type DefaultDisablingStrategy = UpToLimitDisablingStrategy; + #[storage_alias] + pub(crate) type DisabledValidators = StorageValue, Vec, ValueQuery>; + pub struct VersionUncheckedMigrateV14ToV15(core::marker::PhantomData); impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV14ToV15 { fn on_runtime_upgrade() -> Weight { diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index bb6bc801743c..c4d4425ce9fa 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -334,24 +334,27 @@ fn add_offending_validator(params: &SlashParams) { (Some(offender_idx), None) => { // Add the validator to `DisabledValidators` and disable it. Do nothing if it is // already disabled. - if let Err(index) = disabled.binary_search_by_key(&offender_idx, |index| *index) { - disabled.insert(index, offender_idx); + if let Err(index) = disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { + let severity = params.slash; + disabled.insert(index, (offender_idx, severity)); // Propagate disablement to session level T::SessionInterface::disable_validator(offender_idx); } } (Some(offender_idx), Some(reenable_idx)) => { // Remove the validator from `DisabledValidators` and re-enable it. - if let Ok(index) = disabled.binary_search_by_key(&reenable_idx, |index| *index) { + if let Ok(index) = disabled.binary_search_by_key(&reenable_idx, |(index, _)| *index) { disabled.remove(index); // Propagate re-enablement to session level - T::SessionInterface::enable_validator(reenable_idx); + // Placeholder for enabling a validator, to be implemented later + todo!("T::SessionInterface::enable_validator(reenable_idx)"); } // Add the validator to `DisabledValidators` and disable it. Do nothing if it is // already disabled. - if let Err(index) = disabled.binary_search_by_key(&offender_idx, |index| *index) { - disabled.insert(index, offender_idx); + if let Err(index) = disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { + let severity = params.slash; + disabled.insert(index, (offender_idx, severity)); // Propagate disablement to session level T::SessionInterface::disable_validator(offender_idx); } From 996dfab1ca9b5ae2037b91d534d499f386fde38f Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 28 Aug 2024 16:23:01 +0200 Subject: [PATCH 04/48] initial func tests --- substrate/frame/staking/src/tests.rs | 108 ++++++++++++++++++++++++--- 1 file changed, 98 insertions(+), 10 deletions(-) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index ab2c00ca9ccc..171c5b3a69ae 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -8259,13 +8259,15 @@ mod ledger_recovery { mod byzantine_threshold_disabling_strategy { use crate::{ - tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitDisablingStrategy, + tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitDisablingStrategy, UpToLimitWithReEnablingDisablingStrategy, }; - use sp_staking::EraIndex; + use sp_runtime::Perbill; +use sp_staking::EraIndex; // Common test data - the stash of the offending validator, the era of the offence and the // active set const OFFENDER_ID: ::AccountId = 7; + const OFFENDER_SLASH: Perbill = Perbill::from_percent(100); const SLASH_ERA: EraIndex = 1; const ACTIVE_SET: [::ValidatorId; 7] = [1, 2, 3, 4, 5, 6, 7]; const OFFENDER_VALIDATOR_IDX: u32 = 6; // the offender is with index 6 in the active set @@ -8277,48 +8279,134 @@ mod byzantine_threshold_disabling_strategy { pallet_session::Validators::::put(ACTIVE_SET.to_vec()); ActiveEra::::put(ActiveEraInfo { index: 2, start: None }); - let disable_offender = + let disabling_decision = >::decision( &OFFENDER_ID, + OFFENDER_SLASH, SLASH_ERA, &initially_disabled, ); - assert!(disable_offender.is_none()); + assert!(disabling_decision.0.is_none() && disabling_decision.1.is_none()); }); } #[test] fn dont_disable_beyond_byzantine_threshold() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![1, 2]; + let max_slash = Perbill::from_percent(100); + let initially_disabled = vec![(1, max_slash), (2, max_slash)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); - let disable_offender = + let disabling_decision = >::decision( &OFFENDER_ID, + OFFENDER_SLASH, SLASH_ERA, &initially_disabled, ); - assert!(disable_offender.is_none()); + assert!(disabling_decision.0.is_none() && disabling_decision.1.is_none()); }); } #[test] fn disable_when_below_byzantine_threshold() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![1]; + let max_slash = Perbill::from_percent(100); + let initially_disabled = vec![(1, max_slash)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); - let disable_offender = + let disabling_decision = >::decision( &OFFENDER_ID, + OFFENDER_SLASH, SLASH_ERA, &initially_disabled, ); - assert_eq!(disable_offender, Some(OFFENDER_VALIDATOR_IDX)); + assert_eq!(disabling_decision.0, Some(OFFENDER_VALIDATOR_IDX)); }); } + + } + +mod disabling_strategy_with_reenabling { + use crate::{ + tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitWithReEnablingDisablingStrategy, + }; + use sp_runtime::Perbill; +use sp_staking::EraIndex; + + // Common test data - the stash of the offending validator, the era of the offence and the + // active set + const OFFENDER_ID: ::AccountId = 7; + const OFFENDER_SLASH_MINOR: Perbill = Perbill::from_percent(10); + const OFFENDER_SLASH_MAJOR: Perbill = Perbill::from_percent(100); + const SLASH_ERA: EraIndex = 1; + const ACTIVE_SET: [::ValidatorId; 7] = [1, 2, 3, 4, 5, 6, 7]; + const OFFENDER_VALIDATOR_IDX: u32 = 6; // the offender is with index 6 in the active set + + #[test] + fn dont_disable_for_ancient_offence() { + sp_io::TestExternalities::default().execute_with(|| { + let initially_disabled = vec![]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + ActiveEra::::put(ActiveEraInfo { index: 2, start: None }); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MAJOR, + SLASH_ERA, + &initially_disabled, + ); + + assert!(disabling_decision.0.is_none() && disabling_decision.1.is_none()); + }); + } + + #[test] + fn disable_when_below_byzantine_threshold() { + sp_io::TestExternalities::default().execute_with(|| { + let max_slash = Perbill::from_percent(100); + let initially_disabled = vec![(1, max_slash)]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MAJOR, + SLASH_ERA, + &initially_disabled, + ); + + // Disable Offender and do not re-enable anyone + assert_eq!(disabling_decision.0, Some(OFFENDER_VALIDATOR_IDX)); + assert_eq!(disabling_decision.1, None); + }); + } + + #[test] + fn reenable_arbitrary_on_equal_severity() { + sp_io::TestExternalities::default().execute_with(|| { + let max_slash = Perbill::from_percent(100); + let initially_disabled = vec![(1, max_slash), (2, max_slash)]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MAJOR, + SLASH_ERA, + &initially_disabled, + ); + + assert!(disabling_decision.0.is_some() && disabling_decision.1.is_some()); + // Disable 7 and enable 1 + assert_eq!(disabling_decision.0.unwrap(), OFFENDER_VALIDATOR_IDX); + assert_eq!(disabling_decision.1.unwrap(), 1); + }); + } +} \ No newline at end of file From d908f65d58c22cf27f09988ae2e457c3b9d68c22 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 28 Aug 2024 16:23:22 +0200 Subject: [PATCH 05/48] session val index enabling --- substrate/frame/session/src/lib.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 325758d54dd8..dcbf4df74ba4 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -735,6 +735,23 @@ impl Pallet { }) } + /// Re-enable the validator of index `i`, returns `false` if the validator was already enabled. + pub fn enable_index(i: u32) -> bool { + if i >= Validators::::decode_len().unwrap_or(0) as u32 { + return false + } + + // If the validator is not disabled, return false. + DisabledValidators::::mutate(|disabled| { + if let Ok(index) = disabled.binary_search(&i) { + disabled.remove(index); + true + } else { + false + } + }) + } + /// Disable the validator identified by `c`. (If using with the staking pallet, /// this would be their *stash* account.) /// From ec350ff2b9f632231b20d67b8f417145d74bd1f9 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 28 Aug 2024 16:24:20 +0200 Subject: [PATCH 06/48] Disabling Decision Abstraction --- substrate/frame/staking/src/lib.rs | 40 +++++++++++++++++++------ substrate/frame/staking/src/slashing.rs | 10 +++---- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 8a17948ba474..b6cee2f06267 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -848,6 +848,9 @@ pub trait SessionInterface { /// Disable the validator at the given index, returns `false` if the validator was already /// disabled or the index is out of bounds. fn disable_validator(validator_index: u32) -> bool; + /// Re-enable a validator that was previously disabled. Returns `false` if the validator was + /// already enabled or the index is out of bounds. + fn enable_validator(validator_index: u32) -> bool; /// Get the validators from session. fn validators() -> Vec; /// Prune historical session tries up to but not including the given index. @@ -872,6 +875,10 @@ where >::disable_index(validator_index) } + fn enable_validator(validator_index: u32) -> bool { + >::enable_index(validator_index) + } + fn validators() -> Vec<::AccountId> { >::validators() } @@ -885,6 +892,9 @@ impl SessionInterface for () { fn disable_validator(_: u32) -> bool { true } + fn enable_validator(_: u32) -> bool { + true + } fn validators() -> Vec { Vec::new() } @@ -1278,9 +1288,16 @@ pub trait DisablingStrategy { /// (Some(x), Some(y)) disable x instead of y fn decision( offender_stash: &T::AccountId, + offender_slash_severity: Perbill, slash_era: EraIndex, currently_disabled: &Vec<(u32, Perbill)>, - ) -> (Option, Option); + ) -> DisablingDecision; +} + +#[derive(Debug)] +pub struct DisablingDecision { + pub disable: Option, + pub reenable: Option, } /// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a @@ -1309,9 +1326,10 @@ impl DisablingStrategy { fn decision( offender_stash: &T::AccountId, + offender_slash_severity: Perbill, slash_era: EraIndex, currently_disabled: &Vec<(u32, Perbill)>, - ) -> (Option, Option) { + ) -> DisablingDecision { let active_set = T::SessionInterface::validators(); // We don't disable more than the limit @@ -1321,7 +1339,7 @@ impl DisablingStrategy "Won't disable: reached disabling limit {:?}", Self::disable_limit(active_set.len()) ); - return (None, None) + return DisablingDecision { disable: None, reenable: None } } // We don't disable for offences in previous eras @@ -1374,9 +1392,10 @@ impl DisablingStrategy { fn decision( offender_stash: &T::AccountId, + offender_slash_severity: Perbill, slash_era: EraIndex, currently_disabled: &Vec<(u32, Perbill)>, - ) -> (Option, Option) { + ) -> DisablingDecision { let active_set = T::SessionInterface::validators(); // We don't disable for offences in previous eras @@ -1389,7 +1408,6 @@ impl DisablingStrategy ); return (None, None) } - let offender_idx = if let Some(idx) = active_set.iter().position(|i| i == offender_stash) { idx as u32 } else { @@ -1397,7 +1415,7 @@ impl DisablingStrategy return (None, None) }; - // We don't disable more than the limit + // We don't disable more than the limit (but we can re-enable a smaller offender) if currently_disabled.len() >= Self::disable_limit(active_set.len()) { log!( debug, @@ -1405,8 +1423,12 @@ impl DisablingStrategy Self::disable_limit(active_set.len()) ); - // Find the smallest offender to re-enable - if let Some((smallest_idx, _)) = currently_disabled.iter().min_by_key(|(_, perbill)| *perbill) { + // 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) + .min_by_key(|(_, perbill)| *perbill) + { log!(debug, "Will disable {:?} and re-enable {:?}", offender_idx, smallest_idx); return (Some(offender_idx), Some(*smallest_idx)); } else { @@ -1415,8 +1437,8 @@ impl DisablingStrategy } } + // If we are not at the limit, just disable the new offender and dont re-enable anyone log!(debug, "Will disable {:?}", offender_idx); - (Some(offender_idx), None) } } diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index c4d4425ce9fa..79ce39fbf224 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -324,10 +324,9 @@ fn kick_out_if_recent(params: SlashParams) { /// validators provided by [`decision`]. fn add_offending_validator(params: &SlashParams) { DisabledValidators::::mutate(|disabled| { - let (offender, reenable) = - T::DisablingStrategy::decision(params.stash, params.slash_era, &disabled); + let decision = T::DisablingStrategy::decision(params.stash, params.slash, params.slash_era, &disabled); - match (offender, reenable) { + match (decision.disable, decision.reenable) { (None, None) => { // Do nothing } @@ -346,8 +345,7 @@ fn add_offending_validator(params: &SlashParams) { if let Ok(index) = disabled.binary_search_by_key(&reenable_idx, |(index, _)| *index) { disabled.remove(index); // Propagate re-enablement to session level - // Placeholder for enabling a validator, to be implemented later - todo!("T::SessionInterface::enable_validator(reenable_idx)"); + T::SessionInterface::enable_validator(reenable_idx); } // Add the validator to `DisabledValidators` and disable it. Do nothing if it is @@ -361,7 +359,7 @@ fn add_offending_validator(params: &SlashParams) { } _ => { // This case should not happen, but we handle it defensively - log!(error, "Unexpected decision result: {:?}", (offender, reenable)); + log!(error, "Unexpected decision result: {:?}", decision); } } }); From 927d686a3ac623616213f30861a9a20d8d440527 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Sat, 31 Aug 2024 00:01:32 +0200 Subject: [PATCH 07/48] DisablingDecision struct refactor --- substrate/frame/staking/src/lib.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index b6cee2f06267..5920e423c99e 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1326,7 +1326,7 @@ impl DisablingStrategy { fn decision( offender_stash: &T::AccountId, - offender_slash_severity: Perbill, + _offender_slash_severity: Perbill, slash_era: EraIndex, currently_disabled: &Vec<(u32, Perbill)>, ) -> DisablingDecision { @@ -1350,19 +1350,19 @@ impl DisablingStrategy Pallet::::current_era().unwrap_or_default(), slash_era ); - return (None, None) + return DisablingDecision { disable: None, reenable: None } } let offender_idx = if let Some(idx) = active_set.iter().position(|i| i == offender_stash) { idx as u32 } else { log!(debug, "Won't disable: offender not in active set",); - return (None, None) + return DisablingDecision { disable: None, reenable: None } }; log!(debug, "Will disable {:?}", offender_idx); - (Some(offender_idx), None) + DisablingDecision { disable: Some(offender_idx), reenable: None } } } @@ -1406,16 +1406,18 @@ impl DisablingStrategy Pallet::::current_era().unwrap_or_default(), slash_era ); - return (None, None) + return DisablingDecision { disable: None, reenable: None } } + + // We don't disable validators that are not in the active set let offender_idx = if let Some(idx) = active_set.iter().position(|i| i == offender_stash) { idx as u32 } else { log!(debug, "Won't disable: offender not in active set",); - return (None, None) + return DisablingDecision { disable: None, reenable: None } }; - // We don't disable more than the limit (but we can re-enable a smaller offender) + // We don't disable more than the limit (but we can re-enable a smaller offender to make space) if currently_disabled.len() >= Self::disable_limit(active_set.len()) { log!( debug, @@ -1430,15 +1432,15 @@ impl DisablingStrategy .min_by_key(|(_, perbill)| *perbill) { log!(debug, "Will disable {:?} and re-enable {:?}", offender_idx, smallest_idx); - return (Some(offender_idx), Some(*smallest_idx)); + return DisablingDecision { disable: Some(offender_idx), reenable: Some(*smallest_idx) } } else { log!(debug, "No smaller offender found to re-enable"); - return (None, None); + return DisablingDecision { disable: None, reenable: None } } } // If we are not at the limit, just disable the new offender and dont re-enable anyone log!(debug, "Will disable {:?}", offender_idx); - (Some(offender_idx), None) + DisablingDecision { disable: Some(offender_idx), reenable: None } } } From 533025b6fd0ae43370180f83c796771d32dc71d5 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 17 Sep 2024 11:16:00 +0100 Subject: [PATCH 08/48] prdoc --- prdoc/pr_5724.prdoc | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 prdoc/pr_5724.prdoc diff --git a/prdoc/pr_5724.prdoc b/prdoc/pr_5724.prdoc new file mode 100644 index 000000000000..bc7d2dc6d2b9 --- /dev/null +++ b/prdoc/pr_5724.prdoc @@ -0,0 +1,28 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Validator Re-Enabling (master PR) + +doc: + - audience: Runtime Dev + description: | + Implementation of the Stage 3 for the New Disabling Strategy: https://github.com/paritytech/polkadot-sdk/issues/4359 + + This PR changes when an active validator node gets disabled for comitting offences. + When Byzantine Threshold Validators (1/3) are already disabled instead of no longer + disabling the highest offenders will be disabled potentially re-enabling low offenders. + + - audience: Node Operator + Implementation of the Stage 3 for the New Disabling Strategy: https://github.com/paritytech/polkadot-sdk/issues/4359 + + This PR changes when an active validator node gets disabled within parachain consensus (reduced responsibilities and + reduced rewards) for comitting offences. This should not affect active validators on a dato-day basis and will only + be relevant when the network is under attack or there is a wide spread malfunction causing slashes. In that case + lowest offenders might get eventually re-enabled (back to normal responsibilities and normal rewards). + +crates: + - name: pallet-staking + bump: minor + + - name: pallet-session + bump: minor From 29111f077b60b25352ee26754e32bd8d6cf8071e Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 17 Sep 2024 12:02:31 +0100 Subject: [PATCH 09/48] extra staking func tests --- substrate/frame/staking/src/tests.rs | 86 +++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 14 deletions(-) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 171c5b3a69ae..7ff7c573751d 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -8259,7 +8259,7 @@ mod ledger_recovery { mod byzantine_threshold_disabling_strategy { use crate::{ - tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitDisablingStrategy, UpToLimitWithReEnablingDisablingStrategy, + tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitDisablingStrategy, }; use sp_runtime::Perbill; use sp_staking::EraIndex; @@ -8287,7 +8287,7 @@ use sp_staking::EraIndex; &initially_disabled, ); - assert!(disabling_decision.0.is_none() && disabling_decision.1.is_none()); + assert!(disabling_decision.disable.is_none() && disabling_decision.reenable.is_none()); }); } @@ -8306,7 +8306,7 @@ use sp_staking::EraIndex; &initially_disabled, ); - assert!(disabling_decision.0.is_none() && disabling_decision.1.is_none()); + assert!(disabling_decision.disable.is_none() && disabling_decision.reenable.is_none()); }); } @@ -8325,7 +8325,7 @@ use sp_staking::EraIndex; &initially_disabled, ); - assert_eq!(disabling_decision.0, Some(OFFENDER_VALIDATOR_IDX)); + assert_eq!(disabling_decision.disable, Some(OFFENDER_VALIDATOR_IDX)); }); } @@ -8363,15 +8363,14 @@ use sp_staking::EraIndex; &initially_disabled, ); - assert!(disabling_decision.0.is_none() && disabling_decision.1.is_none()); + assert!(disabling_decision.disable.is_none() && disabling_decision.reenable.is_none()); }); } #[test] fn disable_when_below_byzantine_threshold() { sp_io::TestExternalities::default().execute_with(|| { - let max_slash = Perbill::from_percent(100); - let initially_disabled = vec![(1, max_slash)]; + let initially_disabled = vec![(0, OFFENDER_SLASH_MAJOR)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = @@ -8383,16 +8382,15 @@ use sp_staking::EraIndex; ); // Disable Offender and do not re-enable anyone - assert_eq!(disabling_decision.0, Some(OFFENDER_VALIDATOR_IDX)); - assert_eq!(disabling_decision.1, None); + assert_eq!(disabling_decision.disable, Some(OFFENDER_VALIDATOR_IDX)); + assert_eq!(disabling_decision.reenable, None); }); } #[test] fn reenable_arbitrary_on_equal_severity() { sp_io::TestExternalities::default().execute_with(|| { - let max_slash = Perbill::from_percent(100); - let initially_disabled = vec![(1, max_slash), (2, max_slash)]; + let initially_disabled = vec![(0, OFFENDER_SLASH_MAJOR), (1, OFFENDER_SLASH_MAJOR)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = @@ -8403,10 +8401,70 @@ use sp_staking::EraIndex; &initially_disabled, ); - assert!(disabling_decision.0.is_some() && disabling_decision.1.is_some()); + assert!(disabling_decision.disable.is_some() && disabling_decision.reenable.is_some()); // Disable 7 and enable 1 - assert_eq!(disabling_decision.0.unwrap(), OFFENDER_VALIDATOR_IDX); - assert_eq!(disabling_decision.1.unwrap(), 1); + assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); + assert_eq!(disabling_decision.reenable.unwrap(), 0); + }); + } + + #[test] + fn do_not_reenable_higher_offenders() { + sp_io::TestExternalities::default().execute_with(|| { + let initially_disabled = vec![(0, OFFENDER_SLASH_MAJOR), (1, OFFENDER_SLASH_MAJOR)]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MINOR, + SLASH_ERA, + &initially_disabled, + ); + + assert!(disabling_decision.disable.is_none() && disabling_decision.reenable.is_none()); + }); + } + + #[test] + fn reenable_lower_offenders() { + sp_io::TestExternalities::default().execute_with(|| { + let initially_disabled = vec![(0, OFFENDER_SLASH_MINOR), (1, OFFENDER_SLASH_MINOR)]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MAJOR, + SLASH_ERA, + &initially_disabled, + ); + + assert!(disabling_decision.disable.is_some() && disabling_decision.reenable.is_some()); + // Disable 7 and enable 1 + assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); + assert_eq!(disabling_decision.reenable.unwrap(), 0); + }); + } + + #[test] + fn reenable_lower_offenders_unordered() { + sp_io::TestExternalities::default().execute_with(|| { + let initially_disabled = vec![(0, OFFENDER_SLASH_MAJOR), (1, OFFENDER_SLASH_MINOR)]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MAJOR, + SLASH_ERA, + &initially_disabled, + ); + + assert!(disabling_decision.disable.is_some() && disabling_decision.reenable.is_some()); + // Disable 7 and enable 1 + assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); + assert_eq!(disabling_decision.reenable.unwrap(), 1); }); } } \ No newline at end of file From a9d24508c3f46160de88d52664d788f9fee64381 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 17 Sep 2024 12:02:55 +0100 Subject: [PATCH 10/48] decision docs --- substrate/frame/staking/src/lib.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 5920e423c99e..e34676cc555a 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1282,10 +1282,12 @@ impl BenchmarkingConfig for TestBenchmarkingConfig { pub trait DisablingStrategy { /// Make a disabling decision. Potentially returns 2 validators indices. /// First index is to be disabled, second is to be re-enabled. + /// /// Options: /// (None, None) no changes needed /// (Some(x), None) just disable x /// (Some(x), Some(y)) disable x instead of y + /// (None, Some(y)) unreachable fn decision( offender_stash: &T::AccountId, offender_slash_severity: Perbill, @@ -1294,6 +1296,9 @@ pub trait DisablingStrategy { ) -> DisablingDecision; } +/// Helper struct representing a decision coming from a given [`DisablingStrategy`] implementing `decision` +/// +/// Currently supports at most 1 disable + 1 re-enable per decision #[derive(Debug)] pub struct DisablingDecision { pub disable: Option, From dbaa85627b4e6a17712185bcbc17602be532508d Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 17 Sep 2024 12:27:18 +0100 Subject: [PATCH 11/48] migration desc in prdoc --- prdoc/pr_5724.prdoc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/prdoc/pr_5724.prdoc b/prdoc/pr_5724.prdoc index bc7d2dc6d2b9..e110901260ee 100644 --- a/prdoc/pr_5724.prdoc +++ b/prdoc/pr_5724.prdoc @@ -20,6 +20,14 @@ doc: be relevant when the network is under attack or there is a wide spread malfunction causing slashes. In that case lowest offenders might get eventually re-enabled (back to normal responsibilities and normal rewards). +migrations: + db: [] + runtime: + - reference: pallet-staking + description: | + Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>` where the PerBill represents the severity + of the offence in terms of the % slash. + crates: - name: pallet-staking bump: minor From 7bcfd3ef1b01483c77b6286d09b1701e31f560ad Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 17 Sep 2024 12:52:33 +0100 Subject: [PATCH 12/48] severity update todo --- substrate/frame/staking/src/slashing.rs | 5 ++-- substrate/frame/staking/src/tests.rs | 32 ++++++++++++++++++++----- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 79ce39fbf224..42952ddd9513 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -331,14 +331,15 @@ fn add_offending_validator(params: &SlashParams) { // Do nothing } (Some(offender_idx), None) => { - // Add the validator to `DisabledValidators` and disable it. Do nothing if it is - // already disabled. + // Add the validator to `DisabledValidators` and disable it. if let Err(index) = disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { let severity = params.slash; disabled.insert(index, (offender_idx, severity)); // Propagate disablement to session level T::SessionInterface::disable_validator(offender_idx); } + // If already disabled potentially update severity if it is higher + todo!("Add severity updating") } (Some(offender_idx), Some(reenable_idx)) => { // Remove the validator from `DisabledValidators` and re-enable it. diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 7ff7c573751d..9e2db7d6f5ba 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -8441,9 +8441,9 @@ use sp_staking::EraIndex; ); assert!(disabling_decision.disable.is_some() && disabling_decision.reenable.is_some()); - // Disable 7 and enable 1 - assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); - assert_eq!(disabling_decision.reenable.unwrap(), 0); + // Disable 7 and enable 1 + assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); + assert_eq!(disabling_decision.reenable.unwrap(), 0); }); } @@ -8462,9 +8462,29 @@ use sp_staking::EraIndex; ); assert!(disabling_decision.disable.is_some() && disabling_decision.reenable.is_some()); - // Disable 7 and enable 1 - assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); - assert_eq!(disabling_decision.reenable.unwrap(), 1); + // Disable 7 and enable 1 + assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); + assert_eq!(disabling_decision.reenable.unwrap(), 1); + }); + } + + #[test] + fn update_severity() { + sp_io::TestExternalities::default().execute_with(|| { + let initially_disabled = vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MINOR), (0, OFFENDER_SLASH_MAJOR)]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MAJOR, + SLASH_ERA, + &initially_disabled, + ); + + assert!(disabling_decision.disable.is_some() && disabling_decision.reenable.is_none()); + // Disable 7 "again" AKA update their severity + assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); }); } } \ No newline at end of file From 9fe1cacbd4b8b21975c9bef185f43a129ccc5937 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 17 Sep 2024 12:52:40 +0100 Subject: [PATCH 13/48] migration cleanup --- substrate/frame/staking/src/migrations.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index 6bc62009206b..2310a08f3996 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -64,8 +64,6 @@ type StorageVersion = StorageValue, ObsoleteReleases, Value pub mod v16 { use super::*; - // The disabling strategy used by staking pallet - type DefaultDisablingStrategy = UpToLimitWithReEnablingDisablingStrategy; pub struct VersionUncheckedMigrateV15ToV16(core::marker::PhantomData); impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV15ToV16 { fn on_runtime_upgrade() -> Weight { From 0d994addfb795115ecf81f37d5211aae3bec15e9 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 23 Sep 2024 18:13:46 +0100 Subject: [PATCH 14/48] repeated offences can update severity --- substrate/frame/staking/src/lib.rs | 19 +++++++++++--- substrate/frame/staking/src/slashing.rs | 33 +++++++++++++++---------- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index e34676cc555a..560a2c152c5b 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1422,6 +1422,17 @@ impl DisablingStrategy return DisablingDecision { disable: None, reenable: None } }; + // Check if offender is already disabled + if let Some((_, old_severity)) = currently_disabled.iter().find(|(idx, _)| *idx == offender_idx) { + if offender_slash_severity > *old_severity { + log!(debug, "Offender already disabled but with lower severity, will disable again to refresh severity of {:?}", offender_idx); + return DisablingDecision { disable: Some(offender_idx), reenable: None }; + } else { + log!(debug, "Offender already disabled with higher or equal severity"); + return DisablingDecision { disable: None, reenable: None }; + } + } + // We don't disable more than the limit (but we can re-enable a smaller offender to make space) if currently_disabled.len() >= Self::disable_limit(active_set.len()) { log!( @@ -1442,10 +1453,10 @@ impl DisablingStrategy log!(debug, "No smaller offender found to re-enable"); return DisablingDecision { disable: None, reenable: None } } + } else { + // If we are not at the limit, just disable the new offender and dont re-enable anyone + log!(debug, "Will disable {:?}", offender_idx); + return DisablingDecision { disable: Some(offender_idx), reenable: None } } - - // If we are not at the limit, just disable the new offender and dont re-enable anyone - log!(debug, "Will disable {:?}", offender_idx); - DisablingDecision { disable: Some(offender_idx), reenable: None } } } diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 42952ddd9513..310e445b1e39 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -330,17 +330,25 @@ fn add_offending_validator(params: &SlashParams) { (None, None) => { // Do nothing } - (Some(offender_idx), None) => { - // Add the validator to `DisabledValidators` and disable it. - if let Err(index) = disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { - let severity = params.slash; - disabled.insert(index, (offender_idx, severity)); - // Propagate disablement to session level - T::SessionInterface::disable_validator(offender_idx); - } - // If already disabled potentially update severity if it is higher - todo!("Add severity updating") - } + (Some(offender_idx), None) => { + // Check if the offender is already disabled + match disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { + // 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 { + *old_severity = params.slash; + } + } + Err(index) => { + // Offender is not disabled, add to `DisabledValidators` and disable it + let severity = params.slash; + disabled.insert(index, (offender_idx, severity)); + // Propagate disablement to session level + T::SessionInterface::disable_validator(offender_idx); + } + } + } (Some(offender_idx), Some(reenable_idx)) => { // Remove the validator from `DisabledValidators` and re-enable it. if let Ok(index) = disabled.binary_search_by_key(&reenable_idx, |(index, _)| *index) { @@ -349,8 +357,7 @@ fn add_offending_validator(params: &SlashParams) { T::SessionInterface::enable_validator(reenable_idx); } - // Add the validator to `DisabledValidators` and disable it. Do nothing if it is - // already disabled. + // Add the validator to `DisabledValidators` and disable it. if let Err(index) = disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { let severity = params.slash; disabled.insert(index, (offender_idx, severity)); From 50dbecf354a864b81e4475746e98f3e4e6b48a6c Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 23 Sep 2024 18:13:59 +0100 Subject: [PATCH 15/48] extra severity update tests --- substrate/frame/staking/src/tests.rs | 36 ++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 9e2db7d6f5ba..d1b0ee792dee 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -8487,4 +8487,40 @@ use sp_staking::EraIndex; assert_eq!(disabling_decision.disable.unwrap(), OFFENDER_VALIDATOR_IDX); }); } + + #[test] + fn update_cannot_lower_severity() { + sp_io::TestExternalities::default().execute_with(|| { + let initially_disabled = vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MAJOR), (0, OFFENDER_SLASH_MAJOR)]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MINOR, + SLASH_ERA, + &initially_disabled, + ); + + assert!(disabling_decision.disable.is_none() && disabling_decision.reenable.is_none()); + }); + } + + #[test] + fn no_accidental_reenablement_on_repeated_offence() { + sp_io::TestExternalities::default().execute_with(|| { + let initially_disabled = vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MAJOR), (0, OFFENDER_SLASH_MINOR)]; + pallet_session::Validators::::put(ACTIVE_SET.to_vec()); + + let disabling_decision = + >::decision( + &OFFENDER_ID, + OFFENDER_SLASH_MAJOR, + SLASH_ERA, + &initially_disabled, + ); + + assert!(disabling_decision.disable.is_none() && disabling_decision.reenable.is_none()); + }); + } } \ No newline at end of file From 1db6a26aed2170f413dde178218b76a7b303a468 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 23 Sep 2024 18:23:45 +0100 Subject: [PATCH 16/48] refactor match in add_offending_validator --- substrate/frame/staking/src/slashing.rs | 66 ++++++++++--------------- 1 file changed, 25 insertions(+), 41 deletions(-) diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 310e445b1e39..4c568d48cb7f 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -326,50 +326,34 @@ fn add_offending_validator(params: &SlashParams) { DisabledValidators::::mutate(|disabled| { let decision = T::DisablingStrategy::decision(params.stash, params.slash, params.slash_era, &disabled); - match (decision.disable, decision.reenable) { - (None, None) => { - // Do nothing - } - (Some(offender_idx), None) => { - // Check if the offender is already disabled - match disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { - // 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 { - *old_severity = params.slash; - } - } - Err(index) => { - // Offender is not disabled, add to `DisabledValidators` and disable it - let severity = params.slash; - disabled.insert(index, (offender_idx, severity)); - // Propagate disablement to session level - T::SessionInterface::disable_validator(offender_idx); + if let Some(offender_idx) = decision.disable { + // Check if the offender is already disabled + match disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { + // 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 { + *old_severity = params.slash; } } - } - (Some(offender_idx), Some(reenable_idx)) => { - // 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 - T::SessionInterface::enable_validator(reenable_idx); - } - - // Add the validator to `DisabledValidators` and disable it. - if let Err(index) = disabled.binary_search_by_key(&offender_idx, |(index, _)| *index) { + Err(index) => { + // Offender is not disabled, add to `DisabledValidators` and disable it let severity = params.slash; - disabled.insert(index, (offender_idx, severity)); - // Propagate disablement to session level - T::SessionInterface::disable_validator(offender_idx); - } - } - _ => { - // This case should not happen, but we handle it defensively - log!(error, "Unexpected decision result: {:?}", decision); - } - } + disabled.insert(index, (offender_idx, severity)); + // Propagate disablement to session level + T::SessionInterface::disable_validator(offender_idx); + } + } + } + + if let Some(reenable_idx) = decision.reenable { + // 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 + T::SessionInterface::enable_validator(reenable_idx); + } + } }); // `DisabledValidators` should be kept sorted From 29c59a623a28b1f1ddc543025522f5561b4c1531 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 23 Sep 2024 20:10:57 +0100 Subject: [PATCH 17/48] reenabling test update --- substrate/frame/staking/src/tests.rs | 36 +++++++++++++++++++++------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index d1b0ee792dee..2b8d72956865 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -3495,6 +3495,7 @@ fn slashing_independent_of_disabling_validator() { let now = Staking::active_era().unwrap().index; + // --- Disable without a slash --- // offence with no slash associated on_offence_in_era( &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], @@ -3505,7 +3506,18 @@ fn slashing_independent_of_disabling_validator() { // nomination remains untouched. assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21]); - // offence that slashes 25% of the bond + // first validator is disabled but not slashed + assert!(is_disabled(11)); + + // --- Slash without disabling --- + // offence that slashes 50% of the bond (setup for next slash) + on_offence_in_era( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::from_percent(50)], + now, + ); + + // offence that slashes 25% of the bond but does not disable on_offence_in_era( &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], &[Perbill::from_percent(25)], @@ -3515,6 +3527,10 @@ fn slashing_independent_of_disabling_validator() { // nomination remains untouched. assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21]); + // second validator is slashed but not disabled + assert!(!is_disabled(21)); + assert!(is_disabled(11)); + assert_eq!( staking_events_since_last_call(), vec![ @@ -3525,6 +3541,13 @@ fn slashing_independent_of_disabling_validator() { fraction: Perbill::from_percent(0), slash_era: 1 }, + Event::SlashReported { + validator: 11, + fraction: Perbill::from_percent(50), + slash_era: 1 + }, + Event::Slashed { staker: 11, amount: 500 }, + Event::Slashed { staker: 101, amount: 62 }, Event::SlashReported { validator: 21, fraction: Perbill::from_percent(25), @@ -3534,11 +3557,6 @@ fn slashing_independent_of_disabling_validator() { Event::Slashed { staker: 101, amount: 94 } ] ); - - // first validator is disabled but not slashed - assert!(is_disabled(11)); - // second validator is slashed but not disabled - assert!(!is_disabled(21)); }); } @@ -3552,7 +3570,7 @@ fn offence_threshold_doesnt_trigger_new_era() { assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41]); assert_eq!( - UpToLimitDisablingStrategy::::disable_limit( + UpToLimitWithReEnablingDisablingStrategy::::disable_limit( Session::validators().len() ), 1 @@ -3567,7 +3585,7 @@ fn offence_threshold_doesnt_trigger_new_era() { on_offence_now( &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], - &[Perbill::zero()], + &[Perbill::from_percent(50)], ); // 11 should be disabled because the byzantine threshold is 1 @@ -8337,7 +8355,7 @@ mod disabling_strategy_with_reenabling { tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitWithReEnablingDisablingStrategy, }; use sp_runtime::Perbill; -use sp_staking::EraIndex; + use sp_staking::EraIndex; // Common test data - the stash of the offending validator, the era of the offence and the // active set From 2b78972f0ee01e8cd2ad466591a1ae725dac18d4 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 23 Sep 2024 20:11:29 +0100 Subject: [PATCH 18/48] reenabling test updates p2 --- .../election-provider-multi-phase/test-staking-e2e/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs index 0dc202ff2115..30cd153efcf6 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs @@ -157,7 +157,7 @@ fn mass_slash_doesnt_enter_emergency_phase() { // Slashed validators are disabled up to a limit slashed.truncate( - pallet_staking::UpToLimitDisablingStrategy::::disable_limit( + pallet_staking::UpToLimitWithReEnablingDisablingStrategy::::disable_limit( active_set_size_after_slash, ), ); From 32941ab88de510e803f3ba206ce3e6558936a204 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 23 Sep 2024 20:12:15 +0100 Subject: [PATCH 19/48] changing config and default to reenabling strategy --- polkadot/runtime/test-runtime/src/lib.rs | 2 +- polkadot/runtime/westend/src/lib.rs | 2 +- substrate/bin/node/runtime/src/lib.rs | 2 +- .../test-staking-e2e/src/mock.rs | 2 +- substrate/frame/staking/src/mock.rs | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/polkadot/runtime/test-runtime/src/lib.rs b/polkadot/runtime/test-runtime/src/lib.rs index 8e34320d38f2..ba765c1a3841 100644 --- a/polkadot/runtime/test-runtime/src/lib.rs +++ b/polkadot/runtime/test-runtime/src/lib.rs @@ -369,7 +369,7 @@ impl pallet_staking::Config for Runtime { type BenchmarkingConfig = polkadot_runtime_common::StakingBenchmarkingConfig; type EventListeners = (); type WeightInfo = (); - type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy; + type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy; } parameter_types! { diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index eddb7cbd21ea..e44ce5c95d3f 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -747,7 +747,7 @@ impl pallet_staking::Config for Runtime { type BenchmarkingConfig = polkadot_runtime_common::StakingBenchmarkingConfig; type EventListeners = (NominationPools, DelegatedStaking); type WeightInfo = weights::pallet_staking::WeightInfo; - type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy; + type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy; } impl pallet_fast_unstake::Config for Runtime { diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index fd8597563a02..0a8b9f4ef48b 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -698,7 +698,7 @@ impl pallet_staking::Config for Runtime { type EventListeners = NominationPools; type WeightInfo = pallet_staking::weights::SubstrateWeight; type BenchmarkingConfig = StakingBenchmarkingConfig; - type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy; + type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy; } impl pallet_fast_unstake::Config for Runtime { diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs index e45452c1ddf9..abad2e579ac0 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs @@ -304,7 +304,7 @@ impl pallet_staking::Config for Runtime { type MaxUnlockingChunks = MaxUnlockingChunks; type EventListeners = Pools; type WeightInfo = pallet_staking::weights::SubstrateWeight; - type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy; + type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy; type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; } diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index 4a0209fc5b08..f0ee900ccfe2 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -258,7 +258,7 @@ impl OnStakingUpdate for EventListenerMock { } } -// Disabling threshold for `UpToLimitDisablingStrategy` +// Disabling threshold for `UpToLimitDisablingStrategy` and `UpToLimitWithReEnablingDisablingStrategy`` pub(crate) const DISABLING_LIMIT_FACTOR: usize = 3; #[derive_impl(crate::config_preludes::TestDefaultConfig)] @@ -284,7 +284,7 @@ impl crate::pallet::pallet::Config for Test { type HistoryDepth = HistoryDepth; type MaxControllersInDeprecationBatch = MaxControllersInDeprecationBatch; type EventListeners = EventListenerMock; - type DisablingStrategy = pallet_staking::UpToLimitDisablingStrategy; + type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy; } pub struct WeightedNominationsQuota; From c153fc08664b04e8854781e664c6ef9584cbad5b Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 24 Sep 2024 18:32:52 +0100 Subject: [PATCH 20/48] Mock tests and disabling events --- substrate/frame/staking/src/pallet/impls.rs | 12 +- substrate/frame/staking/src/pallet/mod.rs | 4 + substrate/frame/staking/src/slashing.rs | 5 + substrate/frame/staking/src/tests.rs | 158 ++++++++++++++++++++ 4 files changed, 173 insertions(+), 6 deletions(-) diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 15ac3fd2c9d9..015575235d51 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -1495,6 +1495,12 @@ where continue } + Self::deposit_event(Event::::SlashReported { + validator: stash.clone(), + fraction: *slash_fraction, + slash_era, + }); + let unapplied = slashing::compute_slash::(slashing::SlashParams { stash, slash: *slash_fraction, @@ -1505,12 +1511,6 @@ where reward_proportion, }); - Self::deposit_event(Event::::SlashReported { - validator: stash.clone(), - fraction: *slash_fraction, - slash_era, - }); - if let Some(mut unapplied) = unapplied { let nominators_len = unapplied.others.len() as u64; let reporters_len = details.reporters.len() as u64; diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 9e39b72f61db..7a68f7ca5edb 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -863,6 +863,10 @@ pub mod pallet { ForceEra { mode: Forcing }, /// Report of a controller batch deprecation. ControllerBatchDeprecated { failures: u32 }, + /// Validator has been disabled. + ValidatorDisabled { stash: T::AccountId }, + /// Validator has been re-enabled. + ValidatorReenabled { stash: T::AccountId }, } #[pallet::error] diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 4c568d48cb7f..147e6eb9e6e0 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -342,6 +342,8 @@ fn add_offending_validator(params: &SlashParams) { disabled.insert(index, (offender_idx, severity)); // Propagate disablement to session level T::SessionInterface::disable_validator(offender_idx); + // Emit event that a validator got disabled + >::deposit_event(super::Event::::ValidatorDisabled { stash: params.stash.clone() }); } } } @@ -352,6 +354,9 @@ fn add_offending_validator(params: &SlashParams) { disabled.remove(index); // Propagate re-enablement to session level T::SessionInterface::enable_validator(reenable_idx); + // Emit event that a validator got re-enabled + let reenabled_stash = T::SessionInterface::validators()[reenable_idx as usize].clone(); + >::deposit_event(super::Event::::ValidatorReenabled { stash: reenabled_stash }); } } }); diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 2b8d72956865..cf785dc5285c 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -3391,6 +3391,7 @@ fn slash_kicks_validators_not_nominators_and_disables_nominator_for_kicked_valid fraction: Perbill::from_percent(10), slash_era: 1 }, + Event::ValidatorDisabled{ stash: 11 }, Event::Slashed { staker: 11, amount: 100 }, Event::Slashed { staker: 101, amount: 12 }, ] @@ -3463,11 +3464,13 @@ fn non_slashable_offence_disables_validator() { fraction: Perbill::from_percent(0), slash_era: 1 }, + Event::ValidatorDisabled { stash: 11 }, Event::SlashReported { validator: 21, fraction: Perbill::from_percent(25), slash_era: 1 }, + Event::ValidatorDisabled { stash: 21 }, Event::Slashed { staker: 21, amount: 250 }, Event::Slashed { staker: 101, amount: 94 } ] @@ -3541,6 +3544,7 @@ fn slashing_independent_of_disabling_validator() { fraction: Perbill::from_percent(0), slash_era: 1 }, + Event::ValidatorDisabled { stash: 11 }, Event::SlashReported { validator: 11, fraction: Perbill::from_percent(50), @@ -8541,4 +8545,158 @@ mod disabling_strategy_with_reenabling { assert!(disabling_decision.disable.is_none() && disabling_decision.reenable.is_none()); }); } +} + +#[test] +fn reenable_lower_offenders_mock() { + ExtBuilder::default() + .validator_count(7) + .set_status(41, StakerStatus::Validator) + .set_status(51, StakerStatus::Validator) + .set_status(201, StakerStatus::Validator) + .set_status(202, StakerStatus::Validator) + .build_and_execute(|| { + mock::start_active_era(1); + assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41, 51, 201, 202]); + + let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11); + let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21); + let exposure_31 = Staking::eras_stakers(Staking::active_era().unwrap().index, &31); + + // offence with a low slash + on_offence_now( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::from_percent(10)], + ); + on_offence_now( + &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], + &[Perbill::from_percent(20)], + ); + + // it does NOT affect the nominator. + assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21]); + + // both validators should be disabled + assert!(is_disabled(11)); + assert!(is_disabled(21)); + + // offence with a higher slash + on_offence_now( + &[OffenceDetails { offender: (31, exposure_31.clone()), reporters: vec![] }], + &[Perbill::from_percent(50)], + ); + + // First offender is no longer disabled + assert!(!is_disabled(11)); + // Mid offender is still disabled + assert!(is_disabled(21)); + // New offender is disabled + assert!(is_disabled(31)); + + assert_eq!( + staking_events_since_last_call(), + vec![ + Event::StakersElected, + Event::EraPaid { era_index: 0, validator_payout: 11075, remainder: 33225 }, + Event::SlashReported { + validator: 11, + fraction: Perbill::from_percent(10), + slash_era: 1 + }, + Event::ValidatorDisabled{ stash: 11 }, + Event::Slashed { staker: 11, amount: 100 }, + Event::Slashed { staker: 101, amount: 12 }, + Event::SlashReported { + validator: 21, + fraction: Perbill::from_percent(20), + slash_era: 1 + }, + Event::ValidatorDisabled{ stash: 21 }, + Event::Slashed { staker: 21, amount: 200 }, + Event::Slashed { staker: 101, amount: 75 }, + Event::SlashReported { + validator: 31, + fraction: Perbill::from_percent(50), + slash_era: 1 + }, + Event::ValidatorDisabled{ stash: 31 }, + Event::ValidatorReenabled{ stash: 11 }, + Event::Slashed { staker: 31, amount: 250 }, + ] + ); + }); +} + +#[test] +fn do_not_reenable_higher_offenders_mock() { + ExtBuilder::default() + .validator_count(7) + .set_status(41, StakerStatus::Validator) + .set_status(51, StakerStatus::Validator) + .set_status(201, StakerStatus::Validator) + .set_status(202, StakerStatus::Validator) + .build_and_execute(|| { + mock::start_active_era(1); + assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41, 51, 201, 202]); + + let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11); + let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21); + let exposure_31 = Staking::eras_stakers(Staking::active_era().unwrap().index, &31); + + // offence with a major slash + on_offence_now( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::from_percent(50)], + ); + on_offence_now( + &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], + &[Perbill::from_percent(50)], + ); + + // both validators should be disabled + assert!(is_disabled(11)); + assert!(is_disabled(21)); + + // offence with a minor slash + on_offence_now( + &[OffenceDetails { offender: (31, exposure_31.clone()), reporters: vec![] }], + &[Perbill::from_percent(10)], + ); + + // First and second offenders are still disabled + assert!(is_disabled(11)); + assert!(is_disabled(21)); + // New offender is not disabled as limit is reached and his prio is lower + assert!(!is_disabled(31)); + + assert_eq!( + staking_events_since_last_call(), + vec![ + Event::StakersElected, + Event::EraPaid { era_index: 0, validator_payout: 11075, remainder: 33225 }, + Event::SlashReported { + validator: 11, + fraction: Perbill::from_percent(50), + slash_era: 1 + }, + Event::ValidatorDisabled{ stash: 11 }, + Event::Slashed { staker: 11, amount: 500 }, + Event::Slashed { staker: 101, amount: 62 }, + Event::SlashReported { + validator: 21, + fraction: Perbill::from_percent(50), + slash_era: 1 + }, + Event::ValidatorDisabled{ stash: 21 }, + Event::Slashed { staker: 21, amount: 500 }, + Event::Slashed { staker: 101, amount: 187 }, + Event::SlashReported { + validator: 31, + fraction: Perbill::from_percent(10), + slash_era: 1 + }, + Event::Slashed { staker: 31, amount: 50 }, + ] + ); + }); } \ No newline at end of file From 63cca972ea30d43526c2ba14a7946e9803ebb7d2 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 24 Sep 2024 19:07:53 +0100 Subject: [PATCH 21/48] fmt --- .../test-staking-e2e/src/lib.rs | 8 +- .../test-staking-e2e/src/mock.rs | 3 +- substrate/frame/staking/src/lib.rs | 30 ++-- substrate/frame/staking/src/migrations.rs | 11 +- substrate/frame/staking/src/mock.rs | 6 +- substrate/frame/staking/src/pallet/impls.rs | 2 +- substrate/frame/staking/src/slashing.rs | 34 ++-- substrate/frame/staking/src/tests.rs | 170 +++++++++--------- 8 files changed, 144 insertions(+), 120 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs index 30cd153efcf6..f1756c56ab13 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs @@ -156,11 +156,9 @@ fn mass_slash_doesnt_enter_emergency_phase() { assert_eq!(active_set_size_before_slash, active_set_size_after_slash); // Slashed validators are disabled up to a limit - slashed.truncate( - pallet_staking::UpToLimitWithReEnablingDisablingStrategy::::disable_limit( - active_set_size_after_slash, - ), - ); + slashed.truncate(pallet_staking::UpToLimitWithReEnablingDisablingStrategy::< + SLASHING_DISABLING_FACTOR, + >::disable_limit(active_set_size_after_slash)); // Find the indices of the disabled validators let active_set = Session::validators(); diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs index abad2e579ac0..5477ee77ee64 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/mock.rs @@ -304,7 +304,8 @@ impl pallet_staking::Config for Runtime { type MaxUnlockingChunks = MaxUnlockingChunks; type EventListeners = Pools; type WeightInfo = pallet_staking::weights::SubstrateWeight; - type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy; + type DisablingStrategy = + pallet_staking::UpToLimitWithReEnablingDisablingStrategy; type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; } diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 560a2c152c5b..1e6d531773da 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1282,7 +1282,7 @@ impl BenchmarkingConfig for TestBenchmarkingConfig { pub trait DisablingStrategy { /// Make a disabling decision. Potentially returns 2 validators indices. /// First index is to be disabled, second is to be re-enabled. - /// + /// /// Options: /// (None, None) no changes needed /// (Some(x), None) just disable x @@ -1296,8 +1296,9 @@ pub trait DisablingStrategy { ) -> DisablingDecision; } -/// Helper struct representing a decision coming from a given [`DisablingStrategy`] implementing `decision` -/// +/// Helper struct representing a decision coming from a given [`DisablingStrategy`] implementing +/// `decision` +/// /// Currently supports at most 1 disable + 1 re-enable per decision #[derive(Debug)] pub struct DisablingDecision { @@ -1374,11 +1375,13 @@ impl DisablingStrategy /// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a /// limit and if the limit is reached and the new offender is higher (bigger punishment/severity) /// then it re-enables to lowest offender to free up space for the new offender. -/// +/// /// An extension of [`UpToLimitDisablingStrategy`]. pub struct UpToLimitWithReEnablingDisablingStrategy; -impl UpToLimitWithReEnablingDisablingStrategy { +impl + UpToLimitWithReEnablingDisablingStrategy +{ /// Disabling limit calculated from the total number of validators in the active set. When /// reached re-enabling logic might kick in. pub fn disable_limit(validators_len: usize) -> usize { @@ -1423,7 +1426,9 @@ impl DisablingStrategy }; // Check if offender is already disabled - if let Some((_, old_severity)) = currently_disabled.iter().find(|(idx, _)| *idx == offender_idx) { + if let Some((_, old_severity)) = + currently_disabled.iter().find(|(idx, _)| *idx == offender_idx) + { if offender_slash_severity > *old_severity { log!(debug, "Offender already disabled but with lower severity, will disable again to refresh severity of {:?}", offender_idx); return DisablingDecision { disable: Some(offender_idx), reenable: None }; @@ -1433,7 +1438,8 @@ impl DisablingStrategy } } - // We don't disable more than the limit (but we can re-enable a smaller offender to make space) + // We don't disable more than the limit (but we can re-enable a smaller offender to make + // space) if currently_disabled.len() >= Self::disable_limit(active_set.len()) { log!( debug, @@ -1441,14 +1447,18 @@ impl DisablingStrategy Self::disable_limit(active_set.len()) ); - // Find the smallest offender to re-enable that is not higher than offender_slash_severity + // 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) - .min_by_key(|(_, perbill)| *perbill) + .min_by_key(|(_, perbill)| *perbill) { log!(debug, "Will disable {:?} and re-enable {:?}", offender_idx, smallest_idx); - return DisablingDecision { disable: Some(offender_idx), reenable: Some(*smallest_idx) } + return DisablingDecision { + disable: Some(offender_idx), + reenable: Some(*smallest_idx), + } } else { log!(debug, "No smaller offender found to re-enable"); return DisablingDecision { disable: None, reenable: None } diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index 2310a08f3996..c559f828a286 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -60,7 +60,8 @@ impl Default for ObsoleteReleases { #[storage_alias] type StorageVersion = StorageValue, ObsoleteReleases, ValueQuery>; -/// Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>` to track offense severity for re-enabling purposes. +/// Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>` to track offense +/// severity for re-enabling purposes. pub mod v16 { use super::*; @@ -68,10 +69,14 @@ pub mod v16 { impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV15ToV16 { fn on_runtime_upgrade() -> Weight { // Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>`. - // Using max severity (PerBill) for the migration which effectively makes it so offenders before the migration will never be re-enabled. + // Using max severity (PerBill) for the migration which effectively makes it so + // offenders before the migration will never be re-enabled. let max_perbill = Perbill::from_percent(100); // Inject severity - let migrated = v15::DisabledValidators::::take().into_iter().map(|v| (v, max_perbill)).collect::>(); + let migrated = v15::DisabledValidators::::take() + .into_iter() + .map(|v| (v, max_perbill)) + .collect::>(); DisabledValidators::::set(migrated); diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index f0ee900ccfe2..4f305d10c824 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -258,7 +258,8 @@ impl OnStakingUpdate for EventListenerMock { } } -// Disabling threshold for `UpToLimitDisablingStrategy` and `UpToLimitWithReEnablingDisablingStrategy`` +// Disabling threshold for `UpToLimitDisablingStrategy` and +// `UpToLimitWithReEnablingDisablingStrategy`` pub(crate) const DISABLING_LIMIT_FACTOR: usize = 3; #[derive_impl(crate::config_preludes::TestDefaultConfig)] @@ -284,7 +285,8 @@ impl crate::pallet::pallet::Config for Test { type HistoryDepth = HistoryDepth; type MaxControllersInDeprecationBatch = MaxControllersInDeprecationBatch; type EventListeners = EventListenerMock; - type DisablingStrategy = pallet_staking::UpToLimitWithReEnablingDisablingStrategy; + type DisablingStrategy = + pallet_staking::UpToLimitWithReEnablingDisablingStrategy; } pub struct WeightedNominationsQuota; diff --git a/substrate/frame/staking/src/pallet/impls.rs b/substrate/frame/staking/src/pallet/impls.rs index 015575235d51..a460c0e627ec 100644 --- a/substrate/frame/staking/src/pallet/impls.rs +++ b/substrate/frame/staking/src/pallet/impls.rs @@ -2297,7 +2297,7 @@ impl Pallet { Ok(()) } - // Sorted by index + // Sorted by index fn ensure_disabled_validators_sorted() -> Result<(), TryRuntimeError> { ensure!( DisabledValidators::::get().windows(2).all(|pair| pair[0].0 <= pair[1].0), diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 147e6eb9e6e0..4b5c04c8e7a5 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -50,9 +50,9 @@ //! Based on research at use crate::{ - BalanceOf, Config, DisabledValidators, DisablingStrategy, Error, Exposure, NegativeImbalanceOf, - NominatorSlashInEra, Pallet, Perbill, SessionInterface, SpanSlash, UnappliedSlash, - ValidatorSlashInEra, log, + log, BalanceOf, Config, DisabledValidators, DisablingStrategy, Error, Exposure, + NegativeImbalanceOf, NominatorSlashInEra, Pallet, Perbill, SessionInterface, SpanSlash, + UnappliedSlash, ValidatorSlashInEra, }; use alloc::vec::Vec; use codec::{Decode, Encode, MaxEncodedLen}; @@ -323,8 +323,9 @@ fn kick_out_if_recent(params: SlashParams) { /// Inform the [`DisablingStrategy`] implementation about the new offender and disable the list of /// validators provided by [`decision`]. fn add_offending_validator(params: &SlashParams) { - DisabledValidators::::mutate(|disabled| { - let decision = T::DisablingStrategy::decision(params.stash, params.slash, params.slash_era, &disabled); + DisabledValidators::::mutate(|disabled| { + let decision = + T::DisablingStrategy::decision(params.stash, params.slash, params.slash_era, &disabled); if let Some(offender_idx) = decision.disable { // Check if the offender is already disabled @@ -335,7 +336,7 @@ fn add_offending_validator(params: &SlashParams) { if params.slash > *old_severity { *old_severity = params.slash; } - } + }, Err(index) => { // Offender is not disabled, add to `DisabledValidators` and disable it let severity = params.slash; @@ -343,11 +344,13 @@ fn add_offending_validator(params: &SlashParams) { // Propagate disablement to session level T::SessionInterface::disable_validator(offender_idx); // Emit event that a validator got disabled - >::deposit_event(super::Event::::ValidatorDisabled { stash: params.stash.clone() }); - } + >::deposit_event(super::Event::::ValidatorDisabled { + stash: params.stash.clone(), + }); + }, } } - + if let Some(reenable_idx) = decision.reenable { // Remove the validator from `DisabledValidators` and re-enable it. if let Ok(index) = disabled.binary_search_by_key(&reenable_idx, |(index, _)| *index) { @@ -355,14 +358,17 @@ fn add_offending_validator(params: &SlashParams) { // Propagate re-enablement to session level T::SessionInterface::enable_validator(reenable_idx); // Emit event that a validator got re-enabled - let reenabled_stash = T::SessionInterface::validators()[reenable_idx as usize].clone(); - >::deposit_event(super::Event::::ValidatorReenabled { stash: reenabled_stash }); + let reenabled_stash = + T::SessionInterface::validators()[reenable_idx as usize].clone(); + >::deposit_event(super::Event::::ValidatorReenabled { + stash: reenabled_stash, + }); } } - }); + }); - // `DisabledValidators` should be kept sorted - debug_assert!(DisabledValidators::::get().windows(2).all(|pair| pair[0] < pair[1])); + // `DisabledValidators` should be kept sorted + debug_assert!(DisabledValidators::::get().windows(2).all(|pair| pair[0] < pair[1])); } /// Slash nominators. Accepts general parameters and the prior slash percentage of the validator. diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index cf785dc5285c..c455be917b37 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -3391,7 +3391,7 @@ fn slash_kicks_validators_not_nominators_and_disables_nominator_for_kicked_valid fraction: Perbill::from_percent(10), slash_era: 1 }, - Event::ValidatorDisabled{ stash: 11 }, + Event::ValidatorDisabled { stash: 11 }, Event::Slashed { staker: 11, amount: 100 }, Event::Slashed { staker: 101, amount: 12 }, ] @@ -8284,7 +8284,7 @@ mod byzantine_threshold_disabling_strategy { tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitDisablingStrategy, }; use sp_runtime::Perbill; -use sp_staking::EraIndex; + use sp_staking::EraIndex; // Common test data - the stash of the offending validator, the era of the offence and the // active set @@ -8350,13 +8350,12 @@ use sp_staking::EraIndex; assert_eq!(disabling_decision.disable, Some(OFFENDER_VALIDATOR_IDX)); }); } - - } mod disabling_strategy_with_reenabling { use crate::{ - tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitWithReEnablingDisablingStrategy, + tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, + UpToLimitWithReEnablingDisablingStrategy, }; use sp_runtime::Perbill; use sp_staking::EraIndex; @@ -8493,7 +8492,8 @@ mod disabling_strategy_with_reenabling { #[test] fn update_severity() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MINOR), (0, OFFENDER_SLASH_MAJOR)]; + let initially_disabled = + vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MINOR), (0, OFFENDER_SLASH_MAJOR)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = @@ -8513,7 +8513,8 @@ mod disabling_strategy_with_reenabling { #[test] fn update_cannot_lower_severity() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MAJOR), (0, OFFENDER_SLASH_MAJOR)]; + let initially_disabled = + vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MAJOR), (0, OFFENDER_SLASH_MAJOR)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = @@ -8531,7 +8532,8 @@ mod disabling_strategy_with_reenabling { #[test] fn no_accidental_reenablement_on_repeated_offence() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MAJOR), (0, OFFENDER_SLASH_MINOR)]; + let initially_disabled = + vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MAJOR), (0, OFFENDER_SLASH_MINOR)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = @@ -8586,7 +8588,7 @@ fn reenable_lower_offenders_mock() { &[Perbill::from_percent(50)], ); - // First offender is no longer disabled + // First offender is no longer disabled assert!(!is_disabled(11)); // Mid offender is still disabled assert!(is_disabled(21)); @@ -8603,7 +8605,7 @@ fn reenable_lower_offenders_mock() { fraction: Perbill::from_percent(10), slash_era: 1 }, - Event::ValidatorDisabled{ stash: 11 }, + Event::ValidatorDisabled { stash: 11 }, Event::Slashed { staker: 11, amount: 100 }, Event::Slashed { staker: 101, amount: 12 }, Event::SlashReported { @@ -8611,7 +8613,7 @@ fn reenable_lower_offenders_mock() { fraction: Perbill::from_percent(20), slash_era: 1 }, - Event::ValidatorDisabled{ stash: 21 }, + Event::ValidatorDisabled { stash: 21 }, Event::Slashed { staker: 21, amount: 200 }, Event::Slashed { staker: 101, amount: 75 }, Event::SlashReported { @@ -8619,8 +8621,8 @@ fn reenable_lower_offenders_mock() { fraction: Perbill::from_percent(50), slash_era: 1 }, - Event::ValidatorDisabled{ stash: 31 }, - Event::ValidatorReenabled{ stash: 11 }, + Event::ValidatorDisabled { stash: 31 }, + Event::ValidatorReenabled { stash: 11 }, Event::Slashed { staker: 31, amount: 250 }, ] ); @@ -8629,74 +8631,74 @@ fn reenable_lower_offenders_mock() { #[test] fn do_not_reenable_higher_offenders_mock() { - ExtBuilder::default() - .validator_count(7) - .set_status(41, StakerStatus::Validator) - .set_status(51, StakerStatus::Validator) - .set_status(201, StakerStatus::Validator) - .set_status(202, StakerStatus::Validator) - .build_and_execute(|| { - mock::start_active_era(1); - assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41, 51, 201, 202]); - - let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11); - let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21); - let exposure_31 = Staking::eras_stakers(Staking::active_era().unwrap().index, &31); - - // offence with a major slash - on_offence_now( - &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], - &[Perbill::from_percent(50)], - ); - on_offence_now( - &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], - &[Perbill::from_percent(50)], - ); - - // both validators should be disabled - assert!(is_disabled(11)); - assert!(is_disabled(21)); - - // offence with a minor slash - on_offence_now( - &[OffenceDetails { offender: (31, exposure_31.clone()), reporters: vec![] }], - &[Perbill::from_percent(10)], - ); - - // First and second offenders are still disabled - assert!(is_disabled(11)); - assert!(is_disabled(21)); - // New offender is not disabled as limit is reached and his prio is lower - assert!(!is_disabled(31)); - - assert_eq!( - staking_events_since_last_call(), - vec![ - Event::StakersElected, - Event::EraPaid { era_index: 0, validator_payout: 11075, remainder: 33225 }, - Event::SlashReported { - validator: 11, - fraction: Perbill::from_percent(50), - slash_era: 1 - }, - Event::ValidatorDisabled{ stash: 11 }, - Event::Slashed { staker: 11, amount: 500 }, - Event::Slashed { staker: 101, amount: 62 }, - Event::SlashReported { - validator: 21, - fraction: Perbill::from_percent(50), - slash_era: 1 - }, - Event::ValidatorDisabled{ stash: 21 }, - Event::Slashed { staker: 21, amount: 500 }, - Event::Slashed { staker: 101, amount: 187 }, - Event::SlashReported { - validator: 31, - fraction: Perbill::from_percent(10), - slash_era: 1 - }, - Event::Slashed { staker: 31, amount: 50 }, - ] - ); - }); -} \ No newline at end of file + ExtBuilder::default() + .validator_count(7) + .set_status(41, StakerStatus::Validator) + .set_status(51, StakerStatus::Validator) + .set_status(201, StakerStatus::Validator) + .set_status(202, StakerStatus::Validator) + .build_and_execute(|| { + mock::start_active_era(1); + assert_eq_uvec!(Session::validators(), vec![11, 21, 31, 41, 51, 201, 202]); + + let exposure_11 = Staking::eras_stakers(Staking::active_era().unwrap().index, &11); + let exposure_21 = Staking::eras_stakers(Staking::active_era().unwrap().index, &21); + let exposure_31 = Staking::eras_stakers(Staking::active_era().unwrap().index, &31); + + // offence with a major slash + on_offence_now( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::from_percent(50)], + ); + on_offence_now( + &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], + &[Perbill::from_percent(50)], + ); + + // both validators should be disabled + assert!(is_disabled(11)); + assert!(is_disabled(21)); + + // offence with a minor slash + on_offence_now( + &[OffenceDetails { offender: (31, exposure_31.clone()), reporters: vec![] }], + &[Perbill::from_percent(10)], + ); + + // First and second offenders are still disabled + assert!(is_disabled(11)); + assert!(is_disabled(21)); + // New offender is not disabled as limit is reached and his prio is lower + assert!(!is_disabled(31)); + + assert_eq!( + staking_events_since_last_call(), + vec![ + Event::StakersElected, + Event::EraPaid { era_index: 0, validator_payout: 11075, remainder: 33225 }, + Event::SlashReported { + validator: 11, + fraction: Perbill::from_percent(50), + slash_era: 1 + }, + Event::ValidatorDisabled { stash: 11 }, + Event::Slashed { staker: 11, amount: 500 }, + Event::Slashed { staker: 101, amount: 62 }, + Event::SlashReported { + validator: 21, + fraction: Perbill::from_percent(50), + slash_era: 1 + }, + Event::ValidatorDisabled { stash: 21 }, + Event::Slashed { staker: 21, amount: 500 }, + Event::Slashed { staker: 101, amount: 187 }, + Event::SlashReported { + validator: 31, + fraction: Perbill::from_percent(10), + slash_era: 1 + }, + Event::Slashed { staker: 31, amount: 50 }, + ] + ); + }); +} From ca99191329cb1cb00fad7390f8710a83f462a9ef Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 24 Sep 2024 20:52:14 +0100 Subject: [PATCH 22/48] prdoc format fix --- prdoc/pr_5724.prdoc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_5724.prdoc b/prdoc/pr_5724.prdoc index e110901260ee..c795f90c0f21 100644 --- a/prdoc/pr_5724.prdoc +++ b/prdoc/pr_5724.prdoc @@ -12,7 +12,8 @@ doc: When Byzantine Threshold Validators (1/3) are already disabled instead of no longer disabling the highest offenders will be disabled potentially re-enabling low offenders. - - audience: Node Operator + - audience: Node Operator + description: | Implementation of the Stage 3 for the New Disabling Strategy: https://github.com/paritytech/polkadot-sdk/issues/4359 This PR changes when an active validator node gets disabled within parachain consensus (reduced responsibilities and From 27c698fc64d5069ad61b58629f81eb92541520f1 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 1 Oct 2024 12:50:58 +0200 Subject: [PATCH 23/48] storage version bump --- substrate/frame/staking/src/migrations.rs | 4 ++-- substrate/frame/staking/src/pallet/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index c559f828a286..00bf5661d2ea 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -70,7 +70,7 @@ pub mod v16 { fn on_runtime_upgrade() -> Weight { // Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>`. // Using max severity (PerBill) for the migration which effectively makes it so - // offenders before the migration will never be re-enabled. + // offenders before the migration will not be re-enabled this era. let max_perbill = Perbill::from_percent(100); // Inject severity let migrated = v15::DisabledValidators::::take() @@ -85,7 +85,7 @@ pub mod v16 { } } - pub type MigrateV14ToV15 = VersionedMigration< + pub type MigrateV15ToV16 = VersionedMigration< 15, 16, VersionUncheckedMigrateV15ToV16, diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 7a68f7ca5edb..c447d80e5366 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -69,7 +69,7 @@ pub mod pallet { use super::*; /// The in-code storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(15); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(16); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] From 0baf1edd93ac28e364123caf890d26696c0e29c5 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 1 Oct 2024 14:38:31 +0200 Subject: [PATCH 24/48] import clean --- substrate/frame/staking/src/slashing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 4b5c04c8e7a5..4eeb389f1179 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -50,7 +50,7 @@ //! Based on research at use crate::{ - log, BalanceOf, Config, DisabledValidators, DisablingStrategy, Error, Exposure, + BalanceOf, Config, DisabledValidators, DisablingStrategy, Error, Exposure, NegativeImbalanceOf, NominatorSlashInEra, Pallet, Perbill, SessionInterface, SpanSlash, UnappliedSlash, ValidatorSlashInEra, }; From 14f55459d77c4f93336ad696d83bcbf0ee519b7b Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 2 Oct 2024 13:52:29 +0200 Subject: [PATCH 25/48] westend bump --- polkadot/runtime/westend/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index e44ce5c95d3f..ef53822f3036 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -162,7 +162,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: create_runtime_str!("westend"), impl_name: create_runtime_str!("parity-westend"), authoring_version: 2, - spec_version: 1_015_000, + spec_version: 1_016_002, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 26, @@ -1767,6 +1767,7 @@ parameter_types! { /// upgrades in case governance decides to do so. THE ORDER IS IMPORTANT. pub type Migrations = migrations::Unreleased; + /// The runtime migrations per release. #[allow(deprecated, missing_docs)] pub mod migrations { @@ -1799,6 +1800,7 @@ pub mod migrations { MaxPoolsToMigrate, >, pallet_staking::migrations::v15::MigrateV14ToV15, + pallet_staking::migrations::v16::MigrateV15ToV16, ); } From 1880ee5a6ca11bfa9335d0f5c001a600b9653eb3 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 2 Oct 2024 13:53:18 +0200 Subject: [PATCH 26/48] try runtime checks --- substrate/frame/staking/src/migrations.rs | 39 +++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index 00bf5661d2ea..2b1e3b8ee2aa 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -67,6 +67,13 @@ pub mod v16 { pub struct VersionUncheckedMigrateV15ToV16(core::marker::PhantomData); impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV15ToV16 { + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + let old_disabled_validators = v15::DisabledValidators::::get(); + Ok(old_disabled_validators.encode()) + } + fn on_runtime_upgrade() -> Weight { // Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>`. // Using max severity (PerBill) for the migration which effectively makes it so @@ -83,6 +90,38 @@ pub mod v16 { log!(info, "v16 applied successfully."); T::DbWeight::get().reads_writes(1, 1) } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(state: Vec) -> Result<(), TryRuntimeError> { + // Decode state to get old_disabled_validators in a format of Vec + let old_disabled_validators = Vec::::decode(&mut state.as_slice()).expect("Failed to decode state"); + let new_disabled_validators = DisabledValidators::::get(); + + // Compare lengths + frame_support::ensure!( + old_disabled_validators.len() == new_disabled_validators.len(), + "DisabledValidators length mismatch" + ); + + // Compare contents + let new_disabled_validators = new_disabled_validators.into_iter().map(|(v, _)| v).collect::>(); + frame_support::ensure!( + old_disabled_validators == new_disabled_validators, + "DisabledValidator ids mismatch" + ); + + // Verify severity + let max_perbill = Perbill::from_percent(100); + let new_disabled_validators = DisabledValidators::::get(); + for (_, severity) in new_disabled_validators { + frame_support::ensure!( + severity == max_perbill, + "Severity mismatch" + ); + } + + Ok(()) + } } pub type MigrateV15ToV16 = VersionedMigration< From 02c5d512b34bd5df7ab847dd4120ae3f4fe8ca35 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 2 Oct 2024 13:53:25 +0200 Subject: [PATCH 27/48] try runtime tests --- substrate/frame/staking/src/tests.rs | 31 ++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index c455be917b37..7b482ea66e27 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -8702,3 +8702,34 @@ fn do_not_reenable_higher_offenders_mock() { ); }); } + +#[cfg(all(feature = "try-runtime", test))] +mod migration_tests { + use super::*; + use frame_support::traits::UncheckedOnRuntimeUpgrade; + use migrations::{v15, v16}; + + #[test] + fn migrate_v15_to_v16_with_try_runtime() { + ExtBuilder::default() + .validator_count(7) + .build_and_execute(|| { + // Initial setup: Create old `DisabledValidators` in the form of `Vec` + let old_disabled_validators = vec![1u32, 2u32]; + v15::DisabledValidators::::put(old_disabled_validators.clone()); + + // Run pre-upgrade checks + let pre_upgrade_result = v16::VersionUncheckedMigrateV15ToV16::::pre_upgrade(); + assert!(pre_upgrade_result.is_ok()); + let pre_upgrade_state = pre_upgrade_result.unwrap(); + + // Run the migration + v16::VersionUncheckedMigrateV15ToV16::::on_runtime_upgrade(); + + // Run post-upgrade checks + let post_upgrade_result = + v16::VersionUncheckedMigrateV15ToV16::::post_upgrade(pre_upgrade_state); + assert!(post_upgrade_result.is_ok()); + }); + } +} From e0970c75711f598023fe4a2ac0c7cdbb23c63984 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 2 Oct 2024 13:54:35 +0200 Subject: [PATCH 28/48] fmt --- polkadot/runtime/westend/src/lib.rs | 1 - substrate/frame/staking/src/migrations.rs | 12 +++---- substrate/frame/staking/src/slashing.rs | 6 ++-- substrate/frame/staking/src/tests.rs | 38 +++++++++++------------ 4 files changed, 26 insertions(+), 31 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index ef53822f3036..aab6622421e2 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1767,7 +1767,6 @@ parameter_types! { /// upgrades in case governance decides to do so. THE ORDER IS IMPORTANT. pub type Migrations = migrations::Unreleased; - /// The runtime migrations per release. #[allow(deprecated, missing_docs)] pub mod migrations { diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index 2b1e3b8ee2aa..774dd4aadfc7 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -67,7 +67,6 @@ pub mod v16 { pub struct VersionUncheckedMigrateV15ToV16(core::marker::PhantomData); impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV15ToV16 { - #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { let old_disabled_validators = v15::DisabledValidators::::get(); @@ -94,7 +93,8 @@ pub mod v16 { #[cfg(feature = "try-runtime")] fn post_upgrade(state: Vec) -> Result<(), TryRuntimeError> { // Decode state to get old_disabled_validators in a format of Vec - let old_disabled_validators = Vec::::decode(&mut state.as_slice()).expect("Failed to decode state"); + let old_disabled_validators = + Vec::::decode(&mut state.as_slice()).expect("Failed to decode state"); let new_disabled_validators = DisabledValidators::::get(); // Compare lengths @@ -104,7 +104,8 @@ pub mod v16 { ); // Compare contents - let new_disabled_validators = new_disabled_validators.into_iter().map(|(v, _)| v).collect::>(); + let new_disabled_validators = + new_disabled_validators.into_iter().map(|(v, _)| v).collect::>(); frame_support::ensure!( old_disabled_validators == new_disabled_validators, "DisabledValidator ids mismatch" @@ -114,10 +115,7 @@ pub mod v16 { let max_perbill = Perbill::from_percent(100); let new_disabled_validators = DisabledValidators::::get(); for (_, severity) in new_disabled_validators { - frame_support::ensure!( - severity == max_perbill, - "Severity mismatch" - ); + frame_support::ensure!(severity == max_perbill, "Severity mismatch"); } Ok(()) diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 4eeb389f1179..9a0d1568108a 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -50,9 +50,9 @@ //! Based on research at use crate::{ - BalanceOf, Config, DisabledValidators, DisablingStrategy, Error, Exposure, - NegativeImbalanceOf, NominatorSlashInEra, Pallet, Perbill, SessionInterface, SpanSlash, - UnappliedSlash, ValidatorSlashInEra, + BalanceOf, Config, DisabledValidators, DisablingStrategy, Error, Exposure, NegativeImbalanceOf, + NominatorSlashInEra, Pallet, Perbill, SessionInterface, SpanSlash, UnappliedSlash, + ValidatorSlashInEra, }; use alloc::vec::Vec; use codec::{Decode, Encode, MaxEncodedLen}; diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 7b482ea66e27..119004b11ae4 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -8711,25 +8711,23 @@ mod migration_tests { #[test] fn migrate_v15_to_v16_with_try_runtime() { - ExtBuilder::default() - .validator_count(7) - .build_and_execute(|| { - // Initial setup: Create old `DisabledValidators` in the form of `Vec` - let old_disabled_validators = vec![1u32, 2u32]; - v15::DisabledValidators::::put(old_disabled_validators.clone()); - - // Run pre-upgrade checks - let pre_upgrade_result = v16::VersionUncheckedMigrateV15ToV16::::pre_upgrade(); - assert!(pre_upgrade_result.is_ok()); - let pre_upgrade_state = pre_upgrade_result.unwrap(); - - // Run the migration - v16::VersionUncheckedMigrateV15ToV16::::on_runtime_upgrade(); - - // Run post-upgrade checks - let post_upgrade_result = - v16::VersionUncheckedMigrateV15ToV16::::post_upgrade(pre_upgrade_state); - assert!(post_upgrade_result.is_ok()); - }); + ExtBuilder::default().validator_count(7).build_and_execute(|| { + // Initial setup: Create old `DisabledValidators` in the form of `Vec` + let old_disabled_validators = vec![1u32, 2u32]; + v15::DisabledValidators::::put(old_disabled_validators.clone()); + + // Run pre-upgrade checks + let pre_upgrade_result = v16::VersionUncheckedMigrateV15ToV16::::pre_upgrade(); + assert!(pre_upgrade_result.is_ok()); + let pre_upgrade_state = pre_upgrade_result.unwrap(); + + // Run the migration + v16::VersionUncheckedMigrateV15ToV16::::on_runtime_upgrade(); + + // Run post-upgrade checks + let post_upgrade_result = + v16::VersionUncheckedMigrateV15ToV16::::post_upgrade(pre_upgrade_state); + assert!(post_upgrade_result.is_ok()); + }); } } From cb75b92d6e0eeac18d16a9828f672e8dc8d5cd10 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 2 Oct 2024 14:06:20 +0200 Subject: [PATCH 29/48] changelog --- substrate/frame/staking/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/substrate/frame/staking/CHANGELOG.md b/substrate/frame/staking/CHANGELOG.md index 113b7a6200b6..71b806501b28 100644 --- a/substrate/frame/staking/CHANGELOG.md +++ b/substrate/frame/staking/CHANGELOG.md @@ -7,6 +7,10 @@ on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). We maintain a single integer version number for staking pallet to keep track of all storage migrations. +## [v15] +- New default implementation of `DisablingStrategy` - `UpToLimitWithReEnablingDisablingStrategy`. Same as `UpToLimitDisablingStrategy` except when a limit (1/3 default) is reached. When limit is reached the offender is only disabled if his offence is greater or equal than some other already disabled offender. The smallest possible offender is re-enabled to make space for the new greater offender. A limit should thus always be respected. +- `DisabledValidators` changed format to include severity of the offence. + ## [v15] ### Added From e4dfd4a3c4f0f3bf0033d3a3a884647f4da33dca Mon Sep 17 00:00:00 2001 From: Maciej Date: Tue, 15 Oct 2024 10:13:37 +0200 Subject: [PATCH 30/48] Update prdoc/pr_5724.prdoc Co-authored-by: ordian --- prdoc/pr_5724.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_5724.prdoc b/prdoc/pr_5724.prdoc index c795f90c0f21..be9d21c214a8 100644 --- a/prdoc/pr_5724.prdoc +++ b/prdoc/pr_5724.prdoc @@ -17,7 +17,7 @@ doc: Implementation of the Stage 3 for the New Disabling Strategy: https://github.com/paritytech/polkadot-sdk/issues/4359 This PR changes when an active validator node gets disabled within parachain consensus (reduced responsibilities and - reduced rewards) for comitting offences. This should not affect active validators on a dato-day basis and will only + reduced rewards) for comitting offences. This should not affect active validators on a day-to-day basis and will only be relevant when the network is under attack or there is a wide spread malfunction causing slashes. In that case lowest offenders might get eventually re-enabled (back to normal responsibilities and normal rewards). From 12d88d04ea1b92f45ab9528b0b82ae12ef0fd474 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 15 Oct 2024 10:52:08 +0200 Subject: [PATCH 31/48] decision doc fix --- substrate/frame/staking/src/lib.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 1e6d531773da..ae33505eeb41 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1280,14 +1280,7 @@ impl BenchmarkingConfig for TestBenchmarkingConfig { /// Controls validator disabling pub trait DisablingStrategy { - /// Make a disabling decision. Potentially returns 2 validators indices. - /// First index is to be disabled, second is to be re-enabled. - /// - /// Options: - /// (None, None) no changes needed - /// (Some(x), None) just disable x - /// (Some(x), Some(y)) disable x instead of y - /// (None, Some(y)) unreachable + /// Make a disabling decision. Returning a [`DisablingDecision`] fn decision( offender_stash: &T::AccountId, offender_slash_severity: Perbill, @@ -1299,7 +1292,8 @@ pub trait DisablingStrategy { /// Helper struct representing a decision coming from a given [`DisablingStrategy`] implementing /// `decision` /// -/// Currently supports at most 1 disable + 1 re-enable per decision +/// `disable` is the index of the validator to disable, +/// `reenable` is the index of the validator to re-enable. #[derive(Debug)] pub struct DisablingDecision { pub disable: Option, From 9ebd8a708126e3294b3f50b3417f136fa73a3568 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Tue, 15 Oct 2024 12:47:35 +0200 Subject: [PATCH 32/48] changelog fixes --- substrate/frame/staking/CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/substrate/frame/staking/CHANGELOG.md b/substrate/frame/staking/CHANGELOG.md index 71b806501b28..25249ea7fd83 100644 --- a/substrate/frame/staking/CHANGELOG.md +++ b/substrate/frame/staking/CHANGELOG.md @@ -7,7 +7,11 @@ on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). We maintain a single integer version number for staking pallet to keep track of all storage migrations. -## [v15] +## [v16] + + +### Added + - New default implementation of `DisablingStrategy` - `UpToLimitWithReEnablingDisablingStrategy`. Same as `UpToLimitDisablingStrategy` except when a limit (1/3 default) is reached. When limit is reached the offender is only disabled if his offence is greater or equal than some other already disabled offender. The smallest possible offender is re-enabled to make space for the new greater offender. A limit should thus always be respected. - `DisabledValidators` changed format to include severity of the offence. From a3aa92fa7d8308fb963f53b767f3e8af05656b0d Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 30 Oct 2024 13:40:42 +0000 Subject: [PATCH 33/48] #64 fix --- polkadot/runtime/parachains/src/runtime_api_impl/v10.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/polkadot/runtime/parachains/src/runtime_api_impl/v10.rs b/polkadot/runtime/parachains/src/runtime_api_impl/v10.rs index 697890232211..fc3f2520c4aa 100644 --- a/polkadot/runtime/parachains/src/runtime_api_impl/v10.rs +++ b/polkadot/runtime/parachains/src/runtime_api_impl/v10.rs @@ -531,7 +531,12 @@ pub fn disabled_validators() -> Vec where T: shared::Config, { - >::disabled_validators() + // Manually accounting for issue #64 + if shared::Pallet::::on_chain_storage_version() == StorageVersion::new(15) { + shared::migration::v15::DisabledValidators::::get() + } else { + shared::Pallet::::disabled_validators() + } } /// Returns the current state of the node features. From ece9025aca4c7c0aa111e937705b0b47d9bc6bb6 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Thu, 31 Oct 2024 12:41:29 +0000 Subject: [PATCH 34/48] reverting #64 fix in paras runtime --- polkadot/runtime/parachains/src/runtime_api_impl/v10.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/polkadot/runtime/parachains/src/runtime_api_impl/v10.rs b/polkadot/runtime/parachains/src/runtime_api_impl/v10.rs index fc3f2520c4aa..697890232211 100644 --- a/polkadot/runtime/parachains/src/runtime_api_impl/v10.rs +++ b/polkadot/runtime/parachains/src/runtime_api_impl/v10.rs @@ -531,12 +531,7 @@ pub fn disabled_validators() -> Vec where T: shared::Config, { - // Manually accounting for issue #64 - if shared::Pallet::::on_chain_storage_version() == StorageVersion::new(15) { - shared::migration::v15::DisabledValidators::::get() - } else { - shared::Pallet::::disabled_validators() - } + >::disabled_validators() } /// Returns the current state of the node features. From 1e1bc4807d51a19c30be06e8e3b84a1c028d7aef Mon Sep 17 00:00:00 2001 From: Overkillus Date: Thu, 31 Oct 2024 12:54:17 +0000 Subject: [PATCH 35/48] perbill -> OffenceSeverity wrapper --- substrate/frame/staking/src/lib.rs | 18 ++++++++-------- substrate/frame/staking/src/migrations.rs | 18 +++++++++------- substrate/frame/staking/src/pallet/mod.rs | 10 ++++++--- substrate/frame/staking/src/slashing.rs | 12 +++++------ substrate/primitives/staking/src/offence.rs | 23 +++++++++++++++++++++ 5 files changed, 55 insertions(+), 26 deletions(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index ae33505eeb41..e4a3940496fb 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -323,7 +323,7 @@ use sp_runtime::{ Perbill, Perquintill, Rounding, RuntimeDebug, Saturating, }; use sp_staking::{ - offence::{Offence, OffenceError, ReportOffence}, + offence::{Offence, OffenceError, OffenceSeverity, ReportOffence}, EraIndex, ExposurePage, OnStakingUpdate, Page, PagedExposureMetadata, SessionIndex, StakingAccount, }; @@ -1283,9 +1283,9 @@ pub trait DisablingStrategy { /// Make a disabling decision. Returning a [`DisablingDecision`] fn decision( offender_stash: &T::AccountId, - offender_slash_severity: Perbill, + offender_slash_severity: OffenceSeverity, slash_era: EraIndex, - currently_disabled: &Vec<(u32, Perbill)>, + currently_disabled: &Vec<(u32, OffenceSeverity)>, ) -> DisablingDecision; } @@ -1326,9 +1326,9 @@ impl DisablingStrategy { fn decision( offender_stash: &T::AccountId, - _offender_slash_severity: Perbill, + _offender_slash_severity: OffenceSeverity, slash_era: EraIndex, - currently_disabled: &Vec<(u32, Perbill)>, + currently_disabled: &Vec<(u32, OffenceSeverity)>, ) -> DisablingDecision { let active_set = T::SessionInterface::validators(); @@ -1394,9 +1394,9 @@ impl DisablingStrategy { fn decision( offender_stash: &T::AccountId, - offender_slash_severity: Perbill, + offender_slash_severity: OffenceSeverity, slash_era: EraIndex, - currently_disabled: &Vec<(u32, Perbill)>, + currently_disabled: &Vec<(u32, OffenceSeverity)>, ) -> DisablingDecision { let active_set = T::SessionInterface::validators(); @@ -1445,8 +1445,8 @@ impl DisablingStrategy // offender_slash_severity if let Some((smallest_idx, _)) = currently_disabled .iter() - .filter(|(_, perbill)| *perbill <= offender_slash_severity) - .min_by_key(|(_, perbill)| *perbill) + .filter(|(_, severity)| *severity <= offender_slash_severity) + .min_by_key(|(_, severity)| *severity) { log!(debug, "Will disable {:?} and re-enable {:?}", offender_idx, smallest_idx); return DisablingDecision { diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index 774dd4aadfc7..759e9a744f71 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -60,9 +60,10 @@ impl Default for ObsoleteReleases { #[storage_alias] type StorageVersion = StorageValue, ObsoleteReleases, ValueQuery>; -/// Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>` to track offense +/// Migrating `DisabledValidators` from `Vec` to `Vec<(u32, OffenceSeverity)>` to track offense /// severity for re-enabling purposes. pub mod v16 { + use sp_staking::offence::OffenceSeverity; use super::*; pub struct VersionUncheckedMigrateV15ToV16(core::marker::PhantomData); @@ -74,14 +75,15 @@ pub mod v16 { } fn on_runtime_upgrade() -> Weight { - // Migrating `DisabledValidators` from `Vec` to `Vec<(u32, PerBill)>`. - // Using max severity (PerBill) for the migration which effectively makes it so - // offenders before the migration will not be re-enabled this era. - let max_perbill = Perbill::from_percent(100); + // Migrating `DisabledValidators` from `Vec` to `Vec<(u32, OffenceSeverity)>`. + // Using max severity (PerBill 100%) for the migration which effectively makes it so + // offenders before the migration will not be re-enabled this era unless there are + // other 100% offenders. + let max_offence = OffenceSeverity(Perbill::from_percent(100)); // Inject severity let migrated = v15::DisabledValidators::::take() .into_iter() - .map(|v| (v, max_perbill)) + .map(|v| (v, max_offence.clone())) .collect::>(); DisabledValidators::::set(migrated); @@ -112,10 +114,10 @@ pub mod v16 { ); // Verify severity - let max_perbill = Perbill::from_percent(100); + let max_severity = OffenceSeverity(Perbill::from_percent(100)); let new_disabled_validators = DisabledValidators::::get(); for (_, severity) in new_disabled_validators { - frame_support::ensure!(severity == max_perbill, "Severity mismatch"); + frame_support::ensure!(severity == max_severity, "Severity mismatch"); } Ok(()) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index c447d80e5366..0100f6aa68cc 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -41,6 +41,7 @@ use sp_staking::{ EraIndex, Page, SessionIndex, StakingAccount::{self, Controller, Stash}, StakingInterface, + offence::OffenceSeverity, }; mod impls; @@ -723,11 +724,14 @@ pub mod pallet { /// implementor of [`DisablingStrategy`] defines if a validator should be disabled which /// implicitly means that the implementor also controls the max number of disabled validators. /// - /// The vec is always kept sorted so that we can find whether a given validator has previously - /// offended using binary search. + /// The vec is always kept sorted based on the u32 ubdex so that we can find whether a given + /// validator has previously offended using binary search. + /// + /// Additionally, each disabled validator is associated with an `OffenceSeverity` which represents + /// how severe is the offence that got the validator disabled. #[pallet::storage] #[pallet::unbounded] - pub type DisabledValidators = StorageValue<_, Vec<(u32, Perbill)>, ValueQuery>; + pub type DisabledValidators = StorageValue<_, Vec<(u32, OffenceSeverity)>, ValueQuery>; /// The threshold for when users can start calling `chill_other` for other validators / /// nominators. The threshold is compared to the actual number of validators / nominators diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 9a0d1568108a..e9fbfd49a3ee 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -65,7 +65,7 @@ use sp_runtime::{ traits::{Saturating, Zero}, DispatchResult, RuntimeDebug, }; -use sp_staking::{EraIndex, StakingInterface}; +use sp_staking::{EraIndex, StakingInterface, offence::OffenceSeverity}; /// The proportion of the slashing reward to be paid out on the first slashing detection. /// This is f_1 in the paper. @@ -324,8 +324,9 @@ fn kick_out_if_recent(params: SlashParams) { /// validators provided by [`decision`]. fn add_offending_validator(params: &SlashParams) { DisabledValidators::::mutate(|disabled| { + let new_severity = OffenceSeverity(params.slash); let decision = - T::DisablingStrategy::decision(params.stash, params.slash, params.slash_era, &disabled); + T::DisablingStrategy::decision(params.stash, new_severity, params.slash_era, &disabled); if let Some(offender_idx) = decision.disable { // Check if the offender is already disabled @@ -333,14 +334,13 @@ fn add_offending_validator(params: &SlashParams) { // 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 { - *old_severity = params.slash; + if new_severity > *old_severity { + *old_severity = new_severity; } }, Err(index) => { // Offender is not disabled, add to `DisabledValidators` and disable it - let severity = params.slash; - disabled.insert(index, (offender_idx, severity)); + disabled.insert(index, (offender_idx, new_severity)); // Propagate disablement to session level T::SessionInterface::disable_validator(offender_idx); // Emit event that a validator got disabled diff --git a/substrate/primitives/staking/src/offence.rs b/substrate/primitives/staking/src/offence.rs index 2c2ebc1fc971..95214449ea4f 100644 --- a/substrate/primitives/staking/src/offence.rs +++ b/substrate/primitives/staking/src/offence.rs @@ -242,3 +242,26 @@ impl OffenceReportSystem for () { Ok(()) } } + +/// Wrapper type representing the severity of an offence. +/// +/// As of now the only meaningful value taken into account +/// when deciding the severity of an offence is the associated +/// slash amount `Perbill`. +/// +/// For instance used for the purposes of distinguishing who should be +/// prioritized for disablement. +#[derive(Clone, Copy, PartialEq, Eq, Encode, Decode, sp_runtime::RuntimeDebug, scale_info::TypeInfo)] +pub struct OffenceSeverity(pub Perbill); + +impl PartialOrd for OffenceSeverity { + fn partial_cmp(&self, other: &Self) -> Option { + self.0.partial_cmp(&other.0) + } +} + +impl Ord for OffenceSeverity { + fn cmp(&self, other: &Self) -> core::cmp::Ordering { + self.0.cmp(&other.0) + } +} \ No newline at end of file From 3382397b9273b594cfa6440adb46875cd758eee0 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Thu, 31 Oct 2024 12:54:34 +0000 Subject: [PATCH 36/48] OffenceSeverity adjusted tests --- substrate/frame/staking/src/tests.rs | 57 ++++++++++++++-------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 119004b11ae4..890324c77b86 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -8284,12 +8284,13 @@ mod byzantine_threshold_disabling_strategy { tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitDisablingStrategy, }; use sp_runtime::Perbill; - use sp_staking::EraIndex; + use sp_staking::{EraIndex, offence::OffenceSeverity}; // Common test data - the stash of the offending validator, the era of the offence and the // active set const OFFENDER_ID: ::AccountId = 7; - const OFFENDER_SLASH: Perbill = Perbill::from_percent(100); + const MAX_OFFENDER_SEVERITY: OffenceSeverity = OffenceSeverity(Perbill::from_percent(100)); + const MIN_OFFENDER_SEVERITY: OffenceSeverity = OffenceSeverity(Perbill::from_percent(0)); const SLASH_ERA: EraIndex = 1; const ACTIVE_SET: [::ValidatorId; 7] = [1, 2, 3, 4, 5, 6, 7]; const OFFENDER_VALIDATOR_IDX: u32 = 6; // the offender is with index 6 in the active set @@ -8304,7 +8305,7 @@ mod byzantine_threshold_disabling_strategy { let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8316,14 +8317,13 @@ mod byzantine_threshold_disabling_strategy { #[test] fn dont_disable_beyond_byzantine_threshold() { sp_io::TestExternalities::default().execute_with(|| { - let max_slash = Perbill::from_percent(100); - let initially_disabled = vec![(1, max_slash), (2, max_slash)]; + let initially_disabled = vec![(1, MIN_OFFENDER_SEVERITY), (2, MAX_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8335,14 +8335,13 @@ mod byzantine_threshold_disabling_strategy { #[test] fn disable_when_below_byzantine_threshold() { sp_io::TestExternalities::default().execute_with(|| { - let max_slash = Perbill::from_percent(100); - let initially_disabled = vec![(1, max_slash)]; + let initially_disabled = vec![(1, MAX_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8358,13 +8357,13 @@ mod disabling_strategy_with_reenabling { UpToLimitWithReEnablingDisablingStrategy, }; use sp_runtime::Perbill; - use sp_staking::EraIndex; + use sp_staking::{EraIndex, offence::OffenceSeverity}; // Common test data - the stash of the offending validator, the era of the offence and the // active set const OFFENDER_ID: ::AccountId = 7; - const OFFENDER_SLASH_MINOR: Perbill = Perbill::from_percent(10); - const OFFENDER_SLASH_MAJOR: Perbill = Perbill::from_percent(100); + const MAX_OFFENDER_SEVERITY: OffenceSeverity = OffenceSeverity(Perbill::from_percent(100)); + const LOW_OFFENDER_SEVERITY: OffenceSeverity = OffenceSeverity(Perbill::from_percent(0)); const SLASH_ERA: EraIndex = 1; const ACTIVE_SET: [::ValidatorId; 7] = [1, 2, 3, 4, 5, 6, 7]; const OFFENDER_VALIDATOR_IDX: u32 = 6; // the offender is with index 6 in the active set @@ -8379,7 +8378,7 @@ mod disabling_strategy_with_reenabling { let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MAJOR, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8391,13 +8390,13 @@ mod disabling_strategy_with_reenabling { #[test] fn disable_when_below_byzantine_threshold() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![(0, OFFENDER_SLASH_MAJOR)]; + let initially_disabled = vec![(0, MAX_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MAJOR, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8411,13 +8410,13 @@ mod disabling_strategy_with_reenabling { #[test] fn reenable_arbitrary_on_equal_severity() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![(0, OFFENDER_SLASH_MAJOR), (1, OFFENDER_SLASH_MAJOR)]; + let initially_disabled = vec![(0, MAX_OFFENDER_SEVERITY), (1, MAX_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MAJOR, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8432,13 +8431,13 @@ mod disabling_strategy_with_reenabling { #[test] fn do_not_reenable_higher_offenders() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![(0, OFFENDER_SLASH_MAJOR), (1, OFFENDER_SLASH_MAJOR)]; + let initially_disabled = vec![(0, MAX_OFFENDER_SEVERITY), (1, MAX_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MINOR, + LOW_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8450,13 +8449,13 @@ mod disabling_strategy_with_reenabling { #[test] fn reenable_lower_offenders() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![(0, OFFENDER_SLASH_MINOR), (1, OFFENDER_SLASH_MINOR)]; + let initially_disabled = vec![(0, LOW_OFFENDER_SEVERITY), (1, LOW_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MAJOR, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8471,13 +8470,13 @@ mod disabling_strategy_with_reenabling { #[test] fn reenable_lower_offenders_unordered() { sp_io::TestExternalities::default().execute_with(|| { - let initially_disabled = vec![(0, OFFENDER_SLASH_MAJOR), (1, OFFENDER_SLASH_MINOR)]; + let initially_disabled = vec![(0, MAX_OFFENDER_SEVERITY), (1, LOW_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MAJOR, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8493,13 +8492,13 @@ mod disabling_strategy_with_reenabling { fn update_severity() { sp_io::TestExternalities::default().execute_with(|| { let initially_disabled = - vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MINOR), (0, OFFENDER_SLASH_MAJOR)]; + vec![(OFFENDER_VALIDATOR_IDX, LOW_OFFENDER_SEVERITY), (0, MAX_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MAJOR, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8514,13 +8513,13 @@ mod disabling_strategy_with_reenabling { fn update_cannot_lower_severity() { sp_io::TestExternalities::default().execute_with(|| { let initially_disabled = - vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MAJOR), (0, OFFENDER_SLASH_MAJOR)]; + vec![(OFFENDER_VALIDATOR_IDX, MAX_OFFENDER_SEVERITY), (0, MAX_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MINOR, + LOW_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); @@ -8533,13 +8532,13 @@ mod disabling_strategy_with_reenabling { fn no_accidental_reenablement_on_repeated_offence() { sp_io::TestExternalities::default().execute_with(|| { let initially_disabled = - vec![(OFFENDER_VALIDATOR_IDX, OFFENDER_SLASH_MAJOR), (0, OFFENDER_SLASH_MINOR)]; + vec![(OFFENDER_VALIDATOR_IDX, MAX_OFFENDER_SEVERITY), (0, LOW_OFFENDER_SEVERITY)]; pallet_session::Validators::::put(ACTIVE_SET.to_vec()); let disabling_decision = >::decision( &OFFENDER_ID, - OFFENDER_SLASH_MAJOR, + MAX_OFFENDER_SEVERITY, SLASH_ERA, &initially_disabled, ); From bd7cd727e162202e51bed92a5c0eeca4f441b316 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 4 Nov 2024 08:59:48 +0000 Subject: [PATCH 37/48] deduplicate disable_limit --- substrate/frame/staking/src/lib.rs | 36 +++++++++++++++++------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index e4a3940496fb..0fc787f7d556 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1300,10 +1300,26 @@ pub struct DisablingDecision { pub reenable: Option, } -/// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a +/// Calculate the disabling limit based on the number of validators and the disabling limit factor. +/// +/// This is a sensible default implementation for the disabling limit factor for most disabling strategies. +/// +/// Disabling limit factor n=2 -> 1/n = 1/2 = 50% of validators can be disabled +fn factor_based_disable_limit(validators_len: usize, disabling_limit_factor: usize) -> usize { + validators_len + .saturating_sub(1) + .checked_div(disabling_limit_factor) + .unwrap_or_else(|| { + defensive!("DISABLING_LIMIT_FACTOR should not be 0"); + 0 + }) +} + +/// Implementation of [`DisablingStrategy`] using factor_based_disable_limit which disables validators from the active set up to a /// threshold. `DISABLING_LIMIT_FACTOR` is the factor of the maximum disabled validators in the /// active set. E.g. setting this value to `3` means no more than 1/3 of the validators in the /// active set can be disabled in an era. +/// /// By default a factor of 3 is used which is the byzantine threshold. pub struct UpToLimitDisablingStrategy; @@ -1311,13 +1327,7 @@ impl UpToLimitDisablingStrategy usize { - validators_len - .saturating_sub(1) - .checked_div(DISABLING_LIMIT_FACTOR) - .unwrap_or_else(|| { - defensive!("DISABLING_LIMIT_FACTOR should not be 0"); - 0 - }) + factor_based_disable_limit(validators_len, DISABLING_LIMIT_FACTOR) } } @@ -1367,7 +1377,7 @@ impl DisablingStrategy } /// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a -/// limit and if the limit is reached and the new offender is higher (bigger punishment/severity) +/// limit (factor_based_disable_limit) and if the limit is reached and the new offender is higher (bigger punishment/severity) /// then it re-enables to lowest offender to free up space for the new offender. /// /// An extension of [`UpToLimitDisablingStrategy`]. @@ -1379,13 +1389,7 @@ impl /// Disabling limit calculated from the total number of validators in the active set. When /// reached re-enabling logic might kick in. pub fn disable_limit(validators_len: usize) -> usize { - validators_len - .saturating_sub(1) - .checked_div(DISABLING_LIMIT_FACTOR) - .unwrap_or_else(|| { - defensive!("DISABLING_LIMIT_FACTOR should not be 0"); - 0 - }) + factor_based_disable_limit(validators_len, DISABLING_LIMIT_FACTOR) } } From a07a7e8602623ce8fbf251db573959fc5a20fff0 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 4 Nov 2024 09:28:33 +0000 Subject: [PATCH 38/48] typo --- substrate/frame/staking/src/pallet/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 0100f6aa68cc..9a2576972806 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -724,7 +724,7 @@ pub mod pallet { /// implementor of [`DisablingStrategy`] defines if a validator should be disabled which /// implicitly means that the implementor also controls the max number of disabled validators. /// - /// The vec is always kept sorted based on the u32 ubdex so that we can find whether a given + /// The vec is always kept sorted based on the u32 index so that we can find whether a given /// validator has previously offended using binary search. /// /// Additionally, each disabled validator is associated with an `OffenceSeverity` which represents From 675b2be6bf9cba543645e1c744aa998b2f14b1bf Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 4 Nov 2024 09:46:55 +0000 Subject: [PATCH 39/48] non-cumulative assumption for re-enabling strategy impl --- substrate/frame/staking/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 0fc787f7d556..3d183d928996 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1379,6 +1379,10 @@ impl DisablingStrategy /// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a /// limit (factor_based_disable_limit) and if the limit is reached and the new offender is higher (bigger punishment/severity) /// then it re-enables to lowest offender to free up space for the new offender. +/// +/// This strategy is not based on cumulative severity of offences but only on the severity of the highest offence. +/// Offender first committing a 25% offence and then a 50% offence will be treated the same as an offender committing 50% +/// offence. /// /// An extension of [`UpToLimitDisablingStrategy`]. pub struct UpToLimitWithReEnablingDisablingStrategy; From 273342a659c9e694e9be41ac72b018a42900c244 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 4 Nov 2024 19:43:09 +0000 Subject: [PATCH 40/48] md fmt --- substrate/frame/staking/CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/substrate/frame/staking/CHANGELOG.md b/substrate/frame/staking/CHANGELOG.md index 25249ea7fd83..064a7d4a48f4 100644 --- a/substrate/frame/staking/CHANGELOG.md +++ b/substrate/frame/staking/CHANGELOG.md @@ -12,7 +12,11 @@ migrations. ### Added -- New default implementation of `DisablingStrategy` - `UpToLimitWithReEnablingDisablingStrategy`. Same as `UpToLimitDisablingStrategy` except when a limit (1/3 default) is reached. When limit is reached the offender is only disabled if his offence is greater or equal than some other already disabled offender. The smallest possible offender is re-enabled to make space for the new greater offender. A limit should thus always be respected. +- New default implementation of `DisablingStrategy` - `UpToLimitWithReEnablingDisablingStrategy`. + Same as `UpToLimitDisablingStrategy` except when a limit (1/3 default) is reached. When limit is + reached the offender is only disabled if his offence is greater or equal than some other already + disabled offender. The smallest possible offender is re-enabled to make space for the new greater + offender. A limit should thus always be respected. - `DisabledValidators` changed format to include severity of the offence. ## [v15] From cac5a6134bc5a08e812eace0753226b77564507c Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 4 Nov 2024 20:13:03 +0000 Subject: [PATCH 41/48] fmt --- .../src/validate_block/trie_cache.rs | 5 ++- .../src/validate_block/trie_recorder.rs | 5 ++- substrate/frame/staking/src/lib.rs | 44 ++++++++++--------- substrate/frame/staking/src/migrations.rs | 2 +- substrate/frame/staking/src/pallet/mod.rs | 11 ++--- substrate/frame/staking/src/slashing.rs | 2 +- substrate/frame/staking/src/tests.rs | 4 +- substrate/primitives/staking/src/offence.rs | 10 +++-- .../state-machine/src/trie_backend.rs | 20 +++++++-- substrate/primitives/trie/src/recorder.rs | 5 ++- 10 files changed, 67 insertions(+), 41 deletions(-) diff --git a/cumulus/pallets/parachain-system/src/validate_block/trie_cache.rs b/cumulus/pallets/parachain-system/src/validate_block/trie_cache.rs index 035541fb17b1..36efd3decf77 100644 --- a/cumulus/pallets/parachain-system/src/validate_block/trie_cache.rs +++ b/cumulus/pallets/parachain-system/src/validate_block/trie_cache.rs @@ -85,7 +85,10 @@ impl CacheProvider { } impl TrieCacheProvider for CacheProvider { - type Cache<'a> = TrieCache<'a, H> where H: 'a; + type Cache<'a> + = TrieCache<'a, H> + where + H: 'a; fn as_trie_db_cache(&self, storage_root: ::Out) -> Self::Cache<'_> { TrieCache { diff --git a/cumulus/pallets/parachain-system/src/validate_block/trie_recorder.rs b/cumulus/pallets/parachain-system/src/validate_block/trie_recorder.rs index 4a478d047f1b..8dc2f20dd390 100644 --- a/cumulus/pallets/parachain-system/src/validate_block/trie_recorder.rs +++ b/cumulus/pallets/parachain-system/src/validate_block/trie_recorder.rs @@ -115,7 +115,10 @@ impl SizeOnlyRecorderProvider { } impl sp_trie::TrieRecorderProvider for SizeOnlyRecorderProvider { - type Recorder<'a> = SizeOnlyRecorder<'a, H> where H: 'a; + type Recorder<'a> + = SizeOnlyRecorder<'a, H> + where + H: 'a; fn drain_storage_proof(self) -> Option { None diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index f36fd3729c1f..7a3746815017 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1302,25 +1302,26 @@ pub struct DisablingDecision { } /// Calculate the disabling limit based on the number of validators and the disabling limit factor. -/// -/// This is a sensible default implementation for the disabling limit factor for most disabling strategies. -/// +/// +/// This is a sensible default implementation for the disabling limit factor for most disabling +/// strategies. +/// /// Disabling limit factor n=2 -> 1/n = 1/2 = 50% of validators can be disabled fn factor_based_disable_limit(validators_len: usize, disabling_limit_factor: usize) -> usize { - validators_len - .saturating_sub(1) - .checked_div(disabling_limit_factor) - .unwrap_or_else(|| { - defensive!("DISABLING_LIMIT_FACTOR should not be 0"); - 0 - }) + validators_len + .saturating_sub(1) + .checked_div(disabling_limit_factor) + .unwrap_or_else(|| { + defensive!("DISABLING_LIMIT_FACTOR should not be 0"); + 0 + }) } -/// Implementation of [`DisablingStrategy`] using factor_based_disable_limit which disables validators from the active set up to a -/// threshold. `DISABLING_LIMIT_FACTOR` is the factor of the maximum disabled validators in the -/// active set. E.g. setting this value to `3` means no more than 1/3 of the validators in the -/// active set can be disabled in an era. -/// +/// Implementation of [`DisablingStrategy`] using factor_based_disable_limit which disables +/// validators from the active set up to a threshold. `DISABLING_LIMIT_FACTOR` is the factor of the +/// maximum disabled validators in the active set. E.g. setting this value to `3` means no more than +/// 1/3 of the validators in the active set can be disabled in an era. +/// /// By default a factor of 3 is used which is the byzantine threshold. pub struct UpToLimitDisablingStrategy; @@ -1378,12 +1379,13 @@ impl DisablingStrategy } /// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a -/// limit (factor_based_disable_limit) and if the limit is reached and the new offender is higher (bigger punishment/severity) -/// then it re-enables to lowest offender to free up space for the new offender. -/// -/// This strategy is not based on cumulative severity of offences but only on the severity of the highest offence. -/// Offender first committing a 25% offence and then a 50% offence will be treated the same as an offender committing 50% -/// offence. +/// limit (factor_based_disable_limit) and if the limit is reached and the new offender is higher +/// (bigger punishment/severity) then it re-enables to lowest offender to free up space for the new +/// offender. +/// +/// This strategy is not based on cumulative severity of offences but only on the severity of the +/// highest offence. Offender first committing a 25% offence and then a 50% offence will be treated +/// the same as an offender committing 50% offence. /// /// An extension of [`UpToLimitDisablingStrategy`]. pub struct UpToLimitWithReEnablingDisablingStrategy; diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index 759e9a744f71..4e44f645d3c4 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -63,8 +63,8 @@ type StorageVersion = StorageValue, ObsoleteReleases, Value /// Migrating `DisabledValidators` from `Vec` to `Vec<(u32, OffenceSeverity)>` to track offense /// severity for re-enabling purposes. pub mod v16 { - use sp_staking::offence::OffenceSeverity; use super::*; + use sp_staking::offence::OffenceSeverity; pub struct VersionUncheckedMigrateV15ToV16(core::marker::PhantomData); impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV15ToV16 { diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 00b88ab41464..5ca0158d0e6c 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -38,10 +38,10 @@ use sp_runtime::{ }; use sp_staking::{ + offence::OffenceSeverity, EraIndex, Page, SessionIndex, StakingAccount::{self, Controller, Stash}, StakingInterface, - offence::OffenceSeverity, }; mod impls; @@ -726,12 +726,13 @@ pub mod pallet { /// /// The vec is always kept sorted based on the u32 index so that we can find whether a given /// validator has previously offended using binary search. - /// - /// Additionally, each disabled validator is associated with an `OffenceSeverity` which represents - /// how severe is the offence that got the validator disabled. + /// + /// Additionally, each disabled validator is associated with an `OffenceSeverity` which + /// represents how severe is the offence that got the validator disabled. #[pallet::storage] #[pallet::unbounded] - pub type DisabledValidators = StorageValue<_, Vec<(u32, OffenceSeverity)>, ValueQuery>; + pub type DisabledValidators = + StorageValue<_, Vec<(u32, OffenceSeverity)>, ValueQuery>; /// The threshold for when users can start calling `chill_other` for other validators / /// nominators. The threshold is compared to the actual number of validators / nominators diff --git a/substrate/frame/staking/src/slashing.rs b/substrate/frame/staking/src/slashing.rs index 963c6bd6657b..ae76b0707dcb 100644 --- a/substrate/frame/staking/src/slashing.rs +++ b/substrate/frame/staking/src/slashing.rs @@ -65,7 +65,7 @@ use sp_runtime::{ traits::{Saturating, Zero}, DispatchResult, RuntimeDebug, }; -use sp_staking::{EraIndex, StakingInterface, offence::OffenceSeverity}; +use sp_staking::{offence::OffenceSeverity, EraIndex, StakingInterface}; /// The proportion of the slashing reward to be paid out on the first slashing detection. /// This is f_1 in the paper. diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index cb13a260a5a1..fa3aaf6d0ca7 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -8297,7 +8297,7 @@ mod byzantine_threshold_disabling_strategy { tests::Test, ActiveEra, ActiveEraInfo, DisablingStrategy, UpToLimitDisablingStrategy, }; use sp_runtime::Perbill; - use sp_staking::{EraIndex, offence::OffenceSeverity}; + use sp_staking::{offence::OffenceSeverity, EraIndex}; // Common test data - the stash of the offending validator, the era of the offence and the // active set @@ -8370,7 +8370,7 @@ mod disabling_strategy_with_reenabling { UpToLimitWithReEnablingDisablingStrategy, }; use sp_runtime::Perbill; - use sp_staking::{EraIndex, offence::OffenceSeverity}; + use sp_staking::{offence::OffenceSeverity, EraIndex}; // Common test data - the stash of the offending validator, the era of the offence and the // active set diff --git a/substrate/primitives/staking/src/offence.rs b/substrate/primitives/staking/src/offence.rs index 95214449ea4f..e73e8efe5839 100644 --- a/substrate/primitives/staking/src/offence.rs +++ b/substrate/primitives/staking/src/offence.rs @@ -244,14 +244,16 @@ impl OffenceReportSystem for () { } /// Wrapper type representing the severity of an offence. -/// +/// /// As of now the only meaningful value taken into account /// when deciding the severity of an offence is the associated /// slash amount `Perbill`. -/// +/// /// For instance used for the purposes of distinguishing who should be /// prioritized for disablement. -#[derive(Clone, Copy, PartialEq, Eq, Encode, Decode, sp_runtime::RuntimeDebug, scale_info::TypeInfo)] +#[derive( + Clone, Copy, PartialEq, Eq, Encode, Decode, sp_runtime::RuntimeDebug, scale_info::TypeInfo, +)] pub struct OffenceSeverity(pub Perbill); impl PartialOrd for OffenceSeverity { @@ -264,4 +266,4 @@ impl Ord for OffenceSeverity { fn cmp(&self, other: &Self) -> core::cmp::Ordering { self.0.cmp(&other.0) } -} \ No newline at end of file +} diff --git a/substrate/primitives/state-machine/src/trie_backend.rs b/substrate/primitives/state-machine/src/trie_backend.rs index f91ce5d2e52f..8d4dfd34240d 100644 --- a/substrate/primitives/state-machine/src/trie_backend.rs +++ b/substrate/primitives/state-machine/src/trie_backend.rs @@ -73,7 +73,10 @@ pub trait TrieCacheProvider { #[cfg(feature = "std")] impl TrieCacheProvider for LocalTrieCache { - type Cache<'a> = TrieCache<'a, H> where H: 'a; + type Cache<'a> + = TrieCache<'a, H> + where + H: 'a; fn as_trie_db_cache(&self, storage_root: H::Out) -> Self::Cache<'_> { self.as_trie_db_cache(storage_root) @@ -90,7 +93,10 @@ impl TrieCacheProvider for LocalTrieCache { #[cfg(feature = "std")] impl TrieCacheProvider for &LocalTrieCache { - type Cache<'a> = TrieCache<'a, H> where Self: 'a; + type Cache<'a> + = TrieCache<'a, H> + where + Self: 'a; fn as_trie_db_cache(&self, storage_root: H::Out) -> Self::Cache<'_> { (*self).as_trie_db_cache(storage_root) @@ -139,7 +145,10 @@ impl trie_db::TrieCache> for UnimplementedCacheProvider< #[cfg(not(feature = "std"))] impl TrieCacheProvider for UnimplementedCacheProvider { - type Cache<'a> = UnimplementedCacheProvider where H: 'a; + type Cache<'a> + = UnimplementedCacheProvider + where + H: 'a; fn as_trie_db_cache(&self, _storage_root: ::Out) -> Self::Cache<'_> { unimplemented!() @@ -176,7 +185,10 @@ impl trie_db::TrieRecorder for UnimplementedRecorderProvider< #[cfg(not(feature = "std"))] impl TrieRecorderProvider for UnimplementedRecorderProvider { - type Recorder<'a> = UnimplementedRecorderProvider where H: 'a; + type Recorder<'a> + = UnimplementedRecorderProvider + where + H: 'a; fn drain_storage_proof(self) -> Option { unimplemented!() diff --git a/substrate/primitives/trie/src/recorder.rs b/substrate/primitives/trie/src/recorder.rs index 2886577eddc6..4ec13066ded7 100644 --- a/substrate/primitives/trie/src/recorder.rs +++ b/substrate/primitives/trie/src/recorder.rs @@ -252,7 +252,10 @@ pub struct TrieRecorder<'a, H: Hasher> { } impl crate::TrieRecorderProvider for Recorder { - type Recorder<'a> = TrieRecorder<'a, H> where H: 'a; + type Recorder<'a> + = TrieRecorder<'a, H> + where + H: 'a; fn drain_storage_proof(self) -> Option { Some(Recorder::drain_storage_proof(self)) From b8f6e04d72227281a4ad6a8749833423fa735201 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 4 Nov 2024 22:20:17 +0000 Subject: [PATCH 42/48] clean --- substrate/frame/staking/src/migrations.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/migrations.rs b/substrate/frame/staking/src/migrations.rs index 4e44f645d3c4..9dfa93c70b32 100644 --- a/substrate/frame/staking/src/migrations.rs +++ b/substrate/frame/staking/src/migrations.rs @@ -83,7 +83,7 @@ pub mod v16 { // Inject severity let migrated = v15::DisabledValidators::::take() .into_iter() - .map(|v| (v, max_offence.clone())) + .map(|v| (v, max_offence)) .collect::>(); DisabledValidators::::set(migrated); From 2fe9b6da3cf4861ad2686689f9ce1983172b4c4d Mon Sep 17 00:00:00 2001 From: Maciej Date: Tue, 5 Nov 2024 11:20:41 +0000 Subject: [PATCH 43/48] typo Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com> --- substrate/frame/staking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 7a3746815017..f9037ec78bb6 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -1380,7 +1380,7 @@ impl DisablingStrategy /// Implementation of [`DisablingStrategy`] which disables validators from the active set up to a /// limit (factor_based_disable_limit) and if the limit is reached and the new offender is higher -/// (bigger punishment/severity) then it re-enables to lowest offender to free up space for the new +/// (bigger punishment/severity) then it re-enables the lowest offender to free up space for the new /// offender. /// /// This strategy is not based on cumulative severity of offences but only on the severity of the From 3802d74edcc1f327c5afd81fcac34bcdcae9f79e Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 13 Nov 2024 12:13:25 +0000 Subject: [PATCH 44/48] defensive --- substrate/frame/session/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index dcbf4df74ba4..5b1af14a5e82 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -128,7 +128,7 @@ use frame_support::{ ensure, traits::{ EstimateNextNewSession, EstimateNextSessionRotation, FindAuthor, Get, OneSessionHandler, - ValidatorRegistration, ValidatorSet, + ValidatorRegistration, ValidatorSet, Defensive, }, weights::Weight, Parameter, @@ -737,7 +737,7 @@ impl Pallet { /// Re-enable the validator of index `i`, returns `false` if the validator was already enabled. pub fn enable_index(i: u32) -> bool { - if i >= Validators::::decode_len().unwrap_or(0) as u32 { + if i >= Validators::::decode_len().defensive_unwrap_or(0) as u32 { return false } From c4a19f56be1ec729db8d0b9845a2af997efbb3bb Mon Sep 17 00:00:00 2001 From: Overkillus Date: Wed, 13 Nov 2024 14:50:59 +0000 Subject: [PATCH 45/48] fmt --- substrate/frame/session/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/session/src/lib.rs b/substrate/frame/session/src/lib.rs index 5b1af14a5e82..e8b4a355f49a 100644 --- a/substrate/frame/session/src/lib.rs +++ b/substrate/frame/session/src/lib.rs @@ -127,8 +127,8 @@ use frame_support::{ dispatch::DispatchResult, ensure, traits::{ - EstimateNextNewSession, EstimateNextSessionRotation, FindAuthor, Get, OneSessionHandler, - ValidatorRegistration, ValidatorSet, Defensive, + Defensive, EstimateNextNewSession, EstimateNextSessionRotation, FindAuthor, Get, + OneSessionHandler, ValidatorRegistration, ValidatorSet, }, weights::Weight, Parameter, From 7a00178f39914fc5955c4a4392662a0015143d3a Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 18 Nov 2024 10:28:41 +0000 Subject: [PATCH 46/48] westend semver rv --- polkadot/runtime/westend/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 6de19b5643c3..7a5562cc98c1 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -172,7 +172,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { spec_name: alloc::borrow::Cow::Borrowed("westend"), impl_name: alloc::borrow::Cow::Borrowed("parity-westend"), authoring_version: 2, - spec_version: 1_016_002, + spec_version: 1_016_001, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 26, From 107da1f1eb2c84081eaab91df81f17e90870fc7f Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 18 Nov 2024 12:11:21 +0000 Subject: [PATCH 47/48] mass_slash test fix --- .../test-staking-e2e/src/lib.rs | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs index dd340bb8a2e6..059527a183a4 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs @@ -147,28 +147,33 @@ fn mass_slash_doesnt_enter_emergency_phase() { let active_set_size_before_slash = Session::validators().len(); - // Slash more than 1/3 of the active validators - let mut slashed = slash_half_the_active_set(); + // assuming half is above the disabling limit (default 1/3), otherwise test will break + let slashed = slash_half_the_active_set(); let active_set_size_after_slash = Session::validators().len(); // active set should stay the same before and after the slash assert_eq!(active_set_size_before_slash, active_set_size_after_slash); - // Slashed validators are disabled up to a limit - slashed.truncate(pallet_staking::UpToLimitWithReEnablingDisablingStrategy::< - SLASHING_DISABLING_FACTOR, - >::disable_limit(active_set_size_after_slash)); - // Find the indices of the disabled validators let active_set = Session::validators(); - let expected_disabled = slashed + let potentially_disabled = slashed .into_iter() .map(|d| active_set.iter().position(|a| *a == d).unwrap() as u32) .collect::>(); + // Ensure that every actually disabled validator is also in the potentially disabled set (not + // necessarily the other way around) + let disabled = Session::disabled_validators(); + for d in disabled.iter() { + assert!(potentially_disabled.contains(d)); + } + + // Ensure no more than disabling limit of validators (default 1/3) is disabled + let disabling_limit = pallet_staking::UpToLimitWithReEnablingDisablingStrategy::::disable_limit(active_set_size_before_slash); + assert!(disabled.len() == disabling_limit); + assert_eq!(pallet_staking::ForceEra::::get(), pallet_staking::Forcing::NotForcing); - assert_eq!(Session::disabled_validators(), expected_disabled); }); } From a3b820c3b18fa61bcdfb8016e39c45c73d359826 Mon Sep 17 00:00:00 2001 From: Overkillus Date: Mon, 18 Nov 2024 12:36:49 +0000 Subject: [PATCH 48/48] fmt --- .../test-staking-e2e/src/lib.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs index 059527a183a4..26a6345e145f 100644 --- a/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs +++ b/substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs @@ -162,15 +162,17 @@ fn mass_slash_doesnt_enter_emergency_phase() { .map(|d| active_set.iter().position(|a| *a == d).unwrap() as u32) .collect::>(); - // Ensure that every actually disabled validator is also in the potentially disabled set (not - // necessarily the other way around) + // Ensure that every actually disabled validator is also in the potentially disabled set + // (not necessarily the other way around) let disabled = Session::disabled_validators(); for d in disabled.iter() { assert!(potentially_disabled.contains(d)); } // Ensure no more than disabling limit of validators (default 1/3) is disabled - let disabling_limit = pallet_staking::UpToLimitWithReEnablingDisablingStrategy::::disable_limit(active_set_size_before_slash); + let disabling_limit = pallet_staking::UpToLimitWithReEnablingDisablingStrategy::< + SLASHING_DISABLING_FACTOR, + >::disable_limit(active_set_size_before_slash); assert!(disabled.len() == disabling_limit); assert_eq!(pallet_staking::ForceEra::::get(), pallet_staking::Forcing::NotForcing);