Offence reporting returns a result (#5082)

* Offence reporting returns a result

* Bump spec_version

* Use unwrap instead of assertions

* Fix more review grumbles

Author: montekki

bin/node/runtime/src/lib.rs

@@ -82,7 +82,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to 0. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 226,
+	spec_version: 227,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 };

frame/im-online/src/lib.rs

@@ -608,7 +608,9 @@ impl<T: Trait> pallet_session::OneSessionHandler<T::AccountId> for Module<T> {
 
 			let validator_set_count = keys.len() as u32;
 			let offence = UnresponsivenessOffence { session_index, validator_set_count, offenders };
-			T::ReportUnresponsiveness::report_offence(vec![], offence);
+			if let Err(e) = T::ReportUnresponsiveness::report_offence(vec![], offence) {
+				sp_runtime::print(e);
+			}
 		}
 	}
 

frame/im-online/src/mock.rs

@@ -22,7 +22,7 @@ use std::cell::RefCell;
 
 use crate::{Module, Trait};
 use sp_runtime::Perbill;
-use sp_staking::{SessionIndex, offence::ReportOffence};
+use sp_staking::{SessionIndex, offence::{ReportOffence, OffenceError}};
 use sp_runtime::testing::{Header, UintAuthorityId, TestXt};
 use sp_runtime::traits::{IdentityLookup, BlakeTwo256, ConvertInto};
 use sp_core::H256;
