only hold wallet write lock for write operations on the wallet (#210)

* mount v2 router for flexibility, wallet checker now refreshes multiple outputs via single api call
* add read_wallet so we can read without acquiring the lock
* fix the api router
* read wallet without acquiring or holding lock, only acquire the write lock for wallet when updating or adding outputs

wallet/src/checker.rs

@@ -23,6 +23,7 @@ use types::*;
 use keychain::{Identifier, Keychain};
 use secp::pedersen;
 use util;
+use util::LOGGER;
 
 // Transitions a local wallet output from Unconfirmed -> Unspent.
 // Also updates the height and lock_height based on latest from the api.
@@ -30,8 +31,11 @@ fn refresh_output(out: &mut OutputData, api_out: &api::Output) {
 	out.height = api_out.height;
 	out.lock_height = api_out.lock_height;
 
-	if out.status == OutputStatus::Unconfirmed {
+	match out.status {
+		OutputStatus::Unconfirmed => {
 			out.status = OutputStatus::Unspent;
+		},
+		_ => (),
 	}
 }
 
@@ -39,8 +43,11 @@ fn refresh_output(out: &mut OutputData, api_out: &api::Output) {
 // Unspent -> Spent
 // Locked -> Spent
 fn mark_spent_output(out: &mut OutputData) {
-	if vec![OutputStatus::Unspent, OutputStatus::Locked].contains(&out.status) {
+	match out.status {
-		out.status = OutputStatus::Spent;
+		OutputStatus::Unspent | OutputStatus::Locked => {
+			out.status = OutputStatus::Spent
+		},
+		_ => (),
 	}
 }
 
@@ -50,21 +57,24 @@ pub fn refresh_outputs(
 	config: &WalletConfig,
 	keychain: &Keychain,
 ) -> Result<(), Error> {
-	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
+	debug!(LOGGER, "Refreshing wallet outputs");
 	let mut wallet_outputs: HashMap<pedersen::Commitment, Identifier> = HashMap::new();
 	let mut commits: Vec<pedersen::Commitment> = vec![];
 
 	// build a local map of wallet outputs by commits
 	// and a list of outputs we wantot query the node for
+	let _ = WalletData::read_wallet(&config.data_file_dir, |wallet_data| {
 		for out in wallet_data.outputs
-			.values_mut()
+			.values()
 			.filter(|out| out.root_key_id == keychain.root_key_id())
-			.filter(|out| out.status != OutputStatus::Spent) {
+			.filter(|out| out.status != OutputStatus::Spent)
+		{
 			let key_id = keychain.derive_key_id(out.n_child).unwrap();
 			let commit = keychain.commit(out.value, &key_id).unwrap();
 			commits.push(commit);
 			wallet_outputs.insert(commit, out.key_id.clone());
 		}
+	});
 
 	// build the necessary query params -
 	// ?id=xxx&id=yyy&id=zzz
@@ -94,9 +104,11 @@ pub fn refresh_outputs(
 		Err(_) => {},
 	};
 
-		// now for each commit we want to refresh the output for
+	// now for each commit, find the output in the wallet and
-		// find the corresponding api output (if it exists)
+	// the corresponding api output (if it exists)
-		// and refresh it in-place in the wallet
+	// and refresh it in-place in the wallet.
+	// Note: minimizing the time we spend holding the wallet lock.
+	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 		for commit in commits {
 			let id = wallet_outputs.get(&commit).unwrap();
 			if let Entry::Occupied(mut output) = wallet_data.outputs.entry(id.to_hex()) {

wallet/src/info.rs

@@ -20,8 +20,8 @@ pub fn show_info(config: &WalletConfig, keychain: &Keychain) {
 	let root_key_id = keychain.root_key_id();
 	let _ = checker::refresh_outputs(&config, &keychain);
 
-	// operate within a lock on wallet data
+	// just read the wallet here, no need for a write lock
-	let _ = WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
+	let _ = WalletData::read_wallet(&config.data_file_dir, |wallet_data| {
 
 		// get the current height via the api
 		// if we cannot get the current height use the max height known to the wallet
@@ -35,11 +35,8 @@ pub fn show_info(config: &WalletConfig, keychain: &Keychain) {
 			}
 		};
 
-		// need to specify a default value here somehow
-		let minimum_confirmations = 1;
-
 		println!("Outputs - ");
-		println!("key_id, height, lock_height, status, spendable?, coinbase?, value");
+		println!("key_id, height, lock_height, status, coinbase?, num_confs, value");
 		println!("----------------------------------");
 
 		let mut outputs = wallet_data
@@ -55,8 +52,8 @@ pub fn show_info(config: &WalletConfig, keychain: &Keychain) {
 				out.height,
 				out.lock_height,
 				out.status,
-				out.eligible_to_spend(current_height, minimum_confirmations),
 				out.is_coinbase,
+				out.num_confirmations(current_height),
 				out.value,
 			);
 		}

wallet/src/receiver.rs

@@ -1,4 +1,4 @@
-// Copyright 2016 The Grin Developers
+// Copyright 2017 The Grin Developers
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -53,7 +53,7 @@ use core::consensus::reward;
 use core::core::{Block, Transaction, TxKernel, Output, build};
 use core::ser;
 use api::{self, ApiEndpoint, Operation, ApiResult};
-use keychain::{BlindingFactor, Keychain};
+use keychain::{BlindingFactor, Identifier, Keychain};
 use types::*;
 use util;
 use util::LOGGER;
@@ -171,6 +171,36 @@ impl ApiEndpoint for WalletReceiver {
 	}
 }
 
+// Read wallet data without acquiring the write lock.
+fn retrieve_existing_key(
+	config: &WalletConfig,
+	key_id: Identifier,
+) -> Result<(Identifier, u32), Error> {
+	let res = WalletData::read_wallet(&config.data_file_dir, |wallet_data| {
+		if let Some(existing) = wallet_data.get_output(&key_id) {
+			let key_id = existing.key_id.clone();
+			let derivation = existing.n_child;
+			(key_id, derivation)
+		} else {
+			panic!("should never happen");
+		}
+	})?;
+	Ok(res)
+}
+
+fn next_available_key(
+	config: &WalletConfig,
+	keychain: &Keychain,
+) -> Result<(Identifier, u32), Error> {
+	let res = WalletData::read_wallet(&config.data_file_dir, |wallet_data| {
+		let root_key_id = keychain.root_key_id();
+		let derivation = wallet_data.next_child(root_key_id.clone());
+		let key_id = keychain.derive_key_id(derivation).unwrap();
+		(key_id, derivation)
+	})?;
+	Ok(res)
+}
+
 /// Build a coinbase output and the corresponding kernel
 fn receive_coinbase(
 	config: &WalletConfig,
@@ -178,25 +208,15 @@ fn receive_coinbase(
 	block_fees: &BlockFees
 ) -> Result<(Output, TxKernel, BlockFees), Error> {
 	let root_key_id = keychain.root_key_id();
-
-	// operate within a lock on wallet data
-	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 	let key_id = block_fees.key_id();
+
 	let (key_id, derivation) = match key_id {
-			Some(key_id) => {
+		Some(key_id) => retrieve_existing_key(config, key_id)?,
-				if let Some(existing) = wallet_data.get_output(&key_id) {
+		None => next_available_key(config, keychain)?,
-					(existing.key_id.clone(), existing.n_child)
-				} else {
-					panic!("should never happen");
-				}
-			},
-			None => {
-				let derivation = wallet_data.next_child(root_key_id.clone());
-				let key_id = keychain.derive_key_id(derivation)?;
-				(key_id, derivation)
-			}
 	};
 
+	// Now acquire the wallet lock and write the new output.
+	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 		// track the new output and return the stuff needed for reward
 		wallet_data.add_output(OutputData {
 			root_key_id: root_key_id.clone(),
@@ -208,6 +228,7 @@ fn receive_coinbase(
 			lock_height: 0,
 			is_coinbase: true,
 		});
+	})?;
 
 	debug!(
 		LOGGER,
@@ -230,7 +251,6 @@ fn receive_coinbase(
 		block_fees.fees,
 	)?;
 	Ok((out, kern, block_fees))
-	})?
 }
 
 /// Builds a full transaction from the partial one sent to us for transfer
@@ -243,10 +263,7 @@ fn receive_transaction(
 ) -> Result<Transaction, Error> {
 	let root_key_id = keychain.root_key_id();
 
-	// operate within a lock on wallet data
+	let (key_id, derivation) = next_available_key(config, keychain)?;
-	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
-		let derivation = wallet_data.next_child(root_key_id.clone());
-		let key_id = keychain.derive_key_id(derivation)?;
 
 	// double check the fee amount included in the partial tx
 	// we don't necessarily want to just trust the sender
@@ -268,11 +285,11 @@ fn receive_transaction(
 		// build::with_fee(fee_amount),
 	], keychain)?;
 
-		// make sure the resulting transaction is valid (could have been lied to on
+	// make sure the resulting transaction is valid (could have been lied to on excess).
-		// excess)
 	tx_final.validate(&keychain.secp())?;
 
-		// track the new output and return the finalized transaction to broadcast
+	// operate within a lock on wallet data
+	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 		wallet_data.add_output(OutputData {
 			root_key_id: root_key_id.clone(),
 			key_id: key_id.clone(),
@@ -283,6 +300,8 @@ fn receive_transaction(
 			lock_height: 0,
 			is_coinbase: false,
 		});
+	})?;
+
 	debug!(
 		LOGGER,
 		"Received txn and built output - {:?}, {:?}, {}",
@@ -292,5 +311,4 @@ fn receive_transaction(
 	);
 
 	Ok(tx_final)
-	})?
 }

wallet/src/sender.rs

@@ -81,25 +81,21 @@ fn build_send_tx(
 ) -> Result<(Transaction, BlindingFactor), Error> {
 	let key_id = keychain.clone().root_key_id();
 
-	// operate within a lock on wallet data
+	// select some spendable coins from the wallet
-	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
+	let coins = WalletData::read_wallet(&config.data_file_dir, |wallet_data| {
-
+		wallet_data.select(key_id.clone(), current_height, minimum_confirmations)
-		// select some spendable coins from our local wallet
+	})?;
-		let coins = wallet_data.select(key_id.clone(), current_height, minimum_confirmations);
 
 	// build transaction skeleton with inputs and change
-		// TODO - should probably also check we are sending enough to cover the fees + non-zero output
+	let mut parts = inputs_and_change(&coins, config, keychain, key_id, amount)?;
-		let mut parts = inputs_and_change(&coins, keychain, key_id, wallet_data, amount)?;
 
-		// This is more proof of concept than anything but here we set a
+	// This is more proof of concept than anything but here we set lock_height
-		// lock_height on the transaction being sent (based on current chain height via
+	// on tx being sent (based on current chain height via api).
-		// api).
 	parts.push(build::with_lock_height(lock_height));
 
 	let (tx, blind) = build::transaction(parts, &keychain)?;
 
 	Ok((tx, blind))
-	})?
 }
 
 pub fn issue_burn_tx(
@@ -117,14 +113,12 @@ pub fn issue_burn_tx(
 
 	let key_id = keychain.root_key_id();
 
-	// operate within a lock on wallet data
-	WalletData::with_wallet(&config.data_file_dir, |mut wallet_data| {
-
 	// select some spendable coins from the wallet
-		let coins = wallet_data.select(key_id.clone(), current_height, minimum_confirmations);
+	let coins = WalletData::read_wallet(&config.data_file_dir, |wallet_data| {
+		wallet_data.select(key_id.clone(), current_height, minimum_confirmations)
+	})?;
 
-		// build transaction skeleton with inputs and change
+	let mut parts = inputs_and_change(&coins, config, keychain, key_id, amount)?;
-		let mut parts = inputs_and_change(&coins, keychain, key_id, &mut wallet_data, amount)?;
 
 	// add burn output and fees
 	let fee = tx_fee(coins.len(), 2, None);
@@ -139,17 +133,28 @@ pub fn issue_burn_tx(
 	let _: () = api::client::post(url.as_str(), &TxWrapper { tx_hex: tx_hex })
 		.map_err(|e| Error::Node(e))?;
 	Ok(())
-	})?
+}
+
+fn next_available_key(
+	config: &WalletConfig,
+	keychain: &Keychain,
+) -> Result<(Identifier, u32), Error> {
+	let res = WalletData::read_wallet(&config.data_file_dir, |wallet_data| {
+		let root_key_id = keychain.root_key_id();
+		let derivation = wallet_data.next_child(root_key_id.clone());
+		let key_id = keychain.derive_key_id(derivation).unwrap();
+		(key_id, derivation)
+	})?;
+	Ok(res)
 }
 
 fn inputs_and_change(
 	coins: &Vec<OutputData>,
+	config: &WalletConfig,
 	keychain: &Keychain,
 	root_key_id: Identifier,
-	wallet_data: &mut WalletData,
 	amount: u64,
 ) -> Result<Vec<Box<build::Append>>, Error> {
-
 	let mut parts = vec![];
 
 	// calculate the total across all inputs, and how much is left
@@ -177,11 +182,12 @@ fn inputs_and_change(
 		parts.push(build::input(coin.value, key_id));
 	}
 
-	// derive an additional pubkey for change and build the change output
+	let (change_key, change_derivation) = next_available_key(config, keychain)?;
-	let change_derivation = wallet_data.next_child(root_key_id.clone());
+
-	let change_key = keychain.derive_key_id(change_derivation)?;
 	parts.push(build::output(change, change_key.clone()));
 
+	// Acquire wallet lock, add the new change output and lock coins being spent.
+	WalletData::with_wallet(&config.data_file_dir, |wallet_data| {
 		// we got that far, time to start tracking the output representing our change
 		wallet_data.add_output(OutputData {
 			root_key_id: root_key_id.clone(),
@@ -198,6 +204,7 @@ fn inputs_and_change(
 		for coin in coins {
 			wallet_data.lock_output(coin);
 		}
+	})?;
 
 	Ok(parts)
 }

wallet/src/types.rs

@@ -188,6 +188,16 @@ impl OutputData {
 		self.status = OutputStatus::Locked;
 	}
 
+	/// How many confirmations has this output received?
+	pub fn num_confirmations(&self, current_height: u64) -> u64 {
+		if self.status == OutputStatus::Unconfirmed {
+			0
+		} else {
+			current_height - self.height
+		}
+	}
+
+	/// Check if output is eligible for spending based on state and height.
 	pub fn eligible_to_spend(
 		&self,
 		current_height: u64,
@@ -318,6 +328,18 @@ pub struct WalletData {
 }
 
 impl WalletData {
+
+	/// Allows for reading wallet data (without needing to acquire the write lock).
+	pub fn read_wallet<T, F>(data_file_dir: &str, f: F) -> Result<T, Error>
+		where F: FnOnce(&WalletData) -> T
+	{
+		// open the wallet readonly and do what needs to be done with it
+		let data_file_path = &format!("{}{}{}", data_file_dir, MAIN_SEPARATOR, DAT_FILE);
+		let wdat = WalletData::read_or_create(data_file_path)?;
+		let res = f(&wdat);
+		Ok(res)
+	}
+
 	/// Allows the reading and writing of the wallet data within a file lock.
 	/// Just provide a closure taking a mutable WalletData. The lock should
 	/// be held for as short a period as possible to avoid contention.
@@ -335,7 +357,7 @@ impl WalletData {
 		let data_file_path = &format!("{}{}{}", data_file_dir, MAIN_SEPARATOR, DAT_FILE);
 		let lock_file_path = &format!("{}{}{}", data_file_dir, MAIN_SEPARATOR, LOCK_FILE);
 
-		// create the lock files, if it already exists, will produce an error
+		// create the lock file, if it already exists, will produce an error
 		// sleep and retry a few times if we cannot get it the first time
 		let mut retries = 0;
 		loop {
@@ -351,10 +373,11 @@ impl WalletData {
 				});
 			match result {
 				Ok(_) => {
+					info!(LOGGER, "acquired wallet lock ...");
 					break;
 				}
 				Err(e) => {
-					if retries >= 6 {
+					if retries >= 10 {
 						info!(
 							LOGGER,
 							"failed to obtain wallet.lock after {} retries, \
@@ -369,12 +392,11 @@ impl WalletData {
 						retries
 					);
 					retries += 1;
-					thread::sleep(time::Duration::from_millis(1000));
+					thread::sleep(time::Duration::from_millis(250));
 				}
 			}
 		}
 
-
 		// do what needs to be done
 		let mut wdat = WalletData::read_or_create(data_file_path)?;
 		let res = f(&mut wdat);
@@ -387,6 +409,8 @@ impl WalletData {
 			))
 		})?;
 
+		info!(LOGGER, "... released wallet lock");
+
 		Ok(res)
 	}