From fb11dd3f4e6e8ea1a6f41a423cf9208ab18dca60 Mon Sep 17 00:00:00 2001 From: Yeastplume Date: Tue, 11 Dec 2018 17:18:31 +0000 Subject: [PATCH] Don't allow same transaction to be received multiple times (#2124) * don't allow same transaction to be received multiple times * issues in other tests uncovered and fixed * allow tx_hex update function to operate without an account being specified, to allow finalize_tx function to work without an account being specified * rustfmt * fix to retrieve_txs * rustfmt --- src/bin/cmd/wallet_tests.rs | 5 ++++- wallet/src/libwallet/api.rs | 7 ++++++- wallet/src/libwallet/error.rs | 4 ++++ wallet/src/libwallet/internal/tx.rs | 8 +++++--- wallet/src/libwallet/internal/updater.rs | 20 ++++++++++++-------- wallet/tests/repost.rs | 5 +++-- wallet/tests/self_send.rs | 2 +- 7 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/bin/cmd/wallet_tests.rs b/src/bin/cmd/wallet_tests.rs index 122df365..386299a6 100644 --- a/src/bin/cmd/wallet_tests.rs +++ b/src/bin/cmd/wallet_tests.rs @@ -293,7 +293,10 @@ mod wallet_tests { "-g", "Thanks, Yeast!", ]; - execute_command(&app, test_dir, "wallet2", &client2, arg_vec)?; + execute_command(&app, test_dir, "wallet2", &client2, arg_vec.clone())?; + + // shouldn't be allowed to receive twice + assert!(execute_command(&app, test_dir, "wallet2", &client2, arg_vec).is_err()); let arg_vec = vec![ "grin", diff --git a/wallet/src/libwallet/api.rs b/wallet/src/libwallet/api.rs index 842a7710..459d9407 100644 --- a/wallet/src/libwallet/api.rs +++ b/wallet/src/libwallet/api.rs @@ -425,7 +425,7 @@ where let res = Ok(( validated, - updater::retrieve_txs(&mut *w, tx_id, tx_slate_id, &parent_key_id)?, + updater::retrieve_txs(&mut *w, tx_id, tx_slate_id, Some(&parent_key_id))?, )); w.close()?; @@ -832,6 +832,11 @@ where } None => w.parent_key_id(), }; + // Don't do this multiple times + let tx = updater::retrieve_txs(&mut *w, None, Some(slate.id), Some(&parent_key_id))?; + if tx.len() > 0 { + return Err(ErrorKind::TransactionAlreadyReceived(slate.id.to_string()).into()); + } let res = tx::receive_tx(&mut *w, slate, &parent_key_id, false, message); w.close()?; diff --git a/wallet/src/libwallet/error.rs b/wallet/src/libwallet/error.rs index af3b6d5f..d92de64e 100644 --- a/wallet/src/libwallet/error.rs +++ b/wallet/src/libwallet/error.rs @@ -167,6 +167,10 @@ pub enum ErrorKind { #[fail(display = "Transaction already confirmed error")] TransactionAlreadyConfirmed, + /// Transaction has already been received + #[fail(display = "Transaction {} has already been received", _0)] + TransactionAlreadyReceived(String), + /// Attempt to repost a transaction that's not completed and stored #[fail(display = "Transaction building not completed: {}", _0)] TransactionBuildingNotCompleted(u32), diff --git a/wallet/src/libwallet/internal/tx.rs b/wallet/src/libwallet/internal/tx.rs index c00c914e..8529d3f6 100644 --- a/wallet/src/libwallet/internal/tx.rs +++ b/wallet/src/libwallet/internal/tx.rs @@ -169,7 +169,7 @@ where } else if let Some(tx_slate_id) = tx_slate_id { tx_id_string = tx_slate_id.to_string(); } - let tx_vec = updater::retrieve_txs(wallet, tx_id, tx_slate_id, &parent_key_id)?; + let tx_vec = updater::retrieve_txs(wallet, tx_id, tx_slate_id, Some(&parent_key_id))?; if tx_vec.len() != 1 { return Err(ErrorKind::TransactionDoesntExist(tx_id_string))?; } @@ -199,7 +199,7 @@ where C: NodeClient, K: Keychain, { - let tx_vec = updater::retrieve_txs(wallet, Some(tx_id), None, parent_key_id)?; + let tx_vec = updater::retrieve_txs(wallet, Some(tx_id), None, Some(parent_key_id))?; if tx_vec.len() != 1 { return Err(ErrorKind::TransactionDoesntExist(tx_id.to_string()))?; } @@ -219,7 +219,9 @@ where K: Keychain, { let tx_hex = util::to_hex(ser::ser_vec(&slate.tx).unwrap()); - let tx_vec = updater::retrieve_txs(wallet, None, Some(slate.id), parent_key_id)?; + // This will ignore the parent key, so no need to specify account on the + // finalise command + let tx_vec = updater::retrieve_txs(wallet, None, Some(slate.id), None)?; if tx_vec.len() != 1 { return Err(ErrorKind::TransactionDoesntExist(slate.id.to_string()))?; } diff --git a/wallet/src/libwallet/internal/updater.rs b/wallet/src/libwallet/internal/updater.rs index 76821c3c..6c425e15 100644 --- a/wallet/src/libwallet/internal/updater.rs +++ b/wallet/src/libwallet/internal/updater.rs @@ -80,11 +80,12 @@ where } /// Retrieve all of the transaction entries, or a particular entry +/// if `parent_key_id` is set, only return entries from that key pub fn retrieve_txs( wallet: &mut T, tx_id: Option, tx_slate_id: Option, - parent_key_id: &Identifier, + parent_key_id: Option<&Identifier>, ) -> Result, Error> where T: WalletBackend, @@ -93,9 +94,7 @@ where { // just read the wallet here, no need for a write lock let mut txs = if let Some(id) = tx_id { - let tx = wallet - .tx_log_iter() - .find(|t| t.id == id && t.parent_key_id == *parent_key_id); + let tx = wallet.tx_log_iter().find(|t| t.id == id); if let Some(t) = tx { vec![t] } else { @@ -109,14 +108,19 @@ where vec![] } } else { - wallet - .tx_log_iter() - .filter(|t| t.parent_key_id == *parent_key_id) - .collect::>() + wallet.tx_log_iter().collect::>() }; + if let Some(k) = parent_key_id { + txs = txs + .iter() + .filter(|t| t.parent_key_id == *k) + .map(|t| t.clone()) + .collect(); + } txs.sort_by_key(|tx| tx.creation_ts); Ok(txs) } + /// Refreshes the outputs in a wallet with the latest information /// from a node pub fn refresh_outputs( diff --git a/wallet/tests/repost.rs b/wallet/tests/repost.rs index c449e39c..69f0820f 100644 --- a/wallet/tests/repost.rs +++ b/wallet/tests/repost.rs @@ -89,6 +89,8 @@ fn file_repost_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { let send_file = format!("{}/part_tx_1.tx", test_dir); let receive_file = format!("{}/part_tx_2.tx", test_dir); + let mut slate = Slate::blank(2); + // Should have 5 in account1 (5 spendable), 5 in account (2 spendable) wallet::controller::owner_single_use(wallet1.clone(), |api| { let (wallet1_refreshed, wallet1_info) = api.retrieve_summary_info(true, 1)?; @@ -123,7 +125,7 @@ fn file_repost_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { wallet::controller::foreign_single_use(wallet1.clone(), |api| { let adapter = FileWalletCommAdapter::new(); - let mut slate = adapter.receive_tx_async(&send_file)?; + slate = adapter.receive_tx_async(&send_file)?; api.receive_tx(&mut slate, None, None)?; adapter.send_tx_async(&receive_file, &mut slate)?; Ok(()) @@ -135,7 +137,6 @@ fn file_repost_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { w.set_parent_key_id_by_name("mining")?; } - let mut slate = Slate::blank(2); // wallet 1 finalize wallet::controller::owner_single_use(wallet1.clone(), |api| { let adapter = FileWalletCommAdapter::new(); diff --git a/wallet/tests/self_send.rs b/wallet/tests/self_send.rs index 09c0ca22..59379436 100644 --- a/wallet/tests/self_send.rs +++ b/wallet/tests/self_send.rs @@ -92,13 +92,13 @@ fn self_send_test_impl(test_dir: &str) -> Result<(), libwallet::Error> { true, // select all outputs None, )?; + api.tx_lock_outputs(&slate, lock_fn)?; // Send directly to self wallet::controller::foreign_single_use(wallet1.clone(), |api| { api.receive_tx(&mut slate, Some("listener"), None)?; Ok(()) })?; api.finalize_tx(&mut slate)?; - api.tx_lock_outputs(&slate, lock_fn)?; api.post_tx(&slate.tx, false)?; // mines a block bh += 1; Ok(())