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

Adds ability to clear obsolete reports in Offences pallet #14048

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
b1b843d
Initial setup
gupnik Apr 30, 2023
72564c2
Adds ability to clear old reportS
gupnik Apr 30, 2023
ecfe969
Adds session change listener
gupnik May 4, 2023
b6546b5
Adds session info provider
gupnik May 5, 2023
c666aa0
Updates params everywhere
gupnik May 5, 2023
9bbe6e3
Updates params everywhere
gupnik May 5, 2023
5b4954e
adds impl for ()
gupnik May 5, 2023
8e97ca6
Adds doc
gupnik May 5, 2023
d3b7560
Minor change
gupnik May 5, 2023
81c4f99
Adds max session age everywhere
gupnik May 5, 2023
7927788
".git/.scripts/commands/fmt/fmt.sh"
May 5, 2023
8176c15
Addresses review comments
gupnik May 5, 2023
544ed3e
Minor update
gupnik May 5, 2023
217a484
Updates doc
gupnik May 5, 2023
5bae7e1
".git/.scripts/commands/fmt/fmt.sh"
May 5, 2023
74accc2
Adds doc
gupnik May 9, 2023
15b8f46
Uses map to store session reports
gupnik May 10, 2023
cd23321
Updates doc
gupnik May 10, 2023
1fa5c13
".git/.scripts/commands/fmt/fmt.sh"
May 10, 2023
8f62f4c
Merge branch 'master' of github.com:paritytech/substrate into gupnik/…
gupnik May 10, 2023
4a334f5
Fixes doc issue
gupnik May 10, 2023
6b9bc38
Trivial updates
gupnik May 10, 2023
e6e2f1d
Merge branch 'master' of github.com:paritytech/substrate into gupnik/…
gupnik May 10, 2023
003127f
Merge branch 'master' of github.com:paritytech/substrate into gupnik/…
gupnik May 10, 2023
d085786
Merge branch 'master' of github.com:paritytech/substrate into gupnik/…
gupnik May 11, 2023
d84cf00
Addresses some review comments
gupnik May 17, 2023
42c4d90
Addresses more review comments
gupnik May 17, 2023
6ac1857
Uses const for session index
gupnik May 17, 2023
30df0ed
Update frame/offences/src/lib.rs
gupnik Jul 3, 2023
b4be535
Update frame/offences/src/lib.rs
gupnik Jul 3, 2023
8fde92c
Update frame/offences/src/lib.rs
gupnik Jul 3, 2023
5f0a4f9
Update primitives/session/Cargo.toml
gupnik Jul 3, 2023
62f7c65
Merge branch 'master' of github.com:paritytech/substrate into gupnik/…
gupnik Jul 3, 2023
fe5d2c7
Update frame/offences/src/lib.rs
gupnik Jul 3, 2023
392fca3
Addresses some review comments
gupnik Jul 3, 2023
509502b
Merge branch 'gupnik/offence-pallet-optimizations' of github.com:pari…
gupnik Jul 3, 2023
6a58757
Minor fix
gupnik Jul 3, 2023
4e2d07e
No need to use double encode for SessionReports
gupnik Jul 3, 2023
ed41f99
Formatting
gupnik Jul 3, 2023
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: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ impl pallet_session::Config for Runtime {
type NextSessionRotation = Babe;
type SessionManager = pallet_session::historical::NoteHistoricalRoot<Self, Staking>;
type SessionHandler = <SessionKeys as OpaqueKeys>::KeyTypeIdProviders;
type SessionChangeListener = Offences;
type Keys = SessionKeys;
type WeightInfo = pallet_session::weights::SubstrateWeight<Runtime>;
}
Expand Down Expand Up @@ -1331,10 +1332,16 @@ impl pallet_im_online::Config for Runtime {
type MaxPeerDataEncodingSize = MaxPeerDataEncodingSize;
}

parameter_types! {
pub const MaxSessionReportAge: u32 = BondingDuration::get() * SessionsPerEra::get();
}

impl pallet_offences::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type IdentificationTuple = pallet_session::historical::IdentificationTuple<Self>;
type OnOffenceHandler = Staking;
type MaxSessionReportAge = MaxSessionReportAge;
type SessionInfoProvider = Session;
}

