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

Conversation

@niklasad1
Copy link
Contributor

@niklasad1 niklasad1 commented Jul 17, 2019

Hey,

This PR is the next attempt to introduce the slashing aggregator "partly"

What does this

It enables the functionality of what to do each severity level instead of specifying strictly based on misbehavior.

It introducing the following:

  • Slashing module for keeping track of history and slash.
  • AfterSlashing implementation for Staking
  • Generic interface ReportSlash, DoSlash and DoReward
  • RollingWindow Module

By introducing the traits ReportSlash and DoSlash for a generic interface to slash misbehavior(s) with support of a RollingWindow which will keep the last n sessions of misbehaviors. The number of sessions is determined by the misconduct type.

Additionally, the basic Misconducts kinds are pre-defined in the runtime with a window length as input which needs to be initialized by the concrete misconduct such as BabeEquivocation. So, for example, GrandpaEquivocation may need a different window than BabeEquivocation and so on.

However, it is assumed that the window_length and base_severity is static for the concrete misconduct. I have two different macros for them but they are basically the same with different function names because I think it will be easier to use and you might not need both,

See the example use-case below which is probably easier to understand

What is missing

  • Misbehavior reporting
  • Misbehavior implementations (such as BabeEquivocation, Unresponsiveness and similar)
  • macro for ReportSlash (currently very boilerplate-isch)

Example use-case

See test implementation in substrate/srml/staking/src/slash.rs

@parity-cla-bot
Copy link

It looks like @niklasad1 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

1 similar comment
@parity-cla-bot
Copy link

It looks like @niklasad1 signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@niklasad1 niklasad1 requested a review from rphmeier July 17, 2019 08:36
@niklasad1 niklasad1 force-pushed the na-robs-suggest-slash branch from 28dd5be to fa2f9c1 Compare July 31, 2019 22:22
@marcio-diaz
Copy link
Contributor

I see that you added quite a few more things. Is it ready for another round of reviews? something missing?

This commit splits the `DoSlash` trait in two traits: `DoReward` and `DoSlash`

The reasons for it, is that we want reward based on the `total slashed amount`
but the entire amount doesn't necessarily be rewarded. Some part of the total
slashed amount might need to transmitted to some other part such as
`treasory`.

Thus, instead we return the total slash and `ReportSlashing` has to determine
what to do with the total slashed amount.

In other words, the concrete misconduct type determines what to do.
@niklasad1
Copy link
Contributor Author

@marcio-diaz it is close, will probably be reviewable tomorrow

Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

Nit: docs ranting

/// Trait for representing window length
pub trait WindowLength<T> {
/// Fetch window length
fn window_length(&self) -> &T;
Copy link
Contributor

Choose a reason for hiding this comment

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

fn(&self) -> &T implies that T should live inside of self, but the usage of trait looks like it should be returned by value.

}

/// A generic trait for paying out rewards
pub trait DoReward<Reporters, Reward> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could mention that it is related to slashing. Otherwise it looks like generic rewarding trait. Alternatively, we could rename it to something like "do reward reporters".

@niklasad1 niklasad1 force-pushed the na-robs-suggest-slash branch from 22bb264 to 8e3b804 Compare August 2, 2019 18:02
@niklasad1 niklasad1 added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Aug 2, 2019
@gavofyork gavofyork mentioned this pull request Aug 3, 2019
8 tasks
/// Each variant and its data is a seperate kind
#[derive(Copy, Clone, Eq, Hash, PartialEq, Encode, Decode)]
#[cfg_attr(feature = "std", derive(Serialize, Deserialize, Debug))]
pub enum Misbehavior {
Copy link
Member

Choose a reason for hiding this comment

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

This type shouldn't exist.

@gavofyork
Copy link
Member

gavofyork commented Aug 3, 2019

it's looking better, however there's still a structural issue with that fat enum which expects you to manually enumerate all possible offences. instead, just restrict offences to types that implement a trait to an associated type of the configuration trait. the offence trait can include a
const ID: [u8; 16] (to identify it for using as a map key) and const WINDOW_SIZE: u32 as well as a function to determine how much the slash should be fn slash_amount(offender_count: u32, total: u32) -> Self::Balance.

@gavofyork gavofyork added A4-gotissues and removed A0-please_review Pull request needs code review. labels Aug 3, 2019
@niklasad1
Copy link
Contributor Author

niklasad1 commented Aug 4, 2019

Thanks Gav

However, I'm not sure that fn slash_amount(offender_count: u32, balance: u32) -> Self::Balance is what we want because it would require us to have an API to retrieve the entire slashing history for a certain kind (currently handled by slash mod) and ReportSlash would need the concrete type of the slashing victim and its exposure.

Thus, ReportSlash impl would be more complex and I have tried to keep it as simple as possible so far(i.e, just pass data and call some functions)

Well, I can try to implement it and see it how goes...

@niklasad1 niklasad1 closed this Aug 6, 2019
@bkchr bkchr deleted the na-robs-suggest-slash branch August 6, 2019 17:39
@gnunicorn gnunicorn modified the milestones: 2.1, Polkadot Mar 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.