*: Sign authority discovery DHT payload with keystore instead of runtime

Instead of calling a runtime function to sign a dht payload, which then
invokes the keystore, pass the keystore to the authority discovery
module and use it directly.

core/authority-discovery/Cargo.toml

@@ -15,6 +15,7 @@ client = { package = "substrate-client", path = "../../core/client" }
 codec = { package = "parity-scale-codec", default-features = false, version = "1.0.3" }
 derive_more = "0.15.0"
 futures = "0.1.29"
+keystore = { package = "substrate-keystore", path = "../keystore" }
 libp2p = { version = "0.12.0", default-features = false, features = ["secp256k1", "libp2p-websocket"] }
 log = "0.4.8"
 network = { package = "substrate-network", path = "../../core/network" }

core/authority-discovery/primitives/src/lib.rs

@@ -39,15 +39,9 @@ pub type AuthoritySignature = app::Signature;
 decl_runtime_apis! {
 	/// The authority discovery api.
 	///
-	/// This api is used by the `core/authority-discovery` module to retrieve our
-	/// own authority identifier, to retrieve identifiers of the current authority
-	/// set, as well as sign and verify Kademlia Dht external address payloads
-	/// from and to other authorities.
+	/// This api is used by the `core/authority-discovery` module to retrieve identifiers of the current authority set.
 	pub trait AuthorityDiscoveryApi {
 		/// Retrieve authority identifiers of the current authority set.
 		fn authorities() -> Vec<AuthorityId>;
-
-		/// Sign the given payload with the private key corresponding to the returned authority id.
-		fn sign(payload: &Vec<u8>) -> Option<(AuthoritySignature, AuthorityId)>;
 	}
 }

core/authority-discovery/src/error.rs

