client/authority-discovery: Do not double encode signature (#5901)

Previously, when publishing ones address onto the DHT, the signature
signing those addresses would be SCALE encoded twice.

This commit removes the second encoding and adjusts the tests
to catch future regressions.

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

@@ -58,7 +58,7 @@ use futures::task::{Context, Poll};
 use futures::{Future, FutureExt, ready, Stream, StreamExt};
 use futures_timer::Delay;
 
-use codec::{Decode, Encode};
+use codec::Decode;
 use error::{Error, Result};
 use log::{debug, error, log_enabled};
 use prometheus_endpoint::{Counter, CounterVec, Gauge, Opts, U64, register};
@@ -262,16 +262,16 @@ where
 			)
 			.map_err(|_| Error::Signing)?;
 
-		for (sign_result, key) in signatures.iter().zip(keys) {
+		for (sign_result, key) in signatures.into_iter().zip(keys) {
 			let mut signed_addresses = vec![];
 
 			// sign_with_all returns Result<Signature, Error> signature
 			// is generated for a public key that is supported.
 			// Verify that all signatures exist for all provided keys.
-			let signature = sign_result.as_ref().map_err(|_| Error::MissingSignature(key.clone()))?;
+			let signature = sign_result.map_err(|_| Error::MissingSignature(key.clone()))?;
 			schema::SignedAuthorityAddresses {
 				addresses: serialized_addresses.clone(),
-				signature: Encode::encode(&signature),
+				signature,
 			}
 			.encode(&mut signed_addresses)
 				.map_err(Error::EncodingProto)?;

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

@@ -162,8 +162,8 @@ sp_api::mock_impl_runtime_apis! {
 	}
 }
 
-#[derive(Default)]
 struct TestNetwork {
+	peer_id: PeerId,
 	// Whenever functions on `TestNetwork` are called, the function arguments are added to the
 	// vectors below.
 	pub put_value_call: Arc<Mutex<Vec<(kad::record::Key, Vec<u8>)>>>,
@@ -171,6 +171,17 @@ struct TestNetwork {
 	pub set_priority_group_call: Arc<Mutex<Vec<(String, HashSet<Multiaddr>)>>>,
 }
 
+impl Default for TestNetwork {
+	fn default() -> Self {
+		TestNetwork {
+			peer_id: PeerId::random(),
+			put_value_call: Default::default(),
+			get_value_call: Default::default(),
+			set_priority_group_call: Default::default(),
+		}
+	}
+}
+
 impl NetworkProvider for TestNetwork {
 	fn set_priority_group(
 		&self,
@@ -193,11 +204,11 @@ impl NetworkProvider for TestNetwork {
 
 impl NetworkStateInfo for TestNetwork {
 	fn local_peer_id(&self) -> PeerId {
-		PeerId::random()
+		self.peer_id.clone()
 	}
 
 	fn external_addresses(&self) -> Vec<Multiaddr> {
-		vec![]
+		vec!["/ip6/2001:db8::".parse().unwrap()]
 	}
 }
 
@@ -224,34 +235,6 @@ fn new_registers_metrics() {
 	assert!(registry.gather().len() > 0);
 }
 
-#[test]
-fn publish_ext_addresses_puts_record_on_dht() {
-	let (_dht_event_tx, dht_event_rx) = channel(1000);
-	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(),
-		vec![],
-		dht_event_rx.boxed(),
-		Role::Authority(key_store),
-		None,
-	);
-
-	authority_discovery.publish_ext_addresses().unwrap();
-
-	// Expect authority discovery to put a new record onto the dht.
-	assert_eq!(network.put_value_call.lock().unwrap().len(), 1);
-}
-
 #[test]
 fn request_addresses_of_others_triggers_dht_get_query() {
 	let _ = ::env_logger::try_init();
@@ -284,15 +267,57 @@ fn request_addresses_of_others_triggers_dht_get_query() {
 }
 
 #[test]
-fn handle_dht_events_with_value_found_should_call_set_priority_group() {
+fn publish_discover_cycle() {
 	let _ = ::env_logger::try_init();
 
-	// Create authority discovery.
+	// Node A publishing its address.
+
+	let (_dht_event_tx, dht_event_rx) = channel(1000);
+
+	let network: Arc<TestNetwork> = Arc::new(Default::default());
+	let node_a_multiaddr = {
+		let peer_id = network.local_peer_id();
+		let address = network.external_addresses().pop().unwrap();
+
+		address.with(libp2p::core::multiaddr::Protocol::P2p(
+			peer_id.into(),
+		))
+	};
+
+	let key_store = KeyStore::new();
+	let node_a_public = key_store
+		.write()
+		.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None)
+		.unwrap();
+	let test_api = Arc::new(TestApi {
+		authorities: vec![node_a_public.into()],
+	});
+
+	let mut authority_discovery = AuthorityDiscovery::new(
+		test_api,
+		network.clone(),
+		vec![],
+		dht_event_rx.boxed(),
+		Role::Authority(key_store),
+		None,
+	);
+
+	authority_discovery.publish_ext_addresses().unwrap();
+
+	// Expect authority discovery to put a new record onto the dht.
+	assert_eq!(network.put_value_call.lock().unwrap().len(), 1);
+
+	let dht_event = {
+		let (key, value) = network.put_value_call.lock().unwrap().pop().unwrap();
+		sc_network::DhtEvent::ValueFound(vec![(key, value)])
+	};
+
+	// Node B discovering node A's address.
 
 	let (mut dht_event_tx, dht_event_rx) = channel(1000);
-	let key_pair = AuthorityPair::from_seed_slice(&[1; 32]).unwrap();
 	let test_api = Arc::new(TestApi {
-		authorities: vec![key_pair.public()],
+		// Make sure node B identifies node A as an authority.
+		authorities: vec![node_a_public.into()],
 	});
 	let network: Arc<TestNetwork> = Arc::new(Default::default());
 	let key_store = KeyStore::new();
@@ -306,32 +331,10 @@ fn handle_dht_events_with_value_found_should_call_set_priority_group() {
 		None,
 	);
 
-	// Create sample dht event.
-
-	let authority_id_1 = hash_authority_id(key_pair.public().as_ref());
-	let address_1: Multiaddr = "/ip6/2001:db8::".parse().unwrap();
-
-	let mut serialized_addresses = vec![];
-	schema::AuthorityAddresses {
-		addresses: vec![address_1.to_vec()],
-	}
-	.encode(&mut serialized_addresses)
-	.unwrap();
-
-	let signature = key_pair.sign(serialized_addresses.as_ref()).encode();
-	let mut signed_addresses = vec![];
-	schema::SignedAuthorityAddresses {
-		addresses: serialized_addresses,
-		signature,
-	}
-	.encode(&mut signed_addresses)
-	.unwrap();
-
-	let dht_event = sc_network::DhtEvent::ValueFound(vec![(authority_id_1, signed_addresses)]);
 	dht_event_tx.try_send(dht_event).unwrap();
 
-	// Make authority discovery handle the event.
 	let f = |cx: &mut Context<'_>| -> Poll<()> {
+		// Make authority discovery handle the event.
 		if let Poll::Ready(e) = authority_discovery.handle_dht_events(cx) {
 			panic!("Unexpected error: {:?}", e);
 		}
@@ -343,7 +346,7 @@ fn handle_dht_events_with_value_found_should_call_set_priority_group() {
 			network.set_priority_group_call.lock().unwrap()[0],
 			(
 				"authorities".to_string(),
-				HashSet::from_iter(vec![address_1.clone()].into_iter())
+				HashSet::from_iter(vec![node_a_multiaddr.clone()].into_iter())
 			)
 		);
 

substrate/primitives/core/src/traits.rs

@@ -120,7 +120,7 @@ pub trait BareCryptoStore: Send + Sync {
 	/// Given a list of public keys, find the first supported key and
 	/// sign the provided message with that key.
 	///
-	/// Returns a tuple of the used key and the signature
+	/// Returns a tuple of the used key and the SCALE encoded signature.
 	fn sign_with_any(
 		&self,
 		id: KeyTypeId,
@@ -144,8 +144,8 @@ pub trait BareCryptoStore: Send + Sync {
 	/// Provided a list of public keys, sign a message with
 	/// each key given that the key is supported.
 	///
-	/// Returns a list of `Result`s each representing the signature of each key or
-	/// a BareCryptoStoreError for non-supported keys.
+	/// Returns a list of `Result`s each representing the SCALE encoded
+	/// signature of each key or a BareCryptoStoreError for non-supported keys.
 	fn sign_with_all(
 		&self,
 		id: KeyTypeId,