impl pallet_authority_discovery::Config for Runtime {
Expand Down
1 change: 1 addition & 0 deletions frame/authority-discovery/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ mod tests {
type ValidatorIdOf = ConvertInto;
type NextSessionRotation = pallet_session::PeriodicSessions<Period, Offset>;
type WeightInfo = ();
type SessionChangeListener = ();
}

impl pallet_session::historical::Config for Test {
Expand Down
4 changes: 4 additions & 0 deletions frame/babe/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ impl pallet_session::Config for Test {
type SessionHandler = <MockSessionKeys as OpaqueKeys>::KeyTypeIdProviders;
type Keys = MockSessionKeys;
type WeightInfo = ();
type SessionChangeListener = ();
}

impl pallet_session::historical::Config for Test {
Expand Down Expand Up @@ -166,6 +167,7 @@ parameter_types! {
pub const SlashDeferDuration: EraIndex = 0;
pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE;
pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(16);
pub const MaxSessionReportAge: SessionIndex = BondingDuration::get() * SessionsPerEra::get();
}

pub struct OnChainSeqPhragmen;
Expand Down Expand Up @@ -213,6 +215,8 @@ impl pallet_offences::Config for Test {
type RuntimeEvent = RuntimeEvent;
type IdentificationTuple = pallet_session::historical::IdentificationTuple<Self>;
type OnOffenceHandler = Staking;
type SessionInfoProvider = Session;
type MaxSessionReportAge = MaxSessionReportAge;
}

parameter_types! {
Expand Down
1 change: 1 addition & 0 deletions frame/beefy-mmr/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ impl pallet_session::Config for Test {
type SessionHandler = <MockSessionKeys as OpaqueKeys>::KeyTypeIdProviders;
type Keys = MockSessionKeys;
type WeightInfo = ();
type SessionChangeListener = ();
}

pub type MmrLeaf = sp_consensus_beefy::mmr::MmrLeaf<
Expand Down
4 changes: 4 additions & 0 deletions frame/beefy/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ impl pallet_session::Config for Test {
type SessionHandler = <MockSessionKeys as OpaqueKeys>::KeyTypeIdProviders;
type Keys = MockSessionKeys;
type WeightInfo = ();
type SessionChangeListener = ();
}

impl pallet_session::historical::Config for Test {
Expand Down Expand Up @@ -190,6 +191,7 @@ parameter_types! {
pub const BondingDuration: EraIndex = 3;
pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE;
pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(17);
pub const MaxSessionReportAge: SessionIndex = BondingDuration::get() * SessionsPerEra::get();
}

pub struct OnChainSeqPhragmen;
Expand Down Expand Up @@ -237,6 +239,8 @@ impl pallet_offences::Config for Test {
type RuntimeEvent = RuntimeEvent;
type IdentificationTuple = pallet_session::historical::IdentificationTuple<Self>;
type OnOffenceHandler = Staking;
type SessionInfoProvider = Session;
type MaxSessionReportAge = MaxSessionReportAge;
}

// Note, that we can't use `UintAuthorityId` here. Reason is that the implementation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ impl pallet_session::Config for Runtime {
type ValidatorId = AccountId;
type ValidatorIdOf = pallet_staking::StashOf<Runtime>;
type WeightInfo = ();
type SessionChangeListener = ();
}
impl pallet_session::historical::Config for Runtime {
type FullIdentification = pallet_staking::Exposure<AccountId, Balance>;
Expand Down
4 changes: 4 additions & 0 deletions frame/grandpa/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ impl pallet_session::Config for Test {
type SessionHandler = <TestSessionKeys as OpaqueKeys>::KeyTypeIdProviders;
type Keys = TestSessionKeys;
type WeightInfo = ();
type SessionChangeListener = ();
}

impl pallet_session::historical::Config for Test {
Expand Down Expand Up @@ -171,6 +172,7 @@ parameter_types! {
pub const BondingDuration: EraIndex = 3;
pub const RewardCurve: &'static PiecewiseLinear<'static> = &REWARD_CURVE;
pub const OffendingValidatorsThreshold: Perbill = Perbill::from_percent(17);
pub const MaxSessionReportAge: SessionIndex = BondingDuration::get() * SessionsPerEra::get();
}

pub struct OnChainSeqPhragmen;
Expand Down Expand Up @@ -218,6 +220,8 @@ impl pallet_offences::Config for Test {
type RuntimeEvent = RuntimeEvent;
type IdentificationTuple = pallet_session::historical::IdentificationTuple<Self>;
type OnOffenceHandler = Staking;
type SessionInfoProvider = Session;
type MaxSessionReportAge = MaxSessionReportAge;
}

parameter_types! {
Expand Down
1 change: 1 addition & 0 deletions frame/im-online/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ impl pallet_session::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type NextSessionRotation = pallet_session::PeriodicSessions<Period, Offset>;
type WeightInfo = ();
type SessionChangeListener = ();
}

impl pallet_session::historical::Config for Runtime {
Expand Down
1 change: 1 addition & 0 deletions frame/offences/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ frame-system = { version = "4.0.0-dev", default-features = false, path = "../sys
pallet-balances = { version = "4.0.0-dev", default-features = false, path = "../balances" }
sp-runtime = { version = "7.0.0", default-features = false, path = "../../primitives/runtime" }
sp-staking = { version = "4.0.0-dev", default-features = false, path = "../../primitives/staking" }
sp-session = { version = "4.0.0-dev", default-features = false, path = "../../primitives/session" }
sp-std = { version = "5.0.0", default-features = false, path = "../../primitives/std" }

[dev-dependencies]
Expand Down
3 changes: 3 additions & 0 deletions frame/offences/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ impl pallet_session::Config for Test {
type ValidatorId = AccountId;
type ValidatorIdOf = pallet_staking::StashOf<Test>;
type WeightInfo = ();
type SessionChangeListener = ();
}

pallet_staking_reward_curve::build! {
Expand Down Expand Up @@ -204,6 +205,8 @@ impl pallet_offences::Config for Test {
type RuntimeEvent = RuntimeEvent;
type IdentificationTuple = pallet_session::historical::IdentificationTuple<Self>;
type OnOffenceHandler = Staking;
type SessionInfoProvider = Session;
type MaxSessionReportAge = ConstU32<6>;
}

impl<T> frame_system::offchain::SendTransactionTypes<T> for Test
Expand Down
124 changes: 112 additions & 12 deletions frame/offences/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,24 @@

//! # Offences Pallet
//!
//! Tracks reported offences
//! The offences pallet tracks reported offences using 3 key storage types:
//!
//! - [Reports]: A storage map of a report hash to its details
gupnik marked this conversation as resolved.
Show resolved Hide resolved
//! - [ConcurrentReportsIndex]: A storage double map that stores a vector of reports
//! for a specific [Kind] and [OpaqueTimeSlot]
//! - [SessionReports]: A storage map that keeps a vector of active reports for a [SessionIndex].
//!
//! When a new offence is reported using [ReportOffence::report_offence], its `session_index` is
//! first compared against the current `session_index`.
//!
//! If older than [Config::MaxSessionReportAge], the report is rejected right away.
//!
//! Else, all concurrent reports are loaded to determine the slash fraction and updated.
//! The report is also inserted in [SessionReports] at this time.
//! Finally, [Config::OnOffenceHandler] is called to handle any actions for this report.
//!
//! On the start of a new session, `clear_obsolete_reports` clears all reports
//! that are older than [Config::MaxSessionReportAge].

// Ensure we're `no_std` when compiling for Wasm.
#![cfg_attr(not(feature = "std"), no_std)]
Expand All @@ -26,21 +43,21 @@ pub mod migration;
mod mock;
mod tests;

use codec::{Decode, Encode};
use core::marker::PhantomData;

use codec::Encode;
use frame_support::weights::Weight;
use frame_support::{traits::Get, weights::Weight};
use sp_runtime::{traits::Hash, Perbill};
use sp_session::{SessionChangeListener, SessionInfoProvider};
use sp_staking::{
offence::{Kind, Offence, OffenceDetails, OffenceError, OnOffenceHandler, ReportOffence},
SessionIndex,
EraIndex, SessionIndex,
};
use sp_std::prelude::*;

pub use pallet::*;

/// A binary blob which represents a SCALE codec-encoded `O::TimeSlot`.
type OpaqueTimeSlot = Vec<u8>;
pub type OpaqueTimeSlot = Vec<u8>;

/// A type alias for a report identifier.
type ReportIdOf<T> = <T as frame_system::Config>::Hash;
Expand Down Expand Up @@ -68,6 +85,14 @@ pub mod pallet {
type IdentificationTuple: Parameter;
/// A handler called for every offence report.
type OnOffenceHandler: OnOffenceHandler<Self::AccountId, Self::IdentificationTuple, Weight>;

/// Number of sessions for which a report is stored.
/// Once it gets older than this value, it gets cleaned up at the start of a new session.
#[pallet::constant]
type MaxSessionReportAge: Get<EraIndex>;
gupnik marked this conversation as resolved.
Show resolved Hide resolved

/// A trait that provides information about the current session
gupnik marked this conversation as resolved.
Show resolved Hide resolved
type SessionInfoProvider: SessionInfoProvider;
}

/// The primary structure that holds all offence records keyed by report identifiers.
Expand All @@ -92,6 +117,22 @@ pub mod pallet {
ValueQuery,
>;

/// A map that stores all reports along with
Copy link
Member

Choose a reason for hiding this comment

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

Please try to break lines at 100 chars.
I am not sure if the formatter does it, but many editors support a vertical line that makes it easy to do it manually.

/// their kind and time slot info for a`SessionIndex`.
gupnik marked this conversation as resolved.
Show resolved Hide resolved
///
/// On start of a new session, all reports with a session index older than
/// `MaxSessionReportAge` are removed.
///
/// Note that time_slot is encoded as stored as an opaque type.
gupnik marked this conversation as resolved.
Show resolved Hide resolved
#[pallet::storage]
pub type SessionReports<T: Config> = StorageMap<
_,
Twox64Concat,
SessionIndex,
Vec<u8>, // Vec<(Kind, OpaqueTimeSlot, ReportIdOf<T>)>
gupnik marked this conversation as resolved.
Show resolved Hide resolved
ValueQuery,
>;

/// Events type.
#[pallet::event]
#[pallet::generate_deposit(pub(super) fn deposit_event)]
Expand All @@ -103,6 +144,37 @@ pub mod pallet {
}
}

impl<T: Config> Pallet<T> {
/// This is called at the start of each new session. Hence, only one session becomes
/// obsolete in this call, whose data gets cleared up here.
///
/// For example, if the `current_session_index` is 10 and `MaxSessionReportAge` is 6,
/// this clears all reports for `obsolete_session_index` 4.
fn clear_obsolete_reports(current_session_index: SessionIndex) {
if current_session_index <= T::MaxSessionReportAge::get() {
return
}

let obsolete_session_index = current_session_index - T::MaxSessionReportAge::get();
gupnik marked this conversation as resolved.
Show resolved Hide resolved

let session_reports = SessionReports::<T>::take(obsolete_session_index);
let session_reports =
Vec::<(Kind, OpaqueTimeSlot, ReportIdOf<T>)>::decode(&mut &session_reports[..])
.unwrap_or_default();
Copy link
Contributor

Choose a reason for hiding this comment

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

and why unwrap_or_default? if we go to default, there must have been a bug, no? then it can be a defensive_unwrap_or_default, or an early return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will go to default when the vector is empty?

Copy link
Member

Choose a reason for hiding this comment

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

No. If it is empty it just returns Ok(vec![]), an error here means that the storage is corrupted.

DQ but why is this double-encode anyway? Why not store Vec<StorageReportOf<T>>?


for (kind, time_slot, report_id) in &session_reports {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any upper limit to the length of this vector?
We always need to keep the weight of on_initialize bounded…

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, I'd expect that the session reports can be very large, especially if there's a bug. We should be able to clear the offences through governance when these are resulting from a bug, but still it's better to bound a loop on_initialize.

Perhaps we should instead queue clearing offences and process a fixed number on_initialize, until the queue is empty (simple multi-block clearing process).

ConcurrentReportsIndex::<T>::remove(kind, time_slot);
Reports::<T>::remove(report_id);
}
}
}

impl<T: Config> SessionChangeListener for Pallet<T> {
fn on_session_change(session_index: SessionIndex) {
Self::clear_obsolete_reports(session_index)
}
}

impl<T, O> ReportOffence<T::AccountId, T::IdentificationTuple, O> for Pallet<T>
where
T: Config,
Expand All @@ -112,10 +184,20 @@ where
let offenders = offence.offenders();
let time_slot = offence.time_slot();

let session_index = offence.session_index();
let current_session_index = T::SessionInfoProvider::current_session_index();

if current_session_index > T::MaxSessionReportAge::get() &&
gupnik marked this conversation as resolved.
Show resolved Hide resolved
session_index < current_session_index - T::MaxSessionReportAge::get()
{
return Err(OffenceError::ObsoleteReport)
}

// Go through all offenders in the offence report and find all offenders that were spotted
// in unique reports.
let TriageOutcome { concurrent_offenders } =
match Self::triage_offence_report::<O>(reporters, &time_slot, offenders) {
match Self::triage_offence_report::<O>(reporters, &time_slot, session_index, offenders)
{
Some(triage) => triage,
// The report contained only duplicates, so there is no need to slash again.
None => return Err(OffenceError::DuplicateReport),
Expand Down Expand Up @@ -167,9 +249,10 @@ impl<T: Config> Pallet<T> {
fn triage_offence_report<O: Offence<T::IdentificationTuple>>(
reporters: Vec<T::AccountId>,
time_slot: &O::TimeSlot,
session_index: SessionIndex,
offenders: Vec<T::IdentificationTuple>,
) -> Option<TriageOutcome<T>> {
let mut storage = ReportIndexStorage::<T, O>::load(time_slot);
let mut storage = ReportIndexStorage::<T, O>::load(time_slot, session_index);

let mut any_new = false;
for offender in offenders {
Expand All @@ -182,7 +265,7 @@ impl<T: Config> Pallet<T> {
OffenceDetails { offender, reporters: reporters.clone() },
);

storage.insert(report_id);
storage.insert(report_id, &time_slot);
}
}

Expand Down Expand Up @@ -216,28 +299,45 @@ struct TriageOutcome<T: Config> {
#[must_use = "The changes are not saved without called `save`"]
struct ReportIndexStorage<T: Config, O: Offence<T::IdentificationTuple>> {
opaque_time_slot: OpaqueTimeSlot,
session_index: SessionIndex,
concurrent_reports: Vec<ReportIdOf<T>>,
session_reports: Vec<(Kind, OpaqueTimeSlot, ReportIdOf<T>)>,
_phantom: PhantomData<O>,
}

impl<T: Config, O: Offence<T::IdentificationTuple>> ReportIndexStorage<T, O> {
/// Preload indexes from the storage for the specific `time_slot` and the kind of the offence.
fn load(time_slot: &O::TimeSlot) -> Self {
fn load(time_slot: &O::TimeSlot, session_index: SessionIndex) -> Self {
gupnik marked this conversation as resolved.
Show resolved Hide resolved
let opaque_time_slot = time_slot.encode();

let session_reports = SessionReports::<T>::get(session_index);
let session_reports =
Vec::<(Kind, OpaqueTimeSlot, ReportIdOf<T>)>::decode(&mut &session_reports[..])
.unwrap_or_default();
gupnik marked this conversation as resolved.
Show resolved Hide resolved

let concurrent_reports = <ConcurrentReportsIndex<T>>::get(&O::ID, &opaque_time_slot);

Self { opaque_time_slot, concurrent_reports, _phantom: Default::default() }
Self {
opaque_time_slot,
session_index,
concurrent_reports,
session_reports,
_phantom: Default::default(),
}
}

/// Insert a new report to the index.
fn insert(&mut self, report_id: ReportIdOf<T>) {
fn insert(&mut self, report_id: ReportIdOf<T>, time_slot: &O::TimeSlot) {
let opaque_time_slot = time_slot.encode();
self.session_reports.push((O::ID, opaque_time_slot, report_id));

// Update the list of concurrent reports.
self.concurrent_reports.push(report_id);
}

/// Dump the indexes to the storage.
fn save(self) {
SessionReports::<T>::set(self.session_index, self.session_reports.encode());
<ConcurrentReportsIndex<T>>::insert(
&O::ID,
&self.opaque_time_slot,
Expand Down
Loading