Use sign_with for signing grandpa's outgoing message (#6178)

* Use sign_with and stop using `Pair`

* PR feedback

* Remove clone

* Transfer ownership of public to sign_message

* Use Option

* Simplify code

* Fix error message

* Pass keystore as ref

* Pass keystore properly

* Fix tests

substrate/Cargo.lock

@@ -6269,6 +6269,7 @@ dependencies = [
  "sc-telemetry",
  "serde_json",
  "sp-api",
+ "sp-application-crypto",
  "sp-arithmetic",
  "sp-blockchain",
  "sp-consensus",

substrate/bin/node-template/node/src/service.rs

@@ -148,7 +148,7 @@ pub fn new_full(config: Configuration) -> Result<impl AbstractService, ServiceEr
 	// if the node isn't actively participating in consensus then it doesn't
 	// need a keystore, regardless of which protocol we use below.
 	let keystore = if role.is_authority() {
-		Some(service.keystore())
+		Some(service.keystore() as sp_core::traits::BareCryptoStorePtr)
 	} else {
 		None
 	};

substrate/bin/node/cli/src/service.rs

@@ -21,7 +21,6 @@
 //! Service implementation. Specialized wrapper over substrate service.
 
 use std::sync::Arc;
-
 use sc_consensus_babe;
 use grandpa::{
 	self, FinalityProofProvider as GrandpaFinalityProofProvider, StorageAndProofProvider,
@@ -157,6 +156,7 @@ macro_rules! new_full {
 		use futures::prelude::*;
 		use sc_network::Event;
 		use sc_client_api::ExecutorProvider;
+		use sp_core::traits::BareCryptoStorePtr;
 
 		let (
 			role,
@@ -256,7 +256,7 @@ macro_rules! new_full {
 		// if the node isn't actively participating in consensus then it doesn't
 		// need a keystore, regardless of which protocol we use below.
 		let keystore = if role.is_authority() {
-			Some(service.keystore())
+			Some(service.keystore() as BareCryptoStorePtr)
 		} else {
 			None
 		};

substrate/client/finality-grandpa/Cargo.toml

@@ -23,6 +23,7 @@ parking_lot = "0.10.0"
 rand = "0.7.2"
 assert_matches = "1.3.0"
 parity-scale-codec = { version = "1.3.0", features = ["derive"] }
+sp-application-crypto = { version = "2.0.0-rc3", path = "../../primitives/application-crypto" }
 sp-arithmetic = { version = "2.0.0-rc3", path = "../../primitives/arithmetic" }
 sp-runtime = { version = "2.0.0-rc3", path = "../../primitives/runtime" }
 sp-utils = { version = "2.0.0-rc3", path = "../../primitives/utils" }

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

@@ -35,12 +35,12 @@ use parking_lot::Mutex;
 use prometheus_endpoint::Registry;
 use std::{pin::Pin, sync::Arc, task::{Context, Poll}};
 
+use sp_core::traits::BareCryptoStorePtr;
 use finality_grandpa::Message::{Prevote, Precommit, PrimaryPropose};
 use finality_grandpa::{voter, voter_set::VoterSet};
 use sc_network::{NetworkService, ReputationChange};
 use sc_network_gossip::{GossipEngine, Network as GossipNetwork};
 use parity_scale_codec::{Encode, Decode};
-use sp_core::Pair;
 use sp_runtime::traits::{Block as BlockT, Hash as HashT, Header as HeaderT, NumberFor};
 use sc_telemetry::{telemetry, CONSENSUS_DEBUG, CONSENSUS_INFO};
 
@@ -58,7 +58,7 @@ use gossip::{
 	VoteMessage,
 };
 use sp_finality_grandpa::{
-	AuthorityPair, AuthorityId, AuthoritySignature, SetId as SetIdNumber, RoundNumber,
+	AuthorityId, AuthoritySignature, SetId as SetIdNumber, RoundNumber,
 };
 use sp_utils::mpsc::TracingUnboundedReceiver;
 
@@ -272,10 +272,11 @@ impl<B: BlockT, N: Network<B>> NetworkBridge<B, N> {
 	/// network all within the current set.
 	pub(crate) fn round_communication(
 		&self,
+		keystore: Option<BareCryptoStorePtr>,
 		round: Round,
 		set_id: SetId,
 		voters: Arc<VoterSet<AuthorityId>>,
-		local_key: Option<AuthorityPair>,
+		local_key: Option<AuthorityId>,
 		has_voted: HasVoted<B>,
 	) -> (
 		impl Stream<Item = SignedMessage<B>> + Unpin,
@@ -287,10 +288,9 @@ impl<B: BlockT, N: Network<B>> NetworkBridge<B, N> {
 			&*voters,
 		);
 
-		let locals = local_key.and_then(|pair| {
-			let id = pair.public();
+		let local_id = local_key.and_then(|id| {
 			if voters.contains(&id) {
-				Some((pair, id))
+				Some(id)
 			} else {
 				None
 			}
@@ -350,10 +350,11 @@ impl<B: BlockT, N: Network<B>> NetworkBridge<B, N> {
 
 		let (tx, out_rx) = mpsc::channel(0);
 		let outgoing = OutgoingMessages::<B> {
+			keystore: keystore.clone(),
 			round: round.0,
 			set_id: set_id.0,
 			network: self.gossip_engine.clone(),
-			locals,
+			local_id,
 			sender: tx,
 			has_voted,
 		};
@@ -628,10 +629,11 @@ pub struct SetId(pub SetIdNumber);
 pub(crate) struct OutgoingMessages<Block: BlockT> {
 	round: RoundNumber,
 	set_id: SetIdNumber,
-	locals: Option<(AuthorityPair, AuthorityId)>,
+	local_id: Option<AuthorityId>,
 	sender: mpsc::Sender<SignedMessage<Block>>,
 	network: Arc<Mutex<GossipEngine<Block>>>,
 	has_voted: HasVoted<Block>,
+	keystore: Option<BareCryptoStorePtr>,
 }
 
 impl<B: BlockT> Unpin for OutgoingMessages<B> {}
@@ -665,14 +667,26 @@ impl<Block: BlockT> Sink<Message<Block>> for OutgoingMessages<Block>
 		}
 
 		// when locals exist, sign messages on import
-		if let Some((ref pair, _)) = self.locals {
+		if let Some(ref public) = self.local_id {
+			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,
 				msg,
-				pair,
+				public.clone(),
 				self.round,
 				self.set_id,
-			);
+			).ok_or(
+				Error::Signing(format!(
+					"Failed to sign GRANDPA vote for round {} targetting {:?}", self.round, target_hash
+				))
+			)?;
 
 			let message = GossipMessage::Vote(VoteMessage::<Block> {
 				message: signed.clone(),

substrate/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,
@@ -704,11 +703,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);
+		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) => {
-				if local_key.as_ref().map(|k| k.public() == id).unwrap_or(false) {
+				if local_key.as_ref().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.clone(),
 			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,
 			prevote_timer: Box::pin(prevote_timer.map(Ok)),
 			precommit_timer: Box::pin(precommit_timer.map(Ok)),
 			incoming,
@@ -749,10 +749,10 @@ where
 	}
 
 	fn proposed(&self, round: RoundNumber, propose: PrimaryPropose<Block>) -> 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.public(),
+			Some(id) => id,
 			None => return Ok(()),
 		};
 
@@ -788,10 +788,10 @@ where
 	}
 
 	fn prevoted(&self, round: RoundNumber, prevote: Prevote<Block>) -> 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.public(),
+			Some(id) => id,
 			None => return Ok(()),
 		};
 
@@ -829,10 +829,10 @@ where
 	}
 
 	fn precommitted(&self, round: RoundNumber, precommit: Precommit<Block>) -> 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.public(),
+			Some(id) => id,
 			None => return Ok(()),
 		};
 

substrate/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()),
 				});
 			},

substrate/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<String>,
 	/// The keystore that manages the keys of this node.
-	pub keystore: Option<sc_keystore::KeyStorePtr>,
+	pub keystore: Option<BareCryptoStorePtr>,
 }
 
 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<BE, Block: BlockT, C, N>(
 	voters: &Arc<VoterSet<AuthorityId>>,
 	client: Arc<C>,
 	network: &NetworkBridge<Block, N>,
-	keystore: &Option<KeyStorePtr>,
+	keystore: &Option<BareCryptoStorePtr>,
 	metrics: Option<until_imported::Metrics>,
 ) -> (
 	impl Stream<
@@ -602,7 +609,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 = is_voter(voters, keystore.as_ref()).is_some();
 
 	// verification stream
 	let (global_in, global_out) = network.global_communication(
@@ -729,7 +736,7 @@ pub fn run_grandpa_voter<Block: BlockT, BE: 'static, C, N, SC, VR>(
 			.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";
@@ -865,8 +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)
-			.map(|ap| ap.public())
+		let authority_id = is_voter(&self.env.voters, self.env.config.keystore.as_ref())
 			.unwrap_or_default();
 
 		telemetry!(CONSENSUS_DEBUG; "afg.starting_new_voter";
@@ -1089,12 +1095,16 @@ pub fn setup_disabled_grandpa<Block: BlockT, Client, N>(
 /// 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<KeyStorePtr>,
-) -> Option<AuthorityPair> {
+	keystore: Option<&BareCryptoStorePtr>,
+) -> Option<AuthorityId> {
 	match keystore {
 		Some(keystore) => voters
 			.iter()
-			.find_map(|(p, _)| keystore.read().key_pair::<AuthorityPair>(&p).ok()),
+			.find(|(p, _)| {
+				keystore.read()
+					.has_keys(&[(p.to_raw_vec(), AuthorityId::ID)])
+			})
+			.map(|(p, _)| p.clone()),
 		None => None,
 	}
 }
@@ -1102,19 +1112,16 @@ fn is_voter(
 /// Returns the authority id of this node, if available.
 fn authority_id<'a, I>(
 	authorities: &mut I,
-	keystore: &Option<KeyStorePtr>,
+	keystore: Option<&BareCryptoStorePtr>,
 ) -> Option<AuthorityId> where
 	I: Iterator<Item = &'a AuthorityId>,
 {
 	match keystore {
 		Some(keystore) => {
 			authorities
-				.find_map(|p| {
-					keystore.read().key_pair::<AuthorityPair>(&p)
-						.ok()
-						.map(|ap| ap.public())
-				})
-		}
+				.find(|p| keystore.read().has_keys(&[(p.to_raw_vec(), AuthorityId::ID)]))
+				.cloned()
+		},
 		None => None,
 	}
 }

substrate/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<B: BlockT, BE, Client, N: NetworkT<B>> {
 	client: Arc<Client>,
 	network: NetworkBridge<B, N>,
 	persistent_data: PersistentData<B>,
-	keystore: Option<sc_keystore::KeyStorePtr>,
+	keystore: Option<BareCryptoStorePtr>,
 	voter_commands_rx: TracingUnboundedReceiver<VoterCommand<B::Hash, NumberFor<B>>>,
 	_phantom: PhantomData<BE>,
 }
@@ -228,7 +228,7 @@ where
 		client: Arc<Client>,
 		network: NetworkBridge<B, Network>,
 		persistent_data: PersistentData<B>,
-		keystore: Option<sc_keystore::KeyStorePtr>,
+		keystore: Option<BareCryptoStorePtr>,
 		voter_commands_rx: TracingUnboundedReceiver<VoterCommand<B::Hash, NumberFor<B>>>,
 	) -> Self {
 
@@ -239,7 +239,7 @@ where
 			client,
 			network,
 			persistent_data,
-			keystore,
+			keystore: keystore.clone(),
 			voter_commands_rx,
 			_phantom: PhantomData,
 		};

substrate/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::<AuthorityPair>(&authority.to_seed())
@@ -1050,7 +1050,7 @@ fn voter_persists_its_votes() {
 			voter_rx: TracingUnboundedReceiver<()>,
 			net: Arc<Mutex<GrandpaTestNet>>,
 			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,
 		);
 

substrate/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,31 @@ where
 /// Localizes the message to the given set and round and signs the payload.
 #[cfg(feature = "std")]
 pub fn sign_message<H, N>(
+	keystore: BareCryptoStorePtr,
 	message: grandpa::Message<H, N>,
-	pair: &AuthorityPair,
+	public: AuthorityId,
 	round: RoundNumber,
 	set_id: SetId,
-) -> grandpa::SignedMessage<H, N, AuthoritySignature, AuthorityId>
+) -> Option<grandpa::SignedMessage<H, N, AuthoritySignature, AuthorityId>>
 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[..])
+		.ok()?
+		.try_into()
+		.ok()?;
 
-	grandpa::SignedMessage {
+	Some(grandpa::SignedMessage {
 		message,
 		signature,
-		id: pair.public(),
-	}
+		id: public,
+	})
 }
 
 /// WASM function call to check for pending changes.