Protect wallet data file with a file lock

Operations on the wallet data file are now fenced by a lock to
avoid potentially messy concurrent modifications by multiple
processes (i.e. the wallet receiver and a send command). The lock
is done using a create-only lock file, which is an atomic
operation.

pool/src/pool.rs

@@ -115,7 +115,6 @@ impl<T> TransactionPool<T> where T: BlockChain {
         let secp = secp::Secp256k1::with_caps(secp::ContextFlag::Commit);
         tx.validate(&secp).map_err(|_| PoolError::Invalid)?;
 
-
         // The first check invovles ensuring that an identical transaction is 
         // not already in the pool's transaction set.
         // A non-authoritative similar check should be performed under the 

wallet/src/checker.rs

@@ -16,7 +16,7 @@
 //! the wallet storage and update them.
 
 use api;
-use core::core::{Output, DEFAULT_OUTPUT, COINBASE_OUTPUT};
+use core::core::Output;
 use secp::{self, pedersen};
 use util;
 
@@ -27,32 +27,34 @@ use types::{WalletConfig, OutputStatus, WalletData};
 /// with a node whether their status has changed.
 pub fn refresh_outputs(config: &WalletConfig, ext_key: &ExtendedKey) {
 	let secp = secp::Secp256k1::with_caps(secp::ContextFlag::Commit);
-	let mut wallet_data = WalletData::read_or_create().expect("Could not open wallet data.");
 
-	let mut changed = 0;
+	// operate within a lock on wallet data
-	for out in &mut wallet_data.outputs {
+	WalletData::with_wallet(|wallet_data| {
-		if out.status != OutputStatus::Spent {
-			let key = ext_key.derive(&secp, out.n_child).unwrap();
-			let commitment = secp.commit(out.value, key.key).unwrap();
 
-			// TODO check the pool for unconfirmed
+		// check each output that's not spent
+		for out in &mut wallet_data.outputs {
+			if out.status != OutputStatus::Spent {
 
-			let out_res = get_output_by_commitment(config, commitment);
+				// figure out the commitment
-			if out_res.is_ok() {
+				let key = ext_key.derive(&secp, out.n_child).unwrap();
-				out.status = OutputStatus::Unspent;
+				let commitment = secp.commit(out.value, key.key).unwrap();
-				changed += 1;
+
-			} else if out.status == OutputStatus::Unspent {
+				// TODO check the pool for unconfirmed
-				// a UTXO we can't find anymore has been spent
+
-				if let Err(api::Error::NotFound) = out_res {
+				let out_res = get_output_by_commitment(config, commitment);
-					out.status = OutputStatus::Spent;
+				if out_res.is_ok() {
-					changed += 1;
+					// output is known, it's a new utxo
+					out.status = OutputStatus::Unspent;
+
+				} else if out.status == OutputStatus::Unspent {
+					// a UTXO we can't find anymore has been spent
+					if let Err(api::Error::NotFound) = out_res {
+						out.status = OutputStatus::Spent;
+					}
 				}
 			}
 		}
-	}
+	});
-	if changed > 0 {
-		wallet_data.write().unwrap();
-	}
 }
 
 // queries a reachable node for a given output, checking whether it's been

wallet/src/receiver.rs

@@ -143,24 +143,25 @@ impl ApiEndpoint for WalletReceiver {
 fn receive_coinbase(ext_key: &ExtendedKey, amount: u64) -> Result<(Output, TxKernel), Error> {
 	let secp = secp::Secp256k1::with_caps(secp::ContextFlag::Commit);
 
-	// derive a new private for the reward
+	// operate within a lock on wallet data
-	let mut wallet_data = WalletData::read_or_create()?;
+	WalletData::with_wallet(|wallet_data| {
-	let next_child = wallet_data.next_child(ext_key.fingerprint);
-	let coinbase_key = ext_key.derive(&secp, next_child).map_err(|e| Error::Key(e))?;
 
-	// track the new output and return the stuff needed for reward
+		// derive a new private for the reward
-	wallet_data.append_output(OutputData {
+		let next_child = wallet_data.next_child(ext_key.fingerprint);
-		fingerprint: coinbase_key.fingerprint,
+		let coinbase_key = ext_key.derive(&secp, next_child).map_err(|e| Error::Key(e))?;
-		n_child: coinbase_key.n_child,
-		value: amount,
-		status: OutputStatus::Unconfirmed,
-	});
-	wallet_data.write()?;
 
-	debug!("Using child {} for a new coinbase output.",
+		// track the new output and return the stuff needed for reward
-	       coinbase_key.n_child);
+		wallet_data.append_output(OutputData {
+			fingerprint: coinbase_key.fingerprint,
+			n_child: coinbase_key.n_child,
+			value: amount,
+			status: OutputStatus::Unconfirmed,
+		});
+		debug!("Using child {} for a new coinbase output.",
+		       coinbase_key.n_child);
 
-	Block::reward_output(coinbase_key.key, &secp).map_err(&From::from)
+		Block::reward_output(coinbase_key.key, &secp).map_err(&From::from)
+	})?
 }
 
 /// Builds a full transaction from the partial one sent to us for transfer
@@ -172,30 +173,31 @@ fn receive_transaction(ext_key: &ExtendedKey,
 
 	let secp = secp::Secp256k1::with_caps(secp::ContextFlag::Commit);
 
-	// derive a new private for the receiving output
+	// operate within a lock on wallet data
-	let mut wallet_data = WalletData::read_or_create()?;
+	WalletData::with_wallet(|wallet_data| {
-	let next_child = wallet_data.next_child(ext_key.fingerprint);
-	let out_key = ext_key.derive(&secp, next_child).map_err(|e| Error::Key(e))?;
 
-	let (tx_final, _) = build::transaction(vec![build::initial_tx(partial),
+		let next_child = wallet_data.next_child(ext_key.fingerprint);
-	                                            build::with_excess(blinding),
+		let out_key = ext_key.derive(&secp, next_child).map_err(|e| Error::Key(e))?;
-	                                            build::output(amount, out_key.key)])?;
 
-	// make sure the resulting transaction is valid (could have been lied to
+		let (tx_final, _) = build::transaction(vec![build::initial_tx(partial),
-	// on excess)
+		                                            build::with_excess(blinding),
-	tx_final.validate(&secp)?;
+		                                            build::output(amount, out_key.key)])?;
 
-	// track the new output and return the finalized transaction to broadcast
+		// make sure the resulting transaction is valid (could have been lied to
-	wallet_data.append_output(OutputData {
+		// on excess)
-		fingerprint: out_key.fingerprint,
+		tx_final.validate(&secp)?;
-		n_child: out_key.n_child,
-		value: amount,
-		status: OutputStatus::Unconfirmed,
-	});
-	wallet_data.write()?;
 
-	debug!("Using child {} for a new transaction output.",
+		// track the new output and return the finalized transaction to broadcast
-	       out_key.n_child);
+		wallet_data.append_output(OutputData {
+			fingerprint: out_key.fingerprint,
+			n_child: out_key.n_child,
+			value: amount,
+			status: OutputStatus::Unconfirmed,
+		});
 
-	Ok(tx_final)
+		debug!("Using child {} for a new transaction output.",
+		       out_key.n_child);
+
+		Ok(tx_final)
+	})?
 }

wallet/src/sender.rs

@@ -46,38 +46,41 @@ fn build_send_tx(ext_key: &ExtendedKey, amount: u64) -> Result<(Transaction, Sec
 	// first, rebuild the private key from the seed
 	let secp = secp::Secp256k1::with_caps(secp::ContextFlag::Commit);
 
-	// second, check from our local wallet data for outputs to spend
+	// operate within a lock on wallet data
-	let mut wallet_data = WalletData::read()?;
+	WalletData::with_wallet(|wallet_data| {
-	let (coins, change) = wallet_data.select(ext_key.fingerprint, amount);
-	if change < 0 {
-		return Err(Error::NotEnoughFunds((-change) as u64));
-	}
 
-	// TODO add fees, which is likely going to make this iterative
+		// second, check from our local wallet data for outputs to spend
+		let (coins, change) = wallet_data.select(ext_key.fingerprint, amount);
+		if change < 0 {
+			return Err(Error::NotEnoughFunds((-change) as u64));
+		}
 
-	// third, build inputs using the appropriate key
+		// TODO add fees, which is likely going to make this iterative
-	let mut parts = vec![];
-	for coin in &coins {
-		let in_key = ext_key.derive(&secp, coin.n_child).map_err(|e| Error::Key(e))?;
-		parts.push(build::input(coin.value, in_key.key));
-	}
 
-	// fourth, derive a new private for change and build the change output
+		// third, build inputs using the appropriate key
-	let next_child = wallet_data.next_child(ext_key.fingerprint);
+		let mut parts = vec![];
-	let change_key = ext_key.derive(&secp, next_child).map_err(|e| Error::Key(e))?;
+		for coin in &coins {
-	parts.push(build::output(change as u64, change_key.key));
+			let in_key = ext_key.derive(&secp, coin.n_child).map_err(|e| Error::Key(e))?;
+			parts.push(build::input(coin.value, in_key.key));
+		}
 
-	// we got that far, time to start tracking the new output, finalize tx
+		// fourth, derive a new private for change and build the change output
-	// and lock the outputs used
+		let next_child = wallet_data.next_child(ext_key.fingerprint);
-	wallet_data.append_output(OutputData {
+		let change_key = ext_key.derive(&secp, next_child).map_err(|e| Error::Key(e))?;
-		fingerprint: change_key.fingerprint,
+		parts.push(build::output(change as u64, change_key.key));
-		n_child: change_key.n_child,
+
-		value: change as u64,
+		// we got that far, time to start tracking the new output, finalize tx
-		status: OutputStatus::Unconfirmed,
+		// and lock the outputs used
-	});
+		wallet_data.append_output(OutputData {
-	for mut coin in coins {
+			fingerprint: change_key.fingerprint,
-		coin.lock();
+			n_child: change_key.n_child,
-	}
+			value: change as u64,
-	wallet_data.write()?;
+			status: OutputStatus::Unconfirmed,
-	build::transaction(parts).map_err(&From::from)
+		});
+		for mut coin in coins {
+			coin.lock();
+		}
+
+		build::transaction(parts).map_err(&From::from)
+	})?
 }

wallet/src/types.rs

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 use std::convert::From;
-use std::fs::File;
+use std::fs::{self, File, OpenOptions};
 use std::io::Write;
 use std::num;
 use std::path::Path;
@@ -30,6 +30,7 @@ use extkey;
 use util;
 
 const DAT_FILE: &'static str = "wallet.dat";
+const LOCK_FILE: &'static str = "wallet.lock";
 
 /// Wallet errors, mostly wrappers around underlying crypto or I/O errors.
 #[derive(Debug)]
@@ -133,8 +134,38 @@ pub struct WalletData {
 }
 
 impl WalletData {
+	/// 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.
+	/// Note that due to the impossibility to do an actual file lock easily
+	/// across operating systems, this just creates a lock file with a "should
+  /// not exist" option.
+	pub fn with_wallet<T, F>(f: F) -> Result<T, Error>
+		where F: FnOnce(&mut WalletData) -> T
+	{
+		// create the lock files, if it already exists, will produce an error
+    OpenOptions::new().write(true).create_new(true).open(LOCK_FILE).map_err(|e| {
+				Error::WalletData(format!("Could not create wallet lock file. Either \
+            some other process is using the wallet or there's a write access \
+            issue."))
+    })?;
+
+		// do what needs to be done
+		let mut wdat = WalletData::read_or_create()?;
+		let res = f(&mut wdat);
+		wdat.write()?;
+
+		// delete the lock file
+		fs::remove_file(LOCK_FILE).map_err(|e| {
+				Error::WalletData(format!("Could not remove wallet lock file. Maybe insufficient \
+				                           rights?"))
+			})?;
+
+		Ok(res)
+	}
+
 	/// Read the wallet data or created a brand new one if it doesn't exist yet
-	pub fn read_or_create() -> Result<WalletData, Error> {
+	fn read_or_create() -> Result<WalletData, Error> {
 		if Path::new(DAT_FILE).exists() {
 			WalletData::read()
 		} else {
@@ -144,7 +175,7 @@ impl WalletData {
 	}
 
 	/// Read the wallet data from disk.
-	pub fn read() -> Result<WalletData, Error> {
+	fn read() -> Result<WalletData, Error> {
 		let data_file = File::open(DAT_FILE)
       .map_err(|e| Error::WalletData(format!("Could not open {}: {}", DAT_FILE, e)))?;
 		serde_json::from_reader(data_file)
@@ -152,7 +183,7 @@ impl WalletData {
 	}
 
 	/// Write the wallet data to disk.
-	pub fn write(&self) -> Result<(), Error> {
+	fn write(&self) -> Result<(), Error> {
 		let mut data_file = File::create(DAT_FILE)
       .map_err(|e| Error::WalletData(format!("Could not create {}: {}", DAT_FILE, e)))?;
 		let res_json = serde_json::to_vec_pretty(self)