Offences Weight for OnInitialize (#5961)

* Weight accounting for on_offence.

* Try to compute weight.

* Guesstimate upper bounds on db read/writes for slashing

* greater than or equal to

* add new trait

* Update mock.rs

* Add basic weight test

* one more test

* Update frame/staking/src/lib.rs

Co-authored-by: thiolliere <gui.thiolliere@gmail.com>

* Update frame/staking/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Add test for offences queue

Co-authored-by: Tomasz Drwięga <tomasz@parity.io>
Co-authored-by: thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

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<Self>;
 	type OnOffenceHandler = Staking;
+	type WeightSoftLimit = OffencesWeightSoftLimit;
 }
 
 impl pallet_authority_discovery::Trait for Runtime {}

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<Self>;
 	type OnOffenceHandler = Staking;
+	type WeightSoftLimit = OffencesWeightSoftLimit;
 }
 
 impl Trait for Test {

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<Self>;
 	type OnOffenceHandler = Staking;
+	type WeightSoftLimit = OffencesWeightSoftLimit;
 }
 
 impl<T> frame_system::offchain::SendTransactionTypes<T> for Test where Call: From<T> {

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<Self::AccountId, Self::IdentificationTuple>;
+	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! {
@@ -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() {
-				<DeferredOffences<T>>::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();
+
+			<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(_) => {
+								debug::native::error!(
+									target: "pallet-offences",
+									"re-submitting a deferred slash returned Err at {}. This should not happen with pallet-staking",
+									now,
+								);
+								true
+							},
+						}
+					}
+				})
+			});
+
+			consumed
 		}
 	}
 }

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<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());
 }
 
