Use sign_with in consensus (#6008)

* Add derive_more to sp_core

* Convert Vec to Signature

* Use sign_with in AURA and BABE

* Signing errors

* Update slots to return consensus result

* Fix use

* Clone public key

* Match block_params

* WIP

* Use to_public_crypto_pair

* Pass public key only to block import params

* Address PR review

* Fix consensus RPC

* Fix babe tests

* adjust uses

* Fix line widths

substrate/Cargo.lock

@@ -6247,6 +6247,7 @@ dependencies = [
  "sc-client-api",
  "sc-telemetry",
  "sp-api",
+ "sp-application-crypto",
  "sp-blockchain",
  "sp-consensus",
  "sp-core",
@@ -7464,6 +7465,7 @@ dependencies = [
  "blake2-rfc",
  "byteorder 1.3.4",
  "criterion 0.2.11",
+ "derive_more",
  "ed25519-dalek",
  "futures 0.3.4",
  "hash-db",

substrate/client/consensus/aura/src/lib.rs

@@ -32,7 +32,7 @@
 #![forbid(missing_docs, unsafe_code)]
 use std::{
 	sync::Arc, time::Duration, thread, marker::PhantomData, hash::Hash, fmt::Debug, pin::Pin,
-	collections::HashMap
+	collections::HashMap, convert::{TryFrom, TryInto},
 };
 
 use futures::prelude::*;
@@ -54,11 +54,15 @@ use sp_blockchain::{
 	ProvideCache, HeaderBackend,
 };
 use sp_block_builder::BlockBuilder as BlockBuilderApi;
-use sp_runtime::{generic::{BlockId, OpaqueDigestItemId}, Justification};
+use sp_core::crypto::Public;
+use sp_application_crypto::{AppKey, AppPublic};
+use sp_runtime::{
+	generic::{BlockId, OpaqueDigestItemId},
+	Justification,
+};
 use sp_runtime::traits::{Block as BlockT, Header, DigestItemFor, Zero, Member};
 use sp_api::ProvideRuntimeApi;
-
-use sp_core::crypto::Pair;
+use sp_core::{traits::BareCryptoStore, crypto::Pair};
 use sp_inherents::{InherentDataProviders, InherentData};
 use sp_timestamp::{
 	TimestampInherentData, InherentType as TimestampInherent, InherentError as TIError
@@ -152,8 +156,8 @@ pub fn start_aura<B, C, SC, E, I, P, SO, CAW, Error>(
 	E: Environment<B, Error = Error> + Send + Sync + 'static,
 	E::Proposer: Proposer<B, Error = Error, Transaction = sp_api::TransactionFor<C, B>>,
 	P: Pair + Send + Sync,
-	P::Public: Hash + Member + Encode + Decode,
-	P::Signature: Hash + Member + Encode + Decode,
+	P::Public: AppPublic + Hash + Member + Encode + Decode,
+	P::Signature: TryFrom<Vec<u8>> + Hash + Member + Encode + Decode,
 	I: BlockImport<B, Transaction = sp_api::TransactionFor<C, B>> + Send + Sync + 'static,
 	Error: std::error::Error + Send + From<sp_consensus::Error> + 'static,
 	SO: SyncOracle + Send + Sync + Clone,
@@ -201,8 +205,8 @@ impl<B, C, E, I, P, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for AuraW
 	E::Proposer: Proposer<B, Error = Error, Transaction = sp_api::TransactionFor<C, B>>,
 	I: BlockImport<B, Transaction = sp_api::TransactionFor<C, B>> + Send + Sync + 'static,
 	P: Pair + Send + Sync,
-	P::Public: Member + Encode + Decode + Hash,
-	P::Signature: Member + Encode + Decode + Hash + Debug,
+	P::Public: AppPublic + Public + Member + Encode + Decode + Hash,
+	P::Signature: TryFrom<Vec<u8>> + Member + Encode + Decode + Hash + Debug,
 	SO: SyncOracle + Send + Clone,
 	Error: std::error::Error + Send + From<sp_consensus::Error> + 'static,
 {
@@ -212,7 +216,7 @@ impl<B, C, E, I, P, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for AuraW
 		dyn Future<Output = Result<E::Proposer, sp_consensus::Error>> + Send + 'static
 	>>;
 	type Proposer = E::Proposer;
-	type Claim = P;
+	type Claim = P::Public;
 	type EpochData = Vec<AuthorityId<P>>;
 
 	fn logging_target(&self) -> &'static str {
@@ -241,12 +245,7 @@ impl<B, C, E, I, P, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for AuraW
 		slot_number: u64,
 		epoch_data: &Self::EpochData,
 	) -> Option<Self::Claim> {
-		let expected_author = slot_author::<P>(slot_number, epoch_data);
-
-		expected_author.and_then(|p| {
-			self.keystore.read()
-				.key_pair_by_type::<P>(&p, sp_application_crypto::key_types::AURA).ok()
-		})
+		slot_author::<P>(slot_number, epoch_data).cloned()
 	}
 
 	fn pre_digest_data(
@@ -266,11 +265,30 @@ impl<B, C, E, I, P, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for AuraW
 		StorageChanges<sp_api::TransactionFor<C, B>, B>,
 		Self::Claim,
 		Self::EpochData,
-	) -> sp_consensus::BlockImportParams<B, sp_api::TransactionFor<C, B>> + Send> {
-		Box::new(|header, header_hash, body, storage_changes, pair, _epoch| {
+	) -> Result<
+		sp_consensus::BlockImportParams<B, sp_api::TransactionFor<C, B>>,
+		sp_consensus::Error> + Send + 'static>
+	{
+		let keystore = self.keystore.clone();
+		Box::new(move |header, header_hash, body, storage_changes, public, _epoch| {
 			// sign the pre-sealed hash of the block and then
 			// add it to a digest item.
-			let signature = pair.sign(header_hash.as_ref());
+			let public_type_pair = public.to_public_crypto_pair();
+			let public = public.to_raw_vec();
+			let signature = keystore.read()
+				.sign_with(
+					<AuthorityId<P> as AppKey>::ID,
+					&public_type_pair,
+					header_hash.as_ref()
+				)
+				.map_err(|e| sp_consensus::Error::CannotSign(
+					public.clone(), e.to_string(),
+				))?;
+			let signature = signature.clone().try_into()
+				.map_err(|_| sp_consensus::Error::InvalidSignature(
+					signature, public
+				))?;
+
 			let signature_digest_item = <DigestItemFor<B> as CompatibleDigestItem<P>>::aura_seal(signature);
 
 			let mut import_block = BlockImportParams::new(BlockOrigin::Own, header);
@@ -279,7 +297,7 @@ impl<B, C, E, I, P, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for AuraW
 			import_block.storage_changes = Some(storage_changes);
 			import_block.fork_choice = Some(ForkChoiceStrategy::LongestChain);
 
-			import_block
+			Ok(import_block)
 		})
 	}
 
@@ -333,8 +351,8 @@ impl<B: BlockT, C, E, I, P, Error, SO> SlotWorker<B> for AuraWorker<C, E, I, P,
 	E::Proposer: Proposer<B, Error = Error, Transaction = sp_api::TransactionFor<C, B>>,
 	I: BlockImport<B, Transaction = sp_api::TransactionFor<C, B>> + Send + Sync + 'static,
 	P: Pair + Send + Sync,
-	P::Public: Member + Encode + Decode + Hash,
-	P::Signature: Member + Encode + Decode + Hash + Debug,
+	P::Public: AppPublic + Member + Encode + Decode + Hash,
+	P::Signature: TryFrom<Vec<u8>> + Member + Encode + Decode + Hash + Debug,
 	SO: SyncOracle + Send + Sync + Clone,
 	Error: std::error::Error + Send + From<sp_consensus::Error> + 'static,
 {

substrate/client/consensus/babe/rpc/src/lib.rs

@@ -33,7 +33,6 @@ use sp_consensus_babe::{
 use serde::{Deserialize, Serialize};
 use sc_keystore::KeyStorePtr;
 use sp_api::{ProvideRuntimeApi, BlockId};
-use sp_core::crypto::Pair;
 use sp_runtime::traits::{Block as BlockT, Header as _};
 use sp_consensus::{SelectChain, Error as ConsensusError};
 use sp_blockchain::{HeaderBackend, HeaderMetadata, Error as BlockChainError};
@@ -136,13 +135,13 @@ impl<B, C, SC> BabeApi for BabeRPCHandler<B, C, SC>
 				{
 					match claim {
 						PreDigest::Primary { .. } => {
-							claims.entry(key.public()).or_default().primary.push(slot_number);
+							claims.entry(key).or_default().primary.push(slot_number);
 						}
 						PreDigest::SecondaryPlain { .. } => {
-							claims.entry(key.public()).or_default().secondary.push(slot_number);
+							claims.entry(key).or_default().secondary.push(slot_number);
 						}
 						PreDigest::SecondaryVRF { .. } => {
-							claims.entry(key.public()).or_default().secondary_vrf.push(slot_number);
+							claims.entry(key).or_default().secondary_vrf.push(slot_number);
 						},
 					};
 				}

substrate/client/consensus/babe/src/authorship.rs

@@ -126,7 +126,7 @@ fn claim_secondary_slot(
 	epoch: &Epoch,
 	key_pairs: &[(AuthorityPair, usize)],
 	author_secondary_vrf: bool,
-) -> Option<(PreDigest, AuthorityPair)> {
+) -> Option<(PreDigest, AuthorityId)> {
 	let Epoch { authorities, randomness, epoch_index, .. } = epoch;
 
 	if authorities.is_empty() {
@@ -163,7 +163,7 @@ fn claim_secondary_slot(
 				})
 			};
 
-			return Some((pre_digest, pair.clone()));
+			return Some((pre_digest, pair.public()));
 		}
 	}
 
@@ -178,7 +178,7 @@ pub fn claim_slot(
 	slot_number: SlotNumber,
 	epoch: &Epoch,
 	keystore: &KeyStorePtr,
-) -> Option<(PreDigest, AuthorityPair)> {
+) -> Option<(PreDigest, AuthorityId)> {
 	let key_pairs = {
 		let keystore = keystore.read();
 		epoch.authorities.iter()
@@ -197,7 +197,7 @@ pub fn claim_slot_using_key_pairs(
 	slot_number: SlotNumber,
 	epoch: &Epoch,
 	key_pairs: &[(AuthorityPair, usize)],
-) -> Option<(PreDigest, AuthorityPair)> {
+) -> Option<(PreDigest, AuthorityId)> {
 	claim_primary_slot(slot_number, epoch, epoch.config.c, &key_pairs)
 		.or_else(|| {
 			if epoch.config.allowed_slots.is_secondary_plain_slots_allowed() ||
@@ -229,7 +229,7 @@ fn claim_primary_slot(
 	epoch: &Epoch,
 	c: (u64, u64),
 	key_pairs: &[(AuthorityPair, usize)],
-) -> Option<(PreDigest, AuthorityPair)> {
+) -> Option<(PreDigest, AuthorityId)> {
 	let Epoch { authorities, randomness, epoch_index, .. } = epoch;
 
 	for (pair, authority_index) in key_pairs {
@@ -254,7 +254,7 @@ fn claim_primary_slot(
 
 		// early exit on first successful claim
 		if let Some(pre_digest) = pre_digest {
-			return Some((pre_digest, pair.clone()));
+			return Some((pre_digest, pair.public()));
 		}
 	}
 

substrate/client/consensus/babe/src/lib.rs

@@ -76,12 +76,14 @@ pub use sp_consensus_babe::{
 pub use sp_consensus::SyncOracle;
 use std::{
 	collections::HashMap, sync::Arc, u64, pin::Pin, time::{Instant, Duration},
-	any::Any, borrow::Cow
+	any::Any, borrow::Cow, convert::TryInto,
 };
 use sp_consensus::{ImportResult, CanAuthorWith};
 use sp_consensus::import_queue::{
 	BoxJustificationImport, BoxFinalityProofImport,
 };
+use sp_core::{crypto::Public, traits::BareCryptoStore};
+use sp_application_crypto::AppKey;
 use sp_runtime::{
 	generic::{BlockId, OpaqueDigestItemId}, Justification,
 	traits::{Block as BlockT, Header, DigestItemFor, Zero},
@@ -89,7 +91,6 @@ use sp_runtime::{
 use sp_api::{ProvideRuntimeApi, NumberFor};
 use sc_keystore::KeyStorePtr;
 use parking_lot::Mutex;
-use sp_core::Pair;
 use sp_inherents::{InherentDataProviders, InherentData};
 use sc_telemetry::{telemetry, CONSENSUS_TRACE, CONSENSUS_DEBUG};
 use sp_consensus::{
@@ -440,7 +441,7 @@ impl<B, C, E, I, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for BabeWork
 	Error: std::error::Error + Send + From<ConsensusError> + From<I::Error> + 'static,
 {
 	type EpochData = ViableEpochDescriptor<B::Hash, NumberFor<B>, Epoch>;
-	type Claim = (PreDigest, AuthorityPair);
+	type Claim = (PreDigest, AuthorityId);
 	type SyncOracle = SO;
 	type CreateProposer = Pin<Box<
 		dyn Future<Output = Result<E::Proposer, sp_consensus::Error>> + Send + 'static
@@ -517,12 +518,30 @@ impl<B, C, E, I, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for BabeWork
 		StorageChanges<I::Transaction, B>,
 		Self::Claim,
 		Self::EpochData,
-	) -> sp_consensus::BlockImportParams<B, I::Transaction> + Send> {
-		Box::new(|header, header_hash, body, storage_changes, (_, pair), epoch_descriptor| {
+	) -> Result<
+		sp_consensus::BlockImportParams<B, I::Transaction>,
+		sp_consensus::Error> + Send + 'static>
+	{
+		let keystore = self.keystore.clone();
+		Box::new(move |header, header_hash, body, storage_changes, (_, public), epoch_descriptor| {
 			// sign the pre-sealed hash of the block and then
 			// add it to a digest item.
-			let signature = pair.sign(header_hash.as_ref());
-			let digest_item = <DigestItemFor<B> as CompatibleDigestItem>::babe_seal(signature);
+			let public_type_pair = public.clone().into();
+			let public = public.to_raw_vec();
+			let signature = keystore.read()
+				.sign_with(
+					<AuthorityId as AppKey>::ID,
+					&public_type_pair,
+					header_hash.as_ref()
+				)
+				.map_err(|e| sp_consensus::Error::CannotSign(
+					public.clone(), e.to_string(),
+				))?;
+			let signature: AuthoritySignature = signature.clone().try_into()
+				.map_err(|_| sp_consensus::Error::InvalidSignature(
+					signature, public
+				))?;
+			let digest_item = <DigestItemFor<B> as CompatibleDigestItem>::babe_seal(signature.into());
 
 			let mut import_block = BlockImportParams::new(BlockOrigin::Own, header);
 			import_block.post_digests.push(digest_item);
@@ -533,7 +552,7 @@ impl<B, C, E, I, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for BabeWork
 				Box::new(BabeIntermediate::<B> { epoch_descriptor }) as Box<dyn Any>,
 			);
 
-			import_block
+			Ok(import_block)
 		})
 	}
 

substrate/client/consensus/babe/src/tests.rs

@@ -21,7 +21,7 @@
 #![allow(deprecated)]
 use super::*;
 use authorship::claim_slot;
-
+use sp_core::crypto::Pair;
 use sp_consensus_babe::{AuthorityPair, SlotNumber, AllowedSlots};
 use sc_block_builder::{BlockBuilder, BlockBuilderProvider};
 use sp_consensus::{

substrate/client/consensus/slots/Cargo.toml

@@ -16,6 +16,7 @@ targets = ["x86_64-unknown-linux-gnu"]
 codec = { package = "parity-scale-codec", version = "1.3.0" }
 sc-client-api = { version = "2.0.0-dev", path = "../../api" }
 sp-core = { version = "2.0.0-dev", path = "../../../primitives/core" }
+sp-application-crypto = { version = "2.0.0-dev", path = "../../../primitives/application-crypto" }
 sp-blockchain = { version = "2.0.0-dev", path = "../../../primitives/blockchain" }
 sp-runtime = { version = "2.0.0-dev", path = "../../../primitives/runtime" }
 sp-state-machine = { version = "0.8.0-dev", path = "../../../primitives/state-machine" }

substrate/client/consensus/slots/src/lib.rs

@@ -121,11 +121,10 @@ pub trait SimpleSlotWorker<B: BlockT> {
 			StorageChanges<<Self::BlockImport as BlockImport<B>>::Transaction, B>,
 			Self::Claim,
 			Self::EpochData,
-		) -> sp_consensus::BlockImportParams<
-			B,
-			<Self::BlockImport as BlockImport<B>>::Transaction
-		>
-		+ Send
+		) -> Result<
+				sp_consensus::BlockImportParams<B, <Self::BlockImport as BlockImport<B>>::Transaction>,
+				sp_consensus::Error
+			> + Send + 'static
 	>;
 
 	/// Whether to force authoring if offline.
@@ -273,7 +272,7 @@ pub trait SimpleSlotWorker<B: BlockT> {
 		let block_import = self.block_import();
 		let logging_target = self.logging_target();
 
-		Box::pin(proposal_work.map_ok(move |(proposal, claim)| {
+		Box::pin(proposal_work.and_then(move |(proposal, claim)| {
 			let (header, body) = proposal.block.deconstruct();
 			let header_num = *header.number();
 			let header_hash = header.hash();
@@ -288,6 +287,11 @@ pub trait SimpleSlotWorker<B: BlockT> {
 				epoch_data,
 			);
 
+			let block_import_params = match block_import_params {
+				Ok(params) => params,
+				Err(e) => return future::err(e),
+			};
+
 			info!(
 				"🔖 Pre-sealed block for proposal at {}. Hash now {:?}, previously {:?}.",
 				header_num,
@@ -312,6 +316,7 @@ pub trait SimpleSlotWorker<B: BlockT> {
 					"hash" => ?parent_hash, "err" => ?err,
 				);
 			}
+			future::ready(Ok(()))
 		}))
 	}
 }

substrate/primitives/application-crypto/src/lib.rs

@@ -34,7 +34,11 @@ pub use codec;
 #[cfg(feature = "std")]
 pub use serde;
 #[doc(hidden)]
-pub use sp_std::{ops::Deref, vec::Vec};
+pub use sp_std::{
+	convert::TryFrom,
+	ops::Deref,
+	vec::Vec,
+};
 
 pub mod ed25519;
 pub mod sr25519;
@@ -457,6 +461,14 @@ macro_rules! app_crypto_signature_common {
 		impl $crate::AppSignature for Signature {
 			type Generic = $sig;
 		}
+
+		impl $crate::TryFrom<$crate::Vec<u8>> for Signature {
+			type Error = ();
+
+			fn try_from(data: $crate::Vec<u8>) -> Result<Self, Self::Error> {
+				Ok(<$sig>::try_from(data.as_slice())?.into())
+			}
+		}
 	}
 }
 

substrate/primitives/consensus/common/src/error.rs

@@ -17,7 +17,7 @@
 
 //! Error types in Consensus
 use sp_version::RuntimeVersion;
-use sp_core::ed25519::{Public, Signature};
+use sp_core::ed25519::Public;
 use std::error;
 
 /// Result type alias.
@@ -49,7 +49,7 @@ pub enum Error {
 	CannotPropose,
 	/// Error checking signature
 	#[display(fmt="Message signature {:?} by {:?} is invalid.", _0, _1)]
-	InvalidSignature(Signature, Public),
+	InvalidSignature(Vec<u8>, Vec<u8>),
 	/// Invalid authorities set received from the runtime.
 	#[display(fmt="Current state of blockchain has invalid authorities set")]
 	InvalidAuthoritiesSet,
@@ -80,6 +80,9 @@ pub enum Error {
 	#[display(fmt="Chain lookup failed: {}", _0)]
 	#[from(ignore)]
 	ChainLookup(String),
+	/// Signing failed
+	#[display(fmt="Failed to sign using key: {:?}. Reason: {}", _0, _1)]
+	CannotSign(Vec<u8>, String)
 }
 
 impl error::Error for Error {

substrate/primitives/core/Cargo.toml

@@ -13,6 +13,7 @@ documentation = "https://docs.rs/sp-core"
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
+derive_more = "0.99.2"
 sp-std = { version = "2.0.0-dev", default-features = false, path = "../std" }
 codec = { package = "parity-scale-codec", version = "1.3.0", default-features = false, features = ["derive"] }
 log = { version = "0.4.8", default-features = false }

substrate/primitives/core/src/traits.rs

@@ -32,17 +32,22 @@ use std::{
 pub use sp_externalities::{Externalities, ExternalitiesExt};
 
 /// BareCryptoStore error
-#[derive(Debug)]
+#[derive(Debug, derive_more::Display)]
 pub enum BareCryptoStoreError {
 	/// Public key type is not supported
+	#[display(fmt="Key not supported: {:?}", _0)]
 	KeyNotSupported(KeyTypeId),
 	/// Pair not found for public key and KeyTypeId
+	#[display(fmt="Pair was not found: {}", _0)]
 	PairNotFound(String),
 	/// Validation error
+	#[display(fmt="Validation error: {}", _0)]
 	ValidationError(String),
 	/// Keystore unavailable
+	#[display(fmt="Keystore unavailable")]
 	Unavailable,
 	/// Programming errors
+	#[display(fmt="An unknown keystore error occurred: {}", _0)]
 	Other(String)
 }