diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 01a106c51..ca0aa2845 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -306,9 +306,9 @@ pub(crate) fn send_multi_step_proposed_transfer() { use rand_core::OsRng; use zcash_client_backend::{ - data_api::TransactionDataRequest, + data_api::{TransactionDataRequest, TransactionStatus}, fees::ChangeValue, - wallet::{TransparentAddressMetadata, WalletTx}, + wallet::TransparentAddressMetadata, }; use zcash_primitives::{ legacy::keys::{NonHardenedChildIndex, TransparentKeyScope}, @@ -440,9 +440,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { // Verify that a status request has been generated for the second transaction of // the ZIP 320 pair. let tx_data_requests = st.wallet().transaction_data_requests().unwrap(); - assert!(tx_data_requests - .iter() - .any(|r| r == &TransactionDataRequest::GetStatus(*txids.last()))); + assert!(tx_data_requests.contains(&TransactionDataRequest::GetStatus(*txids.last()))); assert!(expected_step0_change < expected_ephemeral); assert_eq!(confirmed_sent.len(), 2); @@ -611,9 +609,7 @@ pub(crate) fn send_multi_step_proposed_transfer() { // Verify that storing the fully transparent transaction causes a transaction // status request to be generated. let tx_data_requests = st.wallet().transaction_data_requests().unwrap(); - assert!(tx_data_requests - .iter() - .any(|r| r == &TransactionDataRequest::GetStatus(txid))); + assert!(tx_data_requests.contains(&TransactionDataRequest::GetStatus(txid))); // We call get_wallet_transparent_output with `allow_unspendable = true` to verify // storage because the decrypted transaction has not yet been mined. @@ -687,40 +683,25 @@ pub(crate) fn send_multi_step_proposed_transfer() { let (h, _) = st.generate_next_block_from_tx(tx_index, tx); st.scan_cached_blocks(h, 1); - // The rest of this test would currently fail without the explicit call to - // `put_tx_meta` below. Ideally the above `scan_cached_blocks` would be - // sufficient, but it does not detect the transaction as interesting to the + // The above `scan_cached_blocks` does not detect `tx` as interesting to the // wallet. If a transaction is in the database with a null `mined_height`, // as in this case, its `mined_height` will remain null unless `put_tx_meta` - // is called on it. Normally `put_tx_meta` would be called via `put_blocks` - // as a result of scanning, but that won't happen for any fully transparent - // transaction, and currently it also will not happen for a partially shielded - // transaction unless it is interesting to the wallet for another reason. - // Therefore we will not currently detect either collisions with uses of - // ephemeral outputs by other wallets, or refunds of funds sent to TEX - // addresses. (#1354, #1379) - - // Check that what we say in the above paragraph remains true, so that we - // don't accidentally fix it without updating this test. + // is called on it. This happens either via `put_blocks` as a result of + // scanning, or via `set_transaction_status` in response to processing the + // `transaction_data_requests` queue. For a fully transparent transaction, + // the latter is required. + + // The reservation should fail because `tx` is not yet seen as mined. reservation_should_fail(&mut st, 1, 22); - // For now, we demonstrate that this problem is the only obstacle to the rest - // of the ZIP 320 code doing the right thing, by manually calling `put_tx_meta`: - crate::wallet::put_tx_meta( - &st.wallet_mut().conn, - &WalletTx::new( - tx.txid(), - tx_index, - vec![], - vec![], - #[cfg(feature = "orchard")] - vec![], - #[cfg(feature = "orchard")] - vec![], - ), - h, - ) - .unwrap(); + // Simulate the wallet processing the `transaction_data_requests` queue. + let tx_data_requests = st.wallet().transaction_data_requests().unwrap(); + assert!(tx_data_requests.contains(&TransactionDataRequest::GetStatus(tx.txid()))); + + // Respond to the GetStatus request. + st.wallet_mut() + .set_transaction_status(tx.txid(), TransactionStatus::Mined(h)) + .unwrap(); // We already reserved 22 addresses, so mining the transaction with the // ephemeral output at index 10 should allow 9 more (..31). diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index aa92b7466..cbe1011ff 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -1994,12 +1994,13 @@ pub(crate) fn set_transaction_status( txid: TxId, status: TransactionStatus, ) -> Result<(), SqliteClientError> { - // In order to have made a Status request, we must already have had the - // raw transaction (the only callers of `queue_tx_retrieval` are in contexts - // where we must have already called `put_tx_data`). Therefore, it is safe - // to unconditionally delete the request from `tx_retrieval_queue` below - // (both in the expired case and the case where it has been mined), because - // we already have all the data we need about this transaction. + // In order to have made a Status request, we must already have had the raw + // transaction (the only callers of `queue_tx_retrieval` with query type + // GetStatus are in contexts where we must have already called `put_tx_data`). + // Therefore, it is safe to unconditionally delete the request from + // `tx_retrieval_queue` below (both in the expired case and the case where + // it has been mined), because we already have all the data we need about + // this transaction. match status { TransactionStatus::TxidNotRecognized | TransactionStatus::NotInMainChain => { diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 88e6c9352..96d4d6be4 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -496,7 +496,7 @@ pub(crate) fn transaction_data_requests( ) -> Result, SqliteClientError> { // We cannot construct address-based transaction data requests for the case where we cannot // determine the height at which to begin, so we require that either the target height or mined - // height be set. + // height be set. let mut address_request_stmt = conn.prepare_cached( "SELECT ssq.address, IFNULL(t.target_height, t.mined_height) FROM transparent_spend_search_queue ssq