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

grandpa: report equivocations #3868

Merged
merged 102 commits into from
May 6, 2020
Merged

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Oct 21, 2019

This PR adds equivocation reporting for GRANDPA. Equivocation detection is implemented in finality-grandpa crate and exposed through Environment hooks (prevote_equivocation and precommit_equivocation).

The GRANDPA runtime module is extended with a report_equivocation method that takes an equivocation proof and a session membership proof. This method validates the membership proof using an abstract KeyOwnerProofSystem (historical session module currently) and the equivocation proof (different vote targets, valid signatures, etc.), if both are valid an offence is reported to an abstract ReportOffence type (offences module currently). Additionally, the method submit_report_equivocation_extrinsic creates and submits a signed transaction for reporting an equivocation.

From the node-side, when we want to report an equivocation we need to create the session membership proof. In particular, the membership proof can only be created at a block when the given validator is part of the current session. We figure out the last block at which the current set id is active in a given chain by looking at previously signalled authority changes and create the membership proof at that block. We then call submit_report_equivocation_extrinsic at the best block.

Important

  1. In order to report an equivocation we need to send a signed transaction, therefore we need to have some key available for this, taking into account that this is also the account that will be potentially rewarded. I created a new app-specific crypto ("fish") that should be used for all equivocation reporting tasks (this will also be used for BABE later on). Any comments on this?

  2. The current implementation for generating membership proofs requires having state available for the block on which the session where the equivocation happened was valid. This is not problematic right now since we require validators to run as archive. A future implementation could leverage offchain workers for generating full membership proof tries when a session ends and storing them locally.

TODO:

  • Publish finality-grandpa 0.9.1 (includes fixes for building under no-std)
  • Fix execution context of submit_report_equivocation_extrinsic call (needs to be offchain context but it's not easy to use that now outside of actual offchain workers)
  • Abstract srml/grandpa reporting logic to trait (so that we can implement it for () to disable reporting, needed to integrate in node-template)
  • Merge Add documentation to SubmitSignedTransaction and actually make it work #4200 and drop cherry-picked changes
  • Cleanup
  • Tests
  • Docs

polkadot companion: paritytech/polkadot#1000

@gavofyork gavofyork added A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage. labels Oct 23, 2019
@burdges
Copy link

burdges commented Dec 1, 2019

We only need the key signing a report transaction to be an account key so that it can pay some fee, right? We've no existing mechanism for actions signed by session keys to change their controller account fees yet? We could maybe permit free reports by the grandpa ed25519 key if the deduplication works well enough, not so universally applicable though. We can chat about this when you like. :)

…ctually make it work

Squashed commit of the following:

commit dc8d71c
Author: Tomasz Drwięga <tomasz@parity.io>
Date:   Tue Dec 3 16:29:33 2019 +0100

    Split the method to avoid confusing type error message.

commit 0c4c037
Author: Tomasz Drwięga <tomasz@parity.io>
Date:   Tue Dec 3 16:19:55 2019 +0100

    Make accounts optional, fix logic.

commit d715f64
Author: Tomasz Drwięga <tomasz@parity.io>
Date:   Tue Dec 3 10:06:20 2019 +0100

    Remove warning.

commit 3f38218
Merge: f85b890 368318c
Author: Tomasz Drwięga <tomasz@parity.io>
Date:   Tue Dec 3 07:08:05 2019 +0100

    Merge branch 'master' into td-signed-transactions

commit f85b890
Merge: f8c9540 d8d5da2
Author: Tomasz Drwięga <tomasz@parity.io>
Date:   Mon Dec 2 13:57:25 2019 +0100

    Merge branch 'master' into td-signed-transactions

commit f8c9540
Author: Tomasz Drwięga <tomasz@parity.io>
Date:   Mon Nov 25 17:34:52 2019 +0100

    Forgotten import.

commit a645b90
Author: Tomasz Drwięga <tomasz@parity.io>
Date:   Mon Nov 25 17:32:10 2019 +0100

    Fix naming and bounds.

commit bc28c60
Author: Tomasz Drwięga <tomasz@parity.io>
Date:   Mon Nov 25 17:01:05 2019 +0100

    Add documentation to signed transactions and actually make them work.
@tomusdrw tomusdrw removed their request for review April 28, 2020 09:15
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

Just took a look at all commits since my last review. LGTM!

@andresilva andresilva merged commit a2512e8 into master May 6, 2020
@andresilva andresilva deleted the andre/report-grandpa-equivocations branch May 6, 2020 16:25
bin/node/primitives/src/lib.rs Show resolved Hide resolved
primitives/session/src/lib.rs Show resolved Hide resolved
primitives/finality-grandpa/src/lib.rs Show resolved Hide resolved
frame/grandpa/src/lib.rs Show resolved Hide resolved
frame/grandpa/src/lib.rs Show resolved Hide resolved
frame/grandpa/src/lib.rs Show resolved Hide resolved
frame/grandpa/src/lib.rs Show resolved Hide resolved
frame/grandpa/src/lib.rs Show resolved Hide resolved
frame/grandpa/src/equivocation.rs Show resolved Hide resolved
#[repr(u8)]
pub enum ReportEquivocationValidityError {
/// The proof provided in the report is not valid.
InvalidEquivocationProof = 1,
Copy link
Member

Choose a reason for hiding this comment

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

These errors should be unique for the whole runtime.

Now, it could happen that they overlap and will be useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where should they go then? Or do I just look for the next free u8?

pallet-staking = { version = "2.0.0-dev", path = "../../../frame/staking" }
pallet-authority-discovery = { version = "2.0.0-dev", path = "../../../frame/authority-discovery" }
pallet-staking = { version = "2.0.0-dev", path = "../../../frame/staking" }
pallet-grandpa = { version = "2.0.0-dev", path = "../../../frame/grandpa" }
Copy link
Contributor

@AurevoirXavier AurevoirXavier May 22, 2020

Choose a reason for hiding this comment

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

This should be in dev-dependencies, pallet-grandpa just used in tests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A3-in_progress Pull request is in progress. No review needed at this stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.