From ee6c180a15bbafa14a6145ca2c6eb83c55f40fee Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 5 Jul 2023 12:02:07 +0300 Subject: [PATCH 01/41] sc-consensus-beefy: add BEEFY fisherman to gossip network Signed-off-by: Adrian Catangiu --- .../beefy/src/communication/fisherman.rs | 113 ++++++++++++++++++ .../beefy/src/communication/gossip.rs | 46 +++++-- .../consensus/beefy/src/communication/mod.rs | 1 + client/consensus/beefy/src/lib.rs | 13 +- client/consensus/beefy/src/tests.rs | 27 ++++- client/consensus/beefy/src/worker.rs | 26 +++- primitives/consensus/beefy/src/commitment.rs | 15 +++ primitives/consensus/beefy/src/mmr.rs | 8 +- primitives/consensus/beefy/src/payload.rs | 2 +- 9 files changed, 223 insertions(+), 28 deletions(-) create mode 100644 client/consensus/beefy/src/communication/fisherman.rs diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs new file mode 100644 index 0000000000000..46e2458cd92cd --- /dev/null +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -0,0 +1,113 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +use crate::justification::BeefyVersionedFinalityProof; +use sc_client_api::Backend; +use sp_api::ProvideRuntimeApi; +use sp_blockchain::HeaderBackend; +use sp_consensus_beefy::{ + crypto::{AuthorityId, Signature}, + BeefyApi, Payload, PayloadProvider, VoteMessage, +}; +use sp_runtime::{ + generic::BlockId, + traits::{Block, NumberFor}, +}; +use std::{marker::PhantomData, sync::Arc}; + +pub(crate) trait BeefyFisherman: Send + Sync { + /// Check `vote` for contained finalized block against expected payload. + /// + /// Note: this fn expects `vote.commitment.block_number` to be finalized. + fn check_vote( + &self, + vote: VoteMessage, AuthorityId, Signature>, + ) -> Result<(), sp_blockchain::Error>; + + /// Check `proof` for contained finalized block against expected payload. + /// + /// Note: this fn expects block referenced in `proof` to be finalized. + fn check_proof( + &self, + proof: BeefyVersionedFinalityProof, + ) -> Result<(), sp_blockchain::Error>; +} + +/// Helper wrapper used to check gossiped votes for (historical) equivocations, +/// and report any such protocol infringements. +pub(crate) struct Fisherman { + pub backend: Arc, + pub runtime: Arc, + pub payload_provider: P, + pub _phantom: PhantomData, +} + +impl Fisherman +where + B: Block, + BE: Backend, + P: PayloadProvider, +{ + fn expected_payload(&self, number: NumberFor) -> Result { + // This should be un-ambiguous since `number` is finalized. + let hash = self.backend.blockchain().expect_block_hash_from_id(&BlockId::Number(number))?; + let header = self.backend.blockchain().expect_header(hash)?; + self.payload_provider + .payload(&header) + .ok_or_else(|| sp_blockchain::Error::Backend("BEEFY Payload not found".into())) + } +} + +impl BeefyFisherman for Fisherman +where + B: Block, + BE: Backend, + P: PayloadProvider, + R: ProvideRuntimeApi + Send + Sync, + R::Api: BeefyApi, +{ + /// Check `vote` for contained finalized block against expected payload. + /// + /// Note: this fn expects `vote.commitment.block_number` to be finalized. + fn check_vote( + &self, + vote: VoteMessage, AuthorityId, Signature>, + ) -> Result<(), sp_blockchain::Error> { + let number = vote.commitment.block_number; + let expected = self.expected_payload(number)?; + if vote.commitment.payload != expected { + // TODO: report equivocation + } + Ok(()) + } + + /// Check `proof` for contained finalized block against expected payload. + /// + /// Note: this fn expects block referenced in `proof` to be finalized. + fn check_proof( + &self, + proof: BeefyVersionedFinalityProof, + ) -> Result<(), sp_blockchain::Error> { + let payload = proof.payload(); + let expected = self.expected_payload(*proof.number())?; + if payload != &expected { + // TODO: report equivocation + } + Ok(()) + } +} diff --git a/client/consensus/beefy/src/communication/gossip.rs b/client/consensus/beefy/src/communication/gossip.rs index 8c025ca067619..5b11b58849997 100644 --- a/client/consensus/beefy/src/communication/gossip.rs +++ b/client/consensus/beefy/src/communication/gossip.rs @@ -32,6 +32,7 @@ use wasm_timer::Instant; use crate::{ communication::{ benefit, cost, + fisherman::BeefyFisherman, peers::{KnownPeers, PeerReport}, }, justification::{ @@ -225,26 +226,29 @@ impl Filter { /// Allows messages for 'rounds >= last concluded' to flow, everything else gets /// rejected/expired. /// +/// Messages for active and expired rounds are validated for expected payloads and attempts +/// to create forks before head of GRANDPA are reported. +/// ///All messaging is handled in a single BEEFY global topic. -pub(crate) struct GossipValidator -where - B: Block, -{ +pub(crate) struct GossipValidator { votes_topic: B::Hash, justifs_topic: B::Hash, gossip_filter: RwLock>, next_rebroadcast: Mutex, known_peers: Arc>>, report_sender: TracingUnboundedSender, + fisherman: F, } -impl GossipValidator +impl GossipValidator where B: Block, + F: BeefyFisherman, { pub(crate) fn new( known_peers: Arc>>, - ) -> (GossipValidator, TracingUnboundedReceiver) { + fisherman: F, + ) -> (GossipValidator, TracingUnboundedReceiver) { let (tx, rx) = tracing_unbounded("mpsc_beefy_gossip_validator", 10_000); let val = GossipValidator { votes_topic: votes_topic::(), @@ -253,6 +257,7 @@ where next_rebroadcast: Mutex::new(Instant::now() + REBROADCAST_AFTER), known_peers, report_sender: tx, + fisherman, }; (val, rx) } @@ -287,9 +292,14 @@ where let filter = self.gossip_filter.read(); match filter.consider_vote(round, set_id) { - Consider::RejectPast => return Action::Discard(cost::OUTDATED_MESSAGE), Consider::RejectFuture => return Action::Discard(cost::FUTURE_MESSAGE), Consider::RejectOutOfScope => return Action::Discard(cost::OUT_OF_SCOPE_MESSAGE), + Consider::RejectPast => { + // We know `vote` is for some past (finalized) block. Have fisherman check + // for equivocations. Best-effort, ignore errors such as state pruned. + let _ = self.fisherman.check_vote(vote); + return Action::Discard(cost::OUTDATED_MESSAGE) + }, Consider::Accept => {}, } @@ -331,9 +341,14 @@ where let guard = self.gossip_filter.read(); // Verify general usefulness of the justification. match guard.consider_finality_proof(round, set_id) { - Consider::RejectPast => return Action::Discard(cost::OUTDATED_MESSAGE), Consider::RejectFuture => return Action::Discard(cost::FUTURE_MESSAGE), Consider::RejectOutOfScope => return Action::Discard(cost::OUT_OF_SCOPE_MESSAGE), + Consider::RejectPast => { + // We know `proof` is for some past (finalized) block. Have fisherman check + // for equivocations. Best-effort, ignore errors such as state pruned. + let _ = self.fisherman.check_proof(proof); + return Action::Discard(cost::OUTDATED_MESSAGE) + }, Consider::Accept => {}, } // Verify justification signatures. @@ -359,9 +374,10 @@ where } } -impl Validator for GossipValidator +impl Validator for GossipValidator where B: Block, + F: BeefyFisherman, { fn peer_disconnected(&self, _context: &mut dyn ValidatorContext, who: &PeerId) { self.known_peers.lock().remove(who); @@ -474,7 +490,7 @@ where #[cfg(test)] pub(crate) mod tests { use super::*; - use crate::keystore::BeefyKeystore; + use crate::{keystore::BeefyKeystore, tests::DummyFisherman}; use sc_network_test::Block; use sp_application_crypto::key_types::BEEFY as BEEFY_KEY_TYPE; use sp_consensus_beefy::{ @@ -482,6 +498,7 @@ pub(crate) mod tests { SignedCommitment, VoteMessage, }; use sp_keystore::{testing::MemoryKeystore, Keystore}; + use std::marker::PhantomData; #[test] fn known_votes_insert_remove() { @@ -577,8 +594,9 @@ pub(crate) mod tests { fn should_validate_messages() { let keys = vec![Keyring::Alice.public()]; let validator_set = ValidatorSet::::new(keys.clone(), 0).unwrap(); + let fisherman = DummyFisherman { _phantom: PhantomData:: }; let (gv, mut report_stream) = - GossipValidator::::new(Arc::new(Mutex::new(KnownPeers::new()))); + GossipValidator::new(Arc::new(Mutex::new(KnownPeers::new())), fisherman); let sender = PeerId::random(); let mut context = TestContext; @@ -705,7 +723,8 @@ pub(crate) mod tests { fn messages_allowed_and_expired() { let keys = vec![Keyring::Alice.public()]; let validator_set = ValidatorSet::::new(keys.clone(), 0).unwrap(); - let (gv, _) = GossipValidator::::new(Arc::new(Mutex::new(KnownPeers::new()))); + let fisherman = DummyFisherman { _phantom: PhantomData:: }; + let (gv, _) = GossipValidator::new(Arc::new(Mutex::new(KnownPeers::new())), fisherman); gv.update_filter(GossipFilterCfg { start: 0, end: 10, validator_set: &validator_set }); let sender = sc_network::PeerId::random(); let topic = Default::default(); @@ -782,7 +801,8 @@ pub(crate) mod tests { fn messages_rebroadcast() { let keys = vec![Keyring::Alice.public()]; let validator_set = ValidatorSet::::new(keys.clone(), 0).unwrap(); - let (gv, _) = GossipValidator::::new(Arc::new(Mutex::new(KnownPeers::new()))); + let fisherman = DummyFisherman { _phantom: PhantomData:: }; + let (gv, _) = GossipValidator::new(Arc::new(Mutex::new(KnownPeers::new())), fisherman); gv.update_filter(GossipFilterCfg { start: 0, end: 10, validator_set: &validator_set }); let sender = sc_network::PeerId::random(); let topic = Default::default(); diff --git a/client/consensus/beefy/src/communication/mod.rs b/client/consensus/beefy/src/communication/mod.rs index 7f9535bfc23f1..c31447d08e565 100644 --- a/client/consensus/beefy/src/communication/mod.rs +++ b/client/consensus/beefy/src/communication/mod.rs @@ -21,6 +21,7 @@ pub mod notification; pub mod request_response; +pub(crate) mod fisherman; pub(crate) mod gossip; pub(crate) mod peers; diff --git a/client/consensus/beefy/src/lib.rs b/client/consensus/beefy/src/lib.rs index 2d2cbd0482784..ba55a5b46a6e2 100644 --- a/client/consensus/beefy/src/lib.rs +++ b/client/consensus/beefy/src/lib.rs @@ -18,6 +18,7 @@ use crate::{ communication::{ + fisherman::Fisherman, notification::{ BeefyBestBlockSender, BeefyBestBlockStream, BeefyVersionedFinalityProofSender, BeefyVersionedFinalityProofStream, @@ -221,10 +222,10 @@ pub async fn start_beefy_gadget( beefy_params: BeefyParams, ) where B: Block, - BE: Backend, + BE: Backend + 'static, C: Client + BlockBackend, P: PayloadProvider, - R: ProvideRuntimeApi, + R: ProvideRuntimeApi + Send + Sync + 'static, R::Api: BeefyApi + MmrApi>, N: GossipNetwork + NetworkRequest + Send + Sync + 'static, S: GossipSyncing + SyncOracle + 'static, @@ -251,10 +252,16 @@ pub async fn start_beefy_gadget( } = network_params; let known_peers = Arc::new(Mutex::new(KnownPeers::new())); + let fisherman = Fisherman { + backend: backend.clone(), + runtime: runtime.clone(), + payload_provider: payload_provider.clone(), + _phantom: PhantomData, + }; // Default votes filter is to discard everything. // Validator is updated later with correct starting round and set id. let (gossip_validator, gossip_report_stream) = - communication::gossip::GossipValidator::new(known_peers.clone()); + communication::gossip::GossipValidator::new(known_peers.clone(), fisherman); let gossip_validator = Arc::new(gossip_validator); let mut gossip_engine = GossipEngine::new( network.clone(), diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 1109a22638221..15622efa36ca2 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -22,6 +22,7 @@ use crate::{ aux_schema::{load_persistent, tests::verify_persisted_version}, beefy_block_import_and_links, communication::{ + fisherman::BeefyFisherman, gossip::{ proofs_topic, tests::sign_commitment, votes_topic, GossipFilterCfg, GossipMessage, GossipValidator, @@ -47,7 +48,7 @@ use sc_network_test::{ }; use sc_utils::notification::NotificationReceiver; use serde::{Deserialize, Serialize}; -use sp_api::{ApiRef, ProvideRuntimeApi}; +use sp_api::{ApiRef, BlockT, ProvideRuntimeApi}; use sp_application_crypto::key_types::BEEFY as BEEFY_KEY_TYPE; use sp_consensus::BlockOrigin; use sp_consensus_beefy::{ @@ -244,6 +245,22 @@ impl TestNetFactory for BeefyTestNet { } } +pub(crate) struct DummyFisherman { + pub _phantom: PhantomData, +} + +impl BeefyFisherman for DummyFisherman { + fn check_proof(&self, _: BeefyVersionedFinalityProof) -> Result<(), sp_blockchain::Error> { + Ok(()) + } + fn check_vote( + &self, + _: VoteMessage, AuthorityId, Signature>, + ) -> Result<(), sp_blockchain::Error> { + Ok(()) + } +} + #[derive(Clone)] pub(crate) struct TestApi { pub beefy_genesis: u64, @@ -364,8 +381,9 @@ async fn voter_init_setup( api: &TestApi, ) -> sp_blockchain::Result> { let backend = net.peer(0).client().as_backend(); + let fisherman = DummyFisherman { _phantom: PhantomData }; let known_peers = Arc::new(Mutex::new(KnownPeers::new())); - let (gossip_validator, _) = GossipValidator::new(known_peers); + let (gossip_validator, _) = GossipValidator::new(known_peers, fisherman); let gossip_validator = Arc::new(gossip_validator); let mut gossip_engine = sc_network_gossip::GossipEngine::new( net.peer(0).network_service().clone(), @@ -386,7 +404,7 @@ fn initialize_beefy( min_block_delta: u32, ) -> impl Future where - API: ProvideRuntimeApi + Sync + Send, + API: ProvideRuntimeApi + Sync + Send +'static, API::Api: BeefyApi + MmrApi>, { let tasks = FuturesUnordered::new(); @@ -1310,9 +1328,10 @@ async fn gossipped_finality_proofs() { let beefy_peers = peers.iter().enumerate().map(|(id, key)| (id, key, api.clone())).collect(); let charlie = &net.peers[2]; + let fisherman = DummyFisherman { _phantom: PhantomData:: }; let known_peers = Arc::new(Mutex::new(KnownPeers::::new())); // Charlie will run just the gossip engine and not the full voter. - let (gossip_validator, _) = GossipValidator::new(known_peers); + let (gossip_validator, _) = GossipValidator::new(known_peers, fisherman); let charlie_gossip_validator = Arc::new(gossip_validator); charlie_gossip_validator.update_filter(GossipFilterCfg:: { start: 1, diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index 17a8891b06142..64e786c1fb6b7 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -18,6 +18,7 @@ use crate::{ communication::{ + fisherman::BeefyFisherman, gossip::{proofs_topic, votes_topic, GossipFilterCfg, GossipMessage, GossipValidator}, peers::PeerReport, request_response::outgoing_requests_engine::{OnDemandJustificationsEngine, ResponseInfo}, @@ -314,7 +315,7 @@ impl PersistedState { } /// A BEEFY worker plays the BEEFY protocol -pub(crate) struct BeefyWorker { +pub(crate) struct BeefyWorker { // utilities pub backend: Arc, pub payload_provider: P, @@ -324,7 +325,7 @@ pub(crate) struct BeefyWorker { // communication pub gossip_engine: GossipEngine, - pub gossip_validator: Arc>, + pub gossip_validator: Arc>, pub gossip_report_stream: TracingUnboundedReceiver, pub on_demand_justifications: OnDemandJustificationsEngine, @@ -341,7 +342,7 @@ pub(crate) struct BeefyWorker { pub persisted_state: PersistedState, } -impl BeefyWorker +impl BeefyWorker where B: Block + Codec, BE: Backend, @@ -349,6 +350,7 @@ where S: SyncOracle, R: ProvideRuntimeApi, R::Api: BeefyApi, + F: BeefyFisherman, { fn best_grandpa_block(&self) -> NumberFor { *self.persisted_state.voting_oracle.best_grandpa_block_header.number() @@ -1041,7 +1043,10 @@ where pub(crate) mod tests { use super::*; use crate::{ - communication::notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream}, + communication::{ + fisherman::Fisherman, + notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream}, + }, tests::{ create_beefy_keystore, get_beefy_streams, make_beefy_ids, BeefyPeer, BeefyTestNet, TestApi, @@ -1060,6 +1065,7 @@ pub(crate) mod tests { mmr::MmrRootProvider, Keyring, Payload, SignedCommitment, }; use sp_runtime::traits::One; + use std::marker::PhantomData; use substrate_test_runtime_client::{ runtime::{Block, Digest, DigestItem, Header}, Backend, @@ -1100,6 +1106,7 @@ pub(crate) mod tests { MmrRootProvider, TestApi, Arc>, + Fisherman>, > { let keystore = create_beefy_keystore(*key); @@ -1125,8 +1132,16 @@ pub(crate) mod tests { let api = Arc::new(TestApi::with_validator_set(&genesis_validator_set)); let network = peer.network_service().clone(); let sync = peer.sync_service().clone(); + let payload_provider = MmrRootProvider::new(api.clone()); + let fisherman = Fisherman { + backend: backend.clone(), + runtime: api.clone(), + payload_provider: payload_provider.clone(), + _phantom: PhantomData, + }; let known_peers = Arc::new(Mutex::new(KnownPeers::new())); - let (gossip_validator, gossip_report_stream) = GossipValidator::new(known_peers.clone()); + let (gossip_validator, gossip_report_stream) = + GossipValidator::new(known_peers.clone(), fisherman); let gossip_validator = Arc::new(gossip_validator); let gossip_engine = GossipEngine::new( network.clone(), @@ -1157,7 +1172,6 @@ pub(crate) mod tests { beefy_genesis, ) .unwrap(); - let payload_provider = MmrRootProvider::new(api.clone()); BeefyWorker { backend, payload_provider, diff --git a/primitives/consensus/beefy/src/commitment.rs b/primitives/consensus/beefy/src/commitment.rs index 5b6ef9ae5ab36..aa2efdb10ca22 100644 --- a/primitives/consensus/beefy/src/commitment.rs +++ b/primitives/consensus/beefy/src/commitment.rs @@ -247,6 +247,21 @@ impl From> for VersionedFinalityProof { } } +impl VersionedFinalityProof { + /// Provide reference to inner `Payload`. + pub fn payload(&self) -> &Payload { + match self { + VersionedFinalityProof::V1(inner) => &inner.commitment.payload, + } + } + /// Block number this proof is for. + pub fn number(&self) -> &N { + match self { + VersionedFinalityProof::V1(inner) => &inner.commitment.block_number, + } + } +} + #[cfg(test)] mod tests { diff --git a/primitives/consensus/beefy/src/mmr.rs b/primitives/consensus/beefy/src/mmr.rs index 991dc07c5a7f3..b318d84475f7f 100644 --- a/primitives/consensus/beefy/src/mmr.rs +++ b/primitives/consensus/beefy/src/mmr.rs @@ -162,6 +162,12 @@ mod mmr_root_provider { _phantom: PhantomData, } + impl Clone for MmrRootProvider { + fn clone(&self) -> Self { + Self { runtime: self.runtime.clone(), _phantom: PhantomData } + } + } + impl MmrRootProvider where B: Block, @@ -184,7 +190,7 @@ mod mmr_root_provider { impl PayloadProvider for MmrRootProvider where B: Block, - R: ProvideRuntimeApi, + R: ProvideRuntimeApi + Send + Sync + 'static, R::Api: MmrApi>, { fn payload(&self, header: &B::Header) -> Option { diff --git a/primitives/consensus/beefy/src/payload.rs b/primitives/consensus/beefy/src/payload.rs index d520de445c95a..c80f00255281b 100644 --- a/primitives/consensus/beefy/src/payload.rs +++ b/primitives/consensus/beefy/src/payload.rs @@ -75,7 +75,7 @@ impl Payload { } /// Trait for custom BEEFY payload providers. -pub trait PayloadProvider { +pub trait PayloadProvider: Clone + Send + Sync + 'static { /// Provide BEEFY payload if available for `header`. fn payload(&self, header: &B::Header) -> Option; } From ce03ec8dc0f03d7e2f82bd42a9a1c537f02afd29 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 5 Jul 2023 14:10:40 +0300 Subject: [PATCH 02/41] sp-consensus-beefy: add invalid fork vote proof and equivalent BeefyApi Signed-off-by: Adrian Catangiu --- primitives/consensus/beefy/src/lib.rs | 70 +++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index c69e26bf574d8..fdfacae037d0a 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -251,6 +251,31 @@ impl EquivocationProof { } } +/// Proof of voter misbehavior on a given set id. +/// This proof shows voter voted on a different fork than finalized by GRANDPA. +#[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)] +pub struct InvalidForkVoteProof { + /// Voting for a block on different fork than one finalized by GRANDPA. + pub vote: VoteMessage, + /// TODO: GRANDPA proof that this block is final + pub grandpa_proof: (), +} + +impl InvalidForkVoteProof { + /// Returns the authority id of the misbehaving voter. + pub fn offender_id(&self) -> &Id { + &self.vote.id + } + /// Returns the round number at which the infringement occurred. + pub fn round_number(&self) -> &Number { + &self.vote.commitment.block_number + } + /// Returns the set id at which the infringement occurred. + pub fn set_id(&self) -> ValidatorSetId { + self.vote.commitment.validator_set_id + } +} + /// Check a commitment signature by encoding the commitment and /// verifying the provided signature using the expected authority id. pub fn check_commitment_signature( @@ -302,6 +327,37 @@ where return valid_first && valid_second } +/// Validates [InvalidForkVoteProof] by checking: +/// 1. `vote` is signed, +/// 2. GRANDPA proof is provided for block number >= vote.commitment.block_number. +/// 3. `vote.commitment.payload` != `expected_payload`. +pub fn check_invalid_fork_proof( + proof: &InvalidForkVoteProof::Signature>, + expected_payload: &Payload, +) -> bool +where + Id: BeefyAuthorityId + PartialEq, + Number: Clone + Encode + PartialEq, + MsgHash: Hash, +{ + let InvalidForkVoteProof { vote, grandpa_proof: _ } = proof; + + // check signature `vote`, if invalid, equivocation report is invalid + if !check_commitment_signature(&vote.commitment, &vote.id, &vote.signature) { + return false + } + + // TODO: add GRANDPA proof + // if GRANDPA proof doesn't show `vote.commitment.block_number` has been finalized, + // we cannot safely prove that `vote` payload is invalid. + // if grandpa_proof.block_number < vote.commitment.block_number { + // return false + // } + + // check that `payload` on the `vote` is different that the `expected_payload`. + &vote.commitment.payload != expected_payload +} + /// New BEEFY validator set notification hook. pub trait OnNewValidatorSet { /// Function called by the pallet when BEEFY validator set changes. @@ -364,6 +420,20 @@ sp_api::decl_runtime_apis! { key_owner_proof: OpaqueKeyOwnershipProof, ) -> Option<()>; + /// Submits an unsigned extrinsic to report an vote for an invalid fork. + /// The caller must provide the invalid vote proof and a key ownership proof + /// (should be obtained using `generate_key_ownership_proof`). The + /// extrinsic will be unsigned and should only be accepted for local + /// authorship (not to be broadcast to the network). This method returns + /// `None` when creation of the extrinsic fails, e.g. if equivocation + /// reporting is disabled for the given runtime (i.e. this method is + /// hardcoded to return `None`). Only useful in an offchain context. + fn submit_report_invalid_fork_unsigned_extrinsic( + invalid_fork_proof: + InvalidForkVoteProof, crypto::AuthorityId, crypto::Signature>, + key_owner_proof: OpaqueKeyOwnershipProof, + ) -> Option<()>; + /// Generates a proof of key ownership for the given authority in the /// given set. An example usage of this module is coupled with the /// session historical module to prove that a given authority key is From e95e316250c2d1a86313f58cdc6475fafd61c13f Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 5 Jul 2023 14:22:00 +0300 Subject: [PATCH 03/41] pallet-beefy: add stubs for reporting invalid fork votes Signed-off-by: Adrian Catangiu --- frame/beefy/src/equivocation.rs | 1 + frame/beefy/src/lib.rs | 63 +++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 84c62fc47f50a..92dd8cb81e64e 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -125,6 +125,7 @@ where pub struct EquivocationReportSystem(sp_std::marker::PhantomData<(T, R, P, L)>); /// Equivocation evidence convenience alias. +// TODO: use an enum that takes either `EquivocationProof` or `InvalidForkVoteProof` pub type EquivocationEvidenceFor = ( EquivocationProof< BlockNumberFor, diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 35d3273e1ef76..3c61bce844fff 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -63,6 +63,7 @@ const LOG_TARGET: &str = "runtime::beefy"; pub mod pallet { use super::*; use frame_system::pallet_prelude::BlockNumberFor; + use sp_consensus_beefy::InvalidForkVoteProof; #[pallet::config] pub trait Config: frame_system::Config { @@ -111,6 +112,8 @@ pub mod pallet { /// Defines methods to publish, check and process an equivocation offence. type EquivocationReportSystem: OffenceReportSystem< Option, + // TODO: make below an enum that takes either `EquivocationProof` or + // `InvalidForkVoteProof` EquivocationEvidenceFor, >; } @@ -265,6 +268,66 @@ pub mod pallet { )?; Ok(Pays::No.into()) } + + /// Report voter voting on invalid fork. This method will verify the + /// invalid fork proof and validate the given key ownership proof + /// against the extracted offender. If both are valid, the offence + /// will be reported. + #[pallet::call_index(2)] + #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count()))] + pub fn report_invalid_fork_vote( + origin: OriginFor, + invalid_fork_proof: Box< + InvalidForkVoteProof< + BlockNumberFor, + T::BeefyId, + ::Signature, + >, + >, + key_owner_proof: T::KeyOwnerProof, + ) -> DispatchResultWithPostInfo { + let reporter = ensure_signed(origin)?; + + // TODO: + // T::EquivocationReportSystem::process_evidence( + // Some(reporter), + // (*invalid_fork_proof, key_owner_proof), + // )?; + // Waive the fee since the report is valid and beneficial + Ok(Pays::No.into()) + } + + /// Report voter voting on invalid fork. This method will verify the + /// invalid fork proof and validate the given key ownership proof + /// against the extracted offender. If both are valid, the offence + /// will be reported. + /// + /// This extrinsic must be called unsigned and it is expected that only + /// block authors will call it (validated in `ValidateUnsigned`), as such + /// if the block author is defined it will be defined as the equivocation + /// reporter. + #[pallet::call_index(3)] + #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count()))] + pub fn report_invalid_fork_vote_unsigned( + origin: OriginFor, + invalid_fork_proof: Box< + InvalidForkVoteProof< + BlockNumberFor, + T::BeefyId, + ::Signature, + >, + >, + key_owner_proof: T::KeyOwnerProof, + ) -> DispatchResultWithPostInfo { + ensure_none(origin)?; + + // TODO: + // T::EquivocationReportSystem::process_evidence( + // None, + // (*invalid_fork_proof, key_owner_proof), + // )?; + Ok(Pays::No.into()) + } } #[pallet::validate_unsigned] From 6651b32f5cf49fb64ca658ec8e10c174c54b9e18 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 5 Jul 2023 15:52:27 +0300 Subject: [PATCH 04/41] sc-consensus-beefy: fisherman reports invalid votes and justifications Signed-off-by: Adrian Catangiu --- .../beefy/src/communication/fisherman.rs | 140 +++++++++++++++--- .../beefy/src/communication/gossip.rs | 8 + client/consensus/beefy/src/tests.rs | 5 +- primitives/consensus/beefy/src/commitment.rs | 4 +- 4 files changed, 129 insertions(+), 28 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 46e2458cd92cd..dd778fa711371 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -16,17 +16,22 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -use crate::justification::BeefyVersionedFinalityProof; +use crate::{ + error::Error, justification::BeefyVersionedFinalityProof, keystore::BeefySignatureHasher, + LOG_TARGET, +}; +use log::debug; use sc_client_api::Backend; use sp_api::ProvideRuntimeApi; use sp_blockchain::HeaderBackend; use sp_consensus_beefy::{ + check_invalid_fork_proof, crypto::{AuthorityId, Signature}, - BeefyApi, Payload, PayloadProvider, VoteMessage, + BeefyApi, InvalidForkVoteProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, }; use sp_runtime::{ generic::BlockId, - traits::{Block, NumberFor}, + traits::{Block, Header, NumberFor}, }; use std::{marker::PhantomData, sync::Arc}; @@ -37,15 +42,12 @@ pub(crate) trait BeefyFisherman: Send + Sync { fn check_vote( &self, vote: VoteMessage, AuthorityId, Signature>, - ) -> Result<(), sp_blockchain::Error>; + ) -> Result<(), Error>; /// Check `proof` for contained finalized block against expected payload. /// /// Note: this fn expects block referenced in `proof` to be finalized. - fn check_proof( - &self, - proof: BeefyVersionedFinalityProof, - ) -> Result<(), sp_blockchain::Error>; + fn check_proof(&self, proof: BeefyVersionedFinalityProof) -> Result<(), Error>; } /// Helper wrapper used to check gossiped votes for (historical) equivocations, @@ -62,14 +64,79 @@ where B: Block, BE: Backend, P: PayloadProvider, + R: ProvideRuntimeApi + Send + Sync, + R::Api: BeefyApi, { - fn expected_payload(&self, number: NumberFor) -> Result { + fn expected_header_and_payload(&self, number: NumberFor) -> Result<(B::Header, Payload), Error> { // This should be un-ambiguous since `number` is finalized. - let hash = self.backend.blockchain().expect_block_hash_from_id(&BlockId::Number(number))?; - let header = self.backend.blockchain().expect_header(hash)?; + let hash = self + .backend + .blockchain() + .expect_block_hash_from_id(&BlockId::Number(number)) + .map_err(|e| Error::Backend(e.to_string()))?; + let header = self + .backend + .blockchain() + .expect_header(hash) + .map_err(|e| Error::Backend(e.to_string()))?; self.payload_provider .payload(&header) - .ok_or_else(|| sp_blockchain::Error::Backend("BEEFY Payload not found".into())) + .map(|payload| (header, payload)) + .ok_or_else(|| Error::Backend("BEEFY Payload not found".into())) + } + + fn active_validator_set_at( + &self, + header: &B::Header, + ) -> Result, Error> { + self.runtime + .runtime_api() + .validator_set(header.hash()) + .map_err(Error::RuntimeApi)? + .ok_or_else(|| Error::Backend("could not get BEEFY validator set".into())) + } + + fn report_invalid_fork_vote( + &self, + proof: InvalidForkVoteProof, AuthorityId, Signature>, + correct_header: &B::Header, + correct_payload: &Payload, + validator_set: &ValidatorSet, // validator set active at the time + ) -> Result<(), Error> { + let set_id = validator_set.id(); + + if proof.vote.commitment.validator_set_id != set_id || + !check_invalid_fork_proof::<_, _, BeefySignatureHasher>(&proof, correct_payload) + { + debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); + return Ok(()) + } + + let hash = correct_header.hash(); + let offender_id = proof.vote.id.clone(); + let runtime_api = self.runtime.runtime_api(); + // generate key ownership proof at that block + let key_owner_proof = match runtime_api + .generate_key_ownership_proof(hash, set_id, offender_id) + .map_err(Error::RuntimeApi)? + { + Some(proof) => proof, + None => { + debug!( + target: LOG_TARGET, + "🥩 Invalid fork vote offender not part of the authority set." + ); + return Ok(()) + }, + }; + + // submit invalid fork vote report at **best** block + let best_block_hash = self.backend.blockchain().info().best_hash; + runtime_api + .submit_report_invalid_fork_unsigned_extrinsic(best_block_hash, proof, key_owner_proof) + .map_err(Error::RuntimeApi)?; + + Ok(()) } } @@ -87,11 +154,15 @@ where fn check_vote( &self, vote: VoteMessage, AuthorityId, Signature>, - ) -> Result<(), sp_blockchain::Error> { + ) -> Result<(), Error> { let number = vote.commitment.block_number; - let expected = self.expected_payload(number)?; - if vote.commitment.payload != expected { - // TODO: report equivocation + let (header, expected_payload) = self.expected_header_and_payload(number)?; + if vote.commitment.payload != expected_payload { + let validator_set = self.active_validator_set_at(&header)?; + // TODO: create/get grandpa proof for block number + let grandpa_proof = (); + let proof = InvalidForkVoteProof { vote, grandpa_proof }; + self.report_invalid_fork_vote(proof, &header, &expected_payload, &validator_set)?; } Ok(()) } @@ -99,14 +170,35 @@ where /// Check `proof` for contained finalized block against expected payload. /// /// Note: this fn expects block referenced in `proof` to be finalized. - fn check_proof( - &self, - proof: BeefyVersionedFinalityProof, - ) -> Result<(), sp_blockchain::Error> { - let payload = proof.payload(); - let expected = self.expected_payload(*proof.number())?; - if payload != &expected { - // TODO: report equivocation + fn check_proof(&self, proof: BeefyVersionedFinalityProof) -> Result<(), Error> { + let (commitment, signatures) = match proof { + BeefyVersionedFinalityProof::::V1(inner) => (inner.commitment, inner.signatures), + }; + let number = commitment.block_number; + let (header, expected_payload) = self.expected_header_and_payload(number)?; + if commitment.payload != expected_payload { + // TODO: create/get grandpa proof for block number + let grandpa_proof = (); + let validator_set = self.active_validator_set_at(&header)?; + if signatures.len() != validator_set.validators().len() { + // invalid proof + return Ok(()) + } + // report every signer of the bad justification + for signed_pair in validator_set.validators().iter().zip(signatures.into_iter()) { + let (id, signature) = signed_pair; + if let Some(signature) = signature { + let vote = + VoteMessage { commitment: commitment.clone(), id: id.clone(), signature }; + let proof = InvalidForkVoteProof { vote, grandpa_proof }; + self.report_invalid_fork_vote( + proof, + &header, + &expected_payload, + &validator_set, + )?; + } + } } Ok(()) } diff --git a/client/consensus/beefy/src/communication/gossip.rs b/client/consensus/beefy/src/communication/gossip.rs index 5b11b58849997..883f4e48f48ec 100644 --- a/client/consensus/beefy/src/communication/gossip.rs +++ b/client/consensus/beefy/src/communication/gossip.rs @@ -298,6 +298,10 @@ where // We know `vote` is for some past (finalized) block. Have fisherman check // for equivocations. Best-effort, ignore errors such as state pruned. let _ = self.fisherman.check_vote(vote); + // 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. return Action::Discard(cost::OUTDATED_MESSAGE) }, Consider::Accept => {}, @@ -347,6 +351,10 @@ where // We know `proof` is for some past (finalized) block. Have fisherman check // for equivocations. Best-effort, ignore errors such as state pruned. let _ = self.fisherman.check_proof(proof); + // 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. return Action::Discard(cost::OUTDATED_MESSAGE) }, Consider::Accept => {}, diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 15622efa36ca2..2d7af835203a2 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -29,6 +29,7 @@ use crate::{ }, request_response::{on_demand_justifications_protocol_config, BeefyJustifsRequestHandler}, }, + error::Error, gossip_protocol_name, justification::*, load_or_init_voter_state, wait_for_runtime_pallet, BeefyRPCLinks, BeefyVoterLinks, KnownPeers, @@ -250,13 +251,13 @@ pub(crate) struct DummyFisherman { } impl BeefyFisherman for DummyFisherman { - fn check_proof(&self, _: BeefyVersionedFinalityProof) -> Result<(), sp_blockchain::Error> { + fn check_proof(&self, _: BeefyVersionedFinalityProof) -> Result<(), Error> { Ok(()) } fn check_vote( &self, _: VoteMessage, AuthorityId, Signature>, - ) -> Result<(), sp_blockchain::Error> { + ) -> Result<(), Error> { Ok(()) } } diff --git a/primitives/consensus/beefy/src/commitment.rs b/primitives/consensus/beefy/src/commitment.rs index aa2efdb10ca22..3c451b70afb06 100644 --- a/primitives/consensus/beefy/src/commitment.rs +++ b/primitives/consensus/beefy/src/commitment.rs @@ -81,7 +81,7 @@ where } } -/// A commitment with matching GRANDPA validators' signatures. +/// A commitment with matching BEEFY validators' signatures. /// /// Note that SCALE-encoding of the structure is optimized for size efficiency over the wire, /// please take a look at custom [`Encode`] and [`Decode`] implementations and @@ -90,7 +90,7 @@ where pub struct SignedCommitment { /// The commitment signatures are collected for. pub commitment: Commitment, - /// GRANDPA validators' signatures for the commitment. + /// BEEFY validators' signatures for the commitment. /// /// The length of this `Vec` must match number of validators in the current set (see /// [Commitment::validator_set_id]). From 7f988efd6969d2606aaa6aec0022ba4de86ee5e7 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 20 Jul 2023 09:49:31 +0200 Subject: [PATCH 05/41] don't check GRANDPA finality 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. --- .../beefy/src/communication/fisherman.rs | 9 +++---- primitives/consensus/beefy/src/lib.rs | 24 +++++++++---------- 2 files changed, 14 insertions(+), 19 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index dd778fa711371..1f63fe58a294f 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -148,7 +148,7 @@ where R: ProvideRuntimeApi + Send + Sync, R::Api: BeefyApi, { - /// Check `vote` for contained finalized block against expected payload. + /// Check `vote` for contained block against expected payload. /// /// Note: this fn expects `vote.commitment.block_number` to be finalized. fn check_vote( @@ -159,9 +159,7 @@ where let (header, expected_payload) = self.expected_header_and_payload(number)?; if vote.commitment.payload != expected_payload { let validator_set = self.active_validator_set_at(&header)?; - // TODO: create/get grandpa proof for block number - let grandpa_proof = (); - let proof = InvalidForkVoteProof { vote, grandpa_proof }; + let proof = InvalidForkVoteProof { vote }; self.report_invalid_fork_vote(proof, &header, &expected_payload, &validator_set)?; } Ok(()) @@ -178,7 +176,6 @@ where let (header, expected_payload) = self.expected_header_and_payload(number)?; if commitment.payload != expected_payload { // TODO: create/get grandpa proof for block number - let grandpa_proof = (); let validator_set = self.active_validator_set_at(&header)?; if signatures.len() != validator_set.validators().len() { // invalid proof @@ -190,7 +187,7 @@ where if let Some(signature) = signature { let vote = VoteMessage { commitment: commitment.clone(), id: id.clone(), signature }; - let proof = InvalidForkVoteProof { vote, grandpa_proof }; + let proof = InvalidForkVoteProof { vote }; self.report_invalid_fork_vote( proof, &header, diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index fdfacae037d0a..7913adc03de82 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -252,13 +252,11 @@ impl EquivocationProof { } /// Proof of voter misbehavior on a given set id. -/// This proof shows voter voted on a different fork than finalized by GRANDPA. +/// This proof shows voter voted on a different fork than what is included in our chain. #[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)] pub struct InvalidForkVoteProof { /// Voting for a block on different fork than one finalized by GRANDPA. pub vote: VoteMessage, - /// TODO: GRANDPA proof that this block is final - pub grandpa_proof: (), } impl InvalidForkVoteProof { @@ -329,8 +327,15 @@ where /// Validates [InvalidForkVoteProof] by checking: /// 1. `vote` is signed, -/// 2. GRANDPA proof is provided for block number >= vote.commitment.block_number. -/// 3. `vote.commitment.payload` != `expected_payload`. +/// 2. `vote.commitment.payload` != `expected_payload`. +/// NOTE: 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. pub fn check_invalid_fork_proof( proof: &InvalidForkVoteProof::Signature>, expected_payload: &Payload, @@ -340,20 +345,13 @@ where Number: Clone + Encode + PartialEq, MsgHash: Hash, { - let InvalidForkVoteProof { vote, grandpa_proof: _ } = proof; + let InvalidForkVoteProof { vote } = proof; // check signature `vote`, if invalid, equivocation report is invalid if !check_commitment_signature(&vote.commitment, &vote.id, &vote.signature) { return false } - // TODO: add GRANDPA proof - // if GRANDPA proof doesn't show `vote.commitment.block_number` has been finalized, - // we cannot safely prove that `vote` payload is invalid. - // if grandpa_proof.block_number < vote.commitment.block_number { - // return false - // } - // check that `payload` on the `vote` is different that the `expected_payload`. &vote.commitment.payload != expected_payload } From 2ab4f5b78d424d4b51c5395e3e1e64b20bc2f956 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 20 Jul 2023 23:00:09 +0200 Subject: [PATCH 06/41] change primitive: vote -> commitment 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. --- .../beefy/src/communication/fisherman.rs | 71 +++++++++---------- frame/beefy/src/lib.rs | 14 ++-- primitives/consensus/beefy/src/lib.rs | 62 +++++++++------- 3 files changed, 78 insertions(+), 69 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 1f63fe58a294f..9820f1fcfbaba 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -27,7 +27,7 @@ use sp_blockchain::HeaderBackend; use sp_consensus_beefy::{ check_invalid_fork_proof, crypto::{AuthorityId, Signature}, - BeefyApi, InvalidForkVoteProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, + BeefyApi, InvalidForkCommitmentProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, Commitment, OpaqueKeyOwnershipProof, }; use sp_runtime::{ generic::BlockId, @@ -96,44 +96,46 @@ where .ok_or_else(|| Error::Backend("could not get BEEFY validator set".into())) } - fn report_invalid_fork_vote( + fn report_invalid_fork_commitments( &self, - proof: InvalidForkVoteProof, AuthorityId, Signature>, + proof: InvalidForkCommitmentProof, AuthorityId, Signature>, correct_header: &B::Header, - correct_payload: &Payload, validator_set: &ValidatorSet, // validator set active at the time ) -> Result<(), Error> { let set_id = validator_set.id(); - if proof.vote.commitment.validator_set_id != set_id || - !check_invalid_fork_proof::<_, _, BeefySignatureHasher>(&proof, correct_payload) + if proof.commitment.validator_set_id != set_id || + !check_invalid_fork_proof::, AuthorityId, BeefySignatureHasher>(&proof) { debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); return Ok(()) } let hash = correct_header.hash(); - let offender_id = proof.vote.id.clone(); + let offender_ids = proof.signatories.iter().cloned().map(|(id, _sig)| id).collect::>(); let runtime_api = self.runtime.runtime_api(); + // generate key ownership proof at that block - let key_owner_proof = match runtime_api - .generate_key_ownership_proof(hash, set_id, offender_id) - .map_err(Error::RuntimeApi)? - { - Some(proof) => proof, - None => { - debug!( - target: LOG_TARGET, - "🥩 Invalid fork vote offender not part of the authority set." - ); - return Ok(()) - }, - }; + let key_owner_proofs = offender_ids.iter() + .filter_map(|id| { + match runtime_api.generate_key_ownership_proof(hash, set_id, id.clone()) { + Ok(Some(proof)) => Some(Ok(proof)), + Ok(None) => { + debug!( + target: LOG_TARGET, + "🥩 Invalid fork vote offender not part of the authority set." + ); + None + }, + Err(e) => Some(Err(Error::RuntimeApi(e))), + } + }) + .collect::>()?; // submit invalid fork vote report at **best** block let best_block_hash = self.backend.blockchain().info().best_hash; runtime_api - .submit_report_invalid_fork_unsigned_extrinsic(best_block_hash, proof, key_owner_proof) + .submit_report_invalid_fork_unsigned_extrinsic(best_block_hash, proof, key_owner_proofs) .map_err(Error::RuntimeApi)?; Ok(()) @@ -159,8 +161,8 @@ where let (header, expected_payload) = self.expected_header_and_payload(number)?; if vote.commitment.payload != expected_payload { let validator_set = self.active_validator_set_at(&header)?; - let proof = InvalidForkVoteProof { vote }; - self.report_invalid_fork_vote(proof, &header, &expected_payload, &validator_set)?; + let proof = InvalidForkCommitmentProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], expected_payload }; + self.report_invalid_fork_commitments(proof, &header, &validator_set)?; } Ok(()) } @@ -182,20 +184,15 @@ where return Ok(()) } // report every signer of the bad justification - for signed_pair in validator_set.validators().iter().zip(signatures.into_iter()) { - let (id, signature) = signed_pair; - if let Some(signature) = signature { - let vote = - VoteMessage { commitment: commitment.clone(), id: id.clone(), signature }; - let proof = InvalidForkVoteProof { vote }; - self.report_invalid_fork_vote( - proof, - &header, - &expected_payload, - &validator_set, - )?; - } - } + let signatories = validator_set.validators().iter().cloned().zip(signatures.into_iter()) + .filter_map(|(id, signature)| signature.map(|sig| (id, sig))).collect(); + + let proof = InvalidForkCommitmentProof { commitment, signatories, expected_payload }; + self.report_invalid_fork_commitments( + proof, + &header, + &validator_set, + )?; } Ok(()) } diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 3c61bce844fff..304374bd8942c 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -63,7 +63,7 @@ const LOG_TARGET: &str = "runtime::beefy"; pub mod pallet { use super::*; use frame_system::pallet_prelude::BlockNumberFor; - use sp_consensus_beefy::InvalidForkVoteProof; + use sp_consensus_beefy::InvalidForkCommitmentProof; #[pallet::config] pub trait Config: frame_system::Config { @@ -275,10 +275,10 @@ pub mod pallet { /// will be reported. #[pallet::call_index(2)] #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count()))] - pub fn report_invalid_fork_vote( + pub fn report_invalid_fork_commitment( origin: OriginFor, invalid_fork_proof: Box< - InvalidForkVoteProof< + InvalidForkCommitmentProof< BlockNumberFor, T::BeefyId, ::Signature, @@ -297,9 +297,9 @@ pub mod pallet { Ok(Pays::No.into()) } - /// Report voter voting on invalid fork. This method will verify the + /// Report commitment on invalid fork. This method will verify the /// invalid fork proof and validate the given key ownership proof - /// against the extracted offender. If both are valid, the offence + /// against the extracted offenders. If both are valid, the offence /// will be reported. /// /// This extrinsic must be called unsigned and it is expected that only @@ -308,10 +308,10 @@ pub mod pallet { /// reporter. #[pallet::call_index(3)] #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count()))] - pub fn report_invalid_fork_vote_unsigned( + pub fn report_invalid_fork_commitment_unsigned( origin: OriginFor, invalid_fork_proof: Box< - InvalidForkVoteProof< + InvalidForkCommitmentProof< BlockNumberFor, T::BeefyId, ::Signature, diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index 7913adc03de82..32d43c6fb95c3 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -251,26 +251,35 @@ impl EquivocationProof { } } -/// Proof of voter misbehavior on a given set id. -/// This proof shows voter voted on a different fork than what is included in our chain. +/// Proof of invalid commitment on a given set id. +/// This proof shows commitment signed on a different fork than finalized by GRANDPA. #[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)] -pub struct InvalidForkVoteProof { - /// Voting for a block on different fork than one finalized by GRANDPA. - pub vote: VoteMessage, +pub struct InvalidForkCommitmentProof { + /// Commitment for a block on different fork than one at the same height in + /// this client's chain. + /// TODO: maybe replace {commitment, signatories} with SignedCommitment + /// (tradeoff: SignedCommitment not ideal since sigs optional, but fewer + /// types to juggle around) - check once usage pattern is clear + pub commitment: Commitment, + /// Signatures on this block + /// TODO: maybe change to HashMap - check once usage pattern is clear + pub signatories: Vec<(Id, Signature)>, + /// Expected payload + pub expected_payload: Payload, } -impl InvalidForkVoteProof { +impl InvalidForkCommitmentProof { /// Returns the authority id of the misbehaving voter. - pub fn offender_id(&self) -> &Id { - &self.vote.id + pub fn offender_ids(&self) -> Vec<&Id> { + self.signatories.iter().map(|(id, _)| id).collect() } /// Returns the round number at which the infringement occurred. pub fn round_number(&self) -> &Number { - &self.vote.commitment.block_number + &self.commitment.block_number } /// Returns the set id at which the infringement occurred. pub fn set_id(&self) -> ValidatorSetId { - self.vote.commitment.validator_set_id + self.commitment.validator_set_id } } @@ -337,23 +346,26 @@ where /// an incorrect block implies validators will only sign blocks they *know* will be /// finalized by GRANDPA. pub fn check_invalid_fork_proof( - proof: &InvalidForkVoteProof::Signature>, - expected_payload: &Payload, + proof: &InvalidForkCommitmentProof::Signature>, ) -> bool where Id: BeefyAuthorityId + PartialEq, Number: Clone + Encode + PartialEq, MsgHash: Hash, { - let InvalidForkVoteProof { vote } = proof; - - // check signature `vote`, if invalid, equivocation report is invalid - if !check_commitment_signature(&vote.commitment, &vote.id, &vote.signature) { - return false + let InvalidForkCommitmentProof { commitment, signatories, expected_payload } = proof; + + // check that `payload` on the `vote` is different that the `expected_payload` (checked first since cheap failfast). + if &commitment.payload != expected_payload { + // 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 } - - // check that `payload` on the `vote` is different that the `expected_payload`. - &vote.commitment.payload != expected_payload } /// New BEEFY validator set notification hook. @@ -418,9 +430,9 @@ sp_api::decl_runtime_apis! { key_owner_proof: OpaqueKeyOwnershipProof, ) -> Option<()>; - /// Submits an unsigned extrinsic to report an vote for an invalid fork. - /// The caller must provide the invalid vote proof and a key ownership proof - /// (should be obtained using `generate_key_ownership_proof`). The + /// Submits an unsigned extrinsic to report commitments to an invalid fork. + /// The caller must provide the invalid commitments proof and key ownership proofs + /// (should be obtained using `generate_key_ownership_proof`) for the offenders. The /// extrinsic will be unsigned and should only be accepted for local /// authorship (not to be broadcast to the network). This method returns /// `None` when creation of the extrinsic fails, e.g. if equivocation @@ -428,8 +440,8 @@ sp_api::decl_runtime_apis! { /// hardcoded to return `None`). Only useful in an offchain context. fn submit_report_invalid_fork_unsigned_extrinsic( invalid_fork_proof: - InvalidForkVoteProof, crypto::AuthorityId, crypto::Signature>, - key_owner_proof: OpaqueKeyOwnershipProof, + InvalidForkCommitmentProof, crypto::AuthorityId, crypto::Signature>, + key_owner_proofs: Vec, ) -> Option<()>; /// Generates a proof of key ownership for the given authority in the From 06260f0feb9cbdda8b2fd3899b1157a77837432e Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 27 Jul 2023 11:05:18 +0200 Subject: [PATCH 07/41] add check_signed_commitment/report_invalid_payload --- .../beefy/src/communication/fisherman.rs | 78 ++++++++++++++++++- client/consensus/beefy/src/tests.rs | 3 + 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 9820f1fcfbaba..bb9554bc8ca08 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -27,7 +27,7 @@ use sp_blockchain::HeaderBackend; use sp_consensus_beefy::{ check_invalid_fork_proof, crypto::{AuthorityId, Signature}, - BeefyApi, InvalidForkCommitmentProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, Commitment, OpaqueKeyOwnershipProof, + BeefyApi, InvalidForkCommitmentProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, Commitment, OpaqueKeyOwnershipProof, SignedCommitment, }; use sp_runtime::{ generic::BlockId, @@ -44,6 +44,11 @@ pub(crate) trait BeefyFisherman: Send + Sync { vote: VoteMessage, AuthorityId, Signature>, ) -> Result<(), Error>; + fn check_signed_commitment( + &self, + signed_commitment: SignedCommitment, Signature>, + ) -> Result<(), Error>; + /// Check `proof` for contained finalized block against expected payload. /// /// Note: this fn expects block referenced in `proof` to be finalized. @@ -96,12 +101,64 @@ where .ok_or_else(|| Error::Backend("could not get BEEFY validator set".into())) } + fn report_invalid_payload( + &self, + signed_commitment: SignedCommitment, Signature>, + correct_payload: &Payload, + correct_header: &B::Header, + ) -> Result<(), Error> { + let validator_set = self.active_validator_set_at(correct_header)?; + let set_id = validator_set.id(); + + let proof = InvalidForkCommitmentProof { + commitment: signed_commitment.commitment.clone(), + signatories: vec![], + expected_payload: correct_payload.clone(), + }; + + if signed_commitment.commitment.validator_set_id != set_id || + signed_commitment.commitment.payload != *correct_payload || + !check_invalid_fork_proof::, AuthorityId, BeefySignatureHasher>(&proof) + { + debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); + return Ok(()) + } + + let offender_ids = proof.signatories.iter().cloned().map(|(id, _sig)| id).collect::>(); + let runtime_api = self.runtime.runtime_api(); + + // generate key ownership proof at that block + let key_owner_proofs = offender_ids.iter() + .filter_map(|id| { + match runtime_api.generate_key_ownership_proof(correct_header.hash(), set_id, id.clone()) { + Ok(Some(proof)) => Some(Ok(proof)), + Ok(None) => { + debug!( + target: LOG_TARGET, + "🥩 Invalid fork vote offender not part of the authority set." + ); + None + }, + Err(e) => Some(Err(Error::RuntimeApi(e))), + } + }) + .collect::>()?; + + // submit invalid fork vote report at **best** block + let best_block_hash = self.backend.blockchain().info().best_hash; + runtime_api + .submit_report_invalid_fork_unsigned_extrinsic(best_block_hash, proof, key_owner_proofs) + .map_err(Error::RuntimeApi)?; + + Ok(()) + } + fn report_invalid_fork_commitments( &self, proof: InvalidForkCommitmentProof, AuthorityId, Signature>, correct_header: &B::Header, - validator_set: &ValidatorSet, // validator set active at the time ) -> Result<(), Error> { + let validator_set = self.active_validator_set_at(correct_header)?; let set_id = validator_set.id(); if proof.commitment.validator_set_id != set_id || @@ -162,7 +219,21 @@ where if vote.commitment.payload != expected_payload { let validator_set = self.active_validator_set_at(&header)?; let proof = InvalidForkCommitmentProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], expected_payload }; - self.report_invalid_fork_commitments(proof, &header, &validator_set)?; + self.report_invalid_fork_commitments(proof, &header)?; + } + Ok(()) + } + + /// Check `commitment` for contained block against expected payload. + fn check_signed_commitment( + &self, + signed_commitment: SignedCommitment, Signature>, + ) -> Result<(), Error> { + let number = signed_commitment.commitment.block_number; + let (header, expected_payload) = self.expected_header_and_payload(number)?; + if signed_commitment.commitment.payload != expected_payload { + let validator_set = self.active_validator_set_at(&header)?; + self.report_invalid_payload(signed_commitment, &expected_payload, &header)?; } Ok(()) } @@ -191,7 +262,6 @@ where self.report_invalid_fork_commitments( proof, &header, - &validator_set, )?; } Ok(()) diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 2d7af835203a2..7be135c19e878 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -254,6 +254,9 @@ impl BeefyFisherman for DummyFisherman { fn check_proof(&self, _: BeefyVersionedFinalityProof) -> Result<(), Error> { Ok(()) } + fn check_signed_commitment(&self, _: SignedCommitment, Signature>) -> Result<(), Error> { + Ok(()) + } fn check_vote( &self, _: VoteMessage, AuthorityId, Signature>, From 0642a815b41b8827f324a736d36ee347760430d0 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 2 Aug 2023 10:36:54 +0200 Subject: [PATCH 08/41] update comments --- client/consensus/beefy/src/communication/fisherman.rs | 2 +- frame/beefy/src/lib.rs | 2 +- primitives/consensus/beefy/src/lib.rs | 6 ++++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index bb9554bc8ca08..fd62d4851453e 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -238,7 +238,7 @@ where Ok(()) } - /// Check `proof` for contained finalized block against expected payload. + /// Check `proof` for contained block against expected payload. /// /// Note: this fn expects block referenced in `proof` to be finalized. fn check_proof(&self, proof: BeefyVersionedFinalityProof) -> Result<(), Error> { diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 304374bd8942c..db72231b58e82 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -113,7 +113,7 @@ pub mod pallet { type EquivocationReportSystem: OffenceReportSystem< Option, // TODO: make below an enum that takes either `EquivocationProof` or - // `InvalidForkVoteProof` + // `InvalidForkCommitmentProof` EquivocationEvidenceFor, >; } diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index 32d43c6fb95c3..1ae5517d86a8e 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -355,7 +355,8 @@ where { let InvalidForkCommitmentProof { commitment, signatories, expected_payload } = proof; - // check that `payload` on the `vote` is different that the `expected_payload` (checked first since cheap failfast). + // check that `payload` on the `vote` is different that the `expected_payload` (checked first + // since cheap failfast). if &commitment.payload != expected_payload { // check check each signatory's signature on the commitment. // if any are invalid, equivocation report is invalid @@ -405,7 +406,8 @@ impl OpaqueKeyOwnershipProof { } sp_api::decl_runtime_apis! { - /// API necessary for BEEFY voters. + /// API necessary for BEEFY voters. Due to the significant conceptual + /// overlap, in large part, this is lifted from the GRANDPA API. #[api_version(3)] pub trait BeefyApi where AuthorityId : Codec + RuntimeAppPublic, From c6c36e3e111ab9d6a6880bf245dd423838cfbe36 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 2 Aug 2023 10:52:56 +0200 Subject: [PATCH 09/41] add dummy for report of invalid fork commitments --- frame/beefy/src/lib.rs | 14 ++++++++++++++ primitives/consensus/beefy/src/lib.rs | 2 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index db72231b58e82..5b05c7a7fd99a 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -366,6 +366,20 @@ impl Pallet { T::EquivocationReportSystem::publish_evidence((equivocation_proof, key_owner_proof)).ok() } + /// Submits an extrinsic to report an invalid fork signed by potentially + /// multiple signatories. This method will create an unsigned extrinsic with + /// a call to `report_invalid_fork_unsigned` and will push the transaction + /// to the pool. Only useful in an offchain context. + pub fn submit_unsigned_invalid_fork_report( + _invalid_fork_proof: sp_consensus_beefy::InvalidForkCommitmentProof, T::BeefyId, + ::Signature, +>, + _key_owner_proofs: Vec, + ) -> Option<()> { + // T::EquivocationReportSystem::publish_evidence((invalid_fork_proof, key_owner_proofs)).ok() + None + } + fn change_authorities( new: BoundedVec, queued: BoundedVec, diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index 1ae5517d86a8e..ac68a7c31f9a6 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -389,7 +389,7 @@ impl OnNewValidatorSet for () { /// the runtime API boundary this type is unknown and as such we keep this /// opaque representation, implementors of the runtime API will have to make /// sure that all usages of `OpaqueKeyOwnershipProof` refer to the same type. -#[derive(Decode, Encode, PartialEq, TypeInfo)] +#[derive(Decode, Encode, PartialEq, TypeInfo, Clone)] pub struct OpaqueKeyOwnershipProof(Vec); impl OpaqueKeyOwnershipProof { /// Create a new `OpaqueKeyOwnershipProof` using the given encoded From 6078b4130deae9c15e1612135de4bae39a8bf53a Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 1 Aug 2023 20:16:11 +0200 Subject: [PATCH 10/41] account for #14471 --- frame/beefy/src/lib.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 5b05c7a7fd99a..69a301e13f69b 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -274,7 +274,10 @@ pub mod pallet { /// against the extracted offender. If both are valid, the offence /// will be reported. #[pallet::call_index(2)] - #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count()))] + #[pallet::weight(T::WeightInfo::report_equivocation( + key_owner_proof.validator_count(), + T::MaxNominators::get(), + ))] pub fn report_invalid_fork_commitment( origin: OriginFor, invalid_fork_proof: Box< @@ -307,7 +310,7 @@ pub mod pallet { /// if the block author is defined it will be defined as the equivocation /// reporter. #[pallet::call_index(3)] - #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count()))] + #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count(), T::MaxNominators::get(),))] pub fn report_invalid_fork_commitment_unsigned( origin: OriginFor, invalid_fork_proof: Box< From 5da56313e2f04d88304e30ba96ab00518cbc0729 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 17:44:39 +0200 Subject: [PATCH 11/41] account for #14373 --- client/consensus/beefy/src/communication/fisherman.rs | 6 +++--- primitives/consensus/beefy/src/lib.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index fd62d4851453e..2fda6ef171858 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -26,7 +26,7 @@ use sp_api::ProvideRuntimeApi; use sp_blockchain::HeaderBackend; use sp_consensus_beefy::{ check_invalid_fork_proof, - crypto::{AuthorityId, Signature}, + ecdsa_crypto::{AuthorityId, Signature}, BeefyApi, InvalidForkCommitmentProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, Commitment, OpaqueKeyOwnershipProof, SignedCommitment, }; use sp_runtime::{ @@ -70,7 +70,7 @@ where BE: Backend, P: PayloadProvider, R: ProvideRuntimeApi + Send + Sync, - R::Api: BeefyApi, + R::Api: BeefyApi, { fn expected_header_and_payload(&self, number: NumberFor) -> Result<(B::Header, Payload), Error> { // This should be un-ambiguous since `number` is finalized. @@ -205,7 +205,7 @@ where BE: Backend, P: PayloadProvider, R: ProvideRuntimeApi + Send + Sync, - R::Api: BeefyApi, + R::Api: BeefyApi, { /// Check `vote` for contained block against expected payload. /// diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index ac68a7c31f9a6..06a921e021fae 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -442,7 +442,7 @@ sp_api::decl_runtime_apis! { /// hardcoded to return `None`). Only useful in an offchain context. fn submit_report_invalid_fork_unsigned_extrinsic( invalid_fork_proof: - InvalidForkCommitmentProof, crypto::AuthorityId, crypto::Signature>, + InvalidForkCommitmentProof, AuthorityId, ::Signature>, key_owner_proofs: Vec, ) -> Option<()>; From 98db7cf58704fd57887b6d68bad38644bede3898 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 17:45:52 +0200 Subject: [PATCH 12/41] EquivocationOffence.{offender->offenders} 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. --- frame/beefy/src/equivocation.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 92dd8cb81e64e..2be8506ceed3f 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -79,8 +79,8 @@ where pub session_index: SessionIndex, /// The size of the validator set at the time of the offence. pub validator_set_count: u32, - /// The authority which produced this equivocation. - pub offender: Offender, + /// The authorities which produced this equivocation. + pub offenders: Vec, } impl Offence for EquivocationOffence @@ -91,7 +91,7 @@ where type TimeSlot = TimeSlot; fn offenders(&self) -> Vec { - vec![self.offender.clone()] + self.offenders.clone() } fn session_index(&self) -> SessionIndex { @@ -227,7 +227,7 @@ where time_slot: TimeSlot { set_id, round }, session_index, validator_set_count, - offender, + offenders: vec![offender], }; R::report_offence(reporter.into_iter().collect(), offence) From 1a45cbeb5c63375a028abcddde97a3f560b33e52 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 17:57:00 +0200 Subject: [PATCH 13/41] EquivocationProof->VoteEquivocationProof --- client/consensus/beefy/src/round.rs | 10 +++++----- client/consensus/beefy/src/tests.rs | 6 +++--- client/consensus/beefy/src/worker.rs | 4 ++-- frame/beefy/src/equivocation.rs | 6 +++--- frame/beefy/src/lib.rs | 10 +++++----- frame/beefy/src/mock.rs | 2 +- primitives/consensus/beefy/src/lib.rs | 8 ++++---- primitives/consensus/beefy/src/test_utils.rs | 6 +++--- 8 files changed, 26 insertions(+), 26 deletions(-) diff --git a/client/consensus/beefy/src/round.rs b/client/consensus/beefy/src/round.rs index 6f400ce47843c..81e0d0634dcdd 100644 --- a/client/consensus/beefy/src/round.rs +++ b/client/consensus/beefy/src/round.rs @@ -22,7 +22,7 @@ use codec::{Decode, Encode}; use log::debug; use sp_consensus_beefy::{ ecdsa_crypto::{AuthorityId, Signature}, - Commitment, EquivocationProof, SignedCommitment, ValidatorSet, ValidatorSetId, VoteMessage, + Commitment, VoteEquivocationProof, SignedCommitment, ValidatorSet, ValidatorSetId, VoteMessage, }; use sp_runtime::traits::{Block, NumberFor}; use std::collections::BTreeMap; @@ -61,7 +61,7 @@ pub fn threshold(authorities: usize) -> usize { pub enum VoteImportResult { Ok, RoundConcluded(SignedCommitment, Signature>), - Equivocation(EquivocationProof, AuthorityId, Signature>), + Equivocation(VoteEquivocationProof, AuthorityId, Signature>), Invalid, Stale, } @@ -153,7 +153,7 @@ where target: LOG_TARGET, "🥩 detected equivocated vote: 1st: {:?}, 2nd: {:?}", previous_vote, vote ); - return VoteImportResult::Equivocation(EquivocationProof { + return VoteImportResult::Equivocation(VoteEquivocationProof { first: previous_vote.clone(), second: vote, }) @@ -203,7 +203,7 @@ mod tests { use sc_network_test::Block; use sp_consensus_beefy::{ - known_payloads::MMR_ROOT_ID, Commitment, EquivocationProof, Keyring, Payload, + known_payloads::MMR_ROOT_ID, Commitment, VoteEquivocationProof, Keyring, Payload, SignedCommitment, ValidatorSet, VoteMessage, }; @@ -485,7 +485,7 @@ mod tests { let mut alice_vote2 = alice_vote1.clone(); alice_vote2.commitment = commitment2; - let expected_result = VoteImportResult::Equivocation(EquivocationProof { + let expected_result = VoteImportResult::Equivocation(VoteEquivocationProof { first: alice_vote1.clone(), second: alice_vote2.clone(), }); diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 7be135c19e878..ee7e3205a0fb6 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -56,7 +56,7 @@ use sp_consensus_beefy::{ ecdsa_crypto::{AuthorityId, Signature}, known_payloads, mmr::{find_mmr_root_digest, MmrRootProvider}, - BeefyApi, Commitment, ConsensusLog, EquivocationProof, Keyring as BeefyKeyring, MmrRootHash, + BeefyApi, Commitment, ConsensusLog, VoteEquivocationProof, Keyring as BeefyKeyring, MmrRootHash, OpaqueKeyOwnershipProof, Payload, SignedCommitment, ValidatorSet, ValidatorSetId, VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID, }; @@ -271,7 +271,7 @@ pub(crate) struct TestApi { pub validator_set: BeefyValidatorSet, pub mmr_root_hash: MmrRootHash, pub reported_equivocations: - Option, AuthorityId, Signature>>>>>, + Option, AuthorityId, Signature>>>>>, } impl TestApi { @@ -325,7 +325,7 @@ sp_api::mock_impl_runtime_apis! { } fn submit_report_equivocation_unsigned_extrinsic( - proof: EquivocationProof, AuthorityId, Signature>, + proof: VoteEquivocationProof, AuthorityId, Signature>, _dummy: OpaqueKeyOwnershipProof, ) -> Option<()> { if let Some(equivocations_buf) = self.inner.reported_equivocations.as_ref() { diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index 64e786c1fb6b7..d6febe336d9c0 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -43,7 +43,7 @@ use sp_consensus::SyncOracle; use sp_consensus_beefy::{ check_equivocation_proof, ecdsa_crypto::{AuthorityId, Signature}, - BeefyApi, Commitment, ConsensusLog, EquivocationProof, PayloadProvider, ValidatorSet, + BeefyApi, Commitment, ConsensusLog, VoteEquivocationProof, PayloadProvider, ValidatorSet, VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID, }; use sp_runtime::{ @@ -934,7 +934,7 @@ where /// isn't necessarily the best block if there are pending authority set changes. pub(crate) fn report_equivocation( &self, - proof: EquivocationProof, AuthorityId, Signature>, + proof: VoteEquivocationProof, AuthorityId, Signature>, ) -> Result<(), Error> { let rounds = self.persisted_state.voting_oracle.active_rounds()?; let (validators, validator_set_id) = (rounds.validators(), rounds.validator_set_id()); diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 2be8506ceed3f..7d3315c8991cc 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -41,7 +41,7 @@ use frame_support::{ }; use frame_system::pallet_prelude::BlockNumberFor; use log::{error, info}; -use sp_consensus_beefy::{EquivocationProof, ValidatorSetId, KEY_TYPE as BEEFY_KEY_TYPE}; +use sp_consensus_beefy::{VoteEquivocationProof, ValidatorSetId, KEY_TYPE as BEEFY_KEY_TYPE}; use sp_runtime::{ transaction_validity::{ InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity, @@ -125,9 +125,9 @@ where pub struct EquivocationReportSystem(sp_std::marker::PhantomData<(T, R, P, L)>); /// Equivocation evidence convenience alias. -// TODO: use an enum that takes either `EquivocationProof` or `InvalidForkVoteProof` +// TODO: use an enum that takes either `VoteEquivocationProof` or `ForkEquivocationProof` pub type EquivocationEvidenceFor = ( - EquivocationProof< + VoteEquivocationProof< BlockNumberFor, ::BeefyId, <::BeefyId as RuntimeAppPublic>::Signature, diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 69a301e13f69b..96ee5623eb5e1 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -41,7 +41,7 @@ use sp_staking::{offence::OffenceReportSystem, SessionIndex}; use sp_std::prelude::*; use sp_consensus_beefy::{ - AuthorityIndex, BeefyAuthorityId, ConsensusLog, EquivocationProof, OnNewValidatorSet, + AuthorityIndex, BeefyAuthorityId, ConsensusLog, VoteEquivocationProof, OnNewValidatorSet, ValidatorSet, BEEFY_ENGINE_ID, GENESIS_AUTHORITY_SET_ID, }; @@ -112,7 +112,7 @@ pub mod pallet { /// Defines methods to publish, check and process an equivocation offence. type EquivocationReportSystem: OffenceReportSystem< Option, - // TODO: make below an enum that takes either `EquivocationProof` or + // TODO: make below an enum that takes either `VoteEquivocationProof` or // `InvalidForkCommitmentProof` EquivocationEvidenceFor, >; @@ -217,7 +217,7 @@ pub mod pallet { pub fn report_equivocation( origin: OriginFor, equivocation_proof: Box< - EquivocationProof< + VoteEquivocationProof< BlockNumberFor, T::BeefyId, ::Signature, @@ -252,7 +252,7 @@ pub mod pallet { pub fn report_equivocation_unsigned( origin: OriginFor, equivocation_proof: Box< - EquivocationProof< + VoteEquivocationProof< BlockNumberFor, T::BeefyId, ::Signature, @@ -359,7 +359,7 @@ impl Pallet { /// an unsigned extrinsic with a call to `report_equivocation_unsigned` and /// will push the transaction to the pool. Only useful in an offchain context. pub fn submit_unsigned_equivocation_report( - equivocation_proof: EquivocationProof< + equivocation_proof: VoteEquivocationProof< BlockNumberFor, T::BeefyId, ::Signature, diff --git a/frame/beefy/src/mock.rs b/frame/beefy/src/mock.rs index af770c0b49448..5d7171ff9ad3a 100644 --- a/frame/beefy/src/mock.rs +++ b/frame/beefy/src/mock.rs @@ -40,7 +40,7 @@ use crate as pallet_beefy; pub use sp_consensus_beefy::{ ecdsa_crypto::{AuthorityId as BeefyId, AuthoritySignature as BeefySignature}, - ConsensusLog, EquivocationProof, BEEFY_ENGINE_ID, + ConsensusLog, VoteEquivocationProof, BEEFY_ENGINE_ID, }; impl_opaque_keys! { diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index 06a921e021fae..3e588f83a199b 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -229,14 +229,14 @@ pub struct VoteMessage { /// BEEFY happens when a voter votes on the same round/block for different payloads. /// Proving is achieved by collecting the signed commitments of conflicting votes. #[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)] -pub struct EquivocationProof { +pub struct VoteEquivocationProof { /// The first vote in the equivocation. pub first: VoteMessage, /// The second vote in the equivocation. pub second: VoteMessage, } -impl EquivocationProof { +impl VoteEquivocationProof { /// Returns the authority id of the equivocator. pub fn offender_id(&self) -> &Id { &self.first.id @@ -302,7 +302,7 @@ where /// Verifies the equivocation proof by making sure that both votes target /// different blocks and that its signatures are valid. pub fn check_equivocation_proof( - report: &EquivocationProof::Signature>, + report: &VoteEquivocationProof::Signature>, ) -> bool where Id: BeefyAuthorityId + PartialEq, @@ -428,7 +428,7 @@ sp_api::decl_runtime_apis! { /// hardcoded to return `None`). Only useful in an offchain context. fn submit_report_equivocation_unsigned_extrinsic( equivocation_proof: - EquivocationProof, AuthorityId, ::Signature>, + VoteEquivocationProof, AuthorityId, ::Signature>, key_owner_proof: OpaqueKeyOwnershipProof, ) -> Option<()>; diff --git a/primitives/consensus/beefy/src/test_utils.rs b/primitives/consensus/beefy/src/test_utils.rs index b83f657af38e3..12190a701ab2e 100644 --- a/primitives/consensus/beefy/src/test_utils.rs +++ b/primitives/consensus/beefy/src/test_utils.rs @@ -17,7 +17,7 @@ #![cfg(feature = "std")] -use crate::{ecdsa_crypto, Commitment, EquivocationProof, Payload, ValidatorSetId, VoteMessage}; +use crate::{ecdsa_crypto, Commitment, VoteEquivocationProof, Payload, ValidatorSetId, VoteMessage}; use codec::Encode; use sp_core::{ecdsa, keccak_256, Pair}; use std::collections::HashMap; @@ -95,7 +95,7 @@ impl From for ecdsa_crypto::Public { pub fn generate_equivocation_proof( vote1: (u64, Payload, ValidatorSetId, &Keyring), vote2: (u64, Payload, ValidatorSetId, &Keyring), -) -> EquivocationProof { +) -> VoteEquivocationProof { let signed_vote = |block_number: u64, payload: Payload, validator_set_id: ValidatorSetId, @@ -106,5 +106,5 @@ pub fn generate_equivocation_proof( }; let first = signed_vote(vote1.0, vote1.1, vote1.2, vote1.3); let second = signed_vote(vote2.0, vote2.1, vote2.2, vote2.3); - EquivocationProof { first, second } + VoteEquivocationProof { first, second } } From 4bd78eb0ca7f06c9cb607d07564d21ead85e8f2b Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 18:07:10 +0200 Subject: [PATCH 14/41] Invalid{""->Vote}EquivocationProof --- frame/beefy/src/equivocation.rs | 6 +++--- frame/beefy/src/lib.rs | 4 ++-- frame/beefy/src/tests.rs | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 7d3315c8991cc..3db2a553a84be 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -212,15 +212,15 @@ where // Validate equivocation proof (check votes are different and signatures are valid). if !sp_consensus_beefy::check_equivocation_proof(&equivocation_proof) { - return Err(Error::::InvalidEquivocationProof.into()) + return Err(Error::::InvalidVoteEquivocationProof.into()) } // Check that the session id for the membership proof is within the // bounds of the set id reported in the equivocation. let set_id_session_index = - crate::SetIdSession::::get(set_id).ok_or(Error::::InvalidEquivocationProof)?; + crate::SetIdSession::::get(set_id).ok_or(Error::::InvalidVoteEquivocationProof)?; if session_index != set_id_session_index { - return Err(Error::::InvalidEquivocationProof.into()) + return Err(Error::::InvalidVoteEquivocationProof.into()) } let offence = EquivocationOffence { diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 96ee5623eb5e1..3850101cb013c 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -197,8 +197,8 @@ pub mod pallet { pub enum Error { /// A key ownership proof provided as part of an equivocation report is invalid. InvalidKeyOwnershipProof, - /// An equivocation proof provided as part of an equivocation report is invalid. - InvalidEquivocationProof, + /// An equivocation proof provided as part of a voter equivocation report is invalid. + InvalidVoteEquivocationProof, /// A given equivocation report is valid but already previously reported. DuplicateOffenceReport, } diff --git a/frame/beefy/src/tests.rs b/frame/beefy/src/tests.rs index e04dc330d0c07..d59bc053a8496 100644 --- a/frame/beefy/src/tests.rs +++ b/frame/beefy/src/tests.rs @@ -451,7 +451,7 @@ fn report_equivocation_invalid_set_id() { Box::new(equivocation_proof), key_owner_proof, ), - Error::::InvalidEquivocationProof, + Error::::InvalidVoteEquivocationProof, ); }); } @@ -494,7 +494,7 @@ fn report_equivocation_invalid_session() { Box::new(equivocation_proof), key_owner_proof, ), - Error::::InvalidEquivocationProof, + Error::::InvalidVoteEquivocationProof, ); }); } @@ -573,7 +573,7 @@ fn report_equivocation_invalid_equivocation_proof() { Box::new(equivocation_proof), key_owner_proof.clone(), ), - Error::::InvalidEquivocationProof, + Error::::InvalidVoteEquivocationProof, ); }; From f4496c9aeb36a1d04f380c852fd370ac10940658 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 18:10:36 +0200 Subject: [PATCH 15/41] check_{""->vote}_equivocation_proof --- client/consensus/beefy/src/worker.rs | 4 ++-- frame/beefy/src/equivocation.rs | 2 +- frame/beefy/src/tests.rs | 12 ++++++------ primitives/consensus/beefy/src/lib.rs | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index d6febe336d9c0..d594101f74575 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -41,7 +41,7 @@ use sp_api::{BlockId, ProvideRuntimeApi}; use sp_arithmetic::traits::{AtLeast32Bit, Saturating}; use sp_consensus::SyncOracle; use sp_consensus_beefy::{ - check_equivocation_proof, + check_vote_equivocation_proof, ecdsa_crypto::{AuthorityId, Signature}, BeefyApi, Commitment, ConsensusLog, VoteEquivocationProof, PayloadProvider, ValidatorSet, VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID, @@ -940,7 +940,7 @@ where let (validators, validator_set_id) = (rounds.validators(), rounds.validator_set_id()); let offender_id = proof.offender_id().clone(); - if !check_equivocation_proof::<_, _, BeefySignatureHasher>(&proof) { + if !check_vote_equivocation_proof::<_, _, BeefySignatureHasher>(&proof) { debug!(target: LOG_TARGET, "🥩 Skip report for bad equivocation {:?}", proof); return Ok(()) } else if let Some(local_id) = self.key_store.authority_id(validators) { diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 3db2a553a84be..756b7d7198b54 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -211,7 +211,7 @@ where .ok_or(Error::::InvalidKeyOwnershipProof)?; // Validate equivocation proof (check votes are different and signatures are valid). - if !sp_consensus_beefy::check_equivocation_proof(&equivocation_proof) { + if !sp_consensus_beefy::check_vote_equivocation_proof(&equivocation_proof) { return Err(Error::::InvalidVoteEquivocationProof.into()) } diff --git a/frame/beefy/src/tests.rs b/frame/beefy/src/tests.rs index d59bc053a8496..a493286decf59 100644 --- a/frame/beefy/src/tests.rs +++ b/frame/beefy/src/tests.rs @@ -19,7 +19,7 @@ use std::vec; use codec::Encode; use sp_consensus_beefy::{ - check_equivocation_proof, generate_equivocation_proof, known_payloads::MMR_ROOT_ID, + check_vote_equivocation_proof, generate_equivocation_proof, known_payloads::MMR_ROOT_ID, Keyring as BeefyKeyring, Payload, ValidatorSet, KEY_TYPE as BEEFY_KEY_TYPE, }; @@ -217,7 +217,7 @@ fn should_sign_and_verify() { (1, payload1.clone(), set_id, &BeefyKeyring::Bob), ); // expect invalid equivocation proof - assert!(!check_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); + assert!(!check_vote_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); // generate an equivocation proof, with two votes in different rounds for // different payloads signed by the same key @@ -226,7 +226,7 @@ fn should_sign_and_verify() { (2, payload2.clone(), set_id, &BeefyKeyring::Bob), ); // expect invalid equivocation proof - assert!(!check_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); + assert!(!check_vote_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); // generate an equivocation proof, with two votes by different authorities let equivocation_proof = generate_equivocation_proof( @@ -234,7 +234,7 @@ fn should_sign_and_verify() { (1, payload2.clone(), set_id, &BeefyKeyring::Bob), ); // expect invalid equivocation proof - assert!(!check_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); + assert!(!check_vote_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); // generate an equivocation proof, with two votes in different set ids let equivocation_proof = generate_equivocation_proof( @@ -242,7 +242,7 @@ fn should_sign_and_verify() { (1, payload2.clone(), set_id + 1, &BeefyKeyring::Bob), ); // expect invalid equivocation proof - assert!(!check_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); + assert!(!check_vote_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); // generate an equivocation proof, with two votes in the same round for // different payloads signed by the same key @@ -252,7 +252,7 @@ fn should_sign_and_verify() { (1, payload2, set_id, &BeefyKeyring::Bob), ); // expect valid equivocation proof - assert!(check_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); + assert!(check_vote_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); } #[test] diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index 3e588f83a199b..2aaef24dae5fd 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -299,9 +299,9 @@ where BeefyAuthorityId::::verify(authority_id, signature, &encoded_commitment) } -/// Verifies the equivocation proof by making sure that both votes target +/// Verifies the vote equivocation proof by making sure that both votes target /// different blocks and that its signatures are valid. -pub fn check_equivocation_proof( +pub fn check_vote_equivocation_proof( report: &VoteEquivocationProof::Signature>, ) -> bool where From 61f9a458cc41e1dae5fb67635e3545ec1944a520 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 18:17:53 +0200 Subject: [PATCH 16/41] InvalidForkCommitmentProof->ForkEquivocationProof --- .../beefy/src/communication/fisherman.rs | 16 ++++++++-------- frame/beefy/src/lib.rs | 8 ++++---- primitives/consensus/beefy/src/lib.rs | 16 ++++++++-------- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 2fda6ef171858..c228e4bd2a7d0 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -25,9 +25,9 @@ use sc_client_api::Backend; use sp_api::ProvideRuntimeApi; use sp_blockchain::HeaderBackend; use sp_consensus_beefy::{ - check_invalid_fork_proof, + check_fork_equivocation_proof, ecdsa_crypto::{AuthorityId, Signature}, - BeefyApi, InvalidForkCommitmentProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, Commitment, OpaqueKeyOwnershipProof, SignedCommitment, + BeefyApi, ForkEquivocationProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, Commitment, OpaqueKeyOwnershipProof, SignedCommitment, }; use sp_runtime::{ generic::BlockId, @@ -110,7 +110,7 @@ where let validator_set = self.active_validator_set_at(correct_header)?; let set_id = validator_set.id(); - let proof = InvalidForkCommitmentProof { + let proof = ForkEquivocationProof { commitment: signed_commitment.commitment.clone(), signatories: vec![], expected_payload: correct_payload.clone(), @@ -118,7 +118,7 @@ where if signed_commitment.commitment.validator_set_id != set_id || signed_commitment.commitment.payload != *correct_payload || - !check_invalid_fork_proof::, AuthorityId, BeefySignatureHasher>(&proof) + !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher>(&proof) { debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); return Ok(()) @@ -155,14 +155,14 @@ where fn report_invalid_fork_commitments( &self, - proof: InvalidForkCommitmentProof, AuthorityId, Signature>, + proof: ForkEquivocationProof, AuthorityId, Signature>, correct_header: &B::Header, ) -> Result<(), Error> { let validator_set = self.active_validator_set_at(correct_header)?; let set_id = validator_set.id(); if proof.commitment.validator_set_id != set_id || - !check_invalid_fork_proof::, AuthorityId, BeefySignatureHasher>(&proof) + !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher>(&proof) { debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); return Ok(()) @@ -218,7 +218,7 @@ where let (header, expected_payload) = self.expected_header_and_payload(number)?; if vote.commitment.payload != expected_payload { let validator_set = self.active_validator_set_at(&header)?; - let proof = InvalidForkCommitmentProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], expected_payload }; + let proof = ForkEquivocationProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], expected_payload }; self.report_invalid_fork_commitments(proof, &header)?; } Ok(()) @@ -258,7 +258,7 @@ where let signatories = validator_set.validators().iter().cloned().zip(signatures.into_iter()) .filter_map(|(id, signature)| signature.map(|sig| (id, sig))).collect(); - let proof = InvalidForkCommitmentProof { commitment, signatories, expected_payload }; + let proof = ForkEquivocationProof { commitment, signatories, expected_payload }; self.report_invalid_fork_commitments( proof, &header, diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 3850101cb013c..9fedfa688b102 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -63,7 +63,7 @@ const LOG_TARGET: &str = "runtime::beefy"; pub mod pallet { use super::*; use frame_system::pallet_prelude::BlockNumberFor; - use sp_consensus_beefy::InvalidForkCommitmentProof; + use sp_consensus_beefy::ForkEquivocationProof; #[pallet::config] pub trait Config: frame_system::Config { @@ -281,7 +281,7 @@ pub mod pallet { pub fn report_invalid_fork_commitment( origin: OriginFor, invalid_fork_proof: Box< - InvalidForkCommitmentProof< + ForkEquivocationProof< BlockNumberFor, T::BeefyId, ::Signature, @@ -314,7 +314,7 @@ pub mod pallet { pub fn report_invalid_fork_commitment_unsigned( origin: OriginFor, invalid_fork_proof: Box< - InvalidForkCommitmentProof< + ForkEquivocationProof< BlockNumberFor, T::BeefyId, ::Signature, @@ -374,7 +374,7 @@ impl Pallet { /// a call to `report_invalid_fork_unsigned` and will push the transaction /// to the pool. Only useful in an offchain context. pub fn submit_unsigned_invalid_fork_report( - _invalid_fork_proof: sp_consensus_beefy::InvalidForkCommitmentProof, T::BeefyId, + _invalid_fork_proof: sp_consensus_beefy::ForkEquivocationProof, T::BeefyId, ::Signature, >, _key_owner_proofs: Vec, diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index 2aaef24dae5fd..de59ec20dc2e1 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -251,10 +251,10 @@ impl VoteEquivocationProof { } } -/// Proof of invalid commitment on a given set id. -/// This proof shows commitment signed on a different fork than finalized by GRANDPA. +/// 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 InvalidForkCommitmentProof { +pub struct ForkEquivocationProof { /// Commitment for a block on different fork than one at the same height in /// this client's chain. /// TODO: maybe replace {commitment, signatories} with SignedCommitment @@ -268,7 +268,7 @@ pub struct InvalidForkCommitmentProof { pub expected_payload: Payload, } -impl InvalidForkCommitmentProof { +impl ForkEquivocationProof { /// Returns the authority id of the misbehaving voter. pub fn offender_ids(&self) -> Vec<&Id> { self.signatories.iter().map(|(id, _)| id).collect() @@ -345,15 +345,15 @@ where /// 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_invalid_fork_proof( - proof: &InvalidForkCommitmentProof::Signature>, +pub fn check_fork_equivocation_proof( + proof: &ForkEquivocationProof::Signature>, ) -> bool where Id: BeefyAuthorityId + PartialEq, Number: Clone + Encode + PartialEq, MsgHash: Hash, { - let InvalidForkCommitmentProof { commitment, signatories, expected_payload } = proof; + let ForkEquivocationProof { commitment, signatories, expected_payload } = proof; // check that `payload` on the `vote` is different that the `expected_payload` (checked first // since cheap failfast). @@ -442,7 +442,7 @@ sp_api::decl_runtime_apis! { /// hardcoded to return `None`). Only useful in an offchain context. fn submit_report_invalid_fork_unsigned_extrinsic( invalid_fork_proof: - InvalidForkCommitmentProof, AuthorityId, ::Signature>, + ForkEquivocationProof, AuthorityId, ::Signature>, key_owner_proofs: Vec, ) -> Option<()>; From 53a1a88e021ea22636f384954f38fc213e71d248 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 18:42:38 +0200 Subject: [PATCH 17/41] renames across submit_report_... --- .../beefy/src/communication/fisherman.rs | 10 ++++---- client/consensus/beefy/src/tests.rs | 2 +- client/consensus/beefy/src/worker.rs | 2 +- frame/beefy/src/equivocation.rs | 8 +++---- frame/beefy/src/lib.rs | 24 +++++++++---------- frame/beefy/src/tests.rs | 22 ++++++++--------- primitives/consensus/beefy/src/lib.rs | 8 +++---- 7 files changed, 38 insertions(+), 38 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index c228e4bd2a7d0..4b79a20eea8ed 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -147,13 +147,13 @@ where // submit invalid fork vote report at **best** block let best_block_hash = self.backend.blockchain().info().best_hash; runtime_api - .submit_report_invalid_fork_unsigned_extrinsic(best_block_hash, proof, key_owner_proofs) + .submit_report_fork_equivocation_unsigned_extrinsic(best_block_hash, proof, key_owner_proofs) .map_err(Error::RuntimeApi)?; Ok(()) } - fn report_invalid_fork_commitments( + fn report_fork_equivocation( &self, proof: ForkEquivocationProof, AuthorityId, Signature>, correct_header: &B::Header, @@ -192,7 +192,7 @@ where // submit invalid fork vote report at **best** block let best_block_hash = self.backend.blockchain().info().best_hash; runtime_api - .submit_report_invalid_fork_unsigned_extrinsic(best_block_hash, proof, key_owner_proofs) + .submit_report_fork_equivocation_unsigned_extrinsic(best_block_hash, proof, key_owner_proofs) .map_err(Error::RuntimeApi)?; Ok(()) @@ -219,7 +219,7 @@ where if vote.commitment.payload != expected_payload { let validator_set = self.active_validator_set_at(&header)?; let proof = ForkEquivocationProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], expected_payload }; - self.report_invalid_fork_commitments(proof, &header)?; + self.report_fork_equivocation(proof, &header)?; } Ok(()) } @@ -259,7 +259,7 @@ where .filter_map(|(id, signature)| signature.map(|sig| (id, sig))).collect(); let proof = ForkEquivocationProof { commitment, signatories, expected_payload }; - self.report_invalid_fork_commitments( + self.report_fork_equivocation( proof, &header, )?; diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index ee7e3205a0fb6..98fc21dd07e49 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -324,7 +324,7 @@ sp_api::mock_impl_runtime_apis! { Some(self.inner.validator_set.clone()) } - fn submit_report_equivocation_unsigned_extrinsic( + fn submit_report_vote_equivocation_unsigned_extrinsic( proof: VoteEquivocationProof, AuthorityId, Signature>, _dummy: OpaqueKeyOwnershipProof, ) -> Option<()> { diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index d594101f74575..a29f0cf5e966f 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -981,7 +981,7 @@ where // submit equivocation report at **best** block let best_block_hash = self.backend.blockchain().info().best_hash; runtime_api - .submit_report_equivocation_unsigned_extrinsic(best_block_hash, proof, key_owner_proof) + .submit_report_vote_equivocation_unsigned_extrinsic(best_block_hash, proof, key_owner_proof) .map_err(Error::RuntimeApi)?; Ok(()) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 756b7d7198b54..2c05429e88dac 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -154,7 +154,7 @@ where use frame_system::offchain::SubmitTransaction; let (equivocation_proof, key_owner_proof) = evidence; - let call = Call::report_equivocation_unsigned { + let call = Call::report_vote_equivocation_unsigned { equivocation_proof: Box::new(equivocation_proof), key_owner_proof, }; @@ -238,12 +238,12 @@ where } /// Methods for the `ValidateUnsigned` implementation: -/// It restricts calls to `report_equivocation_unsigned` to local calls (i.e. extrinsics generated +/// It restricts calls to `report_vote_equivocation_unsigned` to local calls (i.e. extrinsics generated /// on this node) or that already in a block. This guarantees that only block authors can include /// unsigned equivocation reports. impl Pallet { pub fn validate_unsigned(source: TransactionSource, call: &Call) -> TransactionValidity { - if let Call::report_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { + if let Call::report_vote_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { // discard equivocation report not coming from the local node match source { TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }, @@ -281,7 +281,7 @@ impl Pallet { } pub fn pre_dispatch(call: &Call) -> Result<(), TransactionValidityError> { - if let Call::report_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { + if let Call::report_vote_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); T::EquivocationReportSystem::check_evidence(evidence) } else { diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 9fedfa688b102..8b3c0b1a17e83 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -249,7 +249,7 @@ pub mod pallet { key_owner_proof.validator_count(), T::MaxNominators::get(), ))] - pub fn report_equivocation_unsigned( + pub fn report_vote_equivocation_unsigned( origin: OriginFor, equivocation_proof: Box< VoteEquivocationProof< @@ -278,9 +278,9 @@ pub mod pallet { key_owner_proof.validator_count(), T::MaxNominators::get(), ))] - pub fn report_invalid_fork_commitment( + pub fn report_fork_equivocation( origin: OriginFor, - invalid_fork_proof: Box< + fork_equivocation_proof: Box< ForkEquivocationProof< BlockNumberFor, T::BeefyId, @@ -294,7 +294,7 @@ pub mod pallet { // TODO: // T::EquivocationReportSystem::process_evidence( // Some(reporter), - // (*invalid_fork_proof, key_owner_proof), + // (*fork_equivocation_proof, key_owner_proof), // )?; // Waive the fee since the report is valid and beneficial Ok(Pays::No.into()) @@ -311,9 +311,9 @@ pub mod pallet { /// reporter. #[pallet::call_index(3)] #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count(), T::MaxNominators::get(),))] - pub fn report_invalid_fork_commitment_unsigned( + pub fn report_fork_equivocation_unsigned( origin: OriginFor, - invalid_fork_proof: Box< + fork_equivocation_proof: Box< ForkEquivocationProof< BlockNumberFor, T::BeefyId, @@ -327,7 +327,7 @@ pub mod pallet { // TODO: // T::EquivocationReportSystem::process_evidence( // None, - // (*invalid_fork_proof, key_owner_proof), + // (*fork_equivocation_proof, key_owner_proof), // )?; Ok(Pays::No.into()) } @@ -358,7 +358,7 @@ impl Pallet { /// Submits an extrinsic to report an equivocation. This method will create /// an unsigned extrinsic with a call to `report_equivocation_unsigned` and /// will push the transaction to the pool. Only useful in an offchain context. - pub fn submit_unsigned_equivocation_report( + pub fn submit_unsigned_vote_equivocation_report( equivocation_proof: VoteEquivocationProof< BlockNumberFor, T::BeefyId, @@ -371,15 +371,15 @@ impl Pallet { /// Submits an extrinsic to report an invalid fork signed by potentially /// multiple signatories. This method will create an unsigned extrinsic with - /// a call to `report_invalid_fork_unsigned` and will push the transaction + /// a call to `report_fork_equivocation_unsigned` and will push the transaction /// to the pool. Only useful in an offchain context. - pub fn submit_unsigned_invalid_fork_report( - _invalid_fork_proof: sp_consensus_beefy::ForkEquivocationProof, T::BeefyId, + pub fn submit_unsigned_fork_equivocation_report( + _fork_equivocation_proof: sp_consensus_beefy::ForkEquivocationProof, T::BeefyId, ::Signature, >, _key_owner_proofs: Vec, ) -> Option<()> { - // T::EquivocationReportSystem::publish_evidence((invalid_fork_proof, key_owner_proofs)).ok() + // T::EquivocationReportSystem::publish_evidence((fork_equivocation_proof, key_owner_proofs)).ok() None } diff --git a/frame/beefy/src/tests.rs b/frame/beefy/src/tests.rs index a493286decf59..45a7080980763 100644 --- a/frame/beefy/src/tests.rs +++ b/frame/beefy/src/tests.rs @@ -300,7 +300,7 @@ fn report_equivocation_current_set_works() { let key_owner_proof = Historical::prove((BEEFY_KEY_TYPE, &equivocation_key)).unwrap(); // report the equivocation and the tx should be dispatched successfully - assert_ok!(Beefy::report_equivocation_unsigned( + assert_ok!(Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof), key_owner_proof, @@ -383,7 +383,7 @@ fn report_equivocation_old_set_works() { ); // report the equivocation and the tx should be dispatched successfully - assert_ok!(Beefy::report_equivocation_unsigned( + assert_ok!(Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof), key_owner_proof, @@ -446,7 +446,7 @@ fn report_equivocation_invalid_set_id() { // the call for reporting the equivocation should error assert_err!( - Beefy::report_equivocation_unsigned( + Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof), key_owner_proof, @@ -489,7 +489,7 @@ fn report_equivocation_invalid_session() { // report an equivocation for the current set using an key ownership // proof from the previous set, the session should be invalid. assert_err!( - Beefy::report_equivocation_unsigned( + Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof), key_owner_proof, @@ -537,7 +537,7 @@ fn report_equivocation_invalid_key_owner_proof() { // report an equivocation for the current set using a key ownership // proof for a different key than the one in the equivocation proof. assert_err!( - Beefy::report_equivocation_unsigned( + Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof), invalid_key_owner_proof, @@ -568,7 +568,7 @@ fn report_equivocation_invalid_equivocation_proof() { let assert_invalid_equivocation_proof = |equivocation_proof| { assert_err!( - Beefy::report_equivocation_unsigned( + Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof), key_owner_proof.clone(), @@ -646,7 +646,7 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() { let key_owner_proof = Historical::prove((BEEFY_KEY_TYPE, &equivocation_key)).unwrap(); - let call = Call::report_equivocation_unsigned { + let call = Call::report_vote_equivocation_unsigned { equivocation_proof: Box::new(equivocation_proof.clone()), key_owner_proof: key_owner_proof.clone(), }; @@ -681,7 +681,7 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() { assert_ok!(::pre_dispatch(&call)); // we submit the report - Beefy::report_equivocation_unsigned( + Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof), key_owner_proof, @@ -752,7 +752,7 @@ fn valid_equivocation_reports_dont_pay_fees() { let key_owner_proof = Historical::prove((BEEFY_KEY_TYPE, &equivocation_key)).unwrap(); // check the dispatch info for the call. - let info = Call::::report_equivocation_unsigned { + let info = Call::::report_vote_equivocation_unsigned { equivocation_proof: Box::new(equivocation_proof.clone()), key_owner_proof: key_owner_proof.clone(), } @@ -763,7 +763,7 @@ fn valid_equivocation_reports_dont_pay_fees() { assert_eq!(info.pays_fee, Pays::Yes); // report the equivocation. - let post_info = Beefy::report_equivocation_unsigned( + let post_info = Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof.clone()), key_owner_proof.clone(), @@ -777,7 +777,7 @@ fn valid_equivocation_reports_dont_pay_fees() { // report the equivocation again which is invalid now since it is // duplicate. - let post_info = Beefy::report_equivocation_unsigned( + let post_info = Beefy::report_vote_equivocation_unsigned( RuntimeOrigin::none(), Box::new(equivocation_proof), key_owner_proof, diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index de59ec20dc2e1..bf1196adce2f9 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -426,8 +426,8 @@ sp_api::decl_runtime_apis! { /// `None` when creation of the extrinsic fails, e.g. if equivocation /// reporting is disabled for the given runtime (i.e. this method is /// hardcoded to return `None`). Only useful in an offchain context. - fn submit_report_equivocation_unsigned_extrinsic( - equivocation_proof: + fn submit_report_vote_equivocation_unsigned_extrinsic( + vote_equivocation_proof: VoteEquivocationProof, AuthorityId, ::Signature>, key_owner_proof: OpaqueKeyOwnershipProof, ) -> Option<()>; @@ -440,8 +440,8 @@ sp_api::decl_runtime_apis! { /// `None` when creation of the extrinsic fails, e.g. if equivocation /// reporting is disabled for the given runtime (i.e. this method is /// hardcoded to return `None`). Only useful in an offchain context. - fn submit_report_invalid_fork_unsigned_extrinsic( - invalid_fork_proof: + fn submit_report_fork_equivocation_unsigned_extrinsic( + fork_equivocation_proof: ForkEquivocationProof, AuthorityId, ::Signature>, key_owner_proofs: Vec, ) -> Option<()>; From 7e0ec7788316fe03c07d01722d3e7a17382f67be Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 19:07:19 +0200 Subject: [PATCH 18/41] convert EquivocationEvidenceFor to enum (minimal) --- frame/beefy/src/equivocation.rs | 81 +++++++++++++++++++++------------ frame/beefy/src/lib.rs | 18 ++++++-- 2 files changed, 68 insertions(+), 31 deletions(-) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 2c05429e88dac..c1a235ca69a15 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -125,15 +125,21 @@ where pub struct EquivocationReportSystem(sp_std::marker::PhantomData<(T, R, P, L)>); /// Equivocation evidence convenience alias. -// TODO: use an enum that takes either `VoteEquivocationProof` or `ForkEquivocationProof` -pub type EquivocationEvidenceFor = ( - VoteEquivocationProof< - BlockNumberFor, - ::BeefyId, - <::BeefyId as RuntimeAppPublic>::Signature, - >, - ::KeyOwnerProof, -); +pub enum EquivocationEvidenceFor { + VoteEquivocationProof( + VoteEquivocationProof< + BlockNumberFor, + ::BeefyId, + <::BeefyId as RuntimeAppPublic>::Signature, + >, + ::KeyOwnerProof, + ), + // ForkEquivocationProof(sp_consensus_beefy::ForkEquivocationProof< + // BlockNumberFor, + // ::BeefyId, + // <::BeefyId as RuntimeAppPublic>::Signature, + // >) +} impl OffenceReportSystem, EquivocationEvidenceFor> for EquivocationReportSystem @@ -152,11 +158,14 @@ where fn publish_evidence(evidence: EquivocationEvidenceFor) -> Result<(), ()> { use frame_system::offchain::SubmitTransaction; - let (equivocation_proof, key_owner_proof) = evidence; - let call = Call::report_vote_equivocation_unsigned { - equivocation_proof: Box::new(equivocation_proof), - key_owner_proof, + let call = match evidence { + EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => { + Call::report_vote_equivocation_unsigned { + equivocation_proof: Box::new(equivocation_proof), + key_owner_proof, + } + } }; let res = SubmitTransaction::>::submit_unsigned_transaction(call.into()); @@ -170,16 +179,17 @@ where fn check_evidence( evidence: EquivocationEvidenceFor, ) -> Result<(), TransactionValidityError> { - let (equivocation_proof, key_owner_proof) = evidence; - - // Check the membership proof to extract the offender's id - let key = (BEEFY_KEY_TYPE, equivocation_proof.offender_id().clone()); - let offender = P::check_proof(key, key_owner_proof).ok_or(InvalidTransaction::BadProof)?; - - // Check if the offence has already been reported, and if so then we can discard the report. - let time_slot = TimeSlot { - set_id: equivocation_proof.set_id(), - round: *equivocation_proof.round_number(), + let (offender, time_slot) = match evidence { + EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => { + let key = (BEEFY_KEY_TYPE, equivocation_proof.offender_id().clone()); + + // Check if the offence has already been reported, and if so then we can discard the report. + let time_slot = TimeSlot { + set_id: equivocation_proof.set_id(), + round: *equivocation_proof.round_number(), + }; + (P::check_proof(key, key_owner_proof).ok_or(InvalidTransaction::BadProof)?, time_slot) + } }; if R::is_known_offence(&[offender], &time_slot) { @@ -193,9 +203,16 @@ where reporter: Option, evidence: EquivocationEvidenceFor, ) -> Result<(), DispatchError> { - let (equivocation_proof, key_owner_proof) = evidence; let reporter = reporter.or_else(|| >::author()); - let offender = equivocation_proof.offender_id().clone(); + + let (equivocation_proof, key_owner_proof, offender) = match evidence { + EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => + ( + equivocation_proof.clone(), + key_owner_proof, + equivocation_proof.offender_id().clone(), + ), + }; // We check the equivocation within the context of its set id (and // associated session) and round. We also need to know the validator @@ -256,7 +273,10 @@ impl Pallet { }, } - let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); + let evidence = EquivocationEvidenceFor::::VoteEquivocationProof( + *equivocation_proof.clone(), + key_owner_proof.clone(), + ); T::EquivocationReportSystem::check_evidence(evidence)?; let longevity = @@ -281,8 +301,13 @@ impl Pallet { } pub fn pre_dispatch(call: &Call) -> Result<(), TransactionValidityError> { - if let Call::report_vote_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { - let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); + if let Call::report_vote_equivocation_unsigned { equivocation_proof, key_owner_proof } = + call + { + let evidence = EquivocationEvidenceFor::::VoteEquivocationProof( + *equivocation_proof.clone(), + key_owner_proof.clone(), + ); T::EquivocationReportSystem::check_evidence(evidence) } else { Err(InvalidTransaction::Call.into()) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 8b3c0b1a17e83..4ad9f109766e4 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -229,7 +229,10 @@ pub mod pallet { T::EquivocationReportSystem::process_evidence( Some(reporter), - (*equivocation_proof, key_owner_proof), + EquivocationEvidenceFor::VoteEquivocationProof( + *equivocation_proof, + key_owner_proof, + ), )?; // Waive the fee since the report is valid and beneficial Ok(Pays::No.into()) @@ -264,7 +267,10 @@ pub mod pallet { T::EquivocationReportSystem::process_evidence( None, - (*equivocation_proof, key_owner_proof), + EquivocationEvidenceFor::::VoteEquivocationProof( + *equivocation_proof, + key_owner_proof, + ), )?; Ok(Pays::No.into()) } @@ -366,7 +372,13 @@ impl Pallet { >, key_owner_proof: T::KeyOwnerProof, ) -> Option<()> { - T::EquivocationReportSystem::publish_evidence((equivocation_proof, key_owner_proof)).ok() + T::EquivocationReportSystem::publish_evidence( + EquivocationEvidenceFor::::VoteEquivocationProof( + equivocation_proof, + key_owner_proof, + ), + ) + .ok() } /// Submits an extrinsic to report an invalid fork signed by potentially From e8b97b353e25bf5dc6d7fc121937dfcc73c01f56 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 21:22:49 +0200 Subject: [PATCH 19/41] handle ForkEquivocationProof enum variant --- frame/beefy/src/equivocation.rs | 96 ++++++++++++++++++++++----------- frame/beefy/src/lib.rs | 13 ++--- 2 files changed, 73 insertions(+), 36 deletions(-) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index c1a235ca69a15..16517f280ba75 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -41,7 +41,7 @@ use frame_support::{ }; use frame_system::pallet_prelude::BlockNumberFor; use log::{error, info}; -use sp_consensus_beefy::{VoteEquivocationProof, ValidatorSetId, KEY_TYPE as BEEFY_KEY_TYPE}; +use sp_consensus_beefy::{VoteEquivocationProof, ForkEquivocationProof, ValidatorSetId, KEY_TYPE as BEEFY_KEY_TYPE}; use sp_runtime::{ transaction_validity::{ InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity, @@ -134,11 +134,13 @@ pub enum EquivocationEvidenceFor { >, ::KeyOwnerProof, ), - // ForkEquivocationProof(sp_consensus_beefy::ForkEquivocationProof< - // BlockNumberFor, - // ::BeefyId, - // <::BeefyId as RuntimeAppPublic>::Signature, - // >) + ForkEquivocationProof( + ForkEquivocationProof, + ::BeefyId, + <::BeefyId as RuntimeAppPublic>::Signature, + >, + Vec<::KeyOwnerProof>, + ) } impl OffenceReportSystem, EquivocationEvidenceFor> @@ -166,6 +168,12 @@ where key_owner_proof, } } + EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, key_owner_proofs) => { + Call::report_fork_equivocation_unsigned { + equivocation_proof: Box::new(equivocation_proof), + key_owner_proofs, + } + } }; let res = SubmitTransaction::>::submit_unsigned_transaction(call.into()); @@ -179,20 +187,35 @@ where fn check_evidence( evidence: EquivocationEvidenceFor, ) -> Result<(), TransactionValidityError> { - let (offender, time_slot) = match evidence { + let (offenders, key_owner_proofs, time_slot) = match &evidence { EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => { - let key = (BEEFY_KEY_TYPE, equivocation_proof.offender_id().clone()); - // Check if the offence has already been reported, and if so then we can discard the report. let time_slot = TimeSlot { set_id: equivocation_proof.set_id(), round: *equivocation_proof.round_number(), }; - (P::check_proof(key, key_owner_proof).ok_or(InvalidTransaction::BadProof)?, time_slot) - } + (vec![equivocation_proof.offender_id()], vec![key_owner_proof.clone()], time_slot) + }, + EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, key_owner_proofs) => { + // Check if the offence has already been reported, and if so then we can discard the report. + let time_slot = TimeSlot { + set_id: equivocation_proof.set_id(), + round: *equivocation_proof.round_number(), + }; + let offenders = equivocation_proof.offender_ids(); // clone data here + (offenders, key_owner_proofs.to_owned(), time_slot) + }, }; - if R::is_known_offence(&[offender], &time_slot) { + // Validate the key ownership proof extracting the id of the offender. + 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::>>() + .ok_or(InvalidTransaction::BadProof)?; + + if R::is_known_offence(&offenders, &time_slot) { Err(InvalidTransaction::Stale.into()) } else { Ok(()) @@ -205,31 +228,44 @@ where ) -> Result<(), DispatchError> { let reporter = reporter.or_else(|| >::author()); - let (equivocation_proof, key_owner_proof, offender) = match evidence { - EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => - ( - equivocation_proof.clone(), - key_owner_proof, - equivocation_proof.offender_id().clone(), - ), + let (offenders, key_owner_proofs, set_id, round) = match &evidence { + EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => { + (vec![equivocation_proof.offender_id()], vec![key_owner_proof.clone()], equivocation_proof.set_id(), *equivocation_proof.round_number()) + }, + EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, key_owner_proofs) => { + let offenders = equivocation_proof.offender_ids(); // clone data here + (offenders, key_owner_proofs.to_owned(), equivocation_proof.set_id(), *equivocation_proof.round_number()) + }, }; // We check the equivocation within the context of its set id (and // associated session) and round. We also need to know the validator // set count at the time of the offence since it is required to calculate // the slash amount. - let set_id = equivocation_proof.set_id(); - let round = *equivocation_proof.round_number(); - let session_index = key_owner_proof.session(); - let validator_set_count = key_owner_proof.validator_count(); - - // Validate the key ownership proof extracting the id of the offender. - let offender = P::check_proof((BEEFY_KEY_TYPE, offender), key_owner_proof) + let session_index = key_owner_proofs[0].session(); + let validator_set_count = key_owner_proofs[0].validator_count(); + + // Validate the key ownership proof extracting the ids of the offenders. + 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::>>() .ok_or(Error::::InvalidKeyOwnershipProof)?; - // Validate equivocation proof (check votes are different and signatures are valid). - if !sp_consensus_beefy::check_vote_equivocation_proof(&equivocation_proof) { - return Err(Error::::InvalidVoteEquivocationProof.into()) + match &evidence { + EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, _) => { + // Validate equivocation proof (check votes are different and signatures are valid). + if !sp_consensus_beefy::check_vote_equivocation_proof(&equivocation_proof) { + return Err(Error::::InvalidVoteEquivocationProof.into()) + } + }, + EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, _) => { + // Validate equivocation proof (check commitment is to unexpected payload and signatures are valid). + if !sp_consensus_beefy::check_fork_equivocation_proof(&equivocation_proof) { + return Err(Error::::InvalidForkEquivocationProof.into()) + } + }, } // Check that the session id for the membership proof is within the @@ -244,7 +280,7 @@ where time_slot: TimeSlot { set_id, round }, session_index, validator_set_count, - offenders: vec![offender], + offenders, }; R::report_offence(reporter.into_iter().collect(), offence) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 4ad9f109766e4..6bff931f8aadd 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -112,8 +112,6 @@ pub mod pallet { /// Defines methods to publish, check and process an equivocation offence. type EquivocationReportSystem: OffenceReportSystem< Option, - // TODO: make below an enum that takes either `VoteEquivocationProof` or - // `InvalidForkCommitmentProof` EquivocationEvidenceFor, >; } @@ -199,6 +197,8 @@ pub mod pallet { InvalidKeyOwnershipProof, /// An equivocation proof provided as part of a voter equivocation report is invalid. InvalidVoteEquivocationProof, + /// An equivocation proof provided as part of a fork equivocation report is invalid. + InvalidForkEquivocationProof, /// A given equivocation report is valid but already previously reported. DuplicateOffenceReport, } @@ -286,7 +286,7 @@ pub mod pallet { ))] pub fn report_fork_equivocation( origin: OriginFor, - fork_equivocation_proof: Box< + equivocation_proof: Box< ForkEquivocationProof< BlockNumberFor, T::BeefyId, @@ -315,18 +315,19 @@ pub mod pallet { /// 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() #[pallet::call_index(3)] - #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proof.validator_count(), T::MaxNominators::get(),))] + #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proofs[0].validator_count(), T::MaxNominators::get(),))] pub fn report_fork_equivocation_unsigned( origin: OriginFor, - fork_equivocation_proof: Box< + equivocation_proof: Box< ForkEquivocationProof< BlockNumberFor, T::BeefyId, ::Signature, >, >, - key_owner_proof: T::KeyOwnerProof, + key_owner_proofs: Vec, ) -> DispatchResultWithPostInfo { ensure_none(origin)?; From d5ceb231c0746da09170c5f0a418359f7db52cef Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 8 Aug 2023 11:40:51 +0200 Subject: [PATCH 20/41] reduce find_mmr_root_digest trait constraint reduce from Block to Header: less restrictive. --- client/consensus/beefy/src/tests.rs | 4 ++-- primitives/consensus/beefy/src/mmr.rs | 9 ++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 98fc21dd07e49..3fdf28130c77d 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -44,7 +44,7 @@ use sc_consensus::{ }; use sc_network::{config::RequestResponseConfig, ProtocolName}; use sc_network_test::{ - Block, BlockImportAdapter, FullPeerConfig, PassThroughVerifier, Peer, PeersClient, + Block, BlockImportAdapter, Header, FullPeerConfig, PassThroughVerifier, Peer, PeersClient, PeersFullClient, TestNetFactory, }; use sc_utils::notification::NotificationReceiver; @@ -1406,7 +1406,7 @@ async fn gossipped_finality_proofs() { // Simulate Charlie vote on #2 let header = net.lock().peer(2).client().as_client().expect_header(finalize).unwrap(); - let mmr_root = find_mmr_root_digest::(&header).unwrap(); + let mmr_root = find_mmr_root_digest::
(&header).unwrap(); let payload = Payload::from_single_entry(known_payloads::MMR_ROOT_ID, mmr_root.encode()); let commitment = Commitment { payload, block_number, validator_set_id: validator_set.id() }; let signature = sign_commitment(&BeefyKeyring::Charlie, &commitment); diff --git a/primitives/consensus/beefy/src/mmr.rs b/primitives/consensus/beefy/src/mmr.rs index b318d84475f7f..6bee2c0419235 100644 --- a/primitives/consensus/beefy/src/mmr.rs +++ b/primitives/consensus/beefy/src/mmr.rs @@ -134,7 +134,7 @@ pub struct BeefyAuthoritySet { pub type BeefyNextAuthoritySet = BeefyAuthoritySet; /// Extract the MMR root hash from a digest in the given header, if it exists. -pub fn find_mmr_root_digest(header: &B::Header) -> Option { +pub fn find_mmr_root_digest(header: &H) -> Option { let id = OpaqueDigestItemId::Consensus(&BEEFY_ENGINE_ID); let filter = |log: ConsensusLog| match log { @@ -181,7 +181,7 @@ mod mmr_root_provider { /// Simple wrapper that gets MMR root from header digests or from client state. fn mmr_root_from_digest_or_runtime(&self, header: &B::Header) -> Option { - find_mmr_root_digest::(header).or_else(|| { + find_mmr_root_digest::(header).or_else(|| { self.runtime.runtime_api().mmr_root(header.hash()).ok().and_then(|r| r.ok()) }) } @@ -233,7 +233,6 @@ mod tests { #[test] fn extract_mmr_root_digest() { type Header = sp_runtime::generic::Header; - type Block = sp_runtime::generic::Block; let mut header = Header::new( 1u64, Default::default(), @@ -243,7 +242,7 @@ mod tests { ); // verify empty digest shows nothing - assert!(find_mmr_root_digest::(&header).is_none()); + assert!(find_mmr_root_digest::
(&header).is_none()); let mmr_root_hash = H256::random(); header.digest_mut().push(DigestItem::Consensus( @@ -252,7 +251,7 @@ mod tests { )); // verify validator set is correctly extracted from digest - let extracted = find_mmr_root_digest::(&header); + let extracted = find_mmr_root_digest::
(&header); assert_eq!(extracted, Some(mmr_root_hash)); } } From 3b274fa28125c8d6275382c79bd927e54da1eeef Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 8 Aug 2023 12:08:20 +0200 Subject: [PATCH 21/41] fix fork equiv. call interfaces (vecs of sigs) --- frame/beefy/src/lib.rs | 16 ++++++++-------- primitives/consensus/beefy/src/mmr.rs | 2 +- primitives/consensus/beefy/src/payload.rs | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 6bff931f8aadd..58fd93faf05c5 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -279,9 +279,10 @@ pub mod pallet { /// 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() #[pallet::call_index(2)] #[pallet::weight(T::WeightInfo::report_equivocation( - key_owner_proof.validator_count(), + key_owner_proofs[0].validator_count(), T::MaxNominators::get(), ))] pub fn report_fork_equivocation( @@ -293,15 +294,14 @@ pub mod pallet { ::Signature, >, >, - key_owner_proof: T::KeyOwnerProof, + key_owner_proofs: Vec, ) -> DispatchResultWithPostInfo { let reporter = ensure_signed(origin)?; - // TODO: - // T::EquivocationReportSystem::process_evidence( - // Some(reporter), - // (*fork_equivocation_proof, key_owner_proof), - // )?; + T::EquivocationReportSystem::process_evidence( + Some(reporter), + EquivocationEvidenceFor::ForkEquivocationProof(*equivocation_proof, key_owner_proofs), + )?; // Waive the fee since the report is valid and beneficial Ok(Pays::No.into()) } @@ -315,7 +315,7 @@ pub mod pallet { /// 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() + // TODO: fix key_owner_proofs[0].validator_count() #[pallet::call_index(3)] #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proofs[0].validator_count(), T::MaxNominators::get(),))] pub fn report_fork_equivocation_unsigned( diff --git a/primitives/consensus/beefy/src/mmr.rs b/primitives/consensus/beefy/src/mmr.rs index 6bee2c0419235..6df5925acc195 100644 --- a/primitives/consensus/beefy/src/mmr.rs +++ b/primitives/consensus/beefy/src/mmr.rs @@ -205,7 +205,7 @@ mod mmr_root_provider { mod tests { use super::*; use crate::H256; - use sp_runtime::{traits::BlakeTwo256, Digest, DigestItem, OpaqueExtrinsic}; + use sp_runtime::{traits::BlakeTwo256, Digest, DigestItem}; #[test] fn should_construct_version_correctly() { diff --git a/primitives/consensus/beefy/src/payload.rs b/primitives/consensus/beefy/src/payload.rs index c80f00255281b..43b83911dd629 100644 --- a/primitives/consensus/beefy/src/payload.rs +++ b/primitives/consensus/beefy/src/payload.rs @@ -43,7 +43,7 @@ pub mod known_payloads { pub struct Payload(Vec<(BeefyPayloadId, Vec)>); impl Payload { - /// Construct a new payload given an initial vallue + /// Construct a new payload given an initial value pub fn from_single_entry(id: BeefyPayloadId, value: Vec) -> Self { Self(vec![(id, value)]) } From 8701d7fb3d33cbbfa76319a0a9350b84641ff211 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 8 Aug 2023 12:14:15 +0200 Subject: [PATCH 22/41] check proof's payload against correct header's --- .../beefy/src/communication/fisherman.rs | 24 +++++----- frame/beefy/src/equivocation.rs | 16 ++++++- frame/beefy/src/lib.rs | 6 ++- primitives/consensus/beefy/src/lib.rs | 45 ++++++++++++------- 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 4b79a20eea8ed..714195835605a 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -113,12 +113,12 @@ where let proof = ForkEquivocationProof { commitment: signed_commitment.commitment.clone(), signatories: vec![], - expected_payload: correct_payload.clone(), + correct_header: correct_header.clone(), }; if signed_commitment.commitment.validator_set_id != set_id || signed_commitment.commitment.payload != *correct_payload || - !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher>(&proof) + !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher, B::Header>(&proof) { debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); return Ok(()) @@ -155,14 +155,14 @@ where fn report_fork_equivocation( &self, - proof: ForkEquivocationProof, AuthorityId, Signature>, + proof: ForkEquivocationProof, AuthorityId, Signature, B::Header>, correct_header: &B::Header, ) -> Result<(), Error> { let validator_set = self.active_validator_set_at(correct_header)?; let set_id = validator_set.id(); if proof.commitment.validator_set_id != set_id || - !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher>(&proof) + !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher, B::Header>(&proof) { debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); return Ok(()) @@ -215,11 +215,11 @@ where vote: VoteMessage, AuthorityId, Signature>, ) -> Result<(), Error> { let number = vote.commitment.block_number; - let (header, expected_payload) = self.expected_header_and_payload(number)?; + let (correct_header, expected_payload) = self.expected_header_and_payload(number)?; if vote.commitment.payload != expected_payload { - let validator_set = self.active_validator_set_at(&header)?; - let proof = ForkEquivocationProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], expected_payload }; - self.report_fork_equivocation(proof, &header)?; + let validator_set = self.active_validator_set_at(&correct_header)?; + let proof = ForkEquivocationProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], correct_header: correct_header.clone() }; + self.report_fork_equivocation(proof, &correct_header)?; } Ok(()) } @@ -246,10 +246,10 @@ where BeefyVersionedFinalityProof::::V1(inner) => (inner.commitment, inner.signatures), }; let number = commitment.block_number; - let (header, expected_payload) = self.expected_header_and_payload(number)?; + let (correct_header, expected_payload) = self.expected_header_and_payload(number)?; if commitment.payload != expected_payload { // TODO: create/get grandpa proof for block number - let validator_set = self.active_validator_set_at(&header)?; + let validator_set = self.active_validator_set_at(&correct_header)?; if signatures.len() != validator_set.validators().len() { // invalid proof return Ok(()) @@ -258,10 +258,10 @@ where let signatories = validator_set.validators().iter().cloned().zip(signatures.into_iter()) .filter_map(|(id, signature)| signature.map(|sig| (id, sig))).collect(); - let proof = ForkEquivocationProof { commitment, signatories, expected_payload }; + let proof = ForkEquivocationProof { commitment, signatories, correct_header: correct_header.clone() }; self.report_fork_equivocation( proof, - &header, + &correct_header, )?; } Ok(()) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 16517f280ba75..982544488c30d 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -39,10 +39,11 @@ use frame_support::{ log, traits::{Get, KeyOwnerProofSystem}, }; -use frame_system::pallet_prelude::BlockNumberFor; +use frame_system::pallet_prelude::{BlockNumberFor, HeaderFor}; use log::{error, info}; use sp_consensus_beefy::{VoteEquivocationProof, ForkEquivocationProof, ValidatorSetId, KEY_TYPE as BEEFY_KEY_TYPE}; use sp_runtime::{ + traits::Zero, transaction_validity::{ InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity, TransactionValidityError, ValidTransaction, @@ -138,6 +139,7 @@ pub enum EquivocationEvidenceFor { ForkEquivocationProof, ::BeefyId, <::BeefyId as RuntimeAppPublic>::Signature, + HeaderFor, >, Vec<::KeyOwnerProof>, ) @@ -261,6 +263,18 @@ where } }, EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, _) => { + use sp_runtime::traits::Header; + let block_number = equivocation_proof.commitment.block_number; + let correct_header = &equivocation_proof.correct_header; + + // Check that the provided header is correct. + if block_number <= BlockNumberFor::::zero() || + >::block_hash(block_number) != correct_header.hash() + { + // TODO: maybe have a specific error for this + return Err(Error::::InvalidForkEquivocationProof.into()) + } + // Validate equivocation proof (check commitment is to unexpected payload and signatures are valid). if !sp_consensus_beefy::check_fork_equivocation_proof(&equivocation_proof) { return Err(Error::::InvalidForkEquivocationProof.into()) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 58fd93faf05c5..dd0171659f720 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -29,7 +29,7 @@ use frame_support::{ }; use frame_system::{ ensure_none, ensure_signed, - pallet_prelude::{BlockNumberFor, OriginFor}, + pallet_prelude::{BlockNumberFor, OriginFor, HeaderFor}, }; use sp_runtime::{ generic::DigestItem, @@ -292,6 +292,7 @@ pub mod pallet { BlockNumberFor, T::BeefyId, ::Signature, + HeaderFor >, >, key_owner_proofs: Vec, @@ -325,6 +326,7 @@ pub mod pallet { BlockNumberFor, T::BeefyId, ::Signature, + HeaderFor, >, >, key_owner_proofs: Vec, @@ -388,7 +390,7 @@ impl Pallet { /// to the pool. Only useful in an offchain context. pub fn submit_unsigned_fork_equivocation_report( _fork_equivocation_proof: sp_consensus_beefy::ForkEquivocationProof, T::BeefyId, - ::Signature, + ::Signature, HeaderFor >, _key_owner_proofs: Vec, ) -> Option<()> { diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index bf1196adce2f9..2ace78a29cc9d 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -47,7 +47,7 @@ use codec::{Codec, Decode, Encode}; use scale_info::TypeInfo; use sp_application_crypto::RuntimeAppPublic; use sp_core::H256; -use sp_runtime::traits::{Hash, Keccak256, NumberFor}; +use sp_runtime::traits::{Hash, Keccak256, NumberFor, Header}; use sp_std::prelude::*; /// Key type for BEEFY module. @@ -254,7 +254,7 @@ impl VoteEquivocationProof { /// 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 { +pub struct ForkEquivocationProof { /// Commitment for a block on different fork than one at the same height in /// this client's chain. /// TODO: maybe replace {commitment, signatories} with SignedCommitment @@ -264,11 +264,14 @@ pub struct ForkEquivocationProof { /// Signatures on this block /// TODO: maybe change to HashMap - check once usage pattern is clear pub signatories: Vec<(Id, Signature)>, - /// Expected payload - pub expected_payload: Payload, + /// 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, } -impl ForkEquivocationProof { +impl ForkEquivocationProof { /// Returns the authority id of the misbehaving voter. pub fn offender_ids(&self) -> Vec<&Id> { self.signatories.iter().map(|(id, _)| id).collect() @@ -334,9 +337,10 @@ where return valid_first && valid_second } -/// Validates [InvalidForkVoteProof] by checking: -/// 1. `vote` is signed, -/// 2. `vote.commitment.payload` != `expected_payload`. +/// Validates [ForkEquivocationProof] by checking: +/// 1. `commitment` is signed, +/// 2. `correct_header` is valid and matches `commitment.block_number`. +/// 2. `commitment.payload` != `expected_payload(correct_header)`. /// NOTE: 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. @@ -345,19 +349,28 @@ where /// 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( - proof: &ForkEquivocationProof::Signature>, +pub fn check_fork_equivocation_proof( + proof: &ForkEquivocationProof::Signature, Header>, ) -> bool where Id: BeefyAuthorityId + PartialEq, Number: Clone + Encode + PartialEq, MsgHash: Hash, + Header: sp_api::HeaderT, { - let ForkEquivocationProof { commitment, signatories, expected_payload } = proof; - - // check that `payload` on the `vote` is different that the `expected_payload` (checked first - // since cheap failfast). - if &commitment.payload != expected_payload { + let ForkEquivocationProof { commitment, signatories, correct_header } = proof; + + let expected_mmr_root_digest = mmr::find_mmr_root_digest::
(correct_header); + let expected_payload = expected_mmr_root_digest.map(|mmr_root| { + Payload::from_single_entry(known_payloads::MMR_ROOT_ID, mmr_root.encode()) + }); + + // cheap failfasts: + // 1. check that `payload` on the `vote` is different that the `expected_payload` + // 2. if the signatories signed a payload when there should be none (for + // instance for a block prior to BEEFY activation), they should likewise be + // slashed + if Some(&commitment.payload) != expected_payload.as_ref() || expected_mmr_root_digest.is_none() { // 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 @@ -442,7 +455,7 @@ sp_api::decl_runtime_apis! { /// hardcoded to return `None`). Only useful in an offchain context. fn submit_report_fork_equivocation_unsigned_extrinsic( fork_equivocation_proof: - ForkEquivocationProof, AuthorityId, ::Signature>, + ForkEquivocationProof, AuthorityId, ::Signature, Block::Header>, key_owner_proofs: Vec, ) -> Option<()>; From 7c2cdfdc55dab9073a99a08be95144c0a3444811 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 8 Aug 2023 12:22:42 +0200 Subject: [PATCH 23/41] rm superfluous report_fork_equiv.correct_header --- .../consensus/beefy/src/communication/fisherman.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 714195835605a..023d8edcecc0a 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -156,9 +156,8 @@ where fn report_fork_equivocation( &self, proof: ForkEquivocationProof, AuthorityId, Signature, B::Header>, - correct_header: &B::Header, ) -> Result<(), Error> { - let validator_set = self.active_validator_set_at(correct_header)?; + let validator_set = self.active_validator_set_at(&proof.correct_header)?; let set_id = validator_set.id(); if proof.commitment.validator_set_id != set_id || @@ -168,7 +167,7 @@ where return Ok(()) } - let hash = correct_header.hash(); + let hash = proof.correct_header.hash(); let offender_ids = proof.signatories.iter().cloned().map(|(id, _sig)| id).collect::>(); let runtime_api = self.runtime.runtime_api(); @@ -219,7 +218,7 @@ where if vote.commitment.payload != expected_payload { let validator_set = self.active_validator_set_at(&correct_header)?; let proof = ForkEquivocationProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], correct_header: correct_header.clone() }; - self.report_fork_equivocation(proof, &correct_header)?; + self.report_fork_equivocation(proof)?; } Ok(()) } @@ -259,10 +258,7 @@ where .filter_map(|(id, signature)| signature.map(|sig| (id, sig))).collect(); let proof = ForkEquivocationProof { commitment, signatories, correct_header: correct_header.clone() }; - self.report_fork_equivocation( - proof, - &correct_header, - )?; + self.report_fork_equivocation(proof)?; } Ok(()) } From 98eb692ba22ad2363d6726bd21fa5dc32063017b Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 8 Aug 2023 12:53:36 +0200 Subject: [PATCH 24/41] remove duplic. in check_{signed_commitment, proof} 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. --- .../beefy/src/communication/fisherman.rs | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 023d8edcecc0a..deba4876e05ec 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -223,31 +223,15 @@ where Ok(()) } - /// Check `commitment` for contained block against expected payload. + /// Check `signed_commitment` for contained block against expected payload. fn check_signed_commitment( &self, signed_commitment: SignedCommitment, Signature>, ) -> Result<(), Error> { - let number = signed_commitment.commitment.block_number; - let (header, expected_payload) = self.expected_header_and_payload(number)?; - if signed_commitment.commitment.payload != expected_payload { - let validator_set = self.active_validator_set_at(&header)?; - self.report_invalid_payload(signed_commitment, &expected_payload, &header)?; - } - Ok(()) - } - - /// Check `proof` for contained block against expected payload. - /// - /// Note: this fn expects block referenced in `proof` to be finalized. - fn check_proof(&self, proof: BeefyVersionedFinalityProof) -> Result<(), Error> { - let (commitment, signatures) = match proof { - BeefyVersionedFinalityProof::::V1(inner) => (inner.commitment, inner.signatures), - }; + let SignedCommitment {commitment, signatures} = signed_commitment; let number = commitment.block_number; let (correct_header, expected_payload) = self.expected_header_and_payload(number)?; if commitment.payload != expected_payload { - // TODO: create/get grandpa proof for block number let validator_set = self.active_validator_set_at(&correct_header)?; if signatures.len() != validator_set.validators().len() { // invalid proof @@ -262,4 +246,13 @@ where } Ok(()) } + + /// Check `proof` for contained block against expected payload. + fn check_proof(&self, proof: BeefyVersionedFinalityProof) -> Result<(), Error> { + match proof { + BeefyVersionedFinalityProof::::V1(signed_commitment) => { + self.check_signed_commitment(signed_commitment) + } + } + } } From 91ce7777f790a2d07d6be4b371f1cc80b2cc7ab3 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 8 Aug 2023 15:50:37 +0200 Subject: [PATCH 25/41] update outdated comments --- client/consensus/beefy/src/communication/fisherman.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index deba4876e05ec..d92b64f627513 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -36,22 +36,19 @@ use sp_runtime::{ use std::{marker::PhantomData, sync::Arc}; pub(crate) trait BeefyFisherman: Send + Sync { - /// Check `vote` for contained finalized block against expected payload. - /// - /// Note: this fn expects `vote.commitment.block_number` to be finalized. + /// Check `vote` for contained block against expected payload. fn check_vote( &self, vote: VoteMessage, AuthorityId, Signature>, ) -> Result<(), Error>; + /// Check `signed_commitment` for contained block against expected payload. fn check_signed_commitment( &self, signed_commitment: SignedCommitment, Signature>, ) -> Result<(), Error>; - /// Check `proof` for contained finalized block against expected payload. - /// - /// Note: this fn expects block referenced in `proof` to be finalized. + /// Check `proof` for contained block against expected payload. fn check_proof(&self, proof: BeefyVersionedFinalityProof) -> Result<(), Error>; } @@ -207,8 +204,6 @@ where R::Api: BeefyApi, { /// Check `vote` for contained block against expected payload. - /// - /// Note: this fn expects `vote.commitment.block_number` to be finalized. fn check_vote( &self, vote: VoteMessage, AuthorityId, Signature>, From 3c3fc1c64290838baa1957ed73e14339fb391afa Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 8 Aug 2023 23:21:21 +0200 Subject: [PATCH 26/41] remove report_invalid_payload redundant vs report_fork_equivocation --- .../beefy/src/communication/fisherman.rs | 52 ------------------- 1 file changed, 52 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index d92b64f627513..757de88563c0b 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -98,58 +98,6 @@ where .ok_or_else(|| Error::Backend("could not get BEEFY validator set".into())) } - fn report_invalid_payload( - &self, - signed_commitment: SignedCommitment, Signature>, - correct_payload: &Payload, - correct_header: &B::Header, - ) -> Result<(), Error> { - let validator_set = self.active_validator_set_at(correct_header)?; - let set_id = validator_set.id(); - - let proof = ForkEquivocationProof { - commitment: signed_commitment.commitment.clone(), - signatories: vec![], - correct_header: correct_header.clone(), - }; - - if signed_commitment.commitment.validator_set_id != set_id || - signed_commitment.commitment.payload != *correct_payload || - !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher, B::Header>(&proof) - { - debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); - return Ok(()) - } - - let offender_ids = proof.signatories.iter().cloned().map(|(id, _sig)| id).collect::>(); - let runtime_api = self.runtime.runtime_api(); - - // generate key ownership proof at that block - let key_owner_proofs = offender_ids.iter() - .filter_map(|id| { - match runtime_api.generate_key_ownership_proof(correct_header.hash(), set_id, id.clone()) { - Ok(Some(proof)) => Some(Ok(proof)), - Ok(None) => { - debug!( - target: LOG_TARGET, - "🥩 Invalid fork vote offender not part of the authority set." - ); - None - }, - Err(e) => Some(Err(Error::RuntimeApi(e))), - } - }) - .collect::>()?; - - // submit invalid fork vote report at **best** block - let best_block_hash = self.backend.blockchain().info().best_hash; - runtime_api - .submit_report_fork_equivocation_unsigned_extrinsic(best_block_hash, proof, key_owner_proofs) - .map_err(Error::RuntimeApi)?; - - Ok(()) - } - fn report_fork_equivocation( &self, proof: ForkEquivocationProof, AuthorityId, Signature, B::Header>, From f2a97452587fbc5ee5c8d8e7b805047b01a4c065 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 9 Aug 2023 00:37:09 +0200 Subject: [PATCH 27/41] .+report.+{""->_vote}_equivocations --- client/consensus/beefy/src/tests.rs | 24 ++++++++++++-------- client/consensus/beefy/src/worker.rs | 18 +++++++-------- primitives/consensus/beefy/src/test_utils.rs | 4 ++-- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 3fdf28130c77d..9e5974830cbe6 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -270,7 +270,9 @@ pub(crate) struct TestApi { pub beefy_genesis: u64, pub validator_set: BeefyValidatorSet, pub mmr_root_hash: MmrRootHash, - pub reported_equivocations: + pub reported_vote_equivocations: + Option, AuthorityId, Signature>>>>>, + pub reported_fork_equivocations: Option, AuthorityId, Signature>>>>>, } @@ -284,7 +286,8 @@ impl TestApi { beefy_genesis, validator_set: validator_set.clone(), mmr_root_hash, - reported_equivocations: None, + reported_vote_equivocations: None, + reported_fork_equivocations: None, } } @@ -293,12 +296,13 @@ impl TestApi { beefy_genesis: 1, validator_set: validator_set.clone(), mmr_root_hash: GOOD_MMR_ROOT, - reported_equivocations: None, + reported_vote_equivocations: None, + reported_fork_equivocations: None, } } pub fn allow_equivocations(&mut self) { - self.reported_equivocations = Some(Arc::new(Mutex::new(vec![]))); + self.reported_vote_equivocations = Some(Arc::new(Mutex::new(vec![]))); } } @@ -328,7 +332,7 @@ sp_api::mock_impl_runtime_apis! { proof: VoteEquivocationProof, AuthorityId, Signature>, _dummy: OpaqueKeyOwnershipProof, ) -> Option<()> { - if let Some(equivocations_buf) = self.inner.reported_equivocations.as_ref() { + if let Some(equivocations_buf) = self.inner.reported_vote_equivocations.as_ref() { equivocations_buf.lock().push(proof); None } else { @@ -1245,7 +1249,7 @@ async fn beefy_finalizing_after_pallet_genesis() { } #[tokio::test] -async fn beefy_reports_equivocations() { +async fn beefy_reports_vote_equivocations() { sp_tracing::try_init_simple(); let peers = [BeefyKeyring::Alice, BeefyKeyring::Bob, BeefyKeyring::Charlie]; @@ -1295,21 +1299,21 @@ async fn beefy_reports_equivocations() { // run for up to 5 seconds waiting for Alice's report of Bob/Bob_Prime equivocation. for wait_ms in [250, 500, 1250, 3000] { run_for(Duration::from_millis(wait_ms), &net).await; - if !api_alice.reported_equivocations.as_ref().unwrap().lock().is_empty() { + if !api_alice.reported_vote_equivocations.as_ref().unwrap().lock().is_empty() { break } } // Verify expected equivocation - let alice_reported_equivocations = api_alice.reported_equivocations.as_ref().unwrap().lock(); + let alice_reported_equivocations = api_alice.reported_vote_equivocations.as_ref().unwrap().lock(); assert_eq!(alice_reported_equivocations.len(), 1); let equivocation_proof = alice_reported_equivocations.get(0).unwrap(); assert_eq!(equivocation_proof.first.id, BeefyKeyring::Bob.public()); assert_eq!(equivocation_proof.first.commitment.block_number, 1); // Verify neither Bob or Bob_Prime report themselves as equivocating. - assert!(api_bob.reported_equivocations.as_ref().unwrap().lock().is_empty()); - assert!(api_bob_prime.reported_equivocations.as_ref().unwrap().lock().is_empty()); + assert!(api_bob.reported_vote_equivocations.as_ref().unwrap().lock().is_empty()); + assert!(api_bob_prime.reported_vote_equivocations.as_ref().unwrap().lock().is_empty()); // sanity verify no new blocks have been finalized by BEEFY streams_empty_after_timeout(best_blocks, &net, None).await; diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index a29f0cf5e966f..6cbc3babb99fc 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -1061,7 +1061,7 @@ pub(crate) mod tests { use sp_api::HeaderT; use sp_blockchain::Backend as BlockchainBackendT; use sp_consensus_beefy::{ - generate_equivocation_proof, known_payloads, known_payloads::MMR_ROOT_ID, + generate_vote_equivocation_proof, known_payloads, known_payloads::MMR_ROOT_ID, mmr::MmrRootProvider, Keyring, Payload, SignedCommitment, }; use sp_runtime::traits::One; @@ -1609,7 +1609,7 @@ pub(crate) mod tests { } #[tokio::test] - async fn should_not_report_bad_old_or_self_equivocations() { + async fn should_not_report_bad_old_or_self_vote_equivocations() { let block_num = 1; let set_id = 1; let keys = [Keyring::Alice]; @@ -1630,7 +1630,7 @@ pub(crate) mod tests { let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); // generate an equivocation proof, with Bob as perpetrator - let good_proof = generate_equivocation_proof( + let good_proof = generate_vote_equivocation_proof( (block_num, payload1.clone(), set_id, &Keyring::Bob), (block_num, payload2.clone(), set_id, &Keyring::Bob), ); @@ -1638,11 +1638,11 @@ pub(crate) mod tests { // expect voter (Alice) to successfully report it assert_eq!(worker.report_equivocation(good_proof.clone()), Ok(())); // verify Alice reports Bob equivocation to runtime - let reported = api_alice.reported_equivocations.as_ref().unwrap().lock(); + let reported = api_alice.reported_vote_equivocations.as_ref().unwrap().lock(); assert_eq!(reported.len(), 1); assert_eq!(*reported.get(0).unwrap(), good_proof); } - api_alice.reported_equivocations.as_ref().unwrap().lock().clear(); + api_alice.reported_vote_equivocations.as_ref().unwrap().lock().clear(); // now let's try with a bad proof let mut bad_proof = good_proof.clone(); @@ -1650,7 +1650,7 @@ pub(crate) mod tests { // bad proofs are simply ignored assert_eq!(worker.report_equivocation(bad_proof), Ok(())); // verify nothing reported to runtime - assert!(api_alice.reported_equivocations.as_ref().unwrap().lock().is_empty()); + assert!(api_alice.reported_vote_equivocations.as_ref().unwrap().lock().is_empty()); // now let's try with old set it let mut old_proof = good_proof.clone(); @@ -1659,16 +1659,16 @@ pub(crate) mod tests { // old proofs are simply ignored assert_eq!(worker.report_equivocation(old_proof), Ok(())); // verify nothing reported to runtime - assert!(api_alice.reported_equivocations.as_ref().unwrap().lock().is_empty()); + assert!(api_alice.reported_vote_equivocations.as_ref().unwrap().lock().is_empty()); // now let's try reporting a self-equivocation - let self_proof = generate_equivocation_proof( + let self_proof = generate_vote_equivocation_proof( (block_num, payload1.clone(), set_id, &Keyring::Alice), (block_num, payload2.clone(), set_id, &Keyring::Alice), ); // equivocations done by 'self' are simply ignored (not reported) assert_eq!(worker.report_equivocation(self_proof), Ok(())); // verify nothing reported to runtime - assert!(api_alice.reported_equivocations.as_ref().unwrap().lock().is_empty()); + assert!(api_alice.reported_vote_equivocations.as_ref().unwrap().lock().is_empty()); } } diff --git a/primitives/consensus/beefy/src/test_utils.rs b/primitives/consensus/beefy/src/test_utils.rs index 12190a701ab2e..36fe05f3aa31b 100644 --- a/primitives/consensus/beefy/src/test_utils.rs +++ b/primitives/consensus/beefy/src/test_utils.rs @@ -91,8 +91,8 @@ impl From for ecdsa_crypto::Public { } } -/// Create a new `EquivocationProof` based on given arguments. -pub fn generate_equivocation_proof( +/// Create a new `VoteEquivocationProof` based on given arguments. +pub fn generate_vote_equivocation_proof( vote1: (u64, Payload, ValidatorSetId, &Keyring), vote2: (u64, Payload, ValidatorSetId, &Keyring), ) -> VoteEquivocationProof { From faea8d9ad67c26804afa9eeb7b77ada5d6b20a83 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 9 Aug 2023 11:05:09 +0200 Subject: [PATCH 28/41] move correct_header hash check into primitives --- .../consensus/beefy/src/communication/fisherman.rs | 9 +++++++-- frame/beefy/src/equivocation.rs | 14 ++------------ primitives/consensus/beefy/src/lib.rs | 5 +++++ 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 757de88563c0b..36c4f4cd64826 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -105,8 +105,14 @@ where let validator_set = self.active_validator_set_at(&proof.correct_header)?; let set_id = validator_set.id(); + let expected_header_hash = self + .backend + .blockchain() + .expect_block_hash_from_id(&BlockId::Number(proof.commitment.block_number)) + .map_err(|e| Error::Backend(e.to_string()))?; + if proof.commitment.validator_set_id != set_id || - !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher, B::Header>(&proof) + !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher, B::Header>(&proof, &expected_header_hash) { debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); return Ok(()) @@ -159,7 +165,6 @@ where let number = vote.commitment.block_number; let (correct_header, expected_payload) = self.expected_header_and_payload(number)?; if vote.commitment.payload != expected_payload { - let validator_set = self.active_validator_set_at(&correct_header)?; let proof = ForkEquivocationProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], correct_header: correct_header.clone() }; self.report_fork_equivocation(proof)?; } diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 982544488c30d..f91976e8c6201 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -43,7 +43,6 @@ use frame_system::pallet_prelude::{BlockNumberFor, HeaderFor}; use log::{error, info}; use sp_consensus_beefy::{VoteEquivocationProof, ForkEquivocationProof, ValidatorSetId, KEY_TYPE as BEEFY_KEY_TYPE}; use sp_runtime::{ - traits::Zero, transaction_validity::{ InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity, TransactionValidityError, ValidTransaction, @@ -263,20 +262,11 @@ where } }, EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, _) => { - use sp_runtime::traits::Header; let block_number = equivocation_proof.commitment.block_number; - let correct_header = &equivocation_proof.correct_header; - - // Check that the provided header is correct. - if block_number <= BlockNumberFor::::zero() || - >::block_hash(block_number) != correct_header.hash() - { - // TODO: maybe have a specific error for this - return Err(Error::::InvalidForkEquivocationProof.into()) - } + let expected_block_hash = >::block_hash(block_number); // Validate equivocation proof (check commitment is to unexpected payload and signatures are valid). - if !sp_consensus_beefy::check_fork_equivocation_proof(&equivocation_proof) { + if !sp_consensus_beefy::check_fork_equivocation_proof(&equivocation_proof, &expected_block_hash) { return Err(Error::::InvalidForkEquivocationProof.into()) } }, diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index 2ace78a29cc9d..ab14837d31917 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -351,6 +351,7 @@ where /// finalized by GRANDPA. pub fn check_fork_equivocation_proof( proof: &ForkEquivocationProof::Signature, Header>, + expected_header_hash: &Header::Hash, ) -> bool where Id: BeefyAuthorityId + PartialEq, @@ -360,6 +361,10 @@ where { let ForkEquivocationProof { commitment, signatories, correct_header } = proof; + if correct_header.hash() != *expected_header_hash { + return false + } + let expected_mmr_root_digest = mmr::find_mmr_root_digest::
(correct_header); let expected_payload = expected_mmr_root_digest.map(|mmr_root| { Payload::from_single_entry(known_payloads::MMR_ROOT_ID, mmr_root.encode()) From c3df6313c7a8fc78b0d6932409c558245ae96051 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 9 Aug 2023 20:18:12 +0200 Subject: [PATCH 29/41] create_beefy_worker: only push block if at genesis 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. --- client/consensus/beefy/src/worker.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index 6cbc3babb99fc..02759f8600070 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -1157,9 +1157,11 @@ pub(crate) mod tests { known_peers, None, ); - // Push 1 block - will start first session. - let hashes = peer.push_blocks(1, false); - backend.finalize_block(hashes[0], None).unwrap(); + // If chain's still at genesis, push 1 block to start first session. + if backend.blockchain().info().best_hash == backend.blockchain().info().genesis_hash { + let hashes = peer.push_blocks(1, false); + backend.finalize_block(hashes[0], None).unwrap(); + } let first_header = backend .blockchain() .expect_header(backend.blockchain().info().best_hash) From f1f2bb01a4c41fc997d64f31d86a133a6797f0d4 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 9 Aug 2023 20:33:41 +0200 Subject: [PATCH 30/41] create_beefy_worker: opt. instantiate with TestApi 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). --- client/consensus/beefy/src/worker.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index 02759f8600070..c6d70098696b0 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -1100,6 +1100,7 @@ pub(crate) mod tests { key: &Keyring, min_block_delta: u32, genesis_validator_set: ValidatorSet, + runtime_api: Option>, ) -> BeefyWorker< Block, Backend, @@ -1129,7 +1130,7 @@ pub(crate) mod tests { let backend = peer.client().as_backend(); let beefy_genesis = 1; - let api = Arc::new(TestApi::with_validator_set(&genesis_validator_set)); + let api = runtime_api.unwrap_or_else(|| Arc::new(TestApi::with_validator_set(&genesis_validator_set))); let network = peer.network_service().clone(); let sync = peer.sync_service().clone(); let payload_provider = MmrRootProvider::new(api.clone()); @@ -1456,7 +1457,7 @@ pub(crate) mod tests { let keys = &[Keyring::Alice]; let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap(); let mut net = BeefyTestNet::new(1); - let mut worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone()); + let mut worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone(), None); // keystore doesn't contain other keys than validators' assert_eq!(worker.verify_validator_set(&1, &validator_set), Ok(())); @@ -1480,7 +1481,7 @@ pub(crate) mod tests { let validator_set = ValidatorSet::new(make_beefy_ids(&keys), 0).unwrap(); let mut net = BeefyTestNet::new(1); let backend = net.peer(0).client().as_backend(); - let mut worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone()); + let mut worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone(), None); // remove default session, will manually add custom one. worker.persisted_state.voting_oracle.sessions.clear(); @@ -1584,7 +1585,7 @@ pub(crate) mod tests { let keys = &[Keyring::Alice, Keyring::Bob]; let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap(); let mut net = BeefyTestNet::new(1); - let mut worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone()); + let mut worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone(), None); let worker_rounds = worker.active_rounds().unwrap(); assert_eq!(worker_rounds.session_start(), 1); @@ -1622,8 +1623,7 @@ pub(crate) mod tests { let api_alice = Arc::new(api_alice); let mut net = BeefyTestNet::new(1); - let mut worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone()); - worker.runtime = api_alice.clone(); + let worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone(), Some(api_alice.clone())); // let there be a block with num = 1: let _ = net.peer(0).push_blocks(1, false); From b0ed4ae828583d381cf745922c4a5d3413e65f29 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 9 Aug 2023 20:43:27 +0200 Subject: [PATCH 31/41] push to reported_fork_equivocations mock api's `submit_report_fork_equivocation_unsigned_extrinsic` pushes reported equivocations to runtime_api.reported_fork_equivocations --- client/consensus/beefy/src/tests.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 9e5974830cbe6..0fbf1e53da210 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -273,7 +273,7 @@ pub(crate) struct TestApi { pub reported_vote_equivocations: Option, AuthorityId, Signature>>>>>, pub reported_fork_equivocations: - Option, AuthorityId, Signature>>>>>, + Option, AuthorityId, Signature, Header>>>>>, } impl TestApi { @@ -303,6 +303,7 @@ impl TestApi { pub fn allow_equivocations(&mut self) { self.reported_vote_equivocations = Some(Arc::new(Mutex::new(vec![]))); + self.reported_fork_equivocations = Some(Arc::new(Mutex::new(vec![]))); } } @@ -340,6 +341,18 @@ sp_api::mock_impl_runtime_apis! { } } + fn submit_report_fork_equivocation_unsigned_extrinsic( + proof: ForkEquivocationProof, AuthorityId, Signature, Header>, + _dummy: Vec, + ) -> Option<()> { + if let Some(equivocations_buf) = self.inner.reported_fork_equivocations.as_ref() { + equivocations_buf.lock().push(proof); + None + } else { + panic!("Equivocations not expected, but following proof was reported: {:?}", proof); + } + } + fn generate_key_ownership_proof( _dummy1: ValidatorSetId, _dummy2: AuthorityId, From 113076cf50260885d0e2fb656706b06e4300d3b5 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 10 Aug 2023 00:11:13 +0200 Subject: [PATCH 32/41] add generate_fork_equivocation_proof_vote to tests Generates fork equivocation proofs from a vote and a header. --- client/consensus/beefy/src/tests.rs | 8 ++--- frame/beefy/src/tests.rs | 36 ++++++++++---------- primitives/consensus/beefy/src/test_utils.rs | 26 +++++++++----- 3 files changed, 39 insertions(+), 31 deletions(-) diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 0fbf1e53da210..65b3dc652826a 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -56,7 +56,7 @@ use sp_consensus_beefy::{ ecdsa_crypto::{AuthorityId, Signature}, known_payloads, mmr::{find_mmr_root_digest, MmrRootProvider}, - BeefyApi, Commitment, ConsensusLog, VoteEquivocationProof, Keyring as BeefyKeyring, MmrRootHash, + BeefyApi, Commitment, ConsensusLog, ForkEquivocationProof, VoteEquivocationProof, Keyring as BeefyKeyring, MmrRootHash, OpaqueKeyOwnershipProof, Payload, SignedCommitment, ValidatorSet, ValidatorSetId, VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID, }; @@ -1318,9 +1318,9 @@ async fn beefy_reports_vote_equivocations() { } // Verify expected equivocation - let alice_reported_equivocations = api_alice.reported_vote_equivocations.as_ref().unwrap().lock(); - assert_eq!(alice_reported_equivocations.len(), 1); - let equivocation_proof = alice_reported_equivocations.get(0).unwrap(); + let alice_reported_vote_equivocations = api_alice.reported_vote_equivocations.as_ref().unwrap().lock(); + assert_eq!(alice_reported_vote_equivocations.len(), 1); + let equivocation_proof = alice_reported_vote_equivocations.get(0).unwrap(); assert_eq!(equivocation_proof.first.id, BeefyKeyring::Bob.public()); assert_eq!(equivocation_proof.first.commitment.block_number, 1); diff --git a/frame/beefy/src/tests.rs b/frame/beefy/src/tests.rs index 45a7080980763..fd99343c77460 100644 --- a/frame/beefy/src/tests.rs +++ b/frame/beefy/src/tests.rs @@ -19,7 +19,7 @@ use std::vec; use codec::Encode; use sp_consensus_beefy::{ - check_vote_equivocation_proof, generate_equivocation_proof, known_payloads::MMR_ROOT_ID, + check_vote_equivocation_proof, generate_vote_equivocation_proof, known_payloads::MMR_ROOT_ID, Keyring as BeefyKeyring, Payload, ValidatorSet, KEY_TYPE as BEEFY_KEY_TYPE, }; @@ -212,7 +212,7 @@ fn should_sign_and_verify() { // generate an equivocation proof, with two votes in the same round for // same payload signed by the same key - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (1, payload1.clone(), set_id, &BeefyKeyring::Bob), (1, payload1.clone(), set_id, &BeefyKeyring::Bob), ); @@ -221,7 +221,7 @@ fn should_sign_and_verify() { // generate an equivocation proof, with two votes in different rounds for // different payloads signed by the same key - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (1, payload1.clone(), set_id, &BeefyKeyring::Bob), (2, payload2.clone(), set_id, &BeefyKeyring::Bob), ); @@ -229,7 +229,7 @@ fn should_sign_and_verify() { assert!(!check_vote_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); // generate an equivocation proof, with two votes by different authorities - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (1, payload1.clone(), set_id, &BeefyKeyring::Alice), (1, payload2.clone(), set_id, &BeefyKeyring::Bob), ); @@ -237,7 +237,7 @@ fn should_sign_and_verify() { assert!(!check_vote_equivocation_proof::<_, _, Keccak256>(&equivocation_proof)); // generate an equivocation proof, with two votes in different set ids - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (1, payload1.clone(), set_id, &BeefyKeyring::Bob), (1, payload2.clone(), set_id + 1, &BeefyKeyring::Bob), ); @@ -247,7 +247,7 @@ fn should_sign_and_verify() { // generate an equivocation proof, with two votes in the same round for // different payloads signed by the same key let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (1, payload1, set_id, &BeefyKeyring::Bob), (1, payload2, set_id, &BeefyKeyring::Bob), ); @@ -291,7 +291,7 @@ fn report_equivocation_current_set_works() { let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); // generate an equivocation proof, with two votes in the same round for // different payloads signed by the same key - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (block_num, payload1, set_id, &equivocation_keyring), (block_num, payload2, set_id, &equivocation_keyring), ); @@ -377,7 +377,7 @@ fn report_equivocation_old_set_works() { let payload1 = Payload::from_single_entry(MMR_ROOT_ID, vec![42]); let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); // generate an equivocation proof for the old set, - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (block_num, payload1, old_set_id, &equivocation_keyring), (block_num, payload2, old_set_id, &equivocation_keyring), ); @@ -439,7 +439,7 @@ fn report_equivocation_invalid_set_id() { let payload1 = Payload::from_single_entry(MMR_ROOT_ID, vec![42]); let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); // generate an equivocation for a future set - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (block_num, payload1, set_id + 1, &equivocation_keyring), (block_num, payload2, set_id + 1, &equivocation_keyring), ); @@ -481,7 +481,7 @@ fn report_equivocation_invalid_session() { let payload1 = Payload::from_single_entry(MMR_ROOT_ID, vec![42]); let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); // generate an equivocation proof at following era set id = 2 - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (block_num, payload1, set_id, &equivocation_keyring), (block_num, payload2, set_id, &equivocation_keyring), ); @@ -525,7 +525,7 @@ fn report_equivocation_invalid_key_owner_proof() { let payload1 = Payload::from_single_entry(MMR_ROOT_ID, vec![42]); let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); // generate an equivocation proof for the authority at index 0 - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (block_num, payload1, set_id + 1, &equivocation_keyring), (block_num, payload2, set_id + 1, &equivocation_keyring), ); @@ -584,31 +584,31 @@ fn report_equivocation_invalid_equivocation_proof() { // both votes target the same block number and payload, // there is no equivocation. - assert_invalid_equivocation_proof(generate_equivocation_proof( + assert_invalid_equivocation_proof(generate_vote_equivocation_proof( (block_num, payload1.clone(), set_id, &equivocation_keyring), (block_num, payload1.clone(), set_id, &equivocation_keyring), )); // votes targeting different rounds, there is no equivocation. - assert_invalid_equivocation_proof(generate_equivocation_proof( + assert_invalid_equivocation_proof(generate_vote_equivocation_proof( (block_num, payload1.clone(), set_id, &equivocation_keyring), (block_num + 1, payload2.clone(), set_id, &equivocation_keyring), )); // votes signed with different authority keys - assert_invalid_equivocation_proof(generate_equivocation_proof( + assert_invalid_equivocation_proof(generate_vote_equivocation_proof( (block_num, payload1.clone(), set_id, &equivocation_keyring), (block_num, payload1.clone(), set_id, &BeefyKeyring::Charlie), )); // votes signed with a key that isn't part of the authority set - assert_invalid_equivocation_proof(generate_equivocation_proof( + assert_invalid_equivocation_proof(generate_vote_equivocation_proof( (block_num, payload1.clone(), set_id, &equivocation_keyring), (block_num, payload1.clone(), set_id, &BeefyKeyring::Dave), )); // votes targeting different set ids - assert_invalid_equivocation_proof(generate_equivocation_proof( + assert_invalid_equivocation_proof(generate_vote_equivocation_proof( (block_num, payload1, set_id, &equivocation_keyring), (block_num, payload2, set_id + 1, &equivocation_keyring), )); @@ -639,7 +639,7 @@ fn report_equivocation_validate_unsigned_prevents_duplicates() { let payload1 = Payload::from_single_entry(MMR_ROOT_ID, vec![42]); let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (block_num, payload1, set_id, &equivocation_keyring), (block_num, payload2, set_id, &equivocation_keyring), ); @@ -743,7 +743,7 @@ fn valid_equivocation_reports_dont_pay_fees() { // generate equivocation proof let payload1 = Payload::from_single_entry(MMR_ROOT_ID, vec![42]); let payload2 = Payload::from_single_entry(MMR_ROOT_ID, vec![128]); - let equivocation_proof = generate_equivocation_proof( + let equivocation_proof = generate_vote_equivocation_proof( (block_num, payload1, set_id, &equivocation_keyring), (block_num, payload2, set_id, &equivocation_keyring), ); diff --git a/primitives/consensus/beefy/src/test_utils.rs b/primitives/consensus/beefy/src/test_utils.rs index 36fe05f3aa31b..a680d997dc9b3 100644 --- a/primitives/consensus/beefy/src/test_utils.rs +++ b/primitives/consensus/beefy/src/test_utils.rs @@ -17,7 +17,7 @@ #![cfg(feature = "std")] -use crate::{ecdsa_crypto, Commitment, VoteEquivocationProof, Payload, ValidatorSetId, VoteMessage}; +use crate::{ecdsa_crypto, Commitment, ForkEquivocationProof, VoteEquivocationProof, Payload, ValidatorSetId, VoteMessage}; use codec::Encode; use sp_core::{ecdsa, keccak_256, Pair}; use std::collections::HashMap; @@ -91,20 +91,28 @@ impl From for ecdsa_crypto::Public { } } +fn signed_vote(block_number: u64, payload: Payload, validator_set_id: ValidatorSetId, keyring: &Keyring) -> VoteMessage { + let commitment = Commitment { validator_set_id, block_number, payload }; + let signature = keyring.sign(&commitment.encode()); + VoteMessage { commitment, id: keyring.public(), signature } +} + /// Create a new `VoteEquivocationProof` based on given arguments. pub fn generate_vote_equivocation_proof( vote1: (u64, Payload, ValidatorSetId, &Keyring), vote2: (u64, Payload, ValidatorSetId, &Keyring), ) -> VoteEquivocationProof { - let signed_vote = |block_number: u64, - payload: Payload, - validator_set_id: ValidatorSetId, - keyring: &Keyring| { - let commitment = Commitment { validator_set_id, block_number, payload }; - let signature = keyring.sign(&commitment.encode()); - VoteMessage { commitment, id: keyring.public(), signature } - }; let first = signed_vote(vote1.0, vote1.1, vote1.2, vote1.3); let second = signed_vote(vote2.0, vote2.1, vote2.2, vote2.3); VoteEquivocationProof { first, second } } + +/// Create a new `ForkEquivocationProof` based on given arguments. +pub fn generate_fork_equivocation_proof_vote
( + vote: (u64, Payload, ValidatorSetId, &Keyring), + correct_header: Header, +) -> ForkEquivocationProof { + let signed_vote = signed_vote(vote.0, vote.1, vote.2, vote.3); + let signatories = vec![(signed_vote.id, signed_vote.signature)]; + ForkEquivocationProof { commitment: signed_vote.commitment, signatories, correct_header } +} From 772e3b8594984b8978bcb55b40f4ae199ceb9214 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 10 Aug 2023 00:15:40 +0200 Subject: [PATCH 33/41] test: Alice snitches on Bob's vote equivocation i.e. a fork equivocation triggered by a vote --- .../beefy/src/communication/fisherman.rs | 2 +- .../beefy/src/communication/gossip.rs | 2 +- client/consensus/beefy/src/worker.rs | 47 ++++++++++++++++--- 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 36c4f4cd64826..ac73323582e8a 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -98,7 +98,7 @@ where .ok_or_else(|| Error::Backend("could not get BEEFY validator set".into())) } - fn report_fork_equivocation( + pub(crate) fn report_fork_equivocation( &self, proof: ForkEquivocationProof, AuthorityId, Signature, B::Header>, ) -> Result<(), Error> { diff --git a/client/consensus/beefy/src/communication/gossip.rs b/client/consensus/beefy/src/communication/gossip.rs index 883f4e48f48ec..64da631362132 100644 --- a/client/consensus/beefy/src/communication/gossip.rs +++ b/client/consensus/beefy/src/communication/gossip.rs @@ -237,7 +237,7 @@ pub(crate) struct GossipValidator { next_rebroadcast: Mutex, known_peers: Arc>>, report_sender: TracingUnboundedSender, - fisherman: F, + pub(crate) fisherman: F, } impl GossipValidator diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index c6d70098696b0..f42f6b566c649 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -579,7 +579,7 @@ where }, VoteImportResult::Equivocation(proof) => { metric_inc!(self, beefy_equivocation_votes); - self.report_equivocation(proof)?; + self.report_vote_equivocation(proof)?; }, VoteImportResult::Invalid => metric_inc!(self, beefy_invalid_votes), VoteImportResult::Stale => metric_inc!(self, beefy_stale_votes), @@ -932,7 +932,7 @@ where /// extrinsic to report the equivocation. In particular, the session membership /// proof must be generated at the block at which the given set was active which /// isn't necessarily the best block if there are pending authority set changes. - pub(crate) fn report_equivocation( + pub(crate) fn report_vote_equivocation( &self, proof: VoteEquivocationProof, AuthorityId, Signature>, ) -> Result<(), Error> { @@ -1061,7 +1061,7 @@ pub(crate) mod tests { use sp_api::HeaderT; use sp_blockchain::Backend as BlockchainBackendT; use sp_consensus_beefy::{ - generate_vote_equivocation_proof, known_payloads, known_payloads::MMR_ROOT_ID, + generate_vote_equivocation_proof, generate_fork_equivocation_proof_vote, known_payloads, known_payloads::MMR_ROOT_ID, mmr::MmrRootProvider, Keyring, Payload, SignedCommitment, }; use sp_runtime::traits::One; @@ -1638,7 +1638,7 @@ pub(crate) mod tests { ); { // expect voter (Alice) to successfully report it - assert_eq!(worker.report_equivocation(good_proof.clone()), Ok(())); + assert_eq!(worker.report_vote_equivocation(good_proof.clone()), Ok(())); // verify Alice reports Bob equivocation to runtime let reported = api_alice.reported_vote_equivocations.as_ref().unwrap().lock(); assert_eq!(reported.len(), 1); @@ -1650,7 +1650,7 @@ pub(crate) mod tests { let mut bad_proof = good_proof.clone(); bad_proof.first.id = Keyring::Charlie.public(); // bad proofs are simply ignored - assert_eq!(worker.report_equivocation(bad_proof), Ok(())); + assert_eq!(worker.report_vote_equivocation(bad_proof), Ok(())); // verify nothing reported to runtime assert!(api_alice.reported_vote_equivocations.as_ref().unwrap().lock().is_empty()); @@ -1659,7 +1659,7 @@ pub(crate) mod tests { old_proof.first.commitment.validator_set_id = 0; old_proof.second.commitment.validator_set_id = 0; // old proofs are simply ignored - assert_eq!(worker.report_equivocation(old_proof), Ok(())); + assert_eq!(worker.report_vote_equivocation(old_proof), Ok(())); // verify nothing reported to runtime assert!(api_alice.reported_vote_equivocations.as_ref().unwrap().lock().is_empty()); @@ -1669,8 +1669,41 @@ pub(crate) mod tests { (block_num, payload2.clone(), set_id, &Keyring::Alice), ); // equivocations done by 'self' are simply ignored (not reported) - assert_eq!(worker.report_equivocation(self_proof), Ok(())); + assert_eq!(worker.report_vote_equivocation(self_proof), Ok(())); // verify nothing reported to runtime assert!(api_alice.reported_vote_equivocations.as_ref().unwrap().lock().is_empty()); } + + #[tokio::test] + async fn should_report_valid_fork_equivocations() { + let peers = [Keyring::Alice, Keyring::Bob]; + let validator_set = ValidatorSet::new(make_beefy_ids(&peers), 0).unwrap(); + let mut api_alice = TestApi::with_validator_set(&validator_set); + api_alice.allow_equivocations(); + let api_alice = Arc::new(api_alice); + + // instantiate network with Alice and Bob running full voters. + let mut net = BeefyTestNet::new(2); + + let session_len = 10; + let hashes = net.generate_blocks_and_sync(50, session_len, &validator_set, true).await; + let alice_worker = create_beefy_worker(net.peer(0), &peers[0], 1, validator_set.clone(), Some(api_alice)); + + let block_number = 1; + let header = net.peer(1).client().as_backend().blockchain().header(hashes[block_number as usize]).unwrap().unwrap(); + let payload = Payload::from_single_entry(MMR_ROOT_ID, "amievil".encode()); + + let validator_set_id = 0; + + let proof = generate_fork_equivocation_proof_vote((block_number as u64, payload, validator_set_id, &Keyring::Bob), header); + { + // expect fisher (Alice) to successfully report it + assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); + // verify Alice reports Bob's equivocation to runtime + let reported = alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); + assert_eq!(reported.len(), 1); + assert_eq!(*reported.get(0).unwrap(), proof); + } + + } } From 7d21f52237e20e318da38a0fab03ebc15ccdebec Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 10 Aug 2023 10:22:39 +0200 Subject: [PATCH 34/41] store reference to key_store in fisherman required for fisherman to *not* report own equivocations - see next commit. --- client/consensus/beefy/src/communication/fisherman.rs | 3 ++- client/consensus/beefy/src/lib.rs | 6 +++++- client/consensus/beefy/src/worker.rs | 9 +++++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index ac73323582e8a..1bd099cf29316 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -17,7 +17,7 @@ // along with this program. If not, see . use crate::{ - error::Error, justification::BeefyVersionedFinalityProof, keystore::BeefySignatureHasher, + error::Error, justification::BeefyVersionedFinalityProof, keystore::{BeefyKeystore, BeefySignatureHasher}, LOG_TARGET, }; use log::debug; @@ -57,6 +57,7 @@ pub(crate) trait BeefyFisherman: Send + Sync { pub(crate) struct Fisherman { pub backend: Arc, pub runtime: Arc, + pub key_store: Arc, pub payload_provider: P, pub _phantom: PhantomData, } diff --git a/client/consensus/beefy/src/lib.rs b/client/consensus/beefy/src/lib.rs index ba55a5b46a6e2..20093187e333a 100644 --- a/client/consensus/beefy/src/lib.rs +++ b/client/consensus/beefy/src/lib.rs @@ -32,6 +32,7 @@ use crate::{ metrics::register_metrics, round::Rounds, worker::PersistedState, + keystore::BeefyKeystore, }; use futures::{stream::Fuse, StreamExt}; use log::{error, info}; @@ -243,6 +244,8 @@ pub async fn start_beefy_gadget( on_demand_justifications_handler, } = beefy_params; + let key_store: Arc = Arc::new(key_store.into()); + let BeefyNetworkParams { network, sync, @@ -255,6 +258,7 @@ pub async fn start_beefy_gadget( let fisherman = Fisherman { backend: backend.clone(), runtime: runtime.clone(), + key_store: key_store.clone(), payload_provider: payload_provider.clone(), _phantom: PhantomData, }; @@ -318,7 +322,7 @@ pub async fn start_beefy_gadget( payload_provider, runtime, sync, - key_store: key_store.into(), + key_store, gossip_engine, gossip_validator, gossip_report_stream, diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index f42f6b566c649..6e65dd3530c17 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -321,7 +321,7 @@ pub(crate) struct BeefyWorker { pub payload_provider: P, pub runtime: Arc, pub sync: Arc, - pub key_store: BeefyKeystore, + pub key_store: Arc, // communication pub gossip_engine: GossipEngine, @@ -1109,7 +1109,7 @@ pub(crate) mod tests { Arc>, Fisherman>, > { - let keystore = create_beefy_keystore(*key); + let key_store: Arc = Arc::new(Some(create_beefy_keystore(*key)).into()); let (to_rpc_justif_sender, from_voter_justif_stream) = BeefyVersionedFinalityProofStream::::channel(); @@ -1137,6 +1137,7 @@ pub(crate) mod tests { let fisherman = Fisherman { backend: backend.clone(), runtime: api.clone(), + key_store: key_store.clone(), payload_provider: payload_provider.clone(), _phantom: PhantomData, }; @@ -1179,7 +1180,7 @@ pub(crate) mod tests { backend, payload_provider, runtime: api, - key_store: Some(keystore).into(), + key_store, links, gossip_engine, gossip_validator, @@ -1470,7 +1471,7 @@ pub(crate) mod tests { assert_eq!(worker.verify_validator_set(&1, &validator_set), expected); // worker has no keystore - worker.key_store = None.into(); + worker.key_store = Arc::new(None.into()); let expected_err = Err(Error::Keystore("no Keystore".into())); assert_eq!(worker.verify_validator_set(&1, &validator_set), expected_err); } From 6646665ae2a1775e15f447443495ad89b8b20d74 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 10 Aug 2023 10:24:09 +0200 Subject: [PATCH 35/41] test: Alice doesn't snitch *own* vote equivocation i.e. a fork equivocation triggered by a vote --- .../consensus/beefy/src/communication/fisherman.rs | 10 +++++++++- client/consensus/beefy/src/worker.rs | 14 ++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 1bd099cf29316..2a45a749737af 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -119,8 +119,16 @@ where return Ok(()) } - let hash = proof.correct_header.hash(); let offender_ids = proof.signatories.iter().cloned().map(|(id, _sig)| id).collect::>(); + if let Some(local_id) = self.key_store.authority_id(validator_set.validators()) { + if offender_ids.contains(&local_id) { + debug!(target: LOG_TARGET, "🥩 Skip equivocation report for own equivocation"); + // TODO: maybe error here instead? + return Ok(()) + } + } + + let hash = proof.correct_header.hash(); let runtime_api = self.runtime.runtime_api(); // generate key ownership proof at that block diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index 6e65dd3530c17..415a9e08cc74a 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -1696,9 +1696,9 @@ pub(crate) mod tests { let validator_set_id = 0; - let proof = generate_fork_equivocation_proof_vote((block_number as u64, payload, validator_set_id, &Keyring::Bob), header); + let proof = generate_fork_equivocation_proof_vote((block_number as u64, payload.clone(), validator_set_id, &Keyring::Bob), header.clone()); { - // expect fisher (Alice) to successfully report it + // expect fisher (Alice) to successfully process it assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); // verify Alice reports Bob's equivocation to runtime let reported = alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); @@ -1706,5 +1706,15 @@ pub(crate) mod tests { assert_eq!(*reported.get(0).unwrap(), proof); } + let proof = generate_fork_equivocation_proof_vote((block_number as u64, payload, validator_set_id, &Keyring::Alice), header); + { + // expect fisher (Alice) to successfully process it + assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); + // verify Alice does not report her own equivocation to runtime + let reported = alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); + assert_eq!(reported.len(), 1); + assert!(*reported.get(0).unwrap() != proof); + } + } } From c97c963e015601a95d09b359ec999a09f803d4d9 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 10 Aug 2023 11:09:55 +0200 Subject: [PATCH 36/41] un-stub submit_unsigned_fork_equivocation_report --- frame/beefy/src/lib.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index dd0171659f720..d84eb34d6215c 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -389,13 +389,18 @@ impl Pallet { /// a call to `report_fork_equivocation_unsigned` and will push the transaction /// to the pool. Only useful in an offchain context. pub fn submit_unsigned_fork_equivocation_report( - _fork_equivocation_proof: sp_consensus_beefy::ForkEquivocationProof, T::BeefyId, + fork_equivocation_proof: sp_consensus_beefy::ForkEquivocationProof, T::BeefyId, ::Signature, HeaderFor >, - _key_owner_proofs: Vec, + key_owner_proofs: Vec, ) -> Option<()> { - // T::EquivocationReportSystem::publish_evidence((fork_equivocation_proof, key_owner_proofs)).ok() - None + T::EquivocationReportSystem::publish_evidence( + EquivocationEvidenceFor::::ForkEquivocationProof( + fork_equivocation_proof, + key_owner_proofs, + ), + ) + .ok() } fn change_authorities( From 10540ecebb0e50fd0d9b67858cd23119d2dad331 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 24 Aug 2023 10:23:56 +0200 Subject: [PATCH 37/41] Alice reports Bob & Charlie's signed commitment --- client/consensus/beefy/src/worker.rs | 36 ++++++++++++++++---- primitives/consensus/beefy/src/test_utils.rs | 5 +-- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index 415a9e08cc74a..f7827316c5faa 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -1062,7 +1062,7 @@ pub(crate) mod tests { use sp_blockchain::Backend as BlockchainBackendT; use sp_consensus_beefy::{ generate_vote_equivocation_proof, generate_fork_equivocation_proof_vote, known_payloads, known_payloads::MMR_ROOT_ID, - mmr::MmrRootProvider, Keyring, Payload, SignedCommitment, + mmr::MmrRootProvider, Keyring, Payload, SignedCommitment, ForkEquivocationProof, }; use sp_runtime::traits::One; use std::marker::PhantomData; @@ -1677,14 +1677,14 @@ pub(crate) mod tests { #[tokio::test] async fn should_report_valid_fork_equivocations() { - let peers = [Keyring::Alice, Keyring::Bob]; + let peers = [Keyring::Alice, Keyring::Bob, Keyring::Charlie]; let validator_set = ValidatorSet::new(make_beefy_ids(&peers), 0).unwrap(); let mut api_alice = TestApi::with_validator_set(&validator_set); api_alice.allow_equivocations(); let api_alice = Arc::new(api_alice); // instantiate network with Alice and Bob running full voters. - let mut net = BeefyTestNet::new(2); + let mut net = BeefyTestNet::new(3); let session_len = 10; let hashes = net.generate_blocks_and_sync(50, session_len, &validator_set, true).await; @@ -1693,10 +1693,14 @@ pub(crate) mod tests { let block_number = 1; let header = net.peer(1).client().as_backend().blockchain().header(hashes[block_number as usize]).unwrap().unwrap(); let payload = Payload::from_single_entry(MMR_ROOT_ID, "amievil".encode()); + let votes: Vec<_> = peers.iter().map(|k| { + // signed_vote(block_number as u64, payload.clone(), validator_set.id(), k) + (block_number as u64, payload.clone(), validator_set.id(), k) + }).collect(); - let validator_set_id = 0; - let proof = generate_fork_equivocation_proof_vote((block_number as u64, payload.clone(), validator_set_id, &Keyring::Bob), header.clone()); + // verify: Alice reports Bob + let proof = generate_fork_equivocation_proof_vote(votes[1].clone(), header.clone()); { // expect fisher (Alice) to successfully process it assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); @@ -1706,15 +1710,33 @@ pub(crate) mod tests { assert_eq!(*reported.get(0).unwrap(), proof); } - let proof = generate_fork_equivocation_proof_vote((block_number as u64, payload, validator_set_id, &Keyring::Alice), header); + // verify: Alice does not self-report + let proof = generate_fork_equivocation_proof_vote(votes[0].clone(), header.clone()); { // expect fisher (Alice) to successfully process it assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); - // verify Alice does not report her own equivocation to runtime + // verify Alice does *not* report her own equivocation to runtime let reported = alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); assert_eq!(reported.len(), 1); assert!(*reported.get(0).unwrap() != proof); } + // verify: Alice reports VersionedFinalityProof equivocation + let commitment = Commitment { + payload: payload.clone(), + block_number: block_number as u64, + validator_set_id: validator_set.id(), + }; + // only Bob and Charlie sign + let signatories: Vec<_> = vec![Keyring::Bob, Keyring::Charlie].iter().map(|k| (k.public(), k.sign(&commitment.encode()))).collect(); + let proof = ForkEquivocationProof {commitment, signatories, correct_header: header}; + { + // expect fisher (Alice) to successfully process it + assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); + // verify Alice report Bob's and Charlie's equivocation to runtime + let reported = alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); + assert_eq!(reported.len(), 2); + assert_eq!(*reported.get(1).unwrap(), proof); + } } } diff --git a/primitives/consensus/beefy/src/test_utils.rs b/primitives/consensus/beefy/src/test_utils.rs index a680d997dc9b3..20a4277eb9dd9 100644 --- a/primitives/consensus/beefy/src/test_utils.rs +++ b/primitives/consensus/beefy/src/test_utils.rs @@ -17,7 +17,7 @@ #![cfg(feature = "std")] -use crate::{ecdsa_crypto, Commitment, ForkEquivocationProof, VoteEquivocationProof, Payload, ValidatorSetId, VoteMessage}; +use crate::{ecdsa_crypto, ForkEquivocationProof, Commitment, SignedCommitment, VoteEquivocationProof, Payload, ValidatorSetId, VoteMessage}; use codec::Encode; use sp_core::{ecdsa, keccak_256, Pair}; use std::collections::HashMap; @@ -91,6 +91,7 @@ impl From for ecdsa_crypto::Public { } } +/// Create a new `VoteMessage` from commitment primitives and keyring fn signed_vote(block_number: u64, payload: Payload, validator_set_id: ValidatorSetId, keyring: &Keyring) -> VoteMessage { let commitment = Commitment { validator_set_id, block_number, payload }; let signature = keyring.sign(&commitment.encode()); @@ -107,7 +108,7 @@ pub fn generate_vote_equivocation_proof( VoteEquivocationProof { first, second } } -/// Create a new `ForkEquivocationProof` based on given arguments. +/// Create a new `ForkEquivocationProof` based on vote & correct header. pub fn generate_fork_equivocation_proof_vote
( vote: (u64, Payload, ValidatorSetId, &Keyring), correct_header: Header, From 809784b1940798a641005e3435b15a7ac9f8a331 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 24 Aug 2023 10:42:53 +0200 Subject: [PATCH 38/41] cleanup --- client/consensus/beefy/src/communication/fisherman.rs | 2 +- primitives/consensus/beefy/src/lib.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 2a45a749737af..75f4ffe67057d 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -27,7 +27,7 @@ use sp_blockchain::HeaderBackend; use sp_consensus_beefy::{ check_fork_equivocation_proof, ecdsa_crypto::{AuthorityId, Signature}, - BeefyApi, ForkEquivocationProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, Commitment, OpaqueKeyOwnershipProof, SignedCommitment, + BeefyApi, ForkEquivocationProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, SignedCommitment, }; use sp_runtime::{ generic::BlockId, diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index ab14837d31917..a1c007241d815 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -34,13 +34,13 @@ mod commitment; pub mod mmr; mod payload; -#[cfg(feature = "std")] +#[cfg(all(test, feature = "std"))] mod test_utils; pub mod witness; pub use commitment::{Commitment, SignedCommitment, VersionedFinalityProof}; pub use payload::{known_payloads, BeefyPayloadId, Payload, PayloadProvider}; -#[cfg(feature = "std")] +#[cfg(all(test, feature = "std"))] pub use test_utils::*; use codec::{Codec, Decode, Encode}; From 37ec6c90e34c2fbc9b0929926b864f0552a2ec1f Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 24 Aug 2023 10:47:35 +0200 Subject: [PATCH 39/41] fmt --- .../beefy/src/communication/fisherman.rs | 89 +++++++++------ client/consensus/beefy/src/lib.rs | 2 +- client/consensus/beefy/src/round.rs | 6 +- client/consensus/beefy/src/tests.rs | 23 ++-- client/consensus/beefy/src/worker.rs | 77 +++++++++---- frame/beefy/src/equivocation.rs | 102 +++++++++++------- frame/beefy/src/lib.rs | 22 ++-- primitives/consensus/beefy/src/lib.rs | 10 +- primitives/consensus/beefy/src/test_utils.rs | 12 ++- 9 files changed, 228 insertions(+), 115 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 75f4ffe67057d..2fa91ce783966 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -17,7 +17,9 @@ // along with this program. If not, see . use crate::{ - error::Error, justification::BeefyVersionedFinalityProof, keystore::{BeefyKeystore, BeefySignatureHasher}, + error::Error, + justification::BeefyVersionedFinalityProof, + keystore::{BeefyKeystore, BeefySignatureHasher}, LOG_TARGET, }; use log::debug; @@ -27,7 +29,8 @@ use sp_blockchain::HeaderBackend; use sp_consensus_beefy::{ check_fork_equivocation_proof, ecdsa_crypto::{AuthorityId, Signature}, - BeefyApi, ForkEquivocationProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, SignedCommitment, + BeefyApi, ForkEquivocationProof, Payload, PayloadProvider, SignedCommitment, ValidatorSet, + VoteMessage, }; use sp_runtime::{ generic::BlockId, @@ -70,7 +73,10 @@ where R: ProvideRuntimeApi + Send + Sync, R::Api: BeefyApi, { - fn expected_header_and_payload(&self, number: NumberFor) -> Result<(B::Header, Payload), Error> { + fn expected_header_and_payload( + &self, + number: NumberFor, + ) -> Result<(B::Header, Payload), Error> { // This should be un-ambiguous since `number` is finalized. let hash = self .backend @@ -113,13 +119,19 @@ where .map_err(|e| Error::Backend(e.to_string()))?; if proof.commitment.validator_set_id != set_id || - !check_fork_equivocation_proof::, AuthorityId, BeefySignatureHasher, B::Header>(&proof, &expected_header_hash) + !check_fork_equivocation_proof::< + NumberFor, + AuthorityId, + BeefySignatureHasher, + B::Header, + >(&proof, &expected_header_hash) { debug!(target: LOG_TARGET, "🥩 Skip report for bad invalid fork proof {:?}", proof); return Ok(()) } - let offender_ids = proof.signatories.iter().cloned().map(|(id, _sig)| id).collect::>(); + let offender_ids = + proof.signatories.iter().cloned().map(|(id, _sig)| id).collect::>(); if let Some(local_id) = self.key_store.authority_id(validator_set.validators()) { if offender_ids.contains(&local_id) { debug!(target: LOG_TARGET, "🥩 Skip equivocation report for own equivocation"); @@ -132,26 +144,31 @@ where let runtime_api = self.runtime.runtime_api(); // generate key ownership proof at that block - let key_owner_proofs = offender_ids.iter() - .filter_map(|id| { - match runtime_api.generate_key_ownership_proof(hash, set_id, id.clone()) { - Ok(Some(proof)) => Some(Ok(proof)), - Ok(None) => { - debug!( - target: LOG_TARGET, - "🥩 Invalid fork vote offender not part of the authority set." - ); - None - }, - Err(e) => Some(Err(Error::RuntimeApi(e))), - } - }) - .collect::>()?; + let key_owner_proofs = offender_ids + .iter() + .filter_map(|id| { + match runtime_api.generate_key_ownership_proof(hash, set_id, id.clone()) { + Ok(Some(proof)) => Some(Ok(proof)), + Ok(None) => { + debug!( + target: LOG_TARGET, + "🥩 Invalid fork vote offender not part of the authority set." + ); + None + }, + Err(e) => Some(Err(Error::RuntimeApi(e))), + } + }) + .collect::>()?; // submit invalid fork vote report at **best** block let best_block_hash = self.backend.blockchain().info().best_hash; runtime_api - .submit_report_fork_equivocation_unsigned_extrinsic(best_block_hash, proof, key_owner_proofs) + .submit_report_fork_equivocation_unsigned_extrinsic( + best_block_hash, + proof, + key_owner_proofs, + ) .map_err(Error::RuntimeApi)?; Ok(()) @@ -174,7 +191,11 @@ where let number = vote.commitment.block_number; let (correct_header, expected_payload) = self.expected_header_and_payload(number)?; if vote.commitment.payload != expected_payload { - let proof = ForkEquivocationProof { commitment: vote.commitment, signatories: vec![(vote.id, vote.signature)], correct_header: correct_header.clone() }; + let proof = ForkEquivocationProof { + commitment: vote.commitment, + signatories: vec![(vote.id, vote.signature)], + correct_header: correct_header.clone(), + }; self.report_fork_equivocation(proof)?; } Ok(()) @@ -185,7 +206,7 @@ where &self, signed_commitment: SignedCommitment, Signature>, ) -> Result<(), Error> { - let SignedCommitment {commitment, signatures} = signed_commitment; + let SignedCommitment { commitment, signatures } = signed_commitment; let number = commitment.block_number; let (correct_header, expected_payload) = self.expected_header_and_payload(number)?; if commitment.payload != expected_payload { @@ -195,10 +216,19 @@ where return Ok(()) } // report every signer of the bad justification - let signatories = validator_set.validators().iter().cloned().zip(signatures.into_iter()) - .filter_map(|(id, signature)| signature.map(|sig| (id, sig))).collect(); - - let proof = ForkEquivocationProof { commitment, signatories, correct_header: correct_header.clone() }; + let signatories = validator_set + .validators() + .iter() + .cloned() + .zip(signatures.into_iter()) + .filter_map(|(id, signature)| signature.map(|sig| (id, sig))) + .collect(); + + let proof = ForkEquivocationProof { + commitment, + signatories, + correct_header: correct_header.clone(), + }; self.report_fork_equivocation(proof)?; } Ok(()) @@ -207,9 +237,8 @@ where /// Check `proof` for contained block against expected payload. fn check_proof(&self, proof: BeefyVersionedFinalityProof) -> Result<(), Error> { match proof { - BeefyVersionedFinalityProof::::V1(signed_commitment) => { - self.check_signed_commitment(signed_commitment) - } + BeefyVersionedFinalityProof::::V1(signed_commitment) => + self.check_signed_commitment(signed_commitment), } } } diff --git a/client/consensus/beefy/src/lib.rs b/client/consensus/beefy/src/lib.rs index b7576230a254c..b8d7f933c4540 100644 --- a/client/consensus/beefy/src/lib.rs +++ b/client/consensus/beefy/src/lib.rs @@ -29,10 +29,10 @@ use crate::{ }, }, import::BeefyBlockImport, + keystore::BeefyKeystore, metrics::register_metrics, round::Rounds, worker::PersistedState, - keystore::BeefyKeystore, }; use futures::{stream::Fuse, StreamExt}; use log::{error, info}; diff --git a/client/consensus/beefy/src/round.rs b/client/consensus/beefy/src/round.rs index 81e0d0634dcdd..3abe662fc27fd 100644 --- a/client/consensus/beefy/src/round.rs +++ b/client/consensus/beefy/src/round.rs @@ -22,7 +22,7 @@ use codec::{Decode, Encode}; use log::debug; use sp_consensus_beefy::{ ecdsa_crypto::{AuthorityId, Signature}, - Commitment, VoteEquivocationProof, SignedCommitment, ValidatorSet, ValidatorSetId, VoteMessage, + Commitment, SignedCommitment, ValidatorSet, ValidatorSetId, VoteEquivocationProof, VoteMessage, }; use sp_runtime::traits::{Block, NumberFor}; use std::collections::BTreeMap; @@ -203,8 +203,8 @@ mod tests { use sc_network_test::Block; use sp_consensus_beefy::{ - known_payloads::MMR_ROOT_ID, Commitment, VoteEquivocationProof, Keyring, Payload, - SignedCommitment, ValidatorSet, VoteMessage, + known_payloads::MMR_ROOT_ID, Commitment, Keyring, Payload, SignedCommitment, ValidatorSet, + VoteEquivocationProof, VoteMessage, }; use super::{threshold, AuthorityId, Block as BlockT, RoundTracker, Rounds}; diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 12b7884c4a02a..5522e782fde86 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -44,7 +44,7 @@ use sc_consensus::{ }; use sc_network::{config::RequestResponseConfig, ProtocolName}; use sc_network_test::{ - Block, BlockImportAdapter, Header, FullPeerConfig, PassThroughVerifier, Peer, PeersClient, + Block, BlockImportAdapter, FullPeerConfig, Header, PassThroughVerifier, Peer, PeersClient, PeersFullClient, TestNetFactory, }; use sc_utils::notification::NotificationReceiver; @@ -56,9 +56,9 @@ use sp_consensus_beefy::{ ecdsa_crypto::{AuthorityId, Signature}, known_payloads, mmr::{find_mmr_root_digest, MmrRootProvider}, - BeefyApi, Commitment, ConsensusLog, ForkEquivocationProof, VoteEquivocationProof, Keyring as BeefyKeyring, MmrRootHash, - OpaqueKeyOwnershipProof, Payload, SignedCommitment, ValidatorSet, ValidatorSetId, - VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID, + BeefyApi, Commitment, ConsensusLog, ForkEquivocationProof, Keyring as BeefyKeyring, + MmrRootHash, OpaqueKeyOwnershipProof, Payload, SignedCommitment, ValidatorSet, ValidatorSetId, + VersionedFinalityProof, VoteEquivocationProof, VoteMessage, BEEFY_ENGINE_ID, }; use sp_core::H256; use sp_keystore::{testing::MemoryKeystore, Keystore, KeystorePtr}; @@ -254,7 +254,10 @@ impl BeefyFisherman for DummyFisherman { fn check_proof(&self, _: BeefyVersionedFinalityProof) -> Result<(), Error> { Ok(()) } - fn check_signed_commitment(&self, _: SignedCommitment, Signature>) -> Result<(), Error> { + fn check_signed_commitment( + &self, + _: SignedCommitment, Signature>, + ) -> Result<(), Error> { Ok(()) } fn check_vote( @@ -272,8 +275,9 @@ pub(crate) struct TestApi { pub mmr_root_hash: MmrRootHash, pub reported_vote_equivocations: Option, AuthorityId, Signature>>>>>, - pub reported_fork_equivocations: - Option, AuthorityId, Signature, Header>>>>>, + pub reported_fork_equivocations: Option< + Arc, AuthorityId, Signature, Header>>>>, + >, } impl TestApi { @@ -425,7 +429,7 @@ fn initialize_beefy( min_block_delta: u32, ) -> impl Future where - API: ProvideRuntimeApi + Sync + Send +'static, + API: ProvideRuntimeApi + Sync + Send + 'static, API::Api: BeefyApi + MmrApi>, { let tasks = FuturesUnordered::new(); @@ -1318,7 +1322,8 @@ async fn beefy_reports_vote_equivocations() { } // Verify expected equivocation - let alice_reported_vote_equivocations = api_alice.reported_vote_equivocations.as_ref().unwrap().lock(); + let alice_reported_vote_equivocations = + api_alice.reported_vote_equivocations.as_ref().unwrap().lock(); assert_eq!(alice_reported_vote_equivocations.len(), 1); let equivocation_proof = alice_reported_vote_equivocations.get(0).unwrap(); assert_eq!(equivocation_proof.first.id, BeefyKeyring::Bob.public()); diff --git a/client/consensus/beefy/src/worker.rs b/client/consensus/beefy/src/worker.rs index f7827316c5faa..3ec6af0b7a5b5 100644 --- a/client/consensus/beefy/src/worker.rs +++ b/client/consensus/beefy/src/worker.rs @@ -43,8 +43,8 @@ use sp_consensus::SyncOracle; use sp_consensus_beefy::{ check_vote_equivocation_proof, ecdsa_crypto::{AuthorityId, Signature}, - BeefyApi, Commitment, ConsensusLog, VoteEquivocationProof, PayloadProvider, ValidatorSet, - VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID, + BeefyApi, Commitment, ConsensusLog, PayloadProvider, ValidatorSet, VersionedFinalityProof, + VoteEquivocationProof, VoteMessage, BEEFY_ENGINE_ID, }; use sp_runtime::{ generic::OpaqueDigestItemId, @@ -981,7 +981,11 @@ where // submit equivocation report at **best** block let best_block_hash = self.backend.blockchain().info().best_hash; runtime_api - .submit_report_vote_equivocation_unsigned_extrinsic(best_block_hash, proof, key_owner_proof) + .submit_report_vote_equivocation_unsigned_extrinsic( + best_block_hash, + proof, + key_owner_proof, + ) .map_err(Error::RuntimeApi)?; Ok(()) @@ -1061,8 +1065,9 @@ pub(crate) mod tests { use sp_api::HeaderT; use sp_blockchain::Backend as BlockchainBackendT; use sp_consensus_beefy::{ - generate_vote_equivocation_proof, generate_fork_equivocation_proof_vote, known_payloads, known_payloads::MMR_ROOT_ID, - mmr::MmrRootProvider, Keyring, Payload, SignedCommitment, ForkEquivocationProof, + generate_fork_equivocation_proof_vote, generate_vote_equivocation_proof, known_payloads, + known_payloads::MMR_ROOT_ID, mmr::MmrRootProvider, ForkEquivocationProof, Keyring, Payload, + SignedCommitment, }; use sp_runtime::traits::One; use std::marker::PhantomData; @@ -1130,7 +1135,8 @@ pub(crate) mod tests { let backend = peer.client().as_backend(); let beefy_genesis = 1; - let api = runtime_api.unwrap_or_else(|| Arc::new(TestApi::with_validator_set(&genesis_validator_set))); + let api = runtime_api + .unwrap_or_else(|| Arc::new(TestApi::with_validator_set(&genesis_validator_set))); let network = peer.network_service().clone(); let sync = peer.sync_service().clone(); let payload_provider = MmrRootProvider::new(api.clone()); @@ -1624,7 +1630,13 @@ pub(crate) mod tests { let api_alice = Arc::new(api_alice); let mut net = BeefyTestNet::new(1); - let worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone(), Some(api_alice.clone())); + let worker = create_beefy_worker( + net.peer(0), + &keys[0], + 1, + validator_set.clone(), + Some(api_alice.clone()), + ); // let there be a block with num = 1: let _ = net.peer(0).push_blocks(1, false); @@ -1688,24 +1700,36 @@ pub(crate) mod tests { let session_len = 10; let hashes = net.generate_blocks_and_sync(50, session_len, &validator_set, true).await; - let alice_worker = create_beefy_worker(net.peer(0), &peers[0], 1, validator_set.clone(), Some(api_alice)); + let alice_worker = + create_beefy_worker(net.peer(0), &peers[0], 1, validator_set.clone(), Some(api_alice)); let block_number = 1; - let header = net.peer(1).client().as_backend().blockchain().header(hashes[block_number as usize]).unwrap().unwrap(); + let header = net + .peer(1) + .client() + .as_backend() + .blockchain() + .header(hashes[block_number as usize]) + .unwrap() + .unwrap(); let payload = Payload::from_single_entry(MMR_ROOT_ID, "amievil".encode()); - let votes: Vec<_> = peers.iter().map(|k| { - // signed_vote(block_number as u64, payload.clone(), validator_set.id(), k) - (block_number as u64, payload.clone(), validator_set.id(), k) - }).collect(); + let votes: Vec<_> = peers + .iter() + .map(|k| (block_number as u64, payload.clone(), validator_set.id(), k)) + .collect(); // verify: Alice reports Bob let proof = generate_fork_equivocation_proof_vote(votes[1].clone(), header.clone()); { // expect fisher (Alice) to successfully process it - assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); + assert_eq!( + alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), + Ok(()) + ); // verify Alice reports Bob's equivocation to runtime - let reported = alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); + let reported = + alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); assert_eq!(reported.len(), 1); assert_eq!(*reported.get(0).unwrap(), proof); } @@ -1714,9 +1738,13 @@ pub(crate) mod tests { let proof = generate_fork_equivocation_proof_vote(votes[0].clone(), header.clone()); { // expect fisher (Alice) to successfully process it - assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); + assert_eq!( + alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), + Ok(()) + ); // verify Alice does *not* report her own equivocation to runtime - let reported = alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); + let reported = + alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); assert_eq!(reported.len(), 1); assert!(*reported.get(0).unwrap() != proof); } @@ -1728,13 +1756,20 @@ pub(crate) mod tests { validator_set_id: validator_set.id(), }; // only Bob and Charlie sign - let signatories: Vec<_> = vec![Keyring::Bob, Keyring::Charlie].iter().map(|k| (k.public(), k.sign(&commitment.encode()))).collect(); - let proof = ForkEquivocationProof {commitment, signatories, correct_header: header}; + let signatories: Vec<_> = vec![Keyring::Bob, Keyring::Charlie] + .iter() + .map(|k| (k.public(), k.sign(&commitment.encode()))) + .collect(); + let proof = ForkEquivocationProof { commitment, signatories, correct_header: header }; { // expect fisher (Alice) to successfully process it - assert_eq!(alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), Ok(())); + assert_eq!( + alice_worker.gossip_validator.fisherman.report_fork_equivocation(proof.clone()), + Ok(()) + ); // verify Alice report Bob's and Charlie's equivocation to runtime - let reported = alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); + let reported = + alice_worker.runtime.reported_fork_equivocations.as_ref().unwrap().lock(); assert_eq!(reported.len(), 2); assert_eq!(*reported.get(1).unwrap(), proof); } diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index cfc0d78c54f52..7a7529f1b0f94 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -38,7 +38,9 @@ use codec::{self as codec, Decode, Encode}; use frame_support::traits::{Get, KeyOwnerProofSystem}; use frame_system::pallet_prelude::BlockNumberFor; use log::{error, info}; -use sp_consensus_beefy::{VoteEquivocationProof, ForkEquivocationProof, ValidatorSetId, KEY_TYPE as BEEFY_KEY_TYPE}; +use sp_consensus_beefy::{ + ForkEquivocationProof, ValidatorSetId, VoteEquivocationProof, KEY_TYPE as BEEFY_KEY_TYPE, +}; use sp_runtime::{ transaction_validity::{ InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity, @@ -125,20 +127,21 @@ pub struct EquivocationReportSystem(sp_std::marker::PhantomData<(T, pub enum EquivocationEvidenceFor { VoteEquivocationProof( VoteEquivocationProof< - BlockNumberFor, + BlockNumberFor, ::BeefyId, <::BeefyId as RuntimeAppPublic>::Signature, - >, + >, ::KeyOwnerProof, ), ForkEquivocationProof( - ForkEquivocationProof, - ::BeefyId, - <::BeefyId as RuntimeAppPublic>::Signature, - HeaderFor, - >, + ForkEquivocationProof< + BlockNumberFor, + ::BeefyId, + <::BeefyId as RuntimeAppPublic>::Signature, + HeaderFor, + >, Vec<::KeyOwnerProof>, - ) + ), } impl OffenceReportSystem, EquivocationEvidenceFor> @@ -160,18 +163,18 @@ where use frame_system::offchain::SubmitTransaction; let call = match evidence { - EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => { + EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => Call::report_vote_equivocation_unsigned { equivocation_proof: Box::new(equivocation_proof), key_owner_proof, - } - } - EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, key_owner_proofs) => { - Call::report_fork_equivocation_unsigned { - equivocation_proof: Box::new(equivocation_proof), - key_owner_proofs, - } - } + }, + EquivocationEvidenceFor::ForkEquivocationProof( + equivocation_proof, + key_owner_proofs, + ) => Call::report_fork_equivocation_unsigned { + equivocation_proof: Box::new(equivocation_proof), + key_owner_proofs, + }, }; let res = SubmitTransaction::>::submit_unsigned_transaction(call.into()); @@ -187,15 +190,20 @@ where ) -> Result<(), TransactionValidityError> { let (offenders, key_owner_proofs, time_slot) = match &evidence { EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => { - // Check if the offence has already been reported, and if so then we can discard the report. + // Check if the offence has already been reported, and if so then we can discard the + // report. let time_slot = TimeSlot { set_id: equivocation_proof.set_id(), round: *equivocation_proof.round_number(), }; (vec![equivocation_proof.offender_id()], vec![key_owner_proof.clone()], time_slot) }, - EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, key_owner_proofs) => { - // Check if the offence has already been reported, and if so then we can discard the report. + EquivocationEvidenceFor::ForkEquivocationProof( + equivocation_proof, + key_owner_proofs, + ) => { + // Check if the offence has already been reported, and if so then we can discard the + // report. let time_slot = TimeSlot { set_id: equivocation_proof.set_id(), round: *equivocation_proof.round_number(), @@ -209,7 +217,9 @@ where 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())) + .map(|(key, key_owner_proof)| { + P::check_proof((BEEFY_KEY_TYPE, key.clone()), key_owner_proof.clone()) + }) .collect::>>() .ok_or(InvalidTransaction::BadProof)?; @@ -227,12 +237,24 @@ where let reporter = reporter.or_else(|| >::author()); let (offenders, key_owner_proofs, set_id, round) = match &evidence { - EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => { - (vec![equivocation_proof.offender_id()], vec![key_owner_proof.clone()], equivocation_proof.set_id(), *equivocation_proof.round_number()) - }, - EquivocationEvidenceFor::ForkEquivocationProof(equivocation_proof, key_owner_proofs) => { + EquivocationEvidenceFor::VoteEquivocationProof(equivocation_proof, key_owner_proof) => + ( + vec![equivocation_proof.offender_id()], + vec![key_owner_proof.clone()], + equivocation_proof.set_id(), + *equivocation_proof.round_number(), + ), + EquivocationEvidenceFor::ForkEquivocationProof( + equivocation_proof, + key_owner_proofs, + ) => { let offenders = equivocation_proof.offender_ids(); // clone data here - (offenders, key_owner_proofs.to_owned(), equivocation_proof.set_id(), *equivocation_proof.round_number()) + ( + offenders, + key_owner_proofs.to_owned(), + equivocation_proof.set_id(), + *equivocation_proof.round_number(), + ) }, }; @@ -247,7 +269,9 @@ where 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())) + .map(|(key, key_owner_proof)| { + P::check_proof((BEEFY_KEY_TYPE, key.clone()), key_owner_proof.clone()) + }) .collect::>>() .ok_or(Error::::InvalidKeyOwnershipProof)?; @@ -262,8 +286,12 @@ where let block_number = equivocation_proof.commitment.block_number; let expected_block_hash = >::block_hash(block_number); - // Validate equivocation proof (check commitment is to unexpected payload and signatures are valid). - if !sp_consensus_beefy::check_fork_equivocation_proof(&equivocation_proof, &expected_block_hash) { + // Validate equivocation proof (check commitment is to unexpected payload and + // signatures are valid). + if !sp_consensus_beefy::check_fork_equivocation_proof( + &equivocation_proof, + &expected_block_hash, + ) { return Err(Error::::InvalidForkEquivocationProof.into()) } }, @@ -271,8 +299,8 @@ where // Check that the session id for the membership proof is within the // bounds of the set id reported in the equivocation. - let set_id_session_index = - crate::SetIdSession::::get(set_id).ok_or(Error::::InvalidVoteEquivocationProof)?; + let set_id_session_index = crate::SetIdSession::::get(set_id) + .ok_or(Error::::InvalidVoteEquivocationProof)?; if session_index != set_id_session_index { return Err(Error::::InvalidVoteEquivocationProof.into()) } @@ -292,12 +320,14 @@ where } /// Methods for the `ValidateUnsigned` implementation: -/// It restricts calls to `report_vote_equivocation_unsigned` to local calls (i.e. extrinsics generated -/// on this node) or that already in a block. This guarantees that only block authors can include -/// unsigned equivocation reports. +/// It restricts calls to `report_vote_equivocation_unsigned` to local calls (i.e. extrinsics +/// generated on this node) or that already in a block. This guarantees that only block authors can +/// include unsigned equivocation reports. impl Pallet { pub fn validate_unsigned(source: TransactionSource, call: &Call) -> TransactionValidity { - if let Call::report_vote_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { + if let Call::report_vote_equivocation_unsigned { equivocation_proof, key_owner_proof } = + call + { // discard equivocation report not coming from the local node match source { TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }, diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index af869ae26bbc0..e54098f99e35d 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -28,7 +28,7 @@ use frame_support::{ }; use frame_system::{ ensure_none, ensure_signed, - pallet_prelude::{BlockNumberFor, OriginFor, HeaderFor}, + pallet_prelude::{BlockNumberFor, HeaderFor, OriginFor}, }; use log; use sp_runtime::{ @@ -41,8 +41,8 @@ use sp_staking::{offence::OffenceReportSystem, SessionIndex}; use sp_std::prelude::*; use sp_consensus_beefy::{ - AuthorityIndex, BeefyAuthorityId, ConsensusLog, VoteEquivocationProof, OnNewValidatorSet, - ValidatorSet, BEEFY_ENGINE_ID, GENESIS_AUTHORITY_SET_ID, + AuthorityIndex, BeefyAuthorityId, ConsensusLog, OnNewValidatorSet, ValidatorSet, + VoteEquivocationProof, BEEFY_ENGINE_ID, GENESIS_AUTHORITY_SET_ID, }; mod default_weights; @@ -292,7 +292,7 @@ pub mod pallet { BlockNumberFor, T::BeefyId, ::Signature, - HeaderFor + HeaderFor, >, >, key_owner_proofs: Vec, @@ -301,7 +301,10 @@ pub mod pallet { T::EquivocationReportSystem::process_evidence( Some(reporter), - EquivocationEvidenceFor::ForkEquivocationProof(*equivocation_proof, key_owner_proofs), + EquivocationEvidenceFor::ForkEquivocationProof( + *equivocation_proof, + key_owner_proofs, + ), )?; // Waive the fee since the report is valid and beneficial Ok(Pays::No.into()) @@ -389,9 +392,12 @@ impl Pallet { /// a call to `report_fork_equivocation_unsigned` and will push the transaction /// to the pool. Only useful in an offchain context. pub fn submit_unsigned_fork_equivocation_report( - fork_equivocation_proof: sp_consensus_beefy::ForkEquivocationProof, T::BeefyId, - ::Signature, HeaderFor ->, + fork_equivocation_proof: sp_consensus_beefy::ForkEquivocationProof< + BlockNumberFor, + T::BeefyId, + ::Signature, + HeaderFor, + >, key_owner_proofs: Vec, ) -> Option<()> { T::EquivocationReportSystem::publish_evidence( diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index a1c007241d815..f15593bad5836 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -47,7 +47,7 @@ use codec::{Codec, Decode, Encode}; use scale_info::TypeInfo; use sp_application_crypto::RuntimeAppPublic; use sp_core::H256; -use sp_runtime::traits::{Hash, Keccak256, NumberFor, Header}; +use sp_runtime::traits::{Hash, Header, Keccak256, NumberFor}; use sp_std::prelude::*; /// Key type for BEEFY module. @@ -366,16 +366,16 @@ where } let expected_mmr_root_digest = mmr::find_mmr_root_digest::
(correct_header); - let expected_payload = expected_mmr_root_digest.map(|mmr_root| { - Payload::from_single_entry(known_payloads::MMR_ROOT_ID, mmr_root.encode()) - }); + let expected_payload = expected_mmr_root_digest + .map(|mmr_root| Payload::from_single_entry(known_payloads::MMR_ROOT_ID, mmr_root.encode())); // cheap failfasts: // 1. check that `payload` on the `vote` is different that the `expected_payload` // 2. if the signatories signed a payload when there should be none (for // instance for a block prior to BEEFY activation), they should likewise be // slashed - if Some(&commitment.payload) != expected_payload.as_ref() || expected_mmr_root_digest.is_none() { + if Some(&commitment.payload) != expected_payload.as_ref() || expected_mmr_root_digest.is_none() + { // 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 diff --git a/primitives/consensus/beefy/src/test_utils.rs b/primitives/consensus/beefy/src/test_utils.rs index 20a4277eb9dd9..d52148073efdd 100644 --- a/primitives/consensus/beefy/src/test_utils.rs +++ b/primitives/consensus/beefy/src/test_utils.rs @@ -17,7 +17,10 @@ #![cfg(feature = "std")] -use crate::{ecdsa_crypto, ForkEquivocationProof, Commitment, SignedCommitment, VoteEquivocationProof, Payload, ValidatorSetId, VoteMessage}; +use crate::{ + ecdsa_crypto, Commitment, ForkEquivocationProof, Payload, SignedCommitment, ValidatorSetId, + VoteEquivocationProof, VoteMessage, +}; use codec::Encode; use sp_core::{ecdsa, keccak_256, Pair}; use std::collections::HashMap; @@ -92,7 +95,12 @@ impl From for ecdsa_crypto::Public { } /// Create a new `VoteMessage` from commitment primitives and keyring -fn signed_vote(block_number: u64, payload: Payload, validator_set_id: ValidatorSetId, keyring: &Keyring) -> VoteMessage { +fn signed_vote( + block_number: u64, + payload: Payload, + validator_set_id: ValidatorSetId, + keyring: &Keyring, +) -> VoteMessage { let commitment = Commitment { validator_set_id, block_number, payload }; let signature = keyring.sign(&commitment.encode()); VoteMessage { commitment, id: keyring.public(), signature } From 4c2e0ba22ce2211bd90ea297d983de18c6f4d373 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 24 Aug 2023 11:00:15 +0200 Subject: [PATCH 40/41] fixup! cleanup --- frame/beefy/src/equivocation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index 7a7529f1b0f94..625d12ec21324 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -36,7 +36,7 @@ use codec::{self as codec, Decode, Encode}; use frame_support::traits::{Get, KeyOwnerProofSystem}; -use frame_system::pallet_prelude::BlockNumberFor; +use frame_system::pallet_prelude::{BlockNumberFor, HeaderFor}; use log::{error, info}; use sp_consensus_beefy::{ ForkEquivocationProof, ValidatorSetId, VoteEquivocationProof, KEY_TYPE as BEEFY_KEY_TYPE, From 3b655a185b13e09bf321cbff269a78665042905a Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 24 Aug 2023 12:30:42 +0200 Subject: [PATCH 41/41] remove superfluous None check Co-authored-by: Adrian Catangiu --- primitives/consensus/beefy/src/lib.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index f15593bad5836..09da46cdf113f 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -372,10 +372,9 @@ where // cheap failfasts: // 1. check that `payload` on the `vote` is different that the `expected_payload` // 2. if the signatories signed a payload when there should be none (for - // instance for a block prior to BEEFY activation), they should likewise be - // slashed - if Some(&commitment.payload) != expected_payload.as_ref() || expected_mmr_root_digest.is_none() - { + // instance for a block prior to BEEFY activation), then expected_payload = + // None, and they will likewise be slashed + if Some(&commitment.payload) != expected_payload.as_ref() { // 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