From 8ca0e09ffc42975291568f29015f5bcc597f575a Mon Sep 17 00:00:00 2001 From: maciejhirsz Date: Wed, 4 Jan 2017 12:50:50 +0100 Subject: [PATCH 1/4] Better error messages for PoA chains --- ethcore/src/account_provider/mod.rs | 5 +++++ parity/run.rs | 14 +++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/ethcore/src/account_provider/mod.rs b/ethcore/src/account_provider/mod.rs index 5b56c697c..2f810d6be 100644 --- a/ethcore/src/account_provider/mod.rs +++ b/ethcore/src/account_provider/mod.rs @@ -150,6 +150,11 @@ impl AccountProvider { Ok(Address::from(address).into()) } + /// Checks whether an account with a given address is present. + pub fn has_account(&self, address: Address) -> Result { + Ok(self.accounts()?.iter().any(|&a| a == address)) + } + /// Returns addresses of all accounts. pub fn accounts(&self) -> Result, Error> { let accounts = self.sstore.accounts()?; diff --git a/parity/run.rs b/parity/run.rs index 672c80fcf..2b36d6cd1 100644 --- a/parity/run.rs +++ b/parity/run.rs @@ -230,9 +230,21 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc) -> R miner.set_extra_data(cmd.miner_extras.extra_data); miner.set_transactions_limit(cmd.miner_extras.transactions_limit); let engine_signer = cmd.miner_extras.engine_signer; + if engine_signer != Default::default() { + // Check if engine signer exists + if !account_provider.has_account(engine_signer).unwrap_or(false) { + return Err(format!("Consensus signer account not found for the current chain, please run `parity account new -d current-d --chain current-chain`")); + } + + // Check if any passwords have been read from the password file(s) + if passwords.is_empty() { + return Err(format!("No password found for the consensus signer {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", engine_signer)); + } + + // Attempt to sign in the engine signer. if !passwords.into_iter().any(|p| miner.set_engine_signer(engine_signer, p).is_ok()) { - return Err(format!("No password found for the consensus signer {}. Make sure valid password is present in files passed using `--password`.", engine_signer)); + return Err(format!("Invalid password for consensus signer {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", engine_signer)); } } From 4b1b67bfeb370b14c728b8c4bae819ffb985e3ed Mon Sep 17 00:00:00 2001 From: maciejhirsz Date: Wed, 4 Jan 2017 14:05:32 +0100 Subject: [PATCH 2/4] Analog error messages for unlocking accounts --- parity/run.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/parity/run.rs b/parity/run.rs index 2b36d6cd1..ab9abface 100644 --- a/parity/run.rs +++ b/parity/run.rs @@ -234,7 +234,7 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc) -> R if engine_signer != Default::default() { // Check if engine signer exists if !account_provider.has_account(engine_signer).unwrap_or(false) { - return Err(format!("Consensus signer account not found for the current chain, please run `parity account new -d current-d --chain current-chain`")); + return Err("Consensus signer account not found for the current chain, please run `parity account new -d current-d --chain current-chain --keys-path current-keys-path`".to_owned()); } // Check if any passwords have been read from the password file(s) @@ -244,7 +244,7 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc) -> R // Attempt to sign in the engine signer. if !passwords.into_iter().any(|p| miner.set_engine_signer(engine_signer, p).is_ok()) { - return Err(format!("Invalid password for consensus signer {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", engine_signer)); + return Err(format!("No valid password for the consensus signer {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", engine_signer)); } } @@ -490,17 +490,27 @@ fn prepare_account_provider(dirs: &Directories, data_dir: &str, cfg: AccountsCon let path = dirs.keys_path(data_dir); upgrade_key_location(&dirs.legacy_keys_path(cfg.testnet), &path); let dir = Box::new(DiskDirectory::create(&path).map_err(|e| format!("Could not open keys directory: {}", e))?); - let account_service = AccountProvider::new(Box::new( + let account_provider = AccountProvider::new(Box::new( EthStore::open_with_iterations(dir, cfg.iterations).map_err(|e| format!("Could not open keys directory: {}", e))? )); for a in cfg.unlocked_accounts { - if !passwords.iter().any(|p| account_service.unlock_account_permanently(a, (*p).clone()).is_ok()) { - return Err(format!("No password found to unlock account {}. Make sure valid password is present in files passed using `--password`.", a)); + // Check if the account exists + if !account_provider.has_account(a).unwrap_or(false) { + return Err(format!("Account {} not found for the current chain, please run `parity account new -d current-d --chain current-chain --keys-path current-keys-path`", a)); + } + + // Check if any passwords have been read from the password file(s) + if passwords.is_empty() { + return Err(format!("No password found to unlock account {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", a)); + } + + if !passwords.iter().any(|p| account_provider.unlock_account_permanently(a, (*p).clone()).is_ok()) { + return Err(format!("No valid password to unlock account {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", a)); } } - Ok(account_service) + Ok(account_provider) } fn wait_for_exit( From 516c41c1ee1b92799dd8a3af2eb09b53e8cf746e Mon Sep 17 00:00:00 2001 From: maciejhirsz Date: Wed, 4 Jan 2017 14:48:32 +0100 Subject: [PATCH 3/4] Move hints to constants --- parity/run.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/parity/run.rs b/parity/run.rs index ab9abface..de9d7a639 100644 --- a/parity/run.rs +++ b/parity/run.rs @@ -60,6 +60,12 @@ const SNAPSHOT_PERIOD: u64 = 10000; // how many blocks to wait before starting a periodic snapshot. const SNAPSHOT_HISTORY: u64 = 100; +// Pops along with error message when an account address is not found. +const CREATE_ACCOUNT_HINT: &'static str = "Please run `parity account new [-d current-d --chain current-chain --keys-path current-keys-path]`."; + +// Pops along with error messages when a password is missing or invalid. +const VERIFY_PASSWORD_HINT: &'static str = "Make sure valid password is present in files passed using `--password` or in the configuration file."; + #[derive(Debug, PartialEq)] pub struct RunCmd { pub cache_config: CacheConfig, @@ -234,17 +240,17 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc) -> R if engine_signer != Default::default() { // Check if engine signer exists if !account_provider.has_account(engine_signer).unwrap_or(false) { - return Err("Consensus signer account not found for the current chain, please run `parity account new -d current-d --chain current-chain --keys-path current-keys-path`".to_owned()); + return Err(format!("Consensus signer account not found for the current chain. {}", CREATE_ACCOUNT_HINT)); } // Check if any passwords have been read from the password file(s) if passwords.is_empty() { - return Err(format!("No password found for the consensus signer {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", engine_signer)); + return Err(format!("No password found for the consensus signer {}. {}", engine_signer, VERIFY_PASSWORD_HINT)); } // Attempt to sign in the engine signer. if !passwords.into_iter().any(|p| miner.set_engine_signer(engine_signer, p).is_ok()) { - return Err(format!("No valid password for the consensus signer {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", engine_signer)); + return Err(format!("No valid password for the consensus signer {}. {}", engine_signer, VERIFY_PASSWORD_HINT)); } } @@ -497,16 +503,16 @@ fn prepare_account_provider(dirs: &Directories, data_dir: &str, cfg: AccountsCon for a in cfg.unlocked_accounts { // Check if the account exists if !account_provider.has_account(a).unwrap_or(false) { - return Err(format!("Account {} not found for the current chain, please run `parity account new -d current-d --chain current-chain --keys-path current-keys-path`", a)); + return Err(format!("Account {} not found for the current chain. {}", a, CREATE_ACCOUNT_HINT)); } // Check if any passwords have been read from the password file(s) if passwords.is_empty() { - return Err(format!("No password found to unlock account {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", a)); + return Err(format!("No password found to unlock account {}. {}", a, VERIFY_PASSWORD_HINT)); } if !passwords.iter().any(|p| account_provider.unlock_account_permanently(a, (*p).clone()).is_ok()) { - return Err(format!("No valid password to unlock account {}. Make sure valid password is present in files passed using `--password` or in the configuration file.", a)); + return Err(format!("No valid password to unlock account {}. {}", a, VERIFY_PASSWORD_HINT)); } } From a4b4263580bcc2d8873a3b3c3f4ec84b689a8b8c Mon Sep 17 00:00:00 2001 From: maciejhirsz Date: Wed, 4 Jan 2017 16:51:27 +0100 Subject: [PATCH 4/4] Adaptive hints --- parity/params.rs | 29 ++++++++++++++++++++++++++++- parity/run.rs | 16 +++++++++------- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/parity/params.rs b/parity/params.rs index 9399b33e4..93d109979 100644 --- a/parity/params.rs +++ b/parity/params.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . -use std::{str, fs}; +use std::{str, fs, fmt}; use std::time::Duration; use util::{Address, U256, version_data}; use util::journaldb::Algorithm; @@ -60,6 +60,21 @@ impl str::FromStr for SpecType { } } +impl fmt::Display for SpecType { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str(match *self { + SpecType::Mainnet => "homestead", + SpecType::Morden => "morden", + SpecType::Ropsten => "ropsten", + SpecType::Olympic => "olympic", + SpecType::Classic => "classic", + SpecType::Expanse => "expanse", + SpecType::Dev => "dev", + SpecType::Custom(ref custom) => custom, + }) + } +} + impl SpecType { pub fn spec(&self) -> Result { match *self { @@ -305,6 +320,18 @@ mod tests { assert_eq!(SpecType::Mainnet, SpecType::default()); } + #[test] + fn test_spec_type_display() { + assert_eq!(format!("{}", SpecType::Mainnet), "homestead"); + assert_eq!(format!("{}", SpecType::Ropsten), "ropsten"); + assert_eq!(format!("{}", SpecType::Morden), "morden"); + assert_eq!(format!("{}", SpecType::Olympic), "olympic"); + assert_eq!(format!("{}", SpecType::Classic), "classic"); + assert_eq!(format!("{}", SpecType::Expanse), "expanse"); + assert_eq!(format!("{}", SpecType::Dev), "dev"); + assert_eq!(format!("{}", SpecType::Custom("foo/bar".into())), "foo/bar"); + } + #[test] fn test_pruning_parsing() { assert_eq!(Pruning::Auto, "auto".parse().unwrap()); diff --git a/parity/run.rs b/parity/run.rs index de9d7a639..9c31561eb 100644 --- a/parity/run.rs +++ b/parity/run.rs @@ -60,9 +60,6 @@ const SNAPSHOT_PERIOD: u64 = 10000; // how many blocks to wait before starting a periodic snapshot. const SNAPSHOT_HISTORY: u64 = 100; -// Pops along with error message when an account address is not found. -const CREATE_ACCOUNT_HINT: &'static str = "Please run `parity account new [-d current-d --chain current-chain --keys-path current-keys-path]`."; - // Pops along with error messages when a password is missing or invalid. const VERIFY_PASSWORD_HINT: &'static str = "Make sure valid password is present in files passed using `--password` or in the configuration file."; @@ -223,7 +220,7 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc) -> R let passwords = passwords_from_files(&cmd.acc_conf.password_files)?; // prepare account provider - let account_provider = Arc::new(prepare_account_provider(&cmd.dirs, &spec.data_dir, cmd.acc_conf, &passwords)?); + let account_provider = Arc::new(prepare_account_provider(&cmd.spec, &cmd.dirs, &spec.data_dir, cmd.acc_conf, &passwords)?); // let the Engine access the accounts spec.engine.register_account_provider(account_provider.clone()); @@ -240,7 +237,7 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc) -> R if engine_signer != Default::default() { // Check if engine signer exists if !account_provider.has_account(engine_signer).unwrap_or(false) { - return Err(format!("Consensus signer account not found for the current chain. {}", CREATE_ACCOUNT_HINT)); + return Err(format!("Consensus signer account not found for the current chain. {}", build_create_account_hint(&cmd.spec, &cmd.dirs.keys))); } // Check if any passwords have been read from the password file(s) @@ -489,7 +486,7 @@ fn daemonize(_pid_file: String) -> Result<(), String> { Err("daemon is no supported on windows".into()) } -fn prepare_account_provider(dirs: &Directories, data_dir: &str, cfg: AccountsConfig, passwords: &[String]) -> Result { +fn prepare_account_provider(spec: &SpecType, dirs: &Directories, data_dir: &str, cfg: AccountsConfig, passwords: &[String]) -> Result { use ethcore::ethstore::EthStore; use ethcore::ethstore::dir::DiskDirectory; @@ -503,7 +500,7 @@ fn prepare_account_provider(dirs: &Directories, data_dir: &str, cfg: AccountsCon for a in cfg.unlocked_accounts { // Check if the account exists if !account_provider.has_account(a).unwrap_or(false) { - return Err(format!("Account {} not found for the current chain. {}", a, CREATE_ACCOUNT_HINT)); + return Err(format!("Account {} not found for the current chain. {}", a, build_create_account_hint(spec, &dirs.keys))); } // Check if any passwords have been read from the password file(s) @@ -519,6 +516,11 @@ fn prepare_account_provider(dirs: &Directories, data_dir: &str, cfg: AccountsCon Ok(account_provider) } +// Construct an error `String` with an adaptive hint on how to create an account. +fn build_create_account_hint(spec: &SpecType, keys: &str) -> String { + format!("You can create an account via RPC, UI or `parity account new --chain {} --keys-path {}`.", spec, keys) +} + fn wait_for_exit( panic_handler: Arc, _http_server: Option,