beefy: error logs for validators with dummy keys (paritytech#3939)

This outputs: ``` 2024-04-02 14:36:02.135 ERROR tokio-runtime-worker beefy: 🥩 for session starting at block 21990151 no BEEFY authority key found in store, you must generate valid session keys (https://wiki.polkadot.network/docs/maintain-guides-how-to-validate-polkadot#generating-the-session-keys) ``` error log entry, once every session, for nodes running with `Role::Authority` that have no public BEEFY key in their keystore --------- Co-authored-by: Bastian Köcher <[email protected]>
acatangiu · Apr 2, 2024 · 5eff3f9 · 5eff3f9
1 parent d5617cf
commit 5eff3f9
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 73 deletions.
diff --git a/polkadot/node/service/src/lib.rs b/polkadot/node/service/src/lib.rs
@@ -1233,6 +1233,7 @@ pub fn new_full<OverseerGenerator: OverseerGen>(
 			prometheus_registry: prometheus_registry.clone(),
 			links: beefy_links,
 			on_demand_justifications_handler: beefy_on_demand_justifications_handler,
+			is_authority: role.is_authority(),
 		};
 
 		let gadget = beefy::start_beefy_gadget::<_, _, _, _, _, _, _>(beefy_params);
@@ -1242,18 +1243,18 @@ pub fn new_full<OverseerGenerator: OverseerGen>(
 		task_manager
 			.spawn_essential_handle()
 			.spawn_blocking("beefy-gadget", None, gadget);
-		// When offchain indexing is enabled, MMR gadget should also run.
-		if is_offchain_indexing_enabled {
-			task_manager.spawn_essential_handle().spawn_blocking(
-				"mmr-gadget",
-				None,
-				MmrGadget::start(
-					client.clone(),
-					backend.clone(),
-					sp_mmr_primitives::INDEXING_PREFIX.to_vec(),
-				),
-			);
-		}
+	}
+	// When offchain indexing is enabled, MMR gadget should also run.
+	if is_offchain_indexing_enabled {
+		task_manager.spawn_essential_handle().spawn_blocking(
+			"mmr-gadget",
+			None,
+			MmrGadget::start(
+				client.clone(),
+				backend.clone(),
+				sp_mmr_primitives::INDEXING_PREFIX.to_vec(),
+			),
+		);
 	}
 
 	let config = grandpa::Config {

diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs
@@ -652,6 +652,7 @@ pub fn new_full_base(
 		prometheus_registry: prometheus_registry.clone(),
 		links: beefy_links,
 		on_demand_justifications_handler: beefy_on_demand_justifications_handler,
+		is_authority: role.is_authority(),
 	};
 
 	let beefy_gadget = beefy::start_beefy_gadget::<_, _, _, _, _, _, _>(beefy_params);

diff --git a/substrate/client/consensus/beefy/src/lib.rs b/substrate/client/consensus/beefy/src/lib.rs
@@ -222,6 +222,8 @@ pub struct BeefyParams<B: Block, BE, C, N, P, R, S> {
 	pub links: BeefyVoterLinks<B>,
 	/// Handler for incoming BEEFY justifications requests from a remote peer.
 	pub on_demand_justifications_handler: BeefyJustifsRequestHandler<B, C>,
+	/// Whether running under "Authority" role.
+	pub is_authority: bool,
 }
 /// Helper object holding BEEFY worker communication/gossip components.
 ///
@@ -270,6 +272,7 @@ where
 		min_block_delta: u32,
 		gossip_validator: Arc<GossipValidator<B>>,
 		finality_notifications: &mut Fuse<FinalityNotifications<B>>,
+		is_authority: bool,
 	) -> Result<Self, Error> {
 		// Wait for BEEFY pallet to be active before starting voter.
 		let (beefy_genesis, best_grandpa) =
@@ -283,6 +286,7 @@ where
 			runtime.clone(),
 			&key_store,
 			&metrics,
+			is_authority,
 		)
 		.await?;
 		// Update the gossip validator with the right starting round and set id.
@@ -301,6 +305,7 @@ where
 		comms: BeefyComms<B>,
 		links: BeefyVoterLinks<B>,
 		pending_justifications: BTreeMap<NumberFor<B>, BeefyVersionedFinalityProof<B>>,
+		is_authority: bool,
 	) -> BeefyWorker<B, BE, P, R, S> {
 		BeefyWorker {
 			backend: self.backend,
@@ -313,6 +318,7 @@ where
 			comms,
 			links,
 			pending_justifications,
+			is_authority,
 		}
 	}
 
