From abaff6dbc928e5c51b3ff055298a079d77b08e39 Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Fri, 10 Apr 2026 01:04:10 -0600 Subject: [PATCH] External-only scanning: activate, tests, and changelogs Switch scan_cached_blocks to External-only IVKs, halving key-agreement work. Change notes are recovered via enhancement. Includes conditional nullifier pruning in scan_cached_blocks and comprehensive test rewrites. Co-Authored-By: Claude --- zcash_client_backend/CHANGELOG.md | 72 ++ zcash_client_backend/src/data_api/chain.rs | 8 +- zcash_client_backend/src/data_api/testing.rs | 117 +++ .../src/data_api/testing/pool.rs | 895 ++++++++++++++++-- zcash_client_sqlite/CHANGELOG.md | 36 + zcash_client_sqlite/src/testing/pool.rs | 80 ++ zcash_client_sqlite/src/wallet/orchard.rs | 18 + zcash_client_sqlite/src/wallet/sapling.rs | 18 + 8 files changed, 1153 insertions(+), 91 deletions(-) diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 37f18d6b9c..870ba9b471 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -23,6 +23,34 @@ workspace. allowing callers to control which transparent outputs are eligible for selection (e.g., coinbase-only filtering). - `wallet::ConfirmationsPolicyError` + - `WalletWrite::notify_wallet_note_positions` with a default no-op + implementation. Backends should override this to mark note commitment tree + positions for notes discovered during transaction enhancement, so that those + notes become spendable. + - `WalletWrite::prune_tracked_nullifiers` with a default no-op implementation. + Backends that record an internal nullifier-to-locator map during scanning + should override this to prune entries below the pruning horizon. Callers + must only invoke it after the `TransactionDataRequest` queue has been + drained for the range being pruned. +- `zcash_client_backend::decrypt`: + - `DecryptedOutput::nullifier_bytes` + - `DecryptedOutput::note_commitment_tree_position` + - `DecryptedOutput::with_spend_metadata` + - `TxBundlePositions` struct describing the per-pool base positions of a + transaction's shielded bundles within the global note commitment trees. + - `compute_enriched_outputs` function that re-maps a `DecryptedTransaction` + with per-output nullifier bytes and commitment-tree positions derived from + a `TxBundlePositions`. Used by the enhancement path to make late-discovered + change notes spendable. + - `collect_wallet_note_positions` function (gated on `sync` or + `test-dependencies`) that extracts the non-outgoing note positions from a + `DecryptedTransaction` for passing to + `WalletWrite::notify_wallet_note_positions`. +- `zcash_client_backend::scanning::ScanningKeys::from_account_ufvks_with_scopes` +- The `sync` module now services queued transaction-data requests + (enhancement, status retrieval, and transparent address history) so that + compact scanning, transaction enhancement, and transparent history discovery + converge to a complete wallet view during recovery. - `zcash_client_backend::proto::CompactFormatError` - `zcash_client_backend::proto::compact_formats`: - `CompactTx` has added fields `vin` and `vout` @@ -68,6 +96,14 @@ workspace. having no economic value in `zcash_client_sqlite`. - `chain::scan_cached_blocks` now requires `DbT::AccountId: Sync` (in addition to its existing `Send + 'static` bounds). + - `chain::scan_cached_blocks` now uses External-scope IVKs only for batch trial + decryption, halving key-agreement work per output. Change notes (Internal IVK) + are recovered via the enhancement phase: when a note's nullifier matches one + already in the nullifier map, the spending transaction is queued for + enhancement, where `decrypt_and_store_transaction` tries all key scopes. + Callers that use `scan_cached_blocks` directly (outside the `sync` module) + must ensure that enhancement requests are serviced for change notes to be + discovered. - `error::Error::MemoForbidden` has been replaced by `Error::Payment(zip321::PaymentError)`, which can represent both memo-to-transparent and zero-valued-transparent-output errors. @@ -92,6 +128,15 @@ workspace. version. - `input_selection::InputSelector::propose_transaction` trait method. - Trait `Account` has added method `birthday_height` + - `ll::ReceivedShieldedOutput::nullifier` now returns `Option` + (by value) instead of `Option<&Self::Nullifier>`. + - `wallet::decrypt_and_store_transaction` now enriches the decrypted transaction + via `decrypt::compute_enriched_outputs` before handing it to + `WalletWrite::store_decrypted_tx`. This populates Orchard nullifier bytes on + non-outgoing outputs. Sapling nullifier bytes remain unset when called through + this entry point because computing them requires the bundle's base position in + the global note commitment tree; callers that need Sapling nullifiers should go + through the `sync` module's enhancement pipeline instead. - `zcash_client_backend::data_api::wallet::ConfirmationsPolicy::new` now returns `Result` instead of `Result`. - `zcash_client_backend::fees`: @@ -127,6 +172,14 @@ workspace. - `zcash_client_backend::sync`: - `run` now requires `DbT::AccountId: Sync` (in addition to its existing `Send + 'static` bounds). + - `run` now treats a stabilized but non-empty transaction-data request queue + as a terminal state once scanning is idle, preventing an infinite outer-loop + spin when lightwalletd cannot resolve a queued txid. + - `prune_tracked_nullifiers` is now called exclusively from `run()`, after the + post-`running()` drain, and only when that drain returns + `ServiceOutcome::Drained`. + - `TransactionDataRequest::TransactionsInvolvingAddress` handling now always + calls `notify_address_checked` after a successful address-history lookup. - `zcash_client_backend::wallet`: - `OvkPolicy` has been substantially modified to reflect the view that a single outgoing viewing key should be uniformly applied to encrypt all @@ -169,6 +222,25 @@ workspace. - `zcash_client_backend::data_api::testing::transparent::GapLimits` (use `zcash_keys::keys::transparent::GapLimits` instead). +### Fixed +- `zcash_client_memory`: `TransferType::Incoming` shielded outputs handed to + `WalletWrite::store_decrypted_tx` are no longer misclassified as change + notes in the internal scope. Previously the memory backend wrapped them + in `Recipient::InternalAccount` and routed them through + `ReceivedNote::from_sent_tx_output`, which hardcoded `is_change = true` + and `recipient_key_scope = Some(Scope::Internal)`. Two new constructors + (`ReceivedNote::from_decrypted_sapling_output` and + `from_decrypted_orchard_output`) derive `is_change` and + `recipient_key_scope` from the output's `TransferType`, so external + incoming receipts are now stored correctly. +- `zcash_client_memory`: `GetStatus` retrieval requests queued during + `store_decrypted_tx` for unmined transparent-bundle transactions are no + longer wiped by the end-of-function cleanup. The cleanup helper + `TransactionDataRequestQueue::remove_entries_for_txid` was renamed to + `remove_enhancement_entries_for_txid` and narrowed to only strip + `Enhancement` entries, leaving `GetStatus` work intact for the sync + orchestrator. + ## [0.21.2] - 2026-03-10 - The following APIs no longer crash in certain regtest mode configurations with fewer NUs active: diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index 9744c7117d..19fad254d9 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/zcash_client_backend/src/data_api/chain.rs @@ -603,7 +603,13 @@ where let account_ufvks = data_db .get_unified_full_viewing_keys() .map_err(Error::Wallet)?; - let scanning_keys = ScanningKeys::from_account_ufvks(account_ufvks); + // Use External IVK only for batch trial decryption. This halves the + // key-agreement work per output. Change notes (Internal IVK) are recovered + // via the nullifier_map: when a newly discovered note's nullifier matches + // one from a previously-scanned block, the spending transaction is queued + // for enhancement, where decrypt_and_store_transaction tries all key scopes. + let scanning_keys = + ScanningKeys::from_account_ufvks_with_scopes(account_ufvks, &[zip32::Scope::External]); let mut runners = BatchRunners::<_, (), ()>::for_keys(100, &scanning_keys); block_source.with_blocks::<_, DbT::Error>(Some(from_height), Some(limit), |block| { diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index 0f6674c1af..bfc3123763 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -885,6 +885,123 @@ where } } +impl TestState +where + Cache: TestCache, + ParamsT: consensus::Parameters + Send + 'static, + DbT: InputSource + WalletTest + WalletWrite + WalletCommitmentTrees, + ::AccountId: + std::fmt::Debug + ConditionallySelectable + Default + Send + 'static, +{ + /// Decrypts, enriches, and stores a transaction, mirroring the enhancement phase + /// of the sync loop. Outputs are enriched with note commitment tree positions and + /// nullifiers via the production [`compute_enriched_outputs`] function, and the + /// tree is updated to maintain witnesses for the new positions. + /// + /// [`compute_enriched_outputs`]: crate::decrypt::compute_enriched_outputs + pub fn enhance_transaction(&mut self, tx: &Transaction, mined_height: Option) { + use crate::decrypt::{ + TxBundlePositions, collect_wallet_note_positions, compute_enriched_outputs, + decrypt_transaction, + }; + + let ufvks = self.wallet_data.get_unified_full_viewing_keys().unwrap(); + let chain_tip_height = self.wallet_data.chain_height().unwrap(); + let height = mined_height.or_else(|| self.wallet_data.get_tx_height(tx.txid()).unwrap()); + let d_tx = decrypt_transaction(&self.network, height, chain_tip_height, tx, &ufvks); + + // Derive the per-pool bundle base positions, mirroring the production + // `fetch_tx_bundle_positions` logic: start from the previous block's tree + // end-size, then add outputs from any preceding transactions in the same + // block. If `height` falls outside what we've cached (e.g. a non-contiguous + // scan), we leave `positions` as `None` and let `compute_enriched_outputs` + // degrade to position-less enrichment, matching production behavior. + let txid = tx.txid(); + let positions = height.and_then(|h| { + if h == BlockHeight::from(0) { + Some(TxBundlePositions { + sapling_base: Some(0), + #[cfg(feature = "orchard")] + orchard_base: Some(0), + }) + } else { + let prev = self.cached_blocks.get(&(h - 1))?; + let mut sapling_base = prev.sapling_end_size() as u64; + #[cfg(feature = "orchard")] + let mut orchard_base = prev.orchard_end_size() as u64; + + // Read the compact block from the cache to find the tx's + // position and sum shielded outputs from preceding txs. + use crate::data_api::chain::BlockSource; + self.cache + .block_source() + .with_blocks::<_, Infallible>(Some(h), Some(1), |block| { + for vtx in &block.vtx { + if vtx.txid() == txid { + break; + } + sapling_base += vtx.outputs.len() as u64; + #[cfg(feature = "orchard")] + { + orchard_base += vtx.actions.len() as u64; + } + } + Ok(()) + }) + .ok(); + + Some(TxBundlePositions { + sapling_base: Some(sapling_base), + #[cfg(feature = "orchard")] + orchard_base: Some(orchard_base), + }) + } + }); + + let enriched = compute_enriched_outputs(tx, &d_tx, positions.as_ref(), &ufvks); + let wallet_note_positions = collect_wallet_note_positions(&enriched); + self.wallet_data.store_decrypted_tx(enriched).unwrap(); + + if let Some(h) = height { + if !wallet_note_positions.is_empty() { + self.wallet_data + .notify_wallet_note_positions(h..h + 1, &wallet_note_positions) + .unwrap(); + } + } + } + + /// Services pending enhancement requests using [`enhance_transaction`](Self::enhance_transaction). + pub fn service_enhancement_requests(&mut self) { + loop { + let requests = self.wallet_data.transaction_data_requests().unwrap(); + let enhancement_txids: Vec = requests + .into_iter() + .filter_map(|req| match req { + TransactionDataRequest::Enhancement(txid) => Some(txid), + _ => None, + }) + .collect(); + + if enhancement_txids.is_empty() { + break; + } + + let mut any_enhanced = false; + for txid in enhancement_txids { + if let Some(tx) = self.wallet_data.get_transaction(txid).unwrap() { + self.enhance_transaction(&tx, None); + any_enhanced = true; + } + } + + if !any_enhanced { + break; + } + } + } +} + impl TestState where ParamsT: consensus::Parameters + Send + 'static, diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 1f93bd313e..d5a871fffa 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -27,7 +27,6 @@ use zcash_protocol::{ memo::{Memo, MemoBytes}, value::Zatoshis, }; -use zip32::Scope; use zip321::{Payment, TransactionRequest}; use crate::{ @@ -89,7 +88,7 @@ use { }; pub mod dsl; -use dsl::{TestDsl, TestNoteConfig}; +use dsl::TestDsl; /// Trait that exposes the pool-specific types and operations necessary to run the /// single-shielded-pool tests on a given pool. @@ -375,33 +374,71 @@ pub fn zip_315_confirmations_test_steps( ) { let mut st = TestDsl::with_sapling_birthday_account(dsf, cache).build::(); let account = st.test_account().cloned().unwrap(); - let starting_balance = Zatoshis::const_from_u64(60_000); - // Add funds to the wallet in a single note, owned by the internal spending key, - // which will have one confirmation. let confirmations_policy = ConfirmationsPolicy::default(); - let (address_type, min_confirmations) = match input_trust { - InputTrust::Internal => (AddressType::Internal, confirmations_policy.trusted()), - InputTrust::ExternalUntrusted => ( - AddressType::DefaultExternal, - confirmations_policy.untrusted(), - ), - InputTrust::ExternalTrusted => { - (AddressType::DefaultExternal, confirmations_policy.trusted()) - } - }; - let min_confirmations = u32::from(min_confirmations); + let trusted = input_trust == InputTrust::ExternalTrusted; + let min_confirmations = u32::from(match input_trust { + InputTrust::Internal => confirmations_policy.trusted(), + InputTrust::ExternalUntrusted => confirmations_policy.untrusted(), + InputTrust::ExternalTrusted => confirmations_policy.trusted(), + }); - let (_, r, _) = st.add_a_single_note_checking_balance( - TestNoteConfig::from(starting_balance).with_address_type(address_type), - ); - let txid = r.txids()[0]; + // For Internal trust, create the note as change from a real spend + enhancement. + // For External trust variants, inject the note directly via a fake compact block. + let (starting_balance, txid) = if input_trust == InputTrust::Internal { + // Fund the wallet with more than we need so that a spend produces change. + let fund_value = Zatoshis::const_from_u64(100_000); + st.add_a_single_note_checking_balance(fund_value); + + let to = T::random_address(st.rng_mut()); + let change_strategy = + single_output_change_strategy(StandardFeeRule::Zip317, None, T::SHIELDED_PROTOCOL); + let input_selector = GreedyInputSelector::new(); + let request = TransactionRequest::new(vec![Payment::without_memo( + to.to_zcash_address(st.network()), + Zatoshis::const_from_u64(20_000), + )]) + .unwrap(); + let proposal = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request, + ConfirmationsPolicy::MIN, + ) + .unwrap(); + let expected_fee = proposal.steps().head.balance().fee_required(); + let change_value = (fund_value - Zatoshis::const_from_u64(20_000) - expected_fee).unwrap(); + let txids = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ) + .unwrap(); + let (h, _) = st.generate_next_block_including(txids[0]); + st.scan_cached_blocks(h, 1); + st.service_enhancement_requests(); - // Mark the external input as explicitly trusted, if so requested - let trusted = input_trust == InputTrust::ExternalTrusted; - if trusted { - st.wallet_mut().set_tx_trust(txid, true).unwrap(); - } + assert_eq!( + st.get_total_balance(account.id()), + change_value, + "Unexpected total balance after spend" + ); + + (change_value, txids[0]) + } else { + let starting_balance = Zatoshis::const_from_u64(60_000); + let (_, r, _) = st.add_a_single_note_checking_balance(starting_balance); + let txid = r.txids()[0]; + + if trusted { + st.wallet_mut().set_tx_trust(txid, true).unwrap(); + } + + (starting_balance, txid) + }; let add_confirmation = |i: u32| { let (h, _) = st.generate_empty_block(); @@ -410,9 +447,17 @@ pub fn zip_315_confirmations_test_steps( .wallet() .get_received_outputs(txid, TargetHeight::from(h + 1), confirmations_policy) .unwrap(); - assert_eq!(outputs.len(), 1); + // For the Internal case, the spending tx produces both the external send + // and the internal change; find the output that matches starting_balance. + let target_output = outputs + .iter() + .find(|o| o.value() == starting_balance) + .unwrap_or_else(|| { + assert_eq!(outputs.len(), 1); + &outputs[0] + }); assert_eq!( - outputs[0].confirmations_until_spendable(), + target_output.confirmations_until_spendable(), u32::from(if trusted { confirmations_policy.trusted() } else { @@ -1615,6 +1660,7 @@ pub fn send_with_multiple_change_outputs( let (h, _) = st.generate_next_block_including(sent_tx_id); st.scan_cached_blocks(h, 1); + st.service_enhancement_requests(); // Now, create another proposal with more outputs requested. We have two change notes; // we'll spend one of them, and then we'll generate 7 splits. @@ -1776,6 +1822,7 @@ pub fn send_multi_step_proposed_transfer( let (h, _) = st.generate_next_block_including(*txid); st.scan_cached_blocks(h, 1); } + st.service_enhancement_requests(); // Check that there are sent outputs with the correct values. let confirmed_sent: Vec> = txids @@ -2881,59 +2928,79 @@ pub fn spend_succeeds_to_t_addr_zero_change( ); } +/// Verifies that change notes recovered via enhancement are spendable. pub fn change_note_spends_succeed( ds_factory: impl DataStoreFactory, cache: impl TestCache, ) { let mut st = TestDsl::with_sapling_birthday_account(ds_factory, cache).build::(); - // Add funds to the wallet in a single note owned by the internal spending key - let value = Zatoshis::const_from_u64(70000); - st.add_a_single_note_checking_balance( - TestNoteConfig::from(value).with_address_type(AddressType::Internal), - ); - - // Value is considered pending at 10 confirmations. let account = st.test_account().cloned().unwrap(); let account_id = account.id(); - assert_eq!( - st.get_pending_shielded_balance(account_id, ConfirmationsPolicy::default()), - value - ); - assert_eq!( - st.get_spendable_balance(account_id, ConfirmationsPolicy::default()), - Zatoshis::ZERO - ); - let change_note_scope = st - .wallet() - .get_notes(T::SHIELDED_PROTOCOL) - .unwrap() - .iter() - .find_map(|note| (note.note().value() == value).then_some(note.spending_key_scope())); - assert_matches!(change_note_scope, Some(Scope::Internal)); + // Add funds in a single External-scope note + let value = Zatoshis::const_from_u64(100000); + st.add_a_single_note_checking_balance(value); + // Spend part of the funds to produce a change note (Internal scope) + let to = T::random_address(st.rng_mut()); let fee_rule = StandardFeeRule::Zip317; - - // TODO: generate_next_block_from_tx does not currently support transparent outputs. - let to = TransparentAddress::PublicKeyHash([7; 20]).into(); + let change_strategy = single_output_change_strategy(fee_rule, None, T::SHIELDED_PROTOCOL); + let input_selector = GreedyInputSelector::new(); + let request = TransactionRequest::new(vec![Payment::without_memo( + to.to_zcash_address(st.network()), + Zatoshis::const_from_u64(20000), + )]) + .unwrap(); let proposal = st - .propose_standard_transfer::( + .propose_transfer( account_id, - fee_rule, + &input_selector, + &change_strategy, + request, ConfirmationsPolicy::MIN, - &to, - Zatoshis::const_from_u64(50000), - None, - None, - T::SHIELDED_PROTOCOL, ) .unwrap(); + let txids = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ) + .unwrap(); + let (h, _) = st.generate_next_block_including(txids[0]); + st.scan_cached_blocks(h, 1); + st.service_enhancement_requests(); - // Executing the proposal should succeed - assert_matches!( - st.create_proposed_transactions::(account.usk(), OvkPolicy::Sender, &proposal), - Ok(txids) if txids.len() == 1 + // Verify the change note was recovered with the correct balance. + let total_balance = st.get_total_balance(account_id); + assert!( + total_balance > Zatoshis::ZERO, + "Change note was not recovered" + ); + + // Verify the change note is spendable (has a valid commitment tree position). + let spendable_balance = st.get_spendable_balance(account_id, ConfirmationsPolicy::MIN); + assert_eq!( + total_balance, spendable_balance, + "Change note is not spendable; position may be missing from commitment tree" + ); + + // Verify the change note can be proposed for spending. + let to2 = T::random_address(st.rng_mut()); + let proposal2 = st.propose_standard_transfer::( + account_id, + fee_rule, + ConfirmationsPolicy::MIN, + &to2, + Zatoshis::const_from_u64(10000), + None, + None, + T::SHIELDED_PROTOCOL, + ); + assert!( + proposal2.is_ok(), + "Failed to propose spending change note: {proposal2:?}" ); } @@ -3191,10 +3258,19 @@ pub fn external_address_change_spends_detected_in_restore_from_seed( let (h, _) = st.generate_next_block_including(create_proposed_result.unwrap()[0]); st.scan_cached_blocks(h, 1); + st.service_enhancement_requests(); assert_eq!( st.get_total_balance(account.id()), @@ -3808,6 +3890,7 @@ pub fn fully_funded_fully_private( let (h, _) = st.generate_next_block_including(create_proposed_result.unwrap()[0]); st.scan_cached_blocks(h, 1); + st.service_enhancement_requests(); // Since the recipient address is in the same account, the total balance includes the transfer // amount. @@ -4826,6 +4910,8 @@ pub fn scan_cached_blocks_finds_received_notes( ); } +/// Verifies that change notes produced by a spending transaction are recovered via +/// enhancement after External-only batch scanning. pub fn scan_cached_blocks_finds_change_notes( ds_factory: Dsf, cache: impl TestCache, @@ -4836,73 +4922,702 @@ pub fn scan_cached_blocks_finds_change_notes( let mut st = TestDsl::with_sapling_birthday_account(ds_factory, cache).build::(); let account = st.test_account().cloned().unwrap(); - let dfvk = T::test_account_fvk(&st); // Wallet summary is not yet available assert_eq!(st.get_wallet_summary(ConfirmationsPolicy::MIN), None); - // Create a fake CompactBlock sending value to the address + // Add funds to the wallet let value = Zatoshis::const_from_u64(50000); - let (_, _, nf) = st.add_a_single_note_checking_balance(value); + st.add_a_single_note_checking_balance(value); - // Create a second fake CompactBlock spending value from the address - let not_our_key = T::sk_to_fvk(&T::sk(&[0xf5; 32])); - let to2 = T::fvk_default_address(¬_our_key); + // Spend part of the value to an external address + let to2 = T::random_address(st.rng_mut()); let value2 = Zatoshis::const_from_u64(20000); - let (spent_height, _) = st.generate_next_block_spending(&dfvk, (nf, value), to2, value2); + let fee_rule = StandardFeeRule::Zip317; + let change_strategy = single_output_change_strategy(fee_rule, None, T::SHIELDED_PROTOCOL); + let input_selector = GreedyInputSelector::new(); + let request = TransactionRequest::new(vec![Payment::without_memo( + to2.to_zcash_address(st.network()), + value2, + )]) + .unwrap(); + let proposal = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request, + ConfirmationsPolicy::MIN, + ) + .unwrap(); + let expected_fee = proposal.steps().head.balance().fee_required(); + let txids = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ) + .unwrap(); + let (h, _) = st.generate_next_block_including(txids[0]); - // Scan the cache again - st.scan_cached_blocks(spent_height, 1); + // External-only scan: change note (Internal scope) is NOT found by scanning alone. + st.scan_cached_blocks(h, 1); + + // Enhancement recovers the change note via Internal IVK trial decryption. + st.service_enhancement_requests(); // Account balance should equal the change + let expected_change = (value - value2 - expected_fee).unwrap(); + assert_eq!(st.get_total_balance(account.id()), expected_change); assert_eq!( - st.get_total_balance(account.id()), - (value - value2).unwrap() + st.get_spendable_balance(account.id(), ConfirmationsPolicy::MIN), + expected_change ); } +/// Verifies that scanning blocks out of order (spending block before receive block) +/// correctly detects spends and recovers change via enhancement. pub fn scan_cached_blocks_detects_spends_out_of_order( ds_factory: Dsf, cache: impl TestCache, ) where Dsf: DataStoreFactory, ::AccountId: std::fmt::Debug, + ::DataStore: Reset, { let mut st = TestDsl::with_sapling_birthday_account(ds_factory, cache).build::(); let account = st.test_account().cloned().unwrap(); - let dfvk = T::test_account_fvk(&st); // Wallet summary is not yet available assert_eq!(st.get_wallet_summary(ConfirmationsPolicy::MIN), None); - // Create a fake CompactBlock sending value to the address + // Add funds to the wallet in a single note let value = Zatoshis::const_from_u64(50000); - let (received_height, _, nf) = - st.generate_next_block(&dfvk, AddressType::DefaultExternal, value); + let (received_height, _, _) = st.add_a_single_note_checking_balance(value); - // Create a second fake CompactBlock spending value from the address - let not_our_key = T::sk_to_fvk(&T::sk(&[0xf5; 32])); - let to2 = T::fvk_default_address(¬_our_key); + // Spend part of the value + let to2 = T::random_address(st.rng_mut()); let value2 = Zatoshis::const_from_u64(20000); - let (spent_height, _) = st.generate_next_block_spending(&dfvk, (nf, value), to2, value2); + let fee_rule = StandardFeeRule::Zip317; + let change_strategy = single_output_change_strategy(fee_rule, None, T::SHIELDED_PROTOCOL); + let input_selector = GreedyInputSelector::new(); + let request = TransactionRequest::new(vec![Payment::without_memo( + to2.to_zcash_address(st.network()), + value2, + )]) + .unwrap(); + let proposal = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request, + ConfirmationsPolicy::MIN, + ) + .unwrap(); + let expected_fee = proposal.steps().head.balance().fee_required(); + let expected_change = (value - value2 - expected_fee).unwrap(); + let txids = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ) + .unwrap(); + + // Save the raw tx before reset (simulates fetching from lightwalletd) + let spending_tx = st.wallet().get_transaction(txids[0]).unwrap().unwrap(); + let (spent_height, _) = st.generate_next_block_including(txids[0]); + + // Reset wallet, recreate the account from seed. + // TestBuilder uses seed [0u8; 32] by default. + let seed = Secret::new([0u8; 32].to_vec()); + let birthday = account.birthday().clone(); + st.reset(); + let (restored_account_id, _) = st + .wallet_mut() + .create_account("restored", &seed, &birthday, None) + .unwrap(); - // Scan the spending block first. + // Scan the spending block FIRST (out of order) st.scan_cached_blocks(spent_height, 1); + st.enhance_transaction(&spending_tx, Some(spent_height)); // Account balance should equal the change + assert_eq!(st.get_total_balance(restored_account_id), expected_change); + + // Now scan the receive block + st.scan_cached_blocks(received_height, 1); + + // Account balance should be the same (not double-counted) + assert_eq!(st.get_total_balance(restored_account_id), expected_change); +} + +/// Verifies that a chain of self-sends spanning more than `PRUNING_DEPTH` blocks is +/// correctly reconstructed during chunked sync. +/// +/// Under the External-only batch scanning optimization, Internal-scope change notes +/// are recovered only during post-scan transaction enhancement, and enhancement uses +/// the `nullifier_map` to detect whether those change notes were themselves spent. +/// If the nullifier map is pruned before enhancement has a chance to consult it (as +/// happened in a previous revision of this codebase, when `put_blocks` called +/// `prune_tracked_nullifiers` before the sync orchestrator drained the enhancement +/// queue), the cascade of change-note discovery halts at the first change note whose +/// spend has been wiped from the map, and the wallet reports an inflated balance. +/// +/// This test exercises that exact scenario: +/// 1. Fund a wallet with a single note. +/// 2. Build a chain of two self-sends, with tx2 (which spends tx1's change) placed +/// in a block more than `PRUNING_DEPTH` blocks after tx1 but within the same +/// future scan batch. Both spending txs and the intervening empty blocks are +/// deposited into the cache WITHOUT scanning them. +/// 3. Reset the wallet (simulating a fresh recovery from seed) and re-scan the +/// entire cached range with a batch large enough that pruning after the batch +/// would wipe the `nullifier_map` entry for tx2's spend of tx1's change. +/// 4. Assert that enhancement correctly discovers both change notes, marks the +/// first change note as spent, and leaves only the second change note in the +/// balance. +/// +/// With the bug present (pruning inside `put_blocks`), `detect_*_spend` for tx1's +/// change note returns `None` because tx2's spend-nullifier entry was cascaded out +/// of the map, tx2 is never enhanced, and the balance incorrectly equals the value +/// of tx1's change note. +/// +/// With the fix (pruning deferred to the sync orchestrator after enhancement), the +/// cascade completes and the balance correctly equals the value of tx2's change note. +#[cfg(feature = "sync")] +pub fn enhancement_cascade_survives_pruning( + ds_factory: Dsf, + cache: impl TestCache, +) where + Dsf: DataStoreFactory, + ::AccountId: std::fmt::Debug, + ::DataStore: Reset, +{ + use crate::data_api::ll::wallet::PRUNING_DEPTH; + + let mut st = TestDsl::with_sapling_birthday_account(ds_factory, cache).build::(); + + let account = st.test_account().cloned().unwrap(); + let birthday = account.birthday().clone(); + + // Fund the wallet with a single note and scan it so the wallet knows about the + // note as a spendable External-scope receipt. + let funding_value = Zatoshis::const_from_u64(100_000); + let (h_funding, _, _) = st.add_a_single_note_checking_balance(funding_value); + + // Build transaction 1: spend part of the funded note to an external recipient, + // producing an Internal-scope change note. + let ext_addr_1 = T::random_address(st.rng_mut()); + let send_value_1 = Zatoshis::const_from_u64(10_000); + let fee_rule = StandardFeeRule::Zip317; + let change_strategy = single_output_change_strategy(fee_rule, None, T::SHIELDED_PROTOCOL); + let input_selector = GreedyInputSelector::new(); + let request_1 = TransactionRequest::new(vec![Payment::without_memo( + ext_addr_1.to_zcash_address(st.network()), + send_value_1, + )]) + .unwrap(); + let proposal_1 = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request_1, + ConfirmationsPolicy::MIN, + ) + .unwrap(); + let fee_1 = proposal_1.steps().head.balance().fee_required(); + let change_1 = (funding_value - send_value_1 - fee_1).unwrap(); + let txids_1 = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal_1, + ) + .unwrap(); + + // Mine tx1 into the next block, scan it, and run enhancement so the wallet + // discovers tx1's change note and can use it to construct tx2. The change + // note's spendability is contingent on enhancement having run; without it, + // External-only scanning would not have detected the Internal-scope output. + let (h_tx1, _) = st.generate_next_block_including(txids_1[0]); + st.scan_cached_blocks(h_tx1, 1); + st.service_enhancement_requests(); + + // Capture tx1's raw bytes from the wallet so we can replay enhancement + // after the wallet reset that we perform below. + let tx1_raw = st + .wallet() + .get_transaction(txids_1[0]) + .unwrap() + .expect("tx1 should be stored in the wallet after creation"); + + // Add empty filler blocks so that tx2 lands more than `PRUNING_DEPTH` blocks + // after tx1. Under the buggy ordering (pruning inside `put_blocks`), pruning + // at the end of a single large `scan_cached_blocks` call would advance the + // pruning floor past tx1's `h_tx1`, cascade-deleting the `nullifier_map` + // entry for tx2's spend of tx1's change note. With the fix, pruning is + // deferred to `WalletWrite::prune_tracked_nullifiers` invoked by the sync + // orchestrator AFTER enhancement has had a chance to consult the map. + let tx2_offset = PRUNING_DEPTH as usize + 20; + for _ in 0..tx2_offset { + st.generate_empty_block(); + } + + // Build transaction 2: spend tx1's change note to a different external + // recipient, producing another Internal-scope change note. + let ext_addr_2 = T::random_address(st.rng_mut()); + let send_value_2 = Zatoshis::const_from_u64(5_000); + let request_2 = TransactionRequest::new(vec![Payment::without_memo( + ext_addr_2.to_zcash_address(st.network()), + send_value_2, + )]) + .unwrap(); + let proposal_2 = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request_2, + ConfirmationsPolicy::MIN, + ) + .unwrap(); + let fee_2 = proposal_2.steps().head.balance().fee_required(); + let change_2 = (change_1 - send_value_2 - fee_2).unwrap(); + let txids_2 = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal_2, + ) + .unwrap(); + let (h_tx2, _) = st.generate_next_block_including(txids_2[0]); + + // Capture tx2's raw bytes likewise. + let tx2_raw = st + .wallet() + .get_transaction(txids_2[0]) + .unwrap() + .expect("tx2 should be stored in the wallet after creation"); + + // Add trailing empty blocks so that the chunked scan advances the + // fully-scanned height more than `PRUNING_DEPTH` blocks past `h_tx2`. + // Without this, pruning at `(fully_scanned - PRUNING_DEPTH)` would not + // advance past `h_tx2`, and the buggy ordering would NOT actually wipe + // tx2's locator entry — meaning the test would pass even with the bug. + let trailing_fillers = PRUNING_DEPTH as usize + 20; + for _ in 0..trailing_fillers { + st.generate_empty_block(); + } + + // Reset the wallet and restore from the same seed/birthday. The cache is + // preserved, so the scan will observe all blocks from funding through the + // trailing filler. + let seed = Secret::new([0u8; 32].to_vec()); + st.reset(); + let (restored_account_id, _) = st + .wallet_mut() + .create_account("restored", &seed, &birthday, None) + .unwrap(); + + // Re-scan the entire cached range in a single batch. With per-chunk + // semantics this is the largest meaningful chunk: scan everything, then + // enhance, then prune. Under the buggy ordering (pruning inside + // `put_blocks`), this would cascade-delete the `nullifier_map` entry for + // tx2's spend of tx1's change note before enhancement gets a chance to + // consult it. + let scan_start = h_funding; + let scan_limit = + (u32::from(h_tx2) + (trailing_fillers as u32) + 1 - u32::from(scan_start)) as usize; + st.scan_cached_blocks(scan_start, scan_limit); + + // Manually drive enhancement using the raw bytes we captured pre-reset. + // This mirrors what the production `sync::service_transaction_data_requests` + // would do via `fetch_raw_transaction`. Critically: when tx1 is enhanced, + // its change note is stored, and `detect_*_spend(nf_change_1)` consults + // the `nullifier_map` for tx2's spend at `h_tx2`. With the fix that map + // entry is still present (because the sync orchestrator defers pruning + // until after enhancement); under the buggy ordering, pruning would have + // already cascade-deleted the entry, causing the cascade to halt at tx1's + // change note. + st.enhance_transaction(&tx1_raw, Some(h_tx1)); + st.enhance_transaction(&tx2_raw, Some(h_tx2)); + + // Invoke the post-enhancement prune exactly as `sync::run` would. This is + // a no-op for the test's correctness under the fix; the explicit call + // mirrors the production sync orchestrator and exercises the new + // `WalletWrite::prune_tracked_nullifiers` code path. + st.wallet_mut() + .prune_tracked_nullifiers(PRUNING_DEPTH) + .unwrap(); + + // With the fix, tx1's change note is marked as spent by tx2 via + // `detect_*_spend` consulting the still-intact `nullifier_map`, and only + // tx2's change note contributes to the balance. Without the fix, the + // map entry has been cascade-deleted by the time tx1 is enhanced, so + // `detect_*_spend(nf_change_1)` returns None, tx1's change note is stored + // as unspent, and the balance is incorrectly inflated to `change_1`. assert_eq!( - st.get_total_balance(account.id()), - (value - value2).unwrap() + st.get_total_balance(restored_account_id), + change_2, + "Final balance should equal tx2's change note ({:?}), not tx1's change note ({:?}). \ + A mismatch here indicates that the cascade of change-note discovery halted at the \ + first change note, which happens when the nullifier_map is pruned before enhancement.", + change_2, + change_1, ); +} - // Now scan the block in which we received the note that was spent. - st.scan_cached_blocks(received_height, 1); +/// Structural regression test for the pruning-before-enhancement ordering bug. +/// +/// The positive test [`enhancement_cascade_survives_pruning`] demonstrates that a +/// chain of self-sends spanning more than `PRUNING_DEPTH` blocks can be correctly +/// reconstructed if pruning happens after enhancement. For that test to be meaningful +/// we need independent evidence that the alternative ordering (pruning before +/// enhancement) would in fact lose information. Rather than asserting on the +/// user-visible balance — which is a derived property that could become robust for +/// unrelated reasons — this test asserts directly on the structural property that +/// makes the cascade work: the backend's tracked-nullifier entry for tx2's spend of +/// tx1's change note must exist between scanning and pruning, and must be wiped +/// by pruning. +/// +/// Because the notion of "tracked nullifier entry" is backend-specific (SQLite uses +/// a `nullifier_map` table; other backends may differ), this test takes a +/// `count_tracked_spends_at` callback that the caller implements against the +/// concrete backend. The callback returns the number of tracked entries at a +/// specific block height. +/// +/// Concretely: +/// 1. Fund the wallet, build tx1 spending the funding note (producing an +/// Internal-scope change note in tx1), enhance so tx2 can be built against it. +/// 2. Insert `PRUNING_DEPTH + 20` filler blocks, build tx2 spending tx1's change +/// note, then add another `PRUNING_DEPTH + 20` trailing fillers so the +/// fully-scanned height advances past `h_tx2 + PRUNING_DEPTH`. +/// 3. Reset and re-sync. Under External-only scanning, tx1's change note is +/// Internal-scope and therefore unknown at scan time; tx2's spend of that +/// unknown nullifier is recorded in the backend's tracked-spend index at +/// `(h_tx2, tx_index)`, waiting for enhancement to resolve it. +/// 4. **Before** calling `prune_tracked_nullifiers`, assert +/// `count_tracked_spends_at(h_tx2) >= 1`. +/// 5. Call `prune_tracked_nullifiers`. +/// 6. Assert `count_tracked_spends_at(h_tx2) == 0`. With the tracked entry gone, +/// a later enhancement of tx1 would no longer be able to link tx1's change +/// note to its spending transaction, and the cascade would halt. +#[cfg(feature = "sync")] +pub fn pruning_wipes_late_discovered_spend_locator( + ds_factory: Dsf, + cache: Cache, + count_tracked_spends_at: CountFn, +) where + T: ShieldedPoolTester, + Dsf: DataStoreFactory, + ::AccountId: std::fmt::Debug, + ::DataStore: Reset, + Cache: TestCache, + CountFn: Fn( + &TestState::DataStore, LocalNetwork>, + BlockHeight, + ) -> i64, +{ + use crate::data_api::ll::wallet::PRUNING_DEPTH; + + let mut st = TestDsl::with_sapling_birthday_account(ds_factory, cache).build::(); + + let account = st.test_account().cloned().unwrap(); + let birthday = account.birthday().clone(); - // Account balance should be the same. + // Fund the wallet with a single External-scope note. + let funding_value = Zatoshis::const_from_u64(100_000); + let (h_funding, _, _) = st.add_a_single_note_checking_balance(funding_value); + + // tx1: spend part of the funding note, producing an Internal-scope change note. + let ext_addr_1 = T::random_address(st.rng_mut()); + let send_value_1 = Zatoshis::const_from_u64(10_000); + let fee_rule = StandardFeeRule::Zip317; + let change_strategy = single_output_change_strategy(fee_rule, None, T::SHIELDED_PROTOCOL); + let input_selector = GreedyInputSelector::new(); + let request_1 = TransactionRequest::new(vec![Payment::without_memo( + ext_addr_1.to_zcash_address(st.network()), + send_value_1, + )]) + .unwrap(); + let proposal_1 = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request_1, + ConfirmationsPolicy::MIN, + ) + .unwrap(); + let txids_1 = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal_1, + ) + .unwrap(); + let (h_tx1, _) = st.generate_next_block_including(txids_1[0]); + st.scan_cached_blocks(h_tx1, 1); + st.service_enhancement_requests(); + + // Filler blocks so tx2 lands far enough past tx1 that the pruning floor will + // eventually sweep past h_tx2. + let tx2_offset = PRUNING_DEPTH as usize + 20; + for _ in 0..tx2_offset { + st.generate_empty_block(); + } + + // tx2: spend tx1's change note. + let ext_addr_2 = T::random_address(st.rng_mut()); + let send_value_2 = Zatoshis::const_from_u64(5_000); + let request_2 = TransactionRequest::new(vec![Payment::without_memo( + ext_addr_2.to_zcash_address(st.network()), + send_value_2, + )]) + .unwrap(); + let proposal_2 = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request_2, + ConfirmationsPolicy::MIN, + ) + .unwrap(); + let txids_2 = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal_2, + ) + .unwrap(); + let (h_tx2, _) = st.generate_next_block_including(txids_2[0]); + + // Trailing fillers so the fully-scanned height advances well past h_tx2 + PRUNING_DEPTH. + let trailing_fillers = PRUNING_DEPTH as usize + 20; + for _ in 0..trailing_fillers { + st.generate_empty_block(); + } + + // Reset and re-sync from seed. Scanning identifies tx2's spend as an unknown + // nullifier and records it in the backend's tracked-spend index at h_tx2. + let seed = Secret::new([0u8; 32].to_vec()); + st.reset(); + st.wallet_mut() + .create_account("restored", &seed, &birthday, None) + .unwrap(); + let scan_start = h_funding; + let scan_limit = + (u32::from(h_tx2) + (trailing_fillers as u32) + 1 - u32::from(scan_start)) as usize; + st.scan_cached_blocks(scan_start, scan_limit); + + let before = count_tracked_spends_at(&st, h_tx2); + assert!( + before >= 1, + "Expected at least one tracked-spend entry at block_height = h_tx2 ({:?}) for \ + tx2's spend of tx1's change note (got {}). Scanning should have inserted a \ + locator entry for the unresolved spend, since tx1's change note is Internal-scope \ + and therefore unknown under External-only trial decryption.", + h_tx2, + before, + ); + + // Prune. The cascade relation between the locator and the tracked-spend entry + // should wipe the entry at h_tx2. + st.wallet_mut() + .prune_tracked_nullifiers(PRUNING_DEPTH) + .unwrap(); + + let after = count_tracked_spends_at(&st, h_tx2); assert_eq!( - st.get_total_balance(account.id()), - (value - value2).unwrap() + after, 0, + "Pruning should cascade-delete the tracked-spend entry at block_height = h_tx2 \ + ({:?}), but {} entry/entries remain. If this assertion fails, either the cascade \ + relation has been removed or block_fully_scanned has advanced less far than \ + expected — in either case the pruning-before-enhancement ordering that the \ + positive test `enhancement_cascade_survives_pruning` guards against would no \ + longer be a hazard, and that test's rationale needs re-examination.", + h_tx2, after, + ); +} + +/// Verifies that `v_transactions` hides wallet-spending transactions that are in the +/// intermediate scan-only state (inputs marked spent, change outputs not yet stored +/// via enhancement), and shows them with the correct balance delta once enhancement +/// has completed. +/// +/// This test directly guards against the regression that caused the user to see +/// transiently wrong "sent" / "received" amounts during sync under External-only +/// batch scanning. Specifically: +/// +/// 1. Fund the wallet with a note (this tx is a pure receive — always visible). +/// 2. Construct a wallet spend-to-external transaction via `create_proposed_transactions`. +/// The wallet-created tx has `raw` set via `store_transaction_to_be_sent` so it is +/// always visible from the moment of creation (covers the mempool display case). +/// 3. Mine the send, scan the block, but do NOT run enhancement yet. At this point, +/// scanning has marked the wallet input as spent via `mark_notes_spent`, but the +/// Internal-scope change note has not been recovered. Because the wallet-created +/// tx already has `raw` set from creation, the filter still admits it — so the +/// pre-enhancement state is correctly handled for wallet-created transactions. +/// 4. **The more important case**: reset the wallet and re-sync from seed. Under a +/// fresh re-sync, there is no `store_transaction_to_be_sent` history — both the +/// receipt and the send must be discovered from the chain. The receipt is visible +/// post-scan because it has zero wallet-side spends (pure receive). The send is +/// HIDDEN post-scan because it has wallet-side spends but no `raw` yet; it becomes +/// visible after enhancement runs and `put_tx_data` sets `raw`. Before enhancement, +/// `get_tx_history()` should not include the send's half-formed row. +/// 5. Run enhancement. The send now has correct data and appears in `get_tx_history`. +#[cfg(feature = "sync")] +pub fn v_transactions_hides_unenhanced_txs( + ds_factory: Dsf, + cache: impl TestCache, +) where + Dsf: DataStoreFactory, + ::AccountId: std::fmt::Debug, + ::DataStore: Reset, +{ + let mut st = TestDsl::with_sapling_birthday_account(ds_factory, cache).build::(); + + let account = st.test_account().cloned().unwrap(); + let birthday = account.birthday().clone(); + + // Fund the wallet with a single note. This tx has no wallet spends, so under + // both the old and new view, it should appear in `get_tx_history` as a pure + // receive immediately after scanning. + let funding_value = Zatoshis::const_from_u64(100_000); + let (h_funding, _, _) = st.add_a_single_note_checking_balance(funding_value); + + // Construct a spend-to-external transaction. + let ext_addr = T::random_address(st.rng_mut()); + let send_value = Zatoshis::const_from_u64(10_000); + let fee_rule = StandardFeeRule::Zip317; + let change_strategy = single_output_change_strategy(fee_rule, None, T::SHIELDED_PROTOCOL); + let input_selector = GreedyInputSelector::new(); + let request = TransactionRequest::new(vec![Payment::without_memo( + ext_addr.to_zcash_address(st.network()), + send_value, + )]) + .unwrap(); + let proposal = st + .propose_transfer( + account.id(), + &input_selector, + &change_strategy, + request, + ConfirmationsPolicy::MIN, + ) + .unwrap(); + let fee = proposal.steps().head.balance().fee_required(); + let expected_change = (funding_value - send_value - fee).unwrap(); + let txids = st + .create_proposed_transactions::( + account.usk(), + OvkPolicy::Sender, + &proposal, + ) + .unwrap(); + let send_txid = txids[0]; + + // Capture the raw tx bytes for later enhancement replay post-reset. + let send_tx = st + .wallet() + .get_transaction(send_txid) + .unwrap() + .expect("wallet-created tx should be stored"); + + // Mine the send and scan it. Note that because this is a wallet-created tx, + // `store_transaction_to_be_sent` has already populated both `raw` and the + // change note in `received_notes` at creation time, so the filter admits it + // even without running enhancement. + let (h_send, _) = st.generate_next_block_including(send_txid); + st.scan_cached_blocks(h_send, 1); + + // Sanity check: wallet-created send is visible immediately with correct delta. + // This confirms the HAVING-based filter does not hide wallet-created transactions + // that had their state populated via `store_transaction_to_be_sent`. + let history_before_reset = st.wallet().get_tx_history().unwrap(); + assert!( + history_before_reset.iter().any(|tx| tx.txid() == send_txid), + "Wallet-created send tx should be immediately visible in v_transactions via \ + raw IS NOT NULL (set by store_transaction_to_be_sent)" + ); + + // Now the harder case: reset the wallet and re-sync. This simulates the user's + // scenario (they wipe and re-enter the seed). On a fresh re-sync, the wallet + // has no `store_transaction_to_be_sent` history — both txs must be discovered + // purely from the chain. The receipt has no wallet spends, so it's visible + // post-scan. The send has wallet spends, so it must be hidden until + // enhancement runs and sets `raw`. + let seed = Secret::new([0u8; 32].to_vec()); + st.reset(); + st.wallet_mut() + .create_account("restored", &seed, &birthday, None) + .unwrap(); + + // Scan blocks without running enhancement on the send tx. + st.scan_cached_blocks(h_funding, 1); + st.scan_cached_blocks(h_send, 1); + + // Check: receipt visible (pure receive), send HIDDEN (wallet spends present but + // no `raw`). The filter should drop the send tx from v_transactions here because + // displaying it would show the wrong intermediate delta. + let history_post_scan = st.wallet().get_tx_history().unwrap(); + assert!( + history_post_scan + .iter() + .any(|tx| tx.txid() != send_txid && tx.mined_height() == Some(h_funding)), + "Receipt tx should be visible immediately after scanning (no wallet spends \ + means no intermediate state to hide)" + ); + assert!( + !history_post_scan.iter().any(|tx| tx.txid() == send_txid), + "Send tx should be HIDDEN by v_transactions filter after scanning \ + (wallet input marked spent but change note not yet recovered via enhancement). \ + If this assertion fails, the UI would display a transiently-wrong balance \ + delta for the send. Found: {:?}", + history_post_scan + .iter() + .find(|tx| tx.txid() == send_txid) + .map(|tx| tx.account_value_delta()) + ); + + // Now run enhancement explicitly for the send transaction. We pre-captured the + // raw bytes before reset, so we can replay enhancement without gRPC. + st.enhance_transaction(&send_tx, Some(h_send)); + + // After enhancement, the send tx should be visible with the correct delta: + // the wallet sent `send_value + fee` externally and kept `expected_change`, + // so the net outflow is `send_value + fee`. + let history_post_enhance = st.wallet().get_tx_history().unwrap(); + let send_entry = history_post_enhance + .iter() + .find(|tx| tx.txid() == send_txid) + .expect( + "Send tx should be VISIBLE in v_transactions after enhancement \ + (raw now set via put_tx_data, change note now in received_notes)", + ); + + // Verify the delta reflects what the wallet actually sent (outflow). + // account_balance_delta should be -(funding - change) = -(send + fee). + let actual_spent = send_entry.total_spent(); + let actual_received = send_entry.total_received(); + assert_eq!( + actual_spent, funding_value, + "Send tx total_spent should equal the funded note value ({funding_value:?}), got {actual_spent:?}", + ); + assert_eq!( + actual_received, expected_change, + "Send tx total_received should equal the change note value ({expected_change:?}), got {actual_received:?}", + ); + // Net outflow = spent - received; this should equal the external send value + fee. + let net_outflow = (actual_spent - actual_received).unwrap(); + let expected_net_outflow = (funding_value - expected_change).unwrap(); + assert_eq!( + net_outflow, expected_net_outflow, + "Net outflow should equal (external send + fee) = {expected_net_outflow:?}, got {net_outflow:?}", ); } diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 55468ed444..4776e2d111 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -30,6 +30,25 @@ workspace. - `impl<'conn, P, CL, R> WalletWrite for WalletDb, P, CL, R>` to enable calling `WalletWrite` methods inside `WalletDb::transactionally` (amortizing the database transaction overhead). +- `impl WalletWrite::notify_wallet_note_positions for WalletDb`, which marks + note commitment tree positions for notes discovered during transaction + enhancement, so that those notes become spendable. +- `impl WalletWrite::prune_tracked_nullifiers for WalletDb`, which prunes + `nullifier_map` and cascades its locator entries. This is now called from + the `sync` module's orchestration loop AFTER transaction enhancement has + had a chance to consult the map; `put_blocks` no longer prunes internally. +- A new migration (`v_transactions_filter_intermediate_state`) updates the + `v_transactions` view to hide transactions whose DB state is transiently + inconsistent during sync. Two failure modes are filtered out: a wallet + spend that scanning has recorded but whose matching change output has not + yet been recovered via enhancement (detected as `raw IS NULL AND + spent_note_count > 0`), and a transaction whose own enhancement has stored + change notes but whose wallet-side spend cascade has not yet been applied + (detected as `change_note_count > 0 AND spent_note_count = 0`). Downstream + consumers querying `v_transactions` directly may observe transiently fewer + rows during sync than they did previously. +- `sync` feature flag for integration testing of the sync module's + enhancement logic. ### Changed - The `accounts` table now stores IVK item caches instead of FVK item caches for @@ -48,6 +67,23 @@ workspace. `zcash_client_sqlite::error::StandaloneImportConflict` - P2SH UTXOs returned by `get_spendable_transparent_outputs` now include a precomputed input size for accurate ZIP 317 fee estimation. +- The `tx_retrieval_queue` query used by `transaction_data_requests` now orders + results by `(mined_height, tx_index, txid)` so that enhancement processes + transactions in chain order. This is load-bearing for the change-note cascade + discovery that runs during enhancement: when tx A spends a change note created + in tx B, processing B first ensures B's change note is already stored by the + time A's `mark_notes_spent` runs. The `txid` tiebreaker keeps ordering stable + under the request-set equality check in `sync::service_transaction_data_requests`. +- `queue_tx_retrieval` now emits `Enhancement` (not `GetStatus`) for mined + transactions that already have `raw` populated. This is needed because + transactions recorded via `put_tx_meta` during scanning need to go through + `decrypt_and_store_transaction` to recover Internal-scope change outputs and + OVK metadata — a `GetStatus` alone would not do that work. +- The transparent `transaction_data_requests` query no longer excludes + ephemeral-scope addresses. This is required for ZIP 320 shielding flows: + funds parked on a single-use ephemeral taddr must still generate a + spend-detection lookup after the parking transaction mines, so the later + spend can be discovered. ### Removed - `zcash_client_sqlite::GapLimits` use `zcash_keys::keys::transparent::GapLimits` instead. diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index ddd267ba7f..f66e6b494c 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -362,6 +362,86 @@ pub(crate) fn scan_cached_blocks_detects_spends_out_of_order(TestDbFactory::default(), BlockCache::new()) } +#[cfg(feature = "sync")] +pub(crate) fn enhancement_cascade_survives_pruning() { + zcash_client_backend::data_api::testing::pool::enhancement_cascade_survives_pruning::( + TestDbFactory::default(), + BlockCache::new(), + ) +} + +#[cfg(feature = "sync")] +pub(crate) fn v_transactions_hides_unenhanced_txs() { + zcash_client_backend::data_api::testing::pool::v_transactions_hides_unenhanced_txs::( + TestDbFactory::default(), + BlockCache::new(), + ) +} + +/// Structural regression test for the pruning-before-enhancement ordering bug. +/// +/// The positive test [`enhancement_cascade_survives_pruning`] demonstrates that a +/// chain of self-sends spanning more than `PRUNING_DEPTH` blocks can be correctly +/// reconstructed if pruning happens after enhancement. For that test to be meaningful +/// we need to prove that the alternative ordering (pruning before enhancement) would +/// in fact lose information. Rather than asserting on the user-visible balance — which +/// is a derived property that could become robust for unrelated reasons — we assert +/// directly on the structural property that makes the cascade work: the +/// `nullifier_map` row for tx2's spend of tx1's change note must exist between scanning +/// and pruning, and must be wiped by pruning. +/// +/// Concretely: +/// 1. Fund the wallet, build tx1 spending a single external-scope input (produces a +/// change note), build tx2 spending tx1's change note (produces another change note). +/// 2. Put tx2 in a block more than `PRUNING_DEPTH` after tx1, with sufficient trailing +/// fillers that the "fully scanned" height advances past `h_tx2 + PRUNING_DEPTH`. +/// 3. Reset and re-sync: scanning identifies tx2's spend nullifier as an unknown +/// (because under External-only scanning tx1's change note is Internal-scope and +/// therefore not in the known-nullifier set). That unknown spend is recorded in +/// `nullifier_map` at `(h_tx2, tx_index)`, waiting for enhancement to resolve it. +/// 4. **Before** calling `prune_tracked_nullifiers`, assert the `nullifier_map` row at +/// `block_height = h_tx2` is present. +/// 5. Call `prune_tracked_nullifiers`. +/// 6. Assert the row is gone. Because `nullifier_map.tx_locator` has `ON DELETE CASCADE`, +/// pruning the `tx_locator_map` row at `h_tx2` also removes the nullifier entry; +/// with the locator gone, `detect_*_spend` during any later enhancement of tx1 +/// would return `None` and the cascade would halt. +/// +/// This asserts only the specific DB state that the positive test relies on. It does +/// not depend on enhancement semantics, balance-delta arithmetic, or any other +/// higher-level behavior, so it should remain stable under future refactors that +/// strengthen enhancement without changing the pruning contract. +#[cfg(feature = "sync")] +pub(crate) fn pruning_wipes_late_discovered_spend_locator() { + zcash_client_backend::data_api::testing::pool::pruning_wipes_late_discovered_spend_locator::< + T, + _, + _, + _, + >( + TestDbFactory::default(), + BlockCache::new(), + |state, height| { + use rusqlite::params; + + let spend_pool: i64 = match T::SHIELDED_PROTOCOL { + zcash_protocol::ShieldedProtocol::Sapling => 2, + zcash_protocol::ShieldedProtocol::Orchard => 3, + }; + state + .wallet() + .conn() + .query_row( + "SELECT COUNT(*) FROM nullifier_map + WHERE spend_pool = ?1 AND block_height = ?2", + params![spend_pool, u32::from(height)], + |row| row.get::<_, i64>(0), + ) + .unwrap() + }, + ); +} + pub(crate) fn metadata_queries_exclude_unwanted_notes() { zcash_client_backend::data_api::testing::pool::metadata_queries_exclude_unwanted_notes::( TestDbFactory::default(), diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index 1d15380464..08ac8f7ef0 100644 --- a/zcash_client_sqlite/src/wallet/orchard.rs +++ b/zcash_client_sqlite/src/wallet/orchard.rs @@ -528,6 +528,24 @@ pub(crate) mod tests { testing::pool::change_note_spends_succeed::() } + #[cfg(feature = "sync")] + #[test] + fn enhancement_cascade_survives_pruning() { + testing::pool::enhancement_cascade_survives_pruning::() + } + + #[cfg(feature = "sync")] + #[test] + fn pruning_wipes_late_discovered_spend_locator() { + testing::pool::pruning_wipes_late_discovered_spend_locator::() + } + + #[cfg(feature = "sync")] + #[test] + fn v_transactions_hides_unenhanced_txs() { + testing::pool::v_transactions_hides_unenhanced_txs::() + } + #[test] fn account_deletion() { testing::pool::account_deletion::() diff --git a/zcash_client_sqlite/src/wallet/sapling.rs b/zcash_client_sqlite/src/wallet/sapling.rs index 1454959847..2b0af2bfcb 100644 --- a/zcash_client_sqlite/src/wallet/sapling.rs +++ b/zcash_client_sqlite/src/wallet/sapling.rs @@ -539,6 +539,24 @@ pub(crate) mod tests { testing::pool::change_note_spends_succeed::() } + #[cfg(feature = "sync")] + #[test] + fn enhancement_cascade_survives_pruning() { + testing::pool::enhancement_cascade_survives_pruning::() + } + + #[cfg(feature = "sync")] + #[test] + fn pruning_wipes_late_discovered_spend_locator() { + testing::pool::pruning_wipes_late_discovered_spend_locator::() + } + + #[cfg(feature = "sync")] + #[test] + fn v_transactions_hides_unenhanced_txs() { + testing::pool::v_transactions_hides_unenhanced_txs::() + } + #[test] fn account_deletion() { testing::pool::account_deletion::()