Update to work with async keystore – Companion PR for #7000 (#1740)

* Fix keystore types

* Use SyncCryptoStorePtr

* Borrow keystore

* Fix unused imports

* Fix polkadot service

* Fix bitfield-distribution tests

* Fix indentation

* Fix backing tests

* Fix tests

* Fix provisioner tests

* Removed SyncCryptoStorePtr

* Fix services

* Address PR feedback

* Address PR feedback - 2

* Update CryptoStorePtr imports to be from sp_keystore

* Typo

* Fix CryptoStore import

* Document the reason behind using filesystem keystore

* Remove VALIDATORS

* Fix duplicate dependency

* Mark sp-keystore as optional

* Fix availability distribution

* Fix call to sign_with

* Fix keystore usage

* Remove tokio and fix parachains Cargo config

* Typos

* Fix keystore dereferencing

* Fix CryptoStore import

* Fix provisioner

* Fix node backing

* Update services

* Cleanup dependencies

* Use sync_keystore

* Fix node service

* Fix node service - 2

* Fix node service - 3

* Rename CryptoStorePtr to SyncCryptoStorePtr

* "Update Substrate"

* Apply suggestions from code review

* Update node/core/backing/Cargo.toml

* Update primitives/src/v0.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Fix wasm build

* Update Cargo.lock

Co-authored-by: parity-processbot <>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

polkadot/node/core/backing/Cargo.toml

@@ -7,9 +7,9 @@ edition = "2018"
 [dependencies]
 futures = "0.3.5"
 sp-api = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
 sc-client-api = { git = "https://github.com/paritytech/substrate", branch = "master" }
 sp-blockchain = { git = "https://github.com/paritytech/substrate", branch = "master" }
-keystore = { package = "sc-keystore", git = "https://github.com/paritytech/substrate", branch = "master" }
 polkadot-primitives = { path = "../../../primitives" }
 polkadot-node-primitives = { path = "../../primitives" }
 polkadot-subsystem = { package = "polkadot-node-subsystem", path = "../../subsystem" }
@@ -22,7 +22,9 @@ log = "0.4.8"
 
 [dev-dependencies]
 sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-application-crypto = { git = "https://github.com/paritytech/substrate", branch = "master" }
 sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sc-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
 futures = { version = "0.3.5", features = ["thread-pool"] }
 assert_matches = "1.3.0"
 polkadot-node-subsystem-test-helpers = { path = "../../subsystem-test-helpers" }

polkadot/node/core/backing/src/lib.rs

@@ -27,7 +27,7 @@ use futures::{
 	Future, FutureExt, SinkExt, StreamExt,
 };
 