@@ -28,8 +28,6 @@ pub enum Error {
 	HashingAuthorityId(libp2p::core::multiaddr::multihash::EncodeError),
 	/// Failed calling into the Substrate runtime.
 	CallingRuntime(client::error::Error),
-	/// Failed signing the dht payload via the Substrate runtime.
-	SigningDhtPayload,
 	/// From the Dht we only get the hashed authority id. In order to retrieve the actual authority id and to ensure it
 	/// is actually an authority, we match the hash against the hash of the authority id of all other authorities. This
 	/// error is the result of the above failing.
@@ -45,5 +43,5 @@ pub enum Error {
 	/// Failed to parse a libp2p multi address.
 	ParsingMultiaddress(libp2p::core::multiaddr::Error),
 	/// Tokio timer error.
-	PollingTokioTimer(tokio_timer::Error)
+	PollingTokioTimer(tokio_timer::Error),
 }

core/authority-discovery/src/lib.rs

@@ -51,7 +51,8 @@ use futures::{prelude::*, sync::mpsc::Receiver};
 use log::{debug, error, log_enabled, warn};
 use network::specialization::NetworkSpecialization;
 use network::{DhtEvent, ExHashT};
-use primitives::crypto::Pair;
+use primitives::crypto::{key_types, Pair};
+use primitives::traits::BareCryptoStorePtr;
 use prost::Message;
 use sr_primitives::generic::BlockId;
 use sr_primitives::traits::{Block as BlockT, ProvideRuntimeApi};
@@ -75,13 +76,16 @@ where
 	Network: NetworkProvider,
 	Client: ProvideRuntimeApi + Send + Sync + 'static + HeaderBackend<Block>,
 	<Client as ProvideRuntimeApi>::Api: AuthorityDiscoveryApi<Block>,
+
 {
 	client: Arc<Client>,
 
 	network: Arc<Network>,
 	/// Channel we receive Dht events on.
 	dht_event_rx: Receiver<DhtEvent>,
 
+	key_store: BareCryptoStorePtr,
+
 	/// Interval to be proactive, publishing own addresses.
 	publish_interval: tokio_timer::Interval,
 	/// Interval on which to query for addresses of other authorities.
@@ -107,6 +111,7 @@ where
 	pub fn new(
 		client: Arc<Client>,
 		network: Arc<Network>,
+		key_store: BareCryptoStorePtr,
 		dht_event_rx: futures::sync::mpsc::Receiver<DhtEvent>,
 	) -> AuthorityDiscovery<Client, Network, Block> {
 		// Kademlia's default time-to-live for Dht records is 36h, republishing records every 24h. Given that a node
@@ -128,6 +133,7 @@ where
 			client,
 			network,
 			dht_event_rx,
+			key_store,
 			publish_interval,
 			query_interval,
 			address_cache,
@@ -136,8 +142,6 @@ where
 	}
 
 	fn publish_own_ext_addresses(&mut self) -> Result<()> {
-		let id = BlockId::hash(self.client.info().best_hash);
-
 		let addresses = self
 			.network
 			.external_addresses()
@@ -155,27 +159,25 @@ where
 			.encode(&mut serialized_addresses)
 			.map_err(Error::EncodingProto)?;
 
-		let (signature, authority_id) = self
-			.client
-			.runtime_api()
-			.sign(&id, &serialized_addresses)
-			.map_err(Error::CallingRuntime)?
-			.ok_or(Error::SigningDhtPayload)?;
+		for key in self.get_priv_keys_within_authority_set()?.into_iter() {
+			let signature = key.sign(&serialized_addresses);
 
-		let mut signed_addresses = vec![];
-		schema::SignedAuthorityAddresses {
-			addresses: serialized_addresses,
-			// TODO: Instead of encoding via scale and then encoding via proto, we could also convert the signature to a
-			// [u8] and protobuf encode from there.
-			signature: signature.encode(),
-		}
-		.encode(&mut signed_addresses)
-		.map_err(Error::EncodingProto)?;
+			let mut signed_addresses = vec![];
+			schema::SignedAuthorityAddresses {
+				addresses: serialized_addresses.clone(),
+				// TODO: Instead of encoding via scale and then encoding via proto, we could also convert the signature
+				// to a [u8] and protobuf encode from there. Important to keep in mind that this module is not anywhere
+				// close to a resource hot path.
+				signature: signature.encode(),
+			}
+			.encode(&mut signed_addresses)
+				.map_err(Error::EncodingProto)?;
 
-		self.network.put_value(
-			hash_authority_id(authority_id.as_ref())?,
-			signed_addresses,
-		);
+			self.network.put_value(
+				hash_authority_id(key.public().as_ref())?,
+				signed_addresses,
+			);
+		}
 
 		Ok(())
 	}
@@ -291,6 +293,44 @@ where
 		self.address_cache
 			.retain(|peer_id, _addresses| current_authorities.contains(peer_id))
 	}
+
+	fn get_priv_keys_within_authority_set(&mut self) -> Result<Vec<AuthorityPair>> {
+		let keys = self.get_own_public_keys_within_authority_set()?
+			.into_iter()
+			.map(std::convert::Into::into)
+			.filter_map(|pub_key| {
+				self.key_store.read().sr25519_key_pair(key_types::AUTHORITY_DISCOVERY, &pub_key)
+			})
+			.map(std::convert::Into::into)
+			.collect();
+
+		Ok(keys)
+	}
+
+	fn get_own_public_keys_within_authority_set(&mut self) -> Result<HashSet<AuthorityId>> {
+		let local_pub_keys = self.key_store
+			.read()
+			.sr25519_public_keys(key_types::AUTHORITY_DISCOVERY)
+			.into_iter()
+			.collect::<HashSet<_>>();
+
+		let id = BlockId::hash(self.client.info().best_hash);
+		let current_authorities = self
+			.client
+			.runtime_api()
+			.authorities(&id)
+			.map_err(Error::CallingRuntime)?
+			.into_iter()
+			.map(std::convert::Into::into)
+			.collect::<HashSet<_>>();
+
+		let intersection = local_pub_keys.intersection(&current_authorities)
+			.cloned()
+			.map(std::convert::Into::into)
+			.collect();
+
+		Ok(intersection)
+	}
 }
 
 impl<Client, Network, Block> futures::Future for AuthorityDiscovery<Client, Network, Block>
