From 9f61b5b9f47eede5391a3b58901af7d6861a8c2f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Silva?=
<123550+andresilva@users.noreply.github.com>
Date: Mon, 19 Oct 2020 18:07:18 +0100
Subject: [PATCH] grandpa: remove duplicate function to fetch local authority
id (#7359)
* grandpa: remove duplicate authority_id function
* grandpa: rename is_voter to local_authority_id
* grandpa: cleanup rebuild_voter telemetry event
---
.../finality-grandpa/src/environment.rs | 14 ++--
substrate/client/finality-grandpa/src/lib.rs | 83 +++++++++----------
2 files changed, 47 insertions(+), 50 deletions(-)
diff --git a/substrate/client/finality-grandpa/src/environment.rs b/substrate/client/finality-grandpa/src/environment.rs
index aeb1cbca67d..95d7adb9578 100644
--- a/substrate/client/finality-grandpa/src/environment.rs
+++ b/substrate/client/finality-grandpa/src/environment.rs
@@ -724,11 +724,11 @@ where
let prevote_timer = Delay::new(self.config.gossip_duration * 2);
let precommit_timer = Delay::new(self.config.gossip_duration * 4);
- let local_key = crate::is_voter(&self.voters, self.config.keystore.as_ref());
+ let local_id = crate::local_authority_id(&self.voters, self.config.keystore.as_ref());
let has_voted = match self.voter_set_state.has_voted(round) {
HasVoted::Yes(id, vote) => {
- if local_key.as_ref().map(|k| k == &id).unwrap_or(false) {
+ if local_id.as_ref().map(|k| k == &id).unwrap_or(false) {
HasVoted::Yes(id, vote)
} else {
HasVoted::No
@@ -739,7 +739,7 @@ where
// we can only sign when we have a local key in the authority set
// and we have a reference to the keystore.
- let keystore = match (local_key.as_ref(), self.config.keystore.as_ref()) {
+ let keystore = match (local_id.as_ref(), self.config.keystore.as_ref()) {
(Some(id), Some(keystore)) => Some((id.clone(), keystore.clone()).into()),
_ => None,
};
@@ -767,7 +767,7 @@ where
let outgoing = Box::pin(outgoing.sink_err_into());
voter::RoundData {
- voter_id: local_key,
+ voter_id: local_id,
prevote_timer: Box::pin(prevote_timer.map(Ok)),
precommit_timer: Box::pin(precommit_timer.map(Ok)),
incoming,
@@ -776,7 +776,7 @@ where
}
fn proposed(&self, round: RoundNumber, propose: PrimaryPropose<Block>) -> Result<(), Self::Error> {
- let local_id = crate::is_voter(&self.voters, self.config.keystore.as_ref());
+ let local_id = crate::local_authority_id(&self.voters, self.config.keystore.as_ref());
let local_id = match local_id {
Some(id) => id,
@@ -815,7 +815,7 @@ where
}
fn prevoted(&self, round: RoundNumber, prevote: Prevote<Block>) -> Result<(), Self::Error> {
- let local_id = crate::is_voter(&self.voters, self.config.keystore.as_ref());
+ let local_id = crate::local_authority_id(&self.voters, self.config.keystore.as_ref());
let local_id = match local_id {
Some(id) => id,
@@ -876,7 +876,7 @@ where
round: RoundNumber,
precommit: Precommit<Block>,
) -> Result<(), Self::Error> {
- let local_id = crate::is_voter(&self.voters, self.config.keystore.as_ref());
+ let local_id = crate::local_authority_id(&self.voters, self.config.keystore.as_ref());
let local_id = match local_id {
Some(id) => id,
diff --git a/substrate/client/finality-grandpa/src/lib.rs b/substrate/client/finality-grandpa/src/lib.rs
index fce252d01f8..6ab95d7eac9 100644
--- a/substrate/client/finality-grandpa/src/lib.rs
+++ b/substrate/client/finality-grandpa/src/lib.rs
@@ -624,7 +624,7 @@ fn global_communication<BE, Block: BlockT, C, N>(
N: NetworkT<Block>,
NumberFor<Block>: BlockNumberOps,
{
- let is_voter = is_voter(voters, keystore).is_some();
+ let is_voter = local_authority_id(voters, keystore).is_some();
// verification stream
let (global_in, global_out) = network.global_communication(
@@ -671,7 +671,8 @@ pub struct GrandpaParams<Block: BlockT, C, N, SC, VR> {
/// block import worker that has already been instantiated with `block_import`.
pub fn run_grandpa_voter<Block: BlockT, BE: 'static, C, N, SC, VR>(
grandpa_params: GrandpaParams<Block, C, N, SC, VR>,
-) -> sp_blockchain::Result<impl Future<Output = ()> + Unpin + Send + 'static> where
+) -> sp_blockchain::Result<impl Future<Output = ()> + Unpin + Send + 'static>
+where
Block::Hash: Ord,
BE: Backend<Block> + 'static,
N: NetworkT<Block> + Send + Sync + Clone + 'static,
@@ -719,21 +720,27 @@ pub fn run_grandpa_voter<Block: BlockT, BE: 'static, C, N, SC, VR>(
let authorities = persistent_data.authority_set.clone();
let events = telemetry_on_connect
.for_each(move |_| {
- let curr = authorities.current_authorities();
- let mut auths = curr.iter().map(|(p, _)| p);
- let maybe_authority_id = authority_id(&mut auths, conf.keystore.as_ref())
+ let current_authorities = authorities.current_authorities();
+ let set_id = authorities.set_id();
+ let authority_id = local_authority_id(¤t_authorities, conf.keystore.as_ref())
.unwrap_or_default();
+ let authorities = current_authorities
+ .iter()
+ .map(|(id, _)| id.to_string())
+ .collect::<Vec<_>>();
+
+ let authorities = serde_json::to_string(&authorities).expect(
+ "authorities is always at least an empty vector; \
+ elements are always of type string",
+ );
+
telemetry!(CONSENSUS_INFO; "afg.authority_set";
- "authority_id" => maybe_authority_id.to_string(),
- "authority_set_id" => ?authorities.set_id(),
- "authorities" => {
- let authorities: Vec<String> = curr.iter()
- .map(|(id, _)| id.to_string()).collect();
- serde_json::to_string(&authorities)
- .expect("authorities is always at least an empty vector; elements are always of type string")
- }
+ "authority_id" => authority_id.to_string(),
+ "authority_set_id" => ?set_id,
+ "authorities" => authorities,
);
+
future::ready(())
});
future::Either::Left(events)
@@ -864,7 +871,7 @@ where
fn rebuild_voter(&mut self) {
debug!(target: "afg", "{}: Starting new voter with set ID {}", self.env.config.name(), self.env.set_id);
- let authority_id = is_voter(&self.env.voters, self.env.config.keystore.as_ref())
+ let authority_id = local_authority_id(&self.env.voters, self.env.config.keystore.as_ref())
.unwrap_or_default();
telemetry!(CONSENSUS_DEBUG; "afg.starting_new_voter";
@@ -874,17 +881,24 @@ where
);
let chain_info = self.env.client.info();
+
+ let authorities = self
+ .env
+ .voters
+ .iter()
+ .map(|(id, _)| id.to_string())
+ .collect::<Vec<_>>();
+
+ let authorities = serde_json::to_string(&authorities).expect(
+ "authorities is always at least an empty vector; elements are always of type string",
+ );
+
telemetry!(CONSENSUS_INFO; "afg.authority_set";
"number" => ?chain_info.finalized_number,
"hash" => ?chain_info.finalized_hash,
"authority_id" => authority_id.to_string(),
"authority_set_id" => ?self.env.set_id,
- "authorities" => {
- let authorities: Vec<String> = self.env.voters
- .iter().map(|(id, _)| id.to_string()).collect();
- serde_json::to_string(&authorities)
- .expect("authorities is always at least an empty vector; elements are always of type string")
- },
+ "authorities" => authorities,
);
match &*self.env.voter_set_state.read() {
@@ -1078,12 +1092,12 @@ where
Ok(())
}
-/// Checks if this node is a voter in the given voter set.
-///
-/// Returns the key pair of the node that is being used in the current voter set or `None`.
-fn is_voter(
- voters: &Arc<VoterSet<AuthorityId>>,
- keystore: Option<& SyncCryptoStorePtr>,
+/// Checks if this node has any available keys in the keystore for any authority id in the given
+/// voter set. Returns the authority id for which keys are available, or `None` if no keys are
+/// available.
+fn local_authority_id(
+ voters: &VoterSet<AuthorityId>,
+ keystore: Option<&SyncCryptoStorePtr>,
) -> Option<AuthorityId> {
match keystore {
Some(keystore) => voters
@@ -1095,20 +1109,3 @@ fn is_voter(
None => None,
}
}
-
-/// Returns the authority id of this node, if available.
-fn authority_id<'a, I>(
- authorities: &mut I,
- keystore: Option<&SyncCryptoStorePtr>,
-) -> Option<AuthorityId> where
- I: Iterator<Item = &'a AuthorityId>,
-{
- match keystore {
- Some(keystore) => {
- authorities
- .find(|p| SyncCryptoStore::has_keys(&**keystore, &[(p.to_raw_vec(), AuthorityId::ID)]))
- .cloned()
- },
- None => None,
- }
-}
--
GitLab