@@ -423,6 +429,7 @@ where
 		runtime: Arc<R>,
 		key_store: &BeefyKeystore<AuthorityId>,
 		metrics: &Option<VoterMetrics>,
+		is_authority: bool,
 	) -> Result<PersistedState<B>, Error> {
 		// Initialize voter state from AUX DB if compatible.
 		if let Some(mut state) = crate::aux_schema::load_persistent(backend.as_ref())?
@@ -455,7 +462,13 @@ where
 					"🥩 Handling missed BEEFY session after node restart: {:?}.",
 					new_session_start
 				);
-				state.init_session_at(new_session_start, validator_set, key_store, metrics);
+				state.init_session_at(
+					new_session_start,
+					validator_set,
+					key_store,
+					metrics,
+					is_authority,
+				);
 			}
 			return Ok(state)
 		}
@@ -491,6 +504,7 @@ pub async fn start_beefy_gadget<B, BE, C, N, P, R, S>(
 		prometheus_registry,
 		links,
 		mut on_demand_justifications_handler,
+		is_authority,
 	} = beefy_params;
 
 	let BeefyNetworkParams {
@@ -553,6 +567,7 @@ pub async fn start_beefy_gadget<B, BE, C, N, P, R, S>(
 					min_block_delta,
 					beefy_comms.gossip_validator.clone(),
 					&mut finality_notifications,
+					is_authority,
 				).fuse() => {
 					match builder_init_result {
 						Ok(builder) => break builder,
@@ -580,6 +595,7 @@ pub async fn start_beefy_gadget<B, BE, C, N, P, R, S>(
 			beefy_comms,
 			links.clone(),
 			BTreeMap::new(),
+			is_authority,
 		);
 
 		match futures::future::select(

diff --git a/substrate/client/consensus/beefy/src/tests.rs b/substrate/client/consensus/beefy/src/tests.rs
@@ -379,6 +379,7 @@ async fn voter_init_setup(
 		Arc::new(api.clone()),
 		&key_store,
 		&metrics,
+		true,
 	)
 	.await
 }
@@ -438,6 +439,7 @@ where
 			min_block_delta,
 			prometheus_registry: None,
 			on_demand_justifications_handler: on_demand_justif_handler,
+			is_authority: true,
 		};
 		let task = crate::start_beefy_gadget::<_, _, _, _, _, _, _>(beefy_params);
 

diff --git a/substrate/client/consensus/beefy/src/worker.rs b/substrate/client/consensus/beefy/src/worker.rs
@@ -33,7 +33,7 @@ use crate::{
 };
 use codec::{Codec, Decode, DecodeAll, Encode};
 use futures::{stream::Fuse, FutureExt, StreamExt};
-use log::{debug, error, info, log_enabled, trace, warn};
+use log::{debug, error, info, trace, warn};
 use sc_client_api::{Backend, FinalityNotification, FinalityNotifications, HeaderBackend};
 use sc_utils::notification::NotificationReceiver;
 use sp_api::ProvideRuntimeApi;
@@ -51,7 +51,7 @@ use sp_runtime::{
 	SaturatedConversion,
 };
 use std::{
-	collections::{BTreeMap, BTreeSet, VecDeque},
+	collections::{BTreeMap, VecDeque},
 	fmt::Debug,
 	sync::Arc,
 };
@@ -332,6 +332,7 @@ impl<B: Block> PersistedState<B> {
 		validator_set: ValidatorSet<AuthorityId>,
 		key_store: &BeefyKeystore<AuthorityId>,
 		metrics: &Option<VoterMetrics>,
+		is_authority: bool,
 	) {
 		debug!(target: LOG_TARGET, "🥩 New active validator set: {:?}", validator_set);
 
@@ -348,11 +349,16 @@ impl<B: Block> PersistedState<B> {
 			}
 		}
 
-		if log_enabled!(target: LOG_TARGET, log::Level::Debug) {
-			// verify the new validator set - only do it if we're also logging the warning
-			if verify_validator_set::<B>(&new_session_start, &validator_set, key_store).is_err() {
-				metric_inc!(metrics, beefy_no_authority_found_in_store);
-			}
+		// verify we have some BEEFY key available in keystore when role is authority.
+		if is_authority && key_store.public_keys().map_or(false, |k| k.is_empty()) {
+			error!(
+				target: LOG_TARGET,
+				"🥩 for session starting at block {:?} no BEEFY authority key found in store, \
+				you must generate valid session keys \
+				(https://wiki.polkadot.network/docs/maintain-guides-how-to-validate-polkadot#generating-the-session-keys)",
+				new_session_start,
+			);
+			metric_inc!(metrics, beefy_no_authority_found_in_store);
 		}
 
 		let id = validator_set.id();
@@ -390,6 +396,8 @@ pub(crate) struct BeefyWorker<B: Block, BE, P, RuntimeApi, S> {
 	pub persisted_state: PersistedState<B>,
 	/// BEEFY voter metrics
 	pub metrics: Option<VoterMetrics>,
+	/// Node runs under "Authority" role.
+	pub is_authority: bool,
 }
 
 impl<B, BE, P, R, S> BeefyWorker<B, BE, P, R, S>
@@ -425,6 +433,7 @@ where
 			validator_set,
 			&self.key_store,
 			&self.metrics,
+			self.is_authority,
 		);
 	}
 
@@ -1040,33 +1049,6 @@ where
 	}
 }
 
