Merge pull request #4034 from maciejhirsz/mh-pwmsg

Better error messages for PoA chains, closes #4030

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<bool, Error> {
+		Ok(self.accounts()?.iter().any(|&a| a == address))
+	}
+
 	/// Returns addresses of all accounts.
 	pub fn accounts(&self) -> Result<Vec<Address>, Error> {
 		let accounts = self.sstore.accounts()?;

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 <http://www.gnu.org/licenses/>.
 
-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<Spec, String> {
 		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());

parity/run.rs

@@ -60,6 +60,9 @@ const SNAPSHOT_PERIOD: u64 = 10000;
 // how many blocks to wait before starting a periodic snapshot.
 const SNAPSHOT_HISTORY: u64 = 100;
 
+// 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,
@@ -217,7 +220,7 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc<RotatingLogger>) -> 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());
@@ -230,9 +233,21 @@ pub fn execute(cmd: RunCmd, can_restart: bool, logger: Arc<RotatingLogger>) -> 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. {}", build_create_account_hint(&cmd.spec, &cmd.dirs.keys)));
+		}
+
+		// 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 {}. {}", 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 password found for the consensus signer {}. Make sure valid password is present in files passed using `--password`.", engine_signer));
+			return Err(format!("No valid password for the consensus signer {}. {}", engine_signer, VERIFY_PASSWORD_HINT));
 		}
 	}
 
@@ -471,24 +486,39 @@ 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<AccountProvider, String> {
+fn prepare_account_provider(spec: &SpecType, dirs: &Directories, data_dir: &str, cfg: AccountsConfig, passwords: &[String]) -> Result<AccountProvider, String> {
 	use ethcore::ethstore::EthStore;
 	use ethcore::ethstore::dir::DiskDirectory;
 
 	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()) {
+		// Check if the account exists
-			return Err(format!("No password found to unlock account {}. Make sure valid password is present in files passed using `--password`.", a));
+		if !account_provider.has_account(a).unwrap_or(false) {
+			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)
+		if passwords.is_empty() {
+			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 {}. {}", a, VERIFY_PASSWORD_HINT));
 		}
 	}
 
-	Ok(account_service)
+	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(