diff --git a/zcash_client_backend/CHANGELOG.md b/zcash_client_backend/CHANGELOG.md index 8e974132d9..72c22e4f00 100644 --- a/zcash_client_backend/CHANGELOG.md +++ b/zcash_client_backend/CHANGELOG.md @@ -18,6 +18,34 @@ workspace. - `wallet::ConfirmationsPolicy::confirmations_until_spendable` - `DecryptableTransaction` - `ReceivedTransactionOutput` + - `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. - in `zcash_client_backend::proto::compact_formats`: - `CompactTx` has added fields `vin` and `vout` - Added types `CompactTxIn`, `TxOut` @@ -42,6 +70,50 @@ workspace. - `zcash_client_backend::wallet::WalletTransparentOutput::with_known_input_size` ### Changed +- `zcash_client_backend::data_api::ll::ReceivedShieldedOutput::nullifier` now + returns `Option` (by value) instead of `Option<&Self::Nullifier>`. +- `zcash_client_backend::data_api::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. +- `zcash_client_backend::data_api::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 (Orchard nullifier + computation does not depend on the note's commitment tree position, so it + succeeds even without a block source). Sapling nullifier bytes remain unset + when called through this entry point because computing them requires knowing + 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::sync::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. The internal `service_transaction_data_requests` helper + now returns a `ServiceOutcome` (`Drained` or `Stabilized`) so the outer + loop can distinguish "queue empty" from "queue stuck". Behavior is + unchanged when scanning still has work to do — the outer loop continues + and a later scan chunk may unblock previously-stuck requests. +- `zcash_client_backend::sync` `TransactionDataRequest::TransactionsInvolvingAddress` + handling now always calls `notify_address_checked` after a successful + address-history lookup, instead of only when the server returned zero + transactions. This advances the per-output `max_observed_unspent_height` + cursor past address ranges that returned only unrelated activity, so a + spend-search request for a reused address no longer re-surfaces forever. +- `zcash_client_backend::sync`: `prune_tracked_nullifiers` is now called + exclusively from `run()`, after the post-`running()` drain, and only when + that drain returns `ServiceOutcome::Drained`. The previous per-chunk and + per-verify-range prune calls inside `running()` violated the contract on + `WalletWrite::prune_tracked_nullifiers` (which requires the request queue + to be drained for the range being pruned): a stabilized-but-non-empty + drain from inside `running()` would still proceed to prune locators that + an unresolved enhancement retry would later need. The per-chunk + `service_transaction_data_requests` call inside `running()` is preserved + for cross-chunk cascade discovery; only the prune is hoisted out. - `zcash_client_backend::data_api::error::Error::MemoForbidden` has been replaced by `Error::Payment(zip321::PaymentError)`, which can represent both memo-to-transparent and zero-valued-transparent-output errors. @@ -125,6 +197,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/Cargo.toml b/zcash_client_backend/Cargo.toml index 90d7af9499..e467015589 100644 --- a/zcash_client_backend/Cargo.toml +++ b/zcash_client_backend/Cargo.toml @@ -264,6 +264,9 @@ test-dependencies = [ ## Exposes APIs that allow calculation of non-standard fees. non-standard-fees = ["zcash_primitives/non-standard-fees"] +## Enables the prospective ZIP 233 (Network Sustainability Mechanism) feature. +zip-233 = ["zcash_primitives/zip-233"] + #! ### Experimental features ## Exposes unstable APIs. Their behaviour may change at any time. diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 3c959916af..efbf900380 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -1911,6 +1911,15 @@ pub trait WalletRead { /// responding to a set of transaction data requests may result in the creation of new /// transaction data requests, such as when it is necessary to fill in purely-transparent /// transaction history by walking the chain backwards via transparent inputs. + /// + /// Servicing these requests is required in order for the wallet to converge to a complete + /// view of transaction history when compact block scanning omits internal-scope shielded + /// trial decryption. In particular, the wallet relies on: + /// - [`TransactionDataRequest::Enhancement`] to recover internal/change shielded outputs and + /// outgoing metadata from full transactions. + /// - [`TransactionDataRequest::TransactionsInvolvingAddress`] to discover shielding and other + /// transparent-address transactions that are not fully identifiable from compact block data + /// alone. fn transaction_data_requests(&self) -> Result, Self::Error>; /// Returns a vector of [`ReceivedTransactionOutput`] values describing the outputs of the @@ -3109,6 +3118,51 @@ pub trait WalletWrite: WalletRead { received_tx: DecryptedTransaction, ) -> Result<(), Self::Error>; + /// Notifies the wallet that the given note positions have been discovered as belonging + /// to the wallet during enhancement, so that witnesses can be maintained for them. + /// + /// This is needed because External-only batch scanning may not discover all wallet + /// outputs; those found later via enhancement need their commitment tree positions + /// registered so they are spendable. + fn notify_wallet_note_positions( + &mut self, + _block_range: std::ops::Range, + _wallet_note_positions: &[(ShieldedProtocol, incrementalmerkletree::Position)], + ) -> Result<(), Self::Error> { + Ok(()) + } + + /// Prunes backend-tracked metadata about nullifiers whose owning wallet note has + /// not yet been identified, for blocks that are at least `pruning_depth` below the + /// wallet's current fully-scanned chain tip. + /// + /// During compact-block scanning, a wallet backend may observe nullifiers for which + /// the owning note has not yet been decrypted (for example, a spend of a change note + /// whose [`Scope::Internal`] output will only be recovered via + /// [`TransactionDataRequest::Enhancement`] of the output-producing transaction). If + /// the backend retains these observations, a later enhancement step can use them to + /// link the newly-recovered note to its spending transaction; without them, such a + /// note would remain unspent in the wallet's view even after its spend has been + /// mined, producing an inflated balance. + /// + /// Callers MUST only invoke this method after the wallet's + /// [`TransactionDataRequest`] queue has been drained for the range being pruned. + /// Pruning prematurely releases the information the backend needs to attach + /// late-discovered notes to their spends. + /// + /// Because pruning is defined relative to the wallet's contiguous-from-birthday + /// fully-scanned chain tip, the effective pruning floor will not advance during + /// non-linear scan passes (for example, when a higher-priority [`ScanPriority::FoundNote`] + /// range is being scanned ahead of the historical fill). This is correct but means + /// that under non-linear scan plans, tracked nullifier metadata may retain more + /// entries than under purely linear scanning. + /// + /// [`Scope::Internal`]: zip32::Scope::Internal + /// [`ScanPriority::FoundNote`]: scanning::ScanPriority::FoundNote + fn prune_tracked_nullifiers(&mut self, _pruning_depth: u32) -> Result<(), Self::Error> { + Ok(()) + } + /// Sets the trust status of the given transaction to either trusted or untrusted. /// /// The outputs of a trusted transaction will be available for spending with diff --git a/zcash_client_backend/src/data_api/chain.rs b/zcash_client_backend/src/data_api/chain.rs index 887bffeedb..c132798025 100644 --- a/zcash_client_backend/src/data_api/chain.rs +++ b/zcash_client_backend/src/data_api/chain.rs @@ -600,7 +600,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/ll.rs b/zcash_client_backend/src/data_api/ll.rs index ae3f852ffd..de5e712bc6 100644 --- a/zcash_client_backend/src/data_api/ll.rs +++ b/zcash_client_backend/src/data_api/ll.rs @@ -615,7 +615,7 @@ pub trait ReceivedShieldedOutput { fn is_change(&self) -> bool; /// Returns the nullifier that will be revealed when the note is spent, if the output was /// observed using a key that provides the capability for nullifier computation. - fn nullifier(&self) -> Option<&Self::Nullifier>; + fn nullifier(&self) -> Option; /// Returns the position of the note in the note commitment tree, if the transaction that /// produced the output has been mined. fn note_commitment_tree_position(&self) -> Option; @@ -661,8 +661,8 @@ impl ReceivedShieldedOutput for WalletSaplingOutput fn is_change(&self) -> bool { WalletSaplingOutput::is_change(self) } - fn nullifier(&self) -> Option<&::sapling::Nullifier> { - self.nf() + fn nullifier(&self) -> Option<::sapling::Nullifier> { + self.nf().copied() } fn note_commitment_tree_position(&self) -> Option { Some(WalletSaplingOutput::note_commitment_tree_position(self)) @@ -696,11 +696,12 @@ impl ReceivedShieldedOutput for DecryptedOutput<::sapling::Note fn is_change(&self) -> bool { self.transfer_type() == TransferType::WalletInternal } - fn nullifier(&self) -> Option<&::sapling::Nullifier> { - None + fn nullifier(&self) -> Option<::sapling::Nullifier> { + self.nullifier_bytes() + .and_then(|bytes| ::sapling::Nullifier::from_slice(&bytes).ok()) } fn note_commitment_tree_position(&self) -> Option { - None + self.note_commitment_tree_position() } fn recipient_key_scope(&self) -> Option { if self.transfer_type() == TransferType::WalletInternal { @@ -752,8 +753,8 @@ impl ReceivedShieldedOutput for WalletOrchardOutput fn is_change(&self) -> bool { WalletOrchardOutput::is_change(self) } - fn nullifier(&self) -> Option<&::orchard::note::Nullifier> { - self.nf() + fn nullifier(&self) -> Option<::orchard::note::Nullifier> { + self.nf().copied() } fn note_commitment_tree_position(&self) -> Option { Some(WalletOrchardOutput::note_commitment_tree_position(self)) @@ -788,11 +789,12 @@ impl ReceivedShieldedOutput for DecryptedOutput<::orchard::Note fn is_change(&self) -> bool { self.transfer_type() == TransferType::WalletInternal } - fn nullifier(&self) -> Option<&::orchard::note::Nullifier> { - None + fn nullifier(&self) -> Option<::orchard::note::Nullifier> { + self.nullifier_bytes() + .and_then(|bytes| Option::from(::orchard::note::Nullifier::from_bytes(&bytes))) } fn note_commitment_tree_position(&self) -> Option { - None + self.note_commitment_tree_position() } fn recipient_key_scope(&self) -> Option { if self.transfer_type() == TransferType::WalletInternal { diff --git a/zcash_client_backend/src/data_api/ll/wallet.rs b/zcash_client_backend/src/data_api/ll/wallet.rs index fb29ec6f55..811e043594 100644 --- a/zcash_client_backend/src/data_api/ll/wallet.rs +++ b/zcash_client_backend/src/data_api/ll/wallet.rs @@ -459,10 +459,17 @@ where } } - // Prune the nullifier map of entries we no longer need. - wallet_db - .prune_tracked_nullifiers(PRUNING_DEPTH) - .map_err(PutBlocksError::Storage)?; + // NOTE: Nullifier-map pruning is intentionally NOT performed here. Under the + // External-only batch scanning optimization, Internal-scope change notes are + // discovered only during post-scan transaction enhancement, and + // `detect_*_spend` consults the nullifier map to link those late-discovered + // notes to their spending transactions. Pruning here — before enhancement + // has a chance to run — would cascade-delete those locator entries (see + // ON DELETE CASCADE on the `nullifier_map` foreign key in `db.rs`) and cause + // the wallet to report an inflated balance. The sync orchestrator + // (`zcash_client_backend::sync::run` via `WalletWrite::prune_tracked_nullifiers`) + // is now responsible for invoking pruning after it has drained the + // transaction-data request queue for the scanned range. // We will have a start position and a last scanned height in all cases where // `blocks` is non-empty. @@ -732,7 +739,13 @@ where tx_ref, funding_account, d_tx.sapling_outputs(), - |_, _| Ok(None), + |wallet_db, output| { + Ok(output + .nullifier() + .map(|nf| wallet_db.detect_sapling_spend(&nf)) + .transpose()? + .flatten()) + }, |wallet_db, output, tx_ref, spent_in| { wallet_db.put_received_sapling_note(output, tx_ref, d_tx.mined_height(), spent_in) }, @@ -749,7 +762,13 @@ where tx_ref, funding_account, d_tx.orchard_outputs(), - |_, _| Ok(None), + |wallet_db, output| { + Ok(output + .nullifier() + .map(|nf| wallet_db.detect_orchard_spend(&nf)) + .transpose()? + .flatten()) + }, |wallet_db, output, tx_ref, spent_in| { wallet_db.put_received_orchard_note(output, tx_ref, d_tx.mined_height(), spent_in) }, diff --git a/zcash_client_backend/src/data_api/testing.rs b/zcash_client_backend/src/data_api/testing.rs index f4cc42d240..1a494182c2 100644 --- a/zcash_client_backend/src/data_api/testing.rs +++ b/zcash_client_backend/src/data_api/testing.rs @@ -881,6 +881,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 5c604cf553..42040bab7b 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. @@ -4603,6 +4687,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, @@ -4613,73 +4699,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_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 67f1cb6def..db879b5d49 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -49,6 +49,7 @@ use crate::{ Account, MaxSpendMode, SentTransaction, SentTransactionOutput, WalletCommitmentTrees, WalletRead, WalletWrite, error::Error, wallet::input_selection::propose_send_max, }, + decrypt::compute_enriched_outputs, decrypt_transaction, fees::{ ChangeStrategy, DustOutputPolicy, StandardFeeRule, standard::SingleOutputChangeStrategy, @@ -204,6 +205,22 @@ impl PcztRecipient { /// Scans a [`Transaction`] for any information that can be decrypted by the accounts in /// the wallet, and saves it to the wallet. +/// +/// In addition to running [`decrypt_transaction`] over the input transaction, this +/// function enriches the resulting [`crate::data_api::DecryptedTransaction`] via +/// `crate::decrypt::compute_enriched_outputs` so that any change notes recovered via +/// Internal-IVK decryption have their nullifier metadata populated. Without this +/// enrichment, change notes would be stored in the wallet without nullifiers, and +/// subsequent transactions that spend those change notes would fail to mark them as +/// spent — resulting in an inflated wallet balance. +/// +/// Because this entry point does not have access to a block source, it cannot compute +/// per-output Sapling note commitment tree positions; the enrichment is therefore best-effort +/// for Sapling. Orchard nullifiers do not depend on note positions, so they ARE populated +/// regardless. Callers that require Sapling nullifier population should use the +/// [`crate::sync`] module's enhancement pipeline (which fetches block contents from +/// lightwalletd to derive positions) or pass enriched transactions directly via +/// [`WalletWrite::store_decrypted_tx`]. pub fn decrypt_and_store_transaction( params: &ParamsT, data: &mut DbT, @@ -217,13 +234,23 @@ where // Fetch the UnifiedFullViewingKeys we are tracking let ufvks = data.get_unified_full_viewing_keys()?; - data.store_decrypted_tx(decrypt_transaction( - params, - mined_height.map_or_else(|| data.get_tx_height(tx.txid()), |h| Ok(Some(h)))?, - data.chain_height()?, - tx, - &ufvks, - ))?; + let resolved_height = + mined_height.map_or_else(|| data.get_tx_height(tx.txid()), |h| Ok(Some(h)))?; + let chain_tip = data.chain_height()?; + + let d_tx = decrypt_transaction(params, resolved_height, chain_tip, tx, &ufvks); + + // Enrich the decrypted outputs with nullifier metadata. We do not have access + // to bundle base positions here (computing them requires block contents), so + // Sapling nullifiers will be left unset; Orchard nullifiers ARE populated + // because they are derived from the note + FVK alone, without dependence on + // the note's commitment tree position. This is sufficient to enable + // `mark_notes_spent` to detect spends of Orchard change notes in subsequent + // transactions, which is the primary failure mode this enrichment guards + // against. + let d_tx = compute_enriched_outputs(tx, &d_tx, None, &ufvks); + + data.store_decrypted_tx(d_tx)?; Ok(()) } diff --git a/zcash_client_backend/src/decrypt.rs b/zcash_client_backend/src/decrypt.rs index 552c62051a..33993752b8 100644 --- a/zcash_client_backend/src/decrypt.rs +++ b/zcash_client_backend/src/decrypt.rs @@ -1,11 +1,14 @@ use std::collections::HashMap; +use incrementalmerkletree::Position; use sapling::note_encryption::{PreparedIncomingViewingKey, SaplingDomain}; use zcash_keys::keys::UnifiedFullViewingKey; use zcash_note_encryption::{try_note_decryption, try_output_recovery_with_ovk}; use zcash_primitives::{ transaction::Transaction, transaction::components::sapling::zip212_enforcement, }; +#[cfg(any(feature = "sync", feature = "test-dependencies"))] +use zcash_protocol::ShieldedProtocol; use zcash_protocol::{ consensus::{self, BlockHeight, NetworkUpgrade}, memo::MemoBytes, @@ -39,6 +42,8 @@ pub struct DecryptedOutput { account: AccountId, memo: MemoBytes, transfer_type: TransferType, + nullifier_bytes: Option<[u8; 32]>, + note_commitment_tree_position: Option, } impl DecryptedOutput { @@ -55,6 +60,8 @@ impl DecryptedOutput { account, memo, transfer_type, + nullifier_bytes: None, + note_commitment_tree_position: None, } } @@ -84,6 +91,39 @@ impl DecryptedOutput { pub fn transfer_type(&self) -> TransferType { self.transfer_type } + + /// Returns the serialized nullifier for the note, if known. + /// + /// This is populated during transaction enhancement when the note commitment tree + /// position is available, enabling the wallet to detect future spends of this note. + /// Returns `None` for [`TransferType::Outgoing`] outputs (which the wallet cannot spend) + /// or when the position is not yet known. + pub fn nullifier_bytes(&self) -> Option<[u8; 32]> { + self.nullifier_bytes + } + + /// Returns the position of the note in the note commitment tree, if known. + /// + /// This is populated during transaction enhancement from compact block metadata. + /// A note without a known position cannot be spent, because the wallet cannot + /// construct a Merkle path (witness) for it. + pub fn note_commitment_tree_position(&self) -> Option { + self.note_commitment_tree_position + } + + /// Attaches note commitment tree position and nullifier metadata to this output. + /// + /// This is used during the enhancement phase to enrich a [`DecryptedOutput`] with + /// the information needed to make the note spendable. + pub fn with_spend_metadata( + mut self, + note_commitment_tree_position: Option, + nullifier_bytes: Option<[u8; 32]>, + ) -> Self { + self.note_commitment_tree_position = note_commitment_tree_position; + self.nullifier_bytes = nullifier_bytes; + self + } } impl DecryptedOutput { @@ -248,3 +288,168 @@ pub fn decrypt_transaction<'a, P: consensus::Parameters, AccountId: Copy>( orchard_outputs, ) } + +/// Note commitment tree base positions for the Sapling and Orchard bundles of a single +/// transaction. +/// +/// `sapling_base` is the position of the first Sapling output in the transaction's bundle +/// within the global Sapling note commitment tree. `orchard_base` is the analogous position +/// for the first Orchard action. These bases are used by [`compute_enriched_outputs`] to +/// compute per-output positions and (for Sapling) per-output nullifiers. +/// +/// For Orchard, nullifier computation does not depend on position, so `orchard_base` is +/// only required to populate the `commitment_tree_position` field on stored notes (which +/// is needed for witness construction during spending). For Sapling, nullifier computation +/// REQUIRES the position because Sapling's nullifier hash mixes the note's tree position. +#[derive(Clone, Copy, Debug, Default)] +pub struct TxBundlePositions { + /// Position of the first Sapling output in the transaction's bundle within the global + /// Sapling note commitment tree. + pub sapling_base: Option, + /// Position of the first Orchard action in the transaction's bundle within the global + /// Orchard note commitment tree. + #[cfg(feature = "orchard")] + pub orchard_base: Option, +} + +/// Enriches the outputs of a [`DecryptedTransaction`] with note commitment tree positions +/// and nullifier bytes, returning a new `DecryptedTransaction` with the same outputs but +/// with [`DecryptedOutput::nullifier_bytes`] and +/// [`DecryptedOutput::note_commitment_tree_position`] populated where possible. +/// +/// This is used to add spend-tracking metadata to outputs that were discovered via +/// [`decrypt_transaction`] (typically during transaction enhancement, when change notes +/// recovered via Internal-IVK decryption need their nullifiers computed so that subsequent +/// spends of those change notes can be detected). +/// +/// # Arguments +/// - `tx`: The transaction whose outputs are being enriched. +/// - `d_tx`: The decrypted form of the transaction, as returned by [`decrypt_transaction`]. +/// - `positions`: Optional pre-computed bundle base positions. When `None`, Sapling outputs +/// will not get nullifiers (because Sapling nullifier computation requires position), +/// but Orchard outputs WILL get nullifiers (because Orchard nullifier computation does +/// not depend on position). When `Some`, both pools get nullifiers. +/// - `ufvks`: The wallet's full viewing keys, keyed by account identifier. +/// +/// # Returns +/// A new `DecryptedTransaction` containing the enriched outputs. +pub fn compute_enriched_outputs<'a, AccountId: Copy + std::hash::Hash + Eq>( + tx: &'a Transaction, + d_tx: &DecryptedTransaction<'a, Transaction, AccountId>, + positions: Option<&TxBundlePositions>, + ufvks: &HashMap, +) -> DecryptedTransaction<'a, Transaction, AccountId> { + let sapling_outputs = d_tx + .sapling_outputs() + .iter() + .map(|output| { + let position = positions + .and_then(|pos| pos.sapling_base) + .map(|base| Position::from(base + output.index() as u64)); + let nullifier_bytes = if output.transfer_type() == TransferType::Outgoing { + None + } else { + let scope = match output.transfer_type() { + TransferType::WalletInternal => Scope::Internal, + TransferType::Incoming => Scope::External, + TransferType::Outgoing => unreachable!(), + }; + position.and_then(|position| { + ufvks + .get(output.account()) + .and_then(|ufvk| ufvk.sapling()) + .map(|dfvk| output.note().nf(&dfvk.to_nk(scope), position.into()).0) + }) + }; + + DecryptedOutput::new( + output.index(), + output.note().clone(), + *output.account(), + output.memo().clone(), + output.transfer_type(), + ) + .with_spend_metadata(position, nullifier_bytes) + }) + .collect(); + + #[cfg(feature = "orchard")] + let orchard_outputs = d_tx + .orchard_outputs() + .iter() + .map(|output| { + let position = positions + .and_then(|pos| pos.orchard_base) + .map(|base| Position::from(base + output.index() as u64)); + // Orchard nullifier computation does not depend on the note commitment tree + // position; it can be computed from the note plus the wallet's full viewing + // key alone. This means change notes recovered via Internal-IVK enhancement + // can have their nullifiers populated even when the caller does not know the + // tx's position in the global tree (e.g., when called from + // `decrypt_and_store_transaction` which has no block-source access). + let nullifier_bytes = if output.transfer_type() == TransferType::Outgoing { + None + } else { + ufvks + .get(output.account()) + .and_then(|ufvk| ufvk.orchard()) + .map(|fvk| output.note().nullifier(fvk).to_bytes()) + }; + + DecryptedOutput::new( + output.index(), + *output.note(), + *output.account(), + output.memo().clone(), + output.transfer_type(), + ) + .with_spend_metadata(position, nullifier_bytes) + }) + .collect(); + + DecryptedTransaction::new( + d_tx.mined_height(), + tx, + sapling_outputs, + #[cfg(feature = "orchard")] + orchard_outputs, + ) +} + +/// Collects note commitment tree positions from a [`DecryptedTransaction`] for use with +/// [`WalletWrite::notify_wallet_note_positions`]. +/// +/// Only positions for non-outgoing outputs are collected, since the wallet only needs to +/// maintain witnesses for notes it can spend. +/// +/// [`WalletWrite::notify_wallet_note_positions`]: crate::data_api::WalletWrite::notify_wallet_note_positions +#[cfg(any(feature = "sync", feature = "test-dependencies"))] +pub fn collect_wallet_note_positions( + d_tx: &DecryptedTransaction<'_, Transaction, AccountId>, +) -> Vec<(ShieldedProtocol, Position)> { + #[cfg_attr(not(feature = "orchard"), allow(unused_mut))] + let mut positions: Vec<(ShieldedProtocol, Position)> = d_tx + .sapling_outputs() + .iter() + .filter(|output| output.transfer_type() != TransferType::Outgoing) + .filter_map(|output| { + output + .note_commitment_tree_position() + .map(|position| (ShieldedProtocol::Sapling, position)) + }) + .collect(); + + #[cfg(feature = "orchard")] + positions.extend( + d_tx.orchard_outputs() + .iter() + .filter(|output| output.transfer_type() != TransferType::Outgoing) + .filter_map(|output| { + output + .note_commitment_tree_position() + .map(|position| (ShieldedProtocol::Orchard, position)) + }), + ); + + positions +} diff --git a/zcash_client_backend/src/scanning.rs b/zcash_client_backend/src/scanning.rs index e4c0295fe4..01ea99a00d 100644 --- a/zcash_client_backend/src/scanning.rs +++ b/zcash_client_backend/src/scanning.rs @@ -247,6 +247,24 @@ impl ScanningKeys, + ) -> Self { + Self::from_account_ufvks_with_scopes(ufvks, &[Scope::External, Scope::Internal]) + } + + /// Constructs a [`ScanningKeys`] from an iterator of [`UnifiedFullViewingKey`]s, + /// using only the specified scopes for key derivation. + /// + /// Passing `&[Scope::External]` omits internal (change) keys from trial + /// decryption, which halves the key-agreement work per output. Change notes + /// are still recoverable: when a note is discovered whose nullifier matches + /// one already in the `nullifier_map` (from a previously-scanned block), the + /// spending transaction can be queued for enhancement, and + /// [`decrypt_and_store_transaction`] will find the change via IVK Internal. + /// + /// [`decrypt_and_store_transaction`]: crate::data_api::wallet::decrypt_and_store_transaction + pub fn from_account_ufvks_with_scopes( + ufvks: impl IntoIterator, + scopes: &[Scope], ) -> Self { #![allow(clippy::type_complexity)] @@ -262,7 +280,7 @@ impl ScanningKeys ScanningKeys::Error: std::error::Error + Send + Sync + 'static, ::Error: std::error::Error + Send + Sync + 'static, { @@ -77,11 +98,76 @@ where // 2) Pass the commitment tree data to the database. update_subtree_roots(client, db_data).await?; - while running(client, params, db_cache, db_data, batch_size).await? {} + loop { + while running(client, params, db_cache, db_data, batch_size).await? {} + + let outcome = service_transaction_data_requests(client, params, db_data).await?; + + // Only prune the tracked-nullifier map if the request queue fully + // drained. The contract on `WalletWrite::prune_tracked_nullifiers` + // requires the request queue to be drained first, because pruning + // would otherwise cascade-delete locators that an unresolved + // enhancement retry will need. If the queue stabilized with + // unresolved entries, skip pruning entirely — the next sync session + // will retry once the underlying issue (typically lightwalletd not + // recognizing a queued txid) clears, and the map size is bounded by + // the work done during the running() call so growth is finite. + // + // This is the only `prune_tracked_nullifiers` call site in the sync + // module: the per-chunk drain inside `running()` is preserved for + // its cascade-discovery side effects, but pruning is intentionally + // hoisted out of `running()` so we can gate it on this drain + // outcome. + if matches!(outcome, ServiceOutcome::Drained) { + db_data + .prune_tracked_nullifiers(PRUNING_DEPTH) + .map_err(Error::Wallet)?; + } + + let scan_done = db_data + .suggest_scan_ranges() + .map_err(Error::Wallet)? + .into_iter() + .all(|range| range.is_empty()); + + if scan_done { + // No more scan work to do. Break regardless of whether the queue + // drained or stabilized: any still-pending requests cannot be + // unblocked by further scanning on this pass, so we have converged + // as far as this sync run can. A subsequent `run()` call (triggered + // by new chain-tip data, UI action, etc.) can retry the stabilized + // requests with fresh state. + if matches!(outcome, ServiceOutcome::Stabilized) { + debug!( + "sync::run converged with a non-empty but stabilized \ + transaction-data request queue; deferring to next run" + ); + } + break; + } + // scan_done == false → loop again. New scanning work may unblock + // stabilized requests, at which point `service_transaction_data_requests` + // will progress normally on the next iteration (its `last_requests` + // local is freshly None on each call). + } Ok(()) } +/// Outcome of a single call to [`service_transaction_data_requests`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ServiceOutcome { + /// The transaction-data request queue was fully drained. + Drained, + /// Two consecutive queue reads returned the same non-empty set of requests + /// without any successful drain. This typically means lightwalletd does + /// not recognize the queued txid(s), or the backend is re-surfacing a + /// request that we just serviced without state advancing. Further progress + /// requires new input from elsewhere (scanning a new chunk, chain tip + /// advancing, etc.). + Stabilized, +} + async fn running( client: &mut CompactTxStreamerClient, params: &P, @@ -98,7 +184,7 @@ where CaT: BlockCache, CaT::Error: std::error::Error + Send + Sync + 'static, DbT: WalletWrite, - DbT::AccountId: ConditionallySelectable + Default + Send + 'static, + DbT::AccountId: ConditionallySelectable + Copy + Default + Send + 'static, DbT::Error: std::error::Error + Send + Sync + 'static, { // 3) Download chain tip metadata from lightwalletd @@ -151,6 +237,17 @@ where // in CompactBlock files on disk is horrendous for the filesystem. block_deletions.push(db_cache.delete(scan_range.clone())); + // Pruning is intentionally NOT performed here. The verify + // branch has no local visibility into whether the + // transaction-data request queue is drained, and pruning + // without that signal violates the contract on + // `WalletWrite::prune_tracked_nullifiers`. Instead, the outer + // `run()` loop calls `service_transaction_data_requests` + // after `running()` returns and prunes only if the outcome is + // `ServiceOutcome::Drained`. Verify scans are tip-adjacent + // and infrequent, so deferring their prune by one stack frame + // has no observable cost. + if scan_ranges_updated { // The suggested scan ranges have been updated, so we re-request. scan_ranges = db_data.suggest_scan_ranges().map_err(Error::Wallet)?; @@ -190,6 +287,11 @@ where } }) }) { + // Capture the chunk's priority before `scan_range` is moved into + // `db_cache.delete` below; we need it for the post-enhancement + // priority re-check. + let chunk_priority = scan_range.priority(); + // Download the blocks in `scan_range` into the block source. download_blocks(client, db_cache, &scan_range).await?; @@ -202,6 +304,29 @@ where // Delete the now-scanned blocks. block_deletions.push(db_cache.delete(scan_range)); + // Drain enhancement requests generated by this chunk. + // + // This is what makes the External-only batch scanning optimization + // correct under chunked sync: change notes (Internal-IVK) are + // discovered here via `decrypt_transaction` on the full transaction + // bytes, and `detect_*_spend` consults the nullifier map (still + // intact because `put_blocks` no longer prunes, and pruning is + // gated on a Drained outcome in the outer `run()` loop) to link + // those change notes to their spending transactions. Change notes + // written to `received_notes` here will also be picked up + // automatically by the next chunk's `scan_cached_blocks`, whose + // in-memory `Nullifiers` set is rebuilt from the database at the + // top of each call — this handles the cross-chunk forward cascade + // without needing the nullifier map. + // + // The return value is intentionally ignored at this site: the + // intra-chunk drain runs purely for its cascade-discovery side + // effects. The decision of whether it is safe to prune the + // nullifier map lives in `run()`, gated on the outcome of the + // post-`running()` drain — if THAT one stabilizes, pruning is + // skipped, regardless of any per-chunk drain outcomes here. + service_transaction_data_requests(client, params, db_data).await?; + if scan_ranges_updated { // The suggested scan ranges have been updated (either due to a continuity // error or because a higher priority range has been added). @@ -211,6 +336,27 @@ where } return Ok(true); } + + // Enhancement may have extended scan ranges via + // `notify_wallet_note_positions` → `scan_complete` → `extend_range`, + // creating new `FoundNote`-priority ranges for blocks adjacent to + // newly-discovered change-note positions. If any such range now + // outranks the current chunk's priority, bail out to the outer loop + // so the higher-priority ranges get processed first. Without this + // re-check, we would only notice the new ranges on the next outer + // pass, which could leave chunks of a single logical scan range + // processing in mixed priority order. + let latest_ranges = db_data.suggest_scan_ranges().map_err(Error::Wallet)?; + if latest_ranges + .first() + .is_some_and(|next| next.priority() > chunk_priority) + { + info!("Waiting for cached blocks to be deleted..."); + for deletion in block_deletions { + deletion.await.map_err(Error::Cache)?; + } + return Ok(true); + } } info!("Waiting for cached blocks to be deleted..."); @@ -220,6 +366,534 @@ where Ok(false) } +/// Drains the wallet's [`TransactionDataRequest`] queue until no more requests remain or +/// progress has stalled. +/// +/// This is the post-scan phase that recovers information which compact-block scanning could +/// not surface directly: change notes (via the enhancement path, which decrypts full +/// transactions under all key scopes), mempool/unmined transaction statuses, and transparent +/// address history. +/// +/// Returns [`ServiceOutcome::Drained`] when the queue empties, or +/// [`ServiceOutcome::Stabilized`] when two consecutive queue reads return the same +/// non-empty set without progress. The caller (`run`) uses the outcome to decide whether +/// to break out of the outer sync loop. +async fn service_transaction_data_requests( + client: &mut CompactTxStreamerClient, + params: &P, + db_data: &mut DbT, +) -> Result::Error, TrErr>> +where + P: Parameters + Send + 'static, + ChT: GrpcService, + ChT::Error: Into, + ChT::ResponseBody: Body + Send + 'static, + ::Error: Into + Send, + DbT: WalletWrite, + DbT::AccountId: Copy + ConditionallySelectable + Default + Send + 'static, + DbT::Error: std::error::Error + Send + Sync + 'static, +{ + // Drain the transaction-data request queue, re-requesting each iteration because + // servicing a request may itself queue new requests (e.g. enhancement of tx A + // discovering a spend whose cascade queues tx B). We terminate either when the + // queue is empty (Drained) or when two consecutive iterations return the same + // non-empty set of requests (Stabilized). The latter prevents us from looping + // forever when a backend repeatedly surfaces the same request without it being + // successfully serviced (for example, when the upstream lightwalletd does not + // have the tx). + let mut last_requests: Option> = None; + + loop { + let requests = db_data.transaction_data_requests().map_err(Error::Wallet)?; + if requests.is_empty() { + return Ok(ServiceOutcome::Drained); + } + + if last_requests.as_ref() == Some(&requests) { + debug!( + "Transaction-data requests stabilized without draining; deferring remaining work" + ); + return Ok(ServiceOutcome::Stabilized); + } + last_requests = Some(requests.clone()); + + for request in requests { + service_transaction_data_request(client, params, db_data, request).await?; + } + } +} + +/// Services a single [`TransactionDataRequest`], fetching any needed data from the +/// lightwalletd server and committing it to the wallet. +async fn service_transaction_data_request( + client: &mut CompactTxStreamerClient, + params: &P, + db_data: &mut DbT, + request: TransactionDataRequest, +) -> Result<(), Error::Error, TrErr>> +where + P: Parameters + Send + 'static, + ChT: GrpcService, + ChT::Error: Into, + ChT::ResponseBody: Body + Send + 'static, + ::Error: Into + Send, + DbT: WalletWrite, + DbT::AccountId: Copy + ConditionallySelectable + Default + Send + 'static, + DbT::Error: std::error::Error + Send + Sync + 'static, +{ + match request { + TransactionDataRequest::GetStatus(txid) => { + let status = match fetch_raw_transaction(client, txid).await? { + Some(raw) => raw_transaction_status(&raw)?, + None => TransactionStatus::TxidNotRecognized, + }; + db_data + .set_transaction_status(txid, status) + .map_err(Error::Wallet)?; + } + TransactionDataRequest::Enhancement(txid) => { + match fetch_raw_transaction(client, txid).await? { + Some(raw) => { + let status = raw_transaction_status(&raw)?; + store_raw_transaction(client, params, db_data, &raw).await?; + if !matches!(status, TransactionStatus::Mined(_)) { + db_data + .set_transaction_status(txid, status) + .map_err(Error::Wallet)?; + } + } + None => db_data + .set_transaction_status(txid, TransactionStatus::TxidNotRecognized) + .map_err(Error::Wallet)?, + } + } + #[cfg(feature = "transparent-inputs")] + TransactionDataRequest::TransactionsInvolvingAddress(req) => { + service_transactions_involving_address(client, params, db_data, req.clone()).await?; + // Always advance the address-checked cursor after a successful + // server query, regardless of whether transactions were returned. + // The semantics of `notify_address_checked` are "we have queried + // the server over this range," not "we have found something + // relevant." If the target UTXO was spent in one of the returned + // transactions, `mark_transparent_utxo_spent` (called from + // `store_raw_transaction`) has already removed it from the + // `transparent_spend_search_queue`, so this call is a no-op for + // that entry. Without this unconditional advancement, an address + // range that returned only unrelated activity would re-surface + // forever in `transaction_data_requests()`. + let checked_height = req.block_range_end().map(|end| end - 1).unwrap_or( + db_data + .chain_height() + .map_err(Error::Wallet)? + .ok_or(Error::MisbehavingServer)?, + ); + db_data + .notify_address_checked(req, checked_height) + .map_err(Error::Wallet)?; + } + } + + Ok(()) +} + +/// Fetches a single raw transaction by txid from the lightwalletd server. +/// +/// Returns `Ok(None)` if the server does not recognize the txid (`tonic::Code::NotFound`). +async fn fetch_raw_transaction( + client: &mut CompactTxStreamerClient, + txid: TxId, +) -> Result, Error> +where + ChT: GrpcService, + ChT::Error: Into, + ChT::ResponseBody: Body + Send + 'static, + ::Error: Into + Send, +{ + match client + .get_transaction(TxFilter { + block: None, + index: 0, + hash: txid.as_ref().to_vec(), + }) + .await + { + Ok(response) => Ok(Some(response.into_inner())), + Err(status) if status.code() == tonic::Code::NotFound => Ok(None), + Err(status) => Err(Error::Server(status)), + } +} + +/// Classifies a [`service::RawTransaction`] as [`TransactionStatus::Mined`] or +/// [`TransactionStatus::NotInMainChain`] based on its `height` field. +/// +/// lightwalletd uses `0` or `u64::MAX` to indicate "not mined"; any other value is the +/// block height at which the transaction was mined. +fn raw_transaction_status( + raw: &service::RawTransaction, +) -> Result> { + if raw.height == 0 || raw.height == u64::MAX { + Ok(TransactionStatus::NotInMainChain) + } else { + BlockHeight::try_from(raw.height) + .map(TransactionStatus::Mined) + .map_err(|_| Error::MisbehavingServer) + } +} + +/// Parses a [`service::RawTransaction`] into a [`Transaction`] tagged with its mined height +/// (if any), choosing the correct consensus branch ID. +/// +/// For mined transactions the branch ID is derived from the mined height. For unmined +/// transactions the expiry height is used as a proxy (after a two-step parse/re-freeze +/// because `Transaction::read` needs a branch ID up front but pre-v5 transactions don't +/// commit to one). Returns `Ok(None)` if the server reported the txid as unknown, or if +/// an unmined transaction has a zero expiry height and no other way to determine the +/// correct branch ID. +#[allow(clippy::type_complexity)] +fn parse_raw_transaction( + params: &P, + raw: &service::RawTransaction, +) -> Result, Transaction)>, Error> +where + P: Parameters, +{ + let mined_height = match raw_transaction_status(raw)? { + TransactionStatus::Mined(height) => Some(height), + TransactionStatus::NotInMainChain => None, + TransactionStatus::TxidNotRecognized => return Ok(None), + }; + + let tx = if let Some(height) = mined_height { + Transaction::read(&raw.data[..], BranchId::for_height(params, height)) + .map_err(|_| Error::MisbehavingServer)? + } else { + let tx_data = Transaction::read(&raw.data[..], BranchId::Sprout) + .map_err(|_| Error::MisbehavingServer)? + .into_data(); + let expiry_height = tx_data.expiry_height(); + if expiry_height == BlockHeight::from(0) { + return Ok(None); + } + + TransactionData::from_parts( + tx_data.version(), + BranchId::for_height(params, expiry_height), + tx_data.lock_time(), + expiry_height, + #[cfg(all( + any(zcash_unstable = "nu7", zcash_unstable = "zfuture"), + feature = "zip-233" + ))] + tx_data.zip233_amount(), + tx_data.transparent_bundle().cloned(), + tx_data.sprout_bundle().cloned(), + tx_data.sapling_bundle().cloned(), + tx_data.orchard_bundle().cloned(), + ) + .freeze() + .map_err(|_| Error::MisbehavingServer)? + }; + + Ok(Some((mined_height, tx))) +} + +/// Decrypts a raw transaction, enriches its outputs with nullifier and commitment-tree +/// position metadata, stores the result in the wallet, and notifies the wallet of any +/// newly-discovered note positions so that witnesses can be maintained. +/// +/// This is the core of the enhancement path: scanning only uses External-scope keys, so +/// change notes are only recovered here (via `decrypt_transaction`, which tries all key +/// scopes) and need their commitment-tree positions and nullifiers populated before being +/// handed to `store_decrypted_tx`. +async fn store_raw_transaction( + client: &mut CompactTxStreamerClient, + params: &P, + db_data: &mut DbT, + raw: &service::RawTransaction, +) -> Result<(), Error::Error, TrErr>> +where + P: Parameters + Send + 'static, + ChT: GrpcService, + ChT::Error: Into, + ChT::ResponseBody: Body + Send + 'static, + ::Error: Into + Send, + DbT: WalletWrite, + DbT::AccountId: Copy + ConditionallySelectable + Default + Send + 'static, + DbT::Error: std::error::Error + Send + Sync + 'static, +{ + let Some((mined_height, tx)) = parse_raw_transaction(params, raw)? else { + return Ok(()); + }; + + let ufvks = db_data + .get_unified_full_viewing_keys() + .map_err(Error::Wallet)?; + let chain_tip_height = db_data.chain_height().map_err(Error::Wallet)?; + let d_tx = decrypt_transaction(params, mined_height, chain_tip_height, &tx, &ufvks); + let d_tx = enrich_decrypted_transaction(client, db_data, &tx, d_tx, &ufvks).await?; + let wallet_note_positions = collect_wallet_note_positions(&d_tx); + db_data.store_decrypted_tx(d_tx).map_err(Error::Wallet)?; + if let Some(height) = mined_height { + if !wallet_note_positions.is_empty() { + db_data + .notify_wallet_note_positions(height..height + 1, &wallet_note_positions) + .map_err(Error::Wallet)?; + } + } + Ok(()) +} + +/// Enriches a [`crate::data_api::DecryptedTransaction`] with nullifier metadata and +/// commitment-tree positions by fetching the transaction's bundle base positions from +/// lightwalletd. +/// +/// For mined transactions that have a shielded bundle, this calls +/// [`fetch_tx_bundle_positions`] to derive the starting position of the tx's bundle +/// within the global note commitment tree, then delegates to [`compute_enriched_outputs`] +/// which uses those bases to populate per-output positions and nullifiers. For unmined +/// transactions (or pure-transparent txs with no shielded bundle) the position lookup is +/// skipped; Sapling outputs remain without nullifiers, but Orchard outputs still get +/// nullifiers because Orchard nullifier computation does not depend on tree position. +async fn enrich_decrypted_transaction<'a, ChT, DbT, CaErr, TrErr>( + client: &mut CompactTxStreamerClient, + db_data: &DbT, + tx: &'a Transaction, + d_tx: crate::data_api::DecryptedTransaction<'a, Transaction, DbT::AccountId>, + ufvks: &std::collections::HashMap, +) -> Result< + crate::data_api::DecryptedTransaction<'a, Transaction, DbT::AccountId>, + Error, +> +where + ChT: GrpcService, + ChT::Error: Into, + ChT::ResponseBody: Body + Send + 'static, + ::Error: Into + Send, + DbT: WalletRead, + DbT::AccountId: Copy + ConditionallySelectable + Default + Send + 'static, + DbT::Error: std::error::Error + Send + Sync + 'static, +{ + let txid = tx.txid(); + let positions = match d_tx.mined_height() { + Some(height) + if d_tx.tx().sapling_bundle().is_some() || { + #[cfg(feature = "orchard")] + { + d_tx.tx().orchard_bundle().is_some() + } + #[cfg(not(feature = "orchard"))] + { + false + } + } => + { + Some(fetch_tx_bundle_positions(client, db_data, height, txid).await?) + } + _ => None, + }; + + Ok(compute_enriched_outputs( + tx, + &d_tx, + positions.as_ref(), + ufvks, + )) +} + +/// Fetches the compact block containing `txid` at `height` and computes the starting +/// position of that transaction's Sapling (and Orchard, if enabled) bundle within each +/// global note commitment tree. +/// +/// For the height-minus-one tree sizes we prefer `block_metadata` from the wallet (the +/// fast path under normal sync), falling back to `download_chain_state` from lightwalletd +/// when the metadata isn't available (e.g. enhancement of a tx in a block the wallet +/// hasn't yet scanned contiguously up to). +async fn fetch_tx_bundle_positions( + client: &mut CompactTxStreamerClient, + db_data: &DbT, + height: BlockHeight, + txid: TxId, +) -> Result::Error, TrErr>> +where + ChT: GrpcService, + ChT::Error: Into, + ChT::ResponseBody: Body + Send + 'static, + ::Error: Into + Send, + DbT: WalletRead, + DbT::Error: std::error::Error + Send + Sync + 'static, +{ + let mut stream = client + .get_block_range(service::BlockRange { + start: Some(BlockId { + height: u32::from(height).into(), + hash: vec![], + }), + end: Some(BlockId { + height: u32::from(height).into(), + hash: vec![], + }), + pool_types: vec![], + }) + .await? + .into_inner(); + let block = stream + .try_next() + .await + .map_err(Error::Server)? + .ok_or(Error::MisbehavingServer)?; + + let tx_index = block + .vtx + .iter() + .position(|tx| tx.txid() == txid) + .ok_or(Error::MisbehavingServer)?; + + // Fetch prior-block metadata once (not per-pool) to avoid redundant DB + // queries and, on cache miss, redundant gRPC round-trips. + let prior_sapling_tree_size; + #[cfg(feature = "orchard")] + let prior_orchard_tree_size; + + if height == BlockHeight::from(0) { + prior_sapling_tree_size = 0; + #[cfg(feature = "orchard")] + { + prior_orchard_tree_size = 0; + } + } else if let Some(meta) = db_data.block_metadata(height - 1).map_err(Error::Wallet)? { + prior_sapling_tree_size = meta.sapling_tree_size().map(u64::from).unwrap_or(0); + #[cfg(feature = "orchard")] + { + prior_orchard_tree_size = meta.orchard_tree_size().map(u64::from).unwrap_or(0); + } + } else { + let state = download_chain_state(client, height - 1).await?; + prior_sapling_tree_size = state.final_sapling_tree().tree_size(); + #[cfg(feature = "orchard")] + { + prior_orchard_tree_size = state.final_orchard_tree().tree_size(); + } + } + + Ok(TxBundlePositions { + sapling_base: Some( + prior_sapling_tree_size + + block.vtx[..tx_index] + .iter() + .map(|tx| tx.outputs.len() as u64) + .sum::(), + ), + #[cfg(feature = "orchard")] + orchard_base: Some( + prior_orchard_tree_size + + block.vtx[..tx_index] + .iter() + .map(|tx| tx.actions.len() as u64) + .sum::(), + ), + }) +} + +/// Services a [`TransactionDataRequest::TransactionsInvolvingAddress`] request by +/// querying lightwalletd for transactions touching the requested address in the requested +/// block range, and storing each result via [`store_raw_transaction`]. +/// +/// The two output-status filters use different gRPC endpoints: +/// `OutputStatusFilter::All` uses `get_taddress_transactions` (streams all transactions +/// touching the address); `OutputStatusFilter::Unspent` uses `get_address_utxos_stream` +/// plus per-txid lookups (as the UTXO endpoint does not return full transactions). +/// +/// The caller is responsible for invoking `notify_address_checked` after this function +/// returns successfully, regardless of whether any transactions were found, so that the +/// per-output `max_observed_unspent_height` cursor advances and the request stops +/// re-surfacing. +#[cfg(feature = "transparent-inputs")] +async fn service_transactions_involving_address( + client: &mut CompactTxStreamerClient, + params: &P, + db_data: &mut DbT, + request: crate::data_api::TransactionsInvolvingAddress, +) -> Result<(), Error::Error, TrErr>> +where + P: Parameters + Send + 'static, + ChT: GrpcService, + ChT::Error: Into, + ChT::ResponseBody: Body + Send + 'static, + ::Error: Into + Send, + DbT: WalletWrite, + DbT::AccountId: Copy + ConditionallySelectable + Default + Send + 'static, + DbT::Error: std::error::Error + Send + Sync + 'static, +{ + let chain_tip = db_data + .chain_height() + .map_err(Error::Wallet)? + .ok_or(Error::MisbehavingServer)?; + let end_height_exclusive = request.block_range_end().unwrap_or(chain_tip + 1); + if end_height_exclusive <= request.block_range_start() { + return Ok(()); + } + + match request.output_status_filter() { + OutputStatusFilter::All => { + if matches!(request.tx_status_filter(), TransactionStatusFilter::Mempool) { + return Ok(()); + } + + let mut stream = client + .get_taddress_transactions(TransparentAddressBlockFilter { + address: request.address().encode(params), + range: Some(service::BlockRange { + start: Some(BlockId { + height: u32::from(request.block_range_start()).into(), + hash: vec![], + }), + end: Some(BlockId { + height: u32::from(end_height_exclusive - 1).into(), + hash: vec![], + }), + pool_types: vec![], + }), + }) + .await? + .into_inner(); + while let Some(raw) = stream.try_next().await.map_err(Error::Server)? { + store_raw_transaction(client, params, db_data, &raw).await?; + } + } + OutputStatusFilter::Unspent => { + let mut stream = client + .get_address_utxos_stream(service::GetAddressUtxosArg { + addresses: vec![request.address().encode(params)], + start_height: request.block_range_start().into(), + max_entries: 0, + }) + .await? + .into_inner(); + let mut txids = BTreeSet::new(); + while let Some(reply) = stream.try_next().await.map_err(Error::Server)? { + let height = + BlockHeight::try_from(reply.height).map_err(|_| Error::MisbehavingServer)?; + if height < end_height_exclusive { + txids.insert( + reply.txid[..] + .try_into() + .map(TxId::from_bytes) + .map_err(|_| Error::MisbehavingServer)?, + ); + } + } + + for txid in &txids { + if let Some(raw) = fetch_raw_transaction(client, *txid).await? { + store_raw_transaction(client, params, db_data, &raw).await?; + } + } + } + } + + Ok(()) +} + async fn update_subtree_roots( client: &mut CompactTxStreamerClient, db_data: &mut DbT, diff --git a/zcash_client_memory/Cargo.toml b/zcash_client_memory/Cargo.toml index 285b4cce42..49afa0e4a8 100644 --- a/zcash_client_memory/Cargo.toml +++ b/zcash_client_memory/Cargo.toml @@ -97,6 +97,9 @@ test-dependencies = [ "incrementalmerkletree/test-dependencies" ] +## Enables integration testing of the sync module's enhancement logic. +sync = ["zcash_client_backend/sync", "transparent-inputs"] + ## Enables receiving transparent funds and sending to transparent recipients transparent-inputs = [ "dep:bip32", diff --git a/zcash_client_memory/src/types/data_requests.rs b/zcash_client_memory/src/types/data_requests.rs index 1cf67c5b4a..0448375d59 100644 --- a/zcash_client_memory/src/types/data_requests.rs +++ b/zcash_client_memory/src/types/data_requests.rs @@ -14,6 +14,24 @@ impl TransactionDataRequestQueue { pub fn queue_status_retrieval(&mut self, txid: &TxId) { self.0.push_back(TransactionDataRequest::GetStatus(*txid)); } + + pub fn queue_enhancement(&mut self, txid: &TxId) { + self.0.push_back(TransactionDataRequest::Enhancement(*txid)); + } + + /// Removes any pending [`TransactionDataRequest::Enhancement`] entries for + /// the given txid. + /// + /// This deliberately does NOT remove [`TransactionDataRequest::GetStatus`] + /// entries: `store_decrypted_tx` queues fresh `GetStatus` requests for + /// unmined transparent-bundle transactions, and those need to survive the + /// end-of-function cleanup so the sync orchestrator can later poll for + /// their mined status. Removing them here would silently drop the work + /// `store_decrypted_tx` just queued. + pub fn remove_enhancement_entries_for_txid(&mut self, txid: &TxId) { + self.0 + .retain(|req| !matches!(req, TransactionDataRequest::Enhancement(id) if id == txid)); + } } impl Deref for TransactionDataRequestQueue { @@ -24,6 +42,64 @@ impl Deref for TransactionDataRequestQueue { } } +#[cfg(test)] +mod tests { + use super::*; + + fn make_txid(byte: u8) -> TxId { + TxId::from_bytes([byte; 32]) + } + + /// Regression test for the bug where the end-of-`store_decrypted_tx` + /// cleanup wiped both `Enhancement` and `GetStatus` entries for the + /// stored txid, silently dropping `GetStatus` requests that + /// `store_decrypted_tx` had just queued for unmined transparent-bundle + /// transactions. + #[test] + fn remove_enhancement_entries_for_txid_preserves_get_status() { + let tx_a = make_txid(1); + let tx_b = make_txid(2); + let mut queue = TransactionDataRequestQueue::new(); + queue.queue_enhancement(&tx_a); + queue.queue_status_retrieval(&tx_a); + queue.queue_enhancement(&tx_b); + queue.queue_status_retrieval(&tx_b); + + queue.remove_enhancement_entries_for_txid(&tx_a); + + let remaining: Vec<_> = queue.0.iter().collect(); + assert_eq!( + remaining.len(), + 3, + "should have removed exactly one entry (the Enhancement for tx_a)" + ); + assert!( + remaining + .iter() + .any(|r| matches!(r, TransactionDataRequest::GetStatus(t) if *t == tx_a)), + "GetStatus(tx_a) must survive remove_enhancement_entries_for_txid(&tx_a)" + ); + assert!( + remaining + .iter() + .any(|r| matches!(r, TransactionDataRequest::Enhancement(t) if *t == tx_b)), + "Enhancement(tx_b) must survive (different txid)" + ); + assert!( + remaining + .iter() + .any(|r| matches!(r, TransactionDataRequest::GetStatus(t) if *t == tx_b)), + "GetStatus(tx_b) must survive (different txid)" + ); + assert!( + !remaining + .iter() + .any(|r| matches!(r, TransactionDataRequest::Enhancement(t) if *t == tx_a)), + "Enhancement(tx_a) should have been removed" + ); + } +} + mod serialization { use super::*; use crate::{error::Error, proto::memwallet as proto, read_optional}; diff --git a/zcash_client_memory/src/types/notes/received.rs b/zcash_client_memory/src/types/notes/received.rs index 0d07afd466..f3c4061447 100644 --- a/zcash_client_memory/src/types/notes/received.rs +++ b/zcash_client_memory/src/types/notes/received.rs @@ -11,6 +11,7 @@ use zcash_primitives::transaction::TxId; use zcash_protocol::{PoolType, ShieldedProtocol::Sapling, memo::Memo}; use zcash_client_backend::{ + DecryptedOutput, TransferType, data_api::{ReceivedNotes, SentTransactionOutput}, wallet::{Note, NoteId, Recipient, WalletSaplingOutput}, }; @@ -173,6 +174,87 @@ impl ReceivedNote { recipient_key_scope: output.recipient_key_scope(), } } + + /// Constructs a [`ReceivedNote`] from a Sapling [`DecryptedOutput`] produced by the + /// transaction-enhancement path. + /// + /// `is_change` and `recipient_key_scope` are derived from the output's + /// [`TransferType`]: [`TransferType::WalletInternal`] is change in the + /// internal scope, [`TransferType::Incoming`] is a regular receive in the + /// external scope. [`TransferType::Outgoing`] is rejected because outgoing + /// outputs are not stored as received notes. + /// + /// `nf` is populated from `output.nullifier_bytes()` when available; + /// `commitment_tree_position` is populated from + /// `output.note_commitment_tree_position()` when available. Both may be + /// `None` if the enhancement path could not derive them (e.g. Sapling + /// outputs decrypted via `decrypt_and_store_transaction` without bundle + /// position information). + pub fn from_decrypted_sapling_output( + note_id: NoteId, + output: &DecryptedOutput, + ) -> Result { + let (is_change, recipient_key_scope) = match output.transfer_type() { + TransferType::WalletInternal => (true, Some(Scope::Internal)), + TransferType::Incoming => (false, Some(Scope::External)), + TransferType::Outgoing => { + return Err(Error::Other( + "outgoing outputs are not stored as received notes".to_owned(), + )); + } + }; + let nf = output + .nullifier_bytes() + .and_then(|bytes| sapling::Nullifier::from_slice(&bytes).ok()) + .map(Nullifier::Sapling); + Ok(ReceivedNote { + note_id, + txid: *note_id.txid(), + output_index: output.index() as u32, + account_id: *output.account(), + note: Note::Sapling(output.note().clone()), + nf, + is_change, + memo: Memo::try_from(output.memo().clone())?, + commitment_tree_position: output.note_commitment_tree_position(), + recipient_key_scope, + }) + } + + /// Constructs a [`ReceivedNote`] from an Orchard [`DecryptedOutput`] produced by the + /// transaction-enhancement path. See [`Self::from_decrypted_sapling_output`] for the + /// `is_change` / `recipient_key_scope` derivation. + #[cfg(feature = "orchard")] + pub fn from_decrypted_orchard_output( + note_id: NoteId, + output: &DecryptedOutput, + ) -> Result { + let (is_change, recipient_key_scope) = match output.transfer_type() { + TransferType::WalletInternal => (true, Some(Scope::Internal)), + TransferType::Incoming => (false, Some(Scope::External)), + TransferType::Outgoing => { + return Err(Error::Other( + "outgoing outputs are not stored as received notes".to_owned(), + )); + } + }; + let nf = output + .nullifier_bytes() + .and_then(|bytes| Option::from(orchard::note::Nullifier::from_bytes(&bytes))) + .map(Nullifier::Orchard); + Ok(ReceivedNote { + note_id, + txid: *note_id.txid(), + output_index: output.index() as u32, + account_id: *output.account(), + note: Note::Orchard(*output.note()), + nf, + is_change, + memo: Memo::try_from(output.memo().clone())?, + commitment_tree_position: output.note_commitment_tree_position(), + recipient_key_scope, + }) + } } impl From @@ -396,3 +478,93 @@ mod serialization { } } } + +#[cfg(test)] +mod tests { + use super::*; + use zcash_protocol::memo::MemoBytes; + + /// Builds a stub Sapling note for use in classification tests. Field + /// values are arbitrary; the test only cares about how the constructor + /// maps `transfer_type` to `is_change` and `recipient_key_scope`. + fn stub_sapling_note() -> sapling::Note { + sapling::note::Note::from_parts( + sapling::PaymentAddress::from_bytes(&[ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x8e, 0x11, + 0x9d, 0x72, 0x99, 0x2b, 0x56, 0x0d, 0x26, 0x50, 0xff, 0xe0, 0xbe, 0x7f, 0x35, 0x42, + 0xfd, 0x97, 0x00, 0x3c, 0xb7, 0xcc, 0x3a, 0xbf, 0xf8, 0x1a, 0x7f, 0x90, 0x37, 0xf3, + 0xea, + ]) + .unwrap(), + sapling::value::NoteValue::from_raw(99), + sapling::Rseed::AfterZip212([0; 32]), + ) + } + + fn stub_note_id() -> NoteId { + NoteId::new(TxId::from_bytes([0; 32]), Sapling, 0) + } + + fn make_decrypted_output( + transfer_type: TransferType, + ) -> DecryptedOutput { + DecryptedOutput::new( + 0, + stub_sapling_note(), + AccountId::from(0), + MemoBytes::empty(), + transfer_type, + ) + } + + /// Regression test for the bug where `TransferType::Incoming` shielded + /// outputs were stored via `from_sent_tx_output`, which hardcoded + /// `is_change = true` and `recipient_key_scope = Some(Scope::Internal)`, + /// silently reclassifying ordinary incoming receipts as change in the + /// internal scope. + #[test] + fn from_decrypted_sapling_output_incoming_is_external_receive() { + let output = make_decrypted_output(TransferType::Incoming); + let received = ReceivedNote::from_decrypted_sapling_output(stub_note_id(), &output) + .expect("Incoming should produce a valid ReceivedNote"); + assert!( + !received.is_change, + "Incoming output should NOT be marked as change" + ); + assert_eq!( + received.recipient_key_scope, + Some(Scope::External), + "Incoming output should be in the external scope" + ); + } + + /// Sanity check that `WalletInternal` still maps to change in the + /// internal scope, since we use the same constructor for both transfer + /// types. + #[test] + fn from_decrypted_sapling_output_internal_is_change() { + let output = make_decrypted_output(TransferType::WalletInternal); + let received = ReceivedNote::from_decrypted_sapling_output(stub_note_id(), &output) + .expect("WalletInternal should produce a valid ReceivedNote"); + assert!( + received.is_change, + "WalletInternal output should be marked as change" + ); + assert_eq!( + received.recipient_key_scope, + Some(Scope::Internal), + "WalletInternal output should be in the internal scope" + ); + } + + /// `Outgoing` outputs are not received notes and must be rejected. + #[test] + fn from_decrypted_sapling_output_outgoing_is_rejected() { + let output = make_decrypted_output(TransferType::Outgoing); + let result = ReceivedNote::from_decrypted_sapling_output(stub_note_id(), &output); + assert!( + result.is_err(), + "Outgoing output should be rejected by from_decrypted_sapling_output" + ); + } +} diff --git a/zcash_client_memory/src/wallet_read.rs b/zcash_client_memory/src/wallet_read.rs index a48f5e3b79..b4eaed1c63 100644 --- a/zcash_client_memory/src/wallet_read.rs +++ b/zcash_client_memory/src/wallet_read.rs @@ -508,11 +508,16 @@ impl WalletRead for MemoryWalletDb

{ tracing::debug!("get_transaction: {:?}", txid); self.tx_table .get(&txid) - .map(|tx| (tx.status(), tx.expiry_height(), tx.raw())) + .filter(|tx| { + if tx.raw().is_none() { + tracing::debug!("get_transaction({txid:?}): entry exists but raw is None; treating as not-yet-enhanced"); + false + } else { + true + } + }) + .map(|tx| (tx.status(), tx.expiry_height(), tx.raw().unwrap())) .map(|(status, expiry_height, raw)| { - let raw = raw.ok_or_else(|| { - Self::Error::CorruptedData("Transaction raw data not found".to_string()) - })?; // We need to provide a consensus branch ID so that pre-v5 `Transaction` structs // (which don't commit directly to one) can store it internally. diff --git a/zcash_client_memory/src/wallet_write.rs b/zcash_client_memory/src/wallet_write.rs index 5801193aba..70c7f036d8 100644 --- a/zcash_client_memory/src/wallet_write.rs +++ b/zcash_client_memory/src/wallet_write.rs @@ -319,6 +319,7 @@ impl WalletWrite for MemoryWalletDb

{ for transaction in block.transactions().iter() { let txid = transaction.txid(); + self.transaction_data_request_queue.queue_enhancement(&txid); // Mark the Sapling nullifiers of the spent notes as spent in the `sapling_spends` map. for spend in transaction.sapling_spends() { @@ -355,6 +356,11 @@ impl WalletWrite for MemoryWalletDb

{ .and_then(|(height, tx_idx)| self.tx_locator.get(*height, *tx_idx)) .copied(); + if let Some(spending_txid) = spent_in { + self.transaction_data_request_queue + .queue_enhancement(&spending_txid); + } + self.insert_received_sapling_note(note_id, output, spent_in); } @@ -378,6 +384,11 @@ impl WalletWrite for MemoryWalletDb

{ .and_then(|(height, tx_idx)| self.tx_locator.get(*height, *tx_idx)) .copied(); + if let Some(spending_txid) = spent_in { + self.transaction_data_request_queue + .queue_enhancement(&spending_txid); + } + self.insert_received_orchard_note(note_id, output, spent_in) } @@ -595,6 +606,14 @@ impl WalletWrite for MemoryWalletDb

{ Ok(()) } + fn notify_wallet_note_positions( + &mut self, + block_range: Range, + wallet_note_positions: &[(ShieldedProtocol, Position)], + ) -> Result<(), Self::Error> { + self.scan_complete(block_range, wallet_note_positions) + } + /// Adds a transparent UTXO received by the wallet to the data store. fn put_received_transparent_utxo( &mut self, @@ -709,7 +728,20 @@ impl WalletWrite for MemoryWalletDb

{ ); } TransferType::Incoming => { - todo!("store decrypted tx sapling incoming") + // Construct the ReceivedNote directly from the DecryptedOutput + // so the external-scope semantics are preserved. Routing through + // `from_sent_tx_output` would force `is_change = true` and + // `recipient_key_scope = Some(Scope::Internal)`, both of which + // are wrong for an external incoming receipt. + let note_id = NoteId::new( + d_tx.tx().txid(), + Sapling, + u16::try_from(output.index()) + .expect("output indices are representable as u16"), + ); + self.received_notes.insert_received_note( + ReceivedNote::from_decrypted_sapling_output(note_id, output)?, + ); } } } @@ -780,7 +812,20 @@ impl WalletWrite for MemoryWalletDb

{ ); } TransferType::Incoming => { - todo!("store decrypted tx orchard incoming") + // Construct the ReceivedNote directly from the DecryptedOutput + // so the external-scope semantics are preserved. Routing through + // `from_sent_tx_output` would force `is_change = true` and + // `recipient_key_scope = Some(Scope::Internal)`, both of which + // are wrong for an external incoming receipt. + let note_id = NoteId::new( + d_tx.tx().txid(), + Orchard, + u16::try_from(output.index()) + .expect("output indices are representable as u16"), + ); + self.received_notes.insert_received_note( + ReceivedNote::from_decrypted_orchard_output(note_id, output)?, + ); } } } @@ -937,6 +982,14 @@ impl WalletWrite for MemoryWalletDb

{ .queue_status_retrieval(&d_tx.tx().txid()); } } + + // Remove enhancement requests for this transaction now that it has been processed. + // GetStatus entries are intentionally preserved: the queue_status_retrieval calls + // earlier in this function need to survive past this cleanup so the sync orchestrator + // can later poll for unmined transparent-bundle transactions. + self.transaction_data_request_queue + .remove_enhancement_entries_for_txid(&d_tx.tx().txid()); + Ok(()) } diff --git a/zcash_client_sqlite/CHANGELOG.md b/zcash_client_sqlite/CHANGELOG.md index 07b7731dd1..7c208bb369 100644 --- a/zcash_client_sqlite/CHANGELOG.md +++ b/zcash_client_sqlite/CHANGELOG.md @@ -27,6 +27,25 @@ workspace. - `zcash_client_sqlite::AccountRef` is now public. - Implement standalone P2SH address import support - `impl zcash_client_backend::data_api::WalletWrite::import_standalone_transparent_script()` +- `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 - Migrated to `orchard 0.12`, `sapling-crypto 0.6`. @@ -35,6 +54,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/Cargo.toml b/zcash_client_sqlite/Cargo.toml index 52508e31f2..cc8fa52621 100644 --- a/zcash_client_sqlite/Cargo.toml +++ b/zcash_client_sqlite/Cargo.toml @@ -130,6 +130,9 @@ test-dependencies = [ "zcash_client_backend/test-dependencies", ] +## Enables integration testing of the sync module's enhancement logic. +sync = ["zcash_client_backend/sync", "transparent-inputs"] + ## Enables receiving transparent funds and sending to transparent recipients transparent-inputs = [ "dep:bip32", diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 798c359bd1..c6f172e78b 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -1457,6 +1457,33 @@ impl, P: consensus::Parameters, CL: Clock, R: }) } + fn notify_wallet_note_positions( + &mut self, + block_range: std::ops::Range, + wallet_note_positions: &[(ShieldedProtocol, incrementalmerkletree::Position)], + ) -> Result<(), Self::Error> { + self.transactionally(|wdb| { + wallet::scanning::scan_complete( + wdb.conn.0, + &wdb.params, + block_range, + wallet_note_positions, + ) + }) + } + + fn prune_tracked_nullifiers(&mut self, pruning_depth: u32) -> Result<(), Self::Error> { + self.transactionally(|wdb| { + if let Some(meta) = wallet::block_fully_scanned(wdb.conn.0, &wdb.params)? { + wallet::prune_nullifier_map( + wdb.conn.0, + meta.block_height().saturating_sub(pruning_depth), + )?; + } + Ok(()) + }) + } + fn set_tx_trust(&mut self, txid: TxId, trusted: bool) -> Result<(), Self::Error> { self.transactionally(|wdb| wallet::set_tx_trust(wdb.conn.0, txid, trusted)) } diff --git a/zcash_client_sqlite/src/testing/pool.rs b/zcash_client_sqlite/src/testing/pool.rs index 0fcaa40a21..73c95fb9f3 100644 --- a/zcash_client_sqlite/src/testing/pool.rs +++ b/zcash_client_sqlite/src/testing/pool.rs @@ -348,6 +348,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.rs b/zcash_client_sqlite/src/wallet.rs index 88f3ecce24..31ca4a1a09 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -3934,12 +3934,38 @@ pub(crate) fn queue_tx_retrieval( dependent_tx_ref: Option, ) -> Result<(), SqliteClientError> { // Add an entry to the transaction retrieval queue if it would not be redundant. + // + // The `mined_height IS NULL` predicate on the IIF guard means: even if the + // transaction already has `raw` set, a mined tx still gets queued as + // `Enhancement` rather than `GetStatus`. This is deliberate. Under External-only + // batch scanning, a mined tx may have been recorded only via the scan path + // (`put_tx_meta`), and `store_decrypted_tx` is the step that actually runs + // `decrypt_transaction` under all key scopes to recover Internal-scope change + // outputs and OVK metadata. `GetStatus` alone would not do that work, so we only + // treat mempool/unmined transactions (whose state was already fully populated + // via `store_transaction_to_be_sent`) as pure status-refresh candidates. + // + // Note: under per-chunk sync orchestration, a transaction may be re-queued for + // enhancement multiple times during a single sync pass — once when + // `query_nullifier_map` cascade-inserts its row during an earlier chunk's + // enhancement, and again when its enclosing chunk is finally scanned and + // `put_blocks` calls this function for every wallet-touching tx. The + // `service_transaction_data_requests` stabilization check catches this as + // no-progress after one redundant pass, and the idempotent `store_decrypted_tx` + // upserts make each enhancement safe to repeat. The performance cost is + // bounded by the number of wallet-touching transactions per chunk. let mut stmt_insert_tx = conn.prepare_cached( "INSERT INTO tx_retrieval_queue (txid, query_type, dependent_transaction_id) SELECT :txid, IIF( - EXISTS (SELECT 1 FROM transactions WHERE txid = :txid AND raw IS NOT NULL), + EXISTS ( + SELECT 1 + FROM transactions + WHERE txid = :txid + AND raw IS NOT NULL + AND mined_height IS NULL + ), :status_type, :enhancement_type ), @@ -3947,7 +3973,13 @@ pub(crate) fn queue_tx_retrieval( ON CONFLICT (txid) DO UPDATE SET query_type = IIF( - EXISTS (SELECT 1 FROM transactions WHERE txid = :txid AND raw IS NOT NULL), + EXISTS ( + SELECT 1 + FROM transactions + WHERE txid = :txid + AND raw IS NOT NULL + AND mined_height IS NULL + ), :status_type, :enhancement_type ), @@ -3978,28 +4010,61 @@ pub(crate) fn transaction_data_requests( // For transactions with a known expiry height of 0, we will continue to query indefinitely. // Such transactions should be rebroadcast by the wallet until they are either mined or // conflict with another mined transaction. + // The results are ordered by (mined_height, tx_index, txid) so that enhancement + // downstream processes transactions in chain order. This is important for the + // cascade-discovery mechanism in `detect_*_spend`: when tx A spends a change note + // created in tx B (where B is at an earlier block), processing B first ensures + // that by the time A is enhanced, B's change note is already stored in + // `received_notes` and A's `mark_notes_spent` call can successfully mark the + // spend. Without a deterministic block-ordered sequence, A might be processed + // before B and end up with its own `raw` set but the spend relationship + // unrecorded until B's enhancement cascades back via `put_received_note`'s + // `spent_in` param — a window during which `v_transactions` computes a + // transiently-wrong balance delta for A. + // + // The `txid` tiebreaker ensures stable ordering when multiple transactions + // have the same `(mined_height, tx_index)` — most importantly when multiple + // mempool transactions have NULL `tx_index`. This avoids thrashing in the + // stabilization check in `sync::service_transaction_data_requests`, which + // uses `Vec` equality as a termination condition. + // + // Unmined transactions (`mined_height IS NULL`) and cascade-inserted entries + // in `tx_retrieval_queue` without a matching `transactions` row sort to the + // end via `COALESCE(..., 4294967295)`. let mut tx_retrieval_stmt = conn.prepare_cached( - "SELECT txid, query_type FROM tx_retrieval_queue - UNION - SELECT txid, :status_type - FROM transactions - WHERE mined_height IS NULL - AND ( - -- we have no confirmation of expiry - confirmed_unmined_at_height IS NULL - -- a nonzero expiry height is known, and we have confirmation that the transaction was - -- not unmined as of a height greater than or equal to that expiry height - OR ( - expiry_height > 0 - AND confirmed_unmined_at_height < expiry_height - ) - -- the expiry height is unknown and the default expiry height for it is not yet in the - -- stable block range according to the PRUNING_DEPTH - OR ( - expiry_height IS NULL - AND confirmed_unmined_at_height < min_observed_height + :certainty_depth + "SELECT txid, query_type FROM ( + SELECT trq.txid AS txid, + trq.query_type AS query_type, + COALESCE(t.mined_height, 4294967295) AS sort_height, + COALESCE(t.tx_index, 0) AS sort_idx + FROM tx_retrieval_queue trq + LEFT JOIN transactions t ON t.txid = trq.txid + UNION + SELECT t.txid AS txid, + :status_type AS query_type, + COALESCE(t.mined_height, 4294967295) AS sort_height, + COALESCE(t.tx_index, 0) AS sort_idx + FROM transactions t + WHERE t.mined_height IS NULL + AND ( + -- we have no confirmation of expiry + t.confirmed_unmined_at_height IS NULL + -- a nonzero expiry height is known, and we have confirmation that the + -- transaction was not unmined as of a height greater than or equal to + -- that expiry height + OR ( + t.expiry_height > 0 + AND t.confirmed_unmined_at_height < t.expiry_height + ) + -- the expiry height is unknown and the default expiry height for it is + -- not yet in the stable block range according to the PRUNING_DEPTH + OR ( + t.expiry_height IS NULL + AND t.confirmed_unmined_at_height < t.min_observed_height + :certainty_depth + ) ) - )", + ) + ORDER BY sort_height, sort_idx, txid", )?; let result = tx_retrieval_stmt @@ -4343,7 +4408,7 @@ pub(crate) fn query_nullifier_map>( // have been created during the same scan that the locator was added to the nullifier // map, but it would not happen if the transaction in question spent the note with no // change or explicit in-wallet recipient. - put_tx_meta( + let tx_ref = put_tx_meta( conn, &WalletTx::new( txid, @@ -4356,12 +4421,24 @@ pub(crate) fn query_nullifier_map>( vec![], ), height, - ) - .map(Some) + )?; + + // Queue the spending transaction for enhancement so that + // decrypt_and_store_transaction can recover change notes (IVK Internal) + // and outgoing details (OVK) from the full transaction bytes. This is + // needed because scanning may use only External IVK for batch trial + // decryption, deferring Internal/OVK decryption to the enhancement phase. + queue_tx_retrieval(conn, std::iter::once(txid), Some(tx_ref))?; + + Ok(Some(tx_ref)) } /// Deletes from the nullifier map any entries with a locator referencing a block height /// lower than the pruning height. +/// +/// Callers should only prune locators once any pending transaction enhancement and transparent +/// address-history checks that may rely on these entries to link late-discovered notes have +/// converged past `block_height`. pub(crate) fn prune_nullifier_map( conn: &rusqlite::Transaction<'_>, block_height: BlockHeight, diff --git a/zcash_client_sqlite/src/wallet/commitment_tree.rs b/zcash_client_sqlite/src/wallet/commitment_tree.rs index 6f4104c964..e2aba0b764 100644 --- a/zcash_client_sqlite/src/wallet/commitment_tree.rs +++ b/zcash_client_sqlite/src/wallet/commitment_tree.rs @@ -11,7 +11,7 @@ use std::{ use incrementalmerkletree::{Address, Hashable, Level, Position, Retention}; use shardtree::{ - LocatedPrunableTree, LocatedTree, PrunableTree, RetentionFlags, + LocatedPrunableTree, LocatedTree, PrunableTree, RetentionFlags, ShardTree, error::{QueryError, ShardTreeError}, store::{Checkpoint, ShardStore, TreeState}, }; @@ -1140,6 +1140,77 @@ pub(crate) fn check_witnesses( Ok(scan_ranges) } +fn mark_positions( + tree: &mut ShardTree, + positions: impl IntoIterator, +) -> Result<(), ShardTreeError> +where + S: ShardStore, + S::H: Clone + PartialEq + Hashable, +{ + for position in positions { + if tree.get_marked_leaf(position)?.is_some() { + continue; + } + + let leaf = tree + .store() + .get_shard(ShardTree::::subtree_addr(position)) + .map_err(ShardTreeError::Storage)? + .and_then(|subtree| { + subtree + .value_at_position(position) + .map(|(leaf, _retention)| leaf.clone()) + }); + + if let Some(leaf) = leaf { + tree.batch_insert(position, std::iter::once((leaf, Retention::Marked)))?; + } else { + tracing::warn!( + "mark_positions: leaf not found at position {position:?}; \ + note at this position will not be spendable until the shard is populated" + ); + } + } + + Ok(()) +} + +pub(crate) fn mark_wallet_note_positions( + conn: &rusqlite::Transaction<'_>, + wallet_note_positions: &[(ShieldedProtocol, Position)], +) -> Result<(), SqliteClientError> { + let sapling_positions = wallet_note_positions + .iter() + .filter_map(|(protocol, position)| { + (*protocol == ShieldedProtocol::Sapling).then_some(*position) + }) + .collect::>(); + + if !sapling_positions.is_empty() { + let mut tree = sapling_tree(conn).map_err(SqliteClientError::CommitmentTree)?; + mark_positions(&mut tree, sapling_positions).map_err(SqliteClientError::CommitmentTree)?; + } + + #[cfg(feature = "orchard")] + { + let orchard_positions = wallet_note_positions + .iter() + .filter_map(|(protocol, position)| { + (*protocol == ShieldedProtocol::Orchard).then_some(*position) + }) + .collect::>(); + + if !orchard_positions.is_empty() { + let mut tree = orchard_tree(conn).map_err(SqliteClientError::CommitmentTree)?; + mark_positions(&mut tree, orchard_positions) + .map_err(SqliteClientError::CommitmentTree)?; + } + } + + Ok(()) +} + #[cfg(test)] mod tests { use tempfile::NamedTempFile; diff --git a/zcash_client_sqlite/src/wallet/db.rs b/zcash_client_sqlite/src/wallet/db.rs index 7da8e557a4..baeca3254d 100644 --- a/zcash_client_sqlite/src/wallet/db.rs +++ b/zcash_client_sqlite/src/wallet/db.rs @@ -1083,7 +1083,47 @@ LEFT JOIN blocks ON blocks.height = transactions.mined_height LEFT JOIN sent_note_counts ON sent_note_counts.account_id = notes.account_id AND sent_note_counts.transaction_id = notes.transaction_id -GROUP BY notes.account_id, notes.transaction_id"; +GROUP BY notes.account_id, notes.transaction_id +-- NOTE: This HAVING clause is duplicated in the inline VIEW_TRANSACTIONS +-- constant at wallet/db.rs. If you modify it here, update that constant +-- as well (and vice versa). +-- +-- Hide transactions whose DB state is transiently inconsistent during +-- sync, to prevent v_transactions from reporting a wrong balance delta +-- in the brief window between when pieces of the transaction are +-- recorded and when all of them are in place. Two disjoint failure +-- modes are caught: +-- +-- Cause A (intra-tx scan-to-enhance window): scanning has recorded the +-- wallet's spent inputs via `mark_notes_spent`, but enhancement has +-- not yet recovered the Internal-scope change output via +-- decrypt_transaction → put_received_note. `raw IS NULL` signals that +-- enhancement's `put_tx_data` has not yet run. If the tx has wallet +-- spends in this state, its computed delta is `-total_input` instead +-- of the final `-(external_sent + fee)`. +-- +-- Cause B (post-own-enhance, pre-cascade): the tx's own enhancement +-- has run and stored change notes (so `raw IS NOT NULL`) but the +-- cascade that marks its wallet-side spend has not yet fired. This +-- happens when a downstream-in-chain-order dependency transaction +-- has not yet been enhanced, so when THIS tx's `mark_notes_spent` +-- looked up its spend nullifier it found nothing in `received_notes`. +-- The cascade from the dependency's eventual enhancement then records +-- the spend relationship via `put_received_note`'s `spent_in` param. +-- We detect this inconsistent state via the invariant: 'a transaction +-- with a wallet change note can only be wallet-sent, therefore it +-- must also have wallet spend markings to be consistent.' A nonzero +-- `change_note_count` with a zero `spent_note_count` is a violation +-- of that invariant. +-- +-- Pure receives (no change notes, no wallet spends) always satisfy +-- both conditions and are immediately visible. Wallet-created mempool +-- transactions have `raw` set and both sides populated at creation +-- time, so they also pass immediately. +HAVING NOT ( + (transactions.raw IS NULL AND SUM(notes.spent_note_count) > 0) + OR (SUM(notes.change_note_count) > 0 AND SUM(notes.spent_note_count) = 0) +)"; /// Selects all outputs received by the wallet, plus any outputs sent from the wallet to /// external recipients. diff --git a/zcash_client_sqlite/src/wallet/init/migrations.rs b/zcash_client_sqlite/src/wallet/init/migrations.rs index 88d28c3745..6c8ff47d3b 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations.rs @@ -45,6 +45,7 @@ mod utxos_to_txos; mod v_received_output_spends_account; mod v_sapling_shard_unscanned_ranges; mod v_transactions_additional_totals; +mod v_transactions_filter_intermediate_state; mod v_transactions_net; mod v_transactions_note_uniqueness; mod v_transactions_shielding_balance; @@ -221,6 +222,7 @@ pub(super) fn all_migrations< Box::new(account_delete_cascade::Migration), Box::new(v_tx_outputs_key_scopes::Migration), Box::new(standalone_p2sh::Migration), + Box::new(v_transactions_filter_intermediate_state::Migration), ] } @@ -364,6 +366,7 @@ pub const V_0_19_0: &[Uuid] = &[account_delete_cascade::MIGRATION_ID]; pub const CURRENT_LEAF_MIGRATIONS: &[Uuid] = &[ v_tx_outputs_key_scopes::MIGRATION_ID, standalone_p2sh::MIGRATION_ID, + v_transactions_filter_intermediate_state::MIGRATION_ID, ]; pub(super) fn verify_network_compatibility( diff --git a/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_filter_intermediate_state.rs b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_filter_intermediate_state.rs new file mode 100644 index 0000000000..f351f64a00 --- /dev/null +++ b/zcash_client_sqlite/src/wallet/init/migrations/v_transactions_filter_intermediate_state.rs @@ -0,0 +1,231 @@ +//! This migration adds a filter to `v_transactions` that hides transactions in the +//! intermediate scan-only state, where compact-block scanning has recorded their input +//! spends but transaction enhancement has not yet stored their output change notes. +//! +//! Under the External-only batch scanning optimization, Internal-scope change notes +//! are discovered only during post-scan transaction enhancement. This creates a time +//! window between `put_tx_meta` (scan path, which does NOT set `raw`) and `put_tx_data` +//! (enhancement path, which does set `raw`). In that window, the `v_transactions` view +//! computes `account_balance_delta = SUM(received) - SUM(spent)` from an inconsistent +//! DB state: the input is marked spent but the change output isn't stored yet, so the +//! UI transiently displays `-total_input` instead of the final `-(external_sent + fee)`. +//! +//! The filter `WHERE transactions.raw IS NOT NULL` hides any transaction that hasn't +//! been through `put_tx_data` at least once, deferring its visibility to the UI until +//! enhancement has committed a consistent snapshot. + +use std::collections::HashSet; + +use schemerz_rusqlite::RusqliteMigration; +use uuid::Uuid; + +use crate::wallet::init::{WalletMigrationError, migrations::account_delete_cascade}; + +pub(super) const MIGRATION_ID: Uuid = Uuid::from_u128(0x25dad60f_ffc1_44a9_9e54_154c51749c28); + +const DEPENDENCIES: &[Uuid] = &[account_delete_cascade::MIGRATION_ID]; + +pub(super) struct Migration; + +impl schemerz::Migration for Migration { + fn id(&self) -> Uuid { + MIGRATION_ID + } + + fn dependencies(&self) -> HashSet { + DEPENDENCIES.iter().copied().collect() + } + + fn description(&self) -> &'static str { + "Adds a filter to v_transactions that hides transactions in the intermediate \ + scan-only state (inputs marked spent by scanning but outputs not yet recovered \ + by enhancement). This prevents the UI from displaying a transiently-wrong \ + balance delta during sync." + } +} + +impl RusqliteMigration for Migration { + type Error = WalletMigrationError; + + fn up(&self, transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { + transaction.execute_batch( + r#" + DROP VIEW v_transactions; + CREATE VIEW v_transactions AS + WITH + notes AS ( + -- Outputs received in this transaction + SELECT ro.account_id AS account_id, + ro.transaction_id AS transaction_id, + ro.pool AS pool, + id_within_pool_table, + ro.value AS value, + ro.value AS received_value, + 0 AS spent_value, + 0 AS spent_note_count, + CASE + WHEN ro.is_change THEN 1 + ELSE 0 + END AS change_note_count, + CASE + WHEN ro.is_change THEN 0 + ELSE 1 + END AS received_count, + CASE + WHEN (ro.memo IS NULL OR ro.memo = X'F6') + THEN 0 + ELSE 1 + END AS memo_present, + -- The wallet cannot receive transparent outputs in shielding transactions. + CASE + WHEN ro.pool = 0 + THEN 1 + ELSE 0 + END AS does_not_match_shielding + FROM v_received_outputs ro + UNION + -- Outputs spent in this transaction + SELECT ro.account_id AS account_id, + ros.transaction_id AS transaction_id, + ro.pool AS pool, + id_within_pool_table, + -ro.value AS value, + 0 AS received_value, + ro.value AS spent_value, + 1 AS spent_note_count, + 0 AS change_note_count, + 0 AS received_count, + 0 AS memo_present, + -- The wallet cannot spend shielded outputs in shielding transactions. + CASE + WHEN ro.pool != 0 + THEN 1 + ELSE 0 + END AS does_not_match_shielding + FROM v_received_outputs ro + JOIN v_received_output_spends ros + ON ros.pool = ro.pool + AND ros.received_output_id = ro.id_within_pool_table + ), + -- Obtain a count of the notes that the wallet created in each transaction, + -- not counting change notes. + sent_note_counts AS ( + SELECT sent_notes.from_account_id AS account_id, + sent_notes.transaction_id AS transaction_id, + COUNT(DISTINCT sent_notes.id) AS sent_notes, + SUM( + CASE + WHEN (sent_notes.memo IS NULL OR sent_notes.memo = X'F6' OR ro.transaction_id IS NOT NULL) + THEN 0 + ELSE 1 + END + ) AS memo_count + FROM sent_notes + LEFT JOIN v_received_outputs ro ON sent_notes.id = ro.sent_note_id + WHERE COALESCE(ro.is_change, 0) = 0 + GROUP BY account_id, sent_notes.transaction_id + ), + blocks_max_height AS ( + SELECT MAX(blocks.height) AS max_height FROM blocks + ) + SELECT accounts.uuid AS account_uuid, + transactions.mined_height AS mined_height, + transactions.txid AS txid, + transactions.tx_index AS tx_index, + transactions.expiry_height AS expiry_height, + transactions.raw AS raw, + SUM(notes.value) AS account_balance_delta, + SUM(notes.spent_value) AS total_spent, + SUM(notes.received_value) AS total_received, + transactions.fee AS fee_paid, + SUM(notes.change_note_count) > 0 AS has_change, + MAX(COALESCE(sent_note_counts.sent_notes, 0)) AS sent_note_count, + SUM(notes.received_count) AS received_note_count, + SUM(notes.memo_present) + MAX(COALESCE(sent_note_counts.memo_count, 0)) AS memo_count, + blocks.time AS block_time, + ( + transactions.mined_height IS NULL + AND transactions.expiry_height BETWEEN 1 AND blocks_max_height.max_height + ) AS expired_unmined, + SUM(notes.spent_note_count) AS spent_note_count, + ( + -- All of the wallet-spent and wallet-received notes are consistent with a + -- shielding transaction. + SUM(notes.does_not_match_shielding) = 0 + -- The transaction contains at least one wallet-spent output. + AND SUM(notes.spent_note_count) > 0 + -- The transaction contains at least one wallet-received note. + AND (SUM(notes.received_count) + SUM(notes.change_note_count)) > 0 + -- We do not know about any external outputs of the transaction. + AND MAX(COALESCE(sent_note_counts.sent_notes, 0)) = 0 + ) AS is_shielding, + transactions.trust_status + FROM notes + JOIN accounts ON accounts.id = notes.account_id + JOIN transactions ON transactions.id_tx = notes.transaction_id + LEFT JOIN blocks_max_height + LEFT JOIN blocks ON blocks.height = transactions.mined_height + LEFT JOIN sent_note_counts + ON sent_note_counts.account_id = notes.account_id + AND sent_note_counts.transaction_id = notes.transaction_id + GROUP BY notes.account_id, notes.transaction_id + -- NOTE: This HAVING clause is duplicated in the inline VIEW_TRANSACTIONS + -- constant at wallet/db.rs. If you modify it here, update that constant + -- as well (and vice versa). + -- + -- Hide transactions whose DB state is transiently inconsistent during + -- sync, to prevent v_transactions from reporting a wrong balance delta + -- in the brief window between when pieces of the transaction are + -- recorded and when all of them are in place. Two disjoint failure + -- modes are caught: + -- + -- Cause A (intra-tx scan-to-enhance window): scanning has recorded the + -- wallet's spent inputs via `mark_notes_spent`, but enhancement has + -- not yet recovered the Internal-scope change output via + -- decrypt_transaction → put_received_note. `raw IS NULL` signals that + -- enhancement's `put_tx_data` has not yet run. If the tx has wallet + -- spends in this state, its computed delta is `-total_input` instead + -- of the final `-(external_sent + fee)`. + -- + -- Cause B (post-own-enhance, pre-cascade): the tx's own enhancement + -- has run and stored change notes (so `raw IS NOT NULL`) but the + -- cascade that marks its wallet-side spend has not yet fired. This + -- happens when a downstream-in-chain-order dependency transaction + -- has not yet been enhanced, so when THIS tx's `mark_notes_spent` + -- looked up its spend nullifier it found nothing in `received_notes`. + -- The cascade from the dependency's eventual enhancement then records + -- the spend relationship via `put_received_note`'s `spent_in` param. + -- We detect this inconsistent state via the invariant: 'a transaction + -- with a wallet change note can only be wallet-sent, therefore it + -- must also have wallet spend markings to be consistent.' A nonzero + -- `change_note_count` with a zero `spent_note_count` is a violation + -- of that invariant. + -- + -- Pure receives (no change notes, no wallet spends) always satisfy + -- both conditions and are immediately visible. Wallet-created mempool + -- transactions have `raw` set and both sides populated at creation + -- time, so they also pass immediately. + HAVING NOT ( + (transactions.raw IS NULL AND SUM(notes.spent_note_count) > 0) + OR (SUM(notes.change_note_count) > 0 AND SUM(notes.spent_note_count) = 0) + ) + "#, + )?; + + Ok(()) + } + + fn down(&self, _transaction: &rusqlite::Transaction) -> Result<(), Self::Error> { + Err(WalletMigrationError::CannotRevert(MIGRATION_ID)) + } +} + +#[cfg(test)] +mod tests { + use crate::wallet::init::migrations::tests::test_migrate; + + #[test] + fn migrate() { + test_migrate(&[super::MIGRATION_ID]); + } +} diff --git a/zcash_client_sqlite/src/wallet/orchard.rs b/zcash_client_sqlite/src/wallet/orchard.rs index 512e9e4f16..9870fba0b9 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 f7a17c1c8d..59c97e18fc 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::() diff --git a/zcash_client_sqlite/src/wallet/scanning.rs b/zcash_client_sqlite/src/wallet/scanning.rs index da9d2f1dd6..a2007847fa 100644 --- a/zcash_client_sqlite/src/wallet/scanning.rs +++ b/zcash_client_sqlite/src/wallet/scanning.rs @@ -369,6 +369,7 @@ pub(crate) fn scan_complete( .chain(extended_after); replace_queue_entries::(conn, &query_range, replacement, false)?; + super::commitment_tree::mark_wallet_note_positions(conn, wallet_note_positions)?; Ok(()) } diff --git a/zcash_client_sqlite/src/wallet/transparent.rs b/zcash_client_sqlite/src/wallet/transparent.rs index 1a80eef4bb..2738570a77 100644 --- a/zcash_client_sqlite/src/wallet/transparent.rs +++ b/zcash_client_sqlite/src/wallet/transparent.rs @@ -1696,6 +1696,14 @@ pub(crate) fn transaction_data_requests( // least so long as address-based transaction data requests are required at all) these requests // would not be served by address-based lookups, but instead by querying for the spends of the // associated outpoints directly. + // + // Note: this query deliberately does NOT exclude ephemeral-scope addresses. Under the sync + // orchestration introduced alongside External-only scanning, transparent-to-shielded + // shielding flows (ZIP 320) park funds on a single-use ephemeral taddr and then spend them + // in a later transaction. That spend is only observable via an address-history lookup on the + // ephemeral taddr, so we must include those addresses here. A previous revision excluded + // them on the assumption that ephemeral addresses were fully characterised at creation + // time, which is not true once the funds have been moved on-chain. let mut spend_requests_stmt = conn.prepare_cached( "SELECT ssq.address, @@ -1707,7 +1715,6 @@ pub(crate) fn transaction_data_requests( LEFT OUTER JOIN transparent_received_output_spends tros ON tros.transparent_received_output_id = tro.id WHERE tros.transaction_id IS NULL - AND addresses.key_scope != :ephemeral_key_scope AND ( tro.max_observed_unspent_height IS NULL OR tro.max_observed_unspent_height < :chain_tip_height @@ -1720,7 +1727,6 @@ pub(crate) fn transaction_data_requests( let spend_search_rows = spend_requests_stmt.query_and_then( named_params! { - ":ephemeral_key_scope": KeyScope::Ephemeral.encode(), ":chain_tip_height": u32::from(chain_tip_height) }, |row| { @@ -2193,6 +2199,17 @@ pub(crate) fn put_transparent_output( if let Some(spending_transaction_id) = spending_tx_ref { mark_transparent_utxo_spent(conn, spending_transaction_id, output.outpoint())?; + } else if known_unspent { + // Current-UTXO refresh only tells us that the output is presently unspent. Queue an + // address-history lookup so that a later spend, such as transparent-to-shielded + // shielding, can be discovered and enhanced. + queue_transparent_spend_detection( + conn, + params, + *output.recipient_address(), + TxRef(id_tx), + output.outpoint().n(), + )?; } #[cfg(feature = "transparent-inputs")] @@ -2243,12 +2260,20 @@ pub(crate) fn queue_transparent_spend_detection( #[cfg(test)] mod tests { use secrecy::Secret; - use transparent::keys::{NonHardenedChildIndex, TransparentKeyScope}; + use transparent::{ + bundle::{OutPoint, TxOut}, + keys::{NonHardenedChildIndex, TransparentKeyScope}, + }; use zcash_client_backend::{ - data_api::{Account as _, WalletWrite, testing::TestBuilder}, - wallet::{Exposure, TransparentAddressMetadata}, + data_api::{ + Account as _, OutputStatusFilter, TransactionDataRequest, WalletRead as _, WalletWrite, + testing::TestBuilder, + }, + wallet::{Exposure, TransparentAddressMetadata, WalletTransparentOutput}, }; + use zcash_keys::keys::UnifiedAddressRequest; use zcash_primitives::block::BlockHash; + use zcash_protocol::value::Zatoshis; use crate::{ GapLimits, WalletDb, @@ -2267,6 +2292,94 @@ mod tests { ); } + #[test] + fn put_received_transparent_utxo_queues_spend_detection() { + let mut st = TestBuilder::new() + .with_data_store_factory(TestDbFactory::default()) + .with_account_from_sapling_activation(BlockHash([0; 32])) + .build(); + + let account = st.test_account().cloned().unwrap(); + let uaddr = st + .wallet() + .get_last_generated_address_matching( + account.id(), + UnifiedAddressRequest::AllAvailableKeys, + ) + .unwrap() + .unwrap(); + let taddr = uaddr.transparent().unwrap(); + + let observed_height = account.birthday().height() + 12345; + st.wallet_mut().update_chain_tip(observed_height).unwrap(); + + let utxo = WalletTransparentOutput::from_parts( + OutPoint::fake(), + TxOut::new(Zatoshis::const_from_u64(100000), taddr.script().into()), + Some(observed_height), + ) + .unwrap(); + st.wallet_mut() + .put_received_transparent_utxo(&utxo) + .unwrap(); + st.wallet_mut() + .update_chain_tip(observed_height + 1) + .unwrap(); + + let requests = st.wallet().transaction_data_requests().unwrap(); + assert!(requests.iter().any(|req| matches!( + req, + TransactionDataRequest::TransactionsInvolvingAddress(address_req) + if address_req.address() == *taddr + && address_req.block_range_start() == observed_height + 1 + && address_req.block_range_end() == Some(observed_height + 2) + ))); + } + + #[test] + fn put_received_ephemeral_utxo_queues_spend_detection() { + let mut st = TestBuilder::new() + .with_data_store_factory(TestDbFactory::default()) + .with_account_from_sapling_activation(BlockHash([0; 32])) + .build(); + + let account = st.test_account().cloned().unwrap(); + let observed_height = account.birthday().height() + 12345; + st.wallet_mut().update_chain_tip(observed_height).unwrap(); + let [(ephemeral_addr, _)] = st + .wallet_mut() + .reserve_next_n_ephemeral_addresses(account.id(), 1) + .unwrap() + .try_into() + .unwrap(); + + let utxo = WalletTransparentOutput::from_parts( + OutPoint::fake(), + TxOut::new( + Zatoshis::const_from_u64(100000), + ephemeral_addr.script().into(), + ), + Some(observed_height), + ) + .unwrap(); + st.wallet_mut() + .put_received_transparent_utxo(&utxo) + .unwrap(); + st.wallet_mut() + .update_chain_tip(observed_height + 1) + .unwrap(); + + let requests = st.wallet().transaction_data_requests().unwrap(); + assert!(requests.iter().any(|req| matches!( + req, + TransactionDataRequest::TransactionsInvolvingAddress(address_req) + if address_req.address() == ephemeral_addr + && address_req.block_range_start() == observed_height + 1 + && address_req.block_range_end() == Some(observed_height + 2) + && address_req.output_status_filter() == &OutputStatusFilter::All + ))); + } + #[test] fn transparent_balance_across_shielding() { zcash_client_backend::data_api::testing::transparent::transparent_balance_across_shielding(