Skip to content

Commit

Permalink
Keystore overhaul (iter 2) (paritytech#13634)
Browse files Browse the repository at this point in the history
* Remove bloat about remote keystore

* Update docs and remove unused 'KeystoreRef' trait

* Use wherever possible, MemoryKeystore for testing

* Remove unrequired fully qualified method syntax for Keystore
  • Loading branch information
davxy authored and nathanwhit committed Jul 19, 2023
1 parent d25f4fe commit 5d98554
Show file tree
Hide file tree
Showing 44 changed files with 312 additions and 457 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 1 addition & 23 deletions bin/node-template/node/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use sc_client_api::BlockBackend;
use sc_consensus_aura::{ImportQueueParams, SlotProportion, StartAuraParams};
use sc_consensus_grandpa::SharedVoterState;
pub use sc_executor::NativeElseWasmExecutor;
use sc_keystore::LocalKeystore;
use sc_service::{error::Error as ServiceError, Configuration, TaskManager, WarpSyncParams};
use sc_telemetry::{Telemetry, TelemetryWorker};
use sp_consensus_aura::sr25519::AuthorityPair as AuraPair;
Expand Down Expand Up @@ -58,10 +57,6 @@ pub fn new_partial(
>,
ServiceError,
> {
if config.keystore_remote.is_some() {
return Err(ServiceError::Other("Remote Keystores are not supported.".into()))
}

let telemetry = config
.telemetry_endpoints
.clone()
Expand Down Expand Up @@ -147,36 +142,19 @@ pub fn new_partial(
})
}

fn remote_keystore(_url: &String) -> Result<Arc<LocalKeystore>, &'static str> {
// FIXME: here would the concrete keystore be built,
// must return a concrete type (NOT `LocalKeystore`) that
// implements `Keystore`
Err("Remote Keystore not supported.")
}

