From 908a5373692ffd17adefdf58d4332530682fd728 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 5 Jul 2023 12:02:07 +0300 Subject: [PATCH 01/15] 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 9be648f8796c3..8e54f88f1f670 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,13 +490,14 @@ 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_consensus_beefy::{ crypto::Signature, known_payloads, Commitment, Keyring, MmrRootHash, Payload, SignedCommitment, VoteMessage, KEY_TYPE, }; use sp_keystore::{testing::MemoryKeystore, Keystore}; + use std::marker::PhantomData; #[test] fn known_votes_insert_remove() { @@ -576,8 +593,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; @@ -704,7 +722,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(); @@ -781,7 +800,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 0de67f6062339..f01140ce177e8 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 c55849ff7722c..bcacd6eeb0193 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, @@ -220,10 +221,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, @@ -250,10 +251,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 6bb6b1e548557..990189de227eb 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_consensus::BlockOrigin; use sp_consensus_beefy::{ crypto::{AuthorityId, Signature}, @@ -243,6 +244,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, @@ -363,8 +380,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(), @@ -385,7 +403,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(); @@ -1309,9 +1327,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 91b00ddb2cef0..188f5268946c6 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 8ad3cc3721a00..362de24297d24 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 { use super::*; diff --git a/primitives/consensus/beefy/src/mmr.rs b/primitives/consensus/beefy/src/mmr.rs index c303cae2fdcc7..46d984140c4b7 100644 --- a/primitives/consensus/beefy/src/mmr.rs +++ b/primitives/consensus/beefy/src/mmr.rs @@ -155,6 +155,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, @@ -177,7 +183,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 7198d3ba41b1598b6984eddec0cc303660119a06 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 5 Jul 2023 14:10:40 +0300 Subject: [PATCH 02/15] 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 6132c01187f01..6f447d4febb89 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -215,6 +215,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( @@ -266,6 +291,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. @@ -327,6 +383,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 640038417b5bbe0b0730fc98d9ad5d5560a0b247 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 5 Jul 2023 14:22:00 +0300 Subject: [PATCH 03/15] 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 96446cf1b8277..ca22b4d71dfd1 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -124,6 +124,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< ::BlockNumber, diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index d56d5b32f8f36..0a51590f68abb 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 { @@ -107,6 +108,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, >; } @@ -255,6 +258,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 7537bc3096b609aaf7c20f04b76127bc828e2978 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 5 Jul 2023 15:52:27 +0300 Subject: [PATCH 04/15] 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 8e54f88f1f670..9e6d859f03f12 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 990189de227eb..0aef7b8aaa8b0 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, @@ -249,13 +250,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 362de24297d24..b2ce56bd04966 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 70922293538096f684f45f3df8dbff8cdbd9c908 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 20 Jul 2023 09:49:31 +0200 Subject: [PATCH 05/15] 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 6f447d4febb89..500af52e7812a 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -216,13 +216,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 { @@ -293,8 +291,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, @@ -304,20 +309,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 18e307acc0ad8dcce5fb525688f061dc4b45ca11 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 20 Jul 2023 23:00:09 +0200 Subject: [PATCH 06/15] 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 | 69 +++++++++---------- primitives/consensus/beefy/src/lib.rs | 48 +++++++------ 2 files changed, 61 insertions(+), 56 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 1f63fe58a294f..2d89fffacb181 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -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/primitives/consensus/beefy/src/lib.rs b/primitives/consensus/beefy/src/lib.rs index 500af52e7812a..bd71ab2b28248 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -215,12 +215,17 @@ 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. + 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 { @@ -301,23 +306,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. @@ -381,9 +389,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 @@ -391,8 +399,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 fd8408280433d95679d66c3e421ba964956f5d53 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 27 Jul 2023 10:07:17 +0200 Subject: [PATCH 07/15] fixup! change primitive: vote -> commitment --- .../beefy/src/communication/fisherman.rs | 2 +- frame/beefy/src/lib.rs | 14 +++++++------- primitives/consensus/beefy/src/lib.rs | 16 ++++++++++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index 2d89fffacb181..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, diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index 0a51590f68abb..fa5c6575e9885 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 { @@ -265,10 +265,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, @@ -287,9 +287,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 @@ -298,10 +298,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 bd71ab2b28248..d80e17c68a8cc 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -219,7 +219,11 @@ impl EquivocationProof { /// This proof shows commitment signed on a different fork than finalized by GRANDPA. #[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)] pub struct InvalidForkCommitmentProof { - /// Commitment for a block on different fork than one at the same height in this client's chain. + /// 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 @@ -228,18 +232,18 @@ pub struct InvalidForkCommitmentProof { 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 } } From 2b9d49f19fb81e2383facd163c649295f12ef86f Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 27 Jul 2023 11:05:18 +0200 Subject: [PATCH 08/15] 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 0aef7b8aaa8b0..3cb359f2ddc07 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -253,6 +253,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 ef9b8dd8b30b0e2e0de75b45dae27216502a1c3f Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 1 Aug 2023 20:16:11 +0200 Subject: [PATCH 09/15] 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 304374bd8942c..ea7d5312119c2 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 b03f14b11ab9a76c7cd2ea1fdf5f56b907659173 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Tue, 1 Aug 2023 20:21:35 +0200 Subject: [PATCH 10/15] fmt --- .../beefy/src/communication/fisherman.rs | 105 +++++++++++------- client/consensus/beefy/src/tests.rs | 5 +- 2 files changed, 70 insertions(+), 40 deletions(-) diff --git a/client/consensus/beefy/src/communication/fisherman.rs b/client/consensus/beefy/src/communication/fisherman.rs index bb9554bc8ca08..1ca2377bfe342 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -27,7 +27,15 @@ use sp_blockchain::HeaderBackend; use sp_consensus_beefy::{ check_invalid_fork_proof, crypto::{AuthorityId, Signature}, - BeefyApi, InvalidForkCommitmentProof, Payload, PayloadProvider, ValidatorSet, VoteMessage, Commitment, OpaqueKeyOwnershipProof, SignedCommitment, + BeefyApi, + Commitment, + InvalidForkCommitmentProof, + OpaqueKeyOwnershipProof, + Payload, + PayloadProvider, + SignedCommitment, + ValidatorSet, + VoteMessage, }; use sp_runtime::{ generic::BlockId, @@ -72,7 +80,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 @@ -124,25 +135,31 @@ where 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::>(); 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::>()?; + 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; @@ -169,25 +186,27 @@ where } let hash = correct_header.hash(); - let offender_ids = proof.signatories.iter().cloned().map(|(id, _sig)| id).collect::>(); + 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(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; @@ -218,7 +237,11 @@ 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 = InvalidForkCommitmentProof { + commitment: vote.commitment, + signatories: vec![(vote.id, vote.signature)], + expected_payload, + }; self.report_invalid_fork_commitments(proof, &header)?; } Ok(()) @@ -255,13 +278,17 @@ 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 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, + proof, &header, )?; } Ok(()) diff --git a/client/consensus/beefy/src/tests.rs b/client/consensus/beefy/src/tests.rs index 3cb359f2ddc07..a936011e0e109 100644 --- a/client/consensus/beefy/src/tests.rs +++ b/client/consensus/beefy/src/tests.rs @@ -253,7 +253,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( From c53ee4e7be9b58d6af5b7225cda4e3209e5b2a8d Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 2 Aug 2023 10:36:54 +0200 Subject: [PATCH 11/15] 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 1ca2377bfe342..c4780bb5f939b 100644 --- a/client/consensus/beefy/src/communication/fisherman.rs +++ b/client/consensus/beefy/src/communication/fisherman.rs @@ -261,7 +261,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 ea7d5312119c2..c0d26b9fc09cd 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 d80e17c68a8cc..3d418a5567c2d 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -319,7 +319,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 @@ -369,7 +370,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(2)] pub trait BeefyApi { From 2ab10353791a1a67c7a50fcdc2afd21707daa397 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 2 Aug 2023 10:52:56 +0200 Subject: [PATCH 12/15] 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 c0d26b9fc09cd..69a301e13f69b 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -369,6 +369,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 3d418a5567c2d..8f66096b38e18 100644 --- a/primitives/consensus/beefy/src/lib.rs +++ b/primitives/consensus/beefy/src/lib.rs @@ -353,7 +353,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 6ef97f7b2a8c4597274714425bf7f9d66614d8c7 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 2 Aug 2023 11:02:27 +0200 Subject: [PATCH 13/15] clone equivocation report system for invalid forks --- frame/beefy/src/invalid_fork_reporting.rs | 309 ++++++++++++++++++++++ 1 file changed, 309 insertions(+) create mode 100644 frame/beefy/src/invalid_fork_reporting.rs diff --git a/frame/beefy/src/invalid_fork_reporting.rs b/frame/beefy/src/invalid_fork_reporting.rs new file mode 100644 index 0000000000000..853270cb4d7e9 --- /dev/null +++ b/frame/beefy/src/invalid_fork_reporting.rs @@ -0,0 +1,309 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! An opt-in utility module for reporting equivocations. +//! +//! This module defines an offence type for BEEFY equivocations +//! and some utility traits to wire together: +//! - a key ownership proof system (e.g. to prove that a given authority was part of a session); +//! - a system for reporting offences; +//! - a system for signing and submitting transactions; +//! - a way to get the current block author; +//! +//! These can be used in an offchain context in order to submit equivocation +//! reporting extrinsics (from the client that's running the BEEFY protocol). +//! And in a runtime context, so that the BEEFY pallet can validate the +//! equivocation proofs in the extrinsic and report the offences. +//! +//! IMPORTANT: +//! When using this module for enabling equivocation reporting it is required +//! that the `ValidateUnsigned` for the BEEFY pallet is used in the runtime +//! definition. + +use codec::{self as codec, Decode, Encode}; +use frame_support::{ + log, + traits::{Get, KeyOwnerProofSystem}, +}; +use frame_system::pallet_prelude::BlockNumberFor; +use log::{error, info}; +use sp_consensus_beefy::{EquivocationProof, ValidatorSetId, KEY_TYPE}; +use sp_runtime::{ + transaction_validity::{ + InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity, + TransactionValidityError, ValidTransaction, + }, + DispatchError, KeyTypeId, Perbill, RuntimeAppPublic, +}; +use sp_session::{GetSessionNumber, GetValidatorCount}; +use sp_staking::{ + offence::{Kind, Offence, OffenceReportSystem, ReportOffence}, + SessionIndex, +}; +use sp_std::prelude::*; + +use super::{Call, Config, Error, Pallet, LOG_TARGET}; + +/// A round number and set id which point on the time of an offence. +#[derive(Copy, Clone, PartialOrd, Ord, Eq, PartialEq, Encode, Decode)] +pub struct TimeSlot { + // The order of these matters for `derive(Ord)`. + /// BEEFY Set ID. + pub set_id: ValidatorSetId, + /// Round number. + pub round: N, +} + +/// BEEFY equivocation offence report. +pub struct EquivocationOffence +where + N: Copy + Clone + PartialOrd + Ord + Eq + PartialEq + Encode + Decode, +{ + /// Time slot at which this incident happened. + pub time_slot: TimeSlot, + /// The session index in which the incident happened. + 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, +} + +impl Offence for EquivocationOffence +where + N: Copy + Clone + PartialOrd + Ord + Eq + PartialEq + Encode + Decode, +{ + const ID: Kind = *b"beefy:equivocati"; + type TimeSlot = TimeSlot; + + fn offenders(&self) -> Vec { + vec![self.offender.clone()] + } + + fn session_index(&self) -> SessionIndex { + self.session_index + } + + fn validator_set_count(&self) -> u32 { + self.validator_set_count + } + + fn time_slot(&self) -> Self::TimeSlot { + self.time_slot + } + + // The formula is min((3k / n)^2, 1) + // where k = offenders_number and n = validators_number + fn slash_fraction(&self, offenders_count: u32) -> Perbill { + // Perbill type domain is [0, 1] by definition + Perbill::from_rational(3 * offenders_count, self.validator_set_count).square() + } +} + +/// BEEFY equivocation offence report system. +/// +/// This type implements `OffenceReportSystem` such that: +/// - Equivocation reports are published on-chain as unsigned extrinsic via +/// `offchain::SendTransactionTypes`. +/// - On-chain validity checks and processing are mostly delegated to the user provided generic +/// types implementing `KeyOwnerProofSystem` and `ReportOffence` traits. +/// - Offence reporter for unsigned transactions is fetched via the the authorship pallet. +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, + ::BeefyId, + <::BeefyId as RuntimeAppPublic>::Signature, + >, + ::KeyOwnerProof, +); + +// -> +// pub enum EquivocationEvidenceFor { +// EquivocationProof( +// EquivocationProof< +// BlockNumberFor, +// ::BeefyId, +// <::BeefyId as RuntimeAppPublic>::Signature, +// >, +// ::KeyOwnerProof, +// ), +// InvalidForkCommitmentProof(sp_consensus_beefy::InvalidForkCommitmentProof< +// BlockNumberFor, +// ::BeefyId, +// <::BeefyId as RuntimeAppPublic>::Signature, +// >) +// } + + +impl OffenceReportSystem, EquivocationEvidenceFor> + for EquivocationReportSystem +where + T: Config + pallet_authorship::Config + frame_system::offchain::SendTransactionTypes>, + R: ReportOffence< + T::AccountId, + P::IdentificationTuple, + EquivocationOffence>, + >, + P: KeyOwnerProofSystem<(KeyTypeId, T::BeefyId), Proof = T::KeyOwnerProof>, + P::IdentificationTuple: Clone, + L: Get, +{ + type Longevity = L; + + fn publish_evidence(evidence: EquivocationEvidenceFor) -> Result<(), ()> { + use frame_system::offchain::SubmitTransaction; + let (equivocation_proof, key_owner_proof) = evidence; + + let call = Call::report_equivocation_unsigned { + equivocation_proof: Box::new(equivocation_proof), + key_owner_proof, + }; + + let res = SubmitTransaction::>::submit_unsigned_transaction(call.into()); + match res { + Ok(_) => info!(target: LOG_TARGET, "Submitted equivocation report."), + Err(e) => error!(target: LOG_TARGET, "Error submitting equivocation report: {:?}", e), + } + res + } + + 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 = (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(), + }; + + if R::is_known_offence(&[offender], &time_slot) { + Err(InvalidTransaction::Stale.into()) + } else { + Ok(()) + } + } + + fn process_evidence( + 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(); + + // 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((KEY_TYPE, offender), key_owner_proof) + .ok_or(Error::::InvalidKeyOwnershipProof)?; + + // 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()) + } + + // 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)?; + if session_index != set_id_session_index { + return Err(Error::::InvalidEquivocationProof.into()) + } + + let offence = EquivocationOffence { + time_slot: TimeSlot { set_id, round }, + session_index, + validator_set_count, + offender, + }; + + R::report_offence(reporter.into_iter().collect(), offence) + .map_err(|_| Error::::DuplicateOffenceReport)?; + + Ok(()) + } +} + +/// Methods for the `ValidateUnsigned` implementation: +/// It restricts calls to `report_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 { + // discard equivocation report not coming from the local node + match source { + TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }, + _ => { + log::warn!( + target: LOG_TARGET, + "rejecting unsigned report equivocation transaction because it is not local/in-block." + ); + return InvalidTransaction::Call.into() + }, + } + + let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); + T::EquivocationReportSystem::check_evidence(evidence)?; + + let longevity = + >::Longevity::get(); + + ValidTransaction::with_tag_prefix("BeefyEquivocation") + // We assign the maximum priority for any equivocation report. + .priority(TransactionPriority::MAX) + // Only one equivocation report for the same offender at the same slot. + .and_provides(( + equivocation_proof.offender_id().clone(), + equivocation_proof.set_id(), + *equivocation_proof.round_number(), + )) + .longevity(longevity) + // We don't propagate this. This can never be included on a remote node. + .propagate(false) + .build() + } else { + InvalidTransaction::Call.into() + } + } + + pub fn pre_dispatch(call: &Call) -> Result<(), TransactionValidityError> { + if let Call::report_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { + let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); + T::EquivocationReportSystem::check_evidence(evidence) + } else { + Err(InvalidTransaction::Call.into()) + } + } +} From 6d0ecdf699a973ae5d5fa27dfcdbe3a4ca67cc08 Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Wed, 2 Aug 2023 11:18:52 +0200 Subject: [PATCH 14/15] comment duplicate validate_unsigned --- frame/beefy/src/equivocation.rs | 1 + frame/beefy/src/invalid_fork_reporting.rs | 104 +++++++++++----------- 2 files changed, 53 insertions(+), 52 deletions(-) diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index ed93a84159548..d4d947e8f9ff8 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -241,6 +241,7 @@ where /// It restricts calls to `report_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. +// TODO: move to lib.rs and make it generic over the offence report system impl Pallet { pub fn validate_unsigned(source: TransactionSource, call: &Call) -> TransactionValidity { if let Call::report_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { diff --git a/frame/beefy/src/invalid_fork_reporting.rs b/frame/beefy/src/invalid_fork_reporting.rs index 853270cb4d7e9..81336f8f163b1 100644 --- a/frame/beefy/src/invalid_fork_reporting.rs +++ b/frame/beefy/src/invalid_fork_reporting.rs @@ -255,55 +255,55 @@ where } } -/// Methods for the `ValidateUnsigned` implementation: -/// It restricts calls to `report_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 { - // discard equivocation report not coming from the local node - match source { - TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }, - _ => { - log::warn!( - target: LOG_TARGET, - "rejecting unsigned report equivocation transaction because it is not local/in-block." - ); - return InvalidTransaction::Call.into() - }, - } - - let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); - T::EquivocationReportSystem::check_evidence(evidence)?; - - let longevity = - >::Longevity::get(); - - ValidTransaction::with_tag_prefix("BeefyEquivocation") - // We assign the maximum priority for any equivocation report. - .priority(TransactionPriority::MAX) - // Only one equivocation report for the same offender at the same slot. - .and_provides(( - equivocation_proof.offender_id().clone(), - equivocation_proof.set_id(), - *equivocation_proof.round_number(), - )) - .longevity(longevity) - // We don't propagate this. This can never be included on a remote node. - .propagate(false) - .build() - } else { - InvalidTransaction::Call.into() - } - } - - pub fn pre_dispatch(call: &Call) -> Result<(), TransactionValidityError> { - if let Call::report_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { - let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); - T::EquivocationReportSystem::check_evidence(evidence) - } else { - Err(InvalidTransaction::Call.into()) - } - } -} +// Methods for the `ValidateUnsigned` implementation: +// It restricts calls to `report_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 { +// // discard equivocation report not coming from the local node +// match source { +// TransactionSource::Local | TransactionSource::InBlock => { /* allowed */ }, +// _ => { +// log::warn!( +// target: LOG_TARGET, +// "rejecting unsigned report equivocation transaction because it is not local/in-block." +// ); +// return InvalidTransaction::Call.into() +// }, +// } + +// let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); +// T::EquivocationReportSystem::check_evidence(evidence)?; + +// let longevity = +// >::Longevity::get(); + +// ValidTransaction::with_tag_prefix("BeefyEquivocation") +// // We assign the maximum priority for any equivocation report. +// .priority(TransactionPriority::MAX) +// // Only one equivocation report for the same offender at the same slot. +// .and_provides(( +// equivocation_proof.offender_id().clone(), +// equivocation_proof.set_id(), +// *equivocation_proof.round_number(), +// )) +// .longevity(longevity) +// // We don't propagate this. This can never be included on a remote node. +// .propagate(false) +// .build() +// } else { +// InvalidTransaction::Call.into() +// } +// } + +// pub fn pre_dispatch(call: &Call) -> Result<(), TransactionValidityError> { +// if let Call::report_equivocation_unsigned { equivocation_proof, key_owner_proof } = call { +// let evidence = (*equivocation_proof.clone(), key_owner_proof.clone()); +// T::EquivocationReportSystem::check_evidence(evidence) +// } else { +// Err(InvalidTransaction::Call.into()) +// } +// } +// } From e490104331febdb621a9e0606fa50a001259851d Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Mon, 7 Aug 2023 15:22:33 +0200 Subject: [PATCH 15/15] implement InvalidForkReportSystem --- frame/beefy-mmr/src/mock.rs | 1 + frame/beefy/src/equivocation.rs | 30 +++++- frame/beefy/src/invalid_fork_reporting.rs | 116 +++++++++------------- frame/beefy/src/lib.rs | 28 ++++-- frame/beefy/src/mock.rs | 2 + 5 files changed, 98 insertions(+), 79 deletions(-) diff --git a/frame/beefy-mmr/src/mock.rs b/frame/beefy-mmr/src/mock.rs index b17550653d5ea..ff1c6ef2de79c 100644 --- a/frame/beefy-mmr/src/mock.rs +++ b/frame/beefy-mmr/src/mock.rs @@ -124,6 +124,7 @@ impl pallet_beefy::Config for Test { type WeightInfo = (); type KeyOwnerProof = sp_core::Void; type EquivocationReportSystem = (); + type InvalidForkReportSystem = (); } parameter_types! { diff --git a/frame/beefy/src/equivocation.rs b/frame/beefy/src/equivocation.rs index d4d947e8f9ff8..a1a8f6248bd8d 100644 --- a/frame/beefy/src/equivocation.rs +++ b/frame/beefy/src/equivocation.rs @@ -108,6 +108,8 @@ where // The formula is min((3k / n)^2, 1) // where k = offenders_number and n = validators_number + // TODO: progressive slashing may not be the appropriate solution for BEEFY, + // or we may want to saturate it before 1/3rd fn slash_fraction(&self, offenders_count: u32) -> Perbill { // Perbill type domain is [0, 1] by definition Perbill::from_rational(3 * offenders_count, self.validator_set_count).square() @@ -125,7 +127,6 @@ 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, @@ -135,6 +136,33 @@ pub type EquivocationEvidenceFor = ( ::KeyOwnerProof, ); +/// Invalid fork evidence convenience alias. +// pub type InvalidForkEvidenceFor = ( +// InvalidForkCommitmentProof< +// BlockNumberFor, +// ::BeefyId, +// <::BeefyId as RuntimeAppPublic>::Signature, +// >, +// ::KeyOwnerProof, +// ); + +// -> +// pub enum EquivocationEvidenceFor { +// EquivocationProof( +// EquivocationProof< +// BlockNumberFor, +// ::BeefyId, +// <::BeefyId as RuntimeAppPublic>::Signature, +// >, +// ::KeyOwnerProof, +// ), +// InvalidForkCommitmentProof(sp_consensus_beefy::InvalidForkCommitmentProof< +// BlockNumberFor, +// ::BeefyId, +// <::BeefyId as RuntimeAppPublic>::Signature, +// >) +// } + impl OffenceReportSystem, EquivocationEvidenceFor> for EquivocationReportSystem where diff --git a/frame/beefy/src/invalid_fork_reporting.rs b/frame/beefy/src/invalid_fork_reporting.rs index 81336f8f163b1..a6c0cbacf7db0 100644 --- a/frame/beefy/src/invalid_fork_reporting.rs +++ b/frame/beefy/src/invalid_fork_reporting.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}; +use sp_consensus_beefy::{InvalidForkCommitmentProof, ValidatorSetId, KEY_TYPE}; use sp_runtime::{ transaction_validity::{ InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity, @@ -56,20 +56,12 @@ use sp_staking::{ }; use sp_std::prelude::*; -use super::{Call, Config, Error, Pallet, LOG_TARGET}; +use super::{Call, Config, Error, LOG_TARGET}; -/// A round number and set id which point on the time of an offence. -#[derive(Copy, Clone, PartialOrd, Ord, Eq, PartialEq, Encode, Decode)] -pub struct TimeSlot { - // The order of these matters for `derive(Ord)`. - /// BEEFY Set ID. - pub set_id: ValidatorSetId, - /// Round number. - pub round: N, -} +use crate::equivocation::TimeSlot; -/// BEEFY equivocation offence report. -pub struct EquivocationOffence +/// BEEFY invalid fork offence report. +pub struct InvalidForkOffence where N: Copy + Clone + PartialOrd + Ord + Eq + PartialEq + Encode + Decode, { @@ -79,19 +71,19 @@ 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 signed on this invalid fork. + pub offenders: Vec, } -impl Offence for EquivocationOffence +impl Offence for InvalidForkOffence where N: Copy + Clone + PartialOrd + Ord + Eq + PartialEq + Encode + Decode, { - const ID: Kind = *b"beefy:equivocati"; + const ID: Kind = *b"beefy:invalidfor"; type TimeSlot = TimeSlot; fn offenders(&self) -> Vec { - vec![self.offender.clone()] + self.offenders.clone() } fn session_index(&self) -> SessionIndex { @@ -108,26 +100,27 @@ where // The formula is min((3k / n)^2, 1) // where k = offenders_number and n = validators_number + // TODO: progressive slashing may not be the appropriate solution for BEEFY, + // or we may want to saturate it before 1/3rd fn slash_fraction(&self, offenders_count: u32) -> Perbill { // Perbill type domain is [0, 1] by definition Perbill::from_rational(3 * offenders_count, self.validator_set_count).square() } } -/// BEEFY equivocation offence report system. +/// BEEFY invalid fork offence report system. /// /// This type implements `OffenceReportSystem` such that: -/// - Equivocation reports are published on-chain as unsigned extrinsic via +/// - Invalid fork reports are published on-chain as unsigned extrinsic via /// `offchain::SendTransactionTypes`. /// - On-chain validity checks and processing are mostly delegated to the user provided generic /// types implementing `KeyOwnerProofSystem` and `ReportOffence` traits. /// - Offence reporter for unsigned transactions is fetched via the the authorship pallet. -pub struct EquivocationReportSystem(sp_std::marker::PhantomData<(T, R, P, L)>); +pub struct InvalidForkReportSystem(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< +/// Invalid Fork evidence convenience alias. +pub type InvalidForkEvidenceFor = ( + InvalidForkCommitmentProof< BlockNumberFor, ::BeefyId, <::BeefyId as RuntimeAppPublic>::Signature, @@ -135,32 +128,14 @@ pub type EquivocationEvidenceFor = ( ::KeyOwnerProof, ); -// -> -// pub enum EquivocationEvidenceFor { -// EquivocationProof( -// EquivocationProof< -// BlockNumberFor, -// ::BeefyId, -// <::BeefyId as RuntimeAppPublic>::Signature, -// >, -// ::KeyOwnerProof, -// ), -// InvalidForkCommitmentProof(sp_consensus_beefy::InvalidForkCommitmentProof< -// BlockNumberFor, -// ::BeefyId, -// <::BeefyId as RuntimeAppPublic>::Signature, -// >) -// } - - -impl OffenceReportSystem, EquivocationEvidenceFor> - for EquivocationReportSystem +impl OffenceReportSystem, InvalidForkEvidenceFor> + for InvalidForkReportSystem where T: Config + pallet_authorship::Config + frame_system::offchain::SendTransactionTypes>, R: ReportOffence< T::AccountId, P::IdentificationTuple, - EquivocationOffence>, + InvalidForkOffence>, >, P: KeyOwnerProofSystem<(KeyTypeId, T::BeefyId), Proof = T::KeyOwnerProof>, P::IdentificationTuple: Clone, @@ -168,31 +143,33 @@ where { type Longevity = L; - fn publish_evidence(evidence: EquivocationEvidenceFor) -> Result<(), ()> { + fn publish_evidence(evidence: InvalidForkEvidenceFor) -> Result<(), ()> { use frame_system::offchain::SubmitTransaction; - let (equivocation_proof, key_owner_proof) = evidence; + let (invalid_fork_proof, key_owner_proofs) = evidence; - let call = Call::report_equivocation_unsigned { - equivocation_proof: Box::new(equivocation_proof), - key_owner_proof, + let call = Call::report_invalid_fork_commitment_unsigned { + invalid_fork_proof: Box::new(invalid_fork_proof), + key_owner_proofs, }; let res = SubmitTransaction::>::submit_unsigned_transaction(call.into()); match res { - Ok(_) => info!(target: LOG_TARGET, "Submitted equivocation report."), - Err(e) => error!(target: LOG_TARGET, "Error submitting equivocation report: {:?}", e), + Ok(_) => info!(target: LOG_TARGET, "Submitted invalid fork report."), + Err(e) => error!(target: LOG_TARGET, "Error submitting invalid fork report: {:?}", e), } res } - fn check_evidence( - evidence: EquivocationEvidenceFor, - ) -> Result<(), TransactionValidityError> { + fn check_evidence(evidence: InvalidForkEvidenceFor) -> Result<(), TransactionValidityError> { let (equivocation_proof, key_owner_proof) = evidence; // Check the membership proof to extract the offender's id - let key = (KEY_TYPE, equivocation_proof.offender_id().clone()); - let offender = P::check_proof(key, key_owner_proof).ok_or(InvalidTransaction::BadProof)?; + let offenders = equivocation_proof + .offender_ids() + .into_iter() + .map(|key| P::check_proof((KEY_TYPE, key.clone()), key_owner_proof.clone())) + .collect::>>() + .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 { @@ -200,7 +177,7 @@ where round: *equivocation_proof.round_number(), }; - if R::is_known_offence(&[offender], &time_slot) { + if R::is_known_offence(&offenders, &time_slot) { Err(InvalidTransaction::Stale.into()) } else { Ok(()) @@ -209,27 +186,30 @@ where fn process_evidence( reporter: Option, - evidence: EquivocationEvidenceFor, + evidence: InvalidForkEvidenceFor, ) -> Result<(), DispatchError> { - let (equivocation_proof, key_owner_proof) = evidence; + let (invalid_fork_proof, key_owner_proof) = evidence; let reporter = reporter.or_else(|| >::author()); - let offender = 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 // 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 set_id = invalid_fork_proof.set_id(); + let round = *invalid_fork_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((KEY_TYPE, offender), key_owner_proof) + let offenders = invalid_fork_proof + .offender_ids() + .into_iter() + .map(|key| P::check_proof((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_equivocation_proof(&equivocation_proof) { + if !sp_consensus_beefy::check_invalid_fork_proof(&invalid_fork_proof) { return Err(Error::::InvalidEquivocationProof.into()) } @@ -241,11 +221,11 @@ where return Err(Error::::InvalidEquivocationProof.into()) } - let offence = EquivocationOffence { + let offence = InvalidForkOffence { time_slot: TimeSlot { set_id, round }, session_index, validator_set_count, - 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 69a301e13f69b..336454cc4465f 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -47,15 +47,19 @@ use sp_consensus_beefy::{ mod default_weights; mod equivocation; +mod invalid_fork_reporting; #[cfg(test)] mod mock; #[cfg(test)] mod tests; pub use crate::equivocation::{EquivocationOffence, EquivocationReportSystem, TimeSlot}; +pub use crate::invalid_fork_reporting::{InvalidForkOffence, InvalidForkReportSystem}; + pub use pallet::*; use crate::equivocation::EquivocationEvidenceFor; +use crate::invalid_fork_reporting::InvalidForkEvidenceFor; const LOG_TARGET: &str = "runtime::beefy"; @@ -112,10 +116,16 @@ 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 - // `InvalidForkCommitmentProof` EquivocationEvidenceFor, >; + + /// The equivocation handling subsystem. + /// + /// Defines methods to publish, check and process an equivocation offence. + type InvalidForkReportSystem: OffenceReportSystem< + Option, + InvalidForkEvidenceFor, + >; } #[pallet::pallet] @@ -310,7 +320,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(), T::MaxNominators::get(),))] + #[pallet::weight(T::WeightInfo::report_equivocation(key_owner_proofs.validator_count(), T::MaxNominators::get(),))] pub fn report_invalid_fork_commitment_unsigned( origin: OriginFor, invalid_fork_proof: Box< @@ -320,15 +330,13 @@ pub mod pallet { ::Signature, >, >, - key_owner_proof: T::KeyOwnerProof, + key_owner_proofs: T::KeyOwnerProof, ) -> DispatchResultWithPostInfo { ensure_none(origin)?; - - // TODO: - // T::EquivocationReportSystem::process_evidence( - // None, - // (*invalid_fork_proof, key_owner_proof), - // )?; +T::InvalidForkReportSystem::process_evidence( + None, + (*invalid_fork_proof, key_owner_proofs), + )?; Ok(Pays::No.into()) } } diff --git a/frame/beefy/src/mock.rs b/frame/beefy/src/mock.rs index 26a13fe16ca41..9408acc625985 100644 --- a/frame/beefy/src/mock.rs +++ b/frame/beefy/src/mock.rs @@ -117,6 +117,8 @@ impl pallet_beefy::Config for Test { type KeyOwnerProof = >::Proof; type EquivocationReportSystem = super::EquivocationReportSystem; + type InvalidForkReportSystem = + super::InvalidForkReportSystem; } parameter_types! {