Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[zcash_client_sqlite] Replace the put_tx_meta workaround in the test send_multi_step_proposed_transfer #1485

Closed
daira opened this issue Aug 9, 2024 · 0 comments
Labels
A-wallet Area: light wallet backend. C-tech-debt Category: Technical debt that needs to be paid off testing

Comments

@daira
Copy link
Contributor

daira commented Aug 9, 2024

Currently the zcash_client_sqlite::testing::pool::send_multi_step_proposed_transfer test code contains an unrealistic call to zcash_client_sqlite::wallet::put_tx_meta, to ensure that the mined_height database field of the second transaction in a ZIP 320 pair is updated when it is mined in the test.

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

put_tx_meta is an internal method of the zcash_client_sqlite backend, and would not normally be called directly by a wallet implementation. Calling it here was a workaround allowing the intended behaviour of the ZIP 320 implementation in #1257 to be tested, despite the fact that (as of that PR) the second transaction would not be detected as interesting to the wallet in some cases.

The underlying problem that required this workaround should have been fixed in #1473, which (among other things) added a queue to request verification of mined status. However, as @nuttycom said in #1473 (comment):

At present, set_transaction_status is not called by any path in existing tests; it is explicitly introduced as a method for the wallet app to invoke once it has satisfied a transaction data request. It may be possible to update the test to simulate that interaction.

For this issue, replace the call to put_tx_meta with a more realistic simulated interaction with a wallet, if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-wallet Area: light wallet backend. C-tech-debt Category: Technical debt that needs to be paid off testing
Projects
None yet
Development

No branches or pull requests

1 participant