From 77cb2e710140af67c31bdfa4a471685f55454f3e Mon Sep 17 00:00:00 2001 From: wigy Date: Fri, 5 Nov 2021 17:08:24 +0100 Subject: [PATCH 1/9] Offence implementations can disable offenders independently from slashing --- frame/offences/src/lib.rs | 1 + frame/offences/src/migration.rs | 8 ++-- frame/offences/src/mock.rs | 1 + frame/staking/src/mock.rs | 8 ++-- frame/staking/src/pallet/impls.rs | 4 +- frame/staking/src/slashing.rs | 63 ++++++++++++++----------------- frame/staking/src/tests.rs | 14 +++++-- primitives/staking/src/offence.rs | 21 +++++++++++ 8 files changed, 75 insertions(+), 45 deletions(-) diff --git a/frame/offences/src/lib.rs b/frame/offences/src/lib.rs index d50bc55f88357..fd6e8dd1cde27 100644 --- a/frame/offences/src/lib.rs +++ b/frame/offences/src/lib.rs @@ -150,6 +150,7 @@ where &concurrent_offenders, &slash_perbill, offence.session_index(), + offence.disable(), ); // Deposit the event. diff --git a/frame/offences/src/migration.rs b/frame/offences/src/migration.rs index b6e32cbe69e26..ec285694fcf8f 100644 --- a/frame/offences/src/migration.rs +++ b/frame/offences/src/migration.rs @@ -19,7 +19,7 @@ use super::{Config, OffenceDetails, Perbill, SessionIndex}; use frame_support::{ generate_storage_alias, pallet_prelude::ValueQuery, traits::Get, weights::Weight, }; -use sp_staking::offence::OnOffenceHandler; +use sp_staking::offence::{DisableStrategy, OnOffenceHandler}; use sp_std::vec::Vec; /// Type of data stored as a deferred offence @@ -27,6 +27,7 @@ type DeferredOffenceOf = ( Vec::AccountId, ::IdentificationTuple>>, Vec, SessionIndex, + DisableStrategy, ); // Deferred reports that have been rejected by the offence handler and need to be submitted @@ -40,8 +41,8 @@ pub fn remove_deferred_storage() -> Weight { let mut weight = T::DbWeight::get().reads_writes(1, 1); let deferred = >::take(); log::info!(target: "runtime::offences", "have {} deferred offences, applying.", deferred.len()); - for (offences, perbill, session) in deferred.iter() { - let consumed = T::OnOffenceHandler::on_offence(&offences, &perbill, *session); + for (offences, perbill, session, disable) in deferred.iter() { + let consumed = T::OnOffenceHandler::on_offence(&offences, &perbill, *session, *disable); weight = weight.saturating_add(consumed); } @@ -78,6 +79,7 @@ mod test { vec![offence_details], vec![Perbill::from_percent(5 + 1 * 100 / 5)], 1, + DisableStrategy::WhenSlashed, )); // when diff --git a/frame/offences/src/mock.rs b/frame/offences/src/mock.rs index 5e4c94944b6fd..cbd94be30dfce 100644 --- a/frame/offences/src/mock.rs +++ b/frame/offences/src/mock.rs @@ -55,6 +55,7 @@ impl offence::OnOffenceHandler _offenders: &[OffenceDetails], slash_fraction: &[Perbill], _offence_session: SessionIndex, + _disable: DisableStrategy, ) -> Weight { ON_OFFENCE_PERBILL.with(|f| { *f.borrow_mut() = slash_fraction.to_vec(); diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 95d397359f8d6..cb2ae95b1ea7d 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -33,7 +33,7 @@ use sp_runtime::{ testing::{Header, TestXt, UintAuthorityId}, traits::{IdentityLookup, Zero}, }; -use sp_staking::offence::{OffenceDetails, OnOffenceHandler}; +use sp_staking::offence::{DisableStrategy, OffenceDetails, OnOffenceHandler}; use std::cell::RefCell; pub const INIT_TIMESTAMP: u64 = 30_000; @@ -764,11 +764,12 @@ pub(crate) fn on_offence_in_era( >], slash_fraction: &[Perbill], era: EraIndex, + disable: DisableStrategy, ) { let bonded_eras = crate::BondedEras::::get(); for &(bonded_era, start_session) in bonded_eras.iter() { if bonded_era == era { - let _ = Staking::on_offence(offenders, slash_fraction, start_session); + let _ = Staking::on_offence(offenders, slash_fraction, start_session, disable); return } else if bonded_era > era { break @@ -780,6 +781,7 @@ pub(crate) fn on_offence_in_era( offenders, slash_fraction, Staking::eras_start_session_index(era).unwrap(), + disable, ); } else { panic!("cannot slash in era {}", era); @@ -794,7 +796,7 @@ pub(crate) fn on_offence_now( slash_fraction: &[Perbill], ) { let now = Staking::active_era().unwrap().index; - on_offence_in_era(offenders, slash_fraction, now) + on_offence_in_era(offenders, slash_fraction, now, DisableStrategy::WhenSlashed) } pub(crate) fn add_slash(who: &AccountId) { diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index ec34efe397f5a..c224b83931d6c 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -36,7 +36,7 @@ use sp_runtime::{ Perbill, }; use sp_staking::{ - offence::{OffenceDetails, OnOffenceHandler}, + offence::{OffenceDetails, OnOffenceHandler, DisableStrategy}, SessionIndex, }; use sp_std::{collections::btree_map::BTreeMap, prelude::*}; @@ -1139,6 +1139,7 @@ where >], slash_fraction: &[Perbill], slash_session: SessionIndex, + disable: DisableStrategy, ) -> Weight { let reward_proportion = SlashRewardFraction::::get(); let mut consumed_weight: Weight = 0; @@ -1208,6 +1209,7 @@ where window_start, now: active_era, reward_proportion, + disable, }); if let Some(mut unapplied) = unapplied { diff --git a/frame/staking/src/slashing.rs b/frame/staking/src/slashing.rs index 68088d0e0d777..49e68d5982a7c 100644 --- a/frame/staking/src/slashing.rs +++ b/frame/staking/src/slashing.rs @@ -63,6 +63,7 @@ use sp_runtime::{ traits::{Saturating, Zero}, DispatchResult, RuntimeDebug, }; +use sp_staking::offence::DisableStrategy; use sp_std::vec::Vec; /// The proportion of the slashing reward to be paid out on the first slashing detection. @@ -213,6 +214,8 @@ pub(crate) struct SlashParams<'a, T: 'a + Config> { /// The maximum percentage of a slash that ever gets paid out. /// This is f_inf in the paper. pub(crate) reward_proportion: Perbill, + /// When to disable offenders immediately + pub(crate) disable: DisableStrategy, } /// Computes a slash of a validator and nominators. It returns an unapplied @@ -224,15 +227,12 @@ pub(crate) struct SlashParams<'a, T: 'a + Config> { pub(crate) fn compute_slash( params: SlashParams, ) -> Option>> { - let SlashParams { stash, slash, exposure, slash_era, window_start, now, reward_proportion } = - params.clone(); - let mut reward_payout = Zero::zero(); let mut val_slashed = Zero::zero(); // is the slash amount here a maximum for the era? - let own_slash = slash * exposure.own; - if slash * exposure.total == Zero::zero() { + let own_slash = params.slash * params.exposure.own; + if params.slash * params.exposure.total == Zero::zero() { // kick out the validator even if they won't be slashed, // as long as the misbehavior is from their most recent slashing span. kick_out_if_recent::(params); @@ -240,13 +240,13 @@ pub(crate) fn compute_slash( } let (prior_slash_p, _era_slash) = - as Store>::ValidatorSlashInEra::get(&slash_era, stash) + as Store>::ValidatorSlashInEra::get(¶ms.slash_era, params.stash) .unwrap_or((Perbill::zero(), Zero::zero())); // compare slash proportions rather than slash values to avoid issues due to rounding // error. - if slash.deconstruct() > prior_slash_p.deconstruct() { - as Store>::ValidatorSlashInEra::insert(&slash_era, stash, &(slash, own_slash)); + if params.slash.deconstruct() > prior_slash_p.deconstruct() { + as Store>::ValidatorSlashInEra::insert(¶ms.slash_era, params.stash, &(params.slash, own_slash)); } else { // we slash based on the max in era - this new event is not the max, // so neither the validator or any nominators will need an update. @@ -261,14 +261,14 @@ pub(crate) fn compute_slash( // apply slash to validator. { let mut spans = fetch_spans::( - stash, - window_start, + params.stash, + params.window_start, &mut reward_payout, &mut val_slashed, - reward_proportion, + params.reward_proportion, ); - let target_span = spans.compare_and_update_span_slash(slash_era, own_slash); + let target_span = spans.compare_and_update_span_slash(params.slash_era, own_slash); if target_span == Some(spans.span_index()) { // misbehavior occurred within the current slashing span - take appropriate @@ -276,20 +276,19 @@ pub(crate) fn compute_slash( // chill the validator - it misbehaved in the current span and should // not continue in the next election. also end the slashing span. - spans.end_span(now); - >::chill_stash(stash); + spans.end_span(params.now); + >::chill_stash(params.stash); } } - // add the validator to the offenders list and make sure it is disabled for - // the duration of the era - add_offending_validator::(params.stash, true); + let disable_when_slashed = ! matches!(params.disable, DisableStrategy::Never); + add_offending_validator::(params.stash, disable_when_slashed); let mut nominators_slashed = Vec::new(); - reward_payout += slash_nominators::(params, prior_slash_p, &mut nominators_slashed); + reward_payout += slash_nominators::(params.clone(), prior_slash_p, &mut nominators_slashed); Some(UnappliedSlash { - validator: stash.clone(), + validator: params.stash.clone(), own: val_slashed, others: nominators_slashed, reporters: Vec::new(), @@ -316,9 +315,8 @@ fn kick_out_if_recent(params: SlashParams) { >::chill_stash(params.stash); } - // add the validator to the offenders list but since there's no slash being - // applied there's no need to disable the validator - add_offending_validator::(params.stash, false); + let disable_without_slash = matches!(params.disable, DisableStrategy::Always); + add_offending_validator::(params.stash, disable_without_slash); } /// Add the given validator to the offenders list and optionally disable it. @@ -371,13 +369,10 @@ fn slash_nominators( prior_slash_p: Perbill, nominators_slashed: &mut Vec<(T::AccountId, BalanceOf)>, ) -> BalanceOf { - let SlashParams { stash: _, slash, exposure, slash_era, window_start, now, reward_proportion } = - params; - let mut reward_payout = Zero::zero(); - nominators_slashed.reserve(exposure.others.len()); - for nominator in &exposure.others { + nominators_slashed.reserve(params.exposure.others.len()); + for nominator in ¶ms.exposure.others { let stash = &nominator.who; let mut nom_slashed = Zero::zero(); @@ -385,15 +380,15 @@ fn slash_nominators( // had a new max slash for the era. let era_slash = { let own_slash_prior = prior_slash_p * nominator.value; - let own_slash_by_validator = slash * nominator.value; + let own_slash_by_validator = params.slash * nominator.value; let own_slash_difference = own_slash_by_validator.saturating_sub(own_slash_prior); - let mut era_slash = as Store>::NominatorSlashInEra::get(&slash_era, stash) + let mut era_slash = as Store>::NominatorSlashInEra::get(¶ms.slash_era, stash) .unwrap_or_else(|| Zero::zero()); era_slash += own_slash_difference; - as Store>::NominatorSlashInEra::insert(&slash_era, stash, &era_slash); + as Store>::NominatorSlashInEra::insert(¶ms.slash_era, stash, &era_slash); era_slash }; @@ -402,18 +397,18 @@ fn slash_nominators( { let mut spans = fetch_spans::( stash, - window_start, + params.window_start, &mut reward_payout, &mut nom_slashed, - reward_proportion, + params.reward_proportion, ); - let target_span = spans.compare_and_update_span_slash(slash_era, era_slash); + let target_span = spans.compare_and_update_span_slash(params.slash_era, era_slash); if target_span == Some(spans.span_index()) { // End the span, but don't chill the nominator. its nomination // on this validator will be ignored in the future. - spans.end_span(now); + spans.end_span(params.now); } } diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index d6d92d5bd57fc..968d95672a359 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -34,7 +34,7 @@ use sp_runtime::{ Perbill, Percent, }; use sp_staking::{ - offence::{OffenceDetails, OnOffenceHandler}, + offence::{OffenceDetails, OnOffenceHandler, DisableStrategy}, SessionIndex, }; use sp_std::prelude::*; @@ -2316,6 +2316,7 @@ fn slash_in_old_span_does_not_deselect() { }], &[Perbill::from_percent(0)], 1, + DisableStrategy::WhenSlashed, ); // the validator doesn't get chilled again @@ -2332,6 +2333,7 @@ fn slash_in_old_span_does_not_deselect() { // NOTE: A 100% slash here would clean up the account, causing de-registration. &[Perbill::from_percent(95)], 1, + DisableStrategy::WhenSlashed, ); // the validator doesn't get chilled again @@ -2628,6 +2630,7 @@ fn slashing_nominators_by_span_max() { }], &[Perbill::from_percent(10)], 2, + DisableStrategy::WhenSlashed, ); assert_eq!(Balances::free_balance(11), 900); @@ -2654,6 +2657,7 @@ fn slashing_nominators_by_span_max() { }], &[Perbill::from_percent(30)], 3, + DisableStrategy::WhenSlashed, ); // 11 was not further slashed, but 21 and 101 were. @@ -2675,6 +2679,7 @@ fn slashing_nominators_by_span_max() { }], &[Perbill::from_percent(20)], 2, + DisableStrategy::WhenSlashed, ); // 11 was further slashed, but 21 and 101 were not. @@ -2810,6 +2815,7 @@ fn remove_deferred() { &[OffenceDetails { offender: (11, exposure.clone()), reporters: vec![] }], &[Perbill::from_percent(15)], 1, + DisableStrategy::WhenSlashed, ); // fails if empty @@ -3661,7 +3667,7 @@ fn offences_weight_calculated_correctly() { ExtBuilder::default().nominate(true).build_and_execute(|| { // On offence with zero offenders: 4 Reads, 1 Write let zero_offence_weight = ::DbWeight::get().reads_writes(4, 1); - assert_eq!(Staking::on_offence(&[], &[Perbill::from_percent(50)], 0), zero_offence_weight); + assert_eq!(Staking::on_offence(&[], &[Perbill::from_percent(50)], 0, DisableStrategy::WhenSlashed), zero_offence_weight); // On Offence with N offenders, Unapplied: 4 Reads, 1 Write + 4 Reads, 5 Writes let n_offence_unapplied_weight = ::DbWeight::get().reads_writes(4, 1) @@ -3674,7 +3680,7 @@ fn offences_weight_calculated_correctly() { reporters: vec![], } ).collect(); - assert_eq!(Staking::on_offence(&offenders, &[Perbill::from_percent(50)], 0), n_offence_unapplied_weight); + assert_eq!(Staking::on_offence(&offenders, &[Perbill::from_percent(50)], 0, DisableStrategy::WhenSlashed), n_offence_unapplied_weight); // On Offence with one offenders, Applied let one_offender = [ @@ -3695,7 +3701,7 @@ fn offences_weight_calculated_correctly() { // `reward_cost` * reporters (1) + ::DbWeight::get().reads_writes(2, 2); - assert_eq!(Staking::on_offence(&one_offender, &[Perbill::from_percent(50)], 0), one_offence_unapplied_weight); + assert_eq!(Staking::on_offence(&one_offender, &[Perbill::from_percent(50)], 0, DisableStrategy::WhenSlashed), one_offence_unapplied_weight); }); } diff --git a/primitives/staking/src/offence.rs b/primitives/staking/src/offence.rs index a91cb47c117b6..8c315fdc60ffc 100644 --- a/primitives/staking/src/offence.rs +++ b/primitives/staking/src/offence.rs @@ -37,6 +37,17 @@ pub type Kind = [u8; 16]; /// so that we can slash it accordingly. pub type OffenceCount = u32; +/// In case of an offence, which conditions get an offender validator disabled +#[derive(Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Encode, Decode, sp_runtime::RuntimeDebug, scale_info::TypeInfo)] +pub enum DisableStrategy { + /// Independently of slashing, this offence will not disable the offender + Never, + /// Only disable the offender if it is also slashed + WhenSlashed, + /// Independently of slashing, this offence will disable the offender + Always, +} + /// A trait implemented by an offence report. /// /// This trait assumes that the offence is legitimate and was validated already. @@ -79,6 +90,12 @@ pub trait Offence { /// number. Note that for GRANDPA the round number is reset each epoch. fn time_slot(&self) -> Self::TimeSlot; + /// Whether this offense needs to disable offenders until the next era starts. + /// + /// In previous versions any slashed offenders got disabled in every offence. Now each offence + /// can decide whether to disable them regardless of the slashed amount. + fn disable(&self) -> DisableStrategy { DisableStrategy::WhenSlashed } + /// A slash fraction of the total exposure that should be slashed for this /// particular offence kind for the given parameters that happened at a singular `TimeSlot`. /// @@ -149,6 +166,8 @@ pub trait OnOffenceHandler { /// Zero is a valid value for a fraction. /// /// The `session` parameter is the session index of the offence. + /// + /// The `disable` parameter decides if the offenders need to be disabled immediately. /// /// The receiver might decide to not accept this offence. In this case, the call site is /// responsible for queuing the report and re-submitting again. @@ -156,6 +175,7 @@ pub trait OnOffenceHandler { offenders: &[OffenceDetails], slash_fraction: &[Perbill], session: SessionIndex, + disable: DisableStrategy, ) -> Res; } @@ -164,6 +184,7 @@ impl OnOffenceHandler _offenders: &[OffenceDetails], _slash_fraction: &[Perbill], _session: SessionIndex, + _disable: DisableStrategy, ) -> Res { Default::default() } From b3833f853036942fecabd4013175132f7010dc3c Mon Sep 17 00:00:00 2001 From: wigy Date: Fri, 5 Nov 2021 17:36:36 +0100 Subject: [PATCH 2/9] Fix build on CI --- frame/offences/src/mock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/offences/src/mock.rs b/frame/offences/src/mock.rs index cbd94be30dfce..160fe2e506069 100644 --- a/frame/offences/src/mock.rs +++ b/frame/offences/src/mock.rs @@ -36,7 +36,7 @@ use sp_runtime::{ Perbill, }; use sp_staking::{ - offence::{self, Kind, OffenceDetails}, + offence::{self, Kind, OffenceDetails, DisableStrategy}, SessionIndex, }; use std::cell::RefCell; From 2be6c06471779afec8b2a8da9e7c0e677b19d164 Mon Sep 17 00:00:00 2001 From: wigy Date: Fri, 5 Nov 2021 20:33:07 +0100 Subject: [PATCH 3/9] Run cargo fmt --- frame/offences/src/mock.rs | 2 +- frame/staking/src/pallet/impls.rs | 2 +- frame/staking/src/slashing.rs | 13 +++++++++---- frame/staking/src/tests.rs | 2 +- primitives/staking/src/offence.rs | 20 +++++++++++++++++--- 5 files changed, 29 insertions(+), 10 deletions(-) diff --git a/frame/offences/src/mock.rs b/frame/offences/src/mock.rs index 160fe2e506069..f591721412768 100644 --- a/frame/offences/src/mock.rs +++ b/frame/offences/src/mock.rs @@ -36,7 +36,7 @@ use sp_runtime::{ Perbill, }; use sp_staking::{ - offence::{self, Kind, OffenceDetails, DisableStrategy}, + offence::{self, DisableStrategy, Kind, OffenceDetails}, SessionIndex, }; use std::cell::RefCell; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index c224b83931d6c..696173f4d2b08 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -36,7 +36,7 @@ use sp_runtime::{ Perbill, }; use sp_staking::{ - offence::{OffenceDetails, OnOffenceHandler, DisableStrategy}, + offence::{DisableStrategy, OffenceDetails, OnOffenceHandler}, SessionIndex, }; use sp_std::{collections::btree_map::BTreeMap, prelude::*}; diff --git a/frame/staking/src/slashing.rs b/frame/staking/src/slashing.rs index 49e68d5982a7c..520a5f4a7cd4f 100644 --- a/frame/staking/src/slashing.rs +++ b/frame/staking/src/slashing.rs @@ -246,7 +246,11 @@ pub(crate) fn compute_slash( // compare slash proportions rather than slash values to avoid issues due to rounding // error. if params.slash.deconstruct() > prior_slash_p.deconstruct() { - as Store>::ValidatorSlashInEra::insert(¶ms.slash_era, params.stash, &(params.slash, own_slash)); + as Store>::ValidatorSlashInEra::insert( + ¶ms.slash_era, + params.stash, + &(params.slash, own_slash), + ); } else { // we slash based on the max in era - this new event is not the max, // so neither the validator or any nominators will need an update. @@ -281,7 +285,7 @@ pub(crate) fn compute_slash( } } - let disable_when_slashed = ! matches!(params.disable, DisableStrategy::Never); + let disable_when_slashed = !matches!(params.disable, DisableStrategy::Never); add_offending_validator::(params.stash, disable_when_slashed); let mut nominators_slashed = Vec::new(); @@ -383,8 +387,9 @@ fn slash_nominators( let own_slash_by_validator = params.slash * nominator.value; let own_slash_difference = own_slash_by_validator.saturating_sub(own_slash_prior); - let mut era_slash = as Store>::NominatorSlashInEra::get(¶ms.slash_era, stash) - .unwrap_or_else(|| Zero::zero()); + let mut era_slash = + as Store>::NominatorSlashInEra::get(¶ms.slash_era, stash) + .unwrap_or_else(|| Zero::zero()); era_slash += own_slash_difference; diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 968d95672a359..380474da8458e 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -34,7 +34,7 @@ use sp_runtime::{ Perbill, Percent, }; use sp_staking::{ - offence::{OffenceDetails, OnOffenceHandler, DisableStrategy}, + offence::{DisableStrategy, OffenceDetails, OnOffenceHandler}, SessionIndex, }; use sp_std::prelude::*; diff --git a/primitives/staking/src/offence.rs b/primitives/staking/src/offence.rs index 8c315fdc60ffc..0148974dae47b 100644 --- a/primitives/staking/src/offence.rs +++ b/primitives/staking/src/offence.rs @@ -38,7 +38,19 @@ pub type Kind = [u8; 16]; pub type OffenceCount = u32; /// In case of an offence, which conditions get an offender validator disabled -#[derive(Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Encode, Decode, sp_runtime::RuntimeDebug, scale_info::TypeInfo)] +#[derive( + Clone, + Copy, + PartialEq, + Eq, + Hash, + PartialOrd, + Ord, + Encode, + Decode, + sp_runtime::RuntimeDebug, + scale_info::TypeInfo, +)] pub enum DisableStrategy { /// Independently of slashing, this offence will not disable the offender Never, @@ -94,7 +106,9 @@ pub trait Offence { /// /// In previous versions any slashed offenders got disabled in every offence. Now each offence /// can decide whether to disable them regardless of the slashed amount. - fn disable(&self) -> DisableStrategy { DisableStrategy::WhenSlashed } + fn disable(&self) -> DisableStrategy { + DisableStrategy::WhenSlashed + } /// A slash fraction of the total exposure that should be slashed for this /// particular offence kind for the given parameters that happened at a singular `TimeSlot`. @@ -166,7 +180,7 @@ pub trait OnOffenceHandler { /// Zero is a valid value for a fraction. /// /// The `session` parameter is the session index of the offence. - /// + /// /// The `disable` parameter decides if the offenders need to be disabled immediately. /// /// The receiver might decide to not accept this offence. In this case, the call site is From 6db4e1221dd1527f0e6774247a965f489d68d990 Mon Sep 17 00:00:00 2001 From: wigy Date: Sat, 6 Nov 2021 01:09:13 +0100 Subject: [PATCH 4/9] Fixes based on review comments --- frame/offences/src/lib.rs | 2 +- frame/offences/src/mock.rs | 2 +- frame/staking/src/pallet/impls.rs | 4 ++-- frame/staking/src/slashing.rs | 8 ++++---- primitives/staking/src/offence.rs | 11 ++++------- 5 files changed, 12 insertions(+), 15 deletions(-) diff --git a/frame/offences/src/lib.rs b/frame/offences/src/lib.rs index fd6e8dd1cde27..0ad6ea926c400 100644 --- a/frame/offences/src/lib.rs +++ b/frame/offences/src/lib.rs @@ -150,7 +150,7 @@ where &concurrent_offenders, &slash_perbill, offence.session_index(), - offence.disable(), + offence.disable_strategy(), ); // Deposit the event. diff --git a/frame/offences/src/mock.rs b/frame/offences/src/mock.rs index f591721412768..bce51f527abc6 100644 --- a/frame/offences/src/mock.rs +++ b/frame/offences/src/mock.rs @@ -55,7 +55,7 @@ impl offence::OnOffenceHandler _offenders: &[OffenceDetails], slash_fraction: &[Perbill], _offence_session: SessionIndex, - _disable: DisableStrategy, + _disable_strategy: DisableStrategy, ) -> Weight { ON_OFFENCE_PERBILL.with(|f| { *f.borrow_mut() = slash_fraction.to_vec(); diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 696173f4d2b08..38023bf7f7fc8 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1139,7 +1139,7 @@ where >], slash_fraction: &[Perbill], slash_session: SessionIndex, - disable: DisableStrategy, + disable_strategy: DisableStrategy, ) -> Weight { let reward_proportion = SlashRewardFraction::::get(); let mut consumed_weight: Weight = 0; @@ -1209,7 +1209,7 @@ where window_start, now: active_era, reward_proportion, - disable, + disable_strategy, }); if let Some(mut unapplied) = unapplied { diff --git a/frame/staking/src/slashing.rs b/frame/staking/src/slashing.rs index 520a5f4a7cd4f..13e414f7a5232 100644 --- a/frame/staking/src/slashing.rs +++ b/frame/staking/src/slashing.rs @@ -214,8 +214,8 @@ pub(crate) struct SlashParams<'a, T: 'a + Config> { /// The maximum percentage of a slash that ever gets paid out. /// This is f_inf in the paper. pub(crate) reward_proportion: Perbill, - /// When to disable offenders immediately - pub(crate) disable: DisableStrategy, + /// When to disable offenders. + pub(crate) disable_strategy: DisableStrategy, } /// Computes a slash of a validator and nominators. It returns an unapplied @@ -285,7 +285,7 @@ pub(crate) fn compute_slash( } } - let disable_when_slashed = !matches!(params.disable, DisableStrategy::Never); + let disable_when_slashed = params.disable_strategy != DisableStrategy::Never; add_offending_validator::(params.stash, disable_when_slashed); let mut nominators_slashed = Vec::new(); @@ -319,7 +319,7 @@ fn kick_out_if_recent(params: SlashParams) { >::chill_stash(params.stash); } - let disable_without_slash = matches!(params.disable, DisableStrategy::Always); + let disable_without_slash = params.disable_strategy == DisableStrategy::Always; add_offending_validator::(params.stash, disable_without_slash); } diff --git a/primitives/staking/src/offence.rs b/primitives/staking/src/offence.rs index 0148974dae47b..c3937b775982e 100644 --- a/primitives/staking/src/offence.rs +++ b/primitives/staking/src/offence.rs @@ -102,11 +102,8 @@ pub trait Offence { /// number. Note that for GRANDPA the round number is reset each epoch. fn time_slot(&self) -> Self::TimeSlot; - /// Whether this offense needs to disable offenders until the next era starts. - /// - /// In previous versions any slashed offenders got disabled in every offence. Now each offence - /// can decide whether to disable them regardless of the slashed amount. - fn disable(&self) -> DisableStrategy { + /// In which cases this offence needs to disable offenders until the next era starts. + fn disable_strategy(&self) -> DisableStrategy { DisableStrategy::WhenSlashed } @@ -189,7 +186,7 @@ pub trait OnOffenceHandler { offenders: &[OffenceDetails], slash_fraction: &[Perbill], session: SessionIndex, - disable: DisableStrategy, + disable_strategy: DisableStrategy, ) -> Res; } @@ -198,7 +195,7 @@ impl OnOffenceHandler _offenders: &[OffenceDetails], _slash_fraction: &[Perbill], _session: SessionIndex, - _disable: DisableStrategy, + _disable_strategy: DisableStrategy, ) -> Res { Default::default() } From 9266b4eae7b39d62e86943e437a2317424ce56ad Mon Sep 17 00:00:00 2001 From: wigy Date: Mon, 8 Nov 2021 11:21:23 +0100 Subject: [PATCH 5/9] Make parameter naming consistent --- frame/offences/src/migration.rs | 5 +++-- frame/staking/src/mock.rs | 6 +++--- primitives/staking/src/offence.rs | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/frame/offences/src/migration.rs b/frame/offences/src/migration.rs index ec285694fcf8f..8056f73c4e37e 100644 --- a/frame/offences/src/migration.rs +++ b/frame/offences/src/migration.rs @@ -41,8 +41,9 @@ pub fn remove_deferred_storage() -> Weight { let mut weight = T::DbWeight::get().reads_writes(1, 1); let deferred = >::take(); log::info!(target: "runtime::offences", "have {} deferred offences, applying.", deferred.len()); - for (offences, perbill, session, disable) in deferred.iter() { - let consumed = T::OnOffenceHandler::on_offence(&offences, &perbill, *session, *disable); + for (offences, perbill, session, disable_strategy) in deferred.iter() { + let consumed = + T::OnOffenceHandler::on_offence(&offences, &perbill, *session, *disable_strategy); weight = weight.saturating_add(consumed); } diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index cb2ae95b1ea7d..4c95ebf0161d9 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -764,12 +764,12 @@ pub(crate) fn on_offence_in_era( >], slash_fraction: &[Perbill], era: EraIndex, - disable: DisableStrategy, + disable_strategy: DisableStrategy, ) { let bonded_eras = crate::BondedEras::::get(); for &(bonded_era, start_session) in bonded_eras.iter() { if bonded_era == era { - let _ = Staking::on_offence(offenders, slash_fraction, start_session, disable); + let _ = Staking::on_offence(offenders, slash_fraction, start_session, disable_strategy); return } else if bonded_era > era { break @@ -781,7 +781,7 @@ pub(crate) fn on_offence_in_era( offenders, slash_fraction, Staking::eras_start_session_index(era).unwrap(), - disable, + disable_strategy, ); } else { panic!("cannot slash in era {}", era); diff --git a/primitives/staking/src/offence.rs b/primitives/staking/src/offence.rs index c3937b775982e..2bc2d5c792531 100644 --- a/primitives/staking/src/offence.rs +++ b/primitives/staking/src/offence.rs @@ -178,7 +178,7 @@ pub trait OnOffenceHandler { /// /// The `session` parameter is the session index of the offence. /// - /// The `disable` parameter decides if the offenders need to be disabled immediately. + /// The `disable_strategy` parameter decides if the offenders need to be disabled immediately. /// /// The receiver might decide to not accept this offence. In this case, the call site is /// responsible for queuing the report and re-submitting again. From a078f4d84cc03cd0052935f07dc220957e63dd13 Mon Sep 17 00:00:00 2001 From: wigy Date: Mon, 8 Nov 2021 12:37:01 +0100 Subject: [PATCH 6/9] Fix migration and some English --- frame/offences/src/migration.rs | 11 +++++++---- primitives/staking/src/offence.rs | 8 ++++---- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/frame/offences/src/migration.rs b/frame/offences/src/migration.rs index 8056f73c4e37e..d15a6a66f29c5 100644 --- a/frame/offences/src/migration.rs +++ b/frame/offences/src/migration.rs @@ -27,7 +27,6 @@ type DeferredOffenceOf = ( Vec::AccountId, ::IdentificationTuple>>, Vec, SessionIndex, - DisableStrategy, ); // Deferred reports that have been rejected by the offence handler and need to be submitted @@ -41,9 +40,13 @@ pub fn remove_deferred_storage() -> Weight { let mut weight = T::DbWeight::get().reads_writes(1, 1); let deferred = >::take(); log::info!(target: "runtime::offences", "have {} deferred offences, applying.", deferred.len()); - for (offences, perbill, session, disable_strategy) in deferred.iter() { - let consumed = - T::OnOffenceHandler::on_offence(&offences, &perbill, *session, *disable_strategy); + for (offences, perbill, session) in deferred.iter() { + let consumed = T::OnOffenceHandler::on_offence( + &offences, + &perbill, + *session, + DisableStrategy::WhenSlashed, + ); weight = weight.saturating_add(consumed); } diff --git a/primitives/staking/src/offence.rs b/primitives/staking/src/offence.rs index 2bc2d5c792531..fdff02d42065e 100644 --- a/primitives/staking/src/offence.rs +++ b/primitives/staking/src/offence.rs @@ -37,7 +37,7 @@ pub type Kind = [u8; 16]; /// so that we can slash it accordingly. pub type OffenceCount = u32; -/// In case of an offence, which conditions get an offender validator disabled +/// In case of an offence, which conditions get an offending validator disabled. #[derive( Clone, Copy, @@ -52,11 +52,11 @@ pub type OffenceCount = u32; scale_info::TypeInfo, )] pub enum DisableStrategy { - /// Independently of slashing, this offence will not disable the offender + /// Independently of slashing, this offence will not disable the offender. Never, - /// Only disable the offender if it is also slashed + /// Only disable the offender if it is also slashed. WhenSlashed, - /// Independently of slashing, this offence will disable the offender + /// Independently of slashing, this offence will always disable the offender. Always, } From 7525ed547b48cd25e447e17989ef9238b348f528 Mon Sep 17 00:00:00 2001 From: wigy Date: Mon, 8 Nov 2021 15:55:04 +0100 Subject: [PATCH 7/9] Fix migration - again --- frame/offences/src/migration.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/offences/src/migration.rs b/frame/offences/src/migration.rs index d15a6a66f29c5..e3b2c170f473a 100644 --- a/frame/offences/src/migration.rs +++ b/frame/offences/src/migration.rs @@ -82,8 +82,7 @@ mod test { >::append(( vec![offence_details], vec![Perbill::from_percent(5 + 1 * 100 / 5)], - 1, - DisableStrategy::WhenSlashed, + 1 )); // when From e3e5f38369d1a711907420ef5ce53c104f38a4fd Mon Sep 17 00:00:00 2001 From: wigy Date: Mon, 8 Nov 2021 16:23:45 +0100 Subject: [PATCH 8/9] cargo fmt --- frame/offences/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/offences/src/migration.rs b/frame/offences/src/migration.rs index e3b2c170f473a..d655f2cec539a 100644 --- a/frame/offences/src/migration.rs +++ b/frame/offences/src/migration.rs @@ -82,7 +82,7 @@ mod test { >::append(( vec![offence_details], vec![Perbill::from_percent(5 + 1 * 100 / 5)], - 1 + 1, )); // when From cd353c8210990561b73a1409b225c0020044cefe Mon Sep 17 00:00:00 2001 From: wigy Date: Wed, 17 Nov 2021 13:01:42 +0100 Subject: [PATCH 9/9] Cover 2 new cases with a test --- frame/staking/src/tests.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/frame/staking/src/tests.rs b/frame/staking/src/tests.rs index 5e32897021f8e..f8f37bed0066c 100644 --- a/frame/staking/src/tests.rs +++ b/frame/staking/src/tests.rs @@ -2939,6 +2939,40 @@ fn non_slashable_offence_doesnt_disable_validator() { }); } +#[test] +fn slashing_independent_of_disabling_validator() { + ExtBuilder::default().build_and_execute(|| { + mock::start_active_era(1); + assert_eq_uvec!(Session::validators(), vec![11, 21]); + + 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 now = Staking::active_era().unwrap().index; + + // offence with no slash associated, BUT disabling + on_offence_in_era( + &[OffenceDetails { offender: (11, exposure_11.clone()), reporters: vec![] }], + &[Perbill::zero()], + now, + DisableStrategy::Always, + ); + + // offence that slashes 25% of the bond, BUT not disabling + on_offence_in_era( + &[OffenceDetails { offender: (21, exposure_21.clone()), reporters: vec![] }], + &[Perbill::from_percent(25)], + now, + DisableStrategy::Never, + ); + + // the offence for validator 10 was explicitly disabled + assert!(is_disabled(10)); + // whereas validator 20 is explicitly not disabled + assert!(!is_disabled(20)); + }); +} + #[test] fn offence_threshold_triggers_new_era() { ExtBuilder::default()