From ddd9bc2908c2ef65d74da53f2d0d7b0da63a366a Mon Sep 17 00:00:00 2001 From: Yeastplume Date: Thu, 7 Mar 2019 16:19:47 +0000 Subject: [PATCH] 'Awaiting finalization' status and changes to 'wallet check' (#8) * add awaiting confirmation status and change check to not remove unconfirmed transactions by default * test fixes --- apiwallet/src/api.rs | 4 +- libwallet/src/internal/restore.rs | 64 ++++++++++++++++--------------- libwallet/src/internal/updater.rs | 6 ++- libwallet/src/types.rs | 4 +- refwallet/src/command.rs | 8 +++- refwallet/src/display.rs | 6 ++- refwallet/src/lmdb_wallet.rs | 4 +- refwallet/tests/check.rs | 14 +++---- refwallet/tests/transaction.rs | 5 ++- src/bin/cmd/wallet_args.rs | 12 +++++- src/bin/cmd/wallet_tests.rs | 2 +- src/bin/grin-wallet.yml | 6 +++ 12 files changed, 84 insertions(+), 51 deletions(-) diff --git a/apiwallet/src/api.rs b/apiwallet/src/api.rs index a0296aebe..a9141683c 100644 --- a/apiwallet/src/api.rs +++ b/apiwallet/src/api.rs @@ -842,11 +842,11 @@ where } /// Attempt to check and fix the contents of the wallet - pub fn check_repair(&mut self) -> Result<(), Error> { + pub fn check_repair(&mut self, delete_unconfirmed: bool) -> Result<(), Error> { let mut w = self.wallet.lock(); w.open_with_credentials()?; self.update_outputs(&mut w, true); - w.check_repair()?; + w.check_repair(delete_unconfirmed)?; w.close()?; Ok(()) } diff --git a/libwallet/src/internal/restore.rs b/libwallet/src/internal/restore.rs index 8c7a84c88..a07dde7fc 100644 --- a/libwallet/src/internal/restore.rs +++ b/libwallet/src/internal/restore.rs @@ -267,7 +267,7 @@ where /// Check / repair wallet contents /// assume wallet contents have been freshly updated with contents /// of latest block -pub fn check_repair(wallet: &mut T) -> Result<(), Error> +pub fn check_repair(wallet: &mut T, delete_unconfirmed: bool) -> Result<(), Error> where T: WalletBackend, C: NodeClient, @@ -335,37 +335,39 @@ where restore_missing_output(wallet, m, &mut found_parents, &mut None)?; } - // Unlock locked outputs - for m in locked_outs.into_iter() { - let mut o = m.0; - warn!( - "Confirmed output for {} with ID {} ({:?}) exists in UTXO set and is locked. \ - Unlocking and cancelling associated transaction log entries.", - o.value, o.key_id, m.1.commit, - ); - o.status = OutputStatus::Unspent; - cancel_tx_log_entry(wallet, &o)?; - let mut batch = wallet.batch()?; - batch.save(o)?; - batch.commit()?; - } + if delete_unconfirmed { + // Unlock locked outputs + for m in locked_outs.into_iter() { + let mut o = m.0; + warn!( + "Confirmed output for {} with ID {} ({:?}) exists in UTXO set and is locked. \ + Unlocking and cancelling associated transaction log entries.", + o.value, o.key_id, m.1.commit, + ); + o.status = OutputStatus::Unspent; + cancel_tx_log_entry(wallet, &o)?; + let mut batch = wallet.batch()?; + batch.save(o)?; + batch.commit()?; + } - let unconfirmed_outs: Vec<&(OutputData, pedersen::Commitment)> = wallet_outputs - .iter() - .filter(|o| o.0.status == OutputStatus::Unconfirmed) - .collect(); - // Delete unconfirmed outputs - for m in unconfirmed_outs.into_iter() { - let o = m.0.clone(); - warn!( - "Unconfirmed output for {} with ID {} ({:?}) not in UTXO set. \ - Deleting and cancelling associated transaction log entries.", - o.value, o.key_id, m.1, - ); - cancel_tx_log_entry(wallet, &o)?; - let mut batch = wallet.batch()?; - batch.delete(&o.key_id, &o.mmr_index)?; - batch.commit()?; + let unconfirmed_outs: Vec<&(OutputData, pedersen::Commitment)> = wallet_outputs + .iter() + .filter(|o| o.0.status == OutputStatus::Unconfirmed) + .collect(); + // Delete unconfirmed outputs + for m in unconfirmed_outs.into_iter() { + let o = m.0.clone(); + warn!( + "Unconfirmed output for {} with ID {} ({:?}) not in UTXO set. \ + Deleting and cancelling associated transaction log entries.", + o.value, o.key_id, m.1, + ); + cancel_tx_log_entry(wallet, &o)?; + let mut batch = wallet.batch()?; + batch.delete(&o.key_id, &o.mmr_index)?; + batch.commit()?; + } } // restore labels, account paths and child derivation indices diff --git a/libwallet/src/internal/updater.rs b/libwallet/src/internal/updater.rs index 2c993b58d..f31767009 100644 --- a/libwallet/src/internal/updater.rs +++ b/libwallet/src/internal/updater.rs @@ -384,6 +384,7 @@ where let mut unspent_total = 0; let mut immature_total = 0; + let mut awaiting_finalization_total = 0; let mut unconfirmed_total = 0; let mut locked_total = 0; @@ -403,9 +404,9 @@ where // We ignore unconfirmed coinbase outputs completely. if !out.is_coinbase { if minimum_confirmations == 0 { - unspent_total += out.value; - } else { unconfirmed_total += out.value; + } else { + awaiting_finalization_total += out.value; } } } @@ -420,6 +421,7 @@ where last_confirmed_height: current_height, minimum_confirmations, total: unspent_total + unconfirmed_total + immature_total, + amount_awaiting_finalization: awaiting_finalization_total, amount_awaiting_confirmation: unconfirmed_total, amount_immature: immature_total, amount_locked: locked_total, diff --git a/libwallet/src/types.rs b/libwallet/src/types.rs index 11416d4f8..7a7e85191 100644 --- a/libwallet/src/types.rs +++ b/libwallet/src/types.rs @@ -130,7 +130,7 @@ where fn restore(&mut self) -> Result<(), Error>; /// Attempt to check and fix wallet state - fn check_repair(&mut self) -> Result<(), Error>; + fn check_repair(&mut self, delete_unconfirmed: bool) -> Result<(), Error>; } /// Batch trait to update the output data backend atomically. Trying to use a @@ -546,6 +546,8 @@ pub struct WalletInfo { pub minimum_confirmations: u64, /// total amount in the wallet pub total: u64, + /// amount awaiting finalization + pub amount_awaiting_finalization: u64, /// amount awaiting confirmation pub amount_awaiting_confirmation: u64, /// coinbases waiting for lock height diff --git a/refwallet/src/command.rs b/refwallet/src/command.rs index 593a042e8..59d21b002 100644 --- a/refwallet/src/command.rs +++ b/refwallet/src/command.rs @@ -530,13 +530,19 @@ pub fn restore( Ok(()) } +/// wallet check +pub struct CheckArgs { + pub delete_unconfirmed: bool, +} + pub fn check_repair( wallet: Arc>>, + args: CheckArgs, ) -> Result<(), Error> { controller::owner_single_use(wallet.clone(), |api| { warn!("Starting wallet check...",); warn!("Updating all wallet outputs, please wait ...",); - let result = api.check_repair(); + let result = api.check_repair(args.delete_unconfirmed); match result { Ok(_) => { warn!("Wallet check complete",); diff --git a/refwallet/src/display.rs b/refwallet/src/display.rs index 041494fba..32a2395f1 100644 --- a/refwallet/src/display.rs +++ b/refwallet/src/display.rs @@ -270,7 +270,7 @@ pub fn info( if dark_background_color_scheme { table.add_row(row![ - bFG->"Total", + bFG->"Confirmed Total", FG->amount_to_hr_string(wallet_info.total, false) ]); // Only dispay "Immature Coinbase" if we have related outputs in the wallet. @@ -285,6 +285,10 @@ pub fn info( bFY->format!("Awaiting Confirmation (< {})", wallet_info.minimum_confirmations), FY->amount_to_hr_string(wallet_info.amount_awaiting_confirmation, false) ]); + table.add_row(row![ + bFB->format!("Awaiting Finalization"), + FB->amount_to_hr_string(wallet_info.amount_awaiting_finalization, false) + ]); table.add_row(row![ Fr->"Locked by previous transaction", Fr->amount_to_hr_string(wallet_info.amount_locked, false) diff --git a/refwallet/src/lmdb_wallet.rs b/refwallet/src/lmdb_wallet.rs index 905f6d184..3b1bb56d3 100644 --- a/refwallet/src/lmdb_wallet.rs +++ b/refwallet/src/lmdb_wallet.rs @@ -350,8 +350,8 @@ where Ok(()) } - fn check_repair(&mut self) -> Result<(), Error> { - internal::restore::check_repair(self).context(ErrorKind::Restore)?; + fn check_repair(&mut self, delete_unconfirmed: bool) -> Result<(), Error> { + internal::restore::check_repair(self, delete_unconfirmed).context(ErrorKind::Restore)?; Ok(()) } } diff --git a/refwallet/tests/check.rs b/refwallet/tests/check.rs index 0726d880e..b43b58379 100644 --- a/refwallet/tests/check.rs +++ b/refwallet/tests/check.rs @@ -140,7 +140,7 @@ fn check_repair_impl(test_dir: &str) -> Result<(), libwallet::Error> { // this should restore our missing outputs wallet::controller::owner_single_use(wallet1.clone(), |api| { - api.check_repair()?; + api.check_repair(true)?; Ok(()) })?; @@ -181,7 +181,7 @@ fn check_repair_impl(test_dir: &str) -> Result<(), libwallet::Error> { // unlock/restore wallet::controller::owner_single_use(wallet1.clone(), |api| { - api.check_repair()?; + api.check_repair(true)?; Ok(()) })?; @@ -327,7 +327,7 @@ fn two_wallets_one_seed_impl(test_dir: &str) -> Result<(), libwallet::Error> { // 0) Check repair when all is okay should leave wallet contents alone wallet::controller::owner_single_use(wallet1.clone(), |api| { - api.check_repair()?; + api.check_repair(true)?; let info = test_framework::wallet_info(api)?; assert_eq!(info.amount_currently_spendable, base_amount * 6); assert_eq!(info.total, base_amount * 6); @@ -381,7 +381,7 @@ fn two_wallets_one_seed_impl(test_dir: &str) -> Result<(), libwallet::Error> { // 2) check_repair should recover them into a single wallet wallet::controller::owner_single_use(wallet1.clone(), |api| { - api.check_repair()?; + api.check_repair(true)?; Ok(()) })?; @@ -447,7 +447,7 @@ fn two_wallets_one_seed_impl(test_dir: &str) -> Result<(), libwallet::Error> { })?; wallet::controller::owner_single_use(wallet6.clone(), |api| { - api.check_repair()?; + api.check_repair(true)?; Ok(()) })?; @@ -538,7 +538,7 @@ fn two_wallets_one_seed_impl(test_dir: &str) -> Result<(), libwallet::Error> { let outputs = api.retrieve_outputs(true, false, None)?.1; assert_eq!(outputs.len(), 3); assert_eq!(info.amount_currently_spendable, base_amount * 15); - api.check_repair()?; + api.check_repair(true)?; let info = test_framework::wallet_info(api)?; let outputs = api.retrieve_outputs(true, false, None)?.1; assert_eq!(outputs.len(), 6); @@ -556,7 +556,7 @@ fn two_wallets_one_seed_impl(test_dir: &str) -> Result<(), libwallet::Error> { // 7) Ensure check_repair creates missing accounts wallet::controller::owner_single_use(wallet10.clone(), |api| { - api.check_repair()?; + api.check_repair(true)?; api.set_active_account("account_1")?; let info = test_framework::wallet_info(api)?; let outputs = api.retrieve_outputs(true, false, None)?.1; diff --git a/refwallet/tests/transaction.rs b/refwallet/tests/transaction.rs index 159997aea..86d74ed66 100644 --- a/refwallet/tests/transaction.rs +++ b/refwallet/tests/transaction.rs @@ -420,11 +420,12 @@ fn tx_rollback(test_dir: &str) -> Result<(), libwallet::Error> { let (refreshed, wallet2_info) = api.retrieve_summary_info(true, 1)?; assert!(refreshed); assert_eq!(wallet2_info.amount_currently_spendable, 0,); - assert_eq!(wallet2_info.total, amount); + assert_eq!(wallet2_info.amount_awaiting_finalization, amount); Ok(()) })?; - // wallet 1 is bold and doesn't ever post the transaction mine a few more blocks + // wallet 1 is bold and doesn't ever post the transaction + // mine a few more blocks let _ = test_framework::award_blocks_to_wallet(&chain, wallet1.clone(), 5); // Wallet 1 decides to roll back instead diff --git a/src/bin/cmd/wallet_args.rs b/src/bin/cmd/wallet_args.rs index acda1f5e3..a6c7f0d45 100644 --- a/src/bin/cmd/wallet_args.rs +++ b/src/bin/cmd/wallet_args.rs @@ -452,6 +452,13 @@ pub fn parse_info_args(args: &ArgMatches) -> Result Result { + let delete_unconfirmed = args.is_present("delete_unconfirmed"); + Ok(command::CheckArgs { + delete_unconfirmed: delete_unconfirmed, + }) +} + pub fn parse_txs_args(args: &ArgMatches) -> Result { let tx_id = match args.value_of("id") { None => None, @@ -621,7 +628,10 @@ pub fn wallet_command( command::cancel(inst_wallet(), a) } ("restore", Some(_)) => command::restore(inst_wallet()), - ("check", Some(_)) => command::check_repair(inst_wallet()), + ("check", Some(args)) => { + let a = arg_parse!(parse_check_args(&args)); + command::check_repair(inst_wallet(), a) + } _ => { let msg = format!("Unknown wallet command, use 'grin help wallet' for details"); return Err(ErrorKind::ArgumentError(msg).into()); diff --git a/src/bin/cmd/wallet_tests.rs b/src/bin/cmd/wallet_tests.rs index 32814338a..8440690fa 100644 --- a/src/bin/cmd/wallet_tests.rs +++ b/src/bin/cmd/wallet_tests.rs @@ -456,7 +456,7 @@ mod wallet_tests { ]; execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; - let arg_vec = vec!["grin-wallet", "-p", "password", "check"]; + let arg_vec = vec!["grin-wallet", "-p", "password", "check", "-d"]; execute_command(&app, test_dir, "wallet1", &client1, arg_vec)?; // Another file exchange, cancel this time diff --git a/src/bin/grin-wallet.yml b/src/bin/grin-wallet.yml index 9b207e105..cb4b9f18d 100644 --- a/src/bin/grin-wallet.yml +++ b/src/bin/grin-wallet.yml @@ -236,3 +236,9 @@ subcommands: about: Restores a wallet contents from a seed file - check: about: Checks a wallet's outputs against a live node, repairing and restoring missing outputs if required + args: + - delete_unconfirmed: + help: Delete any unconfirmed outputsm unlock any locked outputs and delete associated transactions while doing the check. + short: d + long: delete_unconfirmed + takes_value: false