-impl<Reporter, Offender> offence::OnOffenceHandler<Reporter, Offender> for OnOffenceHandler {
+impl<Reporter, Offender>
+	offence::OnOffenceHandler<Reporter, Offender, Weight> for OnOffenceHandler
+{
 	fn on_offence(
 		_offenders: &[OffenceDetails<Reporter, Offender>],
 		slash_fraction: &[Perbill],
 		_offence_session: SessionIndex,
-	) -> Result<(), ()> {
-		if <Self as offence::OnOffenceHandler<Reporter, Offender>>::can_report() {
+	) -> Result<Weight, ()> {
+		if <Self as offence::OnOffenceHandler<Reporter, Offender, Weight>>::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: FnOnce(&mut Vec<Perbill>) -> 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 {

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

substrate/frame/staking/src/lib.rs

@@ -3218,7 +3218,9 @@ impl<T: Trait> Convert<T::AccountId, Option<Exposure<T::AccountId, BalanceOf<T>>
 }
 
 /// This is intended to be used with `FilterHistoricalOffences`.
-impl <T: Trait> OnOffenceHandler<T::AccountId, pallet_session::historical::IdentificationTuple<T>> for Module<T> where
+impl <T: Trait>
+	OnOffenceHandler<T::AccountId, pallet_session::historical::IdentificationTuple<T>, Weight>
+for Module<T> where
 	T: pallet_session::Trait<ValidatorId = <T as frame_system::Trait>::AccountId>,
 	T: pallet_session::historical::Trait<
 		FullIdentification = Exposure<<T as frame_system::Trait>::AccountId, BalanceOf<T>>,
@@ -3226,24 +3228,32 @@ impl <T: Trait> OnOffenceHandler<T::AccountId, pallet_session::historical::Ident
 	>,
 	T::SessionHandler: pallet_session::SessionHandler<<T as frame_system::Trait>::AccountId>,
 	T::SessionManager: pallet_session::SessionManager<<T as frame_system::Trait>::AccountId>,
-	T::ValidatorIdOf: Convert<<T as frame_system::Trait>::AccountId, Option<<T as frame_system::Trait>::AccountId>>
+	T::ValidatorIdOf: Convert<
+		<T as frame_system::Trait>::AccountId,
+		Option<<T as frame_system::Trait>::AccountId>,
+	>,
 {
 	fn on_offence(
 		offenders: &[OffenceDetails<T::AccountId, pallet_session::historical::IdentificationTuple<T>>],
 		slash_fraction: &[Perbill],
 		slash_session: SessionIndex,
-	) -> Result<(), ()> {
+	) -> Result<Weight, ()> {
 		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 <T: Trait> OnOffenceHandler<T::AccountId, pallet_session::historical::Ident
 				frame_support::print("Error: start_session_index must be set for current_era");
 				0
 			});
+		add_db_reads_writes(1, 0);
 
 		let window_start = active_era.saturating_sub(T::BondingDuration::get());
 
@@ -3261,11 +3272,13 @@ impl <T: Trait> OnOffenceHandler<T::AccountId, pallet_session::historical::Ident
 			active_era
 		} else {
 			let eras = BondedEras::get();
+			add_db_reads_writes(1, 0);
 
 			// reverse because it's more likely to find reports from recent eras.
 			match eras.iter().rev().filter(|&&(_, ref sesh)| sesh <= &slash_session).next() {
-				None => 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 <T: Trait> OnOffenceHandler<T::AccountId, pallet_session::historical::Ident
 				*earliest = Some(active_era)
 			}
 		});
+		add_db_reads_writes(1, 1);
 
 		let slash_defer_duration = T::SlashDeferDuration::get();
 
+		let invulnerables = Self::invulnerables();
+		add_db_reads_writes(1, 0);
+
 		for (details, slash_fraction) in offenders.iter().zip(slash_fraction) {
 			let (stash, exposure) = &details.offender;
 
 			// Skip if the validator is invulnerable.
-			if Self::invulnerables().contains(stash) {
+			if invulnerables.contains(stash) {
 				continue
 			}
 
@@ -3296,21 +3313,40 @@ impl <T: Trait> OnOffenceHandler<T::AccountId, pallet_session::historical::Ident
 			});
 
 			if let Some(mut unapplied) = unapplied {
+				let nominators_len = unapplied.others.len() as u64;
+				let reporters_len = details.reporters.len() as u64;
+
+				{
+					let upper_bound = 1 /* Validator/NominatorSlashInEra */ + 2 /* fetch_spans */;
+					let rw = upper_bound + nominators_len * upper_bound;
+					add_db_reads_writes(rw, rw);
+				}
 				unapplied.reporters = details.reporters.clone();
 				if slash_defer_duration == 0 {
 					// apply right away.
 					slashing::apply_slash::<T>(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.
 					<Self as Store>::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 {

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 = <Test as frame_system::Trait>::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 = <Test as frame_system::Trait>::DbWeight::get().reads_writes(4, 1)
+			+ <Test as frame_system::Trait>::DbWeight::get().reads_writes(4, 5);
+
+		let offenders: Vec<OffenceDetails<<Test as frame_system::Trait>::AccountId, pallet_session::historical::IdentificationTuple<Test>>>
+			= (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 = <Test as frame_system::Trait>::DbWeight::get().reads_writes(4, 1)
+			+ <Test as frame_system::Trait>::DbWeight::get().reads_writes(rw, rw)
+			// One `slash_cost`
+			+ <Test as frame_system::Trait>::DbWeight::get().reads_writes(6, 5)
+			// `slash_cost` * nominators (1)
+			+ <Test as frame_system::Trait>::DbWeight::get().reads_writes(6, 5)
+			// `reward_cost` * reporters (1)
+			+ <Test as frame_system::Trait>::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(|| {

substrate/primitives/staking/src/offence.rs

@@ -127,7 +127,7 @@ impl<Reporter, Offender, O: Offence<Offender>> ReportOffence<Reporter, Offender,
 ///
 /// Used to decouple the module that handles offences and
 /// the one that should punish for those offences.
-pub trait OnOffenceHandler<Reporter, Offender> {
+pub trait OnOffenceHandler<Reporter, Offender, Res> {
 	/// 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<Reporter, Offender> {
 		offenders: &[OffenceDetails<Reporter, Offender>],
 		slash_fraction: &[Perbill],
 		session: SessionIndex,
-	) -> Result<(), ()>;
+	) -> Result<Res, ()>;
 
 	/// 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<Reporter, Offender> {
 	fn can_report() -> bool;
 }
 
-impl<Reporter, Offender> OnOffenceHandler<Reporter, Offender> for () {
+impl<Reporter, Offender, Res: Default> OnOffenceHandler<Reporter, Offender, Res> for () {
 	fn on_offence(
 		_offenders: &[OffenceDetails<Reporter, Offender>],
 		_slash_fraction: &[Perbill],
 		_session: SessionIndex,
-	) -> Result<(), ()> { Ok(()) }
+	) -> Result<Res, ()> {
+		Ok(Default::default())
+	}
 
 	fn can_report() -> bool { true }
 }