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

Offences report system rework #13425

Merged

Conversation

davxy
Copy link
Member

@davxy davxy commented Feb 20, 2023

This PR aims to provide a better abstraction to handle and report equivocations/offences.

The refactory introduces a new trait OffenceReportSystem that should be used when a component requires a higher level subsystem to wrap some of the operations typically required during an offence report:

  1. check the equivocation validity
  2. process and offence evidence
  3. publish an offence evidence

The introduced trait usage is not limited to the consensus equivocation reports but can be used for any type of offence that requires these three typical actions

Furthermore the refactory removes some burden from the user during instantiation of the pallet by providing a simpler interface.


Beside the simplification and encapsulation of some of the procedures, the PR also wants to be a starting point for:

  1. further code refactory
  2. equivocation report system code sharing between babe, aura (not yet provided) and sassafras (these three will end up using the same offence report system instance that now is used by babe)

This trait can also be used by Beefy to report equivocations


polkadot companion: paritytech/polkadot#6784

@davxy davxy added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit D2-breaksapi and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Feb 20, 2023
Copy link
Contributor

@altonen altonen left a comment

Choose a reason for hiding this comment

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

It looks good to me but I'm not very familiar with this area of the codebase

frame/babe/src/equivocation.rs Show resolved Hide resolved
@altonen altonen requested a review from a team February 27, 2023 09:41
frame/babe/src/equivocation.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

lgtm

primitives/consensus/grandpa/src/lib.rs Show resolved Hide resolved
primitives/staking/src/offence.rs Show resolved Hide resolved
frame/grandpa/src/equivocation.rs Outdated Show resolved Hide resolved
@davxy
Copy link
Member Author

davxy commented Mar 7, 2023

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#6784

@davxy
Copy link
Member Author

davxy commented Mar 7, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for 643c745

@davxy
Copy link
Member Author

davxy commented Mar 7, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 851e73a into paritytech:master Mar 7, 2023
@davxy davxy deleted the equivocation-offence-rework branch March 7, 2023 20:32
@nazar-pc nazar-pc mentioned this pull request Apr 2, 2023
1 task
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
* Experiments with common equivocation trait

* Improved equivocation trait

* Fix grandpa equivocation implementation

* Remove some cruft

* Remove some more cruft

* More generic naming

* Simplification of offences manipilation

* More refactory

* Some prograss with the encapsulation of offence report system

* Finally unit type works as a universal null report system

* Align substrate node code

* Further simplification

* Fix test utils

* Remove not required associated type

* Fix benches

* Rollback to prev field name

* Box big params

* Fix typo

* Remove new tag computation

* Remove default implementations

* Better docs

* Return 'Result' instead of bool

* Change offence report system return types

* Some renaming and documentation

* Improve documentation

* More abstract offence report system

* Rename 'consume_evidence' to 'process_evidence'

* Further docs refinements

* Doc for dummy offence report

* Fix rustdoc

* Fix after master merge

* Apply code review suggestions

* Improve docs
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Experiments with common equivocation trait

* Improved equivocation trait

* Fix grandpa equivocation implementation

* Remove some cruft

* Remove some more cruft

* More generic naming

* Simplification of offences manipilation

* More refactory

* Some prograss with the encapsulation of offence report system

* Finally unit type works as a universal null report system

* Align substrate node code

* Further simplification

* Fix test utils

* Remove not required associated type

* Fix benches

* Rollback to prev field name

* Box big params

* Fix typo

* Remove new tag computation

* Remove default implementations

* Better docs

* Return 'Result' instead of bool

* Change offence report system return types

* Some renaming and documentation

* Improve documentation

* More abstract offence report system

* Rename 'consume_evidence' to 'process_evidence'

* Further docs refinements

* Doc for dummy offence report

* Fix rustdoc

* Fix after master merge

* Apply code review suggestions

* Improve docs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants