Keystore overhaul (final) (#13683)

* Introduce keystore specialized sign methods

* Get rid of 'AppKey::UntypedGeneric' associated type.

Untyped generics are accessible using associated types 'Generic' associated type.
I.e. <T as AppKey>::Public::Generic

* Get rid of 'CryptoTypePublicPair'

* Trivial fix

* Small refactory of local keystore implementations

* Remove 'crypto_id' method from 'Public'

* Trivial rename of 'AppKey' to 'AppCrypto'

* Remove unused import

* Improve docs

* Better signature related errors for authority-discovery

* Apply review suggestion

* Apply review suggestions

Co-authored-by: Koute <koute@users.noreply.github.com>

* Authority discoverty signing error revisited

* Signing error revisited for babe and aura as well

* Further cleanup

---------

Co-authored-by: Koute <koute@users.noreply.github.com>

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

@@ -45,7 +45,7 @@ use sc_consensus_slots::{
 };
 use sc_telemetry::TelemetryHandle;
 use sp_api::{Core, ProvideRuntimeApi};
-use sp_application_crypto::{AppKey, AppPublic};
+use sp_application_crypto::{AppCrypto, AppPublic};
 use sp_blockchain::{HeaderBackend, Result as CResult};
 use sp_consensus::{BlockOrigin, Environment, Error as ConsensusError, Proposer, SelectChain};
 use sp_consensus_slots::Slot;
@@ -205,7 +205,7 @@ pub fn start_aura<P, B, C, SC, I, PF, SO, L, CIDP, BS, Error>(
 		telemetry,
 		compatibility_mode,
 	}: StartAuraParams<C, SC, I, PF, SO, L, CIDP, BS, NumberFor<B>>,
-) -> Result<impl Future<Output = ()>, sp_consensus::Error>
+) -> Result<impl Future<Output = ()>, ConsensusError>
 where
 	P: Pair + Send + Sync,
 	P::Public: AppPublic + Hash + Member + Encode + Decode,
@@ -222,7 +222,7 @@ where
 	CIDP: CreateInherentDataProviders<B, ()> + Send + 'static,
 	CIDP::InherentDataProviders: InherentDataProviderExt + Send,
 	BS: BackoffAuthoringBlocksStrategy<NumberFor<B>> + Send + Sync + 'static,
