Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BEEFY: add support for slashing validators signing forking commitments #1903

Conversation

Lederstrumpf
Copy link
Contributor

@Lederstrumpf Lederstrumpf commented Oct 17, 2023

Description

Builds on #1329 - integrates ancestry proofs (https://github.com/Lederstrumpf/merkle-mountain-range/tree/ancestry-proof-rebase-v0.6.0) - see #1441 for details. A presentation held for W3F research describing the slashing protocol can also be found here: https://drive.google.com/file/d/17vJnGhOWw0G0_e3xEXvycYYu3q5Y-_PU/view?usp=drive_link

Includes both header & ancestry proofs for proving a fork equivocation.

For reviewing, I recommend first reviewing the changes in #1329 (excludes ancestry proofs & future block equivocations), and then reviewing the diff of the PRs (Lederstrumpf/polkadot-sdk@rhmb/beefy-slashing-fisherman...Lederstrumpf:polkadot-sdk:rhmb/beefy-slashing-fisherman-ancestry-proofs).

Closes #1120

Below are design choices made and the two routes for implementing them, where A. is currently implemented:

design choices

keeping header proofs

As described in #1441, header proofs are only sufficient for proving equivocations up to 4096 blocks ago, whereas this PR adds ancestry proofs and slashing for future blocks.
While ancestry proofs could replace header proofs entirely, my reasons for keeping header proofs are the following:

  • A reporter may not be running offchain indexing, hence would be unable to generate an ancestry proof. Having both proofs available increases the number of viable fishers.
  • In case there's a bug generating the ancestry proof, the header proof would still be valid

slashing equivocations on future block heights

If the equivocating payload is for a future block, by necessity, we cannot prove the equivocation with a header or ancestry proof, by obvious necessity.
As suggested by @acatangiu, we could wait for equivocation.height to be reached on the canonical chain and only report and slash then. This is sensible since the equivocator's funds would remain locked at least for the following 28 eras, and therefore it would be safe to delay reporting & slashing within that timeframe.

We can also slash eagerly by just checking that the equivocating commitment has been duly signed. My reasons for slashing eagerly instead of waiting for equivocation.height are the following:

  • We'd have to add logic for withholding equivocation reports until equivocation.height is reached, which I expect to be more complex than the logic for slashing eagerly.
  • In snowfork's solidity implementation, it is not checked whether equivocation.height (local name: commitment.blockNumber) is for the current/next era, hence whether the current/next validator set even has the authority to author blocks. As such, without slashing for future blocks, the current/next validators could create an equivocation with equivocation.height > best_block.height + 28 * blocks_per_era, which could only be reported & slashed on Polkadot/Kusama after the equivocators could already unbond. We may want to add a is_block_within_current_era(commitment.height) check on the ETH side regardless to avoid bricking the bridge forever in case of an attack with equivocation.height >> best_block.height, but other BEEFY clients may not perform such a check, so I prefer slashing eagerly to discourage the attack even if the ETH bridge's light client may be unaffected.

A. current implementation

The following cases are handled:

  1. equivocation.height < best_block.height - 4096 => ancestry proof
  2. best_block.height - 4096 <= equivocation.height => header proof (+ optionally ancestry proof, in case the proof is only processed once header proof goes out of scope)
  3. best_block.height < equivocation.height => just check that commitment is signed

As such, a ForkEquivocationProof has both the ancestry proof and the header proof optional:

pub struct ForkEquivocationProof<Number, Id, Signature, Header, Hash> {
	pub commitment: Commitment<Number>,
	pub signatories: Vec<(Id, Signature)>,
	pub correct_header: Option<Header>,
	pub ancestry_proof: Option<AncestryProof<Hash>>,
}

B. alternative implementation

I am considering changing the handling to

  1. equivocation.height < best_block.height - 2048 => ancestry proof + header proof of ancestry proof's mmr root
  2. best_block.height - 2048 <= equivocation.height => header proof for mmr root of equivocation (2048 is equal margin)
  3. best_block.height < equivocation.height => just check that commitment is signed

Which would result in the following proof struct

pub struct ForkEquivocationProof<Number, Id, Signature, Header, Hash> {
	pub commitment: Commitment<Number>,
	pub signatories: Vec<(Id, Signature)>,
	pub correct_header: Header,
	pub ancestry_proof: Option<AncestryProof<Hash>>,
}

where the correct_header serves different purposes depending on route B.1. or B.2.

