BEEFY: add support for slashing validators signing forking commitments (without ancestry proofs) #1329
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).
There was a problem hiding this comment.
PR ready for review, modulo that the proofs will be replaced via the work tracked in #1441. #1441 should reach sufficient maturity prior to going live on Kusama that those ancestry proofs make a better substitute of the header checks in this PR. As such, this PR can be reviewed, but it should not be merged until either
- ancestry proofs are integrated
- BEEFY goes live on Kusama (because going live with slashing that assumes 2/3rd of validators are honest is better than going live with no fork equivocation slashing at all)
| /// 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.
@acatangiu wrt. your comment on old PR here:
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).
It's in primitives since it's also used by the client: https://github.com/lederstrumpf/polkadot-sdk/blob/53e8ae0f2d2f6a6a9ae7ac55e0ccf104db76867e/substrate/client/consensus/beefy/src/communication/fisherman.rs#L121-L131
see vis à vis vote equivocation proofs: https://github.com/lederstrumpf/polkadot-sdk/blob/53e8ae0f2d2f6a6a9ae7ac55e0ccf104db76867e/substrate/client/consensus/beefy/src/worker.rs#L918-L934
In the client, it's used verify a fork equivocation before reporting it. If the proof is generated by the reporter (as it is strictly currently), then this is just a sanity check that the fork equivocation proof generated is actually valid.
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: https://forum.polkadot.network/t/polkadot-kusama-bridge/2971/5 |
Lederstrumpf
changed the title
|
The CI pipeline was cancelled due to failure one of the required jobs. |
| /// invalid fork proof and validate the given key ownership proof | ||
| /// against the extracted offender. If both are valid, the offence | ||
| /// will be reported. | ||
| // TODO: fix key_owner_proofs[0].validator_count() |
There was a problem hiding this comment.
Still needs to be fixed I assume :)
There was a problem hiding this comment.
Done in Lederstrumpf@70785985ec9. In a valid equivocation report, the validator set count will be the same across key ownership proofs, so it's fine to read this from the first one imo. What had to be fixed though (which is what the TODO should have been formulated for), is to account for the length of the key_owner_proofs vec when calculating the weight.
| /// block authors will call it (validated in `ValidateUnsigned`), as such | ||
| /// if the block author is defined it will be defined as the equivocation | ||
| /// reporter. | ||
| // TODO: fix key_owner_proofs[0].validator_count() |
There was a problem hiding this comment.
See #1329 (comment)
| &self, | ||
| header: &B::Header, | ||
| ) -> Result<ValidatorSet<AuthorityId>, Error> { | ||
| self.runtime |
There was a problem hiding this comment.
Could we maybe use sc_consensus_beefy::expect_validator_set, which falls back to headers traversal if state is already discarded for the header? I expect that when this method is called (if RejectPast is returned from consider_*), then there's a big chance that we may hit StateDiscarded here
There was a problem hiding this comment.
See #2689 (comment) - maybe this suggestion is no longer relevant
There was a problem hiding this comment.
Suggestion is still relevant, just that we need to use some non-blocking version of the function
There was a problem hiding this comment.
good catch - I've add a non-blocking version of the function in 816b726. Note this will simply error if a header's parent header cannot be found.
| // TODO: maybe raise cost reputation when seeing votes that are intentional | ||
| // spam: votes that trigger fisherman reports, but don't go through either | ||
| // because signer is/was not authority or similar reasons. | ||
| // The idea is to more quickly disconnect neighbors which are attempting DoS. |
There was a problem hiding this comment.
IMO could worth a couple of words comment on why we are not calling check_vote/check_proof in Consider::Accept branch. Maybe that's obvious for those who's working on BEEFY, though :)
There was a problem hiding this comment.
Yes, add a comment for explaining the reasoning better:
- we want to
check_vote(vote)and/orcheck_proof(proof)(below) on every vote or proof, regardless of past, preset or future, - but we can only expect to see a
ForkEquivocationfor a block already finalized by us - aka onRejectPast- forConsider::Acceptwe know this is the first "valid" proof we're seeing (otherwise block would have been already finalized) - and for "invalid" proofs we don't check for equivocations as they are invalid anyway.
Accepted votes are also checked for VoteEquivocation which can also happen on non-finalized blocks (like double voting), but that happens later in the worker.
There was a problem hiding this comment.
IMO could worth a couple of words comment on why we are not calling check_vote/check_proof in Consider::Accept branch. Maybe that's obvious for those who's working on BEEFY, though :)
Thanks for the good observation Slava :)
This was indeed not (fully) correctly implemented, see https://github.com/lederstrumpf/polkadot-sdk/compare/816b726c5f593a9d111d74ab0dc4757d7bd63a7f..3e9a4a74e6a for updated logic - the description of the logic is here:
- All votes and proofs (i.e. both for active and non-active rounds) are now checked for fork equivocations (since done across the board, I've moved this check to the calling sc_consensus_beefy::communication::gossip::GossipValidator::validate(..)).
- As Adrian's pointed out, accepted votes are also checked later (namely in
add_vote), but for vote equivocations, so they should also be checked for fork equivocations (since an accepted vote may equivocate against a finalized payload without the voter having cast a prior vote for something else). - The reason we're checking proofs and votes for future non-active rounds (i.e. on blocks not finalized by grandpa yet) and flagging these as fork equivocations (despite them not equivocating against existing state) is that these should not have been voted on yet (hence, might be for/from an attack on the counterparty chain of a bridge)
- Proofs for past and active rounds are for grandpa-finalized blocks, hence we should check whether they equivocate against our canonical chain.
but we can only expect to see a
ForkEquivocationfor a block already finalized by us - aka onRejectPast- forConsider::Acceptwe know this is the first "valid" proof we're seeing (otherwise block would have been already finalized) - and for "invalid" proofs we don't check for equivocations as they are invalid anyway.
but we can only expect to see a ForkEquivocation for a block already finalized by us ...
See 3. above
... and for "invalid" proofs we don't check for equivocations as they are invalid anyway.
If you're using "invalid" as defined in validate_finality_proof's verification block:
Invalid proofs include those that pass the trivial validity checks but only have fewer than 2n/3+1 valid signatures. These are within our threat model: they are exactly the sorta proofs that cross-chain attackers would easily be able to craft (without violating our honest-majority assumption) when trying their luck to game BEEFY's subsampling protocol. So we should primarily be checking these ;)
... we know this is the first "valid" proof we're seeing (otherwise block would have been already finalized) ...
not entirely unambiguous to me how you're using "valid" or "finalized" here: We're only accepting proofs for active rounds, and all blocks in active rounds have already been (grandpa-)finalized - with "finalized", do you mean the block's within in the history of the best_beefy_block?
There was a problem hiding this comment.
If you're using "invalid" as defined in validate_finality_proof's verification block:
Invalid proofs include those that pass the trivial validity checks but only have fewer than 2n/3+1 valid signatures. These are within our threat model: they are exactly the sorta proofs that cross-chain attackers would easily be able to craft (without violating our honest-majority assumption) when trying their luck to game BEEFY's subsampling protocol. So we should primarily be checking these ;)
Yes, that is true, good catch!
not entirely unambiguous to me how you're using "valid" or "finalized" here: We're only accepting proofs for active rounds, and all blocks in active rounds have already been (grandpa-)finalized - with "finalized", do you mean the block's within in the history of the best_beefy_block?
When considering a gossiped proof we get Consider::Accept iff the proof is valid (disambiguation: contains more than two thirds validators signatures) and the block number it is for has not been already finalized (disambiguation: not finalized by BEEFY. block_number > best_beefy_block). From this we can infer, that this is the first valid BEEFY proof we're seeing for this block number.
Even so, it's still correct to check for equivocation since being the first doesn't guarantee it's also correct in terms of payload. So, yes, I support your change.
There was a problem hiding this comment.
But doing so, you also need to think about some newly opened scenarios:
- what about blocks between GRANDPA finalized and best imported? i.e.
best_grandpa < number < blockchain().info().best_block
Thinking out loud:
Say our node is network lagging on gossip so it hasn't seen latest GRANDPA+BEEFY finality, and at some point a valid+correct BEEFY proof gets through to us for a block number where we locally haven't seen GRANDPA finality for yet. Meaning we might have multiple forks/hashes matching said block number - only one of which having correct payload => we might incorrectly treat this valid+correct proof as an equivocation.
But even if above happens, the ForkEquivationProof will be rejected on-chain when submitted right? because the payload from the proof must != from the on-chain payload at that block number - and on the "good" fork, that will not be the case - so we just slash on pruned forks where it doesn't matter.
^ Food for thought - maybe I missed something.
There was a problem hiding this comment.
But even if above happens, the ForkEquivationProof will be rejected on-chain when submitted right? because the payload from the proof must != from the on-chain payload at that block number - and on the "good" fork, that will not be the case - so we just slash on pruned forks where it doesn't matter.
Your understanding is correct, see this note on check_fork_equivocation_proof: https://github.com/lederstrumpf/polkadot-sdk/blob/e7718fc8e75ed7133b3c06061374f32beefa7b8f/substrate/primitives/consensus/beefy/src/lib.rs#L492-L498
The most interesting place is indeed best_grandpa < number < blockchain().info().best_block:
If - like in your example - a node is lagging and sees BEEFY proofs on gossip before even seeing the block, then it will report this as an equivocation, but it will be ignored.
Likewise, without finality proofs, it will also get slashed on forks, which is immaterial.
https://github.com/lederstrumpf/polkadot-sdk/blob/e7718fc8e75ed7133b3c06061374f32beefa7b8f/substrate/client/consensus/beefy/src/communication/fisherman.rs#L300
I could tighten the above condition in the fisherman to report if number < blockchain().info().finalized_number, but since the runtime has no notion of finalization, I decided against this since it would just lead to reports that are either invalid (since a node is lagging on finality) or that the runtime would ignore until some other commitment is finalized:
If, as via the comment https://github.com/lederstrumpf/polkadot-sdk/blob/e7718fc8e75ed7133b3c06061374f32beefa7b8f/substrate/primitives/consensus/beefy/src/lib.rs#L492-L498, some nodes vote on commitments that have not been finalized yet, they will either
- not be slashed if the commitment is ultimately finalized
- slashable once some other commitment is ultimately finalized for the same block height (since a valid equivocation proof can be generated at this stage)
which I deem covers the requirements.
| // 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)| { |
There was a problem hiding this comment.
Even if the fork report is invalid overall, if it contains some valid signatures, shouldn't we report them ?
There was a problem hiding this comment.
good observation! I thought I was already filtering for this at the call site - added in b19c092.
My current preference is to leave the responsibility for ensuring correct equivocation reports at the call site (i.e. the fisherman). We can also add this here, but if we are permissive in the runtime processing these, then we have to worry more about & increase our catering for DoS vectors like you point out here.
wdyt?
There was a problem hiding this comment.
My current preference is to leave the responsibility for ensuring correct equivocation reports at the call site (i.e. the fisherman)
I don't think we can leave this check only in the fisherman. We have to check this in the pallet as well, cause otherwise we might receive equivocation reports from other callers (for example someone manually executing an extrinsic) and we would risk not validating them.
There was a problem hiding this comment.
But I think this is fixed. Please move the conversation to #1903 if it isn't
| }; | ||
|
|
||
| if R::is_known_offence(&[offender], &time_slot) { | ||
| // Validate the key ownership proof extracting the id of the offender. | ||
| let offenders = offenders |
There was a problem hiding this comment.
Should we also check for duplicate authorities + signatures here ? If not someone can inflate the size of the proof artificially, while being refunded => DoS vector ?
There was a problem hiding this comment.
https://github.com/lederstrumpf/polkadot-sdk/blob/e7718fc8e75ed7133b3c06061374f32beefa7b8f/substrate/frame/grandpa/src/equivocation.rs#L167-L171
https://github.com/lederstrumpf/polkadot-sdk/blob/e7718fc8e75ed7133b3c06061374f32beefa7b8f/substrate/frame/beefy/src/equivocation.rs#L237-L241
Equivocation reporting takes a lax stance: equivocations are reported if any of the included offences are new.
Should we also check for duplicate authorities + signatures here ?
If not someone can inflate the size of the proof artificially, while being refunded => DoS vector
Firstly, I'm not convinced the problem should be handled here, but rather be the responsibility of the offences pallet since the problem is generic to equivocation reports, so shouldn't be solved at multiple call sites in parallel.
Secondly, when the report is triaged, the offenders are iterated over and only if an offender has not been added to the ReportIndexStorage yet, the report is added to Reports, and the offender added to ReportIndexStorage:
https://github.com/lederstrumpf/polkadot-sdk/blob/master/substrate/frame/offences/src/lib.rs#L175-L187
So even if the offender is duplicated in the same report (with distinct signatures), the reward would only be accounted for once. This does leave the door open for duplicated submissions nonetheless, but there's no reward incentive for doing so.
There was a problem hiding this comment.
I don't mean for us to handle duplicate offence reports. That's indeed the responsibility of the offences pallet.
I mean that if someone sends a fork equivocation report with duplicate authorities, signatures, and key ownership proofs, for example: (A, B, C, C, C, C, C ... 1 million times C) and the report is ok, the system will spend compute cycles in checking C 1 million times while the sender will be refunded for the cost of the extrinsic. Which seems like some kind of DoS. Or do we have a limit on the number of authorities in a report somewhere ?
There was a problem hiding this comment.
please move all review conversations to #1903 ; this PR is closed
| /// Proof of authority misbehavior on a given set id. | ||
| /// This proof shows commitment signed on a different fork. | ||
| #[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)] | ||
| pub struct ForkEquivocationProof<Number, Id, Signature, Header> { |
There was a problem hiding this comment.
- Nit: I would name this
CommitmentEquivocationProof. Seems more consistent withVoteEquivocationProof. - Is there any advantage in being able to report commitment equivocations ? Couldn't we report each vote individually ?
There was a problem hiding this comment.
- I like
CommitmentEquivocationProofmore thanForkEquivocationProoftoo, +1 - yes, advantage is batching everything together - you get the conflicting proofs from the two sides of the bridge and build single
CommitmentEquivocationProoffrom these two and submit in single extrinsic (cheaper, faster)
There was a problem hiding this comment.
On 1:
I'm not yet convinced by CommitmentEquivocationProof: both votes and signed commitments contain commitments, so this introduces an ambiguity.
↦ Would you prefer SignedCommitmentEquivocationProof to ForkEquivocationProof? It's more verbose to avoid the above clash.
The current naming refers to the origin of an equivocation (voting vs. forking) to distinguish between them. ForkEquivocationProof is also deliberately looser in naming since they can be detected also come from a BeefyVersionedFinalityProof, but that's a minor point since at this stage, BeefyVersionedFinalityProof is just a SignedCommitment wrapper.
On 2:
Adrian already explained it well.
There was a problem hiding this comment.
-
I see your point.
SignedCommitmentEquivocationProofseems too verbose. Thinking. Will get back if I have any other idea. -
Thanks for the explanation ! Makes sense.
There was a problem hiding this comment.
IMO SignedCommitmentEquivocationProof is best option so far
Co-authored-by: Serban Iorga <serban300@gmail.com>
Reported-by: Serban Iorga <serban300@gmail.com>
Co-authored-by: Serban Iorga <serban300@gmail.com>
Lederstrumpf
force-pushed
the
rhmb/beefy-slashing-fisherman
branch
from
b68cfe2
to
d2bf211
Compare
| &self, | ||
| header: &B::Header, | ||
| ) -> Result<ValidatorSet<AuthorityId>, Error> { | ||
| self.runtime |
substrate/client/consensus/beefy/src/communication/fisherman.rs
Outdated
Show resolved
Hide resolved
| // TODO: maybe raise cost reputation when seeing votes that are intentional | ||
| // spam: votes that trigger fisherman reports, but don't go through either | ||
| // because signer is/was not authority or similar reasons. | ||
| // The idea is to more quickly disconnect neighbors which are attempting DoS. |
There was a problem hiding this comment.
Yes, add a comment for explaining the reasoning better:
- we want to
check_vote(vote)and/orcheck_proof(proof)(below) on every vote or proof, regardless of past, preset or future, - but we can only expect to see a
ForkEquivocationfor a block already finalized by us - aka onRejectPast- forConsider::Acceptwe know this is the first "valid" proof we're seeing (otherwise block would have been already finalized) - and for "invalid" proofs we don't check for equivocations as they are invalid anyway.
Accepted votes are also checked for VoteEquivocation which can also happen on non-finalized blocks (like double voting), but that happens later in the worker.
Co-authored-by: Adrian Catangiu <adrian@parity.io>
|
Closing since all changes suggested in review here are addressed in #1903, which supersedes this PR. |
Description
Migrates & builds on paritytech/substrate#14744 - see its description & discussion for history.
For proving a fork equivocation, this PR's equivocation proofs currently includes headers that the verifier calculates hashes of and compares with on-chain
<frame_system::Pallet<T>>::block_hash(block_number). Since this mapping has a horizon of 4096 blocks, equivocations predating this horizon cannot be slashed. While this thwarts attacks assuming a 2/3rds of validators honestly participate in BEEFY finalization and at least one honest relayer can update the beefy light client at least once every 4096 blocks, this is clearly only a temporary solution. Should not be merged untilProposed 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_rootreport offense to staking so offending vote author /
SignedCommitmentsignatories get slashedClient-side
adds "fisherman" capabilities to voter gossip - on detecting votes for historical forks, it builds the required proof of misbehavior and submits report.
Closes #1120