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

@@ -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",

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()?;
 

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),

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()))?;
 	}

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<T: ?Sized, C, K>(
 	wallet: &mut T,
 	tx_id: Option<u32>,
 	tx_slate_id: Option<Uuid>,
-	parent_key_id: &Identifier,
+	parent_key_id: Option<&Identifier>,
 ) -> Result<Vec<TxLogEntry>, Error>
 where
 	T: WalletBackend<C, K>,
@@ -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::<Vec<_>>()
+		wallet.tx_log_iter().collect::<Vec<_>>()
 	};
+	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<T: ?Sized, C, K>(

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();

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(())