Remove Offence delay (paritytech#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 <[email protected]> * Add migration logs Co-authored-by: Kian Paimani <[email protected]> * 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 <[email protected]>
autonomys · Aug 8, 2021 · 79975b3 · 79975b3
1 parent 46d8efe
commit 79975b3
Show file tree

Hide file tree

Showing 13 changed files with 132 additions and 314 deletions.
diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
@@ -896,16 +896,10 @@ impl pallet_im_online::Config for Runtime {
 	type WeightInfo = pallet_im_online::weights::SubstrateWeight<Runtime>;
 }
 
-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<Self>;
 	type OnOffenceHandler = Staking;
-	type WeightSoftLimit = OffencesWeightSoftLimit;
 }
 
 impl pallet_authority_discovery::Config for Runtime {}

diff --git a/frame/babe/src/mock.rs b/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<Self>;
 	type OnOffenceHandler = Staking;
-	type WeightSoftLimit = OffencesWeightSoftLimit;
 }
 
 parameter_types! {

diff --git a/frame/grandpa/src/mock.rs b/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<Self>;
 	type OnOffenceHandler = Staking;
-	type WeightSoftLimit = OffencesWeightSoftLimit;
 }
 
 parameter_types! {

diff --git a/frame/offences/benchmarking/src/lib.rs b/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<T: Config>(Offences<T>);
 
@@ -271,8 +270,6 @@ benchmarks! {
 		);
 	}
 	verify {
-		// make sure the report was not deferred
-		assert!(Offences::<T>::deferred_offences().is_empty());
 		let bond_amount: u32 = UniqueSaturatedInto::<u32>::unique_saturated_into(bond_amount::<T>());
 		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::<T>::ID,
 						0_u32.to_le_bytes().to_vec(),
-						true
 					)
 				).into()))
 		);
@@ -336,8 +332,6 @@ benchmarks! {
 		let _ = Offences::<T>::report_offence(reporters, offence);
 	}
 	verify {
-		// make sure the report was not deferred
-		assert!(Offences::<T>::deferred_offences().is_empty());
 		// make sure that all slashes have been applied
 		assert_eq!(
 			System::<T>::event_count(), 0
@@ -372,8 +366,6 @@ benchmarks! {
 		let _ = Offences::<T>::report_offence(reporters, offence);
 	}
 	verify {
-		// make sure the report was not deferred
-		assert!(Offences::<T>::deferred_offences().is_empty());
 		// make sure that all slashes have been applied
 		assert_eq!(
 			System::<T>::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::<T>(o, n)?.0;
-		let offence_details = offenders.into_iter()
-			.map(|offender| OffenceDetails {
-				offender: T::convert(offender),
-				reporters: vec![],
-			})
-			.collect::<Vec<_>>();
-
-		for i in 0 .. d {
-			let fractions = offence_details.iter()
-				.map(|_| Perbill::from_percent(100 * (i + 1) / MAX_DEFERRED_OFFENCES))
-				.collect::<Vec<_>>();
-			deferred_offences.push((offence_details.clone(), fractions.clone(), 0u32));
-		}
-
-		Offences::<T>::set_deferred_offences(deferred_offences);
-		assert!(!Offences::<T>::deferred_offences().is_empty());
-	}: {
-		Offences::<T>::on_initialize(0u32.into());
-	}
-	verify {
-		// make sure that all deferred offences were reported with Ok status.
-		assert!(Offences::<T>::deferred_offences().is_empty());
-		assert_eq!(
-			System::<T>::event_count(), d * (0
-			+ o // offenders slashed
-			+ o * n // nominators slashed
-		));
-	}
 }
 
 impl_benchmark_test_suite!(

diff --git a/frame/offences/benchmarking/src/mock.rs b/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<Self>;
 	type OnOffenceHandler = Staking;
-	type WeightSoftLimit = OffencesWeightSoftLimit;
 }
 
 impl<T> frame_system::offchain::SendTransactionTypes<T> for Test where Call: From<T> {

diff --git a/frame/offences/src/lib.rs b/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<u8>;
 /// A type alias for a report identifier.
 type ReportIdOf<T> = <T as frame_system::Config>::Hash;
 
-/// Type of data stored as a deferred offence
-pub type DeferredOffenceOf<T> = (
-	Vec<OffenceDetails<<T as frame_system::Config>::AccountId, <T as Config>::IdentificationTuple>>,
-	Vec<Perbill>,
-	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<Self::AccountId, Self::IdentificationTuple, Weight>;
-	/// 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<Weight>;
 }
 
 decl_storage! {
@@ -84,10 +74,6 @@ decl_storage! {
 			map hasher(twox_64_concat) ReportIdOf<T>
 			=> Option<OffenceDetails<T::AccountId, T::IdentificationTuple>>;
 
-		/// 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<DeferredOffenceOf<T>>;
-
 		/// 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,53 +92,18 @@ 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),
 	}
 );
 
 decl_module! {
 	pub struct Module<T: Config> 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();
-
-			<DeferredOffences<T>>::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::<T>()
 		}
 	}
 }
@@ -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<T: Config> Module<T> {
-	/// 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<T::AccountId, T::IdentificationTuple>],
-		slash_perbill: &[Perbill],
-		session_index: SessionIndex,
-	) -> bool {
-		match T::OnOffenceHandler::on_offence(
-			&concurrent_offenders,
-			&slash_perbill,
-			session_index,
-		) {
-			Ok(_) => true,
-			Err(_) => {
-				<DeferredOffences<T>>::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<T: Config> Module<T> {
 			None
 		}
 	}
-
-	#[cfg(feature = "runtime-benchmarks")]
-	pub fn set_deferred_offences(offences: Vec<DeferredOffenceOf<T>>) {
-		<DeferredOffences<T>>::put(offences);
-	}
 }
 
 struct TriageOutcome<T: Config> {