From ffca28ba590ba178d081000e239e59a79470f100 Mon Sep 17 00:00:00 2001 From: Lohann Paterno Coutinho Ferreira Date: Mon, 3 May 2021 04:53:09 -0300 Subject: [PATCH] Remove Offence delay (#8414) * Removed can_report api from OnOffenceHandler * Removed DeferredOffences and create a storage migration * Removed missing comments * Mock set_deferred_offences and deferred_offences methods * OnOffenceHandler::on_offence always succeed * Fix benchmark tests * Fix runtime-benchmark cfg methods * Removed 'applied' attribute from Offence event * refactor deprecated deferred offences getter * Validate if offences are submited after on_runtime_upgrade * update changelog * Remove empty lines * Fix remove_deferred_storage weights * Remove Offence::on_runtime_upgrade benchmark * Revert CHANGELOG.md update * Deprecate DeferredOffenceOf type * Update copyright Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Add migration logs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * Fix migration log * Remove unused import * Add migration tests * rustfmt * use generate_storage_alias! macro * Refactor should_resubmit_deferred_offences test * Replace spaces by tabs * Refactor should_resubmit_deferred_offences test * Removed WeightSoftLimit * Removed WeightSoftLimit from tests and mocks * Remove unused imports * Apply suggestions from code review Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- substrate/bin/node/runtime/src/lib.rs | 6 - substrate/frame/babe/src/mock.rs | 7 -- substrate/frame/grandpa/src/mock.rs | 6 - .../frame/offences/benchmarking/src/lib.rs | 48 +------- .../frame/offences/benchmarking/src/mock.rs | 7 +- substrate/frame/offences/src/lib.rs | 96 ++-------------- substrate/frame/offences/src/migration.rs | 99 +++++++++++++++++ substrate/frame/offences/src/mock.rs | 33 +----- substrate/frame/offences/src/tests.rs | 103 +----------------- substrate/frame/staking/src/lib.rs | 17 +-- substrate/frame/staking/src/mock.rs | 4 +- substrate/frame/staking/src/tests.rs | 6 +- substrate/primitives/staking/src/offence.rs | 14 +-- 13 files changed, 132 insertions(+), 314 deletions(-) create mode 100644 substrate/frame/offences/src/migration.rs diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 2eaea18f2a..498593e57a 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -896,16 +896,10 @@ impl pallet_im_online::Config for Runtime { type WeightInfo = pallet_im_online::weights::SubstrateWeight; } -parameter_types! { - pub OffencesWeightSoftLimit: Weight = Perbill::from_percent(60) * - RuntimeBlockWeights::get().max_block; -} - impl pallet_offences::Config for Runtime { type Event = Event; type IdentificationTuple = pallet_session::historical::IdentificationTuple; type OnOffenceHandler = Staking; - type WeightSoftLimit = OffencesWeightSoftLimit; } impl pallet_authority_discovery::Config for Runtime {} diff --git a/substrate/frame/babe/src/mock.rs b/substrate/frame/babe/src/mock.rs index d01a67f403..40ee782e72 100644 --- a/substrate/frame/babe/src/mock.rs +++ b/substrate/frame/babe/src/mock.rs @@ -29,7 +29,6 @@ use frame_system::InitKind; use frame_support::{ parameter_types, traits::{KeyOwnerProofSystem, OnInitialize}, - weights::Weight, }; use sp_io; use sp_core::{H256, U256, crypto::{IsWrappedBy, KeyTypeId, Pair}}; @@ -215,16 +214,10 @@ impl pallet_staking::Config for Test { type WeightInfo = (); } -parameter_types! { - pub OffencesWeightSoftLimit: Weight = Perbill::from_percent(60) - * BlockWeights::get().max_block; -} - impl pallet_offences::Config for Test { type Event = Event; type IdentificationTuple = pallet_session::historical::IdentificationTuple; type OnOffenceHandler = Staking; - type WeightSoftLimit = OffencesWeightSoftLimit; } parameter_types! { diff --git a/substrate/frame/grandpa/src/mock.rs b/substrate/frame/grandpa/src/mock.rs index b13c431dc5..e26020b600 100644 --- a/substrate/frame/grandpa/src/mock.rs +++ b/substrate/frame/grandpa/src/mock.rs @@ -25,7 +25,6 @@ use codec::Encode; use frame_support::{ parameter_types, traits::{KeyOwnerProofSystem, OnFinalize, OnInitialize}, - weights::Weight, }; use pallet_staking::EraIndex; use sp_core::{crypto::KeyTypeId, H256}; @@ -221,15 +220,10 @@ impl pallet_staking::Config for Test { type WeightInfo = (); } -parameter_types! { - pub OffencesWeightSoftLimit: Weight = Perbill::from_percent(60) * BlockWeights::get().max_block; -} - impl pallet_offences::Config for Test { type Event = Event; type IdentificationTuple = pallet_session::historical::IdentificationTuple; type OnOffenceHandler = Staking; - type WeightSoftLimit = OffencesWeightSoftLimit; } parameter_types! { diff --git a/substrate/frame/offences/benchmarking/src/lib.rs b/substrate/frame/offences/benchmarking/src/lib.rs index 08517a4ac8..4e5160c667 100644 --- a/substrate/frame/offences/benchmarking/src/lib.rs +++ b/substrate/frame/offences/benchmarking/src/lib.rs @@ -26,13 +26,13 @@ use sp_std::vec; use frame_system::{RawOrigin, Pallet as System, Config as SystemConfig}; use frame_benchmarking::{benchmarks, account, impl_benchmark_test_suite}; -use frame_support::traits::{Currency, OnInitialize, ValidatorSet, ValidatorSetWithIdentification}; +use frame_support::traits::{Currency, ValidatorSet, ValidatorSetWithIdentification}; use sp_runtime::{ Perbill, traits::{Convert, StaticLookup, Saturating, UniqueSaturatedInto}, }; -use sp_staking::offence::{ReportOffence, Offence, OffenceDetails}; +use sp_staking::offence::{ReportOffence, Offence}; use pallet_balances::Config as BalancesConfig; use pallet_babe::BabeEquivocationOffence; @@ -51,7 +51,6 @@ const SEED: u32 = 0; const MAX_REPORTERS: u32 = 100; const MAX_OFFENDERS: u32 = 100; const MAX_NOMINATORS: u32 = 100; -const MAX_DEFERRED_OFFENCES: u32 = 100; pub struct Pallet(Offences); @@ -271,8 +270,6 @@ benchmarks! { ); } verify { - // make sure the report was not deferred - assert!(Offences::::deferred_offences().is_empty()); let bond_amount: u32 = UniqueSaturatedInto::::unique_saturated_into(bond_amount::()); let slash_amount = slash_fraction * bond_amount; let reward_amount = slash_amount * (1 + n) / 2; @@ -306,7 +303,6 @@ benchmarks! { pallet_offences::Event::Offence( UnresponsivenessOffence::::ID, 0_u32.to_le_bytes().to_vec(), - true ) ).into())) ); @@ -336,8 +332,6 @@ benchmarks! { let _ = Offences::::report_offence(reporters, offence); } verify { - // make sure the report was not deferred - assert!(Offences::::deferred_offences().is_empty()); // make sure that all slashes have been applied assert_eq!( System::::event_count(), 0 @@ -372,8 +366,6 @@ benchmarks! { let _ = Offences::::report_offence(reporters, offence); } verify { - // make sure the report was not deferred - assert!(Offences::::deferred_offences().is_empty()); // make sure that all slashes have been applied assert_eq!( System::::event_count(), 0 @@ -383,42 +375,6 @@ benchmarks! { + n // nominators slashed ); } - - on_initialize { - let d in 1 .. MAX_DEFERRED_OFFENCES; - let o = 10; - let n = 100; - - let mut deferred_offences = vec![]; - let offenders = make_offenders::(o, n)?.0; - let offence_details = offenders.into_iter() - .map(|offender| OffenceDetails { - offender: T::convert(offender), - reporters: vec![], - }) - .collect::>(); - - for i in 0 .. d { - let fractions = offence_details.iter() - .map(|_| Perbill::from_percent(100 * (i + 1) / MAX_DEFERRED_OFFENCES)) - .collect::>(); - deferred_offences.push((offence_details.clone(), fractions.clone(), 0u32)); - } - - Offences::::set_deferred_offences(deferred_offences); - assert!(!Offences::::deferred_offences().is_empty()); - }: { - Offences::::on_initialize(0u32.into()); - } - verify { - // make sure that all deferred offences were reported with Ok status. - assert!(Offences::::deferred_offences().is_empty()); - assert_eq!( - System::::event_count(), d * (0 - + o // offenders slashed - + o * n // nominators slashed - )); - } } impl_benchmark_test_suite!( diff --git a/substrate/frame/offences/benchmarking/src/mock.rs b/substrate/frame/offences/benchmarking/src/mock.rs index a0a09e0fbb..9047120923 100644 --- a/substrate/frame/offences/benchmarking/src/mock.rs +++ b/substrate/frame/offences/benchmarking/src/mock.rs @@ -22,7 +22,7 @@ use super::*; use frame_support::{ parameter_types, - weights::{Weight, constants::WEIGHT_PER_SECOND}, + weights::constants::WEIGHT_PER_SECOND, }; use frame_system as system; use sp_runtime::{ @@ -189,15 +189,10 @@ impl pallet_im_online::Config for Test { type WeightInfo = (); } -parameter_types! { - pub OffencesWeightSoftLimit: Weight = Perbill::from_percent(60) * BlockWeights::get().max_block; -} - impl pallet_offences::Config 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 2765c0aaa0..cd25ca1ef1 100644 --- a/substrate/frame/offences/src/lib.rs +++ b/substrate/frame/offences/src/lib.rs @@ -24,12 +24,13 @@ mod mock; mod tests; +mod migration; use sp_std::vec::Vec; use frame_support::{ - decl_module, decl_event, decl_storage, Parameter, traits::Get, weights::Weight, + decl_module, decl_event, decl_storage, Parameter, weights::Weight, }; -use sp_runtime::{traits::{Hash, Zero}, Perbill}; +use sp_runtime::{traits::Hash, Perbill}; use sp_staking::{ SessionIndex, offence::{Offence, ReportOffence, Kind, OnOffenceHandler, OffenceDetails, OffenceError}, @@ -42,13 +43,6 @@ type OpaqueTimeSlot = Vec; /// A type alias for a report identifier. type ReportIdOf = ::Hash; -/// Type of data stored as a deferred offence -pub type DeferredOffenceOf = ( - Vec::AccountId, ::IdentificationTuple>>, - Vec, - SessionIndex, -); - pub trait WeightInfo { fn report_offence_im_online(r: u32, o: u32, n: u32, ) -> Weight; fn report_offence_grandpa(r: u32, n: u32, ) -> Weight; @@ -71,10 +65,6 @@ pub trait Config: frame_system::Config { type IdentificationTuple: Parameter + Ord; /// A handler called for every offence report. 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! { @@ -84,10 +74,6 @@ decl_storage! { map hasher(twox_64_concat) ReportIdOf => Option>; - /// Deferred reports that have been rejected by the offence handler and need to be submitted - /// at a later time. - DeferredOffences get(fn deferred_offences): Vec>; - /// A vector of reports of the same kind that happened at the same time slot. ConcurrentReportsIndex: double_map hasher(twox_64_concat) Kind, hasher(twox_64_concat) OpaqueTimeSlot @@ -106,10 +92,9 @@ decl_storage! { decl_event!( pub enum Event { /// There is an offence reported of the given `kind` happened at the `session_index` and - /// (kind-specific) time slot. This event is not deposited for duplicate slashes. last - /// element indicates of the offence was applied (true) or queued (false) - /// \[kind, timeslot, applied\]. - Offence(Kind, OpaqueTimeSlot, bool), + /// (kind-specific) time slot. This event is not deposited for duplicate slashes. + /// \[kind, timeslot\]. + Offence(Kind, OpaqueTimeSlot), } ); @@ -117,42 +102,8 @@ decl_module! { pub struct Module for enum Call where origin: T::Origin { fn deposit_event() = default; - fn on_initialize(now: T::BlockNumber) -> Weight { - // only decode storage if we can actually submit anything again. - if !T::OnOffenceHandler::can_report() { - return 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(_) => { - log::error!( - target: "runtime::offences", - "re-submitting a deferred slash returned Err at {:?}. \ - This should not happen with pallet-staking", - now, - ); - true - }, - } - } - }) - }); - - consumed + fn on_runtime_upgrade() -> Weight { + migration::remove_deferred_storage::() } } } @@ -187,14 +138,14 @@ where let slash_perbill: Vec<_> = (0..concurrent_offenders.len()) .map(|_| new_fraction.clone()).collect(); - let applied = Self::report_or_store_offence( + T::OnOffenceHandler::on_offence( &concurrent_offenders, &slash_perbill, offence.session_index(), ); // Deposit the event. - Self::deposit_event(Event::Offence(O::ID, time_slot.encode(), applied)); + Self::deposit_event(Event::Offence(O::ID, time_slot.encode())); Ok(()) } @@ -210,28 +161,6 @@ where } impl Module { - /// Tries (without checking) to report an offence. Stores them in [`DeferredOffences`] in case - /// it fails. Returns false in case it has to store the offence. - fn report_or_store_offence( - concurrent_offenders: &[OffenceDetails], - slash_perbill: &[Perbill], - session_index: SessionIndex, - ) -> bool { - match T::OnOffenceHandler::on_offence( - &concurrent_offenders, - &slash_perbill, - session_index, - ) { - Ok(_) => true, - Err(_) => { - >::mutate(|d| - d.push((concurrent_offenders.to_vec(), slash_perbill.to_vec(), session_index)) - ); - false - } - } - } - /// Compute the ID for the given report properties. /// /// The report id depends on the offence kind, time slot and the id of offender. @@ -285,11 +214,6 @@ impl Module { None } } - - #[cfg(feature = "runtime-benchmarks")] - pub fn set_deferred_offences(offences: Vec>) { - >::put(offences); - } } struct TriageOutcome { diff --git a/substrate/frame/offences/src/migration.rs b/substrate/frame/offences/src/migration.rs new file mode 100644 index 0000000000..ce8a125e7e --- /dev/null +++ b/substrate/frame/offences/src/migration.rs @@ -0,0 +1,99 @@ +// This file is part of Substrate. + +// Copyright (C) 2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use super::{Config, OffenceDetails, Perbill, SessionIndex}; +use frame_support::{traits::Get, weights::Weight, generate_storage_alias}; +use sp_staking::offence::OnOffenceHandler; +use sp_std::vec::Vec; + +/// Type of data stored as a deferred offence +type DeferredOffenceOf = ( + Vec< + OffenceDetails< + ::AccountId, + ::IdentificationTuple, + >, + >, + Vec, + SessionIndex, +); + +// Deferred reports that have been rejected by the offence handler and need to be submitted +// at a later time. +generate_storage_alias!( + Offences, + DeferredOffences => Value>> +); + +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); + weight = weight.saturating_add(consumed); + } + + weight +} + +#[cfg(test)] +mod test { + use super::*; + use crate::mock::{new_test_ext, with_on_offence_fractions, Offences, Runtime as T}; + use frame_support::traits::OnRuntimeUpgrade; + use sp_runtime::Perbill; + use sp_staking::offence::OffenceDetails; + + #[test] + fn should_resubmit_deferred_offences() { + new_test_ext().execute_with(|| { + // given + assert_eq!(>::get().len(), 0); + with_on_offence_fractions(|f| { + assert_eq!(f.clone(), vec![]); + }); + + let offence_details = OffenceDetails::< + ::AccountId, + ::IdentificationTuple, + > { + offender: 5, + reporters: vec![], + }; + + // push deferred offence + >::append(( + vec![offence_details], + vec![Perbill::from_percent(5 + 1 * 100 / 5)], + 1, + )); + + // when + assert_eq!( + Offences::on_runtime_upgrade(), + ::DbWeight::get().reads_writes(1, 2), + ); + + // then + assert!(!>::exists()); + with_on_offence_fractions(|f| { + assert_eq!(f.clone(), vec![Perbill::from_percent(5 + 1 * 100 / 5)]); + }); + }) + } +} diff --git a/substrate/frame/offences/src/mock.rs b/substrate/frame/offences/src/mock.rs index 52dd55207a..4176a54d9e 100644 --- a/substrate/frame/offences/src/mock.rs +++ b/substrate/frame/offences/src/mock.rs @@ -40,7 +40,6 @@ 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()); } @@ -51,25 +50,13 @@ impl _offenders: &[OffenceDetails], slash_fraction: &[Perbill], _offence_session: SessionIndex, - ) -> Result { - if >::can_report() { - ON_OFFENCE_PERBILL.with(|f| { - *f.borrow_mut() = slash_fraction.to_vec(); - }); + ) -> Weight { + ON_OFFENCE_PERBILL.with(|f| { + *f.borrow_mut() = slash_fraction.to_vec(); + }); - Ok(OFFENCE_WEIGHT.with(|w| *w.borrow())) - } else { - Err(()) - } + OFFENCE_WEIGHT.with(|w| *w.borrow()) } - - fn can_report() -> bool { - CAN_REPORT.with(|c| *c.borrow()) - } -} - -pub fn set_can_report(can_report: bool) { - CAN_REPORT.with(|c| *c.borrow_mut() = can_report); } pub fn with_on_offence_fractions) -> R>(f: F) -> R { @@ -78,10 +65,6 @@ pub fn with_on_offence_fractions) -> R>(f: F) -> }) } -pub fn set_offence_weight(new: Weight) { - OFFENCE_WEIGHT.with(|w| *w.borrow_mut() = new); -} - type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; type Block = frame_system::mocking::MockBlock; @@ -127,16 +110,10 @@ impl frame_system::Config for Runtime { type OnSetCode = (); } -parameter_types! { - pub OffencesWeightSoftLimit: Weight = - Perbill::from_percent(60) * BlockWeights::get().max_block; -} - impl Config for Runtime { type Event = Event; type IdentificationTuple = u64; type OnOffenceHandler = OnOffenceHandler; - type WeightSoftLimit = OffencesWeightSoftLimit; } pub fn new_test_ext() -> sp_io::TestExternalities { diff --git a/substrate/frame/offences/src/tests.rs b/substrate/frame/offences/src/tests.rs index 2b7c500dfa..f7bd90fe93 100644 --- a/substrate/frame/offences/src/tests.rs +++ b/substrate/frame/offences/src/tests.rs @@ -22,10 +22,9 @@ use super::*; use crate::mock::{ Offences, System, Offence, Event, KIND, new_test_ext, with_on_offence_fractions, - offence_reports, set_can_report, set_offence_weight, + offence_reports, }; use sp_runtime::Perbill; -use frame_support::traits::OnInitialize; use frame_system::{EventRecord, Phase}; #[test] @@ -132,7 +131,7 @@ fn should_deposit_event() { System::events(), vec![EventRecord { phase: Phase::Initialization, - event: Event::offences(crate::Event::Offence(KIND, time_slot.encode(), true)), + event: Event::offences(crate::Event::Offence(KIND, time_slot.encode())), topics: vec![], }] ); @@ -167,7 +166,7 @@ fn doesnt_deposit_event_for_dups() { System::events(), vec![EventRecord { phase: Phase::Initialization, - event: Event::offences(crate::Event::Offence(KIND, time_slot.encode(), true)), + event: Event::offences(crate::Event::Offence(KIND, time_slot.encode())), topics: vec![], }] ); @@ -285,99 +284,3 @@ fn should_properly_count_offences() { ); }); } - -#[test] -fn should_queue_and_resubmit_rejected_offence() { - new_test_ext().execute_with(|| { - set_can_report(false); - - // will get deferred - let offence = Offence { - validator_set_count: 5, - time_slot: 42, - offenders: vec![5], - }; - Offences::report_offence(vec![], offence).unwrap(); - assert_eq!(Offences::deferred_offences().len(), 1); - // event also indicates unapplied. - assert_eq!( - System::events(), - vec![EventRecord { - phase: Phase::Initialization, - event: Event::offences(crate::Event::Offence(KIND, 42u128.encode(), false)), - topics: vec![], - }] - ); - - // will not dequeue - Offences::on_initialize(2); - - // again - let offence = Offence { - validator_set_count: 5, - time_slot: 62, - offenders: vec![5], - }; - Offences::report_offence(vec![], offence).unwrap(); - assert_eq!(Offences::deferred_offences().len(), 2); - - set_can_report(true); - - // can be submitted - let offence = Offence { - validator_set_count: 5, - time_slot: 72, - offenders: vec![5], - }; - Offences::report_offence(vec![], offence).unwrap(); - assert_eq!(Offences::deferred_offences().len(), 2); - - Offences::on_initialize(3); - 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 b1d6ba6bd9..67726f6922 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -2765,11 +2765,7 @@ where >], slash_fraction: &[Perbill], slash_session: SessionIndex, - ) -> Result { - if !Self::can_report() { - return Err(()); - } - + ) -> Weight { let reward_proportion = SlashRewardFraction::get(); let mut consumed_weight: Weight = 0; let mut add_db_reads_writes = |reads, writes| { @@ -2781,7 +2777,7 @@ where add_db_reads_writes(1, 0); if active_era.is_none() { // this offence need not be re-submitted. - return Ok(consumed_weight) + return consumed_weight } active_era.expect("value checked not to be `None`; qed").index }; @@ -2806,7 +2802,7 @@ where match eras.iter().rev().filter(|&&(_, ref sesh)| sesh <= &slash_session).next() { Some(&(ref slash_era, _)) => *slash_era, // before bonding period. defensive - should be filtered out. - None => return Ok(consumed_weight), + None => return consumed_weight, } }; @@ -2874,12 +2870,7 @@ where } } - Ok(consumed_weight) - } - - fn can_report() -> bool { - // TODO: https://github.com/paritytech/substrate/issues/8343 - true + consumed_weight } } diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index c8556a806a..4027ac1f67 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -715,7 +715,7 @@ pub(crate) fn on_offence_in_era( 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).unwrap(); + let _ = Staking::on_offence(offenders, slash_fraction, start_session); return; } else if bonded_era > era { break; @@ -728,7 +728,7 @@ pub(crate) fn on_offence_in_era( offenders, slash_fraction, Staking::eras_start_session_index(era).unwrap() - ).unwrap(); + ); } else { panic!("cannot slash in era {}", era); } diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 634504ccb6..ec5a61d468 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -3510,7 +3510,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), Ok(zero_offence_weight)); + assert_eq!(Staking::on_offence(&[], &[Perbill::from_percent(50)], 0), 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) @@ -3523,7 +3523,7 @@ fn offences_weight_calculated_correctly() { reporters: vec![], } ).collect(); - assert_eq!(Staking::on_offence(&offenders, &[Perbill::from_percent(50)], 0), Ok(n_offence_unapplied_weight)); + assert_eq!(Staking::on_offence(&offenders, &[Perbill::from_percent(50)], 0), n_offence_unapplied_weight); // On Offence with one offenders, Applied let one_offender = [ @@ -3544,7 +3544,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), Ok(one_offence_unapplied_weight)); + assert_eq!(Staking::on_offence(&one_offender, &[Perbill::from_percent(50)], 0), one_offence_unapplied_weight); }); } diff --git a/substrate/primitives/staking/src/offence.rs b/substrate/primitives/staking/src/offence.rs index 0212d1bd8f..ab72ecda04 100644 --- a/substrate/primitives/staking/src/offence.rs +++ b/substrate/primitives/staking/src/offence.rs @@ -159,13 +159,7 @@ pub trait OnOffenceHandler { offenders: &[OffenceDetails], slash_fraction: &[Perbill], session: SessionIndex, - ) -> 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 - /// return `Err`. Nonetheless, this is up to the implementation and this trait cannot guarantee - /// it. - fn can_report() -> bool; + ) -> Res; } impl OnOffenceHandler for () { @@ -173,11 +167,9 @@ impl OnOffenceHandler _offenders: &[OffenceDetails], _slash_fraction: &[Perbill], _session: SessionIndex, - ) -> Result { - Ok(Default::default()) + ) -> Res { + Default::default() } - - fn can_report() -> bool { true } } /// A details about an offending authority for a particular kind of offence.