Enforce that ProtocolId is a string (#6953)

* Enforce that ProtocolId is a string

* Fix test

client/network/src/behaviour.rs

@@ -80,22 +80,22 @@ pub enum BehaviourOut<B: BlockT> {
 		/// Peer which sent us a request.
 		peer: PeerId,
 		/// Protocol name of the request.
-		protocol: Vec<u8>,
+		protocol: String,
 		/// Time it took to build the response.
 		build_time: Duration,
 	},
 	/// Started a new request with the given node.
 	RequestStarted {
 		peer: PeerId,
 		/// Protocol name of the request.
-		protocol: Vec<u8>,
+		protocol: String,
 	},
 	/// Finished, successfully or not, a previously-started request.
 	RequestFinished {
 		/// Who we were requesting.
 		peer: PeerId,
 		/// Protocol name of the request.
-		protocol: Vec<u8>,
+		protocol: String,
 		/// How long before the response came or the request got cancelled.
 		request_duration: Duration,
 	},
@@ -300,18 +300,18 @@ Behaviour<B, H> {
 					block_requests::SendRequestOutcome::Ok => {
 						self.events.push_back(BehaviourOut::RequestStarted {
 							peer: target,
-							protocol: self.block_requests.protocol_name().to_vec(),
+							protocol: self.block_requests.protocol_name().to_owned(),
 						});
 					},
 					block_requests::SendRequestOutcome::Replaced { request_duration, .. } => {
 						self.events.push_back(BehaviourOut::RequestFinished {
 							peer: target.clone(),
-							protocol: self.block_requests.protocol_name().to_vec(),
+							protocol: self.block_requests.protocol_name().to_owned(),
 							request_duration,
 						});
 						self.events.push_back(BehaviourOut::RequestStarted {
 							peer: target,
-							protocol: self.block_requests.protocol_name().to_vec(),
+							protocol: self.block_requests.protocol_name().to_owned(),
 						});
 					}
 					block_requests::SendRequestOutcome::NotConnected |
@@ -364,14 +364,14 @@ impl<B: BlockT, H: ExHashT> NetworkBehaviourEventProcess<block_requests::Event<B
 			block_requests::Event::AnsweredRequest { peer, total_handling_time } => {
 				self.events.push_back(BehaviourOut::AnsweredRequest {
 					peer,
-					protocol: self.block_requests.protocol_name().to_vec(),
+					protocol: self.block_requests.protocol_name().to_owned(),
 					build_time: total_handling_time,
 				});
 			},
 			block_requests::Event::Response { peer, original_request: _, response, request_duration } => {
 				self.events.push_back(BehaviourOut::RequestFinished {
 					peer: peer.clone(),
-					protocol: self.block_requests.protocol_name().to_vec(),
+					protocol: self.block_requests.protocol_name().to_owned(),
 					request_duration,
 				});
 				let ev = self.substrate.on_block_response(peer, response);
@@ -383,7 +383,7 @@ impl<B: BlockT, H: ExHashT> NetworkBehaviourEventProcess<block_requests::Event<B
 				// we process them by disconnecting the node.
 				self.events.push_back(BehaviourOut::RequestFinished {
 					peer: peer.clone(),
-					protocol: self.block_requests.protocol_name().to_vec(),
+					protocol: self.block_requests.protocol_name().to_owned(),
 					request_duration,
 				});
 				self.substrate.on_block_request_failed(&peer);

client/network/src/block_requests.rs

@@ -124,7 +124,7 @@ pub struct Config {
 	max_response_len: usize,
 	inactivity_timeout: Duration,
 	request_timeout: Duration,
-	protocol: Bytes,
+	protocol: String,
 }
 
 impl Config {
@@ -143,7 +143,7 @@ impl Config {
 			max_response_len: 16 * 1024 * 1024,
 			inactivity_timeout: Duration::from_secs(15),
 			request_timeout: Duration::from_secs(40),
-			protocol: Bytes::new(),
+			protocol: String::new(),
 		};
 		c.set_protocol(id);
 		c
@@ -184,11 +184,11 @@ impl Config {
 
 	/// Set protocol to use for upgrade negotiation.
 	pub fn set_protocol(&mut self, id: &ProtocolId) -> &mut Self {
-		let mut v = Vec::new();
-		v.extend_from_slice(b"/");
-		v.extend_from_slice(id.as_bytes());
-		v.extend_from_slice(b"/sync/2");
-		self.protocol = v.into();
+		let mut s = String::new();
+		s.push_str("/");
+		s.push_str(id.as_ref());
+		s.push_str("/sync/2");
+		self.protocol = s;
 		self
 	}
 }
@@ -258,7 +258,7 @@ where
 	}
 
 	/// Returns the libp2p protocol name used on the wire (e.g. `/foo/sync/2`).
-	pub fn protocol_name(&self) -> &[u8] {
+	pub fn protocol_name(&self) -> &str {
 		&self.config.protocol
 	}
 
@@ -322,7 +322,7 @@ where
 				request: buf,
 				original_request: req,
 				max_response_size: self.config.max_response_len,
-				protocol: self.config.protocol.clone(),
+				protocol: self.config.protocol.as_bytes().to_vec().into(),
 			},
 		});
 
@@ -472,7 +472,7 @@ where
 	fn new_handler(&mut self) -> Self::ProtocolsHandler {
 		let p = InboundProtocol {
 			max_request_len: self.config.max_request_len,
-			protocol: self.config.protocol.clone(),
+			protocol: self.config.protocol.as_bytes().to_owned().into(),
 			marker: PhantomData,
 		};
 		let mut cfg = OneShotHandlerConfig::default();

client/network/src/config.rs

@@ -48,6 +48,7 @@ use std::{
 	io::{self, Write},
 	net::Ipv4Addr,
 	path::{Path, PathBuf},
+	str,
 	sync::Arc,
 };
 use zeroize::Zeroize;
@@ -233,20 +234,26 @@ impl<H: ExHashT + Default, B: BlockT> TransactionPool<H, B> for EmptyTransaction
 	fn transaction(&self, _h: &H) -> Option<B::Extrinsic> { None }
 }
 
-/// Name of a protocol, transmitted on the wire. Should be unique for each chain.
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+/// Name of a protocol, transmitted on the wire. Should be unique for each chain. Always UTF-8.
+#[derive(Clone, PartialEq, Eq, Hash)]
 pub struct ProtocolId(smallvec::SmallVec<[u8; 6]>);
 
-impl<'a> From<&'a [u8]> for ProtocolId {
-	fn from(bytes: &'a [u8]) -> ProtocolId {
-		ProtocolId(bytes.into())
+impl<'a> From<&'a str> for ProtocolId {
+	fn from(bytes: &'a str) -> ProtocolId {
+		ProtocolId(bytes.as_bytes().into())
 	}
 }
 
-impl ProtocolId {
-	/// Exposes the `ProtocolId` as bytes.
-	pub fn as_bytes(&self) -> &[u8] {
-		self.0.as_ref()
+impl AsRef<str> for ProtocolId {
+	fn as_ref(&self) -> &str {
+		str::from_utf8(&self.0[..])
+			.expect("the only way to build a ProtocolId is through a UTF-8 String; qed")
+	}
+}
+
+impl fmt::Debug for ProtocolId {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		fmt::Debug::fmt(self.as_ref(), f)
 	}
 }
 

client/network/src/discovery.rs

@@ -752,7 +752,7 @@ impl NetworkBehaviour for DiscoveryBehaviour {
 // `DiscoveryBehaviour::new_handler` is still correct.
 fn protocol_name_from_protocol_id(id: &ProtocolId) -> Vec<u8> {
 	let mut v = vec![b'/'];
-	v.extend_from_slice(id.as_bytes());
+	v.extend_from_slice(id.as_ref().as_bytes());
 	v.extend_from_slice(b"/kad");
 	v
 }
@@ -773,7 +773,7 @@ mod tests {
 	#[test]
 	fn discovery_working() {
 		let mut first_swarm_peer_id_and_addr = None;
-		let protocol_id = ProtocolId::from(b"dot".as_ref());
+		let protocol_id = ProtocolId::from("dot");
 
 		// Build swarms whose behaviour is `DiscoveryBehaviour`, each aware of
 		// the first swarm via `with_user_defined`.
@@ -877,8 +877,8 @@ mod tests {
 
 	#[test]
 	fn discovery_ignores_peers_with_unknown_protocols() {
-		let supported_protocol_id = ProtocolId::from(b"a".as_ref());
-		let unsupported_protocol_id = ProtocolId::from(b"b".as_ref());
+		let supported_protocol_id = ProtocolId::from("a");
+		let unsupported_protocol_id = ProtocolId::from("b");
 
 		let mut discovery = {
 			let keypair = Keypair::generate_ed25519();
@@ -929,8 +929,8 @@ mod tests {
 
 	#[test]
 	fn discovery_adds_peer_to_kademlia_of_same_protocol_only() {
-		let protocol_a = ProtocolId::from(b"a".as_ref());
-		let protocol_b = ProtocolId::from(b"b".as_ref());
+		let protocol_a = ProtocolId::from("a");
+		let protocol_b = ProtocolId::from("b");
 
 		let mut discovery = {
 			let keypair = Keypair::generate_ed25519();

client/network/src/finality_requests.rs

@@ -129,7 +129,7 @@ impl Config {
 	pub fn set_protocol(&mut self, id: &ProtocolId) -> &mut Self {
 		let mut v = Vec::new();
 		v.extend_from_slice(b"/");
-		v.extend_from_slice(id.as_bytes());
+		v.extend_from_slice(id.as_ref().as_bytes());
 		v.extend_from_slice(b"/finality-proof/1");
 		self.protocol = v.into();
 		self

client/network/src/gossip/tests.rs

@@ -100,7 +100,7 @@ fn build_test_full_node(config: config::NetworkConfiguration)
 		finality_proof_request_builder: None,
 		on_demand: None,
 		transaction_pool: Arc::new(crate::config::EmptyTransactionPool),
-		protocol_id: config::ProtocolId::from(&b"/test-protocol-name"[..]),
+		protocol_id: config::ProtocolId::from("/test-protocol-name"),
 		import_queue,
 		block_announce_validator: Box::new(
 			sp_consensus::block_validation::DefaultBlockAnnounceValidator,

client/network/src/light_client_handler.rs

@@ -156,13 +156,13 @@ impl Config {
 	pub fn set_protocol(&mut self, id: &ProtocolId) -> &mut Self {
 		let mut vl = Vec::new();
 		vl.extend_from_slice(b"/");
-		vl.extend_from_slice(id.as_bytes());
+		vl.extend_from_slice(id.as_ref().as_bytes());
 		vl.extend_from_slice(b"/light/2");
 		self.light_protocol = vl.into();
 
 		let mut vb = Vec::new();
 		vb.extend_from_slice(b"/");
-		vb.extend_from_slice(id.as_bytes());
+		vb.extend_from_slice(id.as_ref().as_bytes());
 		vb.extend_from_slice(b"/sync/2");
 		self.block_protocol = vb.into();
 
@@ -1447,7 +1447,7 @@ mod tests {
 	}
 
 	fn make_config() -> super::Config {
-		super::Config::new(&ProtocolId::from(&b"foo"[..]))
+		super::Config::new(&ProtocolId::from("foo"))
 	}
 
 	fn dummy_header() -> sp_test_primitives::Header {

client/network/src/protocol.rs

@@ -419,7 +419,7 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
 
 		let transactions_protocol: Cow<'static, [u8]> = Cow::from({
 			let mut proto = b"/".to_vec();
-			proto.extend(protocol_id.as_bytes());
+			proto.extend(protocol_id.as_ref().as_bytes());
 			proto.extend(b"/transactions/1");
 			proto
 		});
@@ -428,7 +428,7 @@ impl<B: BlockT, H: ExHashT> Protocol<B, H> {
 
 		let block_announces_protocol: Cow<'static, [u8]> = Cow::from({
 			let mut proto = b"/".to_vec();
-			proto.extend(protocol_id.as_bytes());
+			proto.extend(protocol_id.as_ref().as_bytes());
 			proto.extend(b"/block-announces/1");
 			proto
 		});

client/network/src/protocol/generic_proto/tests.rs

@@ -83,7 +83,7 @@ fn build_nodes() -> (Swarm<CustomProtoWithAddr>, Swarm<CustomProtoWithAddr>) {
 		});
 
 		let behaviour = CustomProtoWithAddr {
-			inner: GenericProto::new(local_peer_id, &b"test"[..], &[1], vec![], peerset),
+			inner: GenericProto::new(local_peer_id, "test", &[1], vec![], peerset),
 			addrs: addrs
 				.iter()
 				.enumerate()

client/network/src/protocol/generic_proto/upgrade/legacy.rs

@@ -49,7 +49,7 @@ impl RegisteredProtocol {
 		-> Self {
 		let protocol = protocol.into();
 		let mut base_name = b"/substrate/".to_vec();
-		base_name.extend_from_slice(protocol.as_bytes());
+		base_name.extend_from_slice(protocol.as_ref().as_bytes());
 		base_name.extend_from_slice(b"/");
 
 		RegisteredProtocol {

client/network/src/service.rs

@@ -1497,28 +1497,28 @@ impl<B: BlockT + 'static, H: ExHashT> Future for NetworkWorker<B, H> {
 				Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::AnsweredRequest { protocol, build_time, .. })) => {
 					if let Some(metrics) = this.metrics.as_ref() {
 						metrics.requests_in_total
-							.with_label_values(&[&maybe_utf8_bytes_to_string(&protocol)])
+							.with_label_values(&[&protocol])
 							.observe(build_time.as_secs_f64());
 					}
 				},
 				Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::RequestStarted { protocol, .. })) => {
 					if let Some(metrics) = this.metrics.as_ref() {
 						metrics.requests_out_started_total
-							.with_label_values(&[&maybe_utf8_bytes_to_string(&protocol)])
+							.with_label_values(&[&protocol])
 							.inc();
 					}
 				},
 				Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::RequestFinished { protocol, request_duration, .. })) => {
 					if let Some(metrics) = this.metrics.as_ref() {
 						metrics.requests_out_finished
-							.with_label_values(&[&maybe_utf8_bytes_to_string(&protocol)])
+							.with_label_values(&[&protocol])
 							.observe(request_duration.as_secs_f64());
 					}
 				},
 				Poll::Ready(SwarmEvent::Behaviour(BehaviourOut::RandomKademliaStarted(protocol))) => {
 					if let Some(metrics) = this.metrics.as_ref() {
 						metrics.kademlia_random_queries_total
-							.with_label_values(&[&maybe_utf8_bytes_to_string(protocol.as_bytes())])
+							.with_label_values(&[&protocol.as_ref()])
 							.inc();
 					}
 				},
@@ -1776,16 +1776,13 @@ impl<B: BlockT + 'static, H: ExHashT> Future for NetworkWorker<B, H> {
 		if let Some(metrics) = this.metrics.as_ref() {
 			metrics.is_major_syncing.set(is_major_syncing as u64);
 			for (proto, num_entries) in this.network_service.num_kbuckets_entries() {
-				let proto = maybe_utf8_bytes_to_string(proto.as_bytes());
-				metrics.kbuckets_num_nodes.with_label_values(&[&proto]).set(num_entries as u64);
+				metrics.kbuckets_num_nodes.with_label_values(&[&proto.as_ref()]).set(num_entries as u64);
 			}
 			for (proto, num_entries) in this.network_service.num_kademlia_records() {
-				let proto = maybe_utf8_bytes_to_string(proto.as_bytes());
-				metrics.kademlia_records_count.with_label_values(&[&proto]).set(num_entries as u64);
+				metrics.kademlia_records_count.with_label_values(&[&proto.as_ref()]).set(num_entries as u64);
 			}
 			for (proto, num_entries) in this.network_service.kademlia_records_total_size() {
-				let proto = maybe_utf8_bytes_to_string(proto.as_bytes());
-				metrics.kademlia_records_sizes_total.with_label_values(&[&proto]).set(num_entries as u64);
+				metrics.kademlia_records_sizes_total.with_label_values(&[&proto.as_ref()]).set(num_entries as u64);
 			}
 			metrics.peers_count.set(num_connected_peers as u64);
 			metrics.peerset_num_discovered.set(this.network_service.user_protocol().num_discovered_peers() as u64);

client/network/src/service/tests.rs

@@ -101,7 +101,7 @@ fn build_test_full_node(config: config::NetworkConfiguration)
 		finality_proof_request_builder: None,
 		on_demand: None,
 		transaction_pool: Arc::new(crate::config::EmptyTransactionPool),
-		protocol_id: config::ProtocolId::from(&b"/test-protocol-name"[..]),
+		protocol_id: config::ProtocolId::from("/test-protocol-name"),
 		import_queue,
 		block_announce_validator: Box::new(
 			sp_consensus::block_validation::DefaultBlockAnnounceValidator,

client/network/test/src/lib.rs

@@ -675,7 +675,7 @@ pub trait TestNetFactory: Sized {
 			finality_proof_request_builder,
 			on_demand: None,
 			transaction_pool: Arc::new(EmptyTransactionPool),
-			protocol_id: ProtocolId::from(&b"test-protocol-name"[..]),
+			protocol_id: ProtocolId::from("test-protocol-name"),
 			import_queue,
 			block_announce_validator: config.block_announce_validator
 				.unwrap_or_else(|| Box::new(DefaultBlockAnnounceValidator)),
@@ -755,7 +755,7 @@ pub trait TestNetFactory: Sized {
 			finality_proof_request_builder,
 			on_demand: None,
 			transaction_pool: Arc::new(EmptyTransactionPool),
-			protocol_id: ProtocolId::from(&b"test-protocol-name"[..]),
+			protocol_id: ProtocolId::from("test-protocol-name"),
 			import_queue,
 			block_announce_validator: Box::new(DefaultBlockAnnounceValidator),
 			metrics_registry: None,

client/service/src/builder.rs

@@ -854,7 +854,7 @@ pub fn build_network<TBl, TExPool, TImpQu, TCl>(
 				);
 				DEFAULT_PROTOCOL_ID
 			}
-		}.as_bytes();
+		};
 		sc_network::config::ProtocolId::from(protocol_id_full)
 	};