Beefy: Provide well-formed ValidatorSet (#10445)

* beefy: provide well-formed ValidatorSet

* pallet-beefy: use well-formed ValidatorSet

* pallet-beefy-mmr: use well-formed ValidatorSet

* beefy-gadget: fail votes early when ValidatorSet empty

* beefy: small efficiency improvements

* address review comments

Signed-off-by: acatangiu <adrian@parity.io>

substrate/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<Public> = SyncCryptoStore::ecdsa_public_keys(&*store, KEY_TYPE)
-			.iter()
+			.drain(..)
-			.map(|k| Public::from(k.clone()))
+			.map(Public::from)
 			.collect();
 
 		Ok(pk)

substrate/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<Public> {
+	pub(crate) fn validators(&self) -> &[Public] {
-		self.validator_set.validators.clone()
+		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::<H256, NumberFor<Block>>::new(ValidatorSet::<Public>::empty());
+		let validators = ValidatorSet::<Public>::new(
-
+			vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()],
-		assert_eq!(0, rounds.validator_set_id());
+			42,
-		assert!(rounds.validators().is_empty());
+		)
-
+		.unwrap();
-		let validators = ValidatorSet::<Public> {
-			validators: vec![
-				Keyring::Alice.public(),
-				Keyring::Bob.public(),
-				Keyring::Charlie.public(),
-			],
-			id: 42,
-		};
 
 		let rounds = Rounds::<H256, NumberFor<Block>>::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::<Public> {
+		let validators = ValidatorSet::<Public>::new(
-			validators: vec![
+			vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()],
-				Keyring::Alice.public(),
+			Default::default(),
-				Keyring::Bob.public(),
+		)
-				Keyring::Charlie.public(),
+		.unwrap();
-			],
-			id: Default::default(),
-		};
 
 		let mut rounds = Rounds::<H256, NumberFor<Block>>::new(validators);
 
@@ -212,14 +201,11 @@ mod tests {
 	fn drop() {
 		sp_tracing::try_init_simple();
 
-		let validators = ValidatorSet::<Public> {
+		let validators = ValidatorSet::<Public>::new(
-			validators: vec![
+			vec![Keyring::Alice.public(), Keyring::Bob.public(), Keyring::Charlie.public()],
-				Keyring::Alice.public(),
+			Default::default(),
-				Keyring::Bob.public(),
+		)
-				Keyring::Charlie.public(),
+		.unwrap();
-			],
-			id: Default::default(),
-		};
 
 		let mut rounds = Rounds::<H256, NumberFor<Block>>::new(validators);
 

substrate/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<Metrics>,
-	rounds: round::Rounds<Payload, NumberFor<B>>,
+	rounds: Option<round::Rounds<Payload, NumberFor<B>>>,
 	finality_notifications: FinalityNotifications<B>,
 	/// Best block we received a GRANDPA notification for
 	best_grandpa_block: NumberFor<B>,
@@ -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<B>,
-		mut active: ValidatorSet<Public>,
+		active: &ValidatorSet<Public>,
 	) -> Result<(), error::Error> {
-		let active: BTreeSet<Public> = active.validators.drain(..).collect();
+		let active: BTreeSet<&Public> = active.validators().iter().collect();
 
-		let store: BTreeSet<Public> = 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(&notification.header) {
 			// Authority set change or genesis set id triggers new voting rounds
 			//
-			// TODO: (adoerr) Enacting a new authority set will also implicitly 'conclude'
+			// TODO: (grandpa-bridge-gadget#366) Enacting a new authority set will also
-			// the currently active BEEFY voting round by starting a new one. This is
+			// implicitly 'conclude' the currently active BEEFY voting round by starting a
-			// temporary and needs to be replaced by proper round life cycle handling.
+			// new one. This should be replaced by proper round life-cycle handling.
-			if active.id != self.rounds.validator_set_id() ||
+			if self.rounds.is_none() ||
-				(active.id == GENESIS_AUTHORITY_SET_ID && self.best_beefy_block.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
+				if log_enabled!(target: "beefy", log::Level::Debug) {
-				let _ = self.verify_validator_set(notification.header.number(), active.clone());
+					// 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) =
+			let (validators, validator_set_id) = if let Some(rounds) = &self.rounds {
-				self.key_store.authority_id(self.rounds.validators().as_slice())
+				(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<B>), 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
+		};
 
-		if vote_added && self.rounds.is_done(&round) {
+		let vote_added = rounds.add_vote(&round, vote);
-			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,

substrate/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 {
+				beefy_log(ConsensusLog::AuthoritiesChange(
-					validators: vec![mock_beefy_id(3), mock_beefy_id(4),],
+					ValidatorSet::new(vec![mock_beefy_id(3), mock_beefy_id(4),], 1,).unwrap()
-					id: 1,
+				)),
-				})),
 				beefy_log(ConsensusLog::MmrRoot(
 					hex!("7d4ae4524bae75d52b63f08eab173b0c263eb95ae2c55c3a1d871241bd0cc559").into()
 				)),

substrate/frame/beefy/src/lib.rs

@@ -97,8 +97,10 @@ pub mod pallet {
 
 impl<T: Config> Pallet<T> {
 	/// Return the current active BEEFY validator set.
-	pub fn validator_set() -> ValidatorSet<T::BeefyId> {
+	pub fn validator_set() -> Option<ValidatorSet<T::BeefyId>> {
-		ValidatorSet::<T::BeefyId> { validators: Self::authorities(), id: Self::validator_set_id() }
+		let validators: Vec<T::BeefyId> = Self::authorities();
+		let id: beefy_primitives::ValidatorSetId = Self::validator_set_id();
+		ValidatorSet::<T::BeefyId>::new(validators, id)
 	}
 
 	fn change_authorities(new: Vec<T::BeefyId>, queued: Vec<T::BeefyId>) {
@@ -109,13 +111,13 @@ impl<T: Config> Pallet<T> {
 
 			let next_id = Self::validator_set_id() + 1u64;
 			<ValidatorSetId<T>>::put(next_id);
-
+			if let Some(validator_set) = ValidatorSet::<T::BeefyId>::new(new, next_id) {
-			let log = DigestItem::Consensus(
+				let log = DigestItem::Consensus(
-				BEEFY_ENGINE_ID,
+					BEEFY_ENGINE_ID,
-				ConsensusLog::AuthoritiesChange(ValidatorSet { validators: new, id: next_id })
+					ConsensusLog::AuthoritiesChange(validator_set).encode(),
-					.encode(),
+				);
-			);
+				<frame_system::Pallet<T>>::deposit_log(log);
-			<frame_system::Pallet<T>>::deposit_log(log);
+			}
 		}
 
 		<NextAuthorities<T>>::put(&queued);

substrate/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 {
+		let want = beefy_log(ConsensusLog::AuthoritiesChange(
-			validators: vec![mock_beefy_id(3), mock_beefy_id(4)],
+			ValidatorSet::new(vec![mock_beefy_id(3), mock_beefy_id(4)], 1).unwrap(),
-			id: 1,
+		));
-		}));
 
 		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.id(), 0u64);
-		assert_eq!(vs.validators[0], want[0]);
+		assert_eq!(vs.validators()[0], want[0]);
-		assert_eq!(vs.validators[1], want[1]);
+		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!(vs.id(), 0u64);
-		assert_eq!(want[0], vs.validators[0]);
+		assert_eq!(want[0], vs.validators()[0]);
-		assert_eq!(want[1], vs.validators[1]);
+		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!(vs.id(), 1u64);
-		assert_eq!(want[2], vs.validators[0]);
+		assert_eq!(want[2], vs.validators()[0]);
-		assert_eq!(want[3], vs.validators[1]);
+		assert_eq!(want[3], vs.validators()[1]);
 	});
 }

substrate/primitives/beefy/src/lib.rs

@@ -84,15 +84,39 @@ pub type ValidatorSetId = u64;
 #[derive(Decode, Encode, Debug, PartialEq, Clone, TypeInfo)]
 pub struct ValidatorSet<AuthorityId> {
 	/// Public keys of the validator set elements
-	pub validators: Vec<AuthorityId>,
+	validators: Vec<AuthorityId>,
 	/// Identifier of the validator set
-	pub id: ValidatorSetId,
+	id: ValidatorSetId,
 }
 
 impl<AuthorityId> ValidatorSet<AuthorityId> {
-	/// Return an empty validator set with id of 0.
+	/// Return a validator set with the given validators and set id.
-	pub fn empty() -> Self {
+	pub fn new<I>(validators: I, id: ValidatorSetId) -> Option<Self>
-		Self { validators: Default::default(), id: Default::default() }
+	where
+		I: IntoIterator<Item = AuthorityId>,
+	{
+		let validators: Vec<AuthorityId> = 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<crypto::AuthorityId>;
+		fn validator_set() -> Option<ValidatorSet<crypto::AuthorityId>>;
+	}
+}
+
+#[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::<Public>::new(vec![], 0), None);
+
+		let alice = ecdsa::Pair::from_string("//Alice", None).unwrap();
+		let set_id = 0;
+		let validators = ValidatorSet::<Public>::new(vec![alice.public()], set_id).unwrap();
+
+		assert_eq!(validators.id(), set_id);
+		assert_eq!(validators.validators(), &vec![alice.public()]);
 	}
 }