From d7b94a12f539c6a9fa135dc80ea8a3bb3a9f9420 Mon Sep 17 00:00:00 2001 From: AntiochP <30642645+antiochp@users.noreply.github.com> Date: Wed, 25 Oct 2017 17:09:34 -0400 Subject: [PATCH] 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 | 104 ++++++++++++++------------ wallet/src/info.rs | 11 +-- wallet/src/receiver.rs | 166 +++++++++++++++++++++++------------------ wallet/src/sender.rs | 113 +++++++++++++++------------- wallet/src/types.rs | 32 +++++++- 5 files changed, 242 insertions(+), 184 deletions(-) diff --git a/wallet/src/checker.rs b/wallet/src/checker.rs index e9c01212b..8f22fcd94 100644 --- a/wallet/src/checker.rs +++ b/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 { - out.status = OutputStatus::Unspent; + 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) { - out.status = OutputStatus::Spent; + match out.status { + OutputStatus::Unspent | OutputStatus::Locked => { + out.status = OutputStatus::Spent + }, + _ => (), } } @@ -50,53 +57,58 @@ pub fn refresh_outputs( config: &WalletConfig, keychain: &Keychain, ) -> Result<(), Error> { - WalletData::with_wallet(&config.data_file_dir, |wallet_data| { - let mut wallet_outputs: HashMap = HashMap::new(); - let mut commits: Vec = vec![]; + debug!(LOGGER, "Refreshing wallet outputs"); + let mut wallet_outputs: HashMap = HashMap::new(); + let mut commits: Vec = vec![]; - // build a local map of wallet outputs by commits - // and a list of outputs we wantot query the node for + // 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) { - 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()); + .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 + let query_params: Vec = commits + .iter() + .map(|commit| { + let id = util::to_hex(commit.as_ref().to_vec()); + format!("id={}", id) + }) + .collect(); + let query_string = query_params.join("&"); + + let url = format!( + "{}/v2/chain/utxos?{}", + config.check_node_api_http_addr, + query_string, + ); + + // build a map of api outputs by commit so we can look them up efficiently + let mut api_outputs: HashMap = HashMap::new(); + match api::client::get::>(url.as_str()) { + Ok(outputs) => { + for out in outputs { + api_outputs.insert(out.commit, out); } + }, + Err(_) => {}, + }; - // build the necessary query params - - // ?id=xxx&id=yyy&id=zzz - let query_params: Vec = commits - .iter() - .map(|commit| { - let id = util::to_hex(commit.as_ref().to_vec()); - format!("id={}", id) - }) - .collect(); - let query_string = query_params.join("&"); - - let url = format!( - "{}/v2/chain/utxos?{}", - config.check_node_api_http_addr, - query_string, - ); - - // build a map of api outputs by commit so we can look them up efficiently - let mut api_outputs: HashMap = HashMap::new(); - match api::client::get::>(url.as_str()) { - Ok(outputs) => { - for out in outputs { - api_outputs.insert(out.commit, out); - } - }, - Err(_) => {}, - }; - - // now for each commit we want to refresh the output for - // find the corresponding api output (if it exists) - // and refresh it in-place in the wallet + // now for each commit, find the output in the wallet and + // the corresponding api output (if it exists) + // 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()) { diff --git a/wallet/src/info.rs b/wallet/src/info.rs index 1074528d5..78a914956 100644 --- a/wallet/src/info.rs +++ b/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 - let _ = WalletData::with_wallet(&config.data_file_dir, |wallet_data| { + // just read the wallet here, no need for a write lock + 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, ); } diff --git a/wallet/src/receiver.rs b/wallet/src/receiver.rs index 213d7ba92..44a83ce22 100644 --- a/wallet/src/receiver.rs +++ b/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(); + let key_id = block_fees.key_id(); - // operate within a lock on wallet data + let (key_id, derivation) = match key_id { + Some(key_id) => retrieve_existing_key(config, key_id)?, + None => next_available_key(config, keychain)?, + }; + + // Now acquire the wallet lock and write the new output. 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) => { - if let Some(existing) = wallet_data.get_output(&key_id) { - (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) - } - }; - // track the new output and return the stuff needed for reward wallet_data.add_output(OutputData { root_key_id: root_key_id.clone(), @@ -208,29 +228,29 @@ fn receive_coinbase( lock_height: 0, is_coinbase: true, }); + })?; - debug!( - LOGGER, - "Received coinbase and built candidate output - {:?}, {:?}, {}", - root_key_id.clone(), - key_id.clone(), - derivation, - ); + debug!( + LOGGER, + "Received coinbase and built candidate output - {:?}, {:?}, {}", + root_key_id.clone(), + key_id.clone(), + derivation, + ); - debug!(LOGGER, "block_fees - {:?}", block_fees); + debug!(LOGGER, "block_fees - {:?}", block_fees); - let mut block_fees = block_fees.clone(); - block_fees.key_id = Some(key_id.clone()); + let mut block_fees = block_fees.clone(); + block_fees.key_id = Some(key_id.clone()); - debug!(LOGGER, "block_fees updated - {:?}", block_fees); + debug!(LOGGER, "block_fees updated - {:?}", block_fees); - let (out, kern) = Block::reward_output( - &keychain, - &key_id, - block_fees.fees, - )?; - Ok((out, kern, block_fees)) - })? + let (out, kern) = Block::reward_output( + &keychain, + &key_id, + block_fees.fees, + )?; + Ok((out, kern, block_fees)) } /// Builds a full transaction from the partial one sent to us for transfer @@ -243,36 +263,33 @@ fn receive_transaction( ) -> Result { let root_key_id = keychain.root_key_id(); + let (key_id, derivation) = next_available_key(config, keychain)?; + + // double check the fee amount included in the partial tx + // we don't necessarily want to just trust the sender + // we could just overwrite the fee here (but we won't) due to the ecdsa sig + let fee = tx_fee(partial.inputs.len(), partial.outputs.len() + 1, None); + if fee != partial.fee { + return Err(Error::FeeDispute { + sender_fee: partial.fee, + recipient_fee: fee, + }); + } + + let out_amount = amount - fee; + + let (tx_final, _) = build::transaction(vec![ + build::initial_tx(partial), + build::with_excess(blinding), + build::output(out_amount, key_id.clone()), + // build::with_fee(fee_amount), + ], keychain)?; + + // make sure the resulting transaction is valid (could have been lied to on excess). + tx_final.validate(&keychain.secp())?; + // operate within a lock on wallet data 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 - // we could just overwrite the fee here (but we won't) due to the ecdsa sig - let fee = tx_fee(partial.inputs.len(), partial.outputs.len() + 1, None); - if fee != partial.fee { - return Err(Error::FeeDispute { - sender_fee: partial.fee, - recipient_fee: fee, - }); - } - - let out_amount = amount - fee; - - let (tx_final, _) = build::transaction(vec![ - build::initial_tx(partial), - build::with_excess(blinding), - build::output(out_amount, key_id.clone()), - // build::with_fee(fee_amount), - ], keychain)?; - - // make sure the resulting transaction is valid (could have been lied to on - // excess) - tx_final.validate(&keychain.secp())?; - - // track the new output and return the finalized transaction to broadcast wallet_data.add_output(OutputData { root_key_id: root_key_id.clone(), key_id: key_id.clone(), @@ -283,14 +300,15 @@ fn receive_transaction( lock_height: 0, is_coinbase: false, }); - debug!( - LOGGER, - "Received txn and built output - {:?}, {:?}, {}", - root_key_id.clone(), - key_id.clone(), - derivation, - ); + })?; - Ok(tx_final) - })? + debug!( + LOGGER, + "Received txn and built output - {:?}, {:?}, {}", + root_key_id.clone(), + key_id.clone(), + derivation, + ); + + Ok(tx_final) } diff --git a/wallet/src/sender.rs b/wallet/src/sender.rs index c7781d6e8..ba9b6345a 100644 --- a/wallet/src/sender.rs +++ b/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 - WalletData::with_wallet(&config.data_file_dir, |wallet_data| { + // select some spendable coins from the wallet + 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 + let mut parts = inputs_and_change(&coins, config, keychain, key_id, amount)?; - // 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, keychain, key_id, wallet_data, amount)?; + // This is more proof of concept than anything but here we set lock_height + // on tx being sent (based on current chain height via api). + parts.push(build::with_lock_height(lock_height)); - // This is more proof of concept than anything but here we set a - // lock_height on the transaction being sent (based on current chain height via - // api). - parts.push(build::with_lock_height(lock_height)); + let (tx, blind) = build::transaction(parts, &keychain)?; - let (tx, blind) = build::transaction(parts, &keychain)?; - - Ok((tx, blind)) - })? + Ok((tx, blind)) } pub fn issue_burn_tx( @@ -117,39 +113,48 @@ 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 = WalletData::read_wallet(&config.data_file_dir, |wallet_data| { + wallet_data.select(key_id.clone(), current_height, minimum_confirmations) + })?; - // select some spendable coins from the wallet - let coins = wallet_data.select(key_id.clone(), current_height, minimum_confirmations); + let mut parts = inputs_and_change(&coins, config, keychain, key_id, amount)?; - // build transaction skeleton with inputs and change - 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); + parts.push(build::output(amount - fee, Identifier::zero())); - // add burn output and fees - let fee = tx_fee(coins.len(), 2, None); - parts.push(build::output(amount - fee, Identifier::zero())); + // finalize the burn transaction and send + let (tx_burn, _) = build::transaction(parts, &keychain)?; + tx_burn.validate(&keychain.secp())?; - // finalize the burn transaction and send - let (tx_burn, _) = build::transaction(parts, &keychain)?; - tx_burn.validate(&keychain.secp())?; + let tx_hex = util::to_hex(ser::ser_vec(&tx_burn).unwrap()); + let url = format!("{}/v1/pool/push", config.check_node_api_http_addr.as_str()); + let _: () = api::client::post(url.as_str(), &TxWrapper { tx_hex: tx_hex }) + .map_err(|e| Error::Node(e))?; + Ok(()) +} - let tx_hex = util::to_hex(ser::ser_vec(&tx_burn).unwrap()); - let url = format!("{}/v1/pool/push", config.check_node_api_http_addr.as_str()); - 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, + config: &WalletConfig, keychain: &Keychain, root_key_id: Identifier, - wallet_data: &mut WalletData, amount: u64, ) -> Result>, Error> { - let mut parts = vec![]; // calculate the total across all inputs, and how much is left @@ -177,27 +182,29 @@ 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_derivation = wallet_data.next_child(root_key_id.clone()); - let change_key = keychain.derive_key_id(change_derivation)?; + let (change_key, change_derivation) = next_available_key(config, keychain)?; + parts.push(build::output(change, change_key.clone())); - // 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(), - key_id: change_key.clone(), - n_child: change_derivation, - value: change as u64, - status: OutputStatus::Unconfirmed, - height: 0, - lock_height: 0, - is_coinbase: false, - }); + // 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(), + key_id: change_key.clone(), + n_child: change_derivation, + value: change as u64, + status: OutputStatus::Unconfirmed, + height: 0, + lock_height: 0, + is_coinbase: false, + }); - // now lock the ouputs we're spending so we avoid accidental double spend attempt - for coin in coins { - wallet_data.lock_output(coin); - } + // now lock the ouputs we're spending so we avoid accidental double spend attempt + for coin in coins { + wallet_data.lock_output(coin); + } + })?; Ok(parts) } diff --git a/wallet/src/types.rs b/wallet/src/types.rs index a5dcfd1ae..46754e8c2 100644 --- a/wallet/src/types.rs +++ b/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(data_file_dir: &str, f: F) -> Result + 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) }