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/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,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<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> {

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<T> = (
+	Vec<
+		OffenceDetails<
+			<T as frame_system::Config>::AccountId,
+			<T as Config>::IdentificationTuple,
+		>,
+	>,
+	Vec<Perbill>,
+	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<T: Config> => Value<Vec<DeferredOffenceOf<T>>>
+);
+
+pub fn remove_deferred_storage<T: Config>() -> Weight {
+	let mut weight = T::DbWeight::get().reads_writes(1, 1);
+	let deferred = <DeferredOffences<T>>::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!(<DeferredOffences<T>>::get().len(), 0);
+			with_on_offence_fractions(|f| {
+				assert_eq!(f.clone(), vec![]);
+			});
+
+			let offence_details = OffenceDetails::<
+				<T as frame_system::Config>::AccountId,
+				<T as Config>::IdentificationTuple,
+			> {
+				offender: 5,
+				reporters: vec![],
+			};
+
+			// push deferred offence
+			<DeferredOffences<T>>::append((
+				vec![offence_details],
+				vec![Perbill::from_percent(5 + 1 * 100 / 5)],
+				1,
+			));
+
+			// when
+			assert_eq!(
+				Offences::on_runtime_upgrade(),
+				<T as frame_system::Config>::DbWeight::get().reads_writes(1, 2),
+			);
+
+			// then
+			assert!(!<DeferredOffences<T>>::exists());
+			with_on_offence_fractions(|f| {
+				assert_eq!(f.clone(), vec![Perbill::from_percent(5 + 1 * 100 / 5)]);
+			});
+		})
+	}
+}

substrate/frame/offences/src/mock.rs

@@ -40,7 +40,6 @@ pub struct OnOffenceHandler;
 
 thread_local! {
 	pub static ON_OFFENCE_PERBILL: RefCell<Vec<Perbill>> = RefCell::new(Default::default());
-	pub static CAN_REPORT: RefCell<bool> = RefCell::new(true);
 	pub static OFFENCE_WEIGHT: RefCell<Weight> = RefCell::new(Default::default());
 }
 
@@ -51,25 +50,13 @@ impl<Reporter, Offender>
 		_offenders: &[OffenceDetails<Reporter, Offender>],
 		slash_fraction: &[Perbill],
 		_offence_session: SessionIndex,
-	) -> Result<Weight, ()> {
-		if <Self as offence::OnOffenceHandler<Reporter, Offender, Weight>>::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: FnOnce(&mut Vec<Perbill>) -> R>(f: F) -> R {
@@ -78,10 +65,6 @@ pub fn with_on_offence_fractions<R, F: FnOnce(&mut Vec<Perbill>) -> R>(f: F) ->
 	})
 }
 
-pub fn set_offence_weight(new: Weight) {
-	OFFENCE_WEIGHT.with(|w| *w.borrow_mut() = new);
-}
-
 type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Runtime>;
 type Block = frame_system::mocking::MockBlock<Runtime>;
 
@@ -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 {

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(<mock::Runtime as Config>::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);
-	})
-}