From 8539b85c99fd3b216bf428724756b62efd6bc202 Mon Sep 17 00:00:00 2001
From: Fedor Sakharov <fedor.sakharov@gmail.com>
Date: Mon, 2 Mar 2020 13:00:38 +0300
Subject: [PATCH] Offence reporting returns a result (#5082)
* Offence reporting returns a result
* Bump spec_version
* Use unwrap instead of assertions
* Fix more review grumbles
---
substrate/bin/node/runtime/src/lib.rs | 2 +-
substrate/frame/im-online/src/lib.rs | 4 ++-
substrate/frame/im-online/src/mock.rs | 5 ++--
substrate/frame/offences/src/lib.rs | 8 +++---
substrate/frame/offences/src/tests.rs | 20 +++++++--------
substrate/frame/staking/src/lib.rs | 7 +++---
substrate/primitives/staking/src/offence.rs | 27 +++++++++++++++++++--
7 files changed, 51 insertions(+), 22 deletions(-)
diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs
index ae8251f6209..85889a50c20 100644
--- a/substrate/bin/node/runtime/src/lib.rs
+++ b/substrate/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,
};
diff --git a/substrate/frame/im-online/src/lib.rs b/substrate/frame/im-online/src/lib.rs
index d6d13c66c25..9aa8d2d67f6 100644
--- a/substrate/frame/im-online/src/lib.rs
+++ b/substrate/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);
+ }
}
}
diff --git a/substrate/frame/im-online/src/mock.rs b/substrate/frame/im-online/src/mock.rs
index a703b24629c..97a0e7eb844 100644
--- a/substrate/frame/im-online/src/mock.rs
+++ b/substrate/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(())
}
}
diff --git a/substrate/frame/offences/src/lib.rs b/substrate/frame/offences/src/lib.rs
index 7831ba65a3b..27983cbb533 100644
--- a/substrate/frame/offences/src/lib.rs
+++ b/substrate/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(())
}
}
diff --git a/substrate/frame/offences/src/tests.rs b/substrate/frame/offences/src/tests.rs
index f2f82cf7a87..0ed98427c65 100644
--- a/substrate/frame/offences/src/tests.rs
+++ b/substrate/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,7 +61,7 @@ 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();
@@ -69,7 +69,7 @@ fn should_not_report_the_same_authority_twice_in_the_same_slot() {
// 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,7 +149,7 @@ 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();
@@ -157,7 +157,7 @@ fn doesnt_deposit_event_for_dups() {
// 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,7 +191,7 @@ 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();
@@ -199,7 +199,7 @@ fn should_properly_count_offences() {
// 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.
diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs
index ee708dabd3c..81bce3b5487 100644
--- a/substrate/frame/staking/src/lib.rs
+++ b/substrate/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(())
}
}
}
diff --git a/substrate/primitives/staking/src/offence.rs b/substrate/primitives/staking/src/offence.rs
index cac1ed065ce..06e73f018b7 100644
--- a/substrate/primitives/staking/src/offence.rs
+++ b/substrate/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.
--
GitLab