-	Error: std::error::Error + Send + From<sp_consensus::Error> + 'static,
+	Error: std::error::Error + Send + From<ConsensusError> + 'static,
 {
 	let worker = build_aura_worker::<P, _, _, _, _, _, _, _, _>(BuildAuraWorkerParams {
 		client,
@@ -320,7 +320,7 @@ where
 	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,
+	Error: std::error::Error + Send + From<ConsensusError> + 'static,
 	SO: SyncOracle + Send + Sync + Clone,
 	L: sc_consensus::JustificationSyncLink<B>,
 	BS: BackoffAuthoringBlocksStrategy<NumberFor<B>> + Send + Sync + 'static,
@@ -374,13 +374,13 @@ where
 	SO: SyncOracle + Send + Clone + Sync,
 	L: sc_consensus::JustificationSyncLink<B>,
 	BS: BackoffAuthoringBlocksStrategy<NumberFor<B>> + Send + Sync + 'static,
-	Error: std::error::Error + Send + From<sp_consensus::Error> + 'static,
+	Error: std::error::Error + Send + From<ConsensusError> + 'static,
 {
 	type BlockImport = I;
 	type SyncOracle = SO;
 	type JustificationSyncLink = L;
 	type CreateProposer =
-		Pin<Box<dyn Future<Output = Result<E::Proposer, sp_consensus::Error>> + Send + 'static>>;
+		Pin<Box<dyn Future<Output = Result<E::Proposer, ConsensusError>> + Send + 'static>>;
 	type Proposer = E::Proposer;
 	type Claim = P::Public;
 	type AuxData = Vec<AuthorityId<P>>;
@@ -393,11 +393,7 @@ where
 		&mut self.block_import
 	}
 
-	fn aux_data(
-		&self,
-		header: &B::Header,
-		_slot: Slot,
-	) -> Result<Self::AuxData, sp_consensus::Error> {
+	fn aux_data(&self, header: &B::Header, _slot: Slot) -> Result<Self::AuxData, ConsensusError> {
 		authorities(
 			self.client.as_ref(),
 			header.hash(),
@@ -406,17 +402,17 @@ where
 		)
 	}
 
-	fn authorities_len(&self, epoch_data: &Self::AuxData) -> Option<usize> {
-		Some(epoch_data.len())
+	fn authorities_len(&self, authorities: &Self::AuxData) -> Option<usize> {
+		Some(authorities.len())
 	}
 
 	async fn claim_slot(
 		&self,
 		_header: &B::Header,
 		slot: Slot,
-		epoch_data: &Self::AuxData,
+		authorities: &Self::AuxData,
 	) -> Option<Self::Claim> {
-		let expected_author = slot_author::<P>(slot, epoch_data);
+		let expected_author = slot_author::<P>(slot, authorities);
 		expected_author.and_then(|p| {
 			if self
 				.keystore
@@ -440,29 +436,30 @@ where
 		body: Vec<B::Extrinsic>,
 		storage_changes: StorageChanges<<Self::BlockImport as BlockImport<B>>::Transaction, B>,
 		public: Self::Claim,
-		_epoch: Self::AuxData,
+		_authorities: Self::AuxData,
 	) -> Result<
 		sc_consensus::BlockImportParams<B, <Self::BlockImport as BlockImport<B>>::Transaction>,
-		sp_consensus::Error,
+		ConsensusError,
 	> {
-		// sign the pre-sealed hash of the block and then
-		// add it to a digest item.
-		let public_type_pair = public.to_public_crypto_pair();
-		let public = public.to_raw_vec();
 		let signature = self
 			.keystore
-			.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()))?
+			.sign_with(
+				<AuthorityId<P> as AppCrypto>::ID,
+				<AuthorityId<P> as AppCrypto>::CRYPTO_ID,
+				public.as_slice(),
+				header_hash.as_ref(),
+			)
+			.map_err(|e| ConsensusError::CannotSign(format!("{}. Key: {:?}", e, public)))?
 			.ok_or_else(|| {
-				sp_consensus::Error::CannotSign(
-					public.clone(),
-					"Could not find key in keystore.".into(),
-				)
+				ConsensusError::CannotSign(format!(
+					"Could not find key in keystore. Key: {:?}",
+					public
+				))
 			})?;
 		let signature = signature
 			.clone()
 			.try_into()
-			.map_err(|_| sp_consensus::Error::InvalidSignature(signature, public))?;
+			.map_err(|_| ConsensusError::InvalidSignature(signature, public.to_raw_vec()))?;
 
 		let signature_digest_item =
 			<DigestItem as CompatibleDigestItem<P::Signature>>::aura_seal(signature);
@@ -507,7 +504,7 @@ where
 	fn proposer(&mut self, block: &B::Header) -> Self::CreateProposer {
 		self.env
 			.init(block)
-			.map_err(|e| sp_consensus::Error::ClientImport(format!("{:?}", e)))
+			.map_err(|e| ConsensusError::ClientImport(format!("{:?}", e)))
 			.boxed()
 	}
 
@@ -620,14 +617,14 @@ where
 							Default::default(),
 						),
 					)
-					.map_err(|_| sp_consensus::Error::InvalidAuthoritiesSet)?;
+					.map_err(|_| ConsensusError::InvalidAuthoritiesSet)?;
 			},
 	}
 
 	runtime_api
 		.authorities(parent_hash)
 		.ok()
-		.ok_or(sp_consensus::Error::InvalidAuthoritiesSet)
+		.ok_or(ConsensusError::InvalidAuthoritiesSet)
 }
 
 #[cfg(test)]

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

@@ -30,7 +30,7 @@ use sc_consensus_epochs::{descendent_query, Epoch as EpochT, SharedEpochChanges}
 use sc_rpc_api::DenyUnsafe;
 use serde::{Deserialize, Serialize};
 use sp_api::ProvideRuntimeApi;
-use sp_application_crypto::AppKey;
+use sp_application_crypto::AppCrypto;
 use sp_blockchain::{Error as BlockChainError, HeaderBackend, HeaderMetadata};
 use sp_consensus::{Error as ConsensusError, SelectChain};
 use sp_consensus_babe::{

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

@@ -22,7 +22,7 @@ use super::Epoch;
 use codec::Encode;
 use sc_consensus_epochs::Epoch as EpochT;
 use schnorrkel::{keys::PublicKey, vrf::VRFInOut};
-use sp_application_crypto::AppKey;
+use sp_application_crypto::AppCrypto;
 use sp_consensus_babe::{
 	digests::{PreDigest, PrimaryPreDigest, SecondaryPlainPreDigest, SecondaryVRFPreDigest},
 	make_transcript, make_transcript_data, AuthorityId, BabeAuthorityWeight, Slot, BABE_VRF_PREFIX,

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

@@ -108,7 +108,7 @@ use sc_consensus_slots::{
 };
 use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_DEBUG, CONSENSUS_TRACE};
 use sp_api::{ApiExt, ProvideRuntimeApi};
-use sp_application_crypto::AppKey;
+use sp_application_crypto::AppCrypto;
 use sp_block_builder::BlockBuilder as BlockBuilderApi;
 use sp_blockchain::{
 	Backend as _, BlockStatus, Error as ClientError, ForkBackend, HeaderBackend, HeaderMetadata,
@@ -117,7 +117,7 @@ use sp_blockchain::{
 use sp_consensus::{BlockOrigin, Environment, Error as ConsensusError, Proposer, SelectChain};
 use sp_consensus_babe::inherents::BabeInherentData;
 use sp_consensus_slots::Slot;
-use sp_core::{crypto::ByteArray, ExecutionContext};
+use sp_core::ExecutionContext;
 use sp_inherents::{CreateInherentDataProviders, InherentData, InherentDataProvider};
 use sp_keystore::KeystorePtr;
 use sp_runtime::{
@@ -274,7 +274,7 @@ pub enum Error<B: BlockT> {
 	MultipleConfigChangeDigests,
 	/// Could not extract timestamp and slot
 	#[error("Could not extract timestamp and slot: {0}")]
-	Extraction(sp_consensus::Error),
+	Extraction(ConsensusError),
 	/// Could not fetch epoch
 	#[error("Could not fetch epoch at {0:?}")]
 	FetchEpoch(B::Hash),
@@ -471,7 +471,7 @@ pub fn start_babe<B, C, SC, E, I, SO, CIDP, BS, L, Error>(
 		max_block_proposal_slot_portion,
 		telemetry,
 	}: BabeParams<B, C, SC, E, I, SO, L, CIDP, BS>,
-) -> Result<BabeWorker<B>, sp_consensus::Error>
+) -> Result<BabeWorker<B>, ConsensusError>
 where
 	B: BlockT,
 	C: ProvideRuntimeApi<B>
@@ -739,7 +739,7 @@ where
 	type SyncOracle = SO;
 	type JustificationSyncLink = L;
 	type CreateProposer =
-		Pin<Box<dyn Future<Output = Result<E::Proposer, sp_consensus::Error>> + Send + 'static>>;
+		Pin<Box<dyn Future<Output = Result<E::Proposer, ConsensusError>> + Send + 'static>>;
 	type Proposer = E::Proposer;
 	type BlockImport = I;
 	type AuxData = ViableEpochDescriptor<B::Hash, NumberFor<B>, Epoch>;
@@ -762,7 +762,7 @@ where
 				slot,
 			)
 			.map_err(|e| ConsensusError::ChainLookup(e.to_string()))?
-			.ok_or(sp_consensus::Error::InvalidAuthoritiesSet)
+			.ok_or(ConsensusError::InvalidAuthoritiesSet)
 	}
 
 	fn authorities_len(&self, epoch_descriptor: &Self::AuxData) -> Option<usize> {
@@ -827,28 +827,21 @@ where
 		(_, public): Self::Claim,
 		epoch_descriptor: Self::AuxData,
 	) -> Result<
-		sc_consensus::BlockImportParams<B, <Self::BlockImport as BlockImport<B>>::Transaction>,
-		sp_consensus::Error,
+		BlockImportParams<B, <Self::BlockImport as BlockImport<B>>::Transaction>,
+		ConsensusError,
 	> {
-		// sign the pre-sealed hash of the block and then
-		// add it to a digest item.
-		let public_type_pair = public.clone().into();
-		let public = public.to_raw_vec();
 		let signature = self
 			.keystore
-			.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()))?
+			.sr25519_sign(<AuthorityId as AppCrypto>::ID, public.as_ref(), header_hash.as_ref())
+			.map_err(|e| ConsensusError::CannotSign(format!("{}. Key: {:?}", e, public)))?
 			.ok_or_else(|| {
-				sp_consensus::Error::CannotSign(
-					public.clone(),
-					"Could not find key in keystore.".into(),
-				)
+				ConsensusError::CannotSign(format!(
+					"Could not find key in keystore. Key: {:?}",
+					public
+				))
 			})?;
-		let signature: AuthoritySignature = signature
-			.clone()
-			.try_into()
-			.map_err(|_| sp_consensus::Error::InvalidSignature(signature, public))?;
-		let digest_item = <DigestItem as CompatibleDigestItem>::babe_seal(signature);
+
+		let digest_item = <DigestItem as CompatibleDigestItem>::babe_seal(signature.into());
 
 		let mut import_block = BlockImportParams::new(BlockOrigin::Own, header);
 		import_block.post_digests.push(digest_item);
@@ -891,11 +884,7 @@ where
 	}
 
 	fn proposer(&mut self, block: &B::Header) -> Self::CreateProposer {
-		Box::pin(
-			self.env
-				.init(block)
-				.map_err(|e| sp_consensus::Error::ClientImport(format!("{:?}", e))),
-		)
+		Box::pin(self.env.init(block).map_err(|e| ConsensusError::ClientImport(e.to_string())))
 	}
 
 	fn telemetry(&self) -> Option<TelemetryHandle> {
@@ -1182,7 +1171,7 @@ where
 			.create_inherent_data_providers
 			.create_inherent_data_providers(parent_hash, ())
 			.await
-			.map_err(|e| Error::<Block>::Client(sp_consensus::Error::from(e).into()))?;
+			.map_err(|e| Error::<Block>::Client(ConsensusError::from(e).into()))?;
 
 		let slot_now = create_inherent_data_providers.slot();
 

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

@@ -72,7 +72,7 @@ use sc_network::types::ProtocolName;
 use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_DEBUG, CONSENSUS_INFO};
 use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver};
 use sp_api::ProvideRuntimeApi;
-use sp_application_crypto::AppKey;
+use sp_application_crypto::AppCrypto;
 use sp_blockchain::{Error as ClientError, HeaderBackend, HeaderMetadata, Result as ClientResult};
 use sp_consensus::SelectChain;
 use sp_consensus_grandpa::{

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

@@ -150,7 +150,7 @@ pub trait SimpleSlotWorker<B: BlockT> {
 		body: Vec<B::Extrinsic>,
 		storage_changes: StorageChanges<<Self::BlockImport as BlockImport<B>>::Transaction, B>,
 		public: Self::Claim,
-		epoch: Self::AuxData,
+		aux_data: Self::AuxData,
 	) -> Result<
 		sc_consensus::BlockImportParams<B, <Self::BlockImport as BlockImport<B>>::Transaction>,
 		sp_consensus::Error,