client/authority-discovery: Ignore Multiaddr without PeerId

client/authority-discovery/src/service.rs

@@ -38,6 +38,9 @@ impl Service {
 	}
 
 	/// Get the addresses for the given [`AuthorityId`] from the local address cache.
+	///
+	/// [`Multiaddr`]s returned always include a [`libp2p::core::multiaddr:Protocol::P2p`]
+	/// component.
 	pub async fn get_addresses_by_authority_id(&mut self, authority: AuthorityId) -> Option<Vec<Multiaddr>> {
 		let (tx, rx) = oneshot::channel();
 

client/authority-discovery/src/worker.rs

@@ -470,8 +470,8 @@ where
 			.collect::<Result<Vec<Vec<Multiaddr>>>>()?
 			.into_iter()
 			.flatten()
-			// Ignore own addresses.
-			.filter(|addr| !addr.iter().any(|protocol| {
+			// Ignore [`Multiaddr`]s without [`PeerId`] and own addresses.
+			.filter(|addr| addr.iter().any(|protocol| {
 				// Parse to PeerId first as Multihashes of old and new PeerId
 				// representation don't equal.
 				//
@@ -480,13 +480,14 @@ where
 				if let multiaddr::Protocol::P2p(hash) = protocol {
 					let peer_id = match PeerId::from_multihash(hash) {
 						Ok(peer_id) => peer_id,
-						Err(_) => return true, // Discard address.
+						Err(_) => return false, // Discard address.
 					};
 
-					return peer_id == local_peer_id;
+					// Discard if equal to local peer id, keep if it differs.
+					return !(peer_id == local_peer_id);
 				}
 
-				false // Multiaddr does not contain a PeerId.
+				false // `protocol` is not a [`Protocol::P2p`], let's keep looking.
 			}))
 			.collect();
 

client/authority-discovery/src/worker/addr_cache.rs

@@ -146,6 +146,7 @@ mod tests {
 
 	use libp2p::multihash;
 	use quickcheck::{Arbitrary, Gen, QuickCheck, TestResult};
+	use rand::Rng;
 
 	use sp_authority_discovery::{AuthorityId, AuthorityPair};
 	use sp_core::crypto::Pair;

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

@@ -611,3 +611,83 @@ fn never_add_own_address_to_priority_group() {
 		"Expect authority discovery to only add `random_multiaddr`."
 	);
 }
+
+#[test]
+fn do_not_cache_addresses_without_peer_id() {
+	let remote_key_store = KeyStore::new();
+	let remote_public = remote_key_store
+		.write()
+		.sr25519_generate_new(key_types::AUTHORITY_DISCOVERY, None)
+		.unwrap();
+
+	let multiaddr_with_peer_id = {
+		let peer_id = PeerId::random();
+		let address: Multiaddr = "/ip6/2001:db8:0:0:0:0:0:2/tcp/30333".parse().unwrap();
+
+		address.with(multiaddr::Protocol::P2p(peer_id.into()))
+	};
+
+	let multiaddr_without_peer_id: Multiaddr = "/ip6/2001:db8:0:0:0:0:0:1/tcp/30333".parse().unwrap();
+
+	let dht_event = {
+		let addresses = vec![
+			multiaddr_with_peer_id.to_vec(),
+			multiaddr_without_peer_id.to_vec(),
+		];
+
+		let mut serialized_addresses = vec![];
+		schema::AuthorityAddresses { addresses }
+		.encode(&mut serialized_addresses)
+			.map_err(Error::EncodingProto)
+			.unwrap();
+
+		let signature = remote_key_store.read()
+			.sign_with(
+				key_types::AUTHORITY_DISCOVERY,
+				&remote_public.clone().into(),
+				serialized_addresses.as_slice(),
+			)
+			.map_err(|_| Error::Signing)
+			.unwrap();
+
+		let mut signed_addresses = vec![];
+		schema::SignedAuthorityAddresses {
+			addresses: serialized_addresses.clone(),
+			signature,
+		}
+			.encode(&mut signed_addresses)
+			.map_err(Error::EncodingProto)
+			.unwrap();
+
+		let key = hash_authority_id(&remote_public.to_raw_vec());
+		let value = signed_addresses;
+		(key, value)
+	};
+
+	let (_dht_event_tx, dht_event_rx) = channel(1);
+	let local_test_api = Arc::new(TestApi {
+		// Make sure the sentry node identifies its validator as an authority.
+		authorities: vec![remote_public.into()],
+	});
+	let local_network: Arc<TestNetwork> = Arc::new(Default::default());
+	let local_key_store = KeyStore::new();
+
+	let (_to_worker, from_service) = mpsc::channel(0);
+	let mut local_worker = Worker::new(
+		from_service,
+		local_test_api,
+		local_network.clone(),
+		vec![],
+		dht_event_rx.boxed(),
+		Role::Authority(local_key_store),
+		None,
+	);
+
+	local_worker.handle_dht_value_found_event(vec![dht_event]).unwrap();
+
+	assert_eq!(
+		Some(&vec![multiaddr_with_peer_id]),
+		local_worker.addr_cache.get_addresses_by_authority_id(&remote_public.into()),
+		"Expect worker to only cache `Multiaddr`s with `PeerId`s.",
+	);
+}