fix(platform-wallet): build shielded spends against a Platform-recorded anchor#3977
Conversation
…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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
✅ Review complete (commit 6a356ca) |
📝 WalkthroughWalkthroughAdds Platform-recorded anchor selection for shielded spends, a depth-aware shielded witness API, retryable ChangesShielded anchor selection fix
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Well-scoped fix: extract_spends_and_anchor now probes Platform-recorded checkpoint depths and returns a retryable pre-broadcast error when none match, with strong test coverage against a real SQLite-backed tree. No blockers: the fix is fund-safe, no consensus-safety issues, and the PR body already documents the acknowledged follow-ups (Swift mirror + potential note re-selection improvement). Two agent 'blocking' claims are actually documented design trade-offs and are downgraded here.
🟡 4 suggestion(s) | 💬 4 nitpick(s)
Findings not posted inline (1)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:83-90: Swift mirror missing case forErrorShieldedNoRecordedAnchor = 19— Rust now emitsPLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHIELDED_NO_RECORDED_ANCHOR = 19as a retryable pre-broadcast condition intentionally distinct fromShieldedSpendUnconfirmed = 18, but the Swift result-code initializer falls through to.errorUnknownat thedefault:arm and `PlatformWal...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1602-1619: Depth>0 branch collapses genuine store errors into retryable no-anchor state
At depth > 0, the match arm `Ok(None) | Err(_) => return Ok(None)` treats real `witness_at_depth` failures (poisoned mutex in `FileBackedShieldedStore`, underlying SQLite/IO errors, tree corruption) identically to the expected `NotContained`/not-yet-checkpointed case. Both then feed the outer loop's `None => break` and surface as the retryable `ShieldedNoRecordedAnchor("wait for the next shielded sync")`. The trait contract explicitly distinguishes the two — `Ok(None)` means the depth is unusable, `Err(_)` means the store itself failed — and depth 0 already preserves that distinction by mapping `Err(e)` to `ShieldedMerkleWitnessUnavailable(e.to_string())`. The consequence is diagnostic, not fund-safety (nothing is broadcast on this path), but the operator loses the underlying error message entirely and the user is told to wait for a sync that will never make this succeed. Split the arm so `Err(e)` at depth > 0 propagates (or at minimum emits a `tracing::warn!` before returning `Ok(None)`).
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1677-1687: Note re-selection over depth-eligible notes not attempted before returning retryable error
`select_notes` in `note_selection.rs` sorts unspent notes by value descending only — position is not considered. If a mid-block wallet has a newly-appended largest note plus older notes that could cover amount+fee, the greedy selector picks the new note; `build_at_depth(depth > 0, false)` then returns `None` because that note post-dates every recorded checkpoint, the walk breaks at the first depth, and the caller returns `ShieldedNoRecordedAnchor` even though the wallet could spend now against a recorded anchor by re-running selection over notes with `position < recorded_checkpoint_size`. The PR body documents this as the intentional design trade-off ("stopping once a selected note post-dates the checkpoint"), so this is not a fund-safety issue and the current path is strictly better than pre-PR (which failed silently). But the tests use a single note at position 0 and never exercise the mixed old/new balance case — the retryable error is fine, but consider whether the deferred `GetShieldedAnchors`-returns-tree-size follow-up is enough, or whether a position-aware selection pass (perhaps as a fallback before returning the retryable error) is worth doing here to reduce the "user asked to wait but the wallet could have spent now" surface.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:83-90: Swift mirror missing case for `ErrorShieldedNoRecordedAnchor = 19`
Rust now emits `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHIELDED_NO_RECORDED_ANCHOR = 19` as a retryable pre-broadcast condition intentionally distinct from `ShieldedSpendUnconfirmed = 18`, but the Swift result-code initializer falls through to `.errorUnknown` at the `default:` arm and `PlatformWalletError.init(result:)` then produces `.unknown`. On-device the user sees a generic unknown-error path instead of the "still syncing — try again shortly" retryable UX this PR set up. The PR body explicitly lists the Swift mirror as a follow-up, so this is not a regression, but the Rust-side fix isn't end-to-end effective until the Swift case + `SendViewModel` branch land. Either land the Swift case in this PR, or ensure the follow-up is tracked and the Rust `message` string is at least surfaced in the meantime so users see the retryable copy.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:83-90: Swift mirror missing case for `ErrorShieldedNoRecordedAnchor = 19`
Rust now emits `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_SHIELDED_NO_RECORDED_ANCHOR = 19` as a retryable pre-broadcast condition intentionally distinct from `ShieldedSpendUnconfirmed = 18`, but the Swift result-code initializer falls through to `.errorUnknown` at the `default:` arm and `PlatformWalletError.init(result:)` then produces `.unknown`. On-device the user sees a generic unknown-error path instead of the "still syncing — try again shortly" retryable UX this PR set up. The PR body explicitly lists the Swift mirror as a follow-up, so this is not a regression, but the Rust-side fix isn't end-to-end effective until the Swift case + `SendViewModel` branch land. Either land the Swift case in this PR, or ensure the follow-up is tracked and the Rust `message` string is at least surfaced in the meantime so users see the retryable copy.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs (1)
514-612: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect regression test; consider extracting shared bundle-construction helper.
Test logic is correct — omitting
insert_anchor_into_statewhile keeping the proof valid should triggerInvalidAnchorErrorviavalidate_anchor_exists, matching the assertion. This ~90-line block duplicates the note/tree/bundle construction fromtest_valid_shielded_withdrawal_proof_succeeds(lines 409-512) almost verbatim, and a similar helper (build_valid_shielded_withdrawal_bundle) already exists inmod security_audit. Promoting that helper to a shared test utility (or module-levelsuper::helper) would let this test callbuild_valid_shielded_withdrawal_bundle(...)and skip straight to omittinginsert_anchor_into_state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs` around lines 514 - 612, The test behavior is correct, but the long setup in test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error duplicates the bundle-construction flow from test_valid_shielded_withdrawal_proof_succeeds and the existing build_valid_shielded_withdrawal_bundle helper in mod security_audit. Extract or reuse a shared helper for note/tree/builder/proof creation, then have this test call that helper and only omit insert_anchor_into_state before process_transition so the assertion still targets validate_anchor_exists and InvalidAnchorError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md`:
- Around line 93-105: The interface section is out of sync with the actual
ShieldedStore API: it mentions anchor_at_depth and checkpoint_id_at_depth even
though the trait currently exposes witness_at_depth plus tree_anchor/tree_size.
Update the SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC wording to either add the missing
methods to ShieldedStore consistently or rephrase this section to describe the
helper behavior using the existing method names, and keep the references to
ShieldedStore, FileBackedShieldedStore, and InMemoryShieldedStore aligned
throughout.
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 127-136: The Swift mirror for PlatformWalletResult is missing the
new Rust error case, so `errorShieldedNoRecordedAnchor = 19` currently falls
through to `.errorUnknown`. Update `PlatformWalletResult` by adding the new enum
case alongside `errorShieldedSpendUnconfirmed`, and extend `init(ffi:)` to map
the FFI value 19 to that case so Swift can distinguish the retryable no-anchor
state from spend-unconfirmed.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs`:
- Around line 514-612: The test behavior is correct, but the long setup in
test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error duplicates
the bundle-construction flow from test_valid_shielded_withdrawal_proof_succeeds
and the existing build_valid_shielded_withdrawal_bundle helper in mod
security_audit. Extract or reuse a shared helper for note/tree/builder/proof
creation, then have this test call that helper and only omit
insert_anchor_into_state before process_transition so the assertion still
targets validate_anchor_exists and InvalidAnchorError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00c4cd03-753c-4027-a8a6-b4d7c1a5de97
📒 Files selected for processing (9)
docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.mddocs/shielded/TESTFLIGHT_FEEDBACK_INVESTIGATION.mdpackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/shielded/file_store.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/store.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.0-dev #3977 +/- ##
==========================================
Coverage 87.18% 87.19%
==========================================
Files 2632 2634 +2
Lines 327563 327718 +155
==========================================
+ Hits 285592 285748 +156
+ Misses 41971 41970 -1
🚀 New features to boost your workflow:
|
|
CodeRabbit retry completed. I agree these are nonblocking cleanup items rather than blockers: the Swift My approval/merge judgment is unchanged; these are follow-up cleanup points for the branch owner to decide whether to fold in. |
shumkov
changed the title
…robe 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 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The current head fixes the prior Swift result-code mapping, empty-selection ordering, redundant note decoding, and early-break test coverage. Two non-blocking issues remain: the new file-backed store method still leaks expected shardtree absence through the error channel, and the value-first selection model can return a retryable wait error even when older notes could spend immediately.
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:301-302: Map expected witness absence to the trait's `Ok(None)` contract
`ShieldedStore::witness_at_depth` documents `Ok(None)` for expected absence cases, including a position appended after the requested checkpoint. The file-backed implementation forwards `ClientPersistentCommitmentTree::witness` errors wholesale, but shardtree returns `QueryError::NotContained` for that normal newer-than-checkpoint case before the grovedb wrapper converts it into `CommitmentTreeError::InvalidData`. The current caller logs and treats the error as an unusable depth, so this is not fund-safety or consensus-critical, but the new trait method's implementation does not match its contract and future callers can easily misclassify normal control flow as store corruption.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1702-1717: Value-first selection can mask spendable older notes
`reserve_unspent_notes` selects and reserves notes before the recorded-anchor depth is known, and `select_notes` sorts by value only. If the largest selected note is newer than every Platform-recorded checkpoint, the depth walk breaks and returns `ShieldedNoRecordedAnchor`; older notes that were not selected may still cover the amount plus fee at a recorded checkpoint. This is fund-safe and now documented/tested as a retryable availability tradeoff, but it can tell the user to wait when an immediately spendable subset exists. Fixing it likely requires selecting after discovering an eligible checkpoint, or retrying selection over notes witnessable at that checkpoint before returning the retryable error.
| tree.witness(Position::from(position), depth) | ||
| .map_err(|e| FileShieldedStoreError(format!("witness({position}, depth {depth}): {e}"))) |
There was a problem hiding this comment.
🟡 Suggestion: Map expected witness absence to the trait's Ok(None) contract
ShieldedStore::witness_at_depth documents Ok(None) for expected absence cases, including a position appended after the requested checkpoint. The file-backed implementation forwards ClientPersistentCommitmentTree::witness errors wholesale, but shardtree returns QueryError::NotContained for that normal newer-than-checkpoint case before the grovedb wrapper converts it into CommitmentTreeError::InvalidData. The current caller logs and treats the error as an unusable depth, so this is not fund-safety or consensus-critical, but the new trait method's implementation does not match its contract and future callers can easily misclassify normal control flow as store corruption.
source: ['codex-rust-quality']
There was a problem hiding this comment.
Still present at 6a356ca.
| 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(), |
There was a problem hiding this comment.
🟡 Suggestion: Value-first selection can mask spendable older notes
reserve_unspent_notes selects and reserves notes before the recorded-anchor depth is known, and select_notes sorts by value only. If the largest selected note is newer than every Platform-recorded checkpoint, the depth walk breaks and returns ShieldedNoRecordedAnchor; older notes that were not selected may still cover the amount plus fee at a recorded checkpoint. This is fund-safe and now documented/tested as a retryable availability tradeoff, but it can tell the user to wait when an immediately spendable subset exists. Fixing it likely requires selecting after discovering an eligible checkpoint, or retrying selection over notes witnessable at that checkpoint before returning the retryable error.
source: ['codex-rust-quality']
There was a problem hiding this comment.
Still present at 6a356ca.
…corded 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 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
The two working documents were removed from the tree in 6a356ca ( SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.mdSpec — shielded spend uses a Platform-recorded anchorFixes the reproduced root cause of TestFlight report B (see Reproduced by two passing tests (committed Goal / non-goal
Chosen approach: build against the most-recent recorded anchorInstead of Data flow, per spend (
Note requirement (B1): value-based selection alone would pick a note newer than Why this approach
Alternatives rejected
Interface / data-flow changes
Failure modes / risks
Test plan
Rollout / scope
TESTFLIGHT_FEEDBACK_INVESTIGATION.mdShielded-pool TestFlight feedback — investigationTracking two TestFlight feedback reports against build 3 ( ReportsReport B — shielded→Core withdrawal never confirms ("may have gone through") — PRIMARY
Report A — switching wallets during a shielded sync — THIN
Mechanism traced (Report B)The dialog is not an ad-hoc string — it is the deliberate handling of
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)
The ambiguous bucket is deliberately wide — both a broadcast that probably failed (ambiguous transport error) and a wait that timed out land in Funds safety (confirmed — no permanent loss)
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 reservationThe reservation set
There is no reconcile path that releases a reservation whose tx demonstrably never landed. For the Severity
Open questions / hypotheses (Report B reliability)
Investigation / reproduction planCode trace (done):
Reproduction (needs a shielded environment + funds):
Two fix tracks (independent):
Root cause (B) — code investigation findings (testnet)Wait has no client timeout. Leading hypothesis: anchor mismatch (
The wallet computes the withdrawal anchor locally ( Why it may not surface as a clear failure: if the rejection is delivered as a Code-dive conclusion (high confidence)The anchor the wallet submits is not guaranteed to be a Platform-recorded anchor, by construction:
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 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 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 datapointA single testnet 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:
Chain: (1) the wallet submits a mid-block anchor drive never recorded; (2) drive rejects exactly that with UX follow-ups (independent of the root cause)
|
|
Reviewed |
…ion on sync
A shielded spend that returns ShieldedSpendUnconfirmed (broadcast accepted, but
the result-wait failed ambiguously) keeps the spent notes' reservation
(pending_nullifiers) so a retry can't double-spend. That reservation was
released only when the tx lands (a later sync marks the note spent) or on app
restart. A broadcast-accepted-but-never-lands spend therefore stranded its
notes for the whole session — funds looked stuck with no in-session resolution.
Fix: remember the Platform-recorded anchor each spend was built against (the
anchor-selection fix guarantees it's a recorded root), then on each sync —
after the normal spent-note reconcile — release any still-pending reservation
whose anchor is no longer in Platform's recorded set. Once the anchor is pruned
(shielded_anchor_retention_blocks = 1000) the transition can never execute, so
with the nullifier still unspent (which "still pending after the reconcile"
already implies) the spend is provably dead and its notes can be freed; the
linked activity row flips Pending -> Failed so the UI shows a clear, retryable
failure. Reuses the GetShieldedAnchors query — no protocol change.
Fund-safety: releases ONLY when the anchor is absent from the freshly-fetched
recorded set (never a still-recorded, slow-but-landing spend); the on-chain
nullifier set stays the authoritative double-spend guard. The spent-note
reconcile runs first, so a landed tx's reservation is already cleared before
the release pass — no race. pending_nullifiers stays in-memory (a restart still
frees everything); the anchor fetch is best-effort (a hiccup never fails sync).
Data model: SubwalletState.pending_nullifiers BTreeSet -> BTreeMap<[u8;32],
PendingSpend { anchor, activity_id }>; arm_pending_release fills it in across
all four spend flows before broadcast. Wallet suite 312 tests pass; fmt +
clippy (--all-features) clean.
Stacks on the shielded anchor-selection fix (PR #3977) — the stored anchor must
be a recorded one, which that PR guarantees.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Same treatment as #3977's docs: docs/ holds durable architecture references; the spec is archived in the PR discussion and the shipped mechanism is documented on prefetch_stranded_release / release_stranded_spends. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta 684bf84..6a356ca is docs-only (deletion of two working-notes markdown files under docs/shielded/), so no new findings arise. All three prior findings remain valid at head 6a356ca and are carried forward: witness_at_depth's Err/Ok(None) trait-contract divergence in file_store.rs, value-first note selection masking a spendable older-anchor subset in operations.rs, and a third near-duplicate real-proof bundle-building block in the drive-abci shielded_withdrawal tests. All are suggestion/nitpick severity with no fund-safety or consensus impact.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (security-auditor), claude-sonnet-5 (security-auditor), gpt-5.5[high] (security-auditor, failed), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed); verifier: opus; specialists: security-auditor, rust-quality, ffi-engineer
🟡 2 suggestion(s) | 💬 1 nitpick(s)
Carried-forward findings already raised (2)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [SUGGESTION] (deduped existing open thread)
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:286-303: witness_at_depth forwards expected absence as Err instead of the trait's Ok(None) contract — Carried forward from prior-1, verified STILL VALID at 6a356ca (file_store.rs unchanged in this delta).ShieldedStore::witness_at_depth(store.rs) documentsOk(None)for expected-absence cases — including 'the position was appended after that checkpoint,' which is the routine mid-block-sync s... - [SUGGESTION] (deduped existing open thread)
packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1700-1718: Value-first note selection can mask a spendable older-anchor subset with ShieldedNoRecordedAnchor — Carried forward from prior-2, verified STILL VALID at 6a356ca (operations.rs unchanged in this delta).reserve_unspent_notes(operations.rs:1767-1787) selects and reserves notes viaselect_notes_with_feeunder a single write lock BEFORE the recorded-anchor depth is known, andselect_notes...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:286-303: witness_at_depth forwards expected absence as Err instead of the trait's Ok(None) contract
Carried forward from prior-1, verified STILL VALID at 6a356ca1 (file_store.rs unchanged in this delta). `ShieldedStore::witness_at_depth` (store.rs) documents `Ok(None)` for expected-absence cases — including 'the position was appended after that checkpoint,' which is the routine mid-block-sync scenario this PR is built around. The file-backed impl at lines 301-302 still forwards every `ClientPersistentCommitmentTree::witness` error wholesale via `FileShieldedStoreError`. The shardtree layer returns `QueryError::NotContained` for the normal newer-than-checkpoint case (surfaced by the grovedb wrapper as `CommitmentTreeError::InvalidData`), so this common expected condition reaches the caller as `Err`, not the documented `Ok(None)`. The current caller in `select_recorded_spends` tolerates this by treating any depth>0 error as an unusable depth and continuing to probe, so fund-safety and consensus are unaffected today. But the trait doc and impl diverge: a future caller that trusts the doc (Err = store corruption, Ok(None) = expected absence) could misclassify normal mid-probe control flow as a hard store-corruption failure, either surfacing spurious errors on legitimate spend attempts or (if inverted) treating real corruption as benign. Fix at the impl boundary by matching on the underlying shardtree/grovedb error variant and mapping the NotContained/appended-after-checkpoint case to `Ok(None)` while keeping real I/O/lock failures on the Err channel.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1700-1718: Value-first note selection can mask a spendable older-anchor subset with ShieldedNoRecordedAnchor
Carried forward from prior-2, verified STILL VALID at 6a356ca1 (operations.rs unchanged in this delta). `reserve_unspent_notes` (operations.rs:1767-1787) selects and reserves notes via `select_notes_with_fee` under a single write lock BEFORE the recorded-anchor depth is known, and `select_notes` sorts strictly by value descending with no checkpoint awareness. Downstream, `select_recorded_spends` builds against depth 0 and then walks `1..MAX_ANCHOR_PROBE_DEPTH`; the first time `build_at_depth` returns `None` for a selected note (a note newer than the probed checkpoint) the loop breaks at line 1712 and the whole spend returns `PlatformWalletError::ShieldedNoRecordedAnchor`. That reasoning is correct for the *selected* set of notes, but says nothing about *unselected* smaller-value notes that may be witnessable at, and cover amount+fee against, a checkpoint Platform did record. If the largest selected notes are all newer than every recorded checkpoint, older un-selected notes are never tried, so the user is told to wait for the next sync even though an immediately spendable subset exists. Fund-safe (reservations released via `cancel_pending`, nullifier is anchor-independent so no double-spend risk) and now covered by a retryable-availability test, but the UX cost is a spurious retry loop. Fix by either discovering the eligible checkpoint depth first and selecting only from notes with `position <= tree_size_at_depth`, or retrying selection over depth-eligible notes before returning the retryable error.
| 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::<DashMemo>::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::<i64>(&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() { |
There was a problem hiding this comment.
💬 Nitpick: Third near-duplicate copy of the real-proof bundle-building boilerplate
Carried forward from prior-3, verified STILL VALID at 6a356ca (tests.rs unchanged in this delta). test_valid_proof_with_older_recorded_anchor_succeeds (line 626) repeats, almost verbatim, the ~40-line real-proof construction already present in test_valid_shielded_withdrawal_proof_succeeds (line 409) and test_valid_proof_with_unrecorded_anchor_returns_invalid_anchor_error (line 529): spending key/fvk/recipient/ask setup, note with fixed rho, memory commitment tree, build+prove+sign the Orchard bundle, compute the platform sighash, serialize the authorized bundle. This file already has a Helper Functions (transition-specific) section (line 23) for exactly this kind of shared scaffolding. With three copies in the same file, a future change to the bundle-building sequence (new required field, changed sighash input) is likely to be updated in two of the three copies and silently drift the third out of sync with what production code actually does. Consider extracting a build_real_spend_bundle(tree, checkpoint_depth, notes_after_checkpoint) -> (bundle, anchor, ...) helper into that section.
source: ['claude']
3 participants
Issue being fixed or feature implemented
A TestFlight report (build 3) described a shielded→Core withdrawal that repeatedly fails: the app shows "Transaction may have gone through — waiting for the next shielded sync to confirm. Do not retry," but the balance never moves (funds untouched), across multiple days. A second report ("switching wallets after starting a shielded sync") is the same root cause from the other side (an interrupted sync).
Root cause (reproduced deterministically): shielded spends build the Orchard proof against the wallet's depth-0 (current) commitment-tree root. The note-commitment sync is chunked by index (
CHUNK_SIZE = 2048), not by block, so the wallet's tree routinely sits mid-block; drive records anchors only at block boundaries (one per block, 1000 retained). So the depth-0 root is frequently a value Platform never recorded →validate_anchor_existsrejects the transition withInvalidAnchorError→ the spend never lands, and the ambiguous rejection is misreported asShieldedSpendUnconfirmed("may have gone through").Two tests committed here reproduce it end-to-end without a testnet: the wallet's mid-block depth-0 anchor is provably not a recorded anchor (
platform-wallet), and a real-proof withdrawal with an unrecorded anchor is rejected withInvalidAnchorError(drive-abci).What was done?
extract_spends_and_anchornow fetches Platform's recorded anchor set (GetShieldedAnchors) and builds against the shallowest checkpoint depth whose root Platform recorded, via a pure, unit-testableselect_recorded_spendsprobe:1..100, taking the first recorded root, stopping once a selected note post-dates the checkpoint (deeper is older still);PlatformWalletError::ShieldedNoRecordedAnchorbefore broadcasting.Supporting changes:
ShieldedStoregainswitness_at_depth(witnessbecomes a depth-0 default — non-breaking); a new FFI codeErrorShieldedNoRecordedAnchor = 19maps to a "still syncing — try again shortly" message, distinct fromShieldedSpendUnconfirmed's "do not retry."Fund-safety (please review): 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(nk, ρ, ψ, cm)-derived and anchor-independent, so spending against an older recorded anchor does not weaken the on-chain double-spend guard.ShieldedNoRecordedAnchoris returned pre-broadcast and routes to each of the four spend paths' reservation-releasing arm (cancel_pending) — notes are freed, nothing is broadcast.The design was written up (spec + root-cause investigation archived in this PR comment) and audited by three independent reviewers (soundness / feasibility / scope) before implementation.
How Has This Been Tested?
select_recorded_spends, realFileBackedShieldedStore+ a real Orchard note): mid-block → selects the prior recorded (block-2 / depth-1) anchor, not depth-0; fully-synced → depth-0; no recorded →ShieldedNoRecordedAnchor.cargo test -p platform-wallet --lib --features shielded→ 304 passed; FFI suite 107 passed;fmt --check+clippy --features shieldedclean.Not covered (network-gated): live testnet shielded→Core withdrawal succeeding after the fix — best confirmed on a device once the Swift code-19 case is wired.
Known follow-ups (out of scope, documented in the archived spec)
ErrorShieldedNoRecordedAnchor = 19case + retryable UI (until then Swift sees it as an unknown code).ShieldedSpendUnconfirmedclassification / no-restart reservation release ("A").Breaking Changes
None externally.
ShieldedStore::witness_at_depthis a new required trait method (both in-tree impls updated;witnesskeeps working via a default);extract_spends_and_anchoris private.Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit