BEEFY: add support for slashing validators signing forking commitments #14744
Conversation
Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
Signed-off-by: Adrian Catangiu <adrian@parity.io>
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
added
the
A3-in_progress
Lederstrumpf
changed the title
Lederstrumpf
changed the title
frame/beefy/src/equivocation.rs
Outdated
| 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)?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
nyet: is_known_offence returns true iff no offenders in the report are unknown
https://github.com/lederstrumpf/substrate/blob/4c2e0ba22ce2211bd90ea297d983de18c6f4d373/frame/offences/src/lib.rs#L144-L151
| /// 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>( |
There was a problem hiding this comment.
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).
| // 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 | ||
| } |
There was a problem hiding this comment.
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
| /// 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, |
There was a problem hiding this comment.
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> { | |||
There was a problem hiding this comment.
can't we just #[derive(Clone)]?
|
The CI pipeline was cancelled due to failure one of the required jobs. |
Co-authored-by: Adrian Catangiu <adrian@parity.io>
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:
SignedCommitmentRationale 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/submitInitialWithHandovercall is detected in Ethereum's mempool / a block.Proposed Solution
Runtime
pallet_beefy:submit_unsigned_vote_equivocation_report(previouslyreport_equivocation) for vote equivocations (VoteEquivocationProof), andsubmit_unsigned_fork_equivocation_reportfor fork equivocations (ForkEquivocationProof), the latter whether detected in votes,SignedCommitment, orVersionedFinalityProofmmr_root!= on-chainmmr_rootSignedCommitmentsignatories get slashedClient-side
Closes paritytech/polkadot-sdk#1120
TODOs
<frame_system::Pallet<System>>::block_hashfor verifying header included inForkEquivocationProof