@@ -77,8 +77,9 @@ thread_local! {
 /// A mock offence report handler.
 pub struct OffenceHandler;
 impl ReportOffence<u64, IdentificationTuple, Offence> for OffenceHandler {
-	fn report_offence(reporters: Vec<u64>, offence: Offence) {
+	fn report_offence(reporters: Vec<u64>, offence: Offence) -> Result<(), OffenceError> {
 		OFFENCES.with(|l| l.borrow_mut().push((reporters, offence)));
+		Ok(())
 	}
 }
 

frame/offences/src/lib.rs

@@ -30,7 +30,7 @@ use frame_support::{
 };
 use sp_runtime::traits::Hash;
 use sp_staking::{
-	offence::{Offence, ReportOffence, Kind, OnOffenceHandler, OffenceDetails},
+	offence::{Offence, ReportOffence, Kind, OnOffenceHandler, OffenceDetails, OffenceError},
 };
 use codec::{Encode, Decode};
 use frame_system as system;
@@ -90,7 +90,7 @@ impl<T: Trait, O: Offence<T::IdentificationTuple>>
 where
 	T::IdentificationTuple: Clone,
 {
-	fn report_offence(reporters: Vec<T::AccountId>, offence: O) {
+	fn report_offence(reporters: Vec<T::AccountId>, offence: O) -> Result<(), OffenceError> {
 		let offenders = offence.offenders();
 		let time_slot = offence.time_slot();
 		let validator_set_count = offence.validator_set_count();
@@ -104,7 +104,7 @@ where
 		) {
 			Some(triage) => triage,
 			// The report contained only duplicates, so there is no need to slash again.
-			None => return,
+			None => return Err(OffenceError::DuplicateReport),
 		};
 
 		// Deposit the event.
@@ -123,6 +123,8 @@ where
 			&slash_perbill,
 			offence.session_index(),
 		);
+
+		Ok(())
 	}
 }
 

frame/offences/src/tests.rs

@@ -40,7 +40,7 @@ fn should_report_an_authority_and_trigger_on_offence() {
 		};
 
 		// when
-		Offences::report_offence(vec![], offence);
+		Offences::report_offence(vec![], offence).unwrap();
 
 		// then
 		with_on_offence_fractions(|f| {
@@ -61,15 +61,15 @@ fn should_not_report_the_same_authority_twice_in_the_same_slot() {
 			time_slot,
 			offenders: vec![5],
 		};
-		Offences::report_offence(vec![], offence.clone());
+		Offences::report_offence(vec![], offence.clone()).unwrap();
 		with_on_offence_fractions(|f| {
 			assert_eq!(f.clone(), vec![Perbill::from_percent(25)]);
 			f.clear();
 		});
 
 		// when
 		// report for the second time
-		Offences::report_offence(vec![], offence);
+		assert_eq!(Offences::report_offence(vec![], offence), Err(OffenceError::DuplicateReport));
 
 		// then
 		with_on_offence_fractions(|f| {
@@ -91,7 +91,7 @@ fn should_report_in_different_time_slot() {
 			time_slot,
 			offenders: vec![5],
 		};
-		Offences::report_offence(vec![], offence.clone());
+		Offences::report_offence(vec![], offence.clone()).unwrap();
 		with_on_offence_fractions(|f| {
 			assert_eq!(f.clone(), vec![Perbill::from_percent(25)]);
 			f.clear();
@@ -100,7 +100,7 @@ fn should_report_in_different_time_slot() {
 		// when
 		// report for the second time
 		offence.time_slot += 1;
-		Offences::report_offence(vec![], offence);
+		Offences::report_offence(vec![], offence).unwrap();
 
 		// then
 		with_on_offence_fractions(|f| {
@@ -123,7 +123,7 @@ fn should_deposit_event() {
 		};
 
 		// when
-		Offences::report_offence(vec![], offence);
+		Offences::report_offence(vec![], offence).unwrap();
 
 		// then
 		assert_eq!(
@@ -149,15 +149,15 @@ fn doesnt_deposit_event_for_dups() {
 			time_slot,
 			offenders: vec![5],
 		};
-		Offences::report_offence(vec![], offence.clone());
+		Offences::report_offence(vec![], offence.clone()).unwrap();
 		with_on_offence_fractions(|f| {
 			assert_eq!(f.clone(), vec![Perbill::from_percent(25)]);
 			f.clear();
 		});
 
 		// when
 		// report for the second time
-		Offences::report_offence(vec![], offence);
+		assert_eq!(Offences::report_offence(vec![], offence), Err(OffenceError::DuplicateReport));
 
 		// then
 		// there is only one event.
@@ -191,15 +191,15 @@ fn should_properly_count_offences() {
 			time_slot,
 			offenders: vec![4],
 		};
-		Offences::report_offence(vec![], offence1);
+		Offences::report_offence(vec![], offence1).unwrap();
 		with_on_offence_fractions(|f| {
 			assert_eq!(f.clone(), vec![Perbill::from_percent(25)]);
 			f.clear();
 		});
 
 		// when
 		// report for the second time
-		Offences::report_offence(vec![], offence2);
+		Offences::report_offence(vec![], offence2).unwrap();
 
 		// then
 		// the 1st authority should have count 2 and the 2nd one should be reported only once.

frame/staking/src/lib.rs

@@ -276,7 +276,7 @@ use sp_runtime::{
 };
 use sp_staking::{
 	SessionIndex,
-	offence::{OnOffenceHandler, OffenceDetails, Offence, ReportOffence},
+	offence::{OnOffenceHandler, OffenceDetails, Offence, ReportOffence, OffenceError},
 };
 #[cfg(feature = "std")]
 use sp_runtime::{Serialize, Deserialize};
@@ -1828,7 +1828,7 @@ impl<T, Reporter, Offender, R, O> ReportOffence<Reporter, Offender, O>
 	R: ReportOffence<Reporter, Offender, O>,
 	O: Offence<Offender>,
 {
-	fn report_offence(reporters: Vec<Reporter>, offence: O) {
+	fn report_offence(reporters: Vec<Reporter>, offence: O) -> Result<(), OffenceError> {
 		<Module<T>>::ensure_storage_upgraded();
 
 		// disallow any slashing from before the current bonding period.
@@ -1840,7 +1840,8 @@ impl<T, Reporter, Offender, R, O> ReportOffence<Reporter, Offender, O>
 		} else {
 			<Module<T>>::deposit_event(
 				RawEvent::OldSlashingReportDiscarded(offence_session)
-			)
+			);
+			Ok(())
 		}
 	}
 }

primitives/staking/src/offence.rs

@@ -91,14 +91,37 @@ pub trait Offence<Offender> {
 	) -> Perbill;
 }
 
+/// Errors that may happen on offence reports.
+#[derive(PartialEq, sp_runtime::RuntimeDebug)]
+pub enum OffenceError {
+	/// The report has already been sumbmitted.
+	DuplicateReport,
+
+	/// Other error has happened.
+	Other(u8),
+}
+
+impl sp_runtime::traits::Printable for OffenceError {
+	fn print(&self) {
+		"OffenceError".print();
+		match self {
+			Self::DuplicateReport => "DuplicateReport".print(),
+			Self::Other(e) => {
+				"Other".print();
+				e.print();
+			}
+		}
+	}
+}
+
 /// A trait for decoupling offence reporters from the actual handling of offence reports.
 pub trait ReportOffence<Reporter, Offender, O: Offence<Offender>> {
 	/// Report an `offence` and reward given `reporters`.
-	fn report_offence(reporters: Vec<Reporter>, offence: O);
+	fn report_offence(reporters: Vec<Reporter>, offence: O) -> Result<(), OffenceError>;
 }
 
 impl<Reporter, Offender, O: Offence<Offender>> ReportOffence<Reporter, Offender, O> for () {
-	fn report_offence(_reporters: Vec<Reporter>, _offence: O) {}
+	fn report_offence(_reporters: Vec<Reporter>, _offence: O) -> Result<(), OffenceError> { Ok(()) }
 }
 
 /// A trait to take action on an offence.