fix(platform-wallet): build shielded spends against a Platform-recorded anchor#3977
fix(platform-wallet): build shielded spends against a Platform-recorded anchor#3977shumkov wants to merge 2 commits into
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>
shumkov
requested review from
QuantumExplorer,
ZocoLini,
lklimek and
llbartekll
as code owners
|
✅ Review complete (commit 1d1d21b) |
📝 WalkthroughWalkthroughIntroduces Platform-recorded anchor selection for shielded spend proof construction: adds a depth-parameterized witness API to shielded stores, probes checkpoint depths to find recorded anchors, adds a new 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.
| 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), | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: 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)).
source: ['claude', 'codex']
| 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, |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['codex']
| 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(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Empty-notes guard runs after the SDK round-trip
extract_spends_and_anchor issues the ShieldedAnchors::fetch_current network round-trip before checking notes.is_empty(). All callers currently filter, so this is defensive, but if the empty case ever triggers the wallet has paid for a proof-verified query and thrown the result away. Move the is_empty check above the fetch_current call — mechanical, no cost, and makes the "notes non-empty ⇒ anchor set" invariant hold before any I/O.
source: ['claude']
| let build_at_depth = |depth: usize, | ||
| strict: bool| | ||
| -> Result<Option<(Vec<SpendableNote>, Anchor)>, PlatformWalletError> { | ||
| let mut spends = Vec::with_capacity(notes.len()); | ||
| let mut anchor: Option<Anchor> = 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))) | ||
| }; |
There was a problem hiding this comment.
💬 Nitpick: Depth walk re-deserializes notes and re-decodes cmx at every probed depth
build_at_depth is invoked up to ~100 times, and each invocation re-runs deserialize_note(¬e.note_data) and ExtractedNoteCommitment::from_bytes(¬e.cmx) per selected note. Only witness_at_depth varies by depth. Not a hot path (single-user withdrawal) and not a correctness issue, but hoisting the per-note deserialization + cmx extraction to a one-shot precompute outside the closure cuts the worst-case redundant work by ~100× and simplifies the anchor-agreement check to a straight MerklePath::root(cmx) comparison.
source: ['claude']
| /// 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(); |
There was a problem hiding this comment.
💬 Nitpick: No test exercises the Ok(None) → break early-termination arm of the depth walk
select_recorded_spends_tests covers the three headline outcomes, but every case uses a single note at position 0, which is witnessable at every retained checkpoint. The None => break arm at 1687 is never actually reached — no_recorded_checkpoint_returns_retryable_error exhausts the loop through the Some(_) => continue branch. Adding a case that places a selected note at a position appended after the shallowest recorded checkpoint would pin the fast-fail behaviour so a future refactor can't silently make the walk run all 99 iterations without failing a test.
source: ['claude']
|
|
||
| /// Extract `SpendableNote` structs with Merkle witnesses and the | ||
| /// tree anchor. | ||
| /// Checkpoint depths probed for a Platform-recorded anchor. Matches the | ||
| /// commitment tree's `max_checkpoints` retention so the probe can reach every | ||
| /// checkpoint the tree still holds before giving up. | ||
| const MAX_ANCHOR_PROBE_DEPTH: usize = 100; |
There was a problem hiding this comment.
💬 Nitpick: MAX_ANCHOR_PROBE_DEPTH couples the probe to the store's max_checkpoints only by convention
The constant is hardcoded to 100 and the doc comment says it matches max_checkpoints. Every current call site to FileBackedShieldedStore::open_path(..., 100) does pass 100, so this is coincidentally correct. But max_checkpoints is a runtime store parameter — a future call site opening the store with different retention would silently under- or over-probe. Also the loop 1..MAX_ANCHOR_PROBE_DEPTH is exclusive (depths 1..=99), so if max_checkpoints = 100 means 100 recorded checkpoints beyond the tip, the deepest one is never probed. Either widen the loop to 1..=MAX_ANCHOR_PROBE_DEPTH and clarify the doc, or expose max_checkpoints on the store trait and derive the bound.
source: ['claude']
|
@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
| ## Interface / data-flow changes | ||
| - `ShieldedStore` trait (breaking; implemented by `FileBackedShieldedStore`, | ||
| `InMemoryShieldedStore`, and any host/FFI persister): | ||
| - `witness_at_depth(&self, position, depth) -> Result<Option<MerklePath>, Err>` | ||
| (generalize today's depth-0-only `witness`). | ||
| - `anchor_at_depth(&self, depth) -> Result<Option<[u8;32]>, 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<Option<u32>, 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.) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n '\b(anchor_at_depth|checkpoint_id_at_depth|witness_at_depth)\b' packages/rs-platform-walletRepository: dashpay/platform
Length of output: 792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== store.rs outline ==\n'
ast-grep outline packages/rs-platform-wallet/src/wallet/shielded/store.rs --view expanded || true
printf '\n== operations.rs outline ==\n'
ast-grep outline packages/rs-platform-wallet/src/wallet/shielded/operations.rs --view expanded || true
printf '\n== spec excerpt ==\n'
sed -n '80,120p' docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md | cat -n
printf '\n== store.rs relevant lines ==\n'
sed -n '240,320p' packages/rs-platform-wallet/src/wallet/shielded/store.rs | cat -n
printf '\n== operations.rs relevant lines ==\n'
sed -n '1570,1635p' packages/rs-platform-wallet/src/wallet/shielded/operations.rs | cat -nRepository: dashpay/platform
Length of output: 21144
Keep the store API wording aligned with the trait
This section names anchor_at_depth and checkpoint_id_at_depth, but ShieldedStore only exposes witness_at_depth plus tree_anchor/tree_size. Either add the missing trait methods or rewrite this to describe the helper logic instead of new store APIs.
🤖 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 `@docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md` around lines 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.
| /// 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, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all FFI result-code mirrors include the new no-recorded-anchor code.
rg -n -C3 'ErrorShieldedSpendUnconfirmed|ErrorShieldedNoRecordedAnchor|ShieldedNoRecordedAnchor' .Repository: dashpay/platform
Length of output: 19917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files mentioning PlatformWalletFFIResultCode\n'
rg -n --hidden --glob '!**/target/**' 'PlatformWalletFFIResultCode' .
printf '\n## possible language mirrors for error/result codes\n'
rg -n --hidden --glob '!**/target/**' 'ErrorShieldedNoRecordedAnchor|ErrorShieldedSpendUnconfirmed|ShieldedNoRecordedAnchor|ShieldedSpendUnconfirmed' packages docs .Repository: dashpay/platform
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Swift mirror\n'
sed -n '1,140p' packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
printf '\n## likely header / Kotlin mirrors\n'
rg -n --hidden --glob '!**/target/**' 'PlatformWalletFFIResultCode|errorShieldedNoRecordedAnchor|ErrorShieldedNoRecordedAnchor|ShieldedNoRecordedAnchor' packages -g '!**/*.md'Repository: dashpay/platform
Length of output: 50372
Add errorShieldedNoRecordedAnchor = 19 to the Swift mirror
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift still stops at errorShieldedSpendUnconfirmed = 18, so the new Rust code will fall through to .errorUnknown. Add the new case and init(ffi:) branch so Swift can distinguish retryable no-anchor from spend-unconfirmed.
🤖 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-platform-wallet-ffi/src/error.rs` around lines 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.
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.18%
=========================================
Files 2632 2632
Lines 327563 327563
=========================================
+ Hits 285592 285593 +1
+ 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
2 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 (
docs/shielded/SHIELDED_WITHDRAWAL_ANCHOR_FIX_SPEC.md, alongside the root-cause investigation) 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 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
New Features
Bug Fixes
Documentation