-
Notifications
You must be signed in to change notification settings - Fork 1.1k
BEEFY: add support for slashing validators signing forking commitments #1903
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
Closed
Lederstrumpf
wants to merge
205
commits into
paritytech:master
from
Lederstrumpf:rhmb/beefy-slashing-fisherman-ancestry-proofs
Closed
Changes from 1 commit
Commits
Show all changes
205 commits
Select commit
Hold shift + click to select a range
d8964df
sc-consensus-beefy: add BEEFY fisherman to gossip network
acatangiu a2678a0
sp-consensus-beefy: add invalid fork vote proof and equivalent BeefyApi
acatangiu 17398d8
pallet-beefy: add stubs for reporting invalid fork votes
acatangiu df31624
sc-consensus-beefy: fisherman reports invalid votes and justifications
acatangiu 938cde1
don't check GRANDPA finality
Lederstrumpf fb08553
change primitive: vote -> commitment
Lederstrumpf 39799b2
add check_signed_commitment/report_invalid_payload
Lederstrumpf 4aa602a
update comments
Lederstrumpf e0e13d9
add dummy for report of invalid fork commitments
Lederstrumpf 165d7fc
account for #14471
Lederstrumpf c7df83b
account for #14373
Lederstrumpf 754ae80
EquivocationOffence.{offender->offenders}
Lederstrumpf 6ed4e99
EquivocationProof->VoteEquivocationProof
Lederstrumpf 1e41551
Invalid{""->Vote}EquivocationProof
Lederstrumpf de2ae90
check_{""->vote}_equivocation_proof
Lederstrumpf 273f34b
InvalidForkCommitmentProof->ForkEquivocationProof
Lederstrumpf eb1e549
renames across submit_report_...
Lederstrumpf 3369783
convert EquivocationEvidenceFor to enum (minimal)
Lederstrumpf 7abbddc
handle ForkEquivocationProof enum variant
Lederstrumpf 8acea22
reduce find_mmr_root_digest trait constraint
Lederstrumpf 103fc53
fix fork equiv. call interfaces (vecs of sigs)
Lederstrumpf 1509a26
check proof's payload against correct header's
Lederstrumpf a62cc7f
rm superfluous report_fork_equiv.correct_header
Lederstrumpf 5445977
remove duplic. in check_{signed_commitment, proof}
Lederstrumpf d35a97c
update outdated comments
Lederstrumpf 4e3e1cc
remove report_invalid_payload
Lederstrumpf f27a876
.+report.+{""->_vote}_equivocations
Lederstrumpf f641759
move correct_header hash check into primitives
Lederstrumpf 5c4104c
create_beefy_worker: only push block if at genesis
Lederstrumpf a0d8b87
create_beefy_worker: opt. instantiate with TestApi
Lederstrumpf 9126904
push to reported_fork_equivocations
Lederstrumpf 02da07e
add generate_fork_equivocation_proof_vote to tests
Lederstrumpf a3b4d3f
test: Alice snitches on Bob's vote equivocation
Lederstrumpf d44e3c8
store reference to key_store in fisherman
Lederstrumpf c27579c
test: Alice doesn't snitch *own* vote equivocation
Lederstrumpf 5516837
un-stub submit_unsigned_fork_equivocation_report
Lederstrumpf 60a71c7
Alice reports Bob & Charlie's signed commitment
ec99b1a
cleanup
Lederstrumpf ea7dd35
remove superfluous None check
Lederstrumpf 3185776
fixup! check proof's payload against correct header's
Lederstrumpf 59c1c2a
fmt
Lederstrumpf ab3eb02
missing keystore
Lederstrumpf 844ed67
graft https://github.com/paritytech/polkadot/pull/7634
Lederstrumpf 6ebebed
impl equiv. runtime interfaces kusama/westend
Lederstrumpf efe4b2a
clone beefy pallet equivocation tests
Lederstrumpf d0dba3b
test renames
Lederstrumpf b438866
test fork equivocation (via vote) reports
Lederstrumpf bd856ac
handle fork equivocations in validate_unsigned & pre_dispatch
Lederstrumpf 9e1e7b1
clone tests for fork equivocations via commitments
Lederstrumpf f5b1ec5
bump number of beefy authorities in tests
Lederstrumpf 7504b96
test fork equivocation (via commitment) reports
Lederstrumpf 036fa2f
cleanup & TODOs
Lederstrumpf 53e8ae0
test that overlapping reports stack correctly
Lederstrumpf 8a14649
note re. temporary equivocation proof structure
Lederstrumpf 4074a27
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf e5537dd
fmt
Lederstrumpf c815af6
cleanup & remove TODO
Lederstrumpf 35330bb
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf bb2e791
add reference to #1441
Lederstrumpf 936406b
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf 16c12eb
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf 8b48fec
fixup! Merge remote-tracking branch 'origin/master' into rhmb/beefy-s…
Lederstrumpf ddd22d2
use ckb-merkle-mountain-range fork
Lederstrumpf 5a4f840
integrate new proof data structure into api
Lederstrumpf c0db0ca
adjust tests for new proof data structure
Lederstrumpf a65014d
fix Proof type parameter usage
Lederstrumpf e642dc4
stubs for tests
Lederstrumpf e882e01
integrate ancestry proof into equivocation proofs
Lederstrumpf f348a86
fix proof serde tests
Lederstrumpf 05b82fa
add generate_ancestry_proof to pallet-mmr
Lederstrumpf 9e2ec2a
pallet beefy: ancestry proof gen. & reporting
Lederstrumpf a6927af
fisher & worker: ancestry proof gen. & reporting
Lederstrumpf 03208f1
tests: report_fork_equivocation_vote_old_set_works
Lederstrumpf 9208734
verify prev_root is at correct block number
Lederstrumpf 852d110
fisherman: retrieve first_mmr_block_num
Lederstrumpf 9e91cf3
impl ancestry in all applicable beefy pallet tests
Lederstrumpf 36e8acf
ckb: disable default-features
Lederstrumpf 3198cb7
implement mmr_api v3 on rococo/westend
Lederstrumpf 16a80f8
fmt
Lederstrumpf 70b970b
require at least header OR ancestry proof, not both
Lederstrumpf 04db083
remove headers from equiv. proofs in frame tests
Lederstrumpf 933cb64
refactor fisherman: cleanup
Lederstrumpf 223370a
handle "equivocations" on future blocks
Lederstrumpf ab891da
update documentation comment
Lederstrumpf ab83492
refactor `check_fork_equivocation_proof`
Lederstrumpf 912d5dc
recurse De Morgan's Laws
Lederstrumpf 6efed14
loosen proof failure condition
Lederstrumpf ccac61f
refactor fork equivocation check
Lederstrumpf dc56cab
refactor flow
Lederstrumpf 55f8932
split up fork equiv. proof check
Lederstrumpf f71b3ad
refactor: comments & conciseness
Lederstrumpf a737d2c
Revert "integrate new proof data structure into api"
Lederstrumpf ba66fde
Revert "fix Proof type parameter usage"
Lederstrumpf 5d263a4
Revert "adjust tests for new proof data structure"
Lederstrumpf 442d3e9
cleanup api change reversion
Lederstrumpf 4200f5b
bump ckb-merkle-mountain-range 0.5.2 -> 0.6.0
Lederstrumpf e1269fd
segregate Proofs and NodeProofs
Lederstrumpf 7222fa5
primitives::{Proof->LeafProof}
Lederstrumpf 2e3cb9f
bump mmr-lib
Lederstrumpf 9a32a60
fix kitchensink runtime MmrApi impl
Lederstrumpf 828502c
test-runtime: impl MmrApi
Lederstrumpf b185596
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf 9da3ea5
account for #1189
Lederstrumpf 12b8eec
Revert "fix proof serde tests"
Lederstrumpf 28cf54a
fix docs
Lederstrumpf f8bf630
refactor check_fork_equivocation_proof
Lederstrumpf 87652dc
test slashing of future block commitments/votes
Lederstrumpf 5ac0893
fisher: don't fail on key ownership proof errors
Lederstrumpf 5f47c9d
report_fork_equivocation: Result<{()->bool}, ..>
Lederstrumpf 96de837
test all combinations of header & ancestry proofs
Lederstrumpf c46ebe7
slash commitments to future blocks
Lederstrumpf 784aef1
only report equivocation if non-zero signers
Lederstrumpf d7c5373
improve docs & naming
Lederstrumpf 86629c2
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf b9e8052
account for #2446
Lederstrumpf 9a974f9
bump `BeefyApi` version
Lederstrumpf dbb62dc
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf 3196327
fix `should_initialize_voter_at_custom_genesis`
Lederstrumpf ef5ad84
forward-port `{correct→canonical}_header`
Lederstrumpf bebd108
`{expected→canonical}_...` renames
Lederstrumpf 9d917f9
impl serde for `application_crypto::Signature`
Lederstrumpf a123e78
impl serde for `sp_core::bandersnatch::Signature`
Lederstrumpf 7a28ac3
stub for `check_versioned_finality_proof`
Lederstrumpf 940d846
update docs
Lederstrumpf e680e6b
Doc: payload checking reports equivocations too
Lederstrumpf 0f8c8bd
Revert "impl serde for `sp_core::bandersnatch::Signature`"
Lederstrumpf abea602
Revert "impl serde for `application_crypto::Signature`"
Lederstrumpf 10d9ed5
Revert "stub for `check_versioned_finality_proof`"
Lederstrumpf 20968b1
fixup! Doc: payload checking reports equivocations too
Lederstrumpf 370f765
filter invalidly signed commitments at call site
Lederstrumpf 28502cd
Apply suggestions from code review
Lederstrumpf d18897d
migrate BeefyFisherman trait fns into Fisherman
Lederstrumpf 8a2a4af
consistent lexical ordering of generic parameters
Lederstrumpf 3904ce5
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf 35b302d
account for #2856
Lederstrumpf ddee581
fix sc_consensus_beefy integration tests
Lederstrumpf 30ea46c
fix sc_consensus_beefy::communication::gossip "unit" tests
Lederstrumpf 0283226
fmt
Lederstrumpf a639db6
revert removal of header hash verification
Lederstrumpf ba13cf2
fix missed rename
Lederstrumpf 7078598
distinguish weights for report_{fork,vote}_equivocation
Lederstrumpf fa73cb5
clippy lints
Lederstrumpf c01784c
tighten equivocation reporting: future -> unfinalized blocks
Lederstrumpf dc84223
Revert "tighten equivocation reporting: future -> unfinalized blocks"
Lederstrumpf 816b726
use `expect_validator_set_nonblocking`
Lederstrumpf d4902be
Check equivocations on all votes & proofs for non-active rounds
Lederstrumpf 3e9a4a7
Check equivocations on all votes & proofs
Lederstrumpf 6be328f
add CanonicalHashHeaderPayload struct
Lederstrumpf e401534
update TODO
Lederstrumpf 4dbfcf3
filter signatories without key ownership proofs
Lederstrumpf e7718fc
debug->warn
Lederstrumpf fb183c9
check session index first as cheap failfast
Lederstrumpf 20b828b
deduplicate ancestry proof generation
Lederstrumpf 09f9556
{canonical->finalized} hash for anc. proof gen
Lederstrumpf 7c529a6
fixup! filter invalidly signed commitments at call site
Lederstrumpf c64286c
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf b21c6e1
merge fixes
Lederstrumpf 1848306
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf be3cbd8
merge fixes
Lederstrumpf 11095c7
clippy
Lederstrumpf e208014
align set_id in beefy key ownership tests
Lederstrumpf ab3f417
sp beefy: feature gate Block trait requirement
Lederstrumpf 914caf9
constrain NodeHash as HashOutput
Lederstrumpf 730df52
straight plumbing impl using wrapper for mmr_root
Lederstrumpf 42b6e4f
use Hashing trait generic
Lederstrumpf 6199919
remove canonical root from outer wrapper
Lederstrumpf 4a6633f
remove generic from beefy pallet CheckFEP type
Lederstrumpf b0c3ce3
add CheckForkEquivocationProof type to runtimes
Lederstrumpf b4aa158
remove Hasher generic from outer call
Lederstrumpf e4caca4
replace trait generic & pallet_mmr dependency
Lederstrumpf c42d22a
remove all pallet_mmr::Config deps of pallet_beefy
Lederstrumpf 95ab3fe
move all fork equiv. checking to assoc fn of trait
Lederstrumpf a543c28
remove contrived degree of freedom: BlockNumber
Lederstrumpf e17b7ca
remove contrived degree of freedom: Signature
Lederstrumpf 6081f25
cleanup return type
Lederstrumpf a814740
remove contrived degree of freedom: Hasher
Lederstrumpf ee4c6c5
consistent Hash/HashT usage
Lederstrumpf 5502a8e
remove redundant mmr_size alias
Lederstrumpf 3913df6
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf d27fc06
reuse verify_with_validator_set
Lederstrumpf 7dd7c3d
deduplicate OpaqueKeyOwnershipProof decoding
Lederstrumpf bb6bd8d
specific mock impl for CheckForkEquivocationProof
Lederstrumpf e291372
add missing versioning on westend's BeefyApi
Lederstrumpf d8625d8
{canonical→best}_root
Lederstrumpf c0ccc9c
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf f8928dd
Merge remote-tracking branch 'upstream/master' into beefy-slashing-fi…
serban300 8d7e6df
Fix correct_beefy_payload() test
serban300 c23f4d2
Address review comments
serban300 dd6ff56
fix CI
serban300 cf51037
taplo
serban300 114a894
Fix tests
serban300 c8e6802
fix rustdoc
serban300 6aacc7d
Use master branch of https://github.com/paritytech/merkle-mountain-ra…
serban300 04b7412
Merge remote-tracking branch 'upstream/master' into beefy-slashing-fi…
serban300 1f75173
Cosmetics
serban300 a84681f
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf 938a211
destructure `KnownSignature` for incumbent tuple interface
Lederstrumpf d4b479d
Use KnownSignature instead of tuple
serban300 06492da
enforce unique equivocator identities in evidence
Lederstrumpf c34617a
bump key ownership proof gen. failures to warnings
Lederstrumpf f603af5
Merge remote-tracking branch 'upstream/master' into beefy-slashing-fi…
serban300 b65b7a5
clarify `ForkEquivocationProof::check`'s params
Lederstrumpf 9deeef3
Merge remote-tracking branch 'origin/master' into rhmb/beefy-slashing…
Lederstrumpf 3913018
account for #4430
Lederstrumpf ada8bdb
add fisherman to `OnDemandJustificationsEngine`
Lederstrumpf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
check session index first as cheap failfast
Suggested-by: Serban Iorga <[email protected]>
- Loading branch information
commit fb183c9ef8af1422ba60741aaeb718d105b36d1a
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move the conversation from #1329 (comment) here please.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@serban300 I see your point that this would still be a DoS vulnerability.
Do you agree that this would then be an issue affecting other call sites of the offences pallet too?
If yes, then I'd still suggest this should be fixed in the offences pallet (I reckon in another PR).
A potential solution that comes to mind is to use a
key🡒key_owner_proofmap rather than a vector for storing theoffenders.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how the offences pallet is used in other places, but for example it wouldn't affect the way it's used from the Grandpa pallet. Because there we only need to send 2 votes for different forks. It seems to be something that depends mostly on the design of the call site. So looks like it would be the responsibility of the call site to handle this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough - in both grandpa & babe, evidence is always for a single offender:
If another call site that allows multiple offenders to be reported within the same evidence gets added, I'd suggest moving the handling to the offences pallet (such as changing to a
key🡒key_owner_proofhashmap).I've added some handling that enforces offenders are unique here by coercing the offenders into a set and checking its length against the vectors. If any offender is duplicated, the evidence is discarded rather than amended, to put the onus of correct equivocation evidence on the reporter.
I used
BTreeSetbut have little opinion on the set type used - e.g. hashbrown'sHashSetis another viable choice, and it's already an (indirect) dependency of other pallets. Itertools or sorting&deduplicating the vec are alternative solutions too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks !
Just one more thing. For the same reason we should also check if the ancestry proof is not bloated (if it doesn't contain unnecessary nodes). I don't know if this is even possible, but leaving it here in order to not forget about it.
@Lederstrumpf do you know if it's possible to have bloated ancestry proofs ? Or if they contain more nodes then necessary, the verification will fail ?