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

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

Draft
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

Lederstrumpf
Copy link
Contributor

Description

Builds on and supersedes #14520.
The original scope was to support slashing validators equivocating by virtue of a gossiped vote which votes on a fork that has not been finalized.

This PR's aim differs in the following ways:

  1. we do not slash for voting on unfinalized payloads, but rather for payloads that differ from the payload known to the runtime. As such, we do not check GRANDPA finalization.
  2. while slashing equivocations detected in votes is also supported, the scope is extended to (and the focus placed on) equivocations detected in a SignedCommitment

Rationale for 1.

Since GRANDPA finalization proof is not checked, which leads to slashing on
forks that may not be finalized. 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.

Rationale for 2.

While dishonest might gossip their equivocating votes via the standard gossip protocol, this hurts the prospect of their attack, and opens the door for the colluders to get slashed even if they don't ultimately carry out the attack on, for instance, the Ethereum bridge. Instead, we should face the reality that we will likely only detect the attack once it is carried out, that is: an equivocating payload of a submitInitial/submitInitialWithHandover call is detected in Ethereum's mempool / a block.

Proposed Solution

Runtime

  • two new extrinsic submission calls in pallet_beefy: submit_unsigned_vote_equivocation_report (previously report_equivocation) for vote equivocations (VoteEquivocationProof), and submit_unsigned_fork_equivocation_report for fork equivocations (ForkEquivocationProof), the latter whether detected in votes, SignedCommitment, or VersionedFinalityProof
  • verify commitment's mmr_root != on-chain mmr_root
  • report offense to staking so offending vote author / SignedCommitment signatories get slashed

Client-side

  • adds "fisherman" capabilities to voter gossip - on detecting votes for historical forks, it builds the required proof of misbehavior and submits report.

Closes paritytech/polkadot-sdk#1120

TODOs

  • Polkadot companion
  • Fish for equivocations on Ethereum (not scope of this PR - [best home is probably Snowfork's relayer])
  • More robust mechanism than <frame_system::Pallet<System>>::block_hash for verifying header included in ForkEquivocationProof
  • Extend test coverage

acatangiu and others added 30 commits August 7, 2023 16:09
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).
mock api's `submit_report_fork_equivocation_unsigned_extrinsic` pushes
reported equivocations to runtime_api.reported_fork_equivocations
Generates fork equivocation proofs from a vote and a header.
i.e. a fork equivocation triggered by a vote
required for fisherman to *not* report own equivocations - see next commit.
i.e. a fork equivocation triggered by a vote
@Lederstrumpf Lederstrumpf added the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 10, 2023
@Lederstrumpf Lederstrumpf changed the title Rhmb/beefy slashing fisherman [RFC] BEEFY: add support for slashing validators signing forking commitments Aug 10, 2023
@Lederstrumpf Lederstrumpf changed the title [RFC] BEEFY: add support for slashing validators signing forking commitments BEEFY: add support for slashing validators signing forking commitments Aug 10, 2023
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

looks good so far 👍

Comment on lines 212 to 217
let offenders = offenders
.into_iter()
.zip(key_owner_proofs.iter())
.map(|(key, key_owner_proof)| P::check_proof((BEEFY_KEY_TYPE, key.clone()), key_owner_proof.clone()))
.collect::<Option<Vec<_>>>()
.ok_or(InvalidTransaction::BadProof)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

what if a validator is reported for a vote equivocation, then is also included in this fork equivocation? is the whole fork_equivocation (all misbehaving validators) discarded because of 'known_offence' of that one validator before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

frame/beefy/src/equivocation.rs Outdated Show resolved Hide resolved
/// 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.
pub fn check_fork_equivocation_proof<Number, Id, MsgHash, Header>(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is ok for now (while only checking header hashes) to live in primitives, but when moving to MMR root membership proofs this should be some pallet_beefy::Config trait type that is provided by pallet_beefy_mmr (or some other generic, pluggable solution).

primitives/consensus/beefy/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +379 to +387
// check check each signatory's signature on the commitment.
// if any are invalid, equivocation report is invalid
// TODO: refactor check_commitment_signature to take a slice of signatories
return signatories.iter().all(|(authority_id, signature)| {
check_commitment_signature(&commitment, authority_id, signature)
})
} else {
false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what if some are valid and one is invalid?

I guess we shouldn't attempt to cover this in the runtime. We can expect fishermen to submit 100% validated equivocation proofs, but let's make a mental note to double check this in the fishermen code and remove invalid signatures so that valid ones still get slashed

primitives/consensus/beefy/src/lib.rs Show resolved Hide resolved
Comment on lines +267 to +271
/// The proof is valid if
/// 1. the header is in our chain
/// 2. its digest's payload != commitment.payload
/// 3. commitment is signed by signatories
pub correct_header: Header,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just go with the mmr historic root membership proofs from the get-go instead of this correct_header temporary thing.

This one is not really useful as the reporting window is only ~7hours before correct_header cannot be verified on-chain anymore because of headers map pruning. Colluding validators can simply stop producing BEEFY finality for 7 hours then provide "valid" BEEFY proofs but for invalid forks to the bridge without being slashed.

And doing it like this in two steps, first using correct_header, then payload_membership_proof means dealing with breaking API changes, bumping API versions, etc.

I think it's not worth it.

@@ -162,6 +162,12 @@ mod mmr_root_provider {
_phantom: PhantomData<B>,
}

impl<B, R> Clone for MmrRootProvider<B, R> {
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we just #[derive(Clone)]?

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3433651

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

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