-use keystore::KeyStorePtr;
+use sp_keystore::SyncCryptoStorePtr;
 use polkadot_primitives::v1::{
 	CommittedCandidateReceipt, BackedCandidate, Id as ParaId, ValidatorId,
 	ValidatorIndex, SigningContext, PoV,
@@ -101,6 +101,7 @@ struct CandidateBackingJob {
 	seconded: Option<Hash>,
 	/// We have already reported misbehaviors for these validators.
 	reported_misbehavior_for: HashSet<ValidatorIndex>,
+	keystore: SyncCryptoStorePtr,
 	table: Table<TableContext>,
 	table_context: TableContext,
 	metrics: Metrics,
@@ -328,9 +329,12 @@ impl CandidateBackingJob {
 		};
 
 		let issued_statement = statement.is_some();
-		if let Some(signed_statement) = statement.and_then(|s| self.sign_statement(s)) {
-			self.import_statement(&signed_statement).await?;
-			self.distribute_signed_statement(signed_statement).await?;
+
+		if let Some(statement) = statement {
+			if let Some(signed_statement) = self.sign_statement(statement).await {
+				self.import_statement(&signed_statement).await?;
+				self.distribute_signed_statement(signed_statement).await?;
+			}
 		}
 
 		Ok(issued_statement)
@@ -530,7 +534,7 @@ impl CandidateBackingJob {
 
 		self.issued_statements.insert(candidate_hash);
 
-		if let Some(signed_statement) = self.sign_statement(statement) {
+		if let Some(signed_statement) = self.sign_statement(statement).await {
 			self.distribute_signed_statement(signed_statement).await?;
 		}
 
@@ -553,8 +557,13 @@ impl CandidateBackingJob {
 		Ok(())
 	}
 
-	fn sign_statement(&self, statement: Statement) -> Option<SignedFullStatement> {
-		let signed = self.table_context.validator.as_ref()?.sign(statement);
+	async fn sign_statement(&self, statement: Statement) -> Option<SignedFullStatement> {
+		let signed = self.table_context
+			.validator
+			.as_ref()?
+			.sign(self.keystore.clone(), statement)
+			.await
+			.ok()?;
 		self.metrics.on_statement_signed();
 		Some(signed)
 	}
@@ -694,14 +703,14 @@ impl util::JobTrait for CandidateBackingJob {
 	type ToJob = ToJob;
 	type FromJob = FromJob;
 	type Error = Error;
-	type RunArgs = KeyStorePtr;
+	type RunArgs = SyncCryptoStorePtr;
 	type Metrics = Metrics;
 
 	const NAME: &'static str = "CandidateBackingJob";
 
 	fn run(
 		parent: Hash,
-		keystore: KeyStorePtr,
+		keystore: SyncCryptoStorePtr,
 		metrics: Metrics,
 		rx_to: mpsc::Receiver<Self::ToJob>,
 		mut tx_from: mpsc::Sender<Self::FromJob>,
@@ -748,7 +757,7 @@ impl util::JobTrait for CandidateBackingJob {
 				&validators,
 				signing_context,
 				keystore.clone(),
-			) {
+			).await {
 				Ok(v) => v,
 				Err(util::Error::NotAValidator) => { return Ok(()) },
 				Err(e) => {
@@ -802,6 +811,7 @@ impl util::JobTrait for CandidateBackingJob {
 				issued_statements: HashSet::new(),
 				seconded: None,
 				reported_misbehavior_for: HashSet::new(),
+				keystore,
 				table: Table::default(),
 				table_context,
 				metrics,
@@ -859,17 +869,17 @@ impl metrics::Metrics for Metrics {
 	}
 }
 
-delegated_subsystem!(CandidateBackingJob(KeyStorePtr, Metrics) <- ToJob as CandidateBackingSubsystem);
+delegated_subsystem!(CandidateBackingJob(SyncCryptoStorePtr, Metrics) <- ToJob as CandidateBackingSubsystem);
 
 #[cfg(test)]
 mod tests {
 	use super::*;
 	use assert_matches::assert_matches;
-	use futures::{executor, future, Future};
+	use futures::{future, Future};
 	use polkadot_primitives::v1::{
 		ScheduledCore, BlockData, CandidateCommitments,
 		PersistedValidationData, ValidationData, TransientValidationData, HeadData,
-		ValidatorPair, ValidityAttestation, GroupRotationInfo,
+		ValidityAttestation, GroupRotationInfo,
 	};
 	use polkadot_subsystem::{
 		messages::RuntimeApiRequest,
@@ -877,6 +887,8 @@ mod tests {
 	};
 	use polkadot_node_primitives::InvalidCandidate;
 	use sp_keyring::Sr25519Keyring;
+	use sp_application_crypto::AppKey;
+	use sp_keystore::{CryptoStore, SyncCryptoStore};
 	use std::collections::HashMap;
 
 	fn validator_pubkeys(val_ids: &[Sr25519Keyring]) -> Vec<ValidatorId> {
@@ -885,7 +897,7 @@ mod tests {
 
 	struct TestState {
 		chain_ids: Vec<ParaId>,
-		keystore: KeyStorePtr,
+		keystore: SyncCryptoStorePtr,
 		validators: Vec<Sr25519Keyring>,
 		validator_public: Vec<ValidatorId>,
 		validation_data: ValidationData,
@@ -912,9 +924,9 @@ mod tests {
 				Sr25519Keyring::Ferdie,
 			];
 
-			let keystore = keystore::Store::new_in_memory();
+			let keystore = Arc::new(sc_keystore::LocalKeystore::in_memory());
 			// Make sure `Alice` key is in the keystore, so this mocked node will be a parachain validator.
-			keystore.write().insert_ephemeral_from_seed::<ValidatorPair>(&validators[0].to_seed())
+			SyncCryptoStore::sr25519_generate_new(&*keystore, ValidatorId::ID, Some(&validators[0].to_seed()))
 				.expect("Insert key into keystore");
 
 			let validator_public = validator_pubkeys(&validators);
@@ -985,7 +997,7 @@ mod tests {
 		virtual_overseer: polkadot_node_subsystem_test_helpers::TestSubsystemContextHandle<CandidateBackingMessage>,
 	}
 
-	fn test_harness<T: Future<Output=()>>(keystore: KeyStorePtr, test: impl FnOnce(TestHarness) -> T) {
+	fn test_harness<T: Future<Output=()>>(keystore: SyncCryptoStorePtr, test: impl FnOnce(TestHarness) -> T) {
 		let pool = sp_core::testing::TaskExecutor::new();
 
 		let (context, virtual_overseer) = polkadot_node_subsystem_test_helpers::make_subsystem_context(pool.clone());
@@ -998,8 +1010,7 @@ mod tests {
 
 		futures::pin_mut!(test_fut);
 		futures::pin_mut!(subsystem);
-
-		executor::block_on(future::select(test_fut, subsystem));
+		futures::executor::block_on(future::select(test_fut, subsystem));
 	}
 
 	fn make_erasure_root(test: &TestState, pov: PoV) -> Hash {
@@ -1203,20 +1214,29 @@ mod tests {
 			}.build();
 
 			let candidate_a_hash = candidate_a.hash();
-
+			let public0 = CryptoStore::sr25519_generate_new(
+				&*test_state.keystore,
+				ValidatorId::ID, Some(&test_state.validators[0].to_seed())
+			).await.expect("Insert key into keystore");
+			let public2 = CryptoStore::sr25519_generate_new(
+				&*test_state.keystore,
+				ValidatorId::ID, Some(&test_state.validators[2].to_seed())
+			).await.expect("Insert key into keystore");
 			let signed_a = SignedFullStatement::sign(
+				&test_state.keystore,
 				Statement::Seconded(candidate_a.clone()),
 				&test_state.signing_context,
 				2,
-				&test_state.validators[2].pair().into(),
-			);
+				&public2.into(),
+			).await.expect("should be signed");
 
 			let signed_b = SignedFullStatement::sign(
+				&test_state.keystore,
 				Statement::Valid(candidate_a_hash),
 				&test_state.signing_context,
 				0,
-				&test_state.validators[0].pair().into(),
-			);
+				&public0.into(),
+			).await.expect("should be signed");
 
 			let statement = CandidateBackingMessage::Statement(test_state.relay_parent, signed_a.clone());
 
@@ -1330,27 +1350,37 @@ mod tests {
 			}.build();
 
 			let candidate_a_hash = candidate_a.hash();
-
+			let public0 = CryptoStore::sr25519_generate_new(
+				&*test_state.keystore,
+				ValidatorId::ID, Some(&test_state.validators[0].to_seed())
+			).await.expect("Insert key into keystore");
+			let public2 = CryptoStore::sr25519_generate_new(
+				&*test_state.keystore,
+				ValidatorId::ID, Some(&test_state.validators[2].to_seed())
+			).await.expect("Insert key into keystore");
 			let signed_a = SignedFullStatement::sign(
+				&test_state.keystore,
 				Statement::Seconded(candidate_a.clone()),
 				&test_state.signing_context,
 				2,
-				&test_state.validators[2].pair().into(),
-			);
+				&public2.into(),
+			).await.expect("should be signed");
 
 			let signed_b = SignedFullStatement::sign(
+				&test_state.keystore,
 				Statement::Valid(candidate_a_hash),
 				&test_state.signing_context,
 				0,
-				&test_state.validators[0].pair().into(),
-			);
+				&public0.into(),
+			).await.expect("should be signed");
 
 			let signed_c = SignedFullStatement::sign(
+				&test_state.keystore,
 				Statement::Invalid(candidate_a_hash),
 				&test_state.signing_context,
 				0,
-				&test_state.validators[0].pair().into(),
-			);
+				&public0.into(),
+			).await.expect("should be signed");
 
 			let statement = CandidateBackingMessage::Statement(test_state.relay_parent, signed_a.clone());
 
@@ -1600,12 +1630,18 @@ mod tests {
 
 			let candidate_hash = candidate.hash();
 
+			let validator2 = CryptoStore::sr25519_generate_new(
+				&*test_state.keystore,
+				ValidatorId::ID, Some(&test_state.validators[2].to_seed())
+			).await.expect("Insert key into keystore");
+
 			let signed_a = SignedFullStatement::sign(
+				&test_state.keystore,
 				Statement::Seconded(candidate.clone()),
 				&test_state.signing_context,
 				2,
-				&test_state.validators[2].pair().into(),
-			);
+				&validator2.into(),
+			).await.expect("should be signed");
 
 			// Send in a `Statement` with a candidate.
 			let statement = CandidateBackingMessage::Statement(
@@ -1733,12 +1769,17 @@ mod tests {
 				..Default::default()
 			}.build();
 
+			let public2 = CryptoStore::sr25519_generate_new(
+				&*test_state.keystore,
+				ValidatorId::ID, Some(&test_state.validators[2].to_seed())
+			).await.expect("Insert key into keystore");
 			let signed_a = SignedFullStatement::sign(
+				&test_state.keystore,
 				Statement::Seconded(candidate.clone()),
 				&test_state.signing_context,
 				2,
-				&test_state.validators[2].pair().into(),
-			);
+				&public2.into(),
+			).await.expect("should be signed");
 
 			// Send in a `Statement` with a candidate.
 			let statement = CandidateBackingMessage::Statement(
@@ -1869,12 +1910,17 @@ mod tests {
 				..Default::default()
 			}.build();
 
+			let public2 = CryptoStore::sr25519_generate_new(
+				&*test_state.keystore,
+				ValidatorId::ID, Some(&test_state.validators[2].to_seed())
+			).await.expect("Insert key into keystore");
 			let seconding = SignedFullStatement::sign(
+				&test_state.keystore,
 				Statement::Seconded(candidate_a.clone()),
 				&test_state.signing_context,
 				2,
-				&test_state.validators[2].pair().into(),
-			);
+				&public2.into(),
+			).await.expect("should be signed");
 
 			let statement = CandidateBackingMessage::Statement(
 				test_state.relay_parent,

polkadot/node/core/bitfield-signing/Cargo.toml

@@ -12,5 +12,5 @@ log = "0.4.8"
 polkadot-primitives = { path = "../../../primitives" }
 polkadot-node-subsystem = { path = "../../subsystem" }
 polkadot-node-subsystem-util = { path = "../../subsystem-util" }
-keystore = { package = "sc-keystore", git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
 wasm-timer = "0.2.4"

polkadot/node/core/bitfield-signing/src/lib.rs

@@ -22,7 +22,7 @@ use futures::{
 	prelude::*,
 	stream, Future,
 };
-use keystore::KeyStorePtr;
+use sp_keystore::{Error as KeystoreError, SyncCryptoStorePtr};
 use polkadot_node_subsystem::{
 	messages::{
 		self, AllMessages, AvailabilityStoreMessage, BitfieldDistributionMessage,
@@ -132,6 +132,9 @@ pub enum Error {
 	/// the runtime API failed to return what we wanted
 	#[from]
 	Runtime(RuntimeApiError),
+	/// the keystore failed to process signing request
+	#[from]
+	Keystore(KeystoreError),
 }
 
 // if there is a candidate pending availability, query the Availability Store
@@ -289,7 +292,7 @@ impl JobTrait for BitfieldSigningJob {
 	type ToJob = ToJob;
 	type FromJob = FromJob;
 	type Error = Error;
-	type RunArgs = KeyStorePtr;
+	type RunArgs = SyncCryptoStorePtr;
 	type Metrics = Metrics;
 
 	const NAME: &'static str = "BitfieldSigningJob";
@@ -308,7 +311,7 @@ impl JobTrait for BitfieldSigningJob {
 
 			// now do all the work we can before we need to wait for the availability store
 			// if we're not a validator, we can just succeed effortlessly
-			let validator = match Validator::new(relay_parent, keystore, sender.clone()).await {
+			let validator = match Validator::new(relay_parent, keystore.clone(), sender.clone()).await {
 				Ok(validator) => validator,
 				Err(util::Error::NotAValidator) => return Ok(()),
 				Err(err) => return Err(Error::Util(err)),
@@ -329,7 +332,10 @@ impl JobTrait for BitfieldSigningJob {
 				Ok(bitfield) => bitfield,
 			};
 
-			let signed_bitfield = validator.sign(bitfield);
+			let signed_bitfield = validator
+				.sign(keystore.clone(), bitfield)
+				.await
+				.map_err(|e| Error::Keystore(e))?;
 			metrics.on_bitfield_signed();
 
 			// make an anonymous scope to contain some use statements to simplify creating the outbound message

polkadot/node/core/provisioner/Cargo.toml

@@ -16,4 +16,8 @@ polkadot-node-subsystem-util = { path = "../../subsystem-util" }
 [dev-dependencies]
 lazy_static = "1.4"
 sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-application-crypto = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sp-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
+sc-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" }
 futures-timer = "3.0.2"
+tempfile = "3.1.0"

polkadot/node/core/provisioner/src/lib.rs

@@ -549,36 +549,37 @@ mod tests {
 	mod select_availability_bitfields {
 		use super::super::*;
 		use super::{default_bitvec, occupied_core};
-		use lazy_static::lazy_static;
-		use polkadot_primitives::v1::{SigningContext, ValidatorIndex, ValidatorPair};
-		use sp_core::crypto::Pair;
-		use std::sync::Mutex;
+		use futures::executor::block_on;
+		use std::sync::Arc;
+		use polkadot_primitives::v1::{SigningContext, ValidatorIndex, ValidatorId};
+		use sp_application_crypto::AppKey;
+		use sp_keystore::{CryptoStore, SyncCryptoStorePtr};
+		use sc_keystore::LocalKeystore;
 
-		lazy_static! {
-			// we can use a normal mutex here, not a futures-aware one, because we don't use any futures-based
-			// concurrency when accessing this. The risk of contention is that multiple tests are run in parallel,
-			// in independent threads, in which case a standard mutex suffices.
-			static ref VALIDATORS: Mutex<HashMap<ValidatorIndex, ValidatorPair>> = Mutex::new(HashMap::new());
-		}
-
-		fn signed_bitfield(
+		async fn signed_bitfield(
+			keystore: &SyncCryptoStorePtr,
 			field: CoreAvailability,
 			validator_idx: ValidatorIndex,
 		) -> SignedAvailabilityBitfield {
-			let mut lock = VALIDATORS.lock().unwrap();
-			let validator = lock
-				.entry(validator_idx)
-				.or_insert_with(|| ValidatorPair::generate().0);
+			let public = CryptoStore::sr25519_generate_new(&**keystore, ValidatorId::ID, None)
+				.await
+				.expect("generated sr25519 key");
 			SignedAvailabilityBitfield::sign(
+				&keystore,
 				field.into(),
 				&<SigningContext<Hash>>::default(),
 				validator_idx,
-				validator,
-			)
+				&public.into(),
+			).await.expect("Should be signed")
 		}
 
 		#[test]
 		fn not_more_than_one_per_validator() {
+			// Configure filesystem-based keystore as generating keys without seed
+			// would trigger the key to be generated on the filesystem.
+			let keystore_path = tempfile::tempdir().expect("Creates keystore path");
+			let keystore : SyncCryptoStorePtr = Arc::new(LocalKeystore::open(keystore_path.path(), None)
+				.expect("Creates keystore"));
 			let bitvec = default_bitvec();
 
 			let cores = vec![occupied_core(0), occupied_core(1)];
@@ -586,9 +587,9 @@ mod tests {
 			// we pass in three bitfields with two validators
 			// this helps us check the postcondition that we get two bitfields back, for which the validators differ
 			let bitfields = vec![
-				signed_bitfield(bitvec.clone(), 0),
-				signed_bitfield(bitvec.clone(), 1),
-				signed_bitfield(bitvec, 1),
+				block_on(signed_bitfield(&keystore, bitvec.clone(), 0)),
+				block_on(signed_bitfield(&keystore, bitvec.clone(), 1)),
+				block_on(signed_bitfield(&keystore, bitvec, 1)),
 			];
 
 			let mut selected_bitfields = select_availability_bitfields(&cores, &bitfields);
@@ -602,14 +603,19 @@ mod tests {
 
 		#[test]
 		fn each_corresponds_to_an_occupied_core() {
+			// Configure filesystem-based keystore as generating keys without seed
+			// would trigger the key to be generated on the filesystem.
+			let keystore_path = tempfile::tempdir().expect("Creates keystore path");
+			let keystore : SyncCryptoStorePtr = Arc::new(LocalKeystore::open(keystore_path.path(), None)
+				.expect("Creates keystore"));
 			let bitvec = default_bitvec();
 
 			let cores = vec![CoreState::Free, CoreState::Scheduled(Default::default())];
 
 			let bitfields = vec![
-				signed_bitfield(bitvec.clone(), 0),
-				signed_bitfield(bitvec.clone(), 1),
-				signed_bitfield(bitvec, 1),
+				block_on(signed_bitfield(&keystore, bitvec.clone(), 0)),
+				block_on(signed_bitfield(&keystore, bitvec.clone(), 1)),
+				block_on(signed_bitfield(&keystore, bitvec, 1)),
 			];
 
 			let mut selected_bitfields = select_availability_bitfields(&cores, &bitfields);
@@ -621,6 +627,11 @@ mod tests {
 
 		#[test]
 		fn more_set_bits_win_conflicts() {
+			// Configure filesystem-based keystore as generating keys without seed
+			// would trigger the key to be generated on the filesystem.
+			let keystore_path = tempfile::tempdir().expect("Creates keystore path");
+			let keystore : SyncCryptoStorePtr = Arc::new(LocalKeystore::open(keystore_path.path(), None)
+				.expect("Creates keystore"));
 			let bitvec_zero = default_bitvec();
 			let bitvec_one = {
 				let mut bitvec = bitvec_zero.clone();
@@ -631,8 +642,8 @@ mod tests {
 			let cores = vec![occupied_core(0)];
 
 			let bitfields = vec![
-				signed_bitfield(bitvec_zero, 0),
-				signed_bitfield(bitvec_one.clone(), 0),
+				block_on(signed_bitfield(&keystore, bitvec_zero, 0)),
+				block_on(signed_bitfield(&keystore, bitvec_one.clone(), 0)),
 			];
 
 			// this test is probablistic: chances are excellent that it does what it claims to.