From 6644b78d7280cc48dd9cb65404c0a164221082ca Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 6 Feb 2023 14:42:54 +0200 Subject: [PATCH] BEEFY: client support for detecting equivocated votes (#13285) * client/beefy: detect equivocated votes * client/beefy: make sure to persist state after voting * client/beefy: drop never-used aux-schema v2 migration * impl review suggestion --------- Signed-off-by: Adrian Catangiu --- Cargo.lock | 3 +- client/beefy/Cargo.toml | 1 - client/beefy/src/aux_schema.rs | 16 +- client/beefy/src/communication/gossip.rs | 7 +- client/beefy/src/justification.rs | 5 +- client/beefy/src/keystore.rs | 62 +--- client/beefy/src/metrics.rs | 6 + client/beefy/src/round.rs | 411 +++++++++++------------ client/beefy/src/tests.rs | 2 +- client/beefy/src/worker.rs | 133 +++----- primitives/beefy/Cargo.toml | 2 + primitives/beefy/src/commitment.rs | 1 + primitives/beefy/src/lib.rs | 79 ++++- 13 files changed, 373 insertions(+), 355 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 52de03f647512..afebaabd6c06c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -512,7 +512,6 @@ dependencies = [ "sp-mmr-primitives", "sp-runtime", "sp-tracing", - "strum", "substrate-prometheus-endpoint", "substrate-test-runtime-client", "tempfile", @@ -9734,6 +9733,7 @@ name = "sp-beefy" version = "4.0.0-dev" dependencies = [ "array-bytes", + "lazy_static", "parity-scale-codec", "scale-info", "serde", @@ -9745,6 +9745,7 @@ dependencies = [ "sp-mmr-primitives", "sp-runtime", "sp-std", + "strum", ] [[package]] diff --git a/client/beefy/Cargo.toml b/client/beefy/Cargo.toml index d81ff2587cf2c..2976d1474012a 100644 --- a/client/beefy/Cargo.toml +++ b/client/beefy/Cargo.toml @@ -39,7 +39,6 @@ sp-runtime = { version = "7.0.0", path = "../../primitives/runtime" } [dev-dependencies] serde = "1.0.136" -strum = { version = "0.24.1", features = ["derive"] } tempfile = "3.1.0" tokio = "1.22.0" sc-block-builder = { version = "0.10.0-dev", path = "../block-builder" } diff --git a/client/beefy/src/aux_schema.rs b/client/beefy/src/aux_schema.rs index 217ea4769d8b9..6e87956dac2db 100644 --- a/client/beefy/src/aux_schema.rs +++ b/client/beefy/src/aux_schema.rs @@ -26,25 +26,25 @@ use sp_blockchain::{Error as ClientError, Result as ClientResult}; use sp_runtime::traits::Block as BlockT; const VERSION_KEY: &[u8] = b"beefy_auxschema_version"; -const WORKER_STATE: &[u8] = b"beefy_voter_state"; +const WORKER_STATE_KEY: &[u8] = b"beefy_voter_state"; const CURRENT_VERSION: u32 = 2; -pub(crate) fn write_current_version(backend: &B) -> ClientResult<()> { +pub(crate) fn write_current_version(backend: &BE) -> ClientResult<()> { info!(target: LOG_TARGET, "🥩 write aux schema version {:?}", CURRENT_VERSION); AuxStore::insert_aux(backend, &[(VERSION_KEY, CURRENT_VERSION.encode().as_slice())], &[]) } /// Write voter state. -pub(crate) fn write_voter_state( - backend: &B, - state: &PersistedState, +pub(crate) fn write_voter_state( + backend: &BE, + state: &PersistedState, ) -> ClientResult<()> { trace!(target: LOG_TARGET, "🥩 persisting {:?}", state); - backend.insert_aux(&[(WORKER_STATE, state.encode().as_slice())], &[]) + AuxStore::insert_aux(backend, &[(WORKER_STATE_KEY, state.encode().as_slice())], &[]) } -fn load_decode(backend: &B, key: &[u8]) -> ClientResult> { +fn load_decode(backend: &BE, key: &[u8]) -> ClientResult> { match backend.get_aux(key)? { None => Ok(None), Some(t) => T::decode(&mut &t[..]) @@ -64,7 +64,7 @@ where match version { None => (), Some(1) => (), // version 1 is totally obsolete and should be simply ignored - Some(2) => return load_decode::<_, PersistedState>(backend, WORKER_STATE), + Some(2) => return load_decode::<_, PersistedState>(backend, WORKER_STATE_KEY), other => return Err(ClientError::Backend(format!("Unsupported BEEFY DB version: {:?}", other))), } diff --git a/client/beefy/src/communication/gossip.rs b/client/beefy/src/communication/gossip.rs index 116a6286ed54d..5d51610214610 100644 --- a/client/beefy/src/communication/gossip.rs +++ b/client/beefy/src/communication/gossip.rs @@ -120,7 +120,7 @@ where /// Note a voting round. /// - /// Noting round will start a live `round`. + /// Noting round will track gossiped votes for `round`. pub(crate) fn note_round(&self, round: NumberFor) { debug!(target: LOG_TARGET, "🥩 About to note gossip round #{}", round); self.known_votes.write().insert(round); @@ -242,9 +242,10 @@ mod tests { use sc_network_test::Block; use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; - use crate::keystore::{tests::Keyring, BeefyKeystore}; + use crate::keystore::BeefyKeystore; use beefy_primitives::{ - crypto::Signature, known_payloads, Commitment, MmrRootHash, Payload, VoteMessage, KEY_TYPE, + crypto::Signature, keyring::Keyring, known_payloads, Commitment, MmrRootHash, Payload, + VoteMessage, KEY_TYPE, }; use super::*; diff --git a/client/beefy/src/justification.rs b/client/beefy/src/justification.rs index 7243c692727f0..6f869015ba763 100644 --- a/client/beefy/src/justification.rs +++ b/client/beefy/src/justification.rs @@ -81,12 +81,13 @@ fn verify_with_validator_set( #[cfg(test)] pub(crate) mod tests { use beefy_primitives::{ - known_payloads, Commitment, Payload, SignedCommitment, VersionedFinalityProof, + keyring::Keyring, known_payloads, Commitment, Payload, SignedCommitment, + VersionedFinalityProof, }; use substrate_test_runtime_client::runtime::Block; use super::*; - use crate::{keystore::tests::Keyring, tests::make_beefy_ids}; + use crate::tests::make_beefy_ids; pub(crate) fn new_finality_proof( block_num: NumberFor, diff --git a/client/beefy/src/keystore.rs b/client/beefy/src/keystore.rs index d1f5615a93701..73788a31b7fdc 100644 --- a/client/beefy/src/keystore.rs +++ b/client/beefy/src/keystore.rs @@ -19,7 +19,6 @@ use sp_application_crypto::RuntimeAppPublic; use sp_core::keccak_256; use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; -use sp_runtime::traits::Keccak256; use log::warn; @@ -30,6 +29,9 @@ use beefy_primitives::{ use crate::{error, LOG_TARGET}; +/// Hasher used for BEEFY signatures. +pub(crate) type BeefySignatureHasher = sp_runtime::traits::Keccak256; + /// A BEEFY specific keystore implemented as a `Newtype`. This is basically a /// wrapper around [`sp_keystore::SyncCryptoStore`] and allows to customize /// common cryptographic functionality. @@ -104,7 +106,7 @@ impl BeefyKeystore { /// /// Return `true` if the signature is authentic, `false` otherwise. pub fn verify(public: &Public, sig: &Signature, message: &[u8]) -> bool { - BeefyAuthorityId::::verify(public, sig, message) + BeefyAuthorityId::::verify(public, sig, message) } } @@ -119,63 +121,13 @@ pub mod tests { use std::sync::Arc; use sc_keystore::LocalKeystore; - use sp_core::{ecdsa, keccak_256, Pair}; - use sp_keystore::{SyncCryptoStore, SyncCryptoStorePtr}; + use sp_core::{ecdsa, Pair}; - use beefy_primitives::{crypto, KEY_TYPE}; + use beefy_primitives::{crypto, keyring::Keyring}; - use super::BeefyKeystore; + use super::*; use crate::error::Error; - /// Set of test accounts using [`beefy_primitives::crypto`] types. - #[allow(missing_docs)] - #[derive(Debug, Clone, Copy, PartialEq, Eq, strum::Display, strum::EnumIter)] - pub(crate) enum Keyring { - Alice, - Bob, - Charlie, - Dave, - Eve, - Ferdie, - One, - Two, - } - - impl Keyring { - /// Sign `msg`. - pub fn sign(self, msg: &[u8]) -> crypto::Signature { - let msg = keccak_256(msg); - ecdsa::Pair::from(self).sign_prehashed(&msg).into() - } - - /// Return key pair. - pub fn pair(self) -> crypto::Pair { - ecdsa::Pair::from_string(self.to_seed().as_str(), None).unwrap().into() - } - - /// Return public key. - pub fn public(self) -> crypto::Public { - self.pair().public() - } - - /// Return seed string. - pub fn to_seed(self) -> String { - format!("//{}", self) - } - } - - impl From for crypto::Pair { - fn from(k: Keyring) -> Self { - k.pair() - } - } - - impl From for ecdsa::Pair { - fn from(k: Keyring) -> Self { - k.pair().into() - } - } - fn keystore() -> SyncCryptoStorePtr { Arc::new(LocalKeystore::in_memory()) } diff --git a/client/beefy/src/metrics.rs b/client/beefy/src/metrics.rs index 71e34e24c4fa0..55fdecc36d4b0 100644 --- a/client/beefy/src/metrics.rs +++ b/client/beefy/src/metrics.rs @@ -30,6 +30,8 @@ pub(crate) struct Metrics { pub beefy_round_concluded: Gauge, /// Best block finalized by BEEFY pub beefy_best_block: Gauge, + /// Best block BEEFY voted on + pub beefy_best_voted: Gauge, /// Next block BEEFY should vote on pub beefy_should_vote_on: Gauge, /// Number of sessions with lagging signed commitment on mandatory block @@ -61,6 +63,10 @@ impl Metrics { Gauge::new("substrate_beefy_best_block", "Best block finalized by BEEFY")?, registry, )?, + beefy_best_voted: register( + Gauge::new("substrate_beefy_best_voted", "Best block voted on by BEEFY")?, + registry, + )?, beefy_should_vote_on: register( Gauge::new("substrate_beefy_should_vote_on", "Next block, BEEFY should vote on")?, registry, diff --git a/client/beefy/src/round.rs b/client/beefy/src/round.rs index 647d42110e926..9ad4e5d0232bf 100644 --- a/client/beefy/src/round.rs +++ b/client/beefy/src/round.rs @@ -19,39 +19,33 @@ use crate::LOG_TARGET; use beefy_primitives::{ - crypto::{Public, Signature}, - ValidatorSet, ValidatorSetId, + crypto::{AuthorityId, Public, Signature}, + Commitment, SignedCommitment, ValidatorSet, ValidatorSetId, VoteMessage, }; use codec::{Decode, Encode}; -use log::{debug, trace}; +use log::debug; use sp_runtime::traits::{Block, NumberFor}; -use std::{collections::BTreeMap, hash::Hash}; +use std::collections::BTreeMap; /// Tracks for each round which validators have voted/signed and /// whether the local `self` validator has voted/signed. /// /// Does not do any validation on votes or signatures, layers above need to handle that (gossip). #[derive(Debug, Decode, Default, Encode, PartialEq)] -struct RoundTracker { - self_vote: bool, +pub(crate) struct RoundTracker { votes: BTreeMap, } impl RoundTracker { - fn add_vote(&mut self, vote: (Public, Signature), self_vote: bool) -> bool { + fn add_vote(&mut self, vote: (Public, Signature)) -> bool { if self.votes.contains_key(&vote.0) { return false } - self.self_vote = self.self_vote || self_vote; self.votes.insert(vote.0, vote.1); true } - fn has_self_vote(&self) -> bool { - self.self_vote - } - fn is_done(&self, threshold: usize) -> bool { self.votes.len() >= threshold } @@ -63,27 +57,37 @@ pub fn threshold(authorities: usize) -> usize { authorities - faulty } +#[derive(Debug, PartialEq)] +pub enum VoteImportResult { + Ok, + RoundConcluded(SignedCommitment, Signature>), + Equivocation, /* TODO: (EquivocationProof, Public, Signature>) */ + Invalid, + Stale, +} + /// Keeps track of all voting rounds (block numbers) within a session. /// Only round numbers > `best_done` are of interest, all others are considered stale. /// /// Does not do any validation on votes or signatures, layers above need to handle that (gossip). #[derive(Debug, Decode, Encode, PartialEq)] -pub(crate) struct Rounds { - rounds: BTreeMap<(Payload, NumberFor), RoundTracker>, +pub(crate) struct Rounds { + rounds: BTreeMap>, RoundTracker>, + previous_votes: BTreeMap<(Public, NumberFor), VoteMessage, Public, Signature>>, session_start: NumberFor, validator_set: ValidatorSet, mandatory_done: bool, best_done: Option>, } -impl Rounds +impl Rounds where - P: Ord + Hash + Clone, B: Block, { pub(crate) fn new(session_start: NumberFor, validator_set: ValidatorSet) -> Self { Rounds { rounds: BTreeMap::new(), + previous_votes: BTreeMap::new(), session_start, validator_set, mandatory_done: false, @@ -111,60 +115,77 @@ where self.mandatory_done } - pub(crate) fn should_self_vote(&self, round: &(P, NumberFor)) -> bool { - Some(round.1) > self.best_done && - self.rounds.get(round).map(|tracker| !tracker.has_self_vote()).unwrap_or(true) - } - pub(crate) fn add_vote( &mut self, - round: &(P, NumberFor), - vote: (Public, Signature), - self_vote: bool, - ) -> bool { - let num = round.1; + vote: VoteMessage, AuthorityId, Signature>, + ) -> VoteImportResult { + let num = vote.commitment.block_number; + let vote_key = (vote.id.clone(), num); + if num < self.session_start || Some(num) <= self.best_done { debug!(target: LOG_TARGET, "🥩 received vote for old stale round {:?}, ignoring", num); - false - } else if !self.validators().iter().any(|id| vote.0 == *id) { + return VoteImportResult::Stale + } else if vote.commitment.validator_set_id != self.validator_set_id() { + debug!( + target: LOG_TARGET, + "🥩 expected set_id {:?}, ignoring vote {:?}.", + self.validator_set_id(), + vote, + ); + return VoteImportResult::Invalid + } else if !self.validators().iter().any(|id| &vote.id == id) { debug!( target: LOG_TARGET, "🥩 received vote {:?} from validator that is not in the validator set, ignoring", vote ); - false + return VoteImportResult::Invalid + } + + if let Some(previous_vote) = self.previous_votes.get(&vote_key) { + // is the same public key voting for a different payload? + if previous_vote.commitment.payload != vote.commitment.payload { + debug!( + target: LOG_TARGET, + "🥩 detected equivocated vote: 1st: {:?}, 2nd: {:?}", previous_vote, vote + ); + // TODO: build `EquivocationProof` and return it here. + return VoteImportResult::Equivocation + } } else { - self.rounds.entry(round.clone()).or_default().add_vote(vote, self_vote) + // this is the first vote sent by `id` for `num`, all good + self.previous_votes.insert(vote_key, vote.clone()); } + + // add valid vote + let round = self.rounds.entry(vote.commitment.clone()).or_default(); + if round.add_vote((vote.id, vote.signature)) && + round.is_done(threshold(self.validator_set.len())) + { + if let Some(round) = self.rounds.remove_entry(&vote.commitment) { + return VoteImportResult::RoundConcluded(self.signed_commitment(round)) + } + } + VoteImportResult::Ok } - pub(crate) fn should_conclude( + fn signed_commitment( &mut self, - round: &(P, NumberFor), - ) -> Option>> { - let done = self - .rounds - .get(round) - .map(|tracker| tracker.is_done(threshold(self.validator_set.len()))) - .unwrap_or(false); - trace!(target: LOG_TARGET, "🥩 Round #{} done: {}", round.1, done); - - if done { - let signatures = self.rounds.remove(round)?.votes; - Some( - self.validators() - .iter() - .map(|authority_id| signatures.get(authority_id).cloned()) - .collect(), - ) - } else { - None - } + round: (Commitment>, RoundTracker), + ) -> SignedCommitment, Signature> { + let votes = round.1.votes; + let signatures = self + .validators() + .iter() + .map(|authority_id| votes.get(authority_id).cloned()) + .collect(); + SignedCommitment { commitment: round.0, signatures } } pub(crate) fn conclude(&mut self, round_num: NumberFor) { // Remove this and older (now stale) rounds. - self.rounds.retain(|&(_, number), _| number > round_num); + self.rounds.retain(|commitment, _| commitment.block_number > round_num); + self.previous_votes.retain(|&(_, number), _| number > round_num); self.mandatory_done = self.mandatory_done || round_num == self.session_start; self.best_done = self.best_done.max(Some(round_num)); debug!(target: LOG_TARGET, "🥩 Concluded round #{}", round_num); @@ -174,16 +195,17 @@ where #[cfg(test)] mod tests { use sc_network_test::Block; - use sp_core::H256; - use beefy_primitives::{crypto::Public, ValidatorSet}; + use beefy_primitives::{ + crypto::Public, keyring::Keyring, known_payloads::MMR_ROOT_ID, Commitment, Payload, + SignedCommitment, ValidatorSet, VoteMessage, + }; - use super::{threshold, Block as BlockT, Hash, RoundTracker, Rounds}; - use crate::keystore::tests::Keyring; + use super::{threshold, Block as BlockT, RoundTracker, Rounds}; + use crate::round::VoteImportResult; - impl Rounds + impl Rounds where - P: Ord + Hash + Clone, B: BlockT, { pub(crate) fn test_set_mandatory_done(&mut self, done: bool) { @@ -197,26 +219,18 @@ mod tests { let bob_vote = (Keyring::Bob.public(), Keyring::Bob.sign(b"I am committed")); let threshold = 2; - // self vote not added yet - assert!(!rt.has_self_vote()); - // adding new vote allowed - assert!(rt.add_vote(bob_vote.clone(), false)); + assert!(rt.add_vote(bob_vote.clone())); // adding existing vote not allowed - assert!(!rt.add_vote(bob_vote, false)); - - // self vote still not added yet - assert!(!rt.has_self_vote()); + assert!(!rt.add_vote(bob_vote)); // vote is not done assert!(!rt.is_done(threshold)); let alice_vote = (Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed")); // adding new vote (self vote this time) allowed - assert!(rt.add_vote(alice_vote, true)); + assert!(rt.add_vote(alice_vote)); - // self vote registered - assert!(rt.has_self_vote()); // vote is now done assert!(rt.is_done(threshold)); } @@ -242,7 +256,7 @@ mod tests { .unwrap(); let session_start = 1u64.into(); - let rounds = Rounds::::new(session_start, validators); + let rounds = Rounds::::new(session_start, validators); assert_eq!(42, rounds.validator_set_id()); assert_eq!(1, rounds.session_start()); @@ -266,65 +280,56 @@ mod tests { Default::default(), ) .unwrap(); - let round = (H256::from_low_u64_le(1), 1); + let validator_set_id = validators.id(); let session_start = 1u64.into(); - let mut rounds = Rounds::::new(session_start, validators); - - // no self vote yet, should self vote - assert!(rounds.should_self_vote(&round)); - + let mut rounds = Rounds::::new(session_start, validators); + + let payload = Payload::from_single_entry(MMR_ROOT_ID, vec![]); + let block_number = 1; + let commitment = Commitment { block_number, payload, validator_set_id }; + let mut vote = VoteMessage { + id: Keyring::Alice.public(), + commitment: commitment.clone(), + signature: Keyring::Alice.sign(b"I am committed"), + }; // add 1st good vote - assert!(rounds.add_vote( - &round, - (Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed")), - true - )); - // round not concluded - assert!(rounds.should_conclude(&round).is_none()); - // self vote already present, should not self vote - assert!(!rounds.should_self_vote(&round)); - - // double voting not allowed - assert!(!rounds.add_vote( - &round, - (Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed")), - true - )); + assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Ok); + // double voting (same vote), ok, no effect + assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Ok); + + vote.id = Keyring::Dave.public(); + vote.signature = Keyring::Dave.sign(b"I am committed"); // invalid vote (Dave is not a validator) - assert!(!rounds.add_vote( - &round, - (Keyring::Dave.public(), Keyring::Dave.sign(b"I am committed")), - false - )); - assert!(rounds.should_conclude(&round).is_none()); + assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Invalid); + vote.id = Keyring::Bob.public(); + vote.signature = Keyring::Bob.sign(b"I am committed"); // add 2nd good vote - assert!(rounds.add_vote( - &round, - (Keyring::Bob.public(), Keyring::Bob.sign(b"I am committed")), - false - )); - // round not concluded - assert!(rounds.should_conclude(&round).is_none()); - - // add 3rd good vote - assert!(rounds.add_vote( - &round, - (Keyring::Charlie.public(), Keyring::Charlie.sign(b"I am committed")), - false - )); - // round concluded - assert!(rounds.should_conclude(&round).is_some()); - rounds.conclude(round.1); + assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Ok); + vote.id = Keyring::Charlie.public(); + vote.signature = Keyring::Charlie.sign(b"I am committed"); + // add 3rd good vote -> round concluded -> signatures present + assert_eq!( + rounds.add_vote(vote.clone()), + VoteImportResult::RoundConcluded(SignedCommitment { + commitment, + signatures: vec![ + Some(Keyring::Alice.sign(b"I am committed")), + Some(Keyring::Bob.sign(b"I am committed")), + Some(Keyring::Charlie.sign(b"I am committed")), + None, + ] + }) + ); + rounds.conclude(block_number); + + vote.id = Keyring::Eve.public(); + vote.signature = Keyring::Eve.sign(b"I am committed"); // Eve is a validator, but round was concluded, adding vote disallowed - assert!(!rounds.add_vote( - &round, - (Keyring::Eve.public(), Keyring::Eve.sign(b"I am committed")), - false - )); + assert_eq!(rounds.add_vote(vote), VoteImportResult::Stale); } #[test] @@ -336,30 +341,39 @@ mod tests { 42, ) .unwrap(); - let alice = (Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed")); + let validator_set_id = validators.id(); + // active rounds starts at block 10 let session_start = 10u64.into(); - let mut rounds = Rounds::::new(session_start, validators); - - let mut vote = (H256::from_low_u64_le(1), 9); + let mut rounds = Rounds::::new(session_start, validators); + + // vote on round 9 + let block_number = 9; + let payload = Payload::from_single_entry(MMR_ROOT_ID, vec![]); + let commitment = Commitment { block_number, payload, validator_set_id }; + let mut vote = VoteMessage { + id: Keyring::Alice.public(), + commitment, + signature: Keyring::Alice.sign(b"I am committed"), + }; // add vote for previous session, should fail - assert!(!rounds.add_vote(&vote, alice.clone(), true)); + assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Stale); // no votes present assert!(rounds.rounds.is_empty()); // simulate 11 was concluded rounds.best_done = Some(11); // add votes for current session, but already concluded rounds, should fail - vote.1 = 10; - assert!(!rounds.add_vote(&vote, alice.clone(), true)); - vote.1 = 11; - assert!(!rounds.add_vote(&vote, alice.clone(), true)); + vote.commitment.block_number = 10; + assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Stale); + vote.commitment.block_number = 11; + assert_eq!(rounds.add_vote(vote.clone()), VoteImportResult::Stale); // no votes present assert!(rounds.rounds.is_empty()); - // add good vote - vote.1 = 12; - assert!(rounds.add_vote(&vote, alice, true)); + // add vote for active round 12 + vote.commitment.block_number = 12; + assert_eq!(rounds.add_vote(vote), VoteImportResult::Ok); // good vote present assert_eq!(rounds.rounds.len(), 1); } @@ -369,88 +383,73 @@ mod tests { sp_tracing::try_init_simple(); let validators = ValidatorSet::::new( - vec![ - Keyring::Alice.public(), - Keyring::Bob.public(), - Keyring::Charlie.public(), - Keyring::Dave.public(), - ], + vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()], Default::default(), ) .unwrap(); + let validator_set_id = validators.id(); let session_start = 1u64.into(); - let mut rounds = Rounds::::new(session_start, validators); - - // round 1 - assert!(rounds.add_vote( - &(H256::from_low_u64_le(1), 1), - (Keyring::Alice.public(), Keyring::Alice.sign(b"I am committed")), - true, - )); - assert!(rounds.add_vote( - &(H256::from_low_u64_le(1), 1), - (Keyring::Bob.public(), Keyring::Bob.sign(b"I am committed")), - false, - )); - assert!(rounds.add_vote( - &(H256::from_low_u64_le(1), 1), - (Keyring::Charlie.public(), Keyring::Charlie.sign(b"I am committed")), - false, - )); - - // round 2 - assert!(rounds.add_vote( - &(H256::from_low_u64_le(2), 2), - (Keyring::Alice.public(), Keyring::Alice.sign(b"I am again committed")), - true, - )); - assert!(rounds.add_vote( - &(H256::from_low_u64_le(2), 2), - (Keyring::Bob.public(), Keyring::Bob.sign(b"I am again committed")), - false, - )); - assert!(rounds.add_vote( - &(H256::from_low_u64_le(2), 2), - (Keyring::Charlie.public(), Keyring::Charlie.sign(b"I am again committed")), - false, - )); - - // round 3 - assert!(rounds.add_vote( - &(H256::from_low_u64_le(3), 3), - (Keyring::Alice.public(), Keyring::Alice.sign(b"I am still committed")), - true, - )); - assert!(rounds.add_vote( - &(H256::from_low_u64_le(3), 3), - (Keyring::Bob.public(), Keyring::Bob.sign(b"I am still committed")), - false, - )); - assert!(rounds.add_vote( - &(H256::from_low_u64_le(3), 3), - (Keyring::Charlie.public(), Keyring::Charlie.sign(b"I am still committed")), - false, - )); - assert_eq!(3, rounds.rounds.len()); - - // conclude unknown round - assert!(rounds.should_conclude(&(H256::from_low_u64_le(5), 5)).is_none()); - assert_eq!(3, rounds.rounds.len()); - - // conclude round 2 - let signatures = rounds.should_conclude(&(H256::from_low_u64_le(2), 2)).unwrap(); - rounds.conclude(2); + let mut rounds = Rounds::::new(session_start, validators); + + let payload = Payload::from_single_entry(MMR_ROOT_ID, vec![]); + let commitment = Commitment { block_number: 1, payload, validator_set_id }; + let mut alice_vote = VoteMessage { + id: Keyring::Alice.public(), + commitment: commitment.clone(), + signature: Keyring::Alice.sign(b"I am committed"), + }; + let mut bob_vote = VoteMessage { + id: Keyring::Bob.public(), + commitment: commitment.clone(), + signature: Keyring::Bob.sign(b"I am committed"), + }; + let mut charlie_vote = VoteMessage { + id: Keyring::Charlie.public(), + commitment, + signature: Keyring::Charlie.sign(b"I am committed"), + }; + let expected_signatures = vec![ + Some(Keyring::Alice.sign(b"I am committed")), + Some(Keyring::Bob.sign(b"I am committed")), + Some(Keyring::Charlie.sign(b"I am committed")), + ]; + + // round 1 - only 2 out of 3 vote + assert_eq!(rounds.add_vote(alice_vote.clone()), VoteImportResult::Ok); + assert_eq!(rounds.add_vote(charlie_vote.clone()), VoteImportResult::Ok); + // should be 1 active round assert_eq!(1, rounds.rounds.len()); + // round 2 - only Charlie votes + charlie_vote.commitment.block_number = 2; + assert_eq!(rounds.add_vote(charlie_vote.clone()), VoteImportResult::Ok); + // should be 2 active rounds + assert_eq!(2, rounds.rounds.len()); + + // round 3 - all validators vote -> round is concluded + alice_vote.commitment.block_number = 3; + bob_vote.commitment.block_number = 3; + charlie_vote.commitment.block_number = 3; + assert_eq!(rounds.add_vote(alice_vote.clone()), VoteImportResult::Ok); + assert_eq!(rounds.add_vote(bob_vote.clone()), VoteImportResult::Ok); assert_eq!( - signatures, - vec![ - Some(Keyring::Alice.sign(b"I am again committed")), - Some(Keyring::Bob.sign(b"I am again committed")), - Some(Keyring::Charlie.sign(b"I am again committed")), - None - ] + rounds.add_vote(charlie_vote.clone()), + VoteImportResult::RoundConcluded(SignedCommitment { + commitment: charlie_vote.commitment, + signatures: expected_signatures + }) ); + // should be only 2 active since this one auto-concluded + assert_eq!(2, rounds.rounds.len()); + + // conclude round 2 + rounds.conclude(2); + // should be no more active rounds since 2 was officially concluded and round "1" is stale + assert!(rounds.rounds.is_empty()); + + // conclude round 3 + rounds.conclude(3); + assert!(rounds.previous_votes.is_empty()); } } diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index 008137d9a9965..69cc9b2f652c4 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -26,12 +26,12 @@ use crate::{ }, gossip_protocol_name, justification::*, - keystore::tests::Keyring as BeefyKeyring, load_or_init_voter_state, wait_for_runtime_pallet, BeefyRPCLinks, BeefyVoterLinks, KnownPeers, PersistedState, }; use beefy_primitives::{ crypto::{AuthorityId, Signature}, + keyring::Keyring as BeefyKeyring, known_payloads, mmr::MmrRootProvider, BeefyApi, Commitment, ConsensusLog, MmrRootHash, Payload, SignedCommitment, ValidatorSet, diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 4d057bffc0cd9..be5443d3dead8 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -26,13 +26,13 @@ use crate::{ keystore::BeefyKeystore, metric_inc, metric_set, metrics::Metrics, - round::Rounds, + round::{Rounds, VoteImportResult}, BeefyVoterLinks, LOG_TARGET, }; use beefy_primitives::{ crypto::{AuthorityId, Signature}, - Commitment, ConsensusLog, Payload, PayloadProvider, SignedCommitment, ValidatorSet, - VersionedFinalityProof, VoteMessage, BEEFY_ENGINE_ID, + Commitment, ConsensusLog, PayloadProvider, ValidatorSet, VersionedFinalityProof, VoteMessage, + BEEFY_ENGINE_ID, }; use codec::{Codec, Decode, Encode}; use futures::{stream::Fuse, FutureExt, StreamExt}; @@ -55,6 +55,7 @@ use std::{ marker::PhantomData, sync::Arc, }; + /// Bound for the number of buffered future voting rounds. const MAX_BUFFERED_VOTE_ROUNDS: usize = 600; /// Bound for the number of buffered votes per round number. @@ -83,17 +84,14 @@ pub(crate) struct VoterOracle { /// 3. lagging behind GRANDPA: queue has [1, N] elements, where all `mandatory_done == false`. /// In this state, everytime a session gets its mandatory block BEEFY finalized, it's /// popped off the queue, eventually getting to state `2. up-to-date`. - sessions: VecDeque>, + sessions: VecDeque>, /// Min delta in block numbers between two blocks, BEEFY should vote on. min_block_delta: u32, } impl VoterOracle { /// Verify provided `sessions` satisfies requirements, then build `VoterOracle`. - pub fn checked_new( - sessions: VecDeque>, - min_block_delta: u32, - ) -> Option { + pub fn checked_new(sessions: VecDeque>, min_block_delta: u32) -> Option { let mut prev_start = Zero::zero(); let mut prev_validator_id = None; // verifies the @@ -136,13 +134,13 @@ impl VoterOracle { // Return reference to rounds pertaining to first session in the queue. // Voting will always happen at the head of the queue. - fn active_rounds(&self) -> Option<&Rounds> { + fn active_rounds(&self) -> Option<&Rounds> { self.sessions.front() } // Return mutable reference to rounds pertaining to first session in the queue. // Voting will always happen at the head of the queue. - fn active_rounds_mut(&mut self) -> Option<&mut Rounds> { + fn active_rounds_mut(&mut self) -> Option<&mut Rounds> { self.sessions.front_mut() } @@ -157,7 +155,7 @@ impl VoterOracle { } /// Add new observed session to the Oracle. - pub fn add_session(&mut self, rounds: Rounds) { + pub fn add_session(&mut self, rounds: Rounds) { self.sessions.push_back(rounds); // Once we add a new session we can drop/prune previous session if it's been finalized. self.try_prune(); @@ -264,6 +262,8 @@ pub(crate) struct PersistedState { best_grandpa_block_header: ::Header, /// Best block a BEEFY voting round has been concluded for. best_beefy_block: NumberFor, + /// Best block we voted on. + best_voted: NumberFor, /// Chooses which incoming votes to accept and which votes to generate. /// Keeps track of voting seen for current and future rounds. voting_oracle: VoterOracle, @@ -273,12 +273,13 @@ impl PersistedState { pub fn checked_new( grandpa_header: ::Header, best_beefy: NumberFor, - sessions: VecDeque>, + sessions: VecDeque>, min_block_delta: u32, ) -> Option { VoterOracle::checked_new(sessions, min_block_delta).map(|voting_oracle| PersistedState { best_grandpa_block_header: grandpa_header, best_beefy_block: best_beefy, + best_voted: Zero::zero(), voting_oracle, }) } @@ -381,7 +382,7 @@ where &self.persisted_state.voting_oracle } - fn active_rounds(&mut self) -> Option<&Rounds> { + fn active_rounds(&mut self) -> Option<&Rounds> { self.persisted_state.voting_oracle.active_rounds() } @@ -486,12 +487,9 @@ where ) -> Result<(), Error> { let block_num = vote.commitment.block_number; let best_grandpa = self.best_grandpa_block(); + self.gossip_validator.note_round(block_num); match self.voting_oracle().triage_round(block_num, best_grandpa)? { - RoundAction::Process => self.handle_vote( - (vote.commitment.payload, vote.commitment.block_number), - (vote.id, vote.signature), - false, - )?, + RoundAction::Process => self.handle_vote(vote)?, RoundAction::Enqueue => { debug!(target: LOG_TARGET, "🥩 Buffer vote for round: {:?}.", block_num); if self.pending_votes.len() < MAX_BUFFERED_VOTE_ROUNDS { @@ -546,57 +544,47 @@ where fn handle_vote( &mut self, - round: (Payload, NumberFor), - vote: (AuthorityId, Signature), - self_vote: bool, + vote: VoteMessage, AuthorityId, Signature>, ) -> Result<(), Error> { - self.gossip_validator.note_round(round.1); - let rounds = self .persisted_state .voting_oracle .active_rounds_mut() .ok_or(Error::UninitSession)?; - if rounds.add_vote(&round, vote, self_vote) { - if let Some(signatures) = rounds.should_conclude(&round) { - self.gossip_validator.conclude_round(round.1); - - let block_num = round.1; - let commitment = Commitment { - payload: round.0, - block_number: block_num, - validator_set_id: rounds.validator_set_id(), - }; - - let finality_proof = - VersionedFinalityProof::V1(SignedCommitment { commitment, signatures }); - - metric_set!(self, beefy_round_concluded, block_num); + let block_number = vote.commitment.block_number; + match rounds.add_vote(vote) { + VoteImportResult::RoundConcluded(signed_commitment) => { + self.gossip_validator.conclude_round(block_number); + metric_set!(self, beefy_round_concluded, block_number); + let finality_proof = VersionedFinalityProof::V1(signed_commitment); info!( target: LOG_TARGET, - "🥩 Round #{} concluded, finality_proof: {:?}.", round.1, finality_proof + "🥩 Round #{} concluded, finality_proof: {:?}.", block_number, finality_proof ); - // We created the `finality_proof` and know to be valid. // New state is persisted after finalization. self.finalize(finality_proof)?; - } else { - let mandatory_round = self + }, + VoteImportResult::Ok => { + // Persist state after handling mandatory block vote. + if self .voting_oracle() .mandatory_pending() - .map(|p| p.0 == round.1) - .unwrap_or(false); - // Persist state after handling self vote to avoid double voting in case - // of voter restarts. - // Also persist state after handling mandatory block vote. - if self_vote || mandatory_round { + .map(|(mandatory_num, _)| mandatory_num == block_number) + .unwrap_or(false) + { crate::aux_schema::write_voter_state(&*self.backend, &self.persisted_state) .map_err(|e| Error::Backend(e.to_string()))?; } - } - } + }, + VoteImportResult::Equivocation => { + // TODO: report returned `EquivocationProof` to chain through `pallet-beefy`. + () + }, + VoteImportResult::Invalid | VoteImportResult::Stale => (), + }; Ok(()) } @@ -695,11 +683,7 @@ where for (num, votes) in votes_to_handle.into_iter() { debug!(target: LOG_TARGET, "🥩 Handle buffered votes for: {:?}.", num); for v in votes.into_iter() { - if let Err(err) = self.handle_vote( - (v.commitment.payload, v.commitment.block_number), - (v.id, v.signature), - false, - ) { + if let Err(err) = self.handle_vote(v) { error!(target: LOG_TARGET, "🥩 Error handling buffered vote: {}", err); }; } @@ -716,7 +700,9 @@ where .voting_target(self.best_beefy_block(), self.best_grandpa_block()) { metric_set!(self, beefy_should_vote_on, target); - self.do_vote(target)?; + if target > self.persisted_state.best_voted { + self.do_vote(target)?; + } } Ok(()) } @@ -766,13 +752,6 @@ where .voting_oracle .active_rounds_mut() .ok_or(Error::UninitSession)?; - if !rounds.should_self_vote(&(payload.clone(), target_number)) { - debug!( - target: LOG_TARGET, - "🥩 Don't double vote for block number: {:?}", target_number - ); - return Ok(()) - } let (validators, validator_set_id) = (rounds.validators(), rounds.validator_set_id()); let authority_id = if let Some(id) = self.key_store.authority_id(validators) { @@ -812,17 +791,17 @@ where debug!(target: LOG_TARGET, "🥩 Sent vote message: {:?}", message); - if let Err(err) = self.handle_vote( - (message.commitment.payload, message.commitment.block_number), - (message.id, message.signature), - true, - ) { + if let Err(err) = self.handle_vote(message) { error!(target: LOG_TARGET, "🥩 Error handling self vote: {}", err); } self.gossip_engine.gossip_message(topic::(), encoded_message, false); - Ok(()) + // Persist state after vote to avoid double voting in case of voter restarts. + self.persisted_state.best_voted = target_number; + metric_set!(self, beefy_best_voted, target_number); + crate::aux_schema::write_voter_state(&*self.backend, &self.persisted_state) + .map_err(|e| Error::Backend(e.to_string())) } fn process_new_state(&mut self) { @@ -848,8 +827,7 @@ where /// Main loop for BEEFY worker. /// - /// Wait for BEEFY runtime pallet to be available, then start the main async loop - /// which is driven by finality notifications and gossiped votes. + /// Run the main async loop which is driven by finality notifications and gossiped votes. pub(crate) async fn run( mut self, mut block_import_justif: Fuse>>, @@ -992,14 +970,15 @@ pub(crate) mod tests { use super::*; use crate::{ communication::notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream}, - keystore::tests::Keyring, tests::{ create_beefy_keystore, get_beefy_streams, make_beefy_ids, BeefyPeer, BeefyTestNet, TestApi, }, BeefyRPCLinks, KnownPeers, }; - use beefy_primitives::{known_payloads, mmr::MmrRootProvider}; + use beefy_primitives::{ + keyring::Keyring, known_payloads, mmr::MmrRootProvider, Payload, SignedCommitment, + }; use futures::{future::poll_fn, task::Poll}; use parking_lot::Mutex; use sc_client_api::{Backend as BackendT, HeaderBackend}; @@ -1007,7 +986,7 @@ pub(crate) mod tests { use sc_network_test::TestNetFactory; use sp_api::HeaderT; use sp_blockchain::Backend as BlockchainBackendT; - use sp_runtime::traits::{One, Zero}; + use sp_runtime::traits::One; use substrate_test_runtime_client::{ runtime::{Block, Digest, DigestItem, Header, H256}, Backend, @@ -1018,7 +997,7 @@ pub(crate) mod tests { &self.voting_oracle } - pub fn active_round(&self) -> Option<&Rounds> { + pub fn active_round(&self) -> Option<&Rounds> { self.voting_oracle.active_rounds() } @@ -1032,7 +1011,7 @@ pub(crate) mod tests { } impl VoterOracle { - pub fn sessions(&self) -> &VecDeque> { + pub fn sessions(&self) -> &VecDeque> { &self.sessions } } @@ -1090,7 +1069,7 @@ pub(crate) mod tests { min_block_delta, ) .unwrap(); - let payload_provider = MmrRootProvider::new(api); + let payload_provider = MmrRootProvider::new(api.clone()); let worker_params = crate::worker::WorkerParams { backend, payload_provider, diff --git a/primitives/beefy/Cargo.toml b/primitives/beefy/Cargo.toml index 6a22f0383a3d0..e4154071d26eb 100644 --- a/primitives/beefy/Cargo.toml +++ b/primitives/beefy/Cargo.toml @@ -22,6 +22,8 @@ sp-io = { version = "7.0.0", default-features = false, path = "../io" } sp-mmr-primitives = { version = "4.0.0-dev", default-features = false, path = "../merkle-mountain-range" } sp-runtime = { version = "7.0.0", default-features = false, path = "../runtime" } sp-std = { version = "5.0.0", default-features = false, path = "../std" } +strum = { version = "0.24.1", features = ["derive"] } +lazy_static = "1.4.0" [dev-dependencies] array-bytes = "4.1" diff --git a/primitives/beefy/src/commitment.rs b/primitives/beefy/src/commitment.rs index 5765ff3609dbb..8642a83aae4be 100644 --- a/primitives/beefy/src/commitment.rs +++ b/primitives/beefy/src/commitment.rs @@ -77,6 +77,7 @@ where self.validator_set_id .cmp(&other.validator_set_id) .then_with(|| self.block_number.cmp(&other.block_number)) + .then_with(|| self.payload.cmp(&other.payload)) } } diff --git a/primitives/beefy/src/lib.rs b/primitives/beefy/src/lib.rs index 8c219040b3523..08467a723b704 100644 --- a/primitives/beefy/src/lib.rs +++ b/primitives/beefy/src/lib.rs @@ -173,7 +173,7 @@ pub enum ConsensusLog { /// /// A vote message is a direct vote created by a BEEFY node on every voting round /// and is gossiped to its peers. -#[derive(Debug, Decode, Encode, TypeInfo)] +#[derive(Clone, Debug, Decode, Encode, PartialEq, TypeInfo)] pub struct VoteMessage { /// Commit to information extracted from a finalized block pub commitment: Commitment, @@ -209,6 +209,83 @@ sp_api::decl_runtime_apis! { } } +#[cfg(feature = "std")] +/// Test accounts using [`crate::crypto`] types. +pub mod keyring { + use super::*; + use sp_core::{ecdsa, keccak_256, Pair}; + use std::collections::HashMap; + use strum::IntoEnumIterator; + + /// Set of test accounts using [`crate::crypto`] types. + #[allow(missing_docs)] + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, strum::Display, strum::EnumIter)] + pub enum Keyring { + Alice, + Bob, + Charlie, + Dave, + Eve, + Ferdie, + One, + Two, + } + + impl Keyring { + /// Sign `msg`. + pub fn sign(self, msg: &[u8]) -> crypto::Signature { + // todo: use custom signature hashing type + let msg = keccak_256(msg); + ecdsa::Pair::from(self).sign_prehashed(&msg).into() + } + + /// Return key pair. + pub fn pair(self) -> crypto::Pair { + ecdsa::Pair::from_string(self.to_seed().as_str(), None).unwrap().into() + } + + /// Return public key. + pub fn public(self) -> crypto::Public { + self.pair().public() + } + + /// Return seed string. + pub fn to_seed(self) -> String { + format!("//{}", self) + } + + /// Get Keyring from public key. + pub fn from_public(who: &crypto::Public) -> Option { + Self::iter().find(|&k| &crypto::Public::from(k) == who) + } + } + + lazy_static::lazy_static! { + static ref PRIVATE_KEYS: HashMap = + Keyring::iter().map(|i| (i, i.pair())).collect(); + static ref PUBLIC_KEYS: HashMap = + PRIVATE_KEYS.iter().map(|(&name, pair)| (name, pair.public())).collect(); + } + + impl From for crypto::Pair { + fn from(k: Keyring) -> Self { + k.pair() + } + } + + impl From for ecdsa::Pair { + fn from(k: Keyring) -> Self { + k.pair().into() + } + } + + impl From for crypto::Public { + fn from(k: Keyring) -> Self { + (*PUBLIC_KEYS).get(&k).cloned().unwrap() + } + } +} + #[cfg(test)] mod tests { use super::*;