Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

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

Merged
merged 1 commit into from
Mar 4, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions client/beefy/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"] }
51 changes: 48 additions & 3 deletions client/beefy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
None => format!("/{}", genesis_hash),
Some(fork_id) => format!("/{}/{}", hex::encode(genesis_hash), fork_id),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is not tested because local chain spec doesn't have a fork_id.
But since fork_id is a simple &str there's not much can go wrong when printing it.

If we want to test this as well, I can switch from using local chain_spec to defining a mock one with customizable fork_id next to all the required traits..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing we could do is to replace chain_spec: &Box<dyn ChainSpec> in params with fork_id: Option<&str>, which also removes the sc-chain-spec dependency from GRANDPA and BEEFY clients, but is an API break, implies companion PRs and arguably makes the protocol name easier to misconfigure.

None => format!("/{}", hex::encode(genesis_hash)),
};
format!("{}{}", chain_prefix, NAME).into()
}
Expand Down Expand Up @@ -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);
}
}
3 changes: 3 additions & 0 deletions client/finality-grandpa/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
6 changes: 3 additions & 3 deletions client/finality-grandpa/src/communication/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,13 @@ pub mod grandpa_protocol_name {
/// Name of the notifications protocol used by GRANDPA.
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
///
/// 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),
None => format!("/{}", genesis_hash),
Some(fork_id) => format!("/{}/{}", hex::encode(genesis_hash), fork_id),
None => format!("/{}", hex::encode(genesis_hash)),
};
format!("{}{}", chain_prefix, NAME).into()
}
Expand Down
43 changes: 43 additions & 0 deletions client/finality-grandpa/src/communication/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}