Skip to content

Commit

Permalink
Replace the put_tx_meta workaround in `send_multi_step_proposed_tra…
Browse files Browse the repository at this point in the history
…nsfer`.

fixes #1485

Signed-off-by: Daira-Emma Hopwood <[email protected]>
  • Loading branch information
daira committed Aug 9, 2024
1 parent 56e4233 commit 1023ce7
Showing 1 changed file with 17 additions and 32 deletions.
49 changes: 17 additions & 32 deletions zcash_client_sqlite/src/testing/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,9 @@ pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {

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},
Expand Down Expand Up @@ -683,40 +683,25 @@ pub(crate) fn send_multi_step_proposed_transfer<T: ShieldedPoolTester>() {
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

This comment has been minimized.

Copy link
@daira

daira Aug 10, 2024

Author Contributor

Sorry, this is not quite right. Should be:

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 either put_tx_meta or set_transaction_status is called on it. The former is normally called internally via put_blocks as a result of scanning, but not for the case of a fully transparent transaction. The latter is called by the wallet implementation in response to processing the transaction_data_requests queue.

This comment has been minimized.

Copy link
@daira

daira Aug 10, 2024

Author Contributor

Filed as #1487

// 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).
Expand Down

0 comments on commit 1023ce7

Please sign in to comment.