@@ -418,21 +458,23 @@ mod tests {
 	use super::*;
 	use client::runtime_api::{ApiExt, Core, RuntimeVersion};
 	use futures::future::poll_fn;
-	use primitives::{ExecutionContext, NativeOrEncoded, Public};
+	use primitives::{ExecutionContext, NativeOrEncoded, testing::KeyStore};
 	use sr_primitives::traits::Zero;
 	use sr_primitives::traits::{ApiRef, Block as BlockT, NumberFor, ProvideRuntimeApi};
 	use std::sync::{Arc, Mutex};
 	use test_client::runtime::Block;
 	use tokio::runtime::current_thread;
 
 	#[derive(Clone)]
-	struct TestApi {}
+	struct TestApi {
+		authorities: Vec<AuthorityId>,
+	}
 
 	impl ProvideRuntimeApi for TestApi {
 		type Api = RuntimeApi;
 
 		fn runtime_api<'a>(&'a self) -> ApiRef<'a, Self::Api> {
-			RuntimeApi {}.into()
+			RuntimeApi{authorities: self.authorities.clone()}.into()
 		}
 	}
 
@@ -477,7 +519,9 @@ mod tests {
 		}
 	}
 
-	struct RuntimeApi {}
+	struct RuntimeApi {
+		authorities: Vec<AuthorityId>,
+	}
 
 	impl Core<Block> for RuntimeApi {
 		fn Core_version_runtime_api_impl(
@@ -543,28 +587,7 @@ mod tests {
 			_: Option<()>,
 			_: Vec<u8>,
 		) -> std::result::Result<NativeOrEncoded<Vec<AuthorityId>>, client::error::Error> {
-			// TODO: cleanup
-			let authority_1_key_pair = AuthorityPair::from_seed_slice(&[1; 32]).unwrap();
-			let authority_2_key_pair = AuthorityPair::from_seed_slice(&[2; 32]).unwrap();
-			return Ok(NativeOrEncoded::Native(vec![
-				authority_1_key_pair.public(),
-				authority_2_key_pair.public(),
-			]));
-		}
-		fn AuthorityDiscoveryApi_sign_runtime_api_impl(
-			&self,
-			_: &BlockId<Block>,
-			_: ExecutionContext,
-			_: Option<&std::vec::Vec<u8>>,
-			_: Vec<u8>,
-		) -> std::result::Result<
-			NativeOrEncoded<Option<(AuthoritySignature, AuthorityId)>>,
-			client::error::Error,
-		> {
-			return Ok(NativeOrEncoded::Native(Some((
-				AuthorityPair::from_seed_slice(&[0; 32]).unwrap().sign(b"1"),
-				AuthorityId::from_slice(&[2; 32]),
-			))));
+			return Ok(NativeOrEncoded::Native(self.authorities.clone()));
 		}
 	}
 
@@ -605,11 +628,13 @@ mod tests {
 	#[test]
 	fn publish_own_ext_addresses_puts_record_on_dht() {
 		let (_dht_event_tx, dht_event_rx) = futures::sync::mpsc::channel(1000);
-		let test_api = Arc::new(TestApi {});
 		let network: Arc<TestNetwork> = Arc::new(Default::default());
+		let key_store = KeyStore::new();
+		let public = key_store.write().sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None).unwrap();
+		let test_api = Arc::new(TestApi {authorities: vec![public.into()]});
 
 		let mut authority_discovery =
-			AuthorityDiscovery::new(test_api, network.clone(), dht_event_rx);
+			AuthorityDiscovery::new(test_api, network.clone(), key_store, dht_event_rx);
 
 		authority_discovery.publish_own_ext_addresses().unwrap();
 
@@ -620,11 +645,18 @@ mod tests {
 	#[test]
 	fn request_addresses_of_others_triggers_dht_get_query() {
 		let (_dht_event_tx, dht_event_rx) = futures::sync::mpsc::channel(1000);
-		let test_api = Arc::new(TestApi {});
+
+		// Generate authority keys
+		let authority_1_key_pair = AuthorityPair::from_seed_slice(&[1; 32]).unwrap();
+		let authority_2_key_pair = AuthorityPair::from_seed_slice(&[2; 32]).unwrap();
+
+		let test_api = Arc::new(TestApi {authorities: vec![authority_1_key_pair.public(), authority_2_key_pair.public()]});
+
 		let network: Arc<TestNetwork> = Arc::new(Default::default());
+		let key_store = KeyStore::new();
 
 		let mut authority_discovery =
-			AuthorityDiscovery::new(test_api, network.clone(), dht_event_rx);
+			AuthorityDiscovery::new(test_api, network.clone(), key_store, dht_event_rx);
 
 		authority_discovery.request_addresses_of_others().unwrap();
 
@@ -637,16 +669,16 @@ mod tests {
 		// Create authority discovery.
 
 		let (mut dht_event_tx, dht_event_rx) = futures::sync::mpsc::channel(1000);
-		let test_api = Arc::new(TestApi {});
+		let key_pair = AuthorityPair::from_seed_slice(&[1; 32]).unwrap();
+		let test_api = Arc::new(TestApi {authorities: vec![key_pair.public()]});
 		let network: Arc<TestNetwork> = Arc::new(Default::default());
+		let key_store = KeyStore::new();
 
 		let mut authority_discovery =
-			AuthorityDiscovery::new(test_api, network.clone(), dht_event_rx);
+			AuthorityDiscovery::new(test_api, network.clone(), key_store, dht_event_rx);
 
 		// Create sample dht event.
 
-		let key_pair = AuthorityPair::from_seed_slice(&[1; 32]).unwrap();
-
 		let authority_id_1 = hash_authority_id(key_pair.public().as_ref()).unwrap();
 		let address_1: libp2p::Multiaddr = "/ip6/2001:db8::".parse().unwrap();
 

node/cli/src/service.rs

@@ -130,7 +130,7 @@ macro_rules! new_full {
 		// back-pressure. Authority discovery is triggering one event per authority within the current authority set.
 		// This estimates the authority set size to be somewhere below 10 000 thereby setting the channel buffer size to
 		// 10 000.
-		let (dht_event_tx, _dht_event_rx) =
+		let (dht_event_tx, dht_event_rx) =
 			mpsc::channel::<DhtEvent>(10_000);
 
 		let service = builder.with_network_protocol(|_| Ok(crate::service::NodeProtocol::new()))?
@@ -169,6 +169,15 @@ macro_rules! new_full {
 
 			let babe = babe::start_babe(babe_config)?;
 			service.spawn_essential_task(babe);
+
+			let authority_discovery = authority_discovery::AuthorityDiscovery::new(
+				service.client(),
+				service.network(),
+				service.keystore(),
+				dht_event_rx,
+			);
+
+			service.spawn_task(authority_discovery);
 		}
 
 		let config = grandpa::Config {

node/runtime/src/lib.rs

@@ -647,10 +647,6 @@ impl_runtime_apis! {
 		fn authorities() -> Vec<AuthorityDiscoveryId> {
 			AuthorityDiscovery::authorities()
 		}
-
-		fn sign(payload: &Vec<u8>) -> Option<(AuthorityDiscoverySignature, AuthorityDiscoveryId)> {
-			  AuthorityDiscovery::sign(payload)
-		}
 	}
 
 	impl system_rpc_runtime_api::AccountNonceApi<Block, AccountId, Index> for Runtime {