From 2f09d2e630799f0756c7e85dbcc1ccd02f4b3335 Mon Sep 17 00:00:00 2001 From: AntiochP <30642645+antiochp@users.noreply.github.com> Date: Mon, 20 Nov 2017 14:12:52 -0500 Subject: [PATCH] rollback the change output in the wallet on tx failure (#345) --- src/bin/grin.rs | 28 +++++++++++++++++--- wallet/src/sender.rs | 62 +++++++++++++++++++++----------------------- wallet/src/types.rs | 5 ++++ 3 files changed, 59 insertions(+), 36 deletions(-) diff --git a/src/bin/grin.rs b/src/bin/grin.rs index 2f0604639..783a7fc00 100644 --- a/src/bin/grin.rs +++ b/src/bin/grin.rs @@ -42,6 +42,7 @@ use daemonize::Daemonize; use config::GlobalConfig; use wallet::WalletConfig; use core::global; +use core::core::amount_to_hr_string; use util::{init_logger, LoggingConfig, LOGGER}; fn start_from_config_file(mut global_config: GlobalConfig) { @@ -261,7 +262,7 @@ fn main() { .subcommand(SubCommand::with_name("restore") .about("Attempt to restore wallet contents from the chain using seed and password."))) - + .get_matches(); match args.subcommand() { @@ -453,9 +454,28 @@ fn wallet_command(wallet_args: &ArgMatches) { (selection_strategy == "all"), ); match result { - Ok(_) => { debug!(LOGGER, "{} coins sent using strategy {} to {}", amount.to_string(), selection_strategy, dest) }, //success messaged logged internally - Err(wallet::Error::NotEnoughFunds(_)) => {}, - Err(e) => panic!(e), + Ok(_) => { + info!( + LOGGER, + "Tx sent: {} grin to {} (strategy '{}')", + amount_to_hr_string(amount), + dest, + selection_strategy, + )}, + Err(wallet::Error::NotEnoughFunds(available)) => { + error!( + LOGGER, + "Tx not sent: insufficient funds (max: {})", + amount_to_hr_string(available), + ); + }, + Err(e) => { + error!( + LOGGER, + "Tx not sent: {:?}", + e + ); + }, }; } ("burn", Some(send_args)) => { diff --git a/wallet/src/sender.rs b/wallet/src/sender.rs index 8dd5292c3..0a47198fb 100644 --- a/wallet/src/sender.rs +++ b/wallet/src/sender.rs @@ -17,7 +17,7 @@ use serde_json; use api; use client; use checker; -use core::core::{build, Transaction, amount_to_hr_string}; +use core::core::{build, Transaction}; use core::ser; use keychain::{BlindingFactor, Identifier, Keychain}; use receiver::TxWrapper; @@ -47,7 +47,7 @@ pub fn issue_send_tx( // proof of concept - set lock_height on the tx let lock_height = chain_tip.height; - let (tx, blind_sum, coins) = build_send_tx( + let (tx, blind_sum, coins, change_key) = build_send_tx( config, keychain, amount, @@ -60,14 +60,20 @@ pub fn issue_send_tx( let partial_tx = build_partial_tx(amount, blind_sum, tx); - // Acquire wallet lock and lock the coins being spent - // so we avoid accidental double spend attempt + // Closure to acquire wallet lock and lock the coins being spent + // so we avoid accidental double spend attempt. let update_wallet = || WalletData::with_wallet(&config.data_file_dir, |wallet_data| { for coin in coins { wallet_data.lock_output(&coin); } }); + // Closure to acquire wallet lock and delete the change output in case of tx failure. + let rollback_wallet = || WalletData::with_wallet(&config.data_file_dir, |wallet_data| { + info!(LOGGER, "cleaning up unused change output from wallet"); + wallet_data.delete_output(&change_key); + }); + if dest == "stdout" { let json_tx = serde_json::to_string_pretty(&partial_tx).unwrap(); update_wallet()?; @@ -75,13 +81,14 @@ pub fn issue_send_tx( } else if &dest[..4] == "http" { let url = format!("{}/v1/receive/transaction", &dest); debug!(LOGGER, "Posting partial transaction to {}", url); - let result=client::send_partial_tx(&url, &partial_tx); - match result { - Err(_)=> { + let res = client::send_partial_tx(&url, &partial_tx); + match res { + Err(_) => { error!(LOGGER, "Communication with receiver failed. Aborting transaction"); - return Ok(()); + rollback_wallet()?; + return res; } - Ok(_)=> { + Ok(_) => { update_wallet()?; } } @@ -103,7 +110,7 @@ fn build_send_tx( lock_height: u64, max_outputs: usize, default_strategy: bool, -) -> Result<(Transaction, BlindingFactor, Vec), Error> { +) -> Result<(Transaction, BlindingFactor, Vec, Identifier), Error> { let key_id = keychain.clone().root_key_id(); // select some spendable coins from the wallet @@ -119,23 +126,15 @@ fn build_send_tx( })?; // build transaction skeleton with inputs and change - let parts = inputs_and_change(&coins, config, keychain, amount); - - if let Err(p) = parts { - let total: u64 = coins.iter().map(|c| c.value).sum(); - error!(LOGGER, "Transaction not sent - Not enough funds (Max: {})", amount_to_hr_string(total)); - return Err(p); - } - - let mut parts=parts.unwrap(); + let (mut parts, change_key) = inputs_and_change(&coins, config, keychain, 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.0.push(build::with_lock_height(lock_height)); + parts.push(build::with_lock_height(lock_height)); - let (tx, blind) = build::transaction(parts.0, &keychain)?; + let (tx, blind) = build::transaction(parts, &keychain)?; - Ok((tx, blind, coins)) + Ok((tx, blind, coins, change_key)) } pub fn issue_burn_tx( @@ -168,14 +167,14 @@ pub fn issue_burn_tx( debug!(LOGGER, "selected some coins - {}", coins.len()); - let mut parts = inputs_and_change(&coins, config, keychain, amount)?; + let (mut parts, _) = inputs_and_change(&coins, config, keychain, amount)?; // add burn output and fees let fee = tx_fee(coins.len(), 2, None); - parts.0.push(build::output(amount - fee, Identifier::zero())); + parts.push(build::output(amount - fee, Identifier::zero())); // finalize the burn transaction and send - let (tx_burn, _) = build::transaction(parts.0, &keychain)?; + let (tx_burn, _) = build::transaction(parts, &keychain)?; tx_burn.validate()?; let tx_hex = util::to_hex(ser::ser_vec(&tx_burn).unwrap()); @@ -190,14 +189,13 @@ fn inputs_and_change( config: &WalletConfig, keychain: &Keychain, amount: u64, -) -> Result<(Vec>,Identifier, u32, u64), Error> { +) -> Result<(Vec>, Identifier), Error> { let mut parts = vec![]; // calculate the total across all inputs, and how much is left let total: u64 = coins.iter().map(|c| c.value).sum(); - let shortage = (total as i64) - (amount as i64); - if shortage < 0 { - return Err(Error::NotEnoughFunds((-shortage) as u64)); + if total < amount { + return Err(Error::NotEnoughFunds(total as u64)); } // sender is responsible for setting the fee on the partial tx @@ -219,7 +217,7 @@ fn inputs_and_change( } // track the output representing our change - let (change_key, change_derivation) = WalletData::with_wallet(&config.data_file_dir, |wallet_data| { + let change_key = WalletData::with_wallet(&config.data_file_dir, |wallet_data| { let root_key_id = keychain.root_key_id(); let change_derivation = wallet_data.next_child(root_key_id.clone()); let change_key = keychain.derive_key_id(change_derivation).unwrap(); @@ -235,12 +233,12 @@ fn inputs_and_change( is_coinbase: false, }); - (change_key, change_derivation) + change_key })?; parts.push(build::output(change, change_key.clone())); - Ok((parts, change_key, change_derivation, change)) + Ok((parts, change_key)) } #[cfg(test)] diff --git a/wallet/src/types.rs b/wallet/src/types.rs index 740af35a9..d2bad5b2c 100644 --- a/wallet/src/types.rs +++ b/wallet/src/types.rs @@ -488,6 +488,11 @@ impl WalletData { self.outputs.insert(out.key_id.to_hex(), out.clone()); } + // TODO - careful with this, only for Unconfirmed (maybe Locked)? + pub fn delete_output(&mut self, id: &keychain::Identifier) { + self.outputs.remove(&id.to_hex()); + } + /// Lock an output data. /// TODO - we should track identifier on these outputs (not just n_child) pub fn lock_output(&mut self, out: &OutputData) {