-/// Verify `active` validator set for `block` against the key store
-///
-/// We want to make sure that we have _at least one_ key in our keystore that
-/// is part of the validator set, that's because if there are no local keys
-/// then we can't perform our job as a validator.
-///
-/// Note that for a non-authority node there will be no keystore, and we will
-/// return an error and don't check. The error can usually be ignored.
-fn verify_validator_set<B: Block>(
-	block: &NumberFor<B>,
-	active: &ValidatorSet<AuthorityId>,
-	key_store: &BeefyKeystore<AuthorityId>,
-) -> Result<(), Error> {
-	let active: BTreeSet<&AuthorityId> = active.validators().iter().collect();
-
-	let public_keys = key_store.public_keys()?;
-	let store: BTreeSet<&AuthorityId> = public_keys.iter().collect();
-
-	if store.intersection(&active).count() == 0 {
-		let msg = "no authority public key found in store".to_string();
-		debug!(target: LOG_TARGET, "🥩 for block {:?} {}", block, msg);
-		Err(Error::Keystore(msg))
-	} else {
-		Ok(())
-	}
-}
-
 #[cfg(test)]
 pub(crate) mod tests {
 	use super::*;
@@ -1208,6 +1190,7 @@ pub(crate) mod tests {
 			comms,
 			pending_justifications: BTreeMap::new(),
 			persisted_state,
+			is_authority: true,
 		}
 	}
 
@@ -1471,32 +1454,6 @@ pub(crate) mod tests {
 		assert_eq!(extracted, Some(validator_set));
 	}
 
-	#[tokio::test]
-	async fn keystore_vs_validator_set() {
-		let keys = &[Keyring::Alice];
-		let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap();
-		let mut net = BeefyTestNet::new(1);
-		let mut worker = create_beefy_worker(net.peer(0), &keys[0], 1, validator_set.clone());
-
-		// keystore doesn't contain other keys than validators'
-		assert_eq!(verify_validator_set::<Block>(&1, &validator_set, &worker.key_store), Ok(()));
-
-		// unknown `Bob` key
-		let keys = &[Keyring::Bob];
-		let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap();
-		let err_msg = "no authority public key found in store".to_string();
-		let expected = Err(Error::Keystore(err_msg));
-		assert_eq!(verify_validator_set::<Block>(&1, &validator_set, &worker.key_store), expected);
-
-		// worker has no keystore
-		worker.key_store = None.into();
-		let expected_err = Err(Error::Keystore("no Keystore".into()));
-		assert_eq!(
-			verify_validator_set::<Block>(&1, &validator_set, &worker.key_store),
-			expected_err
-		);
-	}
-
 	#[tokio::test]
 	async fn should_finalize_correctly() {
 		let keys = [Keyring::Alice];