diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 2f8e393aa4..e484e84d43 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -584,10 +584,15 @@ impl pallet_im_online::Trait for Runtime { type UnsignedPriority = ImOnlineUnsignedPriority; } +parameter_types! { + pub const OffencesWeightSoftLimit: Weight = Perbill::from_percent(60) * MaximumBlockWeight::get(); +} + impl pallet_offences::Trait for Runtime { type Event = Event; type IdentificationTuple = pallet_session::historical::IdentificationTuple; type OnOffenceHandler = Staking; + type WeightSoftLimit = OffencesWeightSoftLimit; } impl pallet_authority_discovery::Trait for Runtime {} diff --git a/substrate/frame/grandpa/src/mock.rs b/substrate/frame/grandpa/src/mock.rs index 2b9b25eee6..e429212cef 100644 --- a/substrate/frame/grandpa/src/mock.rs +++ b/substrate/frame/grandpa/src/mock.rs @@ -231,10 +231,15 @@ impl staking::Trait for Test { type MaxIterations = (); } +parameter_types! { + pub const OffencesWeightSoftLimit: Weight = Perbill::from_percent(60) * MaximumBlockWeight::get(); +} + impl offences::Trait for Test { type Event = TestEvent; type IdentificationTuple = session::historical::IdentificationTuple; type OnOffenceHandler = Staking; + type WeightSoftLimit = OffencesWeightSoftLimit; } impl Trait for Test { diff --git a/substrate/frame/offences/benchmarking/src/mock.rs b/substrate/frame/offences/benchmarking/src/mock.rs index fa6e247abd..15b46fc194 100644 --- a/substrate/frame/offences/benchmarking/src/mock.rs +++ b/substrate/frame/offences/benchmarking/src/mock.rs @@ -20,7 +20,10 @@ #![cfg(test)] use super::*; -use frame_support::parameter_types; +use frame_support::{ + parameter_types, + weights::{Weight, constants::WEIGHT_PER_SECOND}, +}; use frame_system as system; use sp_runtime::{ SaturatedConversion, @@ -34,6 +37,10 @@ type AccountIndex = u32; type BlockNumber = u64; type Balance = u64; +parameter_types! { + pub const MaximumBlockWeight: Weight = 2 * WEIGHT_PER_SECOND; +} + impl frame_system::Trait for Test { type Origin = Origin; type Index = AccountIndex; @@ -46,7 +53,7 @@ impl frame_system::Trait for Test { type Header = sp_runtime::testing::Header; type Event = Event; type BlockHashCount = (); - type MaximumBlockWeight = (); + type MaximumBlockWeight = MaximumBlockWeight; type DbWeight = (); type AvailableBlockRatio = (); type MaximumBlockLength = (); @@ -179,10 +186,15 @@ impl pallet_im_online::Trait for Test { type UnsignedPriority = (); } +parameter_types! { + pub const OffencesWeightSoftLimit: Weight = Perbill::from_percent(60) * MaximumBlockWeight::get(); +} + impl pallet_offences::Trait for Test { type Event = Event; type IdentificationTuple = pallet_session::historical::IdentificationTuple; type OnOffenceHandler = Staking; + type WeightSoftLimit = OffencesWeightSoftLimit; } impl frame_system::offchain::SendTransactionTypes for Test where Call: From { diff --git a/substrate/frame/offences/src/lib.rs b/substrate/frame/offences/src/lib.rs index dd1b052811..a42f09697e 100644 --- a/substrate/frame/offences/src/lib.rs +++ b/substrate/frame/offences/src/lib.rs @@ -28,9 +28,10 @@ mod tests; use sp_std::vec::Vec; use frame_support::{ decl_module, decl_event, decl_storage, Parameter, debug, + traits::Get, weights::Weight, }; -use sp_runtime::{traits::Hash, Perbill}; +use sp_runtime::{traits::{Hash, Zero}, Perbill}; use sp_staking::{ SessionIndex, offence::{Offence, ReportOffence, Kind, OnOffenceHandler, OffenceDetails, OffenceError}, @@ -58,7 +59,11 @@ pub trait Trait: frame_system::Trait { /// Full identification of the validator. type IdentificationTuple: Parameter + Ord; /// A handler called for every offence report. - type OnOffenceHandler: OnOffenceHandler; + type OnOffenceHandler: OnOffenceHandler; + /// The a soft limit on maximum weight that may be consumed while dispatching deferred offences in + /// `on_initialize`. + /// Note it's going to be exceeded before we stop adding to it, so it has to be set conservatively. + type WeightSoftLimit: Get; } decl_storage! { @@ -102,23 +107,39 @@ decl_module! { fn on_initialize(now: T::BlockNumber) -> Weight { // only decode storage if we can actually submit anything again. - if T::OnOffenceHandler::can_report() { - >::mutate(|deferred| { - // keep those that fail to be reported again. An error log is emitted here; this - // should not happen if staking's `can_report` is implemented properly. - deferred.retain(|(o, p, s)| { - T::OnOffenceHandler::on_offence(&o, &p, *s).map_err(|_| { - debug::native::error!( - target: "pallet-offences", - "re-submitting a deferred slash returned Err at {}. This should not happen with pallet-staking", - now, - ); - }).is_err() - }) - }) + if !T::OnOffenceHandler::can_report() { + return 0; } - 0 + let limit = T::WeightSoftLimit::get(); + let mut consumed = Weight::zero(); + + >::mutate(|deferred| { + deferred.retain(|(offences, perbill, session)| { + if consumed >= limit { + true + } else { + // keep those that fail to be reported again. An error log is emitted here; this + // should not happen if staking's `can_report` is implemented properly. + match T::OnOffenceHandler::on_offence(&offences, &perbill, *session) { + Ok(weight) => { + consumed += weight; + false + }, + Err(_) => { + debug::native::error!( + target: "pallet-offences", + "re-submitting a deferred slash returned Err at {}. This should not happen with pallet-staking", + now, + ); + true + }, + } + } + }) + }); + + consumed } } } diff --git a/substrate/frame/offences/src/mock.rs b/substrate/frame/offences/src/mock.rs index 0f5036edc5..b3f35e0171 100644 --- a/substrate/frame/offences/src/mock.rs +++ b/substrate/frame/offences/src/mock.rs @@ -32,7 +32,7 @@ use sp_runtime::traits::{IdentityLookup, BlakeTwo256}; use sp_core::H256; use frame_support::{ impl_outer_origin, impl_outer_event, parameter_types, StorageMap, StorageDoubleMap, - weights::Weight, + weights::{Weight, constants::{WEIGHT_PER_SECOND, RocksDbWeight}}, }; use frame_system as system; @@ -45,20 +45,23 @@ pub struct OnOffenceHandler; thread_local! { pub static ON_OFFENCE_PERBILL: RefCell> = RefCell::new(Default::default()); pub static CAN_REPORT: RefCell = RefCell::new(true); + pub static OFFENCE_WEIGHT: RefCell = RefCell::new(Default::default()); } -impl offence::OnOffenceHandler for OnOffenceHandler { +impl + offence::OnOffenceHandler for OnOffenceHandler +{ fn on_offence( _offenders: &[OffenceDetails], slash_fraction: &[Perbill], _offence_session: SessionIndex, - ) -> Result<(), ()> { - if >::can_report() { + ) -> Result { + if >::can_report() { ON_OFFENCE_PERBILL.with(|f| { *f.borrow_mut() = slash_fraction.to_vec(); }); - Ok(()) + Ok(OFFENCE_WEIGHT.with(|w| *w.borrow())) } else { Err(()) } @@ -79,12 +82,16 @@ pub fn with_on_offence_fractions) -> R>(f: F) -> }) } +pub fn set_offence_weight(new: Weight) { + OFFENCE_WEIGHT.with(|w| *w.borrow_mut() = new); +} + // Workaround for https://github.com/rust-lang/rust/issues/26925 . Remove when sorted. #[derive(Clone, PartialEq, Eq, Debug)] pub struct Runtime; parameter_types! { pub const BlockHashCount: u64 = 250; - pub const MaximumBlockWeight: Weight = 1024; + pub const MaximumBlockWeight: Weight = 2 * WEIGHT_PER_SECOND; pub const MaximumBlockLength: u32 = 2 * 1024; pub const AvailableBlockRatio: Perbill = Perbill::one(); } @@ -101,7 +108,7 @@ impl frame_system::Trait for Runtime { type Event = TestEvent; type BlockHashCount = BlockHashCount; type MaximumBlockWeight = MaximumBlockWeight; - type DbWeight = (); + type DbWeight = RocksDbWeight; type BlockExecutionWeight = (); type ExtrinsicBaseWeight = (); type MaximumExtrinsicWeight = MaximumBlockWeight; @@ -114,10 +121,15 @@ impl frame_system::Trait for Runtime { type OnKilledAccount = (); } +parameter_types! { + pub const OffencesWeightSoftLimit: Weight = Perbill::from_percent(60) * MaximumBlockWeight::get(); +} + impl Trait for Runtime { type Event = TestEvent; type IdentificationTuple = u64; type OnOffenceHandler = OnOffenceHandler; + type WeightSoftLimit = OffencesWeightSoftLimit; } mod offences { diff --git a/substrate/frame/offences/src/tests.rs b/substrate/frame/offences/src/tests.rs index b05fee1790..0fb6620b7d 100644 --- a/substrate/frame/offences/src/tests.rs +++ b/substrate/frame/offences/src/tests.rs @@ -22,7 +22,7 @@ use super::*; use crate::mock::{ Offences, System, Offence, TestEvent, KIND, new_test_ext, with_on_offence_fractions, - offence_reports, set_can_report, + offence_reports, set_can_report, set_offence_weight, }; use sp_runtime::Perbill; use frame_support::traits::OnInitialize; @@ -265,3 +265,48 @@ fn should_queue_and_resubmit_rejected_offence() { assert_eq!(Offences::deferred_offences().len(), 0); }) } + +#[test] +fn weight_soft_limit_is_used() { + new_test_ext().execute_with(|| { + set_can_report(false); + // Only 2 can fit in one block + set_offence_weight(::WeightSoftLimit::get() / 2); + + // Queue 3 offences + // #1 + let offence = Offence { + validator_set_count: 5, + time_slot: 42, + offenders: vec![5], + }; + Offences::report_offence(vec![], offence).unwrap(); + // #2 + let offence = Offence { + validator_set_count: 5, + time_slot: 62, + offenders: vec![5], + }; + Offences::report_offence(vec![], offence).unwrap(); + // #3 + let offence = Offence { + validator_set_count: 5, + time_slot: 72, + offenders: vec![5], + }; + Offences::report_offence(vec![], offence).unwrap(); + // 3 are queued + assert_eq!(Offences::deferred_offences().len(), 3); + + // Allow reporting + set_can_report(true); + + Offences::on_initialize(3); + // Two are completed, one is left in the queue + assert_eq!(Offences::deferred_offences().len(), 1); + + Offences::on_initialize(4); + // All are done now + assert_eq!(Offences::deferred_offences().len(), 0); + }) +} diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index 54e7b5aaaf..bb9664bb2e 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -3218,7 +3218,9 @@ impl Convert> } /// This is intended to be used with `FilterHistoricalOffences`. -impl OnOffenceHandler> for Module where +impl + OnOffenceHandler, Weight> +for Module where T: pallet_session::Trait::AccountId>, T: pallet_session::historical::Trait< FullIdentification = Exposure<::AccountId, BalanceOf>, @@ -3226,24 +3228,32 @@ impl OnOffenceHandler, T::SessionHandler: pallet_session::SessionHandler<::AccountId>, T::SessionManager: pallet_session::SessionManager<::AccountId>, - T::ValidatorIdOf: Convert<::AccountId, Option<::AccountId>> + T::ValidatorIdOf: Convert< + ::AccountId, + Option<::AccountId>, + >, { fn on_offence( offenders: &[OffenceDetails>], slash_fraction: &[Perbill], slash_session: SessionIndex, - ) -> Result<(), ()> { + ) -> Result { if !Self::can_report() { return Err(()) } let reward_proportion = SlashRewardFraction::get(); + let mut consumed_weight: Weight = 0; + let mut add_db_reads_writes = |reads, writes| { + consumed_weight += T::DbWeight::get().reads_writes(reads, writes); + }; let active_era = { let active_era = Self::active_era(); + add_db_reads_writes(1, 0); if active_era.is_none() { // this offence need not be re-submitted. - return Ok(()) + return Ok(consumed_weight) } active_era.expect("value checked not to be `None`; qed").index }; @@ -3252,6 +3262,7 @@ impl OnOffenceHandler OnOffenceHandler return Ok(()), // before bonding period. defensive - should be filtered out. Some(&(ref slash_era, _)) => *slash_era, + // before bonding period. defensive - should be filtered out. + None => return Ok(consumed_weight), } }; @@ -3274,14 +3287,18 @@ impl OnOffenceHandler OnOffenceHandler(unapplied); + { + let slash_cost = (6, 5); + let reward_cost = (2, 2); + add_db_reads_writes( + (1 + nominators_len) * slash_cost.0 + reward_cost.0 * reporters_len, + (1 + nominators_len) * slash_cost.1 + reward_cost.1 * reporters_len + ); + } } else { // defer to end of some `slash_defer_duration` from now. ::UnappliedSlashes::mutate( active_era, move |for_later| for_later.push(unapplied), ); + add_db_reads_writes(1, 1); } + } else { + add_db_reads_writes(4 /* fetch_spans */, 5 /* kick_out_if_recent */) } } - Ok(()) + Ok(consumed_weight) } fn can_report() -> bool { diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 80ffc4b7bf..31137e04eb 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -4667,6 +4667,49 @@ fn migrate_era_should_handle_errors_2() { }); } +#[test] +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), Ok(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) + + ::DbWeight::get().reads_writes(4, 5); + + let offenders: Vec::AccountId, pallet_session::historical::IdentificationTuple>> + = (1..10).map(|i| + OffenceDetails { + offender: (i, Staking::eras_stakers(Staking::active_era().unwrap().index, i)), + reporters: vec![], + } + ).collect(); + assert_eq!(Staking::on_offence(&offenders, &[Perbill::from_percent(50)], 0), Ok(n_offence_unapplied_weight)); + + // On Offence with one offenders, Applied + let one_offender = [ + OffenceDetails { + offender: (11, Staking::eras_stakers(Staking::active_era().unwrap().index, 11)), + reporters: vec![1], + }, + ]; + + let n = 1; // Number of offenders + let rw = 3 + 3 * n; // rw reads and writes + let one_offence_unapplied_weight = ::DbWeight::get().reads_writes(4, 1) + + ::DbWeight::get().reads_writes(rw, rw) + // One `slash_cost` + + ::DbWeight::get().reads_writes(6, 5) + // `slash_cost` * nominators (1) + + ::DbWeight::get().reads_writes(6, 5) + // `reward_cost` * reporters (1) + + ::DbWeight::get().reads_writes(2, 2); + + assert_eq!(Staking::on_offence(&one_offender, &[Perbill::from_percent(50)], 0), Ok(one_offence_unapplied_weight)); + }); +} + #[test] fn on_initialize_weight_is_correct() { ExtBuilder::default().has_stakers(false).build_and_execute(|| { diff --git a/substrate/primitives/staking/src/offence.rs b/substrate/primitives/staking/src/offence.rs index b250dc6c22..e6536b5709 100644 --- a/substrate/primitives/staking/src/offence.rs +++ b/substrate/primitives/staking/src/offence.rs @@ -127,7 +127,7 @@ impl> ReportOffence { +pub trait OnOffenceHandler { /// A handler for an offence of a particular kind. /// /// Note that this contains a list of all previous offenders @@ -148,7 +148,7 @@ pub trait OnOffenceHandler { offenders: &[OffenceDetails], slash_fraction: &[Perbill], session: SessionIndex, - ) -> Result<(), ()>; + ) -> Result; /// Can an offence be reported now or not. This is an method to short-circuit a call into /// `on_offence`. Ideally, a correct implementation should return `false` if `on_offence` will @@ -157,12 +157,14 @@ pub trait OnOffenceHandler { fn can_report() -> bool; } -impl OnOffenceHandler for () { +impl OnOffenceHandler for () { fn on_offence( _offenders: &[OffenceDetails], _slash_fraction: &[Perbill], _session: SessionIndex, - ) -> Result<(), ()> { Ok(()) } + ) -> Result { + Ok(Default::default()) + } fn can_report() -> bool { true } }