Scan with External IVKs only; recover change notes via enhancement#11
Closed
czarcas7ic wants to merge 5 commits into
Closed
Scan with External IVKs only; recover change notes via enhancement#11czarcas7ic wants to merge 5 commits into
czarcas7ic wants to merge 5 commits into
Conversation
scan_cached_blocks previously trial-decrypted every output against both
the External and Internal IVKs for every account, doubling the key
agreement work per output. This commit narrows batch scanning to
External-scope IVKs only and defers Internal (change) note recovery to
the enhancement phase: when a scanned input spends a nullifier that
matches an entry in the nullifier map, the spending transaction is
queued for enhancement, where decrypt_and_store_transaction tries all
key scopes and recovers the change output.
Making that enhancement path actually produce spendable change notes
required a number of supporting fixes:
- decrypt: new compute_enriched_outputs re-maps a DecryptedTransaction
with per-output nullifier bytes and commitment-tree positions derived
from a TxBundlePositions, so late-discovered change notes can be
stored with the spend metadata they need to become spendable. New
collect_wallet_note_positions (gated on sync/test-dependencies)
extracts non-outgoing positions for notify_wallet_note_positions.
- data_api: new WalletWrite::notify_wallet_note_positions hook, called
during enhancement to mark commitment-tree positions for notes
discovered after the original scan has passed.
- data_api: new WalletWrite::prune_tracked_nullifiers hook. The call
site moves from per-chunk inside sync::running() to sync::run(),
after the post-running() drain and only when that drain returns
Drained, to honour the contract that the nullifier map must not be
pruned while enhancement may still consult it. The per-chunk
service_transaction_data_requests call inside running() is preserved
for cross-chunk cascade discovery; only the prune is hoisted out.
- sync::run: a stabilized but non-empty transaction-data request queue
now terminates the outer loop once scanning is idle, preventing an
infinite spin when lightwalletd cannot resolve a queued txid.
service_transaction_data_requests returns a new ServiceOutcome
(Drained | Stabilized) so callers can distinguish those states.
- sync: TransactionsInvolvingAddress handling now always calls
notify_address_checked after a successful lookup (not only on zero
results) so reused-address spend-detection cursors advance past
unrelated activity.
- zcash_client_sqlite: new v_transactions_filter_intermediate_state
migration hides transactions in transient inconsistent states during
sync (a wallet spend recorded before its matching change output is
recovered, or a change output stored before its wallet-side spend
cascade has been applied).
- zcash_client_sqlite: tx_retrieval_queue is now ordered by
(mined_height, tx_index, txid) so enhancement processes transactions
in chain order. This is load-bearing for the change-note cascade:
when tx A spends a change note created in tx B, B must be processed
first so its change note is already stored when A's mark_notes_spent
runs. The txid tiebreaker keeps ordering stable under the
request-set equality check in service_transaction_data_requests.
- zcash_client_sqlite: queue_tx_retrieval now emits Enhancement (not
GetStatus) for mined transactions whose raw column is already
populated. Transactions recorded via put_tx_meta during scanning
still need decrypt_and_store_transaction to recover Internal-scope
change outputs and OVK metadata -- a GetStatus alone would not do
that work.
- zcash_client_sqlite: transparent transaction_data_requests no longer
excludes ephemeral-scope addresses. 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.
- zcash_client_memory: TransferType::Incoming shielded outputs handed
to store_decrypted_tx are no longer misclassified as Internal change
notes. 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,orchard}_output) derive both
fields 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 end-of-function cleanup.
TransactionDataRequestQueue::remove_entries_for_txid is renamed to
remove_enhancement_entries_for_txid and narrowed to only strip
Enhancement entries, leaving GetStatus work intact for the sync
orchestrator.
czarcas7ic
force-pushed
the
adam/external-ivk-only-scan
branch
from
1bb55d8 to
46cbbbd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Deduplicate block_metadata / download_chain_state calls in fetch_tx_bundle_positions to avoid redundant DB queries and potential double gRPC round-trips for the same block. - Add tracing::warn in mark_positions when a leaf is not found in the shard tree, so unspendable notes are diagnosable. - Fix test helper enhance_transaction to read the compact block from the cache and sum outputs from preceding txs, instead of assuming the wallet tx is at index 0 within its block. - Restore PRUNING_DEPTH to pub(crate) since all consumers are intra-crate; remove the now-incorrect CHANGELOG entry. - Add tracing::debug in memory backend get_transaction when filtering out rawless entries, preserving observability for the semantic change from error to Ok(None). - Eliminate dead scope derivation for Outgoing in compute_enriched_outputs by moving scope into the non-Outgoing branch. - Remove unused _params parameter from enrich_decrypted_transaction. - Add cross-reference comments between the v_transactions HAVING clause in db.rs and the migration file. Co-Authored-By: Claude <noreply@anthropic.com>
- Revert doc delink of import_standalone_transparent_pubkey/script (unrelated doc fix). - Remove #[allow(irrefutable_let_patterns)] lint suppression (pre-existing warning, not caused by this PR). Co-Authored-By: Claude <noreply@anthropic.com>
…gration The verify_schema test compares the VIEW_TRANSACTIONS constant in db.rs against the view SQL produced by the migration. The cross-reference comments must be identical in both locations so the string comparison passes. Use the migration's comment text (which references db.rs) in both places. Co-Authored-By: Claude <noreply@anthropic.com>
The memory backend was inheriting the default no-op, so change notes discovered during enhancement never had their commitment tree positions registered via scan_complete. In edge cases (incremental sync with subtree-boundary change notes), this could leave them permanently unspendable. Co-Authored-By: Claude <noreply@anthropic.com>
Author
|
Replaced by a stacked PR series for reviewability:
The stacked tip is an exact diff match against this branch (verified via |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Narrows
scan_cached_blocksto trial-decrypt with External-scope IVKs only, halving key-agreement work per output. Change notes (Internal scope) are recovered later via the enhancement phase: when a scanned input consumes a nullifier already in the nullifier map, the spending tx is queued for enhancement, anddecrypt_and_store_transactiontrials all scopes.The rest of the diff is the supporting fixes needed to make that split safe — each one closes a recovery path the old dual-IVK scan happened to cover. They land together because the scan change isn't safe without them.
Supporting fixes
decrypt:compute_enriched_outputsre-maps aDecryptedTransactionwith per-output nullifier bytes and commitment-tree positions, so late-recovered change notes carry the metadata they need to become spendable.collect_wallet_note_positionsfeedsnotify_wallet_note_positions.WalletWrite: newnotify_wallet_note_positionsandprune_tracked_nullifiershooks. The prune call moves from per-chunk insidesync::running()out tosync::run(), only firing when the post-drain returnsDrained— the old call site could prune locators that a pending enhancement retry still needed.sync::run: a stabilized-but-non-empty request queue now terminates the outer loop once scanning is idle, instead of spinning forever on a txid lightwalletd can't resolve (service_transaction_data_requestsreturns a newServiceOutcome).notify_address_checkedis now called on every successfulTransactionsInvolvingAddresslookup so reused-address cursors advance past unrelated activity.zcash_client_sqlite:v_transactions_filter_intermediate_statemigration: hides txs in transient inconsistent states during sync (spend recorded without its change output, or change output stored without its spend cascade). Directv_transactionsconsumers may see transiently fewer rows during sync.tx_retrieval_queueordered by(mined_height, tx_index, txid)so enhancement runs in chain order — load-bearing for the change-note cascade (B must be processed before A when A spends B's change).queue_tx_retrievalemitsEnhancement(notGetStatus) for mined txs withrawpopulated, sodecrypt_and_store_transactionactually runs and recovers Internal-scope outputs.transaction_data_requestsno longer excludes ephemeral-scope addresses — required for ZIP 320 shielding spend detection after the parking tx mines.zcash_client_memory:TransferType::Incomingoutputs are no longer misclassified asInternalchange. Newfrom_decrypted_{sapling,orchard}_outputconstructors deriveis_change/recipient_key_scopefrom the actualTransferTypeinstead of hardcoding them.GetStatusrequests queued duringstore_decrypted_txfor unmined transparent-bundle txs are no longer wiped by end-of-function cleanup (renamed toremove_enhancement_entries_for_txid, narrowed toEnhancementonly).