The advantage of B against A is that in route B.1., ancestry proofs would remain valid if they are only processed in a block later than the one they were generated for, whereas in A.1. they are only in scope for the current block. The disadvantage is a more complicated proof structure, and it's not crystal clear to me that the advantage of B is important: it's irrelevant if the fisher is also the block producer, but would account for networking delays of other fishers.
Hence, I appreciate feedback on my reasoning here before implementing B.

acatangiu and others added 30 commits August 29, 2023 00:27
GRANDPA finalization proof is not checked, which leads to slashing on
forks. This is fine since honest validators will not be slashed on the
chain finalized by GRANDPA, which is the only chain that ultimately
matters. The only material difference not checking GRANDPA proofs makes
is that validators are not slashed for signing BEEFY commitments prior
to the blocks committed to being finalized by GRANDPA. This is fine too,
since the slashing risk of committing to an incorrect block implies
validators will only sign blocks they *know* will be finalized by
GRANDPA.
instead of using votes as the underlying primitive, rather use
commitments since they're a more universal container for signed payloads
(for instance `SignedCommitment` is also the primitive used by ethereum relayers).
SignedCommitments are already aggregates of multiple signatures. Will
use SignedCommitment directly next.
previously assumed equivocation report for singular malicious party.
With fork equivocations, the expectation should be that most
equivocation reports will be for multiple simultaneous equivocators.
reduce from Block to Header: less restrictive.
check_signed commitment wasn't complete anyway.
good to have both interfaces since BeefyVersionedFinalityProof is what's
passed on the gossip layer, and SignedCommitments are naturally
reconstructed from multiple sources, for instance submit_initial calls
on Ethereum.
redundant vs report_fork_equivocation
No need to trigger first session if chain's already had blocks built,
such as with generate_blocks_and_sync, which needs to build on genesis.
If chain is not at genesis and create_beefy_worker, it will panic trying
to canonicalize an invalid (first) block.
can pass in Arc of TestApi now. Required since fisherman reports should be
pushed to `reported_fork_equivocations`, and should be logged with the
same instance (see upcoming commit).
Copy link
Contributor

@serban300 serban300 left a comment

Choose a reason for hiding this comment

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

We should check all the incoming justifications. Right now from what I understand we are missing the ones that come from the OnDemandJustificationsEngine.

See: #1903 (comment)

@acatangiu
Copy link
Contributor

We should check all the incoming justifications. Right now from what I understand we are missing the ones that come from the OnDemandJustificationsEngine.

See: #1903 (comment)

Let's first focus on the runtime side, how equivocations are submitted, processed and punished - it is the most important piece.

We can continue the fisherman work subsequently.

@serban300
Copy link
Contributor

We should check all the incoming justifications. Right now from what I understand we are missing the ones that come from the OnDemandJustificationsEngine.
See: #1903 (comment)

Let's first focus on the runtime side, how equivocations are submitted, processed and punished - it is the most important piece.

We can continue the fisherman work subsequently.

Ok. I just want to finish a PR related to the comment which shouldn't take long (today or Monday) and then will continue with the runtime side.

@serban300
Copy link
Contributor

We should check all the incoming justifications. Right now from what I understand we are missing the ones that come from the OnDemandJustificationsEngine.
See: #1903 (comment)

Let's first focus on the runtime side, how equivocations are submitted, processed and punished - it is the most important piece.
We can continue the fisherman work subsequently.

Ok. I just want to finish a PR related to the comment which shouldn't take long (today or Monday) and then will continue with the runtime side.

It's not going as fast as I planned. Leaving here the branch where I started working on this:
https://github.com/serban300/polkadot-sdk/tree/beefy-equivocation-client
And I'll continue it later.

Prioritizing the runtime side for the moment.

github-merge-queue bot pushed a commit that referenced this pull request May 13, 2024
Extracting the logic for generating and verifying ancestry proofs from
this PR: #1903 with small
adjustments

@Lederstrumpf I added you as a co-author to each commit. Please let me
know if you want me to also associate an e-mail address with your name
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6220033

@Lederstrumpf Lederstrumpf force-pushed the rhmb/beefy-slashing-fisherman-ancestry-proofs branch from b83bb43 to b65b7a5 Compare May 16, 2024 09:45
@paritytech-review-bot paritytech-review-bot bot requested a review from a team May 16, 2024 09:46
@serban300
Copy link
Contributor

Opened a separate PR for the runtime part. It starts from the high-level approach here, but it introduces some significant changes: #4522

