BEEFY and GRANDPA protocol names should use full genesis hash (#10974)

std::fmt::Display shows formats as reduced hash (e.g. 0xb0a8…dafe)

Use hex::encode to format full hash.

Signed-off-by: acatangiu <adrian@parity.io>

substrate/Cargo.lock

@@ -484,6 +484,7 @@ dependencies = [
  "beefy-primitives",
  "fnv",
  "futures 0.3.19",
+ "hex",
  "log 0.4.14",
  "parity-scale-codec",
  "parking_lot 0.12.0",
@@ -494,6 +495,7 @@ dependencies = [
  "sc-network-gossip",
  "sc-network-test",
  "sc-utils",
+ "serde",
  "sp-api",
  "sp-application-crypto",
  "sp-arithmetic",
@@ -8525,6 +8527,7 @@ dependencies = [
  "fork-tree",
  "futures 0.3.19",
  "futures-timer",
+ "hex",
  "log 0.4.14",
  "parity-scale-codec",
  "parking_lot 0.12.0",
@@ -8539,6 +8542,7 @@ dependencies = [
  "sc-network-test",
  "sc-telemetry",
  "sc-utils",
+ "serde",
  "serde_json",
  "sp-api",
  "sp-application-crypto",

substrate/client/beefy/Cargo.toml

@@ -10,6 +10,7 @@ description = "BEEFY Client gadget for substrate"
 [dependencies]
 fnv = "1.0.6"
 futures = "0.3"
+hex = "0.4.2"
 log = "0.4"
 parking_lot = "0.12.0"
 thiserror = "1.0"
@@ -39,4 +40,5 @@ beefy-primitives = { version = "4.0.0-dev", path = "../../primitives/beefy" }
 sp-tracing = { version = "5.0.0", path = "../../primitives/tracing" }
 sc-network-test = { version = "0.8.0", path = "../network/test" }
 
+serde = "1.0.136"
 strum = { version = "0.23", features = ["derive"] }

substrate/client/beefy/src/lib.rs

@@ -53,13 +53,13 @@ pub(crate) mod beefy_protocol_name {
 	/// Name of the notifications protocol used by BEEFY.
 	///
 	/// Must be registered towards the networking in order for BEEFY to properly function.
-	pub fn standard_name<Hash: std::fmt::Display>(
+	pub fn standard_name<Hash: AsRef<[u8]>>(
 		genesis_hash: &Hash,
 		chain_spec: &Box<dyn ChainSpec>,
 	) -> std::borrow::Cow<'static, str> {
 		let chain_prefix = match chain_spec.fork_id() {
-			Some(fork_id) => format!("/{}/{}", genesis_hash, fork_id),
+			Some(fork_id) => format!("/{}/{}", hex::encode(genesis_hash), fork_id),
-			None => format!("/{}", genesis_hash),
+			None => format!("/{}", hex::encode(genesis_hash)),
 		};
 		format!("{}{}", chain_prefix, NAME).into()
 	}
@@ -190,3 +190,48 @@ where
 
 	worker.run().await
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use sc_chain_spec::{ChainSpec, GenericChainSpec};
+	use serde::{Deserialize, Serialize};
+	use sp_core::H256;
+	use sp_runtime::{BuildStorage, Storage};
+
+	#[derive(Debug, Serialize, Deserialize)]
+	struct Genesis(std::collections::BTreeMap<String, String>);
+	impl BuildStorage for Genesis {
+		fn assimilate_storage(&self, storage: &mut Storage) -> Result<(), String> {
+			storage.top.extend(
+				self.0.iter().map(|(a, b)| (a.clone().into_bytes(), b.clone().into_bytes())),
+			);
+			Ok(())
+		}
+	}
+
+	#[test]
+	fn beefy_protocol_name() {
+		let chain_spec = GenericChainSpec::<Genesis>::from_json_file(std::path::PathBuf::from(
+			"../chain-spec/res/chain_spec.json",
+		))
+		.unwrap()
+		.cloned_box();
+
+		// Create protocol name using random genesis hash.
+		let genesis_hash = H256::random();
+		let expected = format!("/{}/beefy/1", hex::encode(genesis_hash));
+		let proto_name = beefy_protocol_name::standard_name(&genesis_hash, &chain_spec);
+		assert_eq!(proto_name.to_string(), expected);
+
+		// Create protocol name using hardcoded genesis hash. Verify exact representation.
+		let genesis_hash = [
+			50, 4, 60, 123, 58, 106, 216, 246, 194, 188, 139, 193, 33, 212, 202, 171, 9, 55, 123,
+			94, 8, 43, 12, 251, 187, 57, 173, 19, 188, 74, 205, 147,
+		];
+		let expected =
+			"/32043c7b3a6ad8f6c2bc8bc121d4caab09377b5e082b0cfbbb39ad13bc4acd93/beefy/1".to_string();
+		let proto_name = beefy_protocol_name::standard_name(&genesis_hash, &chain_spec);
+		assert_eq!(proto_name.to_string(), expected);
+	}
+}

substrate/client/finality-grandpa/Cargo.toml

@@ -19,6 +19,7 @@ dyn-clone = "1.0"
 fork-tree = { version = "3.0.0", path = "../../utils/fork-tree" }
 futures = "0.3.19"
 futures-timer = "3.0.1"
+hex = "0.4.2"
 log = "0.4.8"
 parking_lot = "0.12.0"
 rand = "0.8.4"
@@ -58,5 +59,7 @@ sc-network-test = { version = "0.8.0", path = "../network/test" }
 sp-keyring = { version = "6.0.0", path = "../../primitives/keyring" }
 substrate-test-runtime-client = { version = "2.0.0", path = "../../test-utils/runtime/client" }
 sp-tracing = { version = "5.0.0", path = "../../primitives/tracing" }
+
+serde = "1.0.136"
 tokio = "1.15"
 tempfile = "3.1.0"

substrate/client/finality-grandpa/src/communication/mod.rs

@@ -77,13 +77,13 @@ pub mod grandpa_protocol_name {
 	/// Name of the notifications protocol used by GRANDPA.
 	///
 	/// Must be registered towards the networking in order for GRANDPA to properly function.
-	pub fn standard_name<Hash: std::fmt::Display>(
+	pub fn standard_name<Hash: AsRef<[u8]>>(
 		genesis_hash: &Hash,
 		chain_spec: &Box<dyn ChainSpec>,
 	) -> std::borrow::Cow<'static, str> {
 		let chain_prefix = match chain_spec.fork_id() {
-			Some(fork_id) => format!("/{}/{}", genesis_hash, fork_id),
+			Some(fork_id) => format!("/{}/{}", hex::encode(genesis_hash), fork_id),
-			None => format!("/{}", genesis_hash),
+			None => format!("/{}", hex::encode(genesis_hash)),
 		};
 		format!("{}{}", chain_prefix, NAME).into()
 	}

substrate/client/finality-grandpa/src/communication/tests.rs

@@ -535,3 +535,46 @@ fn peer_with_higher_view_leads_to_catch_up_request() {
 
 	futures::executor::block_on(test);
 }
+
+fn local_chain_spec() -> Box<dyn sc_chain_spec::ChainSpec> {
+	use sc_chain_spec::{ChainSpec, GenericChainSpec};
+	use serde::{Deserialize, Serialize};
+	use sp_runtime::{BuildStorage, Storage};
+
+	#[derive(Debug, Serialize, Deserialize)]
+	struct Genesis(std::collections::BTreeMap<String, String>);
+	impl BuildStorage for Genesis {
+		fn assimilate_storage(&self, storage: &mut Storage) -> Result<(), String> {
+			storage.top.extend(
+				self.0.iter().map(|(a, b)| (a.clone().into_bytes(), b.clone().into_bytes())),
+			);
+			Ok(())
+		}
+	}
+	let chain_spec = GenericChainSpec::<Genesis>::from_json_file(std::path::PathBuf::from(
+		"../chain-spec/res/chain_spec.json",
+	))
+	.unwrap();
+	chain_spec.cloned_box()
+}
+
+#[test]
+fn grandpa_protocol_name() {
+	let chain_spec = local_chain_spec();
+
+	// Create protocol name using random genesis hash.
+	let genesis_hash = sp_core::H256::random();
+	let expected = format!("/{}/grandpa/1", hex::encode(genesis_hash));
+	let proto_name = grandpa_protocol_name::standard_name(&genesis_hash, &chain_spec);
+	assert_eq!(proto_name.to_string(), expected);
+
+	// Create protocol name using hardcoded genesis hash. Verify exact representation.
+	let genesis_hash = [
+		53, 79, 112, 97, 119, 217, 39, 202, 147, 138, 225, 38, 88, 182, 215, 185, 110, 88, 8, 53,
+		125, 210, 158, 151, 50, 113, 102, 59, 245, 199, 221, 240,
+	];
+	let expected =
+		"/354f706177d927ca938ae12658b6d7b96e5808357dd29e973271663bf5c7ddf0/grandpa/1".to_string();
+	let proto_name = grandpa_protocol_name::standard_name(&genesis_hash, &chain_spec);
+	assert_eq!(proto_name.to_string(), expected);
+}