diff --git a/client/beefy/src/keystore.rs b/client/beefy/src/keystore.rs index 7ee1ceb46bc35..eaae17e2015af 100644 --- a/client/beefy/src/keystore.rs +++ b/client/beefy/src/keystore.rs @@ -89,8 +89,8 @@ impl BeefyKeystore { let store = self.0.clone().ok_or_else(|| error::Error::Keystore("no Keystore".into()))?; let pk: Vec = SyncCryptoStore::ecdsa_public_keys(&*store, KEY_TYPE) - .iter() - .map(|k| Public::from(k.clone())) + .drain(..) + .map(Public::from) .collect(); Ok(pk) diff --git a/client/beefy/src/round.rs b/client/beefy/src/round.rs index db41f0f465db6..b0df365b4e6f1 100644 --- a/client/beefy/src/round.rs +++ b/client/beefy/src/round.rs @@ -74,15 +74,15 @@ where N: Ord + AtLeast32BitUnsigned + MaybeDisplay + Clone, { pub(crate) fn validator_set_id(&self) -> ValidatorSetId { - self.validator_set.id + self.validator_set.id() } - pub(crate) fn validators(&self) -> Vec { - self.validator_set.validators.clone() + pub(crate) fn validators(&self) -> &[Public] { + self.validator_set.validators() } pub(crate) fn add_vote(&mut self, round: &(H, N), vote: (Public, Signature)) -> bool { - if self.validator_set.validators.iter().any(|id| vote.0 == *id) { + if self.validator_set.validators().iter().any(|id| vote.0 == *id) { self.rounds.entry(round.clone()).or_default().add_vote(vote) } else { false @@ -93,7 +93,7 @@ where let done = self .rounds .get(round) - .map(|tracker| tracker.is_done(threshold(self.validator_set.validators.len()))) + .map(|tracker| tracker.is_done(threshold(self.validator_set.len()))) .unwrap_or(false); debug!(target: "beefy", "🥩 Round #{} done: {}", round.1, done); @@ -108,7 +108,7 @@ where Some( self.validator_set - .validators + .validators() .iter() .map(|authority_id| { signatures.iter().find_map(|(id, sig)| { @@ -139,26 +139,18 @@ mod tests { fn new_rounds() { sp_tracing::try_init_simple(); - let rounds = Rounds::>::new(ValidatorSet::::empty()); - - assert_eq!(0, rounds.validator_set_id()); - assert!(rounds.validators().is_empty()); - - let validators = ValidatorSet:: { - validators: vec![ - Keyring::Alice.public(), - Keyring::Bob.public(), - Keyring::Charlie.public(), - ], - id: 42, - }; + let validators = ValidatorSet::::new( + vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()], + 42, + ) + .unwrap(); let rounds = Rounds::>::new(validators); assert_eq!(42, rounds.validator_set_id()); assert_eq!( - vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()], + &vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()], rounds.validators() ); } @@ -167,14 +159,11 @@ mod tests { fn add_vote() { sp_tracing::try_init_simple(); - let validators = ValidatorSet:: { - validators: vec![ - Keyring::Alice.public(), - Keyring::Bob.public(), - Keyring::Charlie.public(), - ], - id: Default::default(), - }; + let validators = ValidatorSet::::new( + vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()], + Default::default(), + ) + .unwrap(); let mut rounds = Rounds::>::new(validators); @@ -212,14 +201,11 @@ mod tests { fn drop() { sp_tracing::try_init_simple(); - let validators = ValidatorSet:: { - validators: vec![ - Keyring::Alice.public(), - Keyring::Bob.public(), - Keyring::Charlie.public(), - ], - id: Default::default(), - }; + let validators = ValidatorSet::::new( + vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()], + Default::default(), + ) + .unwrap(); let mut rounds = Rounds::>::new(validators); diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index bf29a50f19640..306540077ae2f 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -20,7 +20,7 @@ use std::{collections::BTreeSet, fmt::Debug, marker::PhantomData, sync::Arc}; use codec::{Codec, Decode, Encode}; use futures::{future, FutureExt, StreamExt}; -use log::{debug, error, info, trace, warn}; +use log::{debug, error, info, log_enabled, trace, warn}; use parking_lot::Mutex; use sc_client_api::{Backend, FinalityNotification, FinalityNotifications}; @@ -79,7 +79,7 @@ where /// Min delta in block numbers between two blocks, BEEFY should vote on min_block_delta: u32, metrics: Option, - rounds: round::Rounds>, + rounds: Option>>, finality_notifications: FinalityNotifications, /// Best block we received a GRANDPA notification for best_grandpa_block: NumberFor, @@ -125,7 +125,7 @@ where gossip_validator, min_block_delta, metrics, - rounds: round::Rounds::new(ValidatorSet::empty()), + rounds: None, finality_notifications: client.finality_notification_stream(), best_grandpa_block: client.info().finalized_number, best_beefy_block: None, @@ -172,7 +172,7 @@ where Some(new) } else { let at = BlockId::hash(header.hash()); - self.client.runtime_api().validator_set(&at).ok() + self.client.runtime_api().validator_set(&at).ok().flatten() }; trace!(target: "beefy", "🥩 active validator set: {:?}", new); @@ -190,11 +190,12 @@ where fn verify_validator_set( &self, block: &NumberFor, - mut active: ValidatorSet, + active: &ValidatorSet, ) -> Result<(), error::Error> { - let active: BTreeSet = active.validators.drain(..).collect(); + let active: BTreeSet<&Public> = active.validators().iter().collect(); - let store: BTreeSet = self.key_store.public_keys()?.drain(..).collect(); + let public_keys = self.key_store.public_keys()?; + let store: BTreeSet<&Public> = public_keys.iter().collect(); let missing: Vec<_> = store.difference(&active).cloned().collect(); @@ -214,26 +215,31 @@ where if let Some(active) = self.validator_set(¬ification.header) { // Authority set change or genesis set id triggers new voting rounds // - // TODO: (adoerr) Enacting a new authority set will also implicitly 'conclude' - // the currently active BEEFY voting round by starting a new one. This is - // temporary and needs to be replaced by proper round life cycle handling. - if active.id != self.rounds.validator_set_id() || - (active.id == GENESIS_AUTHORITY_SET_ID && self.best_beefy_block.is_none()) + // TODO: (grandpa-bridge-gadget#366) Enacting a new authority set will also + // implicitly 'conclude' the currently active BEEFY voting round by starting a + // new one. This should be replaced by proper round life-cycle handling. + if self.rounds.is_none() || + active.id() != self.rounds.as_ref().unwrap().validator_set_id() || + (active.id() == GENESIS_AUTHORITY_SET_ID && self.best_beefy_block.is_none()) { debug!(target: "beefy", "🥩 New active validator set id: {:?}", active); - metric_set!(self, beefy_validator_set_id, active.id); + metric_set!(self, beefy_validator_set_id, active.id()); // BEEFY should produce a signed commitment for each session - if active.id != self.last_signed_id + 1 && active.id != GENESIS_AUTHORITY_SET_ID { + if active.id() != self.last_signed_id + 1 && active.id() != GENESIS_AUTHORITY_SET_ID + { metric_inc!(self, beefy_skipped_sessions); } - // verify the new validator set - let _ = self.verify_validator_set(notification.header.number(), active.clone()); + if log_enabled!(target: "beefy", log::Level::Debug) { + // verify the new validator set - only do it if we're also logging the warning + let _ = self.verify_validator_set(notification.header.number(), &active); + } - self.rounds = round::Rounds::new(active.clone()); + let id = active.id(); + self.rounds = Some(round::Rounds::new(active)); - debug!(target: "beefy", "🥩 New Rounds for id: {:?}", active.id); + debug!(target: "beefy", "🥩 New Rounds for id: {:?}", id); self.best_beefy_block = Some(*notification.header.number()); @@ -244,9 +250,13 @@ where } if self.should_vote_on(*notification.header.number()) { - let authority_id = if let Some(id) = - self.key_store.authority_id(self.rounds.validators().as_slice()) - { + let (validators, validator_set_id) = if let Some(rounds) = &self.rounds { + (rounds.validators(), rounds.validator_set_id()) + } else { + debug!(target: "beefy", "🥩 Missing validator set - can't vote for: {:?}", notification.header.hash()); + return + }; + let authority_id = if let Some(id) = self.key_store.authority_id(validators) { debug!(target: "beefy", "🥩 Local authority id: {:?}", id); id } else { @@ -266,7 +276,7 @@ where let commitment = Commitment { payload, block_number: notification.header.number(), - validator_set_id: self.rounds.validator_set_id(), + validator_set_id, }; let encoded_commitment = commitment.encode(); @@ -305,12 +315,19 @@ where fn handle_vote(&mut self, round: (Payload, NumberFor), vote: (Public, Signature)) { self.gossip_validator.note_round(round.1); - let vote_added = self.rounds.add_vote(&round, vote); + let rounds = if let Some(rounds) = self.rounds.as_mut() { + rounds + } else { + debug!(target: "beefy", "🥩 Missing validator set - can't handle vote {:?}", vote); + return + }; + + let vote_added = rounds.add_vote(&round, vote); - if vote_added && self.rounds.is_done(&round) { - if let Some(signatures) = self.rounds.drop(&round) { + if vote_added && rounds.is_done(&round) { + if let Some(signatures) = rounds.drop(&round) { // id is stored for skipped session metric calculation - self.last_signed_id = self.rounds.validator_set_id(); + self.last_signed_id = rounds.validator_set_id(); let commitment = Commitment { payload: round.0, diff --git a/frame/beefy-mmr/src/tests.rs b/frame/beefy-mmr/src/tests.rs index f27bc450ad146..9cfbd94b53bca 100644 --- a/frame/beefy-mmr/src/tests.rs +++ b/frame/beefy-mmr/src/tests.rs @@ -84,10 +84,9 @@ fn should_contain_mmr_digest() { beefy_log(ConsensusLog::MmrRoot( hex!("f3e3afbfa69e89cd1e99f8d3570155962f3346d1d8758dc079be49ef70387758").into() )), - beefy_log(ConsensusLog::AuthoritiesChange(ValidatorSet { - validators: vec![mock_beefy_id(3), mock_beefy_id(4),], - id: 1, - })), + beefy_log(ConsensusLog::AuthoritiesChange( + ValidatorSet::new(vec![mock_beefy_id(3), mock_beefy_id(4),], 1,).unwrap() + )), beefy_log(ConsensusLog::MmrRoot( hex!("7d4ae4524bae75d52b63f08eab173b0c263eb95ae2c55c3a1d871241bd0cc559").into() )), diff --git a/frame/beefy/src/lib.rs b/frame/beefy/src/lib.rs index c18b28901bb62..9ef0c0d69f305 100644 --- a/frame/beefy/src/lib.rs +++ b/frame/beefy/src/lib.rs @@ -97,8 +97,10 @@ pub mod pallet { impl Pallet { /// Return the current active BEEFY validator set. - pub fn validator_set() -> ValidatorSet { - ValidatorSet:: { validators: Self::authorities(), id: Self::validator_set_id() } + pub fn validator_set() -> Option> { + let validators: Vec = Self::authorities(); + let id: beefy_primitives::ValidatorSetId = Self::validator_set_id(); + ValidatorSet::::new(validators, id) } fn change_authorities(new: Vec, queued: Vec) { @@ -109,13 +111,13 @@ impl Pallet { let next_id = Self::validator_set_id() + 1u64; >::put(next_id); - - let log = DigestItem::Consensus( - BEEFY_ENGINE_ID, - ConsensusLog::AuthoritiesChange(ValidatorSet { validators: new, id: next_id }) - .encode(), - ); - >::deposit_log(log); + if let Some(validator_set) = ValidatorSet::::new(new, next_id) { + let log = DigestItem::Consensus( + BEEFY_ENGINE_ID, + ConsensusLog::AuthoritiesChange(validator_set).encode(), + ); + >::deposit_log(log); + } } >::put(&queued); diff --git a/frame/beefy/src/tests.rs b/frame/beefy/src/tests.rs index 252c03efb54a9..900a3770279be 100644 --- a/frame/beefy/src/tests.rs +++ b/frame/beefy/src/tests.rs @@ -70,10 +70,9 @@ fn session_change_updates_authorities() { assert!(1 == Beefy::validator_set_id()); - let want = beefy_log(ConsensusLog::AuthoritiesChange(ValidatorSet { - validators: vec![mock_beefy_id(3), mock_beefy_id(4)], - id: 1, - })); + let want = beefy_log(ConsensusLog::AuthoritiesChange( + ValidatorSet::new(vec![mock_beefy_id(3), mock_beefy_id(4)], 1).unwrap(), + )); let log = System::digest().logs[0].clone(); @@ -109,11 +108,11 @@ fn validator_set_at_genesis() { let want = vec![mock_beefy_id(1), mock_beefy_id(2)]; new_test_ext(vec![1, 2, 3, 4]).execute_with(|| { - let vs = Beefy::validator_set(); + let vs = Beefy::validator_set().unwrap(); - assert_eq!(vs.id, 0u64); - assert_eq!(vs.validators[0], want[0]); - assert_eq!(vs.validators[1], want[1]); + assert_eq!(vs.id(), 0u64); + assert_eq!(vs.validators()[0], want[0]); + assert_eq!(vs.validators()[1], want[1]); }); } @@ -124,18 +123,18 @@ fn validator_set_updates_work() { new_test_ext(vec![1, 2, 3, 4]).execute_with(|| { init_block(1); - let vs = Beefy::validator_set(); + let vs = Beefy::validator_set().unwrap(); - assert_eq!(vs.id, 0u64); - assert_eq!(want[0], vs.validators[0]); - assert_eq!(want[1], vs.validators[1]); + assert_eq!(vs.id(), 0u64); + assert_eq!(want[0], vs.validators()[0]); + assert_eq!(want[1], vs.validators()[1]); init_block(2); - let vs = Beefy::validator_set(); + let vs = Beefy::validator_set().unwrap(); - assert_eq!(vs.id, 1u64); - assert_eq!(want[2], vs.validators[0]); - assert_eq!(want[3], vs.validators[1]); + assert_eq!(vs.id(), 1u64); + assert_eq!(want[2], vs.validators()[0]); + assert_eq!(want[3], vs.validators()[1]); }); } diff --git a/primitives/beefy/src/lib.rs b/primitives/beefy/src/lib.rs index 20f6b9d0d8fb4..bf72b801eec0a 100644 --- a/primitives/beefy/src/lib.rs +++ b/primitives/beefy/src/lib.rs @@ -84,15 +84,39 @@ pub type ValidatorSetId = u64; #[derive(Decode, Encode, Debug, PartialEq, Clone, TypeInfo)] pub struct ValidatorSet { /// Public keys of the validator set elements - pub validators: Vec, + validators: Vec, /// Identifier of the validator set - pub id: ValidatorSetId, + id: ValidatorSetId, } impl ValidatorSet { - /// Return an empty validator set with id of 0. - pub fn empty() -> Self { - Self { validators: Default::default(), id: Default::default() } + /// Return a validator set with the given validators and set id. + pub fn new(validators: I, id: ValidatorSetId) -> Option + where + I: IntoIterator, + { + let validators: Vec = validators.into_iter().collect(); + if validators.is_empty() { + // No validators; the set would be empty. + None + } else { + Some(Self { validators, id }) + } + } + + /// Return a reference to the vec of validators. + pub fn validators(&self) -> &[AuthorityId] { + &self.validators + } + + /// Return the validator set id. + pub fn id(&self) -> ValidatorSetId { + self.id + } + + /// Return the number of validators in the set. + pub fn len(&self) -> usize { + self.validators.len() } } @@ -135,6 +159,26 @@ sp_api::decl_runtime_apis! { pub trait BeefyApi { /// Return the current active BEEFY validator set - fn validator_set() -> ValidatorSet; + fn validator_set() -> Option>; + } +} + +#[cfg(test)] +mod tests { + use super::*; + use sp_application_crypto::ecdsa::{self, Public}; + use sp_core::Pair; + + #[test] + fn validator_set() { + // Empty set not allowed. + assert_eq!(ValidatorSet::::new(vec![], 0), None); + + let alice = ecdsa::Pair::from_string("//Alice", None).unwrap(); + let set_id = 0; + let validators = ValidatorSet::::new(vec![alice.public()], set_id).unwrap(); + + assert_eq!(validators.id(), set_id); + assert_eq!(validators.validators(), &vec![alice.public()]); } }