From 6959fe967e1b5967eb37aef23466a4dfbda40a24 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 1 Jul 2026 18:45:19 +0700 Subject: [PATCH 1/6] test(shielded): reproduce the withdrawal anchor-mismatch root cause (TestFlight report B) A TestFlight report described a shielded->Core withdrawal that repeatedly shows "Transaction may have gone through" but never lands, with funds untouched. Investigation (docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md) traced it to an anchor mismatch, reproduced deterministically here without a testnet: - platform-wallet (--features shielded) wallet::shielded::file_store::tests:: depth0_spend_anchor_mid_block_is_not_a_recorded_block_boundary_anchor On the real SQLite-backed commitment tree: the spend anchor is the depth-0 (current) tree root (witness(pos,0).root == tree_anchor), but the wallet syncs commitments by index-chunk (CHUNK_SIZE=2048), not by block, so its tree routinely stops mid-block. drive records one anchor per block (block-end root). The test shows a mid-block depth-0 anchor equals neither adjacent block-boundary anchor -> it is a root Platform never recorded. - drive-abci (--features shielded_test_data) ...shielded_withdrawal::tests::proof_verification:: test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error A real Orchard proof + correct value balance + sufficient pool, but the anchor is not recorded (no insert_anchor_into_state) -> InvalidAnchorError. Identical to the passing ..._proof_succeeds test except the missing anchor record, isolating the anchor as the sole cause (proof passes first). Chain: the wallet submits a mid-block anchor drive never recorded; drive rejects exactly that with InvalidAnchorError; the withdrawal never lands, and the ambiguous rejection is misreported as "may have gone through". Fix direction (build the proof against a recorded anchor, not the depth-0 tip) is in the investigation doc; not implemented here. Co-Authored-By: Claude Opus 4.8 --- .../TESTFLIGHT_FEEDBACK_INVESTIGATION.md | 145 ++++++++++++++++++ .../shielded_withdrawal/tests.rs | 99 ++++++++++++ .../src/wallet/shielded/file_store.rs | 93 +++++++++++ 3 files changed, 337 insertions(+) create mode 100644 docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md diff --git a/docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md b/docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md new file mode 100644 index 0000000000..d0a3a5dbc5 --- /dev/null +++ b/docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md @@ -0,0 +1,145 @@ +# Shielded-pool TestFlight feedback — investigation + +Tracking two TestFlight feedback reports against build **3** (`cfBundleShortVersion 1.0`, `cfBundleVersion 3`, appAppleId `6782996681`). Both concern the shielded (Orchard) pool. Source exports: `~/Downloads/testflight_feedback*.zip`. + +## Reports + +### Report B — shielded→Core withdrawal never confirms ("may have gone through") — PRIMARY +- **Feedback id:** `AHlzEsyX33yEvRlSE4Qtm34`, 2026-06-29T18:44 UTC +- **Device:** iPhone18,2 (iPhone 16-class), iOS 26.6, locale pl-PL, tz Europe/Rome, uptime ~398 s +- **Comment (verbatim):** "Yesterday I sent tx from shielded to core and had the same msg as above. Today I checked and the balance was not sent. Today I have again info it might go through but will see later if it gone through or not." +- **Screenshot:** "Send Dash" sheet, recipient a **Core address** (`yTDFHbti48ArHwTszgM…`), **Transaction Type: Withdrawal to Core**, **Send From: Shielded** (0.2719731 DASH selected). A modal titled **"Success"** with body **"Transaction may have gone through — waiting for the next shielded sync to confirm. Do not retry."** + a **Done** button. +- **Reproduced across two days** with the same outcome (balance not actually moved). + +### Report A — switching wallets during a shielded sync — THIN +- **Feedback id:** `ACKbeJnBYOVKfN6dXEPFXG8`, 2026-06-28T22:35 UTC (uploaded twice — `testflight_feedback.zip` and `(1).zip` are byte-identical) +- **Device:** iPhone13,2 (iPhone 12 Pro), iOS 26.5, locale en-US, carrier AT&T, tz America/Los_Angeles +- **Comment (verbatim, truncated by TestFlight):** "Switching between wallets after starting a shielded sync " +- **No screenshot, no crash log.** Insufficient to reproduce yet; needs the full comment or a crash report. + +## Mechanism traced (Report B) + +The dialog is **not** an ad-hoc string — it is the deliberate handling of `PlatformWalletError::ShieldedSpendUnconfirmed`. + +1. UI: `SendViewModel.swift:734-744` catches `PlatformWalletError.shieldedSpendUnconfirmed` and surfaces it through the **success** path (title "Success"), explicitly to avoid inviting a retry that could double-spend. +2. Rust: `operations.rs::classify_spend_wait_failure` (≈1896) classifies a post-broadcast `wait_for_response` failure: + - `carries_consensus_rejection(wait_err)` → `ShieldedBroadcastFailed` (Platform executed + rejected on merits). + - **otherwise** (DAPI timeout / internal error / `StateTransitionBroadcastError` with empty consensus data → `cause: None`) → **`ShieldedSpendUnconfirmed`** — the note reservations are **kept**; the next sync reconciles. + +So the withdrawal **is broadcast**, then the wait-for-result fails **ambiguously** (no consensus verdict), and the reservation is intentionally held. The user hitting this **repeatedly** means the withdrawal ST's wait-for-result keeps failing ambiguously and the tx is not landing. + +### Broadcast → wait → classify (precise path) + +`operations.rs::broadcast_shielded_spend` (≈1785): + +1. `state_transition.broadcast(sdk, None)`: + - `Ok` → fall through to the wait. + - `broadcast_definitely_failed(e)` (consensus rejection, gRPC verdict code, `NoAvailableAddresses`) → `ShieldedBroadcastFailed` → outer match **releases** the reservation. + - **otherwise** (`AlreadyExists`, `DeadlineExceeded`, `Cancelled`, `Unknown`, `Internal`, `Aborted`, `DataLoss`, …) → warn "may have been admitted" → **fall through to the wait anyway**. +2. `state_transition.wait_for_response::(sdk, None)`: + - `Ok` → Confirmed. + - `Err` → `classify_spend_wait_failure`: consensus rejection → `ShieldedBroadcastFailed`; **else → `ShieldedSpendUnconfirmed`** (keep reservation). + +The ambiguous bucket is deliberately wide — both a broadcast that *probably* failed (ambiguous transport error) and a wait that timed out land in `ShieldedSpendUnconfirmed`, on the conservative "a lost ACK may have delivered, so don't re-spend" principle. + +## Funds safety (confirmed — no permanent loss) + +`pending_nullifiers` is **in-memory only** (`operations.rs:757-764`): +- If the tx **landed** → the next shielded sync marks the spent notes spent (clears the reservation). +- If the tx **never landed** → an **app restart drops the in-memory reservation and frees the notes**. + +So the notes are **not** permanently stranded — consistent with the user seeing the balance "back" the next day (app relaunch between sessions). The cost is UX: within a session the funds appear reserved/unavailable, the user is told "do not retry," and there is no clear resolution short of relaunch. + +### Confirmed gap: no sync-time release of a stale reservation + +The reservation set `pending_nullifiers` (`store.rs:348`) is only ever cleared by: +- `mark_spent` — when a later **sync proves the note spent** (the tx landed); or +- `clear_pending` — called by the spend-flow finalizer on a **definite** failure; or +- **process restart** — `pending_nullifiers` is in-memory only and is never persisted, so a relaunch rebuilds the store without it. + +There is **no reconcile path that releases a reservation whose tx demonstrably never landed**. For the `ShieldedSpendUnconfirmed` outcome the reservation is held indefinitely within the session, the activity row stays `Pending`, and the UI shows "do not retry" — so a genuinely non-landing withdrawal **strands the notes until the user force-quits the app**. This is the highest-leverage, most testable defect in the report, independent of *why* the withdrawal failed to land. + +## Severity + +- **Not** fund-loss. Funds return to spendable after a restart + sync. +- **Reliability bug:** the shielded→Core withdrawal repeatedly fails to confirm (ambiguous wait failure) — the user cannot complete a withdrawal. +- **UX bug:** modal title "Success" contradicts "may have gone through"; "Do not retry" with no confirmation path leaves the user stuck; funds appear unavailable until relaunch. + +## Open questions / hypotheses (Report B reliability) + +1. **Where does the ambiguity originate?** Is `wait_for_state_transition_result` timing out at DAPI, or is the ST being rejected without a surfaced consensus verdict? (`classify_spend_wait_failure` treats both as "unconfirmed.") +2. **Does the ST actually reach mempool / execute?** If it never lands across the next sync, is it never accepted, or accepted-but-slow? +3. **Withdrawal-specific?** Do shield / unshield / shielded-transfer confirm fine while only "withdrawal to Core" (transparent recipient) stalls? Points at the transparent-output path or its proof/fee handling. +4. **Network:** which network was the reporter on (mainnet/testnet)? DAPI wait reliability differs. + +## Investigation / reproduction plan + +Code trace (done): +- [x] Read the withdraw-to-Core operation (`operations.rs` ≈930-1070) end to end: reserve → build → record-pending → broadcast → wait → classify. +- [x] Map `broadcast_shielded_spend` + `broadcast_definitely_failed` + `classify_spend_wait_failure` — the ambiguous bucket. +- [x] Confirm the reservation lifecycle: a non-landing reservation is released **only** on tx-landing or restart — no sync-time release. **This is the key fixable gap.** + +Reproduction (needs a shielded environment + funds): +- [ ] Stand up / reuse a shielded devnet (see `~/.claude/.../reference-devnet-deploy-and-shielded-image-build.md`) or confirm which network the reporter used. +- [ ] Fund a shielded note; attempt a shielded→Core withdrawal via the Rust SDK / a focused harness with `tracing` at `debug`. +- [ ] Capture the exact failure shape: does `broadcast()` return Ok then `wait_for_response` time out, or does `broadcast()` itself return an ambiguous error? Capture the `wait_err`/transport code. +- [ ] Determine whether the ST ever reaches mempool / a block (does it land late, or never?) — distinguishes "slow confirm" from "genuine non-landing." +- [ ] For Report A: obtain the untruncated comment / any crash log; attempt a wallet-switch-during-shielded-sync repro in the simulator. + +Two fix tracks (independent): +- **Reservation-staleness (high leverage, unit-testable now):** add a bounded sync-time release for a `ShieldedSpendUnconfirmed` reservation whose nullifier is still absent on chain after the spend can no longer be valid (anchor/expiry window elapsed) — free the notes **without** a restart and flip the activity to a retryable state. Testable at the Rust layer without the network. +- **Root cause (needs repro):** why the withdrawal ST does not land — broadcast transport, wait timeout sizing for proof-heavy shielded STs, or an execution-time rejection that never surfaces a consensus verdict. + +## Root cause (B) — code investigation findings (testnet) + +**Wait has no client timeout.** `withdraw` calls `wait_for_response(sdk, None)`; with `wait_timeout = None` the SDK waits *without* a client-side duration cap (`rs-sdk/.../broadcast.rs:246`). So "the wait timed out too soon" is **not** the cause — `ShieldedSpendUnconfirmed` fires only when the DAPI result genuinely never resolves. + +**Leading hypothesis: anchor mismatch (`validate_anchor_exists`).** The drive-abci shielded-withdrawal validation (`shielded_withdrawal/transform_into_action/v0/mod.rs`) rejects on, among others: +- `validate_minimum_pool_notes` (anonymity floor), +- `InvalidShieldedProofError` (Orchard proof fails), +- **`validate_anchor_exists`** — the transition's `anchor` (commitment-tree Merkle root at build time) must exist in Platform's recorded-anchors tree, +- `validate_nullifiers` (double-spend / intra-bundle dup). + +The wallet computes the withdrawal anchor **locally** (`operations.rs::extract_spends_and_anchor`): `store.witness(note.position).root(cmx)` — the root of the wallet's own commitment tree at the note's checkpoint. Platform accepts it only if that exact root is one it recorded. A wallet whose local tree diverges from any Platform-recorded checkpoint (sync lag, frontier discrepancy, or a note in the local tree Platform hasn't recorded) produces an anchor Platform never had → **rejected every attempt**. This fits the report precisely: repeatable, never lands, funds untouched (the ST fails, notes aren't spent), and — because the rejection apparently doesn't reach the wallet as a clean consensus verdict — it lands in the ambiguous `ShieldedSpendUnconfirmed` bucket ("may have gone through") instead of a clear failure. + +**Why it may not surface as a clear failure:** if the rejection is delivered as a `StateTransitionBroadcastError` with empty consensus data (or the result is never retrievable because the ST is refused pre-block), `classify_spend_wait_failure` / `broadcast_definitely_failed` treat it as ambiguous → unconfirmed. + +### Code-dive conclusion (high confidence) + +The anchor the wallet submits is **not guaranteed to be a Platform-recorded anchor**, by construction: + +| Side | Behaviour | +|---|---| +| **Wallet anchor** | `extract_spends_and_anchor` → `witness(position, 0)` — **depth 0 = the current tree root** (matches the proof's `tree_anchor()`, also depth 0). | +| **Wallet sync** | appends commitments in `CHUNK_SIZE` units (`aligned_start = already_have / CHUNK_SIZE * CHUNK_SIZE`) and checkpoints at the post-append leaf count — **aligned to chunk/stream boundaries, never to block boundaries**. | +| **drive recording** | `record_anchor_if_changed` runs at **block-processing-end** — exactly **one anchor per block** (the block-end root), retained `shielded_anchor_retention_blocks = 1000`. | +| **drive validation** | `validate_anchor_exists` → `has_shielded_anchor(anchor)`; absent → **`InvalidAnchorError`**. | + +So the wallet's depth-0 root equals a drive-recorded anchor **only** when its current tree happens to sit exactly on a block-end commitment count. Any other state — mid-block stream stop, commitments appended past the last block drive recorded, or a `CHUNK_SIZE` boundary that isn't a block boundary — yields a root **Platform never recorded** → every withdrawal rejected. The developers already flag this exact hazard at `sync.rs:548` (*"the depth-0 witness then reflects a state Platform never recorded"*) and fixed only the checkpoint-id-dedup variant; the **block-alignment gap remains unguarded**. This matches the report precisely (repeatable, never lands, funds untouched) and is consistent with the rejection arriving ambiguously enough to be misclassified as `ShieldedSpendUnconfirmed`. + +Note: shield (deposit) does **not** spend, so it carries no anchor — which is why the user could fund a shielded balance yet never complete a spend. The same depth-0 anchor is used by `unshield` and `shielded_transfer`, so those are expected to be equally affected. + +### Fix direction (B) +Build the spend/withdrawal proof against a **recorded** anchor, not the bleeding-edge depth-0 root: select the most-recent checkpoint whose root drive actually recorded (a block-aligned, within-1000-block root) and generate the witness against **that** checkpoint. This is the standard shielded-wallet pattern (spend against a confirmed anchor, not the tip). Requires the wallet to know which of its checkpoints are block-aligned/recorded — i.e., associate checkpoints with the block heights drive records anchors at. + +### Still worth a testnet datapoint +A single testnet `tracing=debug` withdrawal capturing `InvalidAnchorError` (vs. a different rejection) converts "high confidence" to "confirmed" before investing in the fix. Cheap if a shielded-funded testnet wallet already exists; otherwise it is the multi-step bootstrap. + +### Separable bug (overlaps with A) +A definitively-rejected withdrawal is **misclassified** — it should surface as a clear failure (release the reservation, allow retry), not "may have gone through." That is the **A** reservation-release / classification work. + +## Reproduction — hard evidence (deterministic, no testnet) + +Two committed tests reproduce the root cause end to end, without a testnet: + +1. **Wallet produces a non-recorded anchor** — `platform-wallet` (`--features shielded`), `wallet::shielded::file_store::tests::depth0_spend_anchor_mid_block_is_not_a_recorded_block_boundary_anchor` — **PASSES**. On the real SQLite-backed commitment tree it appends two "blocks" of commitments, captures the depth-0 `tree_anchor()` at each block boundary (what drive records), then stops **mid-block** (index-chunk sync) and shows the wallet's mid-block anchor is **neither** recorded boundary anchor. It also pins that the spend anchor `witness(0).root(cmx)` equals that mid-block `tree_anchor()` — i.e. the value the withdrawal actually submits. + +2. **drive rejects that anchor** — `drive-abci` (`--features shielded_test_data`), `…shielded_withdrawal::tests::…::test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error` — a **real** Orchard proof, correct value balance, sufficient pool, but the anchor is not recorded (no `insert_anchor_into_state`) → `StateError::InvalidAnchorError`. Identical to `test_valid_shielded_withdrawal_proof_succeeds` except for the missing anchor record, isolating the anchor as the sole cause. + +Chain: **(1)** the wallet submits a mid-block anchor drive never recorded; **(2)** drive rejects exactly that with `InvalidAnchorError`; the proof passes first (proof-before-anchor order), so the failure isn't a proof/structure problem — it's the anchor. This is the user's "withdrawal never lands," confirmed in code. + +## UX follow-ups (independent of the root cause) + +- Don't title the ambiguous outcome "Success." Use a neutral "Submitted — confirming" state. +- After the next sync reconciles, surface the **definitive** outcome (landed vs returned-to-spendable) instead of leaving the user guessing. +- Consider releasing the reservation on the reconcile pass (not only on restart) so funds free up without a relaunch. diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs index f40be3e4b7..f84478ea59 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs @@ -511,6 +511,105 @@ mod tests { ); } + /// Gold-standard reproduction of TestFlight report B ("shielded→Core + /// withdrawal never lands"). Identical to + /// `test_valid_shielded_withdrawal_proof_succeeds` — a **real** Orchard + /// proof, correct value balance, sufficient pool balance — EXCEPT the + /// anchor is **not** recorded in state (`insert_anchor_into_state` is + /// omitted). A legitimate spend is then refused with `InvalidAnchorError` + /// purely because its anchor isn't in the recorded set. + /// + /// This is exactly the wallet's failure mode: it builds the proof + /// against its depth-0 tree root, but the index-chunk sync leaves that + /// root mid-block, so Platform (which records one anchor per block) never + /// recorded it. See `platform-wallet`'s + /// `depth0_spend_anchor_mid_block_is_not_a_recorded_block_boundary_anchor`, + /// which proves the wallet produces such a non-recorded anchor. + #[test] + fn test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error() { + let platform_version = PlatformVersion::latest(); + let platform = setup_platform(); + insert_dummy_encrypted_notes(&platform, 250); + let mut rng = StdRng::seed_from_u64(0); + let pk = get_proving_key(); + + // --- Create keys --- + let sk = SpendingKey::from_bytes([0u8; 32]).unwrap(); + let fvk = FullViewingKey::from(&sk); + let recipient = fvk.address_at(0u32, Scope::External); + let ask = SpendAuthorizingKey::from(&sk); + + // --- Create a spendable note with value 500M --- + let rho_bytes: [u8; 32] = { + let mut b = [0u8; 32]; + b[0] = 1; + b + }; + let rho = Rho::from_bytes(&rho_bytes).unwrap(); + let rseed = RandomSeed::from_bytes([42u8; 32], &rho).unwrap(); + let note = + Note::from_parts(recipient, NoteValue::from_raw(500_000_000), rho, rseed).unwrap(); + + // --- Build commitment tree and get anchor + merkle path --- + let cmx = ExtractedNoteCommitment::from(note.commitment()); + let mut tree = ClientMemoryCommitmentTree::new(100); + tree.append(cmx.to_bytes(), Retention::Marked).unwrap(); + tree.checkpoint(0u32).unwrap(); + let anchor = tree.anchor().unwrap(); + let merkle_path = tree.witness(Position::from(0u64), 0).unwrap().unwrap(); + + // --- Build bundle: spend 500M -> output 5K --- + let mut builder = Builder::::new(BundleType::DEFAULT, anchor); + builder.add_spend(fvk.clone(), note, merkle_path).unwrap(); + builder + .add_output(None, recipient, NoteValue::from_raw(5_000), [0u8; 36]) + .unwrap(); + let (unauthorized, _) = builder.build::(&mut rng).unwrap().unwrap(); + + let output_script = create_output_script(); + let unshielding_amount = 499_995_000u64; + let extra_sighash_data = dpp::shielded::shielded_withdrawal_extra_sighash_data_v0( + output_script.as_bytes(), + unshielding_amount, + 1, + Pooling::Never, + ); + let bundle_commitment: [u8; 32] = unauthorized.commitment().into(); + let sighash = compute_platform_sighash(&bundle_commitment, &extra_sighash_data); + let proven = unauthorized.create_proof(pk, &mut rng).unwrap(); + let bundle = proven.apply_signatures(rng, sighash, &[ask]).unwrap(); + + let (actions, value_balance, anchor_bytes, proof_bytes, binding_sig) = + serialize_authorized_bundle_i64(&bundle); + assert_eq!(value_balance, 499_995_000); + + // Pool balance passes; the anchor is DELIBERATELY not recorded + // (no `insert_anchor_into_state`) — the crux of the reproduction. + set_pool_total_balance(&platform, 500_000_000); + + let transition = create_shielded_withdrawal_transition( + actions, + value_balance as u64, + anchor_bytes, + proof_bytes, + binding_sig, + 1, + Pooling::Never, + output_script, + ); + + let processing_result = process_transition(&platform, transition, platform_version); + + // Real proof passes; anchor validation then rejects the unrecorded + // anchor. This is why the withdrawal never lands. + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::StateError(StateError::InvalidAnchorError(_)) + )] + ); + } + #[test] fn test_wrong_encrypted_note_size_returns_error() { let platform_version = PlatformVersion::latest(); diff --git a/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs b/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs index 741bc8bcfa..36bee868bb 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs @@ -555,4 +555,97 @@ mod tests { confirming reset cleared the SQLite tree tables" ); } + + /// Reproduces the shielded **withdrawal-never-lands** root cause (TestFlight + /// report B): the wallet builds a spend against its depth-0 (current) tree + /// root, but that root is a *Platform-recorded* anchor only when the tree + /// sits exactly on a block boundary. + /// + /// - The spend anchor is `witness(pos, 0).root(cmx)`, which equals + /// `tree_anchor()` (both depth-0; see the comment on `witness`). This test + /// asserts that equality directly. + /// - The wallet syncs commitments by index-chunk (`CHUNK_SIZE = 2048` in + /// `sync.rs`), **not** by block, so its tree routinely stops mid-block. + /// - drive records **one anchor per block** (`record_anchor_if_changed` at + /// block-processing-end) and `validate_anchor_exists` rejects any anchor + /// it never recorded (`InvalidAnchorError`). + /// + /// So a mid-block depth-0 anchor is rejected every attempt — repeatable, + /// never lands, funds untouched. The team already names this failure at the + /// `tree_size` test above ("Anchor not found in the recorded anchors"). + #[test] + fn depth0_spend_anchor_mid_block_is_not_a_recorded_block_boundary_anchor() { + use grovedb_commitment_tree::ExtractedNoteCommitment; + + let path = temp_tree_path("anchor_midblock"); + let mut store = FileBackedShieldedStore::open_path(&path, 100).unwrap(); + + let cmx = |b: u8| { + let mut c = [0u8; 32]; + c[0] = b; + c + }; + + // Two blocks of commitments. drive records ONE anchor per block, at + // block-processing-end (after ALL of that block's commitments): + // block 1 = commitments 1..=3 -> recorded anchor at tree size 3 + // block 2 = commitments 4..=6 -> recorded anchor at tree size 6 + for b in 1..=3u8 { + store.append_commitment(&cmx(b), true).unwrap(); + } + store.checkpoint_tree(3).unwrap(); + let recorded_after_block1 = store.tree_anchor().unwrap(); + + // The index-chunk sync appends block 2's commitments incrementally; a + // chunk/stream boundary that lands mid-block (the common case — a + // 2048-leaf chunk rarely ends on a block boundary) leaves the wallet at + // tree size 4, and it checkpoints there (sync.rs checkpoints at the + // post-append leaf count). Its depth-0 anchor is now the root at size 4 + // — a state drive never recorded. + store.append_commitment(&cmx(4), true).unwrap(); + store.checkpoint_tree(4).unwrap(); + let wallet_depth0_mid_block = store.tree_anchor().unwrap(); + + // The spend path uses exactly this anchor: `extract_spends_and_anchor` + // builds it as `witness(pos, 0).root(cmx)`. Pin that it equals the + // mid-block `tree_anchor()`. + let cmx0 = ExtractedNoteCommitment::from_bytes(&cmx(1)) + .into_option() + .expect("valid cmx"); + let spend_anchor = store + .witness(0) + .unwrap() + .expect("witness for marked position 0") + .root(cmx0) + .to_bytes(); + assert_eq!( + spend_anchor, wallet_depth0_mid_block, + "the spend anchor (depth-0 witness root) must equal the mid-block tree_anchor" + ); + + // Finish block 2. drive records the anchor at tree size 6. + store.append_commitment(&cmx(5), true).unwrap(); + store.append_commitment(&cmx(6), true).unwrap(); + store.checkpoint_tree(6).unwrap(); + let recorded_after_block2 = store.tree_anchor().unwrap(); + + let _ = std::fs::remove_file(&path); + + // drive's recorded anchor set is {block1, block2}. The wallet's mid-block + // spend anchor is neither -> `validate_anchor_exists` rejects it with + // InvalidAnchorError, and the withdrawal never lands. + assert_ne!( + wallet_depth0_mid_block, recorded_after_block1, + "mid-block spend anchor must differ from block 1's recorded anchor" + ); + assert_ne!( + wallet_depth0_mid_block, recorded_after_block2, + "mid-block spend anchor must differ from block 2's recorded anchor" + ); + assert_ne!( + recorded_after_block1, recorded_after_block2, + "the two block-boundary anchors differ (the tree grew), so drive's \ + recorded set is exactly these two and the mid-block anchor is outside it" + ); + } } From 1d1d21bce1026c73e78730fab36647c1f8779731 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Wed, 1 Jul 2026 21:53:31 +0700 Subject: [PATCH 2/6] fix(shielded): build spends against a Platform-recorded anchor Shielded spends (withdraw / unshield / transfer / identity-create-from-pool) built the Orchard proof against the wallet's depth-0 (current) commitment-tree root. The index-chunk sync (CHUNK_SIZE=2048) routinely leaves the tree mid-block, and drive records anchors only at block boundaries, so the depth-0 root is frequently one Platform never recorded -> validate_anchor_exists rejects the transition (InvalidAnchorError) and the spend never lands. The ambiguous rejection then surfaces as "Transaction may have gone through" (ShieldedSpendUnconfirmed), the TestFlight report B symptom. Reproduced by two tests committed earlier (wallet mid-block anchor is non-recorded; drive rejects a valid-proof unrecorded anchor). Fix: extract_spends_and_anchor now fetches Platform's recorded anchor set (GetShieldedAnchors) and builds against the shallowest checkpoint depth whose root is recorded, via a pure, unit-testable select_recorded_spends probe: - depth 0 (fully-synced fast path) if its root is recorded; - else walk older checkpoints (1..100), taking the first recorded root, and stop once a selected note post-dates the checkpoint (deeper is older still); - else return the new retryable PlatformWalletError::ShieldedNoRecordedAnchor BEFORE broadcasting. Fund-safety: the anchor is always the root of the same-depth witnesses (MerklePath::root) with a cross-note agreement check, so anchor<->witness stay consistent by construction; the Orchard nullifier is anchor-independent, so an older recorded anchor does not weaken the on-chain double-spend guard (auditor-confirmed). ShieldedNoRecordedAnchor is returned pre-broadcast and routes to each caller's reservation-releasing arm (cancel_pending), so notes are freed and nothing is broadcast. Also: ShieldedStore gains witness_at_depth (witness becomes a depth-0 default); new FFI code ErrorShieldedNoRecordedAnchor = 19 maps to a retryable "still syncing" message (distinct from ShieldedSpendUnconfirmed's "do not retry"). Design was spec'd (docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md) and audited by three independent reviewers (soundness / feasibility / scope) before implementation. Wallet suite 304 passing; FFI 107; fmt + clippy clean. Follow-ups (documented in the spec): Report-A sync convergence so a chronically-interrupted wallet reaches a recorded checkpoint; the Swift FFIResultCode case for code 19; optionally returning each anchor's tree size from the query for a fully deterministic selection; and the ShieldedSpendUnconfirmed classification/reservation-release (A). Co-Authored-By: Claude Opus 4.8 --- .../SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md | 159 +++++++ packages/rs-platform-wallet-ffi/src/error.rs | 13 + .../src/shielded_send.rs | 31 ++ packages/rs-platform-wallet/src/error.rs | 13 + .../src/wallet/shielded/file_store.rs | 16 +- .../src/wallet/shielded/operations.rs | 450 +++++++++++++++--- .../src/wallet/shielded/store.rs | 33 +- 7 files changed, 636 insertions(+), 79 deletions(-) create mode 100644 docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md diff --git a/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md b/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md new file mode 100644 index 0000000000..8e0b8bf63c --- /dev/null +++ b/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md @@ -0,0 +1,159 @@ +# Spec — shielded spend uses a Platform-recorded anchor + +Fixes the reproduced root cause of TestFlight report B (see +`TESTFLIGHT_FEEDBACK_INVESTIGATION.md`): shielded spends (withdraw / unshield / +transfer) are rejected with `InvalidAnchorError` because the wallet builds the +proof against its **depth-0** commitment-tree root, which — with an index-chunk +sync (`CHUNK_SIZE = 2048`) that routinely stops mid-block — is a root Platform +never recorded (drive records **one anchor per block**, retains 1000). + +Reproduced by two passing tests (committed `6959fe967e`): +`platform-wallet …depth0_spend_anchor_mid_block_is_not_a_recorded_block_boundary_anchor` +and `drive-abci …test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error`. + +## Goal / non-goal + +- **Goal:** a shielded spend the wallet builds is accepted by Platform's + `validate_anchor_exists` — i.e. its anchor is always a drive-recorded root. +- **Goal:** when the wallet cannot yet build against a recorded anchor, fail + with a **clear, retryable** error instead of broadcasting a doomed transition + that surfaces as "may have gone through." +- **Non-goal (this spec):** the reservation-release / `ShieldedSpendUnconfirmed` + misclassification (tracked as "A"); UX copy. Called out where they interact. + +> **Revised after 3-reviewer audit** (soundness / feasibility / scope). The +> cryptographic core is confirmed sound; changes folded in: drop the +> most-recent-anchor fast-path; `anchor_at_depth` is an explicit store-trait +> addition (implemented via the `witness(marked_pos, d).root(cmx)` workaround to +> avoid an external `grovedb-commitment-tree` crate change); position-aware note +> selection is required (B1); the "no recorded checkpoint" outcome (B2) is a +> graceful, fund-safe retryable error — auto-enable in that pathological case is +> a follow-up (see Scope). + +## Chosen approach: build against the most-recent recorded anchor + +Instead of `witness(pos, 0)` (bleeding-edge root), select the **shallowest +checkpoint whose root Platform has recorded**, and witness + prove against that. + +Data flow, per spend (`extract_spends_and_anchor`): + +1. **Fetch the recorded anchor set** from Platform: `ShieldedAnchors::fetch_current(sdk)` + (`ShieldedAnchors(Vec<[u8;32]>)`) — the ≤1000 retained roots, into a `HashSet`. + (Always the full set — `GetMostRecentShieldedAnchor` would *not* match in the + target mid-block case, since the wallet's recorded checkpoint is a *prior* + block, not the latest.) +2. **Walk the wallet's checkpoints newest→oldest**, `d = 0..max_checkpoints`. For + each `d`, compute the tree root at `d` (`anchor_at_depth(d)`); take the first + `d` whose root ∈ the recorded set — `d*` / `anchor*`, with tree size + `size* = checkpoint_id@d*`. `d = 0` is the fully-synced fast path. +3. **Select notes confined to `d*`:** filter candidate `unspent_notes` to + `position < size*` *before* value-based coverage selection, so every selected + note is witnessable at `d*`. +4. **Witness every selected note at `d*`** (`witness(pos, d*)`), assert each + witness root equals `anchor*`, and pass `anchor*` to the proof builder. +5. **Failure → retryable, no broadcast:** + - No `d` in `0..max_checkpoints` has a recorded root → `ShieldedNoRecordedAnchor`. + - A `d*` exists but confirmed-at-`d*` balance (notes with `position < size*`) + can't cover amount+fee → `ShieldedNoRecordedAnchor` (same class: "wait for + the next sync"). Never surface `ShieldedMerkleWitnessUnavailable` here. + +Note requirement (B1): value-based selection alone would pick a note newer than +`d*`, whose `witness(pos, d*)` returns `Err(NotContained)` (→ opaque +`ShieldedMerkleWitnessUnavailable`). The `position < size*` pre-filter prevents +this; deeper `d` only has *fewer* eligible notes, so if the shallowest recorded +`d*` can't cover the amount, no deeper one can — fail fast with the retryable +error. + +### Why this approach +- **Correct by construction:** the anchor is a value Platform recorded, so + `validate_anchor_exists` passes; the drive repro test's rejection cannot occur. +- **Uses existing primitives:** `GetShieldedAnchors` query + `witness(pos, depth)` + already exist; the proof builder already takes an explicit `anchor`. +- **No stream/tree-format change:** avoids block-aligned checkpointing, which is + infeasible because the sync stream exposes only a per-chunk `block_height`, not + per-commitment block boundaries. +- **Fund-safe:** spending against an older recorded anchor is standard shielded + practice; the note is unspent, the witness root equals the anchor, and the + nullifier set is the authoritative double-spend guard. + +## Alternatives rejected +- **Block-aligned checkpointing** (checkpoint at each block's last commitment): + cleanest in theory, but the sync stream gives only per-chunk `block_height` + (chain tip at response time), not per-commitment block membership — the wallet + cannot identify block boundaries within a chunk. Rejected as infeasible without + a drive/DAPI query-shape change. +- **Precondition check only** (verify depth-0 anchor is recorded; else clear + error, don't broadcast): safe and small, but does **not** enable withdrawal + when mid-block — leaves the user unable to withdraw. Kept as the *fallback* + branch (step 4), not the whole fix. +- **Depth-`N`-back heuristic** (always spend `N` checkpoints back): fragile — `N` + checkpoints is not a fixed number of blocks, and a chunk-boundary checkpoint + root still isn't guaranteed recorded. Rejected. + +## Interface / data-flow changes +- `ShieldedStore` trait (breaking; implemented by `FileBackedShieldedStore`, + `InMemoryShieldedStore`, and any host/FFI persister): + - `witness_at_depth(&self, position, depth) -> Result, Err>` + (generalize today's depth-0-only `witness`). + - `anchor_at_depth(&self, depth) -> Result, Err>` — the tree + root at checkpoint depth `d`. Implemented via `witness_at_depth(any_marked_pos, + d).root(cmx)` (the repro-test workaround) to avoid touching the pinned + external `grovedb-commitment-tree` crate. + - `checkpoint_id_at_depth(&self, depth) -> Result, Err>` — the + `checkpoint_id` (== tree size, per `sync.rs`) at depth `d`, for the + `position < size*` note filter. (`InMemoryShieldedStore` may return a stub / + be excluded from the probe path if it lacks a real tree.) +- `extract_spends_and_anchor` gains `sdk: &Arc` and does the fetch + + probe + filter. It has FOUR callers (audit): `withdraw`, `unshield`, + `shielded_transfer`, `identity_create_from_shielded_pool`. +- New retryable error `PlatformWalletError::ShieldedNoRecordedAnchor`; FFI/UI maps + it to "still syncing — try again shortly" (distinct from `ShieldedSpendUnconfirmed`). +- Query: `ShieldedAnchors::fetch_current(sdk).await` (existing `FetchCurrent` impl). + +## Failure modes / risks +- **Anchor/witness mismatch** → proof rejected. Mitigation: assert each selected + note's `witness(pos, d*).root(cmx)` equals `anchor*` (extends today's "all notes + agree" check to equal the *selected* anchor). +- **Concurrency (C1):** depth indices shift if a sync checkpoints concurrently. + Mitigation: fetch the anchor set *outside* the store lock; then do the + probe (`anchor_at_depth`) **and** all note witnesses under a **single** store + read-lock hold so `d*`, `size*`, and the witnesses are mutually consistent. +- **Recorded set (≤1000)** — `HashSet` membership; one round trip per spend (rare). +- **Pruning race** — a freshly-queried anchor has ~1000 blocks of runway; a deep + `d*` erodes it but stays within the window at query time. Negligible. +- **No recorded checkpoint / insufficient confirmed-at-`d*` funds** → clean + `ShieldedNoRecordedAnchor`; before broadcast, so the generic `Err` arm runs + `cancel_pending` (reservation released) in all four callers. No fund risk. +- **Double-spend:** unchanged — the Orchard nullifier is `(nk, ρ, ψ, cm)`-derived, + anchor-independent; the on-chain nullifier set stays authoritative (auditor-confirmed). + +## Test plan +- **Keep** the two reproduction tests (they pin the pre-fix mechanism + the drive + contract; both remain valid). +- **Wallet unit (new):** given a tree with recorded anchors at block-boundary + sizes {B1,B2} and a mid-block depth-0 state, `select_recorded_anchor` returns + `B2`'s anchor (most-recent recorded), not the depth-0 root; and witnessing at + that depth yields a root ∈ recorded set. +- **Wallet unit (new):** no checkpoint root recorded → `ShieldedNoRecordedAnchor`. +- **drive-abci (existing, real-proof):** a withdrawal built against a **recorded** + anchor succeeds (`…_proof_succeeds` already covers this) — confirms the fix's + output shape is accepted. +- **Regression:** full `platform-wallet --features shielded` + the shielded + drive-abci suite green; `clippy --all-features`; `fmt --check`. + +## Rollout / scope +- **This PR:** the wallet-only anchor-selection fix across all four spend paths + + the `ShieldedNoRecordedAnchor` error + position-aware selection + tests. No + protocol/drive/DAPI change. +- **Follow-up 1 (Report A — convergence):** ensure a shielded sync reliably + reaches the tip (isn't stranded mid-chunk by wallet-switch/backgrounding), so a + recorded checkpoint is actually created. Without it, a chronically-interrupted + wallet gets the clean `ShieldedNoRecordedAnchor` error but still can't withdraw. +- **Follow-up 2 (robust auto-enable, protocol):** have `GetShieldedAnchors` / + `GetMostRecentShieldedAnchor` return each anchor's **tree size** (auditor's + suggestion) so the wallet can deliberately checkpoint on a recorded boundary and + know exactly which notes are confirmed — collapses B1+B2 but needs drive + DAPI + + proof-verifier + SDK changes. +- **Follow-up A (classification/reservation):** the `ShieldedSpendUnconfirmed` + "may have gone through" misclassification + no-restart reservation release. + This fix removes the dominant *cause*; A improves the *residual* ambiguous case. diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index de1a6cb944..c18ef2d681 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -124,6 +124,16 @@ pub enum PlatformWalletFFIResultCode { /// must NOT auto-retry — a retry would select different unreserved notes /// and could double-send if the original spend landed. ErrorShieldedSpendUnconfirmed = 18, + /// Maps `PlatformWalletError::ShieldedNoRecordedAnchor`. The wallet could + /// not build the spend against any Platform-recorded anchor yet: its local + /// commitment tree is mid-block (an index-chunk sync routinely stops + /// between block boundaries) while Platform records an anchor only at each + /// block boundary. The transition was NOT broadcast and any note + /// reservations were released, so this is RETRYABLE — the host should wait + /// for the next shielded sync (which advances the tree onto a recorded + /// boundary) and try again. Distinct from `ErrorShieldedSpendUnconfirmed`, + /// where a spend WAS broadcast and must NOT be retried. + ErrorShieldedNoRecordedAnchor = 19, NotFound = 98, // Used exclusively for all the Option that are retuned as errors ErrorUnknown = 99, @@ -237,6 +247,9 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::ShieldedSpendUnconfirmed { .. } => { PlatformWalletFFIResultCode::ErrorShieldedSpendUnconfirmed } + PlatformWalletError::ShieldedNoRecordedAnchor(..) => { + PlatformWalletFFIResultCode::ErrorShieldedNoRecordedAnchor + } _ => PlatformWalletFFIResultCode::ErrorUnknown, }; PlatformWalletFFIResult::err(code, error.to_string()) diff --git a/packages/rs-platform-wallet-ffi/src/shielded_send.rs b/packages/rs-platform-wallet-ffi/src/shielded_send.rs index a7c9a0283e..bd6fe5f224 100644 --- a/packages/rs-platform-wallet-ffi/src/shielded_send.rs +++ b/packages/rs-platform-wallet-ffi/src/shielded_send.rs @@ -448,6 +448,14 @@ fn map_spend_result( e.to_string(), ) } + // Retryable: the wallet couldn't build the spend against any + // Platform-recorded anchor yet (its commitment tree is mid-block after + // an index-chunk sync). Nothing was broadcast and the notes were + // released, so the host may retry after the next shielded sync. + Err(e @ PlatformWalletError::ShieldedNoRecordedAnchor(_)) => PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorShieldedNoRecordedAnchor, + format!("Wallet is still syncing to a confirmed state — try again shortly. ({e})"), + ), // Definitive failure: the transition was not executed and the notes // were released; the host may retry. Err(e @ PlatformWalletError::ShieldedBroadcastFailed(_)) => PlatformWalletFFIResult::err( @@ -628,6 +636,14 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_identity_create_from_p ), ) } + // Retryable: no Platform-recorded anchor covered the selected notes yet + // (the commitment tree is mid-block after an index-chunk sync). Nothing + // was broadcast and the notes were released, so the host may retry after + // the next shielded sync. + Err(e @ PlatformWalletError::ShieldedNoRecordedAnchor(_)) => PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorShieldedNoRecordedAnchor, + format!("Wallet is still syncing to a confirmed state — try again shortly. ({e})"), + ), // Definitive failure: the transition was not executed and the spent notes were released. Err(e @ PlatformWalletError::ShieldedBroadcastFailed(_)) => PlatformWalletFFIResult::err( PlatformWalletFFIResultCode::ErrorShieldedBroadcastFailed, @@ -1332,6 +1348,21 @@ mod tests { "broadcast-failed message must carry the wallet Display payload" ); + // No Platform-recorded anchor yet → its own retryable code, distinct + // from the "was broadcast, do NOT retry" unconfirmed code above. + let no_anchor: Result<(), PlatformWalletError> = Err( + PlatformWalletError::ShieldedNoRecordedAnchor("mid-block".to_string()), + ); + let result = map_spend_result(no_anchor, "shielded withdraw"); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorShieldedNoRecordedAnchor + ); + assert!( + message_of(&result).contains("try again shortly"), + "no-recorded-anchor message must be the retryable guidance" + ); + let other: Result<(), PlatformWalletError> = Err(PlatformWalletError::ShieldedNoUnspentNotes); let result = map_spend_result(other, "shielded withdraw"); diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index c94cb7093d..c95c086ade 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -234,6 +234,19 @@ pub enum PlatformWalletError { #[error("Shielded Merkle witness unavailable: {0}")] ShieldedMerkleWitnessUnavailable(String), + /// No Platform-recorded anchor covers the notes selected for a shielded + /// spend, so the wallet cannot build a proof Platform will accept. + /// + /// Platform records one commitment-tree anchor per block, but an + /// index-chunk sync routinely leaves the wallet's tree mid-block, so the + /// current (depth-0) root is frequently a value Platform never recorded. + /// This variant is **retryable**: it is returned *before* any broadcast, + /// the note reservations are released by the caller's generic error path, + /// and the next shielded sync advances the tree onto a recorded boundary. + /// `0` carries a human-readable reason. + #[error("Shielded spend cannot use a Platform-recorded anchor: {0}")] + ShieldedNoRecordedAnchor(String), + #[error("Shielded key derivation failed: {0}")] ShieldedKeyDerivation(String), diff --git a/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs b/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs index 36bee868bb..436b0513a5 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/file_store.rs @@ -283,19 +283,23 @@ impl ShieldedStore for FileBackedShieldedStore { .map_err(|e| FileShieldedStoreError(format!("read tree anchor: {e}"))) } - fn witness( + fn witness_at_depth( &self, position: u64, + depth: usize, ) -> Result, Self::Error> { let tree = self .tree .lock() .map_err(|e| FileShieldedStoreError(format!("tree mutex poisoned: {e}")))?; - // `checkpoint_depth = 0` = current tree state. The Halo 2 - // proof we're about to build uses `tree_anchor()` — also - // depth 0 — so the witness root must agree. - tree.witness(Position::from(position), 0) - .map_err(|e| FileShieldedStoreError(format!("witness({position}): {e}"))) + // `checkpoint_depth = 0` is the current tree state; deeper values + // reach older checkpoints so a spend can be built against a root + // Platform actually recorded (it records one anchor per block, while + // an index-chunk sync routinely leaves the tree mid-block). The proof + // uses whichever anchor this witness produces via `MerklePath::root`, + // so the anchor and the authentication path always agree. + tree.witness(Position::from(position), depth) + .map_err(|e| FileShieldedStoreError(format!("witness({position}, depth {depth}): {e}"))) } fn tree_size(&self) -> Result { diff --git a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs index c376c99a78..f703877241 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs @@ -33,9 +33,10 @@ use crate::error::PlatformWalletError; use crate::wallet::persister::WalletPersister; use crate::wallet::platform_wallet::WalletId; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashSet}; use std::sync::Arc; +use dash_sdk::platform::fetch_current_no_parameters::FetchCurrent; use dash_sdk::platform::transition::broadcast::BroadcastStateTransition; use dash_sdk::platform::transition::identity_create_from_shielded_pool::IdentityCreateFromShieldedPool; use dpp::address_funds::{ @@ -662,7 +663,7 @@ pub async fn unshield( // it. A build failure leaves it `None` and records nothing. let mut pending_entry = None; let result = async { - let (spends, anchor) = extract_spends_and_anchor(store, &selected_notes).await?; + let (spends, anchor) = extract_spends_and_anchor(sdk, store, &selected_notes).await?; // The builder computes and returns the fee authoritatively; `exact_fee` (== the // minimum) was already used above for note reservation. @@ -824,7 +825,7 @@ pub async fn transfer( let mut pending_entry = None; let result = async { - let (spends, anchor) = extract_spends_and_anchor(store, &selected_notes).await?; + let (spends, anchor) = extract_spends_and_anchor(sdk, store, &selected_notes).await?; // The builder computes and returns the fee authoritatively; `exact_fee` (== the // minimum) was already used above for note reservation. @@ -974,7 +975,7 @@ pub async fn withdraw( let mut pending_entry = None; let result = async { - let (spends, anchor) = extract_spends_and_anchor(store, &selected_notes).await?; + let (spends, anchor) = extract_spends_and_anchor(sdk, store, &selected_notes).await?; // The builder computes and returns the fee authoritatively; `exact_fee` (== the // minimum) was already used above for note reservation. @@ -1163,7 +1164,7 @@ where // outer match; it lives here so the flip can see it. let mut pending_entry = None; let result = async { - let (spends, anchor) = extract_spends_and_anchor(store, &selected_notes).await?; + let (spends, anchor) = extract_spends_and_anchor(sdk, store, &selected_notes).await?; let build = build_identity_create_from_shielded_pool_transition( public_keys, @@ -1508,88 +1509,188 @@ fn default_orchard_address(keys: &OrchardKeySet) -> Result( + sdk: &Arc, store: &Arc>, notes: &[ShieldedNote], ) -> Result<(Vec, Anchor), PlatformWalletError> { - use grovedb_commitment_tree::ExtractedNoteCommitment; + // Fetch the recorded anchor set OUTSIDE the store lock so the network + // round-trip doesn't serialize with other store users, and so the lock is + // held only for the mutually-consistent depth/witness probe below. + let dash_sdk::query_types::ShieldedAnchors(recorded_anchors) = + dash_sdk::query_types::ShieldedAnchors::fetch_current(sdk).await?; + let recorded: HashSet<[u8; 32]> = recorded_anchors.into_iter().collect(); + + if notes.is_empty() { + return Err(PlatformWalletError::ShieldedBuildError( + "no spendable notes selected — anchor undefined".to_string(), + )); + } + // Hold a single read lock across the whole probe so the checkpoint depths + // and the per-note witnesses stay mutually consistent: a concurrent sync + // checkpointing mid-probe would otherwise shift the depth indices out from + // under us. let store = store.read().await; + select_recorded_spends(&*store, notes, &recorded) +} - let mut spends = Vec::with_capacity(notes.len()); - let mut anchor: Option = None; - for note in notes { - let orchard_note = deserialize_note(¬e.note_data).ok_or_else(|| { - PlatformWalletError::ShieldedBuildError(format!( - "Failed to deserialize note at position {}", - note.position - )) - })?; - - let merkle_path = store - .witness(note.position) - .map_err(|e| PlatformWalletError::ShieldedMerkleWitnessUnavailable(e.to_string()))? - .ok_or_else(|| { - PlatformWalletError::ShieldedMerkleWitnessUnavailable(format!( - "no witness available for note at position {} (not marked, or pruned past this position)", - note.position - )) - })?; +/// Pick the shallowest checkpoint depth whose tree root is in `recorded`, +/// witnessing every note at that depth. Pure (no SDK, no async), so the depth +/// walk can be unit-tested against a real commitment tree. +/// +/// Depth 0 is the current tree state (the fully-synced fast path). Deeper +/// checkpoints are older and hold strictly fewer positions, so the probe stops +/// as soon as a selected note is no longer witnessable at a depth — no deeper +/// checkpoint could contain it. Returns +/// [`PlatformWalletError::ShieldedNoRecordedAnchor`] when no probed depth has a +/// recorded root (a clean, retryable outcome — nothing is broadcast). +fn select_recorded_spends( + store: &S, + notes: &[ShieldedNote], + recorded: &HashSet<[u8; 32]>, +) -> Result<(Vec, Anchor), PlatformWalletError> { + use grovedb_commitment_tree::ExtractedNoteCommitment; - // Compute the anchor this witness was generated against. - // All selected notes must share the same anchor — if not, - // the store handed us witnesses from different - // checkpoints, which the spend builder would reject - // downstream with `AnchorMismatch`. Surface the mismatch - // here so the host doesn't pay the ~30 s proof cost - // first. - let cmx = ExtractedNoteCommitment::from_bytes(¬e.cmx) - .into_option() - .ok_or_else(|| { + // Build every selected note's `SpendableNote` plus the shared anchor at a + // single checkpoint `depth`. + // + // `strict` (depth 0 only): a missing/failed witness is a hard + // `ShieldedMerkleWitnessUnavailable` (the note is expected to be + // witnessable at the current tip). With `strict == false` (deeper + // checkpoints) a missing/`NotContained` witness means the note post-dates + // this older checkpoint, so the depth is unusable — return `Ok(None)` and + // let the caller stop probing deeper. An anchor disagreement across notes + // is always a hard error (the spend builder would reject it downstream). + let build_at_depth = |depth: usize, + strict: bool| + -> Result, Anchor)>, PlatformWalletError> { + let mut spends = Vec::with_capacity(notes.len()); + let mut anchor: Option = None; + for note in notes { + let orchard_note = deserialize_note(¬e.note_data).ok_or_else(|| { PlatformWalletError::ShieldedBuildError(format!( - "invalid stored cmx for note at position {}", + "Failed to deserialize note at position {}", note.position )) })?; - let witness_anchor = merkle_path.root(cmx); - match &anchor { - None => anchor = Some(witness_anchor), - Some(prev) if prev.to_bytes() != witness_anchor.to_bytes() => { - return Err(PlatformWalletError::ShieldedBuildError(format!( - "witness anchor mismatch across selected notes (position {})", - note.position - ))); + + let merkle_path = match store.witness_at_depth(note.position, depth) { + Ok(Some(path)) => path, + Ok(None) if strict => { + return Err(PlatformWalletError::ShieldedMerkleWitnessUnavailable(format!( + "no witness available for note at position {} (not marked, or pruned past this position)", + note.position + ))); + } + Err(e) if strict => { + return Err(PlatformWalletError::ShieldedMerkleWitnessUnavailable( + e.to_string(), + )); + } + // depth > 0: the note isn't witnessable at this older checkpoint + // (appended after it, or the depth doesn't exist), so this depth + // can't cover the selection — signal "unusable depth". + Ok(None) | Err(_) => return Ok(None), + }; + + // The anchor is derived from the witness path itself + // (`MerklePath::root(cmx)`); all selected notes must agree on it, or + // the store handed back witnesses from different checkpoints and the + // spend builder would reject the mismatch downstream. + let cmx = ExtractedNoteCommitment::from_bytes(¬e.cmx) + .into_option() + .ok_or_else(|| { + PlatformWalletError::ShieldedBuildError(format!( + "invalid stored cmx for note at position {}", + note.position + )) + })?; + let witness_anchor = merkle_path.root(cmx); + match &anchor { + None => anchor = Some(witness_anchor), + Some(prev) if prev.to_bytes() != witness_anchor.to_bytes() => { + return Err(PlatformWalletError::ShieldedBuildError(format!( + "witness anchor mismatch across selected notes (position {})", + note.position + ))); + } + _ => {} } - _ => {} + + spends.push(SpendableNote { + note: orchard_note, + merkle_path, + }); } - spends.push(SpendableNote { - note: orchard_note, - merkle_path, - }); + // `notes` is non-empty (the caller checked), so `anchor` is set. + let anchor = anchor.ok_or_else(|| { + PlatformWalletError::ShieldedBuildError( + "no spendable notes selected — anchor undefined".to_string(), + ) + })?; + Ok(Some((spends, anchor))) + }; + + // Fast path: a fully-synced wallet's depth-0 root is a recorded anchor. + let (spends, anchor) = match build_at_depth(0, true)? { + Some(pair) => pair, + // Unreachable — a strict build returns `Some` or errors — but stay + // fund-safe (a clean error, never a panic) if that invariant breaks. + None => { + return Err(PlatformWalletError::ShieldedMerkleWitnessUnavailable( + "depth-0 witness probe returned no witness for a selected note".to_string(), + )); + } + }; + if recorded.contains(&anchor.to_bytes()) { + return Ok((spends, anchor)); } - let anchor = anchor.ok_or_else(|| { - PlatformWalletError::ShieldedBuildError( - "no spendable notes selected — anchor undefined".to_string(), - ) - })?; + // Otherwise walk older checkpoints newest→oldest for the shallowest + // recorded root. + for depth in 1..MAX_ANCHOR_PROBE_DEPTH { + match build_at_depth(depth, false)? { + Some((spends, anchor)) if recorded.contains(&anchor.to_bytes()) => { + return Ok((spends, anchor)); + } + // A root exists at this depth but Platform didn't record it — try an + // older checkpoint. + Some(_) => continue, + // A selected note isn't witnessable this deep; every deeper + // checkpoint is older still, so none can cover it either. + None => break, + } + } - Ok((spends, anchor)) + Err(PlatformWalletError::ShieldedNoRecordedAnchor( + "no recorded anchor covers the selected notes; wait for the next shielded sync".to_string(), + )) } /// Mark the selected notes as spent for `id`. Also queues a @@ -2309,3 +2410,216 @@ mod record_activity_status_tests { assert_eq!(stored.block_height, Some(900)); } } + +/// Unit tests for the pure anchor-selection probe ([`select_recorded_spends`]) +/// against a real SQLite-backed commitment tree — no SDK, no network. +/// +/// These pin the fix for the shielded-withdrawal "never lands" root cause: +/// the wallet must build a spend against a Platform-recorded anchor, not the +/// bleeding-edge depth-0 root a mid-block index-chunk sync leaves behind. They +/// reuse the block-boundary tree shape from the `file_store` reproduction test. +#[cfg(test)] +mod select_recorded_spends_tests { + use super::*; + use crate::wallet::shielded::file_store::FileBackedShieldedStore; + use dashcore::Network; + use grovedb_commitment_tree::{ExtractedNoteCommitment, Note, NoteValue, RandomSeed, Rho}; + + /// Unique temp path for a test tree (no `tempfile` dev-dep). + fn temp_tree_path(tag: &str) -> std::path::PathBuf { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos(); + std::env::temp_dir().join(format!("select_recorded_spends_{tag}_{nanos}.sqlite")) + } + + /// A filler leaf commitment for non-owned positions. Any canonical 32-byte + /// field element works — the probe only needs the tree to grow between + /// blocks so successive checkpoint roots differ. + fn filler_cmx(b: u8) -> [u8; 32] { + let mut c = [0u8; 32]; + c[0] = b; + c + } + + /// Build one real, spendable Orchard note owned by a fixed test seed and + /// return the wallet's `ShieldedNote` view of it. + /// + /// `note_data` is the real serialized note so `deserialize_note` accepts + /// it, and `cmx` is the note's real extracted commitment so that appending + /// `cmx` as the leaf at `position` makes `witness(position, d).root(cmx)` + /// reproduce the tree's anchor at depth `d`. + fn real_note(position: u64) -> ShieldedNote { + let keys = OrchardKeySet::from_seed(&[0x42; 32], Network::Testnet, 0) + .expect("ZIP-32 derivation from a fixed seed"); + let recipient = keys.default_address; + + // rho and rseed must be canonical Pallas base-field elements; not every + // 32-byte pattern is, so scan deterministically for a valid pair drawn + // from disjoint byte regions (mirroring the sync tests' note builders). + let rho = (1u16..=u16::MAX) + .find_map(|n| { + let mut b = [0u8; 32]; + b[0..2].copy_from_slice(&n.to_le_bytes()); + Rho::from_bytes(&b).into_option() + }) + .expect("a canonical rho exists"); + let rseed = (1u16..=u16::MAX) + .find_map(|m| { + let mut b = [0u8; 32]; + b[2..4].copy_from_slice(&m.to_le_bytes()); + RandomSeed::from_bytes(b, &rho).into_option() + }) + .expect("a canonical rseed exists"); + + let value = NoteValue::from_raw(100_000); + let note = Note::from_parts(recipient, value, rho, rseed) + .into_option() + .expect("valid note parts"); + let cmx = ExtractedNoteCommitment::from(note.commitment()).to_bytes(); + + // `recipient(43) || value(8 LE) || rho(32) || rseed(32)` — the exact + // format `deserialize_note` expects. + let mut note_data = Vec::with_capacity(115); + note_data.extend_from_slice(¬e.recipient().to_raw_address_bytes()); + note_data.extend_from_slice(¬e.value().inner().to_le_bytes()); + note_data.extend_from_slice(¬e.rho().to_bytes()); + note_data.extend_from_slice(note.rseed().as_bytes()); + + ShieldedNote { + position, + cmx, + nullifier: [0x07; 32], + block_height: 1, + is_spent: false, + value: 100_000, + note_data, + } + } + + /// Mid-block: the wallet's depth-0 root is not recorded, but a prior + /// block-boundary checkpoint is — the probe must select that older recorded + /// anchor (the shallowest one), never the mid-block depth-0 root Platform + /// never recorded. + #[test] + fn mid_block_selects_prior_recorded_checkpoint_not_depth0() { + let path = temp_tree_path("midblock"); + let mut store = FileBackedShieldedStore::open_path(&path, 100).unwrap(); + + // The owned note lives at position 0, present since block 1. + let note = real_note(0); + + // Block 1 = positions 0,1,2 (leaf 0 is the owned note's cmx). drive + // records ONE anchor per block, at block-processing-end. + store.append_commitment(¬e.cmx, true).unwrap(); + store.append_commitment(&filler_cmx(0xA1), true).unwrap(); + store.append_commitment(&filler_cmx(0xA2), true).unwrap(); + store.checkpoint_tree(3).unwrap(); + let root_block1 = store.tree_anchor().unwrap(); + + // Block 2 = positions 3,4,5. Its block-end root is the second recorded + // anchor. + store.append_commitment(&filler_cmx(0xB1), true).unwrap(); + store.append_commitment(&filler_cmx(0xB2), true).unwrap(); + store.append_commitment(&filler_cmx(0xB3), true).unwrap(); + store.checkpoint_tree(6).unwrap(); + let root_block2 = store.tree_anchor().unwrap(); + + // Mid-block: the index-chunk sync appends one more commitment (position + // 6) and checkpoints there. The depth-0 root is now a state drive never + // recorded. + store.append_commitment(&filler_cmx(0xC1), true).unwrap(); + store.checkpoint_tree(7).unwrap(); + let root_depth0 = store.tree_anchor().unwrap(); + + // drive's recorded set is exactly the two block-boundary roots. + let recorded: HashSet<[u8; 32]> = [root_block1, root_block2].into_iter().collect(); + + let (spends, anchor) = + select_recorded_spends(&store, std::slice::from_ref(¬e), &recorded) + .expect("a prior recorded checkpoint covers the owned note"); + + let _ = std::fs::remove_file(&path); + + assert_eq!(spends.len(), 1, "the single owned note is spendable"); + assert!( + recorded.contains(&anchor.to_bytes()), + "the selected anchor must be a Platform-recorded root" + ); + assert_eq!( + anchor.to_bytes(), + root_block2, + "must pick the shallowest recorded checkpoint (block 2 / depth 1), not a deeper one" + ); + assert_ne!( + anchor.to_bytes(), + root_depth0, + "must NOT use the mid-block depth-0 root drive never recorded" + ); + } + + /// Fully synced: the wallet's depth-0 root IS recorded, so the probe takes + /// the fast path and returns the depth-0 anchor without walking deeper. + #[test] + fn fully_synced_returns_depth0_anchor() { + let path = temp_tree_path("fastpath"); + let mut store = FileBackedShieldedStore::open_path(&path, 100).unwrap(); + + let note = real_note(0); + + // One block, checkpointed exactly on its boundary: depth 0 == a recorded + // anchor. + store.append_commitment(¬e.cmx, true).unwrap(); + store.append_commitment(&filler_cmx(0xA1), true).unwrap(); + store.append_commitment(&filler_cmx(0xA2), true).unwrap(); + store.checkpoint_tree(3).unwrap(); + let root_depth0 = store.tree_anchor().unwrap(); + + let recorded: HashSet<[u8; 32]> = [root_depth0].into_iter().collect(); + + let (spends, anchor) = + select_recorded_spends(&store, std::slice::from_ref(¬e), &recorded) + .expect("depth-0 root is recorded"); + + let _ = std::fs::remove_file(&path); + + assert_eq!(spends.len(), 1); + assert_eq!( + anchor.to_bytes(), + root_depth0, + "the fully-synced fast path returns the depth-0 anchor" + ); + } + + /// No checkpoint root is recorded: the probe exhausts every depth and + /// returns the retryable `ShieldedNoRecordedAnchor` — nothing is broadcast. + #[test] + fn no_recorded_checkpoint_returns_retryable_error() { + let path = temp_tree_path("none"); + let mut store = FileBackedShieldedStore::open_path(&path, 100).unwrap(); + + let note = real_note(0); + + store.append_commitment(¬e.cmx, true).unwrap(); + store.append_commitment(&filler_cmx(0xA1), true).unwrap(); + store.append_commitment(&filler_cmx(0xA2), true).unwrap(); + store.checkpoint_tree(3).unwrap(); + + // Platform recorded none of this wallet's checkpoint roots. + let recorded: HashSet<[u8; 32]> = HashSet::new(); + + // `SpendableNote` isn't `Debug`, so match rather than `expect_err`. + let result = select_recorded_spends(&store, std::slice::from_ref(¬e), &recorded); + + let _ = std::fs::remove_file(&path); + + match result { + Err(PlatformWalletError::ShieldedNoRecordedAnchor(_)) => {} + Err(other) => { + panic!("expected ShieldedNoRecordedAnchor, got error: {other:?}") + } + Ok(_) => panic!("expected ShieldedNoRecordedAnchor, got Ok"), + } + } +} diff --git a/packages/rs-platform-wallet/src/wallet/shielded/store.rs b/packages/rs-platform-wallet/src/wallet/shielded/store.rs index e57ca6d92c..d6f6d1801c 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/store.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/store.rs @@ -252,14 +252,36 @@ pub trait ShieldedStore: Send + Sync { /// Return the current tree root (Sinsemilla anchor, 32 bytes). fn tree_anchor(&self) -> Result<[u8; 32], Self::Error>; - /// Generate a Merkle authentication path for `position` - /// against the current tree state. Returns `Ok(None)` if no - /// witness is available (position not marked, or pruned). - fn witness( + /// Generate a Merkle authentication path for `position` as of the + /// checkpoint at `depth` (0 = current tree state, 1 = the previous + /// checkpoint, and so on). Returns `Ok(None)` when no witness is + /// available at that depth — the position is unmarked/pruned, the + /// requested checkpoint depth doesn't exist, or the position was + /// appended after that checkpoint. + /// + /// Building a spend against an older-but-recorded checkpoint is how the + /// wallet keeps its anchor consistent with a root Platform actually + /// recorded: Platform records one anchor per block while an index-chunk + /// sync routinely leaves the tree mid-block, so the depth-0 root is often + /// one Platform never recorded. + fn witness_at_depth( &self, position: u64, + depth: usize, ) -> Result, Self::Error>; + /// Generate a Merkle authentication path for `position` against the + /// current tree state. Returns `Ok(None)` if no witness is available + /// (position not marked, or pruned). + /// + /// Delegates to [`Self::witness_at_depth`] at depth 0. + fn witness( + &self, + position: u64, + ) -> Result, Self::Error> { + self.witness_at_depth(position, 0) + } + /// Number of leaves currently in the shared commitment tree /// (= highest appended position + 1, or 0 when empty). /// @@ -633,9 +655,10 @@ impl ShieldedStore for InMemoryShieldedStore { Ok(self.anchor) } - fn witness( + fn witness_at_depth( &self, _position: u64, + _depth: usize, ) -> Result, Self::Error> { Err(InMemoryStoreError( "Merkle witness not supported in in-memory store".into(), From 6a845c2ac46223ccb601bca75ede74a6a453e55d Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Thu, 2 Jul 2026 14:34:58 +0700 Subject: [PATCH 3/6] refactor(platform-wallet): address PR review on the shielded anchor probe Polish from the #3977 review (all non-blocking; PR already approved): - select_recorded_spends: deserialize each note + decode its cmx ONCE (hoisted before the depth walk) so each probed depth only re-witnesses. - depth > 0: split Ok(None) (note post-dates this checkpoint -> unusable depth) from Err (a genuine store failure), logging the latter via tracing::warn! before treating the depth as unusable, so the diagnostic isn't swallowed; still non-aborting. - extract_spends_and_anchor: run the empty-notes guard before the ShieldedAnchors round-trip. - MAX_ANCHOR_PROBE_DEPTH: document the max_checkpoints coupling. - test: note_newer_than_recorded_checkpoint_breaks_and_returns_retryable_error pins the walk's early-termination + the value-selection trade-off. - spec: align the Interface section with the as-built witness_at_depth-only design (no separate anchor_at_depth / checkpoint_id_at_depth). - swift: mirror the new FFI code as PlatformWalletResultCode .errorShieldedNoRecordedAnchor = 19 + PlatformWalletError .shieldedNoRecordedAnchor, with the init(ffi:)/init(result:) mappings and a retryable doc-comment (distinct from shieldedSpendUnconfirmed). Wallet 305 tests pass; fmt + clippy clean; iOS build (build_ios.sh) succeeds. Co-Authored-By: Claude Opus 4.8 --- .../SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md | 32 ++-- .../src/wallet/shielded/operations.rs | 147 +++++++++++++----- .../PlatformWallet/PlatformWalletResult.swift | 18 +++ 3 files changed, 142 insertions(+), 55 deletions(-) diff --git a/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md b/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md index 8e0b8bf63c..6fd49c2df8 100644 --- a/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md +++ b/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md @@ -91,21 +91,25 @@ error. root still isn't guaranteed recorded. Rejected. ## Interface / data-flow changes -- `ShieldedStore` trait (breaking; implemented by `FileBackedShieldedStore`, - `InMemoryShieldedStore`, and any host/FFI persister): - - `witness_at_depth(&self, position, depth) -> Result, Err>` - (generalize today's depth-0-only `witness`). - - `anchor_at_depth(&self, depth) -> Result, Err>` — the tree - root at checkpoint depth `d`. Implemented via `witness_at_depth(any_marked_pos, - d).root(cmx)` (the repro-test workaround) to avoid touching the pinned - external `grovedb-commitment-tree` crate. - - `checkpoint_id_at_depth(&self, depth) -> Result, Err>` — the - `checkpoint_id` (== tree size, per `sync.rs`) at depth `d`, for the - `position < size*` note filter. (`InMemoryShieldedStore` may return a stub / - be excluded from the probe path if it lacks a real tree.) +- `ShieldedStore` trait: a single new method + `witness_at_depth(&self, position, depth) -> Result, Err>`; + the existing `witness` becomes a default delegating to `witness_at_depth(pos, 0)` + (so existing callers are unchanged). Implemented by `FileBackedShieldedStore` + (passes `depth` to `ClientPersistentCommitmentTree::witness`) and + `InMemoryShieldedStore` (same stub as before, `depth`-agnostic). No `dyn`/FFI + implementor exists, so the addition is contained. + - *As-built note:* the anchor at a depth is derived **inline** from the + selected notes' own witnesses (`witness_at_depth(note.position, depth).root(cmx)`), + so no separate `anchor_at_depth` was needed. The "note too new for this + checkpoint" case is detected directly by `witness_at_depth` returning + `Ok(None)`/`Err` at that depth (→ the probe stops), so no + `checkpoint_id_at_depth` / explicit `position < size*` filter is needed + either — both were considered in earlier drafts but the localized probe + subsumes them. - `extract_spends_and_anchor` gains `sdk: &Arc` and does the fetch + - probe + filter. It has FOUR callers (audit): `withdraw`, `unshield`, - `shielded_transfer`, `identity_create_from_shielded_pool`. + probe (via the pure, unit-testable `select_recorded_spends`). It has FOUR + callers: `withdraw`, `unshield`, `shielded_transfer`, + `identity_create_from_shielded_pool`. - New retryable error `PlatformWalletError::ShieldedNoRecordedAnchor`; FFI/UI maps it to "still syncing — try again shortly" (distinct from `ShieldedSpendUnconfirmed`). - Query: `ShieldedAnchors::fetch_current(sdk).await` (existing `FetchCurrent` impl). diff --git a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs index f703877241..ea21204591 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs @@ -1509,9 +1509,12 @@ fn default_orchard_address(keys: &OrchardKeySet) -> Result( store: &Arc>, notes: &[ShieldedNote], ) -> Result<(Vec, Anchor), PlatformWalletError> { + // Nothing selected — fail before the network round-trip. + if notes.is_empty() { + return Err(PlatformWalletError::ShieldedBuildError( + "no spendable notes selected — anchor undefined".to_string(), + )); + } + // Fetch the recorded anchor set OUTSIDE the store lock so the network // round-trip doesn't serialize with other store users, and so the lock is // held only for the mutually-consistent depth/witness probe below. @@ -1545,12 +1555,6 @@ async fn extract_spends_and_anchor( dash_sdk::query_types::ShieldedAnchors::fetch_current(sdk).await?; let recorded: HashSet<[u8; 32]> = recorded_anchors.into_iter().collect(); - if notes.is_empty() { - return Err(PlatformWalletError::ShieldedBuildError( - "no spendable notes selected — anchor undefined".to_string(), - )); - } - // Hold a single read lock across the whole probe so the checkpoint depths // and the per-note witnesses stay mutually consistent: a concurrent sync // checkpointing mid-probe would otherwise shift the depth indices out from @@ -1576,35 +1580,54 @@ fn select_recorded_spends( ) -> Result<(Vec, Anchor), PlatformWalletError> { use grovedb_commitment_tree::ExtractedNoteCommitment; + // Deserialize each note and decode its commitment ONCE — both are + // independent of the checkpoint depth, so hoisting them out of the probe + // keeps the depth walk cheap (each probed depth only re-witnesses). + let prepared: Vec<(u64, grovedb_commitment_tree::Note, ExtractedNoteCommitment)> = notes + .iter() + .map(|note| { + let orchard_note = deserialize_note(¬e.note_data).ok_or_else(|| { + PlatformWalletError::ShieldedBuildError(format!( + "Failed to deserialize note at position {}", + note.position + )) + })?; + let cmx = ExtractedNoteCommitment::from_bytes(¬e.cmx) + .into_option() + .ok_or_else(|| { + PlatformWalletError::ShieldedBuildError(format!( + "invalid stored cmx for note at position {}", + note.position + )) + })?; + Ok((note.position, orchard_note, cmx)) + }) + .collect::>()?; + // Build every selected note's `SpendableNote` plus the shared anchor at a // single checkpoint `depth`. // // `strict` (depth 0 only): a missing/failed witness is a hard - // `ShieldedMerkleWitnessUnavailable` (the note is expected to be - // witnessable at the current tip). With `strict == false` (deeper - // checkpoints) a missing/`NotContained` witness means the note post-dates + // `ShieldedMerkleWitnessUnavailable` (the note is expected to be witnessable + // at the current tip). At depth > 0, `Ok(None)` means the note post-dates // this older checkpoint, so the depth is unusable — return `Ok(None)` and - // let the caller stop probing deeper. An anchor disagreement across notes - // is always a hard error (the spend builder would reject it downstream). + // let the caller stop probing deeper; a genuine store `Err` (poisoned mutex, + // IO, tree corruption) is logged — the probe would otherwise discard the + // message — and likewise treated as an unusable depth rather than aborting, + // so a transient read can't strand a spend a shallower depth already + // covered. An anchor disagreement across notes is always a hard error (the + // spend builder would reject it downstream). let build_at_depth = |depth: usize, strict: bool| -> Result, Anchor)>, PlatformWalletError> { - let mut spends = Vec::with_capacity(notes.len()); + let mut spends = Vec::with_capacity(prepared.len()); let mut anchor: Option = None; - for note in notes { - let orchard_note = deserialize_note(¬e.note_data).ok_or_else(|| { - PlatformWalletError::ShieldedBuildError(format!( - "Failed to deserialize note at position {}", - note.position - )) - })?; - - let merkle_path = match store.witness_at_depth(note.position, depth) { + for (position, note, cmx) in &prepared { + let merkle_path = match store.witness_at_depth(*position, depth) { Ok(Some(path)) => path, Ok(None) if strict => { return Err(PlatformWalletError::ShieldedMerkleWitnessUnavailable(format!( - "no witness available for note at position {} (not marked, or pruned past this position)", - note.position + "no witness available for note at position {position} (not marked, or pruned past this position)" ))); } Err(e) if strict => { @@ -1613,37 +1636,39 @@ fn select_recorded_spends( )); } // depth > 0: the note isn't witnessable at this older checkpoint - // (appended after it, or the depth doesn't exist), so this depth - // can't cover the selection — signal "unusable depth". - Ok(None) | Err(_) => return Ok(None), + // (appended after it, or the depth doesn't exist). + Ok(None) => return Ok(None), + // depth > 0: a genuine store failure. Log it so the operator sees + // it (the anchor probe otherwise swallows the message), then treat + // the depth as unusable — never a mid-probe abort. + Err(e) => { + tracing::warn!( + position = *position, + depth, + error = %e, + "shielded anchor probe: witness_at_depth failed at depth > 0; skipping depth" + ); + return Ok(None); + } }; // The anchor is derived from the witness path itself // (`MerklePath::root(cmx)`); all selected notes must agree on it, or // the store handed back witnesses from different checkpoints and the // spend builder would reject the mismatch downstream. - let cmx = ExtractedNoteCommitment::from_bytes(¬e.cmx) - .into_option() - .ok_or_else(|| { - PlatformWalletError::ShieldedBuildError(format!( - "invalid stored cmx for note at position {}", - note.position - )) - })?; - let witness_anchor = merkle_path.root(cmx); + let witness_anchor = merkle_path.root(*cmx); match &anchor { None => anchor = Some(witness_anchor), Some(prev) if prev.to_bytes() != witness_anchor.to_bytes() => { return Err(PlatformWalletError::ShieldedBuildError(format!( - "witness anchor mismatch across selected notes (position {})", - note.position + "witness anchor mismatch across selected notes (position {position})" ))); } _ => {} } spends.push(SpendableNote { - note: orchard_note, + note: *note, merkle_path, }); } @@ -2622,4 +2647,44 @@ mod select_recorded_spends_tests { Ok(_) => panic!("expected ShieldedNoRecordedAnchor, got Ok"), } } + + /// A selected note that post-dates the only recorded checkpoint. Depth 0 + /// (which contains the note) isn't recorded; at depth 1 the note is not yet + /// in the tree, so the probe's early-termination fires (a deeper checkpoint + /// is older still and can't contain it) and the retryable error is returned + /// — even though a recorded anchor exists, it doesn't cover this note. This + /// pins the walk's break arm and the value-selection trade-off (a mid-block + /// wallet whose selected note is newer than every recorded checkpoint waits + /// for the next sync rather than spending). + #[test] + fn note_newer_than_recorded_checkpoint_breaks_and_returns_retryable_error() { + let path = temp_tree_path("toonew"); + let mut store = FileBackedShieldedStore::open_path(&path, 100).unwrap(); + + // Block 1 = positions 0,1,2 (fillers), checkpointed on its boundary — + // the only root Platform recorded. + store.append_commitment(&filler_cmx(0xA0), true).unwrap(); + store.append_commitment(&filler_cmx(0xA1), true).unwrap(); + store.append_commitment(&filler_cmx(0xA2), true).unwrap(); + store.checkpoint_tree(3).unwrap(); + let root_block1 = store.tree_anchor().unwrap(); + + // The owned note is appended AFTER block 1 (position 3) and checkpointed: + // present at depth 0, absent from the block-1 checkpoint (depth 1). + let note = real_note(3); + store.append_commitment(¬e.cmx, true).unwrap(); + store.checkpoint_tree(4).unwrap(); + + let recorded: HashSet<[u8; 32]> = [root_block1].into_iter().collect(); + + let result = select_recorded_spends(&store, std::slice::from_ref(¬e), &recorded); + + let _ = std::fs::remove_file(&path); + + match result { + Err(PlatformWalletError::ShieldedNoRecordedAnchor(_)) => {} + Err(other) => panic!("expected ShieldedNoRecordedAnchor, got error: {other:?}"), + Ok(_) => panic!("expected ShieldedNoRecordedAnchor, got Ok"), + } + } } diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift index 2c311f91e9..45b570e091 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift @@ -39,6 +39,13 @@ public enum PlatformWalletResultCode: Int32, Sendable { /// outcome. Do NOT auto-retry — a retry would rebuild the bundle and /// could double-execute if the original landed. case errorShieldedSpendUnconfirmed = 18 + /// A shielded spend could not be built against a Platform-recorded anchor: + /// the wallet's commitment tree isn't synced to a checkpoint Platform has + /// recorded (an in-progress / interrupted sync leaves it mid-block). Nothing + /// was broadcast and the notes were released. This is retryable — let the + /// shielded sync reach a confirmed state and try again. Distinct from + /// `errorShieldedSpendUnconfirmed`, which must NOT be retried. + case errorShieldedNoRecordedAnchor = 19 case notFound = 98 case errorUnknown = 99 @@ -82,6 +89,8 @@ public enum PlatformWalletResultCode: Int32, Sendable { self = .errorShieldedBroadcastUnconfirmed case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHIELDED_SPEND_UNCONFIRMED: self = .errorShieldedSpendUnconfirmed + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHIELDED_NO_RECORDED_ANCHOR: + self = .errorShieldedNoRecordedAnchor case PLATFORM_WALLET_FFI_RESULT_CODE_NOT_FOUND: self = .notFound case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_UNKNOWN: @@ -177,6 +186,13 @@ public enum PlatformWalletError: LocalizedError { /// notes reserved wallet-side (a shield reserves nothing) until the /// next sync reconciles the outcome. Do NOT auto-retry. case shieldedSpendUnconfirmed(String) + /// A shielded spend could not be built against a Platform-recorded anchor — + /// the wallet's commitment tree isn't synced to a checkpoint Platform has + /// recorded (an in-progress / interrupted sync leaves it mid-block). Nothing + /// was broadcast and the notes were released; retryable once the shielded + /// sync reaches a confirmed state. Distinct from `shieldedSpendUnconfirmed`, + /// which must NOT be retried. + case shieldedNoRecordedAnchor(String) case notFound(String) case unknown(String) @@ -192,6 +208,7 @@ public enum PlatformWalletError: LocalizedError { .arithmeticOverflow(let m), .noSelectableInputs(let m), .walletAlreadyExists(let m), .shieldedBroadcastFailed(let m), .shieldedBroadcastUnconfirmed(let m), .shieldedSpendUnconfirmed(let m), + .shieldedNoRecordedAnchor(let m), .notFound(let m), .unknown(let m): return m } @@ -222,6 +239,7 @@ public enum PlatformWalletError: LocalizedError { case .errorShieldedBroadcastFailed: self = .shieldedBroadcastFailed(detail) case .errorShieldedBroadcastUnconfirmed: self = .shieldedBroadcastUnconfirmed(detail) case .errorShieldedSpendUnconfirmed: self = .shieldedSpendUnconfirmed(detail) + case .errorShieldedNoRecordedAnchor: self = .shieldedNoRecordedAnchor(detail) case .notFound: self = .notFound(detail) case .errorUnknown: self = .unknown(detail) } From 91e0be4ec4543b1196796129e2a7a1f8452ad2c6 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 3 Jul 2026 02:04:39 +0700 Subject: [PATCH 4/6] test(drive-abci): pin acceptance of a real-proof spend at an older recorded anchor The existing coverage proves the rejection side (unrecorded anchor -> InvalidAnchorError) and the fresh-anchor success case, but not the case the wallet-side fix actually produces: witnesses taken at a deeper checkpoint whose block-boundary root Platform recorded, while newer commitments and a newer boundary anchor already exist. This pins that a real Orchard proof built at checkpoint depth 1 validates end-to-end, so the anchor-selection fix's keystone no longer rests on reasoning alone. Co-Authored-By: Claude Fable 5 --- .../shielded_withdrawal/tests.rs | 125 ++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs index f84478ea59..dd9182e489 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs @@ -610,6 +610,131 @@ mod tests { ); } + /// Keystone acceptance test for the anchor-selection fix: a **real** + /// Orchard proof built against an OLDER recorded anchor — with the + /// commitment tree (and the recorded set) advanced past it — must + /// validate successfully. This is exactly the spend shape + /// `platform-wallet`'s `select_recorded_spends` produces after the + /// fix: witnesses taken at a deeper checkpoint whose root Platform + /// recorded, while newer commitments and a newer boundary anchor + /// already exist. `test_valid_shielded_withdrawal_proof_succeeds` + /// covers only the fresh-anchor (depth-0-recorded) case, and + /// `test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error` + /// only the rejection side; this pins the acceptance side the whole + /// fix rests on. + #[test] + fn test_valid_proof_with_older_recorded_anchor_succeeds() { + let platform_version = PlatformVersion::latest(); + let platform = setup_platform(); + insert_dummy_encrypted_notes(&platform, 250); + let mut rng = StdRng::seed_from_u64(0); + let pk = get_proving_key(); + + let sk = SpendingKey::from_bytes([0u8; 32]).unwrap(); + let fvk = FullViewingKey::from(&sk); + let recipient = fvk.address_at(0u32, Scope::External); + let ask = SpendAuthorizingKey::from(&sk); + + let rho_bytes: [u8; 32] = { + let mut b = [0u8; 32]; + b[0] = 1; + b + }; + let rho = Rho::from_bytes(&rho_bytes).unwrap(); + let rseed = RandomSeed::from_bytes([42u8; 32], &rho).unwrap(); + let note = + Note::from_parts(recipient, NoteValue::from_raw(500_000_000), rho, rseed).unwrap(); + + // The spent note lands in "block 1", whose boundary root is + // recorded (anchor_old). Later commitments then arrive and a + // newer boundary is recorded (anchor_new). The spend is built at + // checkpoint depth 1 — the wallet fix's exact output when its + // depth-0 root is unrecorded. + let cmx = ExtractedNoteCommitment::from(note.commitment()); + let mut tree = ClientMemoryCommitmentTree::new(100); + tree.append(cmx.to_bytes(), Retention::Marked).unwrap(); + tree.checkpoint(0u32).unwrap(); // block-1 boundary + + for later in 2u8..=4 { + let mut b = [0u8; 32]; + b[0] = later; + let rho_later = Rho::from_bytes(&b).unwrap(); + let rseed_later = RandomSeed::from_bytes([42u8; 32], &rho_later).unwrap(); + let note_later = + Note::from_parts(recipient, NoteValue::from_raw(1_000), rho_later, rseed_later) + .unwrap(); + let cmx_later = ExtractedNoteCommitment::from(note_later.commitment()); + tree.append(cmx_later.to_bytes(), Retention::Ephemeral) + .unwrap(); + } + tree.checkpoint(1u32).unwrap(); // block-2 boundary + + let anchor_new = tree.anchor().unwrap(); + // Witness at checkpoint depth 1 → path rooted at the OLDER + // (block-1) recorded root, not the tip. + let merkle_path = tree.witness(Position::from(0u64), 1).unwrap().unwrap(); + let anchor_old = merkle_path.root(cmx); + assert_ne!( + anchor_old.to_bytes(), + anchor_new.to_bytes(), + "tree must have advanced past the spend's anchor for this test to be meaningful" + ); + + // --- Build bundle against the older anchor --- + let mut builder = Builder::::new(BundleType::DEFAULT, anchor_old); + builder.add_spend(fvk.clone(), note, merkle_path).unwrap(); + builder + .add_output(None, recipient, NoteValue::from_raw(5_000), [0u8; 36]) + .unwrap(); + let (unauthorized, _) = builder.build::(&mut rng).unwrap().unwrap(); + + let output_script = create_output_script(); + let unshielding_amount = 499_995_000u64; + let extra_sighash_data = dpp::shielded::shielded_withdrawal_extra_sighash_data_v0( + output_script.as_bytes(), + unshielding_amount, + 1, + Pooling::Never, + ); + let bundle_commitment: [u8; 32] = unauthorized.commitment().into(); + let sighash = compute_platform_sighash(&bundle_commitment, &extra_sighash_data); + let proven = unauthorized.create_proof(pk, &mut rng).unwrap(); + let bundle = proven.apply_signatures(rng, sighash, &[ask]).unwrap(); + + let (actions, value_balance, anchor_bytes, proof_bytes, binding_sig) = + serialize_authorized_bundle_i64(&bundle); + assert_eq!(value_balance, 499_995_000); + assert_eq!( + anchor_bytes, + anchor_old.to_bytes(), + "the bundle must carry the older anchor" + ); + + // The recorded set holds BOTH boundaries — like drive after two + // block-ends — and the spend references the older one. + insert_anchor_into_state(&platform, &anchor_bytes); + insert_anchor_into_state(&platform, &anchor_new.to_bytes()); + set_pool_total_balance(&platform, 500_000_000); + + let transition = create_shielded_withdrawal_transition( + actions, + value_balance as u64, + anchor_bytes, + proof_bytes, + binding_sig, + 1, + Pooling::Never, + output_script, + ); + + let processing_result = process_transition(&platform, transition, platform_version); + + assert_matches!( + processing_result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }] + ); + } + #[test] fn test_wrong_encrypted_note_size_returns_error() { let platform_version = PlatformVersion::latest(); From 684bf843068aa00ab7f023cde5dff54a140adec2 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 3 Jul 2026 02:05:10 +0700 Subject: [PATCH 5/6] chore: cargo fmt Co-Authored-By: Claude Fable 5 --- .../state_transitions/shielded_withdrawal/tests.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs index dd9182e489..841eadeed2 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs @@ -660,9 +660,13 @@ mod tests { b[0] = later; let rho_later = Rho::from_bytes(&b).unwrap(); let rseed_later = RandomSeed::from_bytes([42u8; 32], &rho_later).unwrap(); - let note_later = - Note::from_parts(recipient, NoteValue::from_raw(1_000), rho_later, rseed_later) - .unwrap(); + let note_later = Note::from_parts( + recipient, + NoteValue::from_raw(1_000), + rho_later, + rseed_later, + ) + .unwrap(); let cmx_later = ExtractedNoteCommitment::from(note_later.commitment()); tree.append(cmx_later.to_bytes(), Retention::Ephemeral) .unwrap(); From 6a356ca1367e995dea4f41822a27a7b949f455d0 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 3 Jul 2026 02:17:21 +0700 Subject: [PATCH 6/6] docs: drop investigation/spec working notes from the tree docs/ holds durable architecture references; the TestFlight investigation log and the fix spec are working artifacts of this PR. Their content stays available in the PR discussion and in this branch's history, and the shipped mechanism is documented where it lives: extract_spends_and_anchor / select_recorded_spends doc comments and the drive-abci anchor tests. Co-Authored-By: Claude Fable 5 --- .../SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md | 163 ------------------ .../TESTFLIGHT_FEEDBACK_INVESTIGATION.md | 145 ---------------- 2 files changed, 308 deletions(-) delete mode 100644 docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md delete mode 100644 docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md diff --git a/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md b/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md deleted file mode 100644 index 6fd49c2df8..0000000000 --- a/docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md +++ /dev/null @@ -1,163 +0,0 @@ -# Spec — shielded spend uses a Platform-recorded anchor - -Fixes the reproduced root cause of TestFlight report B (see -`TESTFLIGHT_FEEDBACK_INVESTIGATION.md`): shielded spends (withdraw / unshield / -transfer) are rejected with `InvalidAnchorError` because the wallet builds the -proof against its **depth-0** commitment-tree root, which — with an index-chunk -sync (`CHUNK_SIZE = 2048`) that routinely stops mid-block — is a root Platform -never recorded (drive records **one anchor per block**, retains 1000). - -Reproduced by two passing tests (committed `6959fe967e`): -`platform-wallet …depth0_spend_anchor_mid_block_is_not_a_recorded_block_boundary_anchor` -and `drive-abci …test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error`. - -## Goal / non-goal - -- **Goal:** a shielded spend the wallet builds is accepted by Platform's - `validate_anchor_exists` — i.e. its anchor is always a drive-recorded root. -- **Goal:** when the wallet cannot yet build against a recorded anchor, fail - with a **clear, retryable** error instead of broadcasting a doomed transition - that surfaces as "may have gone through." -- **Non-goal (this spec):** the reservation-release / `ShieldedSpendUnconfirmed` - misclassification (tracked as "A"); UX copy. Called out where they interact. - -> **Revised after 3-reviewer audit** (soundness / feasibility / scope). The -> cryptographic core is confirmed sound; changes folded in: drop the -> most-recent-anchor fast-path; `anchor_at_depth` is an explicit store-trait -> addition (implemented via the `witness(marked_pos, d).root(cmx)` workaround to -> avoid an external `grovedb-commitment-tree` crate change); position-aware note -> selection is required (B1); the "no recorded checkpoint" outcome (B2) is a -> graceful, fund-safe retryable error — auto-enable in that pathological case is -> a follow-up (see Scope). - -## Chosen approach: build against the most-recent recorded anchor - -Instead of `witness(pos, 0)` (bleeding-edge root), select the **shallowest -checkpoint whose root Platform has recorded**, and witness + prove against that. - -Data flow, per spend (`extract_spends_and_anchor`): - -1. **Fetch the recorded anchor set** from Platform: `ShieldedAnchors::fetch_current(sdk)` - (`ShieldedAnchors(Vec<[u8;32]>)`) — the ≤1000 retained roots, into a `HashSet`. - (Always the full set — `GetMostRecentShieldedAnchor` would *not* match in the - target mid-block case, since the wallet's recorded checkpoint is a *prior* - block, not the latest.) -2. **Walk the wallet's checkpoints newest→oldest**, `d = 0..max_checkpoints`. For - each `d`, compute the tree root at `d` (`anchor_at_depth(d)`); take the first - `d` whose root ∈ the recorded set — `d*` / `anchor*`, with tree size - `size* = checkpoint_id@d*`. `d = 0` is the fully-synced fast path. -3. **Select notes confined to `d*`:** filter candidate `unspent_notes` to - `position < size*` *before* value-based coverage selection, so every selected - note is witnessable at `d*`. -4. **Witness every selected note at `d*`** (`witness(pos, d*)`), assert each - witness root equals `anchor*`, and pass `anchor*` to the proof builder. -5. **Failure → retryable, no broadcast:** - - No `d` in `0..max_checkpoints` has a recorded root → `ShieldedNoRecordedAnchor`. - - A `d*` exists but confirmed-at-`d*` balance (notes with `position < size*`) - can't cover amount+fee → `ShieldedNoRecordedAnchor` (same class: "wait for - the next sync"). Never surface `ShieldedMerkleWitnessUnavailable` here. - -Note requirement (B1): value-based selection alone would pick a note newer than -`d*`, whose `witness(pos, d*)` returns `Err(NotContained)` (→ opaque -`ShieldedMerkleWitnessUnavailable`). The `position < size*` pre-filter prevents -this; deeper `d` only has *fewer* eligible notes, so if the shallowest recorded -`d*` can't cover the amount, no deeper one can — fail fast with the retryable -error. - -### Why this approach -- **Correct by construction:** the anchor is a value Platform recorded, so - `validate_anchor_exists` passes; the drive repro test's rejection cannot occur. -- **Uses existing primitives:** `GetShieldedAnchors` query + `witness(pos, depth)` - already exist; the proof builder already takes an explicit `anchor`. -- **No stream/tree-format change:** avoids block-aligned checkpointing, which is - infeasible because the sync stream exposes only a per-chunk `block_height`, not - per-commitment block boundaries. -- **Fund-safe:** spending against an older recorded anchor is standard shielded - practice; the note is unspent, the witness root equals the anchor, and the - nullifier set is the authoritative double-spend guard. - -## Alternatives rejected -- **Block-aligned checkpointing** (checkpoint at each block's last commitment): - cleanest in theory, but the sync stream gives only per-chunk `block_height` - (chain tip at response time), not per-commitment block membership — the wallet - cannot identify block boundaries within a chunk. Rejected as infeasible without - a drive/DAPI query-shape change. -- **Precondition check only** (verify depth-0 anchor is recorded; else clear - error, don't broadcast): safe and small, but does **not** enable withdrawal - when mid-block — leaves the user unable to withdraw. Kept as the *fallback* - branch (step 4), not the whole fix. -- **Depth-`N`-back heuristic** (always spend `N` checkpoints back): fragile — `N` - checkpoints is not a fixed number of blocks, and a chunk-boundary checkpoint - root still isn't guaranteed recorded. Rejected. - -## Interface / data-flow changes -- `ShieldedStore` trait: a single new method - `witness_at_depth(&self, position, depth) -> Result, Err>`; - the existing `witness` becomes a default delegating to `witness_at_depth(pos, 0)` - (so existing callers are unchanged). Implemented by `FileBackedShieldedStore` - (passes `depth` to `ClientPersistentCommitmentTree::witness`) and - `InMemoryShieldedStore` (same stub as before, `depth`-agnostic). No `dyn`/FFI - implementor exists, so the addition is contained. - - *As-built note:* the anchor at a depth is derived **inline** from the - selected notes' own witnesses (`witness_at_depth(note.position, depth).root(cmx)`), - so no separate `anchor_at_depth` was needed. The "note too new for this - checkpoint" case is detected directly by `witness_at_depth` returning - `Ok(None)`/`Err` at that depth (→ the probe stops), so no - `checkpoint_id_at_depth` / explicit `position < size*` filter is needed - either — both were considered in earlier drafts but the localized probe - subsumes them. -- `extract_spends_and_anchor` gains `sdk: &Arc` and does the fetch + - probe (via the pure, unit-testable `select_recorded_spends`). It has FOUR - callers: `withdraw`, `unshield`, `shielded_transfer`, - `identity_create_from_shielded_pool`. -- New retryable error `PlatformWalletError::ShieldedNoRecordedAnchor`; FFI/UI maps - it to "still syncing — try again shortly" (distinct from `ShieldedSpendUnconfirmed`). -- Query: `ShieldedAnchors::fetch_current(sdk).await` (existing `FetchCurrent` impl). - -## Failure modes / risks -- **Anchor/witness mismatch** → proof rejected. Mitigation: assert each selected - note's `witness(pos, d*).root(cmx)` equals `anchor*` (extends today's "all notes - agree" check to equal the *selected* anchor). -- **Concurrency (C1):** depth indices shift if a sync checkpoints concurrently. - Mitigation: fetch the anchor set *outside* the store lock; then do the - probe (`anchor_at_depth`) **and** all note witnesses under a **single** store - read-lock hold so `d*`, `size*`, and the witnesses are mutually consistent. -- **Recorded set (≤1000)** — `HashSet` membership; one round trip per spend (rare). -- **Pruning race** — a freshly-queried anchor has ~1000 blocks of runway; a deep - `d*` erodes it but stays within the window at query time. Negligible. -- **No recorded checkpoint / insufficient confirmed-at-`d*` funds** → clean - `ShieldedNoRecordedAnchor`; before broadcast, so the generic `Err` arm runs - `cancel_pending` (reservation released) in all four callers. No fund risk. -- **Double-spend:** unchanged — the Orchard nullifier is `(nk, ρ, ψ, cm)`-derived, - anchor-independent; the on-chain nullifier set stays authoritative (auditor-confirmed). - -## Test plan -- **Keep** the two reproduction tests (they pin the pre-fix mechanism + the drive - contract; both remain valid). -- **Wallet unit (new):** given a tree with recorded anchors at block-boundary - sizes {B1,B2} and a mid-block depth-0 state, `select_recorded_anchor` returns - `B2`'s anchor (most-recent recorded), not the depth-0 root; and witnessing at - that depth yields a root ∈ recorded set. -- **Wallet unit (new):** no checkpoint root recorded → `ShieldedNoRecordedAnchor`. -- **drive-abci (existing, real-proof):** a withdrawal built against a **recorded** - anchor succeeds (`…_proof_succeeds` already covers this) — confirms the fix's - output shape is accepted. -- **Regression:** full `platform-wallet --features shielded` + the shielded - drive-abci suite green; `clippy --all-features`; `fmt --check`. - -## Rollout / scope -- **This PR:** the wallet-only anchor-selection fix across all four spend paths + - the `ShieldedNoRecordedAnchor` error + position-aware selection + tests. No - protocol/drive/DAPI change. -- **Follow-up 1 (Report A — convergence):** ensure a shielded sync reliably - reaches the tip (isn't stranded mid-chunk by wallet-switch/backgrounding), so a - recorded checkpoint is actually created. Without it, a chronically-interrupted - wallet gets the clean `ShieldedNoRecordedAnchor` error but still can't withdraw. -- **Follow-up 2 (robust auto-enable, protocol):** have `GetShieldedAnchors` / - `GetMostRecentShieldedAnchor` return each anchor's **tree size** (auditor's - suggestion) so the wallet can deliberately checkpoint on a recorded boundary and - know exactly which notes are confirmed — collapses B1+B2 but needs drive + DAPI - + proof-verifier + SDK changes. -- **Follow-up A (classification/reservation):** the `ShieldedSpendUnconfirmed` - "may have gone through" misclassification + no-restart reservation release. - This fix removes the dominant *cause*; A improves the *residual* ambiguous case. diff --git a/docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md b/docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md deleted file mode 100644 index d0a3a5dbc5..0000000000 --- a/docs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.md +++ /dev/null @@ -1,145 +0,0 @@ -# Shielded-pool TestFlight feedback — investigation - -Tracking two TestFlight feedback reports against build **3** (`cfBundleShortVersion 1.0`, `cfBundleVersion 3`, appAppleId `6782996681`). Both concern the shielded (Orchard) pool. Source exports: `~/Downloads/testflight_feedback*.zip`. - -## Reports - -### Report B — shielded→Core withdrawal never confirms ("may have gone through") — PRIMARY -- **Feedback id:** `AHlzEsyX33yEvRlSE4Qtm34`, 2026-06-29T18:44 UTC -- **Device:** iPhone18,2 (iPhone 16-class), iOS 26.6, locale pl-PL, tz Europe/Rome, uptime ~398 s -- **Comment (verbatim):** "Yesterday I sent tx from shielded to core and had the same msg as above. Today I checked and the balance was not sent. Today I have again info it might go through but will see later if it gone through or not." -- **Screenshot:** "Send Dash" sheet, recipient a **Core address** (`yTDFHbti48ArHwTszgM…`), **Transaction Type: Withdrawal to Core**, **Send From: Shielded** (0.2719731 DASH selected). A modal titled **"Success"** with body **"Transaction may have gone through — waiting for the next shielded sync to confirm. Do not retry."** + a **Done** button. -- **Reproduced across two days** with the same outcome (balance not actually moved). - -### Report A — switching wallets during a shielded sync — THIN -- **Feedback id:** `ACKbeJnBYOVKfN6dXEPFXG8`, 2026-06-28T22:35 UTC (uploaded twice — `testflight_feedback.zip` and `(1).zip` are byte-identical) -- **Device:** iPhone13,2 (iPhone 12 Pro), iOS 26.5, locale en-US, carrier AT&T, tz America/Los_Angeles -- **Comment (verbatim, truncated by TestFlight):** "Switching between wallets after starting a shielded sync " -- **No screenshot, no crash log.** Insufficient to reproduce yet; needs the full comment or a crash report. - -## Mechanism traced (Report B) - -The dialog is **not** an ad-hoc string — it is the deliberate handling of `PlatformWalletError::ShieldedSpendUnconfirmed`. - -1. UI: `SendViewModel.swift:734-744` catches `PlatformWalletError.shieldedSpendUnconfirmed` and surfaces it through the **success** path (title "Success"), explicitly to avoid inviting a retry that could double-spend. -2. Rust: `operations.rs::classify_spend_wait_failure` (≈1896) classifies a post-broadcast `wait_for_response` failure: - - `carries_consensus_rejection(wait_err)` → `ShieldedBroadcastFailed` (Platform executed + rejected on merits). - - **otherwise** (DAPI timeout / internal error / `StateTransitionBroadcastError` with empty consensus data → `cause: None`) → **`ShieldedSpendUnconfirmed`** — the note reservations are **kept**; the next sync reconciles. - -So the withdrawal **is broadcast**, then the wait-for-result fails **ambiguously** (no consensus verdict), and the reservation is intentionally held. The user hitting this **repeatedly** means the withdrawal ST's wait-for-result keeps failing ambiguously and the tx is not landing. - -### Broadcast → wait → classify (precise path) - -`operations.rs::broadcast_shielded_spend` (≈1785): - -1. `state_transition.broadcast(sdk, None)`: - - `Ok` → fall through to the wait. - - `broadcast_definitely_failed(e)` (consensus rejection, gRPC verdict code, `NoAvailableAddresses`) → `ShieldedBroadcastFailed` → outer match **releases** the reservation. - - **otherwise** (`AlreadyExists`, `DeadlineExceeded`, `Cancelled`, `Unknown`, `Internal`, `Aborted`, `DataLoss`, …) → warn "may have been admitted" → **fall through to the wait anyway**. -2. `state_transition.wait_for_response::(sdk, None)`: - - `Ok` → Confirmed. - - `Err` → `classify_spend_wait_failure`: consensus rejection → `ShieldedBroadcastFailed`; **else → `ShieldedSpendUnconfirmed`** (keep reservation). - -The ambiguous bucket is deliberately wide — both a broadcast that *probably* failed (ambiguous transport error) and a wait that timed out land in `ShieldedSpendUnconfirmed`, on the conservative "a lost ACK may have delivered, so don't re-spend" principle. - -## Funds safety (confirmed — no permanent loss) - -`pending_nullifiers` is **in-memory only** (`operations.rs:757-764`): -- If the tx **landed** → the next shielded sync marks the spent notes spent (clears the reservation). -- If the tx **never landed** → an **app restart drops the in-memory reservation and frees the notes**. - -So the notes are **not** permanently stranded — consistent with the user seeing the balance "back" the next day (app relaunch between sessions). The cost is UX: within a session the funds appear reserved/unavailable, the user is told "do not retry," and there is no clear resolution short of relaunch. - -### Confirmed gap: no sync-time release of a stale reservation - -The reservation set `pending_nullifiers` (`store.rs:348`) is only ever cleared by: -- `mark_spent` — when a later **sync proves the note spent** (the tx landed); or -- `clear_pending` — called by the spend-flow finalizer on a **definite** failure; or -- **process restart** — `pending_nullifiers` is in-memory only and is never persisted, so a relaunch rebuilds the store without it. - -There is **no reconcile path that releases a reservation whose tx demonstrably never landed**. For the `ShieldedSpendUnconfirmed` outcome the reservation is held indefinitely within the session, the activity row stays `Pending`, and the UI shows "do not retry" — so a genuinely non-landing withdrawal **strands the notes until the user force-quits the app**. This is the highest-leverage, most testable defect in the report, independent of *why* the withdrawal failed to land. - -## Severity - -- **Not** fund-loss. Funds return to spendable after a restart + sync. -- **Reliability bug:** the shielded→Core withdrawal repeatedly fails to confirm (ambiguous wait failure) — the user cannot complete a withdrawal. -- **UX bug:** modal title "Success" contradicts "may have gone through"; "Do not retry" with no confirmation path leaves the user stuck; funds appear unavailable until relaunch. - -## Open questions / hypotheses (Report B reliability) - -1. **Where does the ambiguity originate?** Is `wait_for_state_transition_result` timing out at DAPI, or is the ST being rejected without a surfaced consensus verdict? (`classify_spend_wait_failure` treats both as "unconfirmed.") -2. **Does the ST actually reach mempool / execute?** If it never lands across the next sync, is it never accepted, or accepted-but-slow? -3. **Withdrawal-specific?** Do shield / unshield / shielded-transfer confirm fine while only "withdrawal to Core" (transparent recipient) stalls? Points at the transparent-output path or its proof/fee handling. -4. **Network:** which network was the reporter on (mainnet/testnet)? DAPI wait reliability differs. - -## Investigation / reproduction plan - -Code trace (done): -- [x] Read the withdraw-to-Core operation (`operations.rs` ≈930-1070) end to end: reserve → build → record-pending → broadcast → wait → classify. -- [x] Map `broadcast_shielded_spend` + `broadcast_definitely_failed` + `classify_spend_wait_failure` — the ambiguous bucket. -- [x] Confirm the reservation lifecycle: a non-landing reservation is released **only** on tx-landing or restart — no sync-time release. **This is the key fixable gap.** - -Reproduction (needs a shielded environment + funds): -- [ ] Stand up / reuse a shielded devnet (see `~/.claude/.../reference-devnet-deploy-and-shielded-image-build.md`) or confirm which network the reporter used. -- [ ] Fund a shielded note; attempt a shielded→Core withdrawal via the Rust SDK / a focused harness with `tracing` at `debug`. -- [ ] Capture the exact failure shape: does `broadcast()` return Ok then `wait_for_response` time out, or does `broadcast()` itself return an ambiguous error? Capture the `wait_err`/transport code. -- [ ] Determine whether the ST ever reaches mempool / a block (does it land late, or never?) — distinguishes "slow confirm" from "genuine non-landing." -- [ ] For Report A: obtain the untruncated comment / any crash log; attempt a wallet-switch-during-shielded-sync repro in the simulator. - -Two fix tracks (independent): -- **Reservation-staleness (high leverage, unit-testable now):** add a bounded sync-time release for a `ShieldedSpendUnconfirmed` reservation whose nullifier is still absent on chain after the spend can no longer be valid (anchor/expiry window elapsed) — free the notes **without** a restart and flip the activity to a retryable state. Testable at the Rust layer without the network. -- **Root cause (needs repro):** why the withdrawal ST does not land — broadcast transport, wait timeout sizing for proof-heavy shielded STs, or an execution-time rejection that never surfaces a consensus verdict. - -## Root cause (B) — code investigation findings (testnet) - -**Wait has no client timeout.** `withdraw` calls `wait_for_response(sdk, None)`; with `wait_timeout = None` the SDK waits *without* a client-side duration cap (`rs-sdk/.../broadcast.rs:246`). So "the wait timed out too soon" is **not** the cause — `ShieldedSpendUnconfirmed` fires only when the DAPI result genuinely never resolves. - -**Leading hypothesis: anchor mismatch (`validate_anchor_exists`).** The drive-abci shielded-withdrawal validation (`shielded_withdrawal/transform_into_action/v0/mod.rs`) rejects on, among others: -- `validate_minimum_pool_notes` (anonymity floor), -- `InvalidShieldedProofError` (Orchard proof fails), -- **`validate_anchor_exists`** — the transition's `anchor` (commitment-tree Merkle root at build time) must exist in Platform's recorded-anchors tree, -- `validate_nullifiers` (double-spend / intra-bundle dup). - -The wallet computes the withdrawal anchor **locally** (`operations.rs::extract_spends_and_anchor`): `store.witness(note.position).root(cmx)` — the root of the wallet's own commitment tree at the note's checkpoint. Platform accepts it only if that exact root is one it recorded. A wallet whose local tree diverges from any Platform-recorded checkpoint (sync lag, frontier discrepancy, or a note in the local tree Platform hasn't recorded) produces an anchor Platform never had → **rejected every attempt**. This fits the report precisely: repeatable, never lands, funds untouched (the ST fails, notes aren't spent), and — because the rejection apparently doesn't reach the wallet as a clean consensus verdict — it lands in the ambiguous `ShieldedSpendUnconfirmed` bucket ("may have gone through") instead of a clear failure. - -**Why it may not surface as a clear failure:** if the rejection is delivered as a `StateTransitionBroadcastError` with empty consensus data (or the result is never retrievable because the ST is refused pre-block), `classify_spend_wait_failure` / `broadcast_definitely_failed` treat it as ambiguous → unconfirmed. - -### Code-dive conclusion (high confidence) - -The anchor the wallet submits is **not guaranteed to be a Platform-recorded anchor**, by construction: - -| Side | Behaviour | -|---|---| -| **Wallet anchor** | `extract_spends_and_anchor` → `witness(position, 0)` — **depth 0 = the current tree root** (matches the proof's `tree_anchor()`, also depth 0). | -| **Wallet sync** | appends commitments in `CHUNK_SIZE` units (`aligned_start = already_have / CHUNK_SIZE * CHUNK_SIZE`) and checkpoints at the post-append leaf count — **aligned to chunk/stream boundaries, never to block boundaries**. | -| **drive recording** | `record_anchor_if_changed` runs at **block-processing-end** — exactly **one anchor per block** (the block-end root), retained `shielded_anchor_retention_blocks = 1000`. | -| **drive validation** | `validate_anchor_exists` → `has_shielded_anchor(anchor)`; absent → **`InvalidAnchorError`**. | - -So the wallet's depth-0 root equals a drive-recorded anchor **only** when its current tree happens to sit exactly on a block-end commitment count. Any other state — mid-block stream stop, commitments appended past the last block drive recorded, or a `CHUNK_SIZE` boundary that isn't a block boundary — yields a root **Platform never recorded** → every withdrawal rejected. The developers already flag this exact hazard at `sync.rs:548` (*"the depth-0 witness then reflects a state Platform never recorded"*) and fixed only the checkpoint-id-dedup variant; the **block-alignment gap remains unguarded**. This matches the report precisely (repeatable, never lands, funds untouched) and is consistent with the rejection arriving ambiguously enough to be misclassified as `ShieldedSpendUnconfirmed`. - -Note: shield (deposit) does **not** spend, so it carries no anchor — which is why the user could fund a shielded balance yet never complete a spend. The same depth-0 anchor is used by `unshield` and `shielded_transfer`, so those are expected to be equally affected. - -### Fix direction (B) -Build the spend/withdrawal proof against a **recorded** anchor, not the bleeding-edge depth-0 root: select the most-recent checkpoint whose root drive actually recorded (a block-aligned, within-1000-block root) and generate the witness against **that** checkpoint. This is the standard shielded-wallet pattern (spend against a confirmed anchor, not the tip). Requires the wallet to know which of its checkpoints are block-aligned/recorded — i.e., associate checkpoints with the block heights drive records anchors at. - -### Still worth a testnet datapoint -A single testnet `tracing=debug` withdrawal capturing `InvalidAnchorError` (vs. a different rejection) converts "high confidence" to "confirmed" before investing in the fix. Cheap if a shielded-funded testnet wallet already exists; otherwise it is the multi-step bootstrap. - -### Separable bug (overlaps with A) -A definitively-rejected withdrawal is **misclassified** — it should surface as a clear failure (release the reservation, allow retry), not "may have gone through." That is the **A** reservation-release / classification work. - -## Reproduction — hard evidence (deterministic, no testnet) - -Two committed tests reproduce the root cause end to end, without a testnet: - -1. **Wallet produces a non-recorded anchor** — `platform-wallet` (`--features shielded`), `wallet::shielded::file_store::tests::depth0_spend_anchor_mid_block_is_not_a_recorded_block_boundary_anchor` — **PASSES**. On the real SQLite-backed commitment tree it appends two "blocks" of commitments, captures the depth-0 `tree_anchor()` at each block boundary (what drive records), then stops **mid-block** (index-chunk sync) and shows the wallet's mid-block anchor is **neither** recorded boundary anchor. It also pins that the spend anchor `witness(0).root(cmx)` equals that mid-block `tree_anchor()` — i.e. the value the withdrawal actually submits. - -2. **drive rejects that anchor** — `drive-abci` (`--features shielded_test_data`), `…shielded_withdrawal::tests::…::test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error` — a **real** Orchard proof, correct value balance, sufficient pool, but the anchor is not recorded (no `insert_anchor_into_state`) → `StateError::InvalidAnchorError`. Identical to `test_valid_shielded_withdrawal_proof_succeeds` except for the missing anchor record, isolating the anchor as the sole cause. - -Chain: **(1)** the wallet submits a mid-block anchor drive never recorded; **(2)** drive rejects exactly that with `InvalidAnchorError`; the proof passes first (proof-before-anchor order), so the failure isn't a proof/structure problem — it's the anchor. This is the user's "withdrawal never lands," confirmed in code. - -## UX follow-ups (independent of the root cause) - -- Don't title the ambiguous outcome "Success." Use a neutral "Submitted — confirming" state. -- After the next sync reconciles, surface the **definitive** outcome (landed vs returned-to-spendable) instead of leaving the user guessing. -- Consider releasing the reservation on the reconcile pass (not only on restart) so funds free up without a relaunch.