Make accounts refresh time configurable. (#7345)

* Configurable accounts refresh time.

* Fix tests.

ethstore/src/ethstore.rs

@@ -29,8 +29,6 @@ use presale::PresaleWallet;
 use json::{self, Uuid, OpaqueKeyFile};
 use {import, Error, SimpleSecretStore, SecretStore, SecretVaultRef, StoreAccountRef, Derivation, OpaqueSecret};
 
-const REFRESH_TIME_SEC: u64 = 5;
-
 /// Accounts store.
 pub struct EthStore {
 	store: EthMultiStore,
@@ -49,6 +47,16 @@ impl EthStore {
 		})
 	}
 
+	/// Modify account refresh timeout - how often they are re-read from `KeyDirectory`.
+	///
+	/// Setting this to low values (or 0) will cause new accounts to be picked up quickly,
+	/// although it may induce heavy disk reads and is not recommended if you manage many keys (say over 10k).
+	///
+	/// By default refreshing is disabled, so only accounts created using this instance of `EthStore` are taken into account.
+	pub fn set_refresh_time(&self, time: Duration) {
+		self.store.set_refresh_time(time)
+	}
+
 	fn get(&self, account: &StoreAccountRef) -> Result<SafeAccount, Error> {
 		let mut accounts = self.store.get_accounts(account)?.into_iter();
 		accounts.next().ok_or(Error::InvalidAccount)
@@ -254,6 +262,7 @@ pub struct EthMultiStore {
 struct Timestamp {
 	dir_hash: Option<u64>,
 	last_checked: Instant,
+	refresh_time: Duration,
 }
 
 impl EthMultiStore {
@@ -272,16 +281,28 @@ impl EthMultiStore {
 			timestamp: Mutex::new(Timestamp {
 				dir_hash: None,
 				last_checked: Instant::now(),
+				// by default we never refresh accounts
+				refresh_time: Duration::from_secs(u64::max_value()),
 			}),
 		};
 		store.reload_accounts()?;
 		Ok(store)
 	}
 
+	/// Modify account refresh timeout - how often they are re-read from `KeyDirectory`.
+	///
+	/// Setting this to low values (or 0) will cause new accounts to be picked up quickly,
+	/// although it may induce heavy disk reads and is not recommended if you manage many keys (say over 10k).
+	///
+	/// By default refreshing is disabled, so only accounts created using this instance of `EthStore` are taken into account.
+	pub fn set_refresh_time(&self, time: Duration) {
+		self.timestamp.lock().refresh_time = time;
+	}
+
 	fn reload_if_changed(&self) -> Result<(), Error> {
 		let mut last_timestamp = self.timestamp.lock();
 		let now = Instant::now();
-		if (now - last_timestamp.last_checked) > Duration::from_secs(REFRESH_TIME_SEC) {
+		if now - last_timestamp.last_checked > last_timestamp.refresh_time {
 			let dir_hash = Some(self.dir.unique_repr()?);
 			last_timestamp.last_checked = now;
 			if last_timestamp.dir_hash == dir_hash {
@@ -319,22 +340,23 @@ impl EthMultiStore {
 	}
 
 	fn get_accounts(&self, account: &StoreAccountRef) -> Result<Vec<SafeAccount>, Error> {
-		{
+		let from_cache = |account| {
 			let cache = self.cache.read();
 			if let Some(accounts) = cache.get(account) {
 				if !accounts.is_empty() {
-					return Ok(accounts.clone())
+					return Some(accounts.clone())
 				}
 			}
-		}
 
-		self.reload_if_changed()?;
+			None
-		let cache = self.cache.read();
+		};
-		let accounts = cache.get(account).ok_or(Error::InvalidAccount)?;
+
-		if accounts.is_empty() {
+		match from_cache(account) {
-			Err(Error::InvalidAccount)
+			Some(accounts) => Ok(accounts),
-		} else {
+			None => {
-			Ok(accounts.clone())
+				self.reload_if_changed()?;
+				from_cache(account).ok_or(Error::InvalidAccount)
+			}
 		}
 	}
 
@@ -470,11 +492,20 @@ impl SimpleSecretStore for EthMultiStore {
 	}
 
 	fn account_ref(&self, address: &Address) -> Result<StoreAccountRef, Error> {
-		use std::collections::Bound;
+		let read_from_cache = |address: &Address| {
-		self.reload_if_changed()?;
+			use std::collections::Bound;
-		let cache = self.cache.read();
+			let cache = self.cache.read();
-		let mut r = cache.range((Bound::Included(*address), Bound::Included(*address)));
+			let mut r = cache.range((Bound::Included(*address), Bound::Included(*address)));
-		r.next().ok_or(Error::InvalidAccount).map(|(k, _)| k.clone())
+			r.next().map(|(k, _)| k.clone())
+		};
+
+		match read_from_cache(address) {
+			Some(account) => Ok(account),
+			None => {
+				self.reload_if_changed()?;
+				read_from_cache(address).ok_or(Error::InvalidAccount)
+			}
+		}
 	}
 
 	fn accounts(&self) -> Result<Vec<StoreAccountRef>, Error> {

parity/cli/mod.rs

@@ -335,6 +335,10 @@ usage! {
 			"--keys-iterations=[NUM]",
 			"Specify the number of iterations to use when deriving key from the password (bigger is more secure)",
 
+			ARG arg_accounts_refresh: (u64) = 5u64, or |c: &Config| otry!(c.account).refresh_time.clone(),
+			"--accounts-refresh=[TIME]",
+			"Specify the cache time of accounts read from disk. If you manage thousands of accounts set this to 0 to disable refresh.",
+
 			ARG arg_unlock: (Option<String>) = None, or |c: &Config| otry!(c.account).unlock.as_ref().map(|vec| vec.join(",")),
 			"--unlock=[ACCOUNTS]",
 			"Unlock ACCOUNTS for the duration of the execution. ACCOUNTS is a comma-delimited list of addresses. Implies --no-ui.",
@@ -1009,6 +1013,7 @@ struct Account {
 	unlock: Option<Vec<String>>,
 	password: Option<Vec<String>>,
 	keys_iterations: Option<u32>,
+	refresh_time: Option<u64>,
 	disable_hardware: Option<bool>,
 	fast_unlock: Option<bool>,
 }
@@ -1428,6 +1433,7 @@ mod tests {
 			arg_unlock: Some("0xdeadbeefcafe0000000000000000000000000000".into()),
 			arg_password: vec!["~/.safe/password.file".into()],
 			arg_keys_iterations: 10240u32,
+			arg_accounts_refresh: 5u64,
 			flag_no_hardware_wallets: false,
 			flag_fast_unlock: false,
 
@@ -1665,6 +1671,7 @@ mod tests {
 				unlock: Some(vec!["0x1".into(), "0x2".into(), "0x3".into()]),
 				password: Some(vec!["passwdfile path".into()]),
 				keys_iterations: None,
+				refresh_time: None,
 				disable_hardware: None,
 				fast_unlock: None,
 			}),

parity/configuration.rs

@@ -484,6 +484,7 @@ impl Configuration {
 	fn accounts_config(&self) -> Result<AccountsConfig, String> {
 		let cfg = AccountsConfig {
 			iterations: self.args.arg_keys_iterations,
+			refresh_time: self.args.arg_accounts_refresh,
 			testnet: self.args.flag_testnet,
 			password_files: self.args.arg_password.clone(),
 			unlocked_accounts: to_addresses(&self.args.arg_unlock)?,

parity/params.rs

@@ -188,6 +188,7 @@ impl str::FromStr for ResealPolicy {
 #[derive(Debug, PartialEq)]
 pub struct AccountsConfig {
 	pub iterations: u32,
+	pub refresh_time: u64,
 	pub testnet: bool,
 	pub password_files: Vec<String>,
 	pub unlocked_accounts: Vec<Address>,
@@ -199,6 +200,7 @@ impl Default for AccountsConfig {
 	fn default() -> Self {
 		AccountsConfig {
 			iterations: 10240,
+			refresh_time: 5,
 			testnet: false,
 			password_files: Vec::new(),
 			unlocked_accounts: Vec::new(),

parity/run.rs

@@ -909,9 +909,15 @@ fn prepare_account_provider(spec: &SpecType, dirs: &Directories, data_dir: &str,
 			],
 		},
 	};
+
+	let ethstore = EthStore::open_with_iterations(dir, cfg.iterations).map_err(|e| format!("Could not open keys directory: {}", e))?;
+	if cfg.refresh_time > 0 {
+		ethstore.set_refresh_time(::std::time::Duration::from_secs(cfg.refresh_time));
+	}
 	let account_provider = AccountProvider::new(
-		Box::new(EthStore::open_with_iterations(dir, cfg.iterations).map_err(|e| format!("Could not open keys directory: {}", e))?),
+		Box::new(ethstore),
-		account_settings);
+		account_settings,
+	);
 
 	for a in cfg.unlocked_accounts {
 		// Check if the account exists