client/authority-discovery: Allow to be run by sentry node (#5568)

* client/authority-discovery: Allow to be run by sentry node

When run as a sentry node, the authority discovery module does not
publish any addresses to the dht, but still discovers validators and
sentry nodes of validators.

* client/authority-discovery/src/lib: Wrap lines at 100 characters

* client/authority-discovery: Remove TODO and unused import

* client/authority-discovery: Pass role to new unit tests

* client/authority-discovery: Apply suggestions

Co-Authored-By: André Silva <123550+andresilva@users.noreply.github.com>

* bin/node/cli/src/service: Use expressions instead of statements

Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>

substrate/.maintain/sentry-node/docker-compose.yml

@@ -82,13 +82,12 @@ services:
       - "--chain=local"
       - "--port"
       - "30333"
-      - "--charlie"
       - "--sentry"
       - "/dns4/validator-a/tcp/30333/p2p/QmRpheLN4JWdAnY7HGJfWFNbfkQCb6tFf4vvA6hgjMZKrR"
+      - "--reserved-nodes"
+      - "/dns4/validator-a/tcp/30333/p2p/QmRpheLN4JWdAnY7HGJfWFNbfkQCb6tFf4vvA6hgjMZKrR"
       - "--bootnodes"
       - "/dns4/validator-b/tcp/30333/p2p/QmSVnNf9HwVMT1Y4cK1P6aoJcEZjmoTXpjKBmAABLMnZEk"
-      - "--reserved-nodes"
-      - "/dns4/validator-a/tcp/30333/p2p/QmRpheLN4JWdAnY7HGJfWFNbfkQCb6tFf4vvA6hgjMZKrR"
       - "--no-telemetry"
       - "--rpc-cors"
       - "all"
@@ -97,7 +96,6 @@ services:
       - "--unsafe-rpc-external"
       - "--log"
       - "sub-authority-discovery=trace"
-      - "--sentry"
       - "--prometheus-external"
 
   validator-b:

substrate/bin/node/cli/src/service.rs

@@ -146,7 +146,7 @@ macro_rules! new_full {
 
 		($with_startup_data)(&block_import, &babe_link);
 
-		if let sc_service::config::Role::Authority { sentry_nodes } = &role {
+		if let sc_service::config::Role::Authority { .. } = &role {
 			let proposer = sc_basic_authorship::ProposerFactory::new(
 				service.client(),
 				service.transaction_pool()
@@ -174,6 +174,23 @@ macro_rules! new_full {
 
 			let babe = sc_consensus_babe::start_babe(babe_config)?;
 			service.spawn_essential_task("babe-proposer", babe);
+		}
+
+		// Spawn authority discovery module.
+		if matches!(role, sc_service::config::Role::Authority{..} | sc_service::config::Role::Sentry {..}) {
+			let (sentries, authority_discovery_role) = match role {
+				sc_service::config::Role::Authority { ref sentry_nodes } => (
+					sentry_nodes.clone(),
+					sc_authority_discovery::Role::Authority (
+						service.keystore(),
+					),
+				),
+				sc_service::config::Role::Sentry {..} => (
+					vec![],
+					sc_authority_discovery::Role::Sentry,
+				),
+				_ => unreachable!("Due to outer matches! constraint; qed.")
+			};
 
 			let network = service.network();
 			let dht_event_stream = network.event_stream("authority-discovery").filter_map(|e| async move { match e {
@@ -183,9 +200,9 @@ macro_rules! new_full {
 			let authority_discovery = sc_authority_discovery::AuthorityDiscovery::new(
 				service.client(),
 				network,
-				sentry_nodes.clone(),
+				sentries,
-				service.keystore(),
 				dht_event_stream,
+				authority_discovery_role,
 				service.prometheus_registry(),
 			);
 

substrate/client/authority-discovery/src/lib.rs

@@ -25,13 +25,11 @@
 //!
 //! 1. **Makes itself discoverable**
 //!
-//!    1. Retrieves its external addresses.
+//!    1. Retrieves its external addresses (including peer id) or the ones of its sentry nodes.
 //!
-//!    2. Adds its network peer id to the addresses.
+//!    2. Signs the above.
 //!
-//!    3. Signs the above.
+//!    3. Puts the signature and the addresses on the libp2p Kademlia DHT.
-//!
-//!    4. Puts the signature and the addresses on the libp2p Kademlia DHT.
 //!
 //!
 //! 2. **Discovers other authorities**
@@ -43,6 +41,12 @@
 //!    3. Validates the signatures of the retrieved key value pairs.
 //!
 //!    4. Adds the retrieved external addresses as priority nodes to the peerset.
+//!
+//! When run as a sentry node, the authority discovery module does not
+//! publish any addresses to the DHT but still discovers validators and
+//! sentry nodes of validators, i.e. only step 2 (Discovers other authorities)
+//! is executed.
+
 use std::collections::{HashMap, HashSet};
 use std::convert::TryInto;
 use std::marker::PhantomData;
@@ -62,7 +66,7 @@ use prost::Message;
 use sc_client_api::blockchain::HeaderBackend;
 use sc_network::{Multiaddr, config::MultiaddrWithPeerId, DhtEvent, ExHashT, NetworkStateInfo};
 use sp_authority_discovery::{AuthorityDiscoveryApi, AuthorityId, AuthoritySignature, AuthorityPair};
-use sp_core::crypto::{key_types, CryptoTypePublicPair, Pair};
+use sp_core::crypto::{key_types, Pair};
 use sp_core::traits::BareCryptoStorePtr;
 use sp_runtime::{traits::Block as BlockT, generic::BlockId};
 use sp_api::ProvideRuntimeApi;
@@ -89,6 +93,17 @@ const LIBP2P_KADEMLIA_BOOTSTRAP_TIME: Duration = Duration::from_secs(30);
 /// discovery module.
 const AUTHORITIES_PRIORITY_GROUP_NAME: &'static str = "authorities";
 
+/// Role an authority discovery module can run as.
+pub enum Role {
+	/// Actual authority as well as a reference to its key store.
+	Authority(BareCryptoStorePtr),
+	/// Sentry node that guards an authority.
+	///
+	/// No reference to its key store needed, as sentry nodes don't have an identity to sign
+	/// addresses with in the first place.
+	Sentry,
+}
+
 /// An `AuthorityDiscovery` makes a given authority discoverable and discovers other authorities.
 pub struct AuthorityDiscovery<Client, Network, Block>
 where
@@ -111,8 +126,6 @@ where
 	/// Channel we receive Dht events on.
 	dht_event_rx: Pin<Box<dyn Stream<Item = DhtEvent> + Send>>,
 
-	key_store: BareCryptoStorePtr,
-
 	/// Interval to be proactive, publishing own addresses.
 	publish_interval: Interval,
 	/// Interval on which to query for addresses of other authorities.
@@ -122,6 +135,8 @@ where
 
 	metrics: Option<Metrics>,
 
+	role: Role,
+
 	phantom: PhantomData<Block>,
 }
 
@@ -142,8 +157,8 @@ where
 		client: Arc<Client>,
 		network: Arc<Network>,
 		sentry_nodes: Vec<MultiaddrWithPeerId>,
-		key_store: BareCryptoStorePtr,
 		dht_event_rx: Pin<Box<dyn Stream<Item = DhtEvent> + Send>>,
+		role: Role,
 		prometheus_registry: Option<prometheus_endpoint::Registry>,
 	) -> Self {
 		// Kademlia's default time-to-live for Dht records is 36h, republishing records every 24h.
@@ -189,10 +204,10 @@ where
 			network,
 			sentry_nodes,
 			dht_event_rx,
-			key_store,
 			publish_interval,
 			query_interval,
 			addr_cache,
+			role,
 			metrics,
 			phantom: PhantomData,
 		}
@@ -200,6 +215,14 @@ where
 
 	/// Publish either our own or if specified the public addresses of our sentry nodes.
 	fn publish_ext_addresses(&mut self) -> Result<()> {
+		let key_store = match &self.role {
+			Role::Authority(key_store) => key_store,
+			// Only authority nodes can put addresses (their own or the ones of their sentry nodes)
+			// on the Dht. Sentry nodes don't have a known identity to authenticate such addresses,
+			// thus `publish_ext_addresses` becomes a no-op.
+			Role::Sentry => return Ok(()),
+		};
+
 		if let Some(metrics) = &self.metrics {
 			metrics.publish.inc()
 		}
@@ -226,13 +249,12 @@ where
 			.encode(&mut serialized_addresses)
 			.map_err(Error::EncodingProto)?;
 
-		let keys: Vec<CryptoTypePublicPair> = self.get_own_public_keys_within_authority_set()?
+		let keys = AuthorityDiscovery::get_own_public_keys_within_authority_set(
-			.into_iter()
+			&key_store,
-			.map(Into::into)
+			&self.client,
-			.collect();
+		)?.into_iter().map(Into::into).collect::<Vec<_>>();
 
-		let signatures = self.key_store
+		let signatures = key_store.read()
-			.read()
 			.sign_with_all(
 				key_types::AUTHORITY_DISCOVERY,
 				keys.clone(),
@@ -424,17 +446,17 @@ where
 	// one for the upcoming session. In addition it could be participating in the current authority
 	// set with two keys. The function does not return all of the local authority discovery public
 	// keys, but only the ones intersecting with the current authority set.
-	fn get_own_public_keys_within_authority_set(&mut self) -> Result<HashSet<AuthorityId>> {
+	fn get_own_public_keys_within_authority_set(
-		let local_pub_keys = self.key_store
+		key_store: &BareCryptoStorePtr,
-			.read()
+		client: &Client,
+	) -> Result<HashSet<AuthorityId>> {
+		let local_pub_keys = key_store.read()
 			.sr25519_public_keys(key_types::AUTHORITY_DISCOVERY)
 			.into_iter()
 			.collect::<HashSet<_>>();
 
-		let id = BlockId::hash(self.client.info().best_hash);
+		let id = BlockId::hash(client.info().best_hash);
-		let current_authorities = self
+		let current_authorities = client.runtime_api()
-			.client
-			.runtime_api()
 			.authorities(&id)
 			.map_err(Error::CallingRuntime)?
 			.into_iter()
@@ -458,7 +480,10 @@ where
 			"Applying priority group {:?} to peerset.", addresses,
 		);
 		self.network
-			.set_priority_group(AUTHORITIES_PRIORITY_GROUP_NAME.to_string(), addresses.into_iter().collect())
+			.set_priority_group(
+				AUTHORITIES_PRIORITY_GROUP_NAME.to_string(),
+				addresses.into_iter().collect(),
+			)
 			.map_err(Error::SettingPeersetPriorityGroup)?;
 
 		Ok(())

substrate/client/authority-discovery/src/tests.rs

@@ -216,8 +216,8 @@ fn new_registers_metrics() {
 		test_api,
 		network.clone(),
 		vec![],
-		key_store,
 		dht_event_rx.boxed(),
+		Role::Authority(key_store),
 		Some(registry.clone()),
 	);
 
@@ -241,8 +241,8 @@ fn publish_ext_addresses_puts_record_on_dht() {
 		test_api,
 		network.clone(),
 		vec![],
-		key_store,
 		dht_event_rx.boxed(),
+		Role::Authority(key_store),
 		None,
 	);
 
@@ -272,8 +272,8 @@ fn request_addresses_of_others_triggers_dht_get_query() {
 		test_api,
 		network.clone(),
 		vec![],
-		key_store,
 		dht_event_rx.boxed(),
+		Role::Authority(key_store),
 		None,
 	);
 
@@ -301,8 +301,8 @@ fn handle_dht_events_with_value_found_should_call_set_priority_group() {
 		test_api,
 		network.clone(),
 		vec![],
-		key_store,
 		dht_event_rx.boxed(),
+		Role::Authority(key_store),
 		None,
 	);
 
@@ -366,8 +366,8 @@ fn terminate_when_event_stream_terminates() {
 		test_api,
 		network.clone(),
 		vec![],
-		key_store,
 		dht_event_rx.boxed(),
+		Role::Authority(key_store),
 		None,
 	);
 
@@ -407,8 +407,8 @@ fn dont_stop_polling_when_error_is_returned() {
 		test_api,
 		network.clone(),
 		vec![],
-		key_store,
 		dht_event_rx.boxed(),
+		Role::Authority(key_store),
 		None,
 	);