From 6925d543a20a22f49769d61cf72c2605c4835773 Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Thu, 28 May 2020 17:47:39 +0200 Subject: [PATCH 01/10] Use sign_with and stop using `Pair` --- Cargo.lock | 1 + bin/node-template/node/src/service.rs | 2 +- bin/node/cli/src/service.rs | 4 +- client/finality-grandpa/Cargo.toml | 1 + .../finality-grandpa/src/communication/mod.rs | 32 ++++++++---- client/finality-grandpa/src/environment.rs | 12 ++--- client/finality-grandpa/src/import.rs | 1 + client/finality-grandpa/src/lib.rs | 52 +++++++++++++------ client/finality-grandpa/src/observer.rs | 8 +-- client/finality-grandpa/src/tests.rs | 9 ++-- primitives/finality-grandpa/src/lib.rs | 24 ++++++--- 11 files changed, 95 insertions(+), 51 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f3c9672e37ceb..f00c2bb51be77 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6371,6 +6371,7 @@ dependencies = [ "sc-telemetry", "serde_json", "sp-api", + "sp-application-crypto", "sp-arithmetic", "sp-blockchain", "sp-consensus", diff --git a/bin/node-template/node/src/service.rs b/bin/node-template/node/src/service.rs index 8e57a041373a9..91cd8879e0c8b 100644 --- a/bin/node-template/node/src/service.rs +++ b/bin/node-template/node/src/service.rs @@ -140,7 +140,7 @@ pub fn new_full(config: Configuration) -> Result> NetworkBridge { /// network all within the current set. pub(crate) fn round_communication( &self, + keystore: &Option, round: Round, set_id: SetId, voters: Arc>, - local_key: Option, + local_key: Option, has_voted: HasVoted, ) -> ( impl Stream> + Unpin, @@ -287,10 +288,9 @@ impl> NetworkBridge { &*voters, ); - let locals = local_key.and_then(|pair| { - let id = pair.public(); + let locals = local_key.and_then(|id| { if voters.contains(&id) { - Some((pair, id)) + Some(id) } else { None } @@ -350,6 +350,7 @@ impl> NetworkBridge { let (tx, out_rx) = mpsc::channel(0); let outgoing = OutgoingMessages:: { + keystore: keystore.clone(), round: round.0, set_id: set_id.0, network: self.gossip_engine.clone(), @@ -628,10 +629,11 @@ pub struct SetId(pub SetIdNumber); pub(crate) struct OutgoingMessages { round: RoundNumber, set_id: SetIdNumber, - locals: Option<(AuthorityPair, AuthorityId)>, + locals: Option, sender: mpsc::Sender>, network: Arc>>, has_voted: HasVoted, + keystore: Option, } impl Unpin for OutgoingMessages {} @@ -665,14 +667,24 @@ impl Sink> for OutgoingMessages } // when locals exist, sign messages on import - if let Some((ref pair, _)) = self.locals { + if let Some(ref public) = self.locals { + let keystore = match &self.keystore { + Some(keystore) => keystore.clone(), + None => { + return Err(Error::Signing("Cannot sign without a keystore".to_string())) + } + }; + let target_hash = *(msg.target().0); let signed = sp_finality_grandpa::sign_message( + keystore.clone(), msg, - pair, + public, self.round, self.set_id, - ); + ).map_err(|_| { + Error::Signing("Signing failed".to_owned()) + })?; let message = GossipMessage::Vote(VoteMessage:: { message: signed.clone(), diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index afcc3891ac39f..49ff0df74b0ed 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -35,7 +35,6 @@ use finality_grandpa::{ voter, voter_set::VoterSet, }; use sp_blockchain::{HeaderBackend, HeaderMetadata, Error as ClientError}; -use sp_core::Pair; use sp_runtime::generic::BlockId; use sp_runtime::traits::{ Block as BlockT, Header as HeaderT, NumberFor, One, Zero, @@ -708,7 +707,7 @@ where let has_voted = match self.voter_set_state.has_voted(round) { HasVoted::Yes(id, vote) => { - if local_key.as_ref().map(|k| k.public() == id).unwrap_or(false) { + if local_key.clone().map(|k| k == id).unwrap_or(false) { HasVoted::Yes(id, vote) } else { HasVoted::No @@ -718,6 +717,7 @@ where }; let (incoming, outgoing) = self.network.round_communication( + &self.config.keystore, crate::communication::Round(round), crate::communication::SetId(self.set_id), self.voters.clone(), @@ -740,7 +740,7 @@ where let outgoing = Box::pin(outgoing.sink_err_into()); voter::RoundData { - voter_id: local_key.map(|pair| pair.public()), + voter_id: local_key.map(|public| public), prevote_timer: Box::pin(prevote_timer.map(Ok)), precommit_timer: Box::pin(precommit_timer.map(Ok)), incoming, @@ -752,7 +752,7 @@ where let local_id = crate::is_voter(&self.voters, &self.config.keystore); let local_id = match local_id { - Some(id) => id.public(), + Some(id) => id, None => return Ok(()), }; @@ -791,7 +791,7 @@ where let local_id = crate::is_voter(&self.voters, &self.config.keystore); let local_id = match local_id { - Some(id) => id.public(), + Some(id) => id, None => return Ok(()), }; @@ -832,7 +832,7 @@ where let local_id = crate::is_voter(&self.voters, &self.config.keystore); let local_id = match local_id { - Some(id) => id.public(), + Some(id) => id, None => return Ok(()), }; diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 7e3799b1e25e1..b37ab7907a62e 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -669,6 +669,7 @@ where Error::Blockchain(error) => ConsensusError::ClientImport(error), Error::Client(error) => ConsensusError::ClientImport(error.to_string()), Error::Safety(error) => ConsensusError::ClientImport(error), + Error::Signing(error) => ConsensusError::ClientImport(error), Error::Timer(error) => ConsensusError::ClientImport(error.to_string()), }); }, diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 7b20d082a01ab..9eebc7d55bd4a 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -56,8 +56,10 @@ #![warn(missing_docs)] -use futures::prelude::*; -use futures::StreamExt; +use futures::{ + prelude::*, + StreamExt, +}; use log::{debug, info}; use sc_client_api::{ backend::{AuxStore, Backend}, @@ -70,10 +72,13 @@ use sp_api::ProvideRuntimeApi; use sp_blockchain::{HeaderBackend, Error as ClientError, HeaderMetadata}; use sp_runtime::generic::BlockId; use sp_runtime::traits::{NumberFor, Block as BlockT, DigestFor, Zero}; -use sc_keystore::KeyStorePtr; use sp_inherents::InherentDataProviders; use sp_consensus::{SelectChain, BlockImport}; -use sp_core::Pair; +use sp_core::{ + crypto::Public, + traits::BareCryptoStorePtr, +}; +use sp_application_crypto::AppKey; use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver}; use sc_telemetry::{telemetry, CONSENSUS_INFO, CONSENSUS_DEBUG}; use parking_lot::RwLock; @@ -131,10 +136,10 @@ use aux_schema::PersistentData; use environment::{Environment, VoterSetState}; use until_imported::UntilGlobalMessageBlocksImported; use communication::{NetworkBridge, Network as NetworkT}; -use sp_finality_grandpa::{AuthorityList, AuthorityPair, AuthoritySignature, SetId}; +use sp_finality_grandpa::{AuthorityList, AuthoritySignature, SetId}; // Re-export these two because it's just so damn convenient. -pub use sp_finality_grandpa::{AuthorityId, GrandpaApi, ScheduledChange}; +pub use sp_finality_grandpa::{AuthorityId, AuthorityPair, GrandpaApi, ScheduledChange}; use std::marker::PhantomData; #[cfg(test)] @@ -264,7 +269,7 @@ pub struct Config { /// Some local identifier of the voter. pub name: Option, /// The keystore that manages the keys of this node. - pub keystore: Option, + pub keystore: Option, } impl Config { @@ -284,6 +289,8 @@ pub enum Error { Blockchain(String), /// Could not complete a round on disk. Client(ClientError), + /// Could not sign outgoing message + Signing(String), /// An invariant has been violated (e.g. not finalizing pending change blocks in-order) Safety(String), /// A timer failed to fire. @@ -586,7 +593,7 @@ fn global_communication( voters: &Arc>, client: Arc, network: &NetworkBridge, - keystore: &Option, + keystore: &Option, metrics: Option, ) -> ( impl Stream< @@ -866,7 +873,6 @@ where 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) - .map(|ap| ap.public()) .unwrap_or_default(); telemetry!(CONSENSUS_DEBUG; "afg.starting_new_voter"; @@ -1089,12 +1095,20 @@ pub fn setup_disabled_grandpa( /// Returns the key pair of the node that is being used in the current voter set or `None`. fn is_voter( voters: &Arc>, - keystore: &Option, -) -> Option { + keystore: &Option, +) -> Option { match keystore { Some(keystore) => voters .iter() - .find_map(|(p, _)| keystore.read().key_pair::(&p).ok()), + .find_map(|(p, _)| { + let can_vote = keystore.read() + .has_keys(&[(p.to_raw_vec(), AuthorityId::ID)]); + if can_vote { + Some(p.clone()) + } else { + None + } + }), None => None, } } @@ -1102,7 +1116,7 @@ fn is_voter( /// Returns the authority id of this node, if available. fn authority_id<'a, I>( authorities: &mut I, - keystore: &Option, + keystore: &Option, ) -> Option where I: Iterator, { @@ -1110,11 +1124,15 @@ fn authority_id<'a, I>( Some(keystore) => { authorities .find_map(|p| { - keystore.read().key_pair::(&p) - .ok() - .map(|ap| ap.public()) + let can_vote = keystore.read() + .has_keys(&[(p.to_raw_vec(), AuthorityId::ID)]); + if can_vote { + Some(p.clone()) + } else { + None + } }) - } + }, None => None, } } diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index cd678b3bb4542..f7179d70e7a34 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -26,7 +26,7 @@ use finality_grandpa::{ BlockNumberOps, Error as GrandpaError, voter, voter_set::VoterSet }; use log::{debug, info, warn}; - +use sp_core::traits::BareCryptoStorePtr; use sp_consensus::SelectChain; use sc_client_api::backend::Backend; use sp_utils::mpsc::TracingUnboundedReceiver; @@ -211,7 +211,7 @@ struct ObserverWork> { client: Arc, network: NetworkBridge, persistent_data: PersistentData, - keystore: Option, + keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, _phantom: PhantomData, } @@ -228,7 +228,7 @@ where client: Arc, network: NetworkBridge, persistent_data: PersistentData, - keystore: Option, + keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, ) -> Self { @@ -239,7 +239,7 @@ where client, network, persistent_data, - keystore, + keystore: keystore.clone(), voter_commands_rx, _phantom: PhantomData, }; diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 25e6253652001..714101331e0ae 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -282,7 +282,7 @@ fn make_ids(keys: &[Ed25519Keyring]) -> AuthorityList { keys.iter().map(|key| key.clone().public().into()).map(|id| (id, 1)).collect() } -fn create_keystore(authority: Ed25519Keyring) -> (KeyStorePtr, tempfile::TempDir) { +fn create_keystore(authority: Ed25519Keyring) -> (BareCryptoStorePtr, tempfile::TempDir) { let keystore_path = tempfile::tempdir().expect("Creates keystore path"); let keystore = sc_keystore::Store::open(keystore_path.path(), None).expect("Creates keystore"); keystore.write().insert_ephemeral_from_seed::(&authority.to_seed()) @@ -1050,7 +1050,7 @@ fn voter_persists_its_votes() { voter_rx: TracingUnboundedReceiver<()>, net: Arc>, client: PeersClient, - keystore: KeyStorePtr, + keystore: BareCryptoStorePtr, } impl Future for ResettableVoter { @@ -1136,7 +1136,7 @@ fn voter_persists_its_votes() { let config = Config { gossip_duration: TEST_GOSSIP_DURATION, justification_period: 32, - keystore: Some(keystore), + keystore: Some(keystore.clone()), name: Some(format!("peer#{}", 1)), is_authority: true, observer_enabled: true, @@ -1160,10 +1160,11 @@ fn voter_persists_its_votes() { ); let (round_rx, round_tx) = network.round_communication( + &Some(keystore), communication::Round(1), communication::SetId(0), Arc::new(VoterSet::new(voters).unwrap()), - Some(peers[1].pair().into()), + Some(peers[1].public().into()), HasVoted::No, ); diff --git a/primitives/finality-grandpa/src/lib.rs b/primitives/finality-grandpa/src/lib.rs index 2e81c8cecbb70..18d2b087f1cc1 100644 --- a/primitives/finality-grandpa/src/lib.rs +++ b/primitives/finality-grandpa/src/lib.rs @@ -29,6 +29,9 @@ use codec::{Encode, Decode, Input, Codec}; use sp_runtime::{ConsensusEngineId, RuntimeDebug, traits::NumberFor}; use sp_std::borrow::Cow; use sp_std::vec::Vec; +#[cfg(feature = "std")] +use sp_core::traits::BareCryptoStorePtr; +use sp_std::convert::TryInto; #[cfg(feature = "std")] use log::debug; @@ -370,25 +373,32 @@ where /// Localizes the message to the given set and round and signs the payload. #[cfg(feature = "std")] pub fn sign_message( + keystore: BareCryptoStorePtr, message: grandpa::Message, - pair: &AuthorityPair, + public: &AuthorityId, round: RoundNumber, set_id: SetId, -) -> grandpa::SignedMessage +) -> Result, ()> where H: Encode, N: Encode, { - use sp_core::Pair; + use sp_core::crypto::Public; + use sp_application_crypto::AppKey; let encoded = localized_payload(round, set_id, &message); - let signature = pair.sign(&encoded[..]); + let signature = keystore.read() + .sign_with(AuthorityId::ID, &public.to_public_crypto_pair(), &encoded[..]) + .map_err(|_| ())?; + + let signature = signature.clone().try_into() + .map_err(|_| ())?; - grandpa::SignedMessage { + Ok(grandpa::SignedMessage { message, signature, - id: pair.public(), - } + id: public.clone(), + }) } /// WASM function call to check for pending changes. From 918266faefdfb4e8270224ae74245430e45edc85 Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Sat, 30 May 2020 23:11:13 +0200 Subject: [PATCH 02/10] PR feedback --- client/finality-grandpa/src/communication/mod.rs | 8 ++++---- client/finality-grandpa/src/environment.rs | 4 ++-- primitives/finality-grandpa/src/lib.rs | 5 ++--- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 519288b09f44a..1c8de3a864a09 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -288,7 +288,7 @@ impl> NetworkBridge { &*voters, ); - let locals = local_key.and_then(|id| { + let local_id = local_key.and_then(|id| { if voters.contains(&id) { Some(id) } else { @@ -354,7 +354,7 @@ impl> NetworkBridge { round: round.0, set_id: set_id.0, network: self.gossip_engine.clone(), - locals, + local_id, sender: tx, has_voted, }; @@ -629,7 +629,7 @@ pub struct SetId(pub SetIdNumber); pub(crate) struct OutgoingMessages { round: RoundNumber, set_id: SetIdNumber, - locals: Option, + local_id: Option, sender: mpsc::Sender>, network: Arc>>, has_voted: HasVoted, @@ -667,7 +667,7 @@ impl Sink> for OutgoingMessages } // when locals exist, sign messages on import - if let Some(ref public) = self.locals { + if let Some(ref public) = self.local_id { let keystore = match &self.keystore { Some(keystore) => keystore.clone(), None => { diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 49ff0df74b0ed..366af50e6f1a5 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -707,7 +707,7 @@ where let has_voted = match self.voter_set_state.has_voted(round) { HasVoted::Yes(id, vote) => { - if local_key.clone().map(|k| k == id).unwrap_or(false) { + if local_key.as_ref().map(|k| k == &id).unwrap_or(false) { HasVoted::Yes(id, vote) } else { HasVoted::No @@ -740,7 +740,7 @@ where let outgoing = Box::pin(outgoing.sink_err_into()); voter::RoundData { - voter_id: local_key.map(|public| public), + voter_id: local_key, prevote_timer: Box::pin(prevote_timer.map(Ok)), precommit_timer: Box::pin(precommit_timer.map(Ok)), incoming, diff --git a/primitives/finality-grandpa/src/lib.rs b/primitives/finality-grandpa/src/lib.rs index 18d2b087f1cc1..75d340d4f39c3 100644 --- a/primitives/finality-grandpa/src/lib.rs +++ b/primitives/finality-grandpa/src/lib.rs @@ -389,9 +389,8 @@ where let encoded = localized_payload(round, set_id, &message); let signature = keystore.read() .sign_with(AuthorityId::ID, &public.to_public_crypto_pair(), &encoded[..]) - .map_err(|_| ())?; - - let signature = signature.clone().try_into() + .map_err(|_| ())? + .try_into() .map_err(|_| ())?; Ok(grandpa::SignedMessage { From 28c77bfde6c28d9a583f36314d5e48af8f28d73e Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Sun, 31 May 2020 02:34:31 +0200 Subject: [PATCH 03/10] Remove clone --- client/finality-grandpa/src/communication/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 1c8de3a864a09..3afedf6c2e837 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -677,7 +677,7 @@ impl Sink> for OutgoingMessages let target_hash = *(msg.target().0); let signed = sp_finality_grandpa::sign_message( - keystore.clone(), + keystore, msg, public, self.round, From 39955dbf031f4e78739e5c6a05fc5894812d1ba3 Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Sun, 31 May 2020 03:31:22 +0200 Subject: [PATCH 04/10] Transfer ownership of public to sign_message --- client/finality-grandpa/src/communication/mod.rs | 2 +- primitives/finality-grandpa/src/lib.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 3afedf6c2e837..eed3df2423ffb 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -679,7 +679,7 @@ impl Sink> for OutgoingMessages let signed = sp_finality_grandpa::sign_message( keystore, msg, - public, + public.clone(), self.round, self.set_id, ).map_err(|_| { diff --git a/primitives/finality-grandpa/src/lib.rs b/primitives/finality-grandpa/src/lib.rs index 75d340d4f39c3..787bd5fc67747 100644 --- a/primitives/finality-grandpa/src/lib.rs +++ b/primitives/finality-grandpa/src/lib.rs @@ -375,7 +375,7 @@ where pub fn sign_message( keystore: BareCryptoStorePtr, message: grandpa::Message, - public: &AuthorityId, + public: AuthorityId, round: RoundNumber, set_id: SetId, ) -> Result, ()> @@ -396,7 +396,7 @@ where Ok(grandpa::SignedMessage { message, signature, - id: public.clone(), + id: public, }) } From df6891bfbcdabb9ac2b123790a25b6f329537384 Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Mon, 1 Jun 2020 22:38:02 +0200 Subject: [PATCH 05/10] Use Option --- client/finality-grandpa/src/communication/mod.rs | 4 ++-- primitives/finality-grandpa/src/lib.rs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index eed3df2423ffb..a7f2ca98cb58b 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -682,9 +682,9 @@ impl Sink> for OutgoingMessages public.clone(), self.round, self.set_id, - ).map_err(|_| { + ).ok_or( Error::Signing("Signing failed".to_owned()) - })?; + )?; let message = GossipMessage::Vote(VoteMessage:: { message: signed.clone(), diff --git a/primitives/finality-grandpa/src/lib.rs b/primitives/finality-grandpa/src/lib.rs index 787bd5fc67747..889468a352819 100644 --- a/primitives/finality-grandpa/src/lib.rs +++ b/primitives/finality-grandpa/src/lib.rs @@ -378,7 +378,7 @@ pub fn sign_message( public: AuthorityId, round: RoundNumber, set_id: SetId, -) -> Result, ()> +) -> Option> where H: Encode, N: Encode, @@ -389,11 +389,11 @@ where let encoded = localized_payload(round, set_id, &message); let signature = keystore.read() .sign_with(AuthorityId::ID, &public.to_public_crypto_pair(), &encoded[..]) - .map_err(|_| ())? + .ok()? .try_into() - .map_err(|_| ())?; + .ok()?; - Ok(grandpa::SignedMessage { + Some(grandpa::SignedMessage { message, signature, id: public, From d78323e66ecf0b86682c9bbdc218e2e5a4d5f111 Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Mon, 1 Jun 2020 22:38:26 +0200 Subject: [PATCH 06/10] Simplify code --- client/finality-grandpa/src/lib.rs | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 9eebc7d55bd4a..4ce1d54f2d626 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -1100,15 +1100,11 @@ fn is_voter( match keystore { Some(keystore) => voters .iter() - .find_map(|(p, _)| { - let can_vote = keystore.read() - .has_keys(&[(p.to_raw_vec(), AuthorityId::ID)]); - if can_vote { - Some(p.clone()) - } else { - None - } - }), + .find(|(p, _)| { + keystore.read() + .has_keys(&[(p.to_raw_vec(), AuthorityId::ID)]) + }) + .map(|(p, _)| p.clone()), None => None, } } @@ -1123,15 +1119,8 @@ fn authority_id<'a, I>( match keystore { Some(keystore) => { authorities - .find_map(|p| { - let can_vote = keystore.read() - .has_keys(&[(p.to_raw_vec(), AuthorityId::ID)]); - if can_vote { - Some(p.clone()) - } else { - None - } - }) + .find(|p| keystore.read().has_keys(&[(p.to_raw_vec(), AuthorityId::ID)])) + .cloned() }, None => None, } From 6e46595ab23c015f47d2e89870b99f959fff4457 Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Mon, 1 Jun 2020 23:03:17 +0200 Subject: [PATCH 07/10] Fix error message --- client/finality-grandpa/src/communication/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index a7f2ca98cb58b..66fe29608d7e2 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -683,7 +683,9 @@ impl Sink> for OutgoingMessages self.round, self.set_id, ).ok_or( - Error::Signing("Signing failed".to_owned()) + Error::Signing(format!( + "Failed to sign GRANDPA vote for round {} targetting {:?}", self.round, target_hash + )) )?; let message = GossipMessage::Vote(VoteMessage:: { From 581c44ed63ad885f6f3b2d983234a8b22363939e Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Mon, 1 Jun 2020 23:03:54 +0200 Subject: [PATCH 08/10] Pass keystore as ref --- client/finality-grandpa/src/environment.rs | 8 ++++---- client/finality-grandpa/src/lib.rs | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 366af50e6f1a5..432531789e5ea 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -703,7 +703,7 @@ 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); + let local_key = crate::is_voter(&self.voters, self.config.keystore.as_ref()); let has_voted = match self.voter_set_state.has_voted(round) { HasVoted::Yes(id, vote) => { @@ -749,7 +749,7 @@ where } fn proposed(&self, round: RoundNumber, propose: PrimaryPropose) -> Result<(), Self::Error> { - let local_id = crate::is_voter(&self.voters, &self.config.keystore); + let local_id = crate::is_voter(&self.voters, self.config.keystore.as_ref()); let local_id = match local_id { Some(id) => id, @@ -788,7 +788,7 @@ where } fn prevoted(&self, round: RoundNumber, prevote: Prevote) -> Result<(), Self::Error> { - let local_id = crate::is_voter(&self.voters, &self.config.keystore); + let local_id = crate::is_voter(&self.voters, self.config.keystore.as_ref()); let local_id = match local_id { Some(id) => id, @@ -829,7 +829,7 @@ where } fn precommitted(&self, round: RoundNumber, precommit: Precommit) -> Result<(), Self::Error> { - let local_id = crate::is_voter(&self.voters, &self.config.keystore); + let local_id = crate::is_voter(&self.voters, self.config.keystore.as_ref()); let local_id = match local_id { Some(id) => id, diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 4ce1d54f2d626..481544b5c640d 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -609,7 +609,7 @@ fn global_communication( N: NetworkT, NumberFor: BlockNumberOps, { - let is_voter = is_voter(voters, keystore).is_some(); + let is_voter = is_voter(voters, keystore.as_ref()).is_some(); // verification stream let (global_in, global_out) = network.global_communication( @@ -736,7 +736,7 @@ pub fn run_grandpa_voter( .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) + let maybe_authority_id = authority_id(&mut auths, conf.keystore.as_ref()) .unwrap_or_default(); telemetry!(CONSENSUS_INFO; "afg.authority_set"; @@ -872,7 +872,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) + let authority_id = is_voter(&self.env.voters, self.env.config.keystore.as_ref()) .unwrap_or_default(); telemetry!(CONSENSUS_DEBUG; "afg.starting_new_voter"; @@ -1095,7 +1095,7 @@ pub fn setup_disabled_grandpa( /// Returns the key pair of the node that is being used in the current voter set or `None`. fn is_voter( voters: &Arc>, - keystore: &Option, + keystore: Option<&BareCryptoStorePtr>, ) -> Option { match keystore { Some(keystore) => voters @@ -1112,7 +1112,7 @@ fn is_voter( /// Returns the authority id of this node, if available. fn authority_id<'a, I>( authorities: &mut I, - keystore: &Option, + keystore: Option<&BareCryptoStorePtr>, ) -> Option where I: Iterator, { From 44c2e76bbf23ed9a205e70a0938cbccf51b725ee Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Thu, 4 Jun 2020 12:53:37 +0200 Subject: [PATCH 09/10] Pass keystore properly --- client/finality-grandpa/src/communication/mod.rs | 2 +- client/finality-grandpa/src/environment.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 66fe29608d7e2..e331d8b089fc7 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -272,7 +272,7 @@ impl> NetworkBridge { /// network all within the current set. pub(crate) fn round_communication( &self, - keystore: &Option, + keystore: Option, round: Round, set_id: SetId, voters: Arc>, diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 432531789e5ea..6db854bacc13b 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -717,7 +717,7 @@ where }; let (incoming, outgoing) = self.network.round_communication( - &self.config.keystore, + self.config.keystore.clone(), crate::communication::Round(round), crate::communication::SetId(self.set_id), self.voters.clone(), From 788bfd37f021fdca3c82dafd5f3716e28f555f46 Mon Sep 17 00:00:00 2001 From: Rakan Alhneiti Date: Thu, 4 Jun 2020 13:21:11 +0200 Subject: [PATCH 10/10] Fix tests --- client/finality-grandpa/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index 714101331e0ae..ffd8f1c8c642d 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1160,7 +1160,7 @@ fn voter_persists_its_votes() { ); let (round_rx, round_tx) = network.round_communication( - &Some(keystore), + Some(keystore), communication::Round(1), communication::SetId(0), Arc::new(VoterSet::new(voters).unwrap()),