/// Builds a new service for a full client.
pub fn new_full(mut config: Configuration) -> Result<TaskManager, ServiceError> {
let sc_service::PartialComponents {
client,
backend,
mut task_manager,
import_queue,
mut keystore_container,
keystore_container,
select_chain,
transaction_pool,
other: (block_import, grandpa_link, mut telemetry),
} = new_partial(&config)?;

if let Some(url) = &config.keystore_remote {
match remote_keystore(url) {
Ok(k) => keystore_container.set_remote_keystore(k),
Err(e) =>
return Err(ServiceError::Other(format!(
"Error hooking up remote keystore for {}: {}",
url, e
))),
};
}
let grandpa_protocol_name = sc_consensus_grandpa::protocol_standard_name(
&client.block_hash(0).ok().flatten().expect("Genesis block exists; qed"),
&config.chain_spec,
Expand Down
1 change: 0 additions & 1 deletion bin/node/cli/benches/block_production.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
transaction_pool: Default::default(),
network: network_config,
keystore: KeystoreConfig::InMemory,
keystore_remote: Default::default(),
database: DatabaseSource::RocksDb { path: root.join("db"), cache_size: 128 },
trie_cache_maximum_size: Some(64 * 1024 * 1024),
state_pruning: Some(PruningMode::ArchiveAll),
Expand Down
1 change: 0 additions & 1 deletion bin/node/cli/benches/transaction_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ fn new_node(tokio_handle: Handle) -> node_cli::service::NewFullBase {
},
network: network_config,
keystore: KeystoreConfig::InMemory,
keystore_remote: Default::default(),
database: DatabaseSource::RocksDb { path: root.join("db"), cache_size: 128 },
trie_cache_maximum_size: Some(64 * 1024 * 1024),
state_pruning: Some(PruningMode::ArchiveAll),
Expand Down
35 changes: 18 additions & 17 deletions bin/node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ mod tests {
use sp_core::{crypto::Pair as CryptoPair, Public};
use sp_inherents::InherentDataProvider;
use sp_keyring::AccountKeyring;
use sp_keystore::{Keystore, KeystorePtr};
use sp_keystore::KeystorePtr;
use sp_runtime::{
generic::{Digest, Era, SignedPayload},
key_types::BABE,
Expand All @@ -615,12 +615,13 @@ mod tests {
sp_tracing::try_init_simple();

let keystore_path = tempfile::tempdir().expect("Creates keystore path");
let keystore: KeystorePtr =
Arc::new(LocalKeystore::open(keystore_path.path(), None).expect("Creates keystore"));
let alice: sp_consensus_babe::AuthorityId =
Keystore::sr25519_generate_new(&*keystore, BABE, Some("//Alice"))
.expect("Creates authority pair")
.into();
let keystore: KeystorePtr = LocalKeystore::open(keystore_path.path(), None)
.expect("Creates keystore")
.into();
let alice: sp_consensus_babe::AuthorityId = keystore
.sr25519_generate_new(BABE, Some("//Alice"))
.expect("Creates authority pair")
.into();

let chain_spec = crate::chain_spec::tests::integration_test_config_with_single_authority();

Expand Down Expand Up @@ -735,16 +736,16 @@ mod tests {
// sign the pre-sealed hash of the block and then
// add it to a digest item.
let to_sign = pre_hash.encode();
let signature = Keystore::sign_with(
&*keystore,
sp_consensus_babe::AuthorityId::ID,
&alice.to_public_crypto_pair(),
&to_sign,
)
.unwrap()
.unwrap()
.try_into()
.unwrap();
let signature = keystore
.sign_with(
sp_consensus_babe::AuthorityId::ID,
&alice.to_public_crypto_pair(),
&to_sign,
)
.unwrap()
.unwrap()
.try_into()
.unwrap();
let item = <DigestItem as CompatibleDigestItem>::babe_seal(signature);
slot += 1;

Expand Down
63 changes: 22 additions & 41 deletions bin/node/executor/tests/submit_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ use sp_application_crypto::AppKey;
use sp_core::offchain::{testing::TestTransactionPoolExt, TransactionPoolExt};
use sp_keyring::sr25519::Keyring::Alice;
use sp_keystore::{testing::MemoryKeystore, Keystore, KeystoreExt};
use std::sync::Arc;

pub mod common;
use self::common::*;
Expand Down Expand Up @@ -63,25 +62,16 @@ fn should_submit_signed_transaction() {
t.register_extension(TransactionPoolExt::new(pool));

let keystore = MemoryKeystore::new();
Keystore::sr25519_generate_new(
&keystore,
sr25519::AuthorityId::ID,
Some(&format!("{}/hunter1", PHRASE)),
)
.unwrap();
Keystore::sr25519_generate_new(
&keystore,
sr25519::AuthorityId::ID,
Some(&format!("{}/hunter2", PHRASE)),
)
.unwrap();
Keystore::sr25519_generate_new(
&keystore,
sr25519::AuthorityId::ID,
Some(&format!("{}/hunter3", PHRASE)),
)
.unwrap();
t.register_extension(KeystoreExt(Arc::new(keystore)));
keystore
.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter1", PHRASE)))
.unwrap();
keystore
.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter2", PHRASE)))
.unwrap();
keystore
.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter3", PHRASE)))
.unwrap();
t.register_extension(KeystoreExt::new(keystore));

t.execute_with(|| {
let results =
Expand All @@ -106,19 +96,13 @@ fn should_submit_signed_twice_from_the_same_account() {
t.register_extension(TransactionPoolExt::new(pool));

let keystore = MemoryKeystore::new();
Keystore::sr25519_generate_new(
&keystore,
sr25519::AuthorityId::ID,
Some(&format!("{}/hunter1", PHRASE)),
)
.unwrap();
Keystore::sr25519_generate_new(
&keystore,
sr25519::AuthorityId::ID,
Some(&format!("{}/hunter2", PHRASE)),
)
.unwrap();
t.register_extension(KeystoreExt(Arc::new(keystore)));
keystore
.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter1", PHRASE)))
.unwrap();
keystore
.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter2", PHRASE)))
.unwrap();
t.register_extension(KeystoreExt::new(keystore));

t.execute_with(|| {
let result =
Expand Down Expand Up @@ -169,7 +153,7 @@ fn should_submit_signed_twice_from_all_accounts() {
keystore
.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter2", PHRASE)))
.unwrap();
t.register_extension(KeystoreExt(Arc::new(keystore)));
t.register_extension(KeystoreExt::new(keystore));

t.execute_with(|| {
let results = Signer::<Runtime, TestAuthorityId>::all_accounts()
Expand Down Expand Up @@ -227,13 +211,10 @@ fn submitted_transaction_should_be_valid() {
t.register_extension(TransactionPoolExt::new(pool));

let keystore = MemoryKeystore::new();
Keystore::sr25519_generate_new(
&keystore,
sr25519::AuthorityId::ID,
Some(&format!("{}/hunter1", PHRASE)),
)
.unwrap();
t.register_extension(KeystoreExt(Arc::new(keystore)));
keystore
.sr25519_generate_new(sr25519::AuthorityId::ID, Some(&format!("{}/hunter1", PHRASE)))
.unwrap();
t.register_extension(KeystoreExt::new(keystore));

t.execute_with(|| {
let results =
Expand Down
12 changes: 6 additions & 6 deletions bin/utils/chain-spec-builder/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
use std::{
fs,
path::{Path, PathBuf},
sync::Arc,
};

use ansi_term::Style;
Expand All @@ -32,7 +31,7 @@ use sp_core::{
crypto::{ByteArray, Ss58Codec},
sr25519,
};
use sp_keystore::{Keystore, KeystorePtr};
use sp_keystore::KeystorePtr;

/// A utility to easily create a testnet chain spec definition with a given set
/// of authorities and endowed accounts and/or generate random accounts.
Expand Down Expand Up @@ -164,16 +163,17 @@ fn generate_chain_spec(

fn generate_authority_keys_and_store(seeds: &[String], keystore_path: &Path) -> Result<(), String> {
for (n, seed) in seeds.iter().enumerate() {
let keystore: KeystorePtr = Arc::new(
let keystore: KeystorePtr =
LocalKeystore::open(keystore_path.join(format!("auth-{}", n)), None)
.map_err(|err| err.to_string())?,
);
.map_err(|err| err.to_string())?
.into();

let (_, _, grandpa, babe, im_online, authority_discovery) =
chain_spec::authority_keys_from_seed(seed);

let insert_key = |key_type, public| {
Keystore::insert(&*keystore, key_type, &format!("//{}", seed), public)
keystore
.insert(key_type, &format!("//{}", seed), public)
.map_err(|_| format!("Failed to insert key: {}", grandpa))
};

Expand Down
4 changes: 2 additions & 2 deletions client/authority-discovery/src/worker/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ fn addresses_to_publish_adds_p2p() {
Arc::new(TestApi { authorities: vec![] }),
network.clone(),
Box::pin(dht_event_rx),
Role::PublishAndDiscover(Arc::new(MemoryKeystore::new())),
Role::PublishAndDiscover(MemoryKeystore::new().into()),
Some(prometheus_endpoint::Registry::new()),
Default::default(),
);
Expand Down Expand Up @@ -731,7 +731,7 @@ fn addresses_to_publish_respects_existing_p2p_protocol() {
Arc::new(TestApi { authorities: vec![] }),
network.clone(),
Box::pin(dht_event_rx),
Role::PublishAndDiscover(Arc::new(MemoryKeystore::new())),
Role::PublishAndDiscover(MemoryKeystore::new().into()),
Some(prometheus_endpoint::Registry::new()),
Default::default(),
);
Expand Down
11 changes: 6 additions & 5 deletions client/cli/src/commands/insert_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ use clap::Parser;
use sc_keystore::LocalKeystore;
use sc_service::config::{BasePath, KeystoreConfig};
use sp_core::crypto::{KeyTypeId, SecretString};
use sp_keystore::{Keystore, KeystorePtr};
use std::sync::Arc;
use sp_keystore::KeystorePtr;

/// The `insert` command
#[derive(Debug, Clone, Parser)]
Expand Down Expand Up @@ -67,9 +66,9 @@ impl InsertKeyCmd {
let config_dir = base_path.config_dir(chain_spec.id());

let (keystore, public) = match self.keystore_params.keystore_config(&config_dir)? {
(_, KeystoreConfig::Path { path, password }) => {
KeystoreConfig::Path { path, password } => {
let public = with_crypto_scheme!(self.scheme, to_vec(&suri, password.clone()))?;
let keystore: KeystorePtr = Arc::new(LocalKeystore::open(path, password)?);
let keystore: KeystorePtr = LocalKeystore::open(path, password)?.into();
(keystore, public)
},
_ => unreachable!("keystore_config always returns path and password; qed"),
Expand All @@ -78,7 +77,8 @@ impl InsertKeyCmd {
let key_type =
KeyTypeId::try_from(self.key_type.as_str()).map_err(|_| Error::KeyTypeInvalid)?;

Keystore::insert(&*keystore, key_type, &suri, &public[..])
keystore
.insert(key_type, &suri, &public[..])
.map_err(|_| Error::KeystoreOperation)?;

Ok(())
Expand All @@ -95,6 +95,7 @@ mod tests {
use super::*;
use sc_service::{ChainSpec, ChainType, GenericChainSpec, NoExtension};
use sp_core::{sr25519::Pair, ByteArray, Pair as _};
use sp_keystore::Keystore;
use tempfile::TempDir;

struct Cli;
Expand Down
7 changes: 3 additions & 4 deletions client/cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,10 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
///
/// By default this is retrieved from `KeystoreParams` if it is available. Otherwise it uses
/// `KeystoreConfig::InMemory`.
fn keystore_config(&self, config_dir: &PathBuf) -> Result<(Option<String>, KeystoreConfig)> {
fn keystore_config(&self, config_dir: &PathBuf) -> Result<KeystoreConfig> {
self.keystore_params()
.map(|x| x.keystore_config(config_dir))
.unwrap_or_else(|| Ok((None, KeystoreConfig::InMemory)))
.unwrap_or_else(|| Ok(KeystoreConfig::InMemory))
}

/// Get the database cache size.
Expand Down Expand Up @@ -505,7 +505,7 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
let role = self.role(is_dev)?;
let max_runtime_instances = self.max_runtime_instances()?.unwrap_or(8);
let is_validator = role.is_authority();
let (keystore_remote, keystore) = self.keystore_config(&config_dir)?;
let keystore = self.keystore_config(&config_dir)?;
let telemetry_endpoints = self.telemetry_endpoints(&chain_spec)?;
let runtime_cache_size = self.runtime_cache_size()?;

Expand All @@ -524,7 +524,6 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: Sized {
node_key,
DCV::p2p_listen_port(),
)?,
keystore_remote,
keystore,
database: self.database_config(&config_dir, database_cache_size, database)?,
trie_cache_maximum_size: self.trie_cache_maximum_size()?,
Expand Down
6 changes: 2 additions & 4 deletions client/cli/src/params/keystore_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,7 @@ pub fn secret_string_from_str(s: &str) -> std::result::Result<SecretString, Stri

impl KeystoreParams {
/// Get the keystore configuration for the parameters
///
/// Returns a vector of remote-urls and the local Keystore configuration
pub fn keystore_config(&self, config_dir: &Path) -> Result<(Option<String>, KeystoreConfig)> {
pub fn keystore_config(&self, config_dir: &Path) -> Result<KeystoreConfig> {
let password = if self.password_interactive {
Some(SecretString::new(input_keystore_password()?))
} else if let Some(ref file) = self.password_filename {
Expand All @@ -85,7 +83,7 @@ impl KeystoreParams {
.clone()
.unwrap_or_else(|| config_dir.join(DEFAULT_KEYSTORE_CONFIG_PATH));

Ok((self.keystore_uri.clone(), KeystoreConfig::Path { path, password }))
Ok(KeystoreConfig::Path { path, password })
}

/// helper method to fetch password from `KeyParams` or read from stdin
Expand Down
1 change: 0 additions & 1 deletion client/cli/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,6 @@ mod tests {
transaction_pool: Default::default(),
network: NetworkConfiguration::new_memory(),
keystore: sc_service::config::KeystoreConfig::InMemory,
keystore_remote: None,
database: sc_client_db::DatabaseSource::ParityDb { path: PathBuf::from("db") },
trie_cache_maximum_size: None,
state_pruning: None,
Expand Down
Loading

0 comments on commit 5d98554

Please sign in to comment.