set cost for an equivocation to the worst case `INVALID_PROOF` on
Kusama, i.e. baseline 5k + 25 per bad signature.
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
Extracting the logic for generating and verifying ancestry proofs from
this PR: paritytech#1903 with small
adjustments

@Lederstrumpf I added you as a co-author to each commit. Please let me
know if you want me to also associate an e-mail address with your name
liuchengxu pushed a commit to liuchengxu/polkadot-sdk that referenced this pull request Jun 19, 2024
Extracting the logic for generating and verifying ancestry proofs from
this PR: paritytech#1903 with small
adjustments

@Lederstrumpf I added you as a co-author to each commit. Please let me
know if you want me to also associate an e-mail address with your name
programskillforverification pushed a commit to programskillforverification/polkadot-sdk that referenced this pull request Jul 3, 2024
Related to paritytech#4523

Extracting part of paritytech#1903
(credits to @Lederstrumpf for the high-level strategy), but also
introducing significant adjustments both to the approach and to the
code. The main adjustment is the fact that the `ForkVotingProof` accepts
only one vote, compared to the original version which accepted a
`vec![]`. With this approach more calls are needed in order to report
multiple equivocated votes on the same commit, but it simplifies a lot
the checking logic. We can add support for reporting multiple signatures
at once in the future.

There are 2 things that are missing in order to consider this issue
done, but I would propose to do them in a separate PR since this one is
already pretty big:
- benchmarks/computing a weight for the new extrinsic (this wasn't
present in paritytech#1903 either)
- exposing an API for generating the ancestry proof. I'm not sure if we
should do this in the Mmr pallet or in the Beefy pallet

Co-authored-by: Robert Hambrock <[email protected]>

---------

Co-authored-by: Adrian Catangiu <[email protected]>
TomaszWaszczyk pushed a commit to TomaszWaszczyk/polkadot-sdk that referenced this pull request Jul 7, 2024
Related to paritytech#4523

Extracting part of paritytech#1903
(credits to @Lederstrumpf for the high-level strategy), but also
introducing significant adjustments both to the approach and to the
code. The main adjustment is the fact that the `ForkVotingProof` accepts
only one vote, compared to the original version which accepted a
`vec![]`. With this approach more calls are needed in order to report
multiple equivocated votes on the same commit, but it simplifies a lot
the checking logic. We can add support for reporting multiple signatures
at once in the future.

There are 2 things that are missing in order to consider this issue
done, but I would propose to do them in a separate PR since this one is
already pretty big:
- benchmarks/computing a weight for the new extrinsic (this wasn't
present in paritytech#1903 either)
- exposing an API for generating the ancestry proof. I'm not sure if we
should do this in the Mmr pallet or in the Beefy pallet

Co-authored-by: Robert Hambrock <[email protected]>

---------

Co-authored-by: Adrian Catangiu <[email protected]>
@acatangiu acatangiu closed this Jul 18, 2024
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Related to paritytech#1903

For paritytech#1903 we will need to add a Fisherman struct. This PR:
- defines a basic version of `Fisherman` and moves into it the logic
that we have now for reporting double voting equivocations
- splits the logic for generating the key ownership proofs into a more
generic separate method
- renames `EquivocationProof` to `DoubleVotingProof` since later we will
introduce a new type of equivocation

The PR doesn't contain any functional changes
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Extracting the logic for generating and verifying ancestry proofs from
this PR: paritytech#1903 with small
adjustments

@Lederstrumpf I added you as a co-author to each commit. Please let me
know if you want me to also associate an e-mail address with your name
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Related to paritytech#4523

Extracting part of paritytech#1903
(credits to @Lederstrumpf for the high-level strategy), but also
introducing significant adjustments both to the approach and to the
code. The main adjustment is the fact that the `ForkVotingProof` accepts
only one vote, compared to the original version which accepted a
`vec![]`. With this approach more calls are needed in order to report
multiple equivocated votes on the same commit, but it simplifies a lot
the checking logic. We can add support for reporting multiple signatures
at once in the future.

There are 2 things that are missing in order to consider this issue
done, but I would propose to do them in a separate PR since this one is
already pretty big:
- benchmarks/computing a weight for the new extrinsic (this wasn't
present in paritytech#1903 either)
- exposing an API for generating the ancestry proof. I'm not sure if we
should do this in the Mmr pallet or in the Beefy pallet

Co-authored-by: Robert Hambrock <[email protected]>

---------

Co-authored-by: Adrian Catangiu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T15-bridges This PR/Issue is related to bridges.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

BEEFY: slash validators voting on non-finalized forks
6 participants