Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Offence reporting returns a result #5082

Merged
merged 4 commits into from
Mar 2, 2020
Merged

Offence reporting returns a result #5082

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2fccaa5
Offence reporting returns a result
montekki Feb 27, 2020
98a5c69
Bump spec_version
montekki Feb 27, 2020
1f683c3
Use unwrap instead of assertions
montekki Feb 28, 2020
ad1e2bf
Fix more review grumbles
montekki Feb 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,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,
};
Expand Down
7 changes: 6 additions & 1 deletion frame/im-online/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,12 @@ 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) {
debug::info!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this, it will never be printed (in wasm). This functionality is only for offchain workers (it requires that the RuntimeLogger is intialized). Better to just use sp_runtime::print.

Copy link
Contributor Author

@montekki montekki Feb 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the rest of the code in this module uses this format so I decided to keep it the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code you are referring too is offchain, that is the reason it uses this format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

target: "imonline",
"Failed to report an offence: {:?}", e,
);
}
}
}

Expand Down
5 changes: 3 additions & 2 deletions frame/im-online/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(())
}
}

Expand Down
8 changes: 5 additions & 3 deletions frame/offences/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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.
Expand All @@ -123,6 +123,8 @@ where
&slash_perbill,
offence.session_index(),
);

Ok(())
}
}

Expand Down
20 changes: 10 additions & 10 deletions frame/offences/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand All @@ -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| {
Expand All @@ -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();
Expand All @@ -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| {
Expand All @@ -123,7 +123,7 @@ fn should_deposit_event() {
};

// when
Offences::report_offence(vec![], offence);
Offences::report_offence(vec![], offence).unwrap();

// then
assert_eq!(
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
7 changes: 4 additions & 3 deletions frame/staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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.
Expand All @@ -1840,7 +1840,8 @@ impl<T, Reporter, Offender, R, O> ReportOffence<Reporter, Offender, O>
} else {
<Module<T>>::deposit_event(
RawEvent::OldSlashingReportDiscarded(offence_session)
)
);
Ok(())
}
}
}
11 changes: 9 additions & 2 deletions primitives/staking/src/offence.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,21 @@ 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we at least want to have a variant that may takes a u8 or whatever? Otherwise we will need to update this for every error that may come u.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that Other(u8) should be added when the need arises, r.n. it would be just an unused variant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remind you on your next pr :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added it here

}

/// 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.
Expand Down