Repository navigation
feat(platform-wallet): own the shielded-tip payment decisions in the wallet - #5709
PastaPastaPasta wants to merge 21 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. |
|
🕓 Review not started yet because this PR is a draft.
Commit 158ae6a. Normal review starts when eligible; priority review starts as soon as a slot is available. |
…wallet The example apps decided when a tip was still in flight, whether a failed tip could be sent again, whether a username's recipient had changed, and whether an identity's tip balance could be shown. Each kept that state in memory or in host preferences, so a relaunch forgot an unsettled tip and the user could pay twice. The wallet now makes each of these decisions. - A tip is recorded as unresolved before broadcast: the activity row is flushed and a parked recovery record is armed in the shielded store, so it survives a relaunch. A tip to an address with an unsettled payment (or to the address last confirmed for the name) is refused, as is a second tip while one is being sent. The send's own outcome, a scan, or the anchor prune settles the record. - ShieldedTipDisposition (NotSent / Unresolved) is the retry decision, passed through the C send as out_disposition and thrown as ShieldedTipFailure in Swift and Kotlin. - review_shielded_tip returns the recipient, how it compares with the one last confirmed for the name (kept per wallet in the shielded store), and whether a payment to it is unresolved. - identity_tip_account reports whether this session proved the identity's slot (discovery, load, or a seed check); a restored index is unverified. Closes #5674. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Swift and Kotlin tip screens each parsed the typed DASH amount, with different rules: Kotlin's BigDecimal accepted exponents and signs that Swift refused. parse_shielded_tip_amount turns ASCII digits with an optional decimal point into exact credits and refuses signs, grouping, exponents, zero, and fractions finer than one credit; C, JNI, Swift and Kotlin expose it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…p failures as not sent
- parse_shielded_tip_amount trims whitespace and takes one `.` or `,`
separator, since iOS decimal keypads type the locale's; a missing whole
part (".5") is zero. JNI refuses a parsed amount that does not fit a
signed 64-bit value instead of wrapping it negative.
- The Swift and Kotlin sends throw ShieldedTipFailure(notSent) for a failure
raised before the native call (invalid arguments, manager shutdown), so
hosts read only the disposition instead of knowing where the SDK throws.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Int32 cbindgen emits them as macros, which Swift imports as Int32. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
12b4bb7 to
c3ec759
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of c3ec759 confirms that the payment decisions largely belong to the Rust wallet, but four blocking gaps remain in broadcast classification, recovery-record lifetime, settlement durability, and handling of unreadable recovery bytes. These gaps can remove or bypass the retry barrier before the host durably knows the original payment's outcome. Kotlin build/tests have passed; Rust workspace tests remain in progress, and no local builds or tests were run.
🔴 4 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes to send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/operations.rs and the persistent recovery guards directly alter funds-movement safety, including crash recovery, retries, and prevention of duplicate payments. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3138-3143: Preserve the tip guard after potentially delivered broadcast failures
The guarded tip uses broadcast_shielded_spend, whose broadcast_definitely_failed helper treats gRPC Unavailable as definitive. That helper explicitly documents that Unavailable can follow delivery or hide an earlier delivered retry. If a relay forwards the transfer but its acknowledgement is lost, this becomes ShieldedBroadcastFailed; transfer_with then calls cancel_pending, deleting the durable guard, and ShieldedTipDisposition::of reports NotSent. Retrying with other funded notes can pay the recipient twice: on-chain nullifiers protect the original inputs, not the payment intent. The new tip-level retry guarantee cannot use this note-release heuristic. Retain the guard and report Unresolved for potentially delivered transport failures, and test a delivered request followed by Unavailable.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3114-3123: Protect unresolved tip guards from account purge and Clear
The payment barrier is an ordinary nullifier-backed PendingRedrive, but existing lifecycle operations can delete those records without resolving the payment. Send an ambiguous tip from ordinary account 0, then rebind from accounts [0, 1] to [1]: register_locked protects only empty-nullifier identity-debit guards, and FileBackedShieldedStore::purge_subwallet deletes this tip's row. A tip from account 1 now passes the unresolved check even though the original transfer may still execute. Clear likewise deletes every recovery row. Preserving recipient confirmations across Clear does not preserve this payment barrier, and asking callers to keep the paying account bound delegates the safety decision back to the host. Retain unresolved tip guards independently of rebuildable account state, or reject destructive lifecycle operations until settlement; serialize that protection with guard creation. Add ambiguous-tip → rebind/Clear → send-from-another-account regression coverage.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3103-3107: Propagate the guarded activity write failure before flushing
A successful flush does not prove that the Pending activity was successfully queued. record_pending_activity returns Some(entry) even when queue_shielded_changeset logs and discards persister.store's error. FFIPersister can roll back its store round and then return success from the independent flush callback, so this new durability gate still broadcasts after a known activity-write failure. On restart the signed guard survives without its recipient row, conservatively blocking every recipient until reconciliation. Keep that fail-closed fallback for unexpected loss, but propagate known write failures before broadcasting through a fallible guarded activity-recording path. Test a persister whose store fails while flush succeeds.
In `packages/rs-platform-wallet/src/wallet/shielded/tips.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:117-119: Persist settlement before removing the durable tip guard
An empty redrive set does not establish that the payment's outcome was durably recorded. During reconciliation, apply_scanned_nullifier_spends calls mark_spent, and FileBackedShieldedStore::mark_spent immediately deletes the recovery row from SQLite. The spent-note and Confirmed activity updates reach the host later through the coordinator's changeset fan-out. A process exit between those steps leaves the host's previously flushed Pending activity but no recovery row after restart; this check returns false, permitting another payment from a different funded account even though the original payment executed. The successful-send path also removes the guard without requiring durable settlement persistence. Retain the barrier until settlement is durably recorded, or write a durable settlement marker before deleting it. Add a reopen test interrupted after recovery-row deletion but before host persistence.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:121-125: Fail closed when an unresolved transition cannot be decoded
This condition treats a deserialization failure as equivalent to a successfully decoded non-transfer. FileBackedShieldedStore::rehydrate_pending_spends accepts a nullifier-backed row with valid routing fields, widths and flags even when st_bytes cannot decode. Consequently, damage limited to an interrupted tip's signed bytes leaves its reservation restored but makes review/send ignore its payment barrier; other notes can fund a replacement while the original payment's outcome remains unknown. A parked record also has its attempt budget exhausted, so the redrive loop skips decoding it. Skip only successfully classified non-transfer transitions; return ShieldedRecoveryCorrupted or conservatively block on unreadable bytes. Replace the vec![7; 16] 'unshield-like' fixture with a serialized non-transfer, and test malformed bytes separately.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:556-559: No test proves a second concurrent tip is refused instead of queued
The new try_lock is the wallet-wide mechanism that prevents an overlapping repeat from waiting behind the first send and paying again after it succeeds. The added tests cover persisted guards and disposition mapping, but none exercises contention on this mutex or its sharing across PlatformWallet clones. Add a deterministic test holding the send gate on one handle and invoking send_shielded_tip through a clone; assert ShieldedTipPending/NotSent before recipient resolution or broadcast. This can be tested locally without a funded network.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:1795: Reject embedded NUL before converting an amount to a C string
Swift strings can contain U+0000, but the C export reads this argument with CStr::from_ptr. parseShieldedTipAmount("1\u{0000}e3") therefore parses only "1" and returns 100,000,000,000 credits, although the canonical Rust parser rejects the complete string. The JNI path already rejects embedded NUL through CString::new. Reject an unrepresentable string before this null-terminated ABI conversion, or make the export length-aware, so the Swift binding preserves the parser's input.
In `packages/rs-platform-wallet/src/wallet/shielded/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/store.rs:502-514: Document the public ShieldedStore source-compatibility break
confirmed_tip_recipient and confirm_tip_recipient are new required methods on the existing public ShieldedStore trait, whose documentation explicitly supports consumer-provided implementations. Those implementations stop compiling until both methods are added. The breaking-change description covers only the unreleased send bindings, and the existing Rust API migration notes do not mention these methods. Document the custom-backend migration, including wallet scoping, retention across shielded Clear, and deletion with purge_wallet. This is a Rust API compatibility change, not a PlatformVersion or GroveVersion change.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [NITPICK] packages/rs-platform-wallet-ffi/src/shielded_send.rs:2600-2602: New review export uses unwrap() on an infallible conversion in unsafe FFI
This conversion cannot fail because the slice is constructed with exactly 32 bytes, so it is not a reachable panic defect. However, the review conventions prohibit production unwrap and permit expect when its message states the established invariant. Express that invariant here rather than adding another unexplained unwrap to an unsafe export.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Preserve authoritative payment outcomes across coroutine cancellation — TeardownGate.withOp uses withContext(Dispatchers.IO), whose return handoff can discard a completed blocking JNI result when the caller is cancelled. Kotlin can therefore receive CancellationException after native payment completion instead of receiving the completed outcome. The base revision already routes sendShieldedTip through this same gate, so this is a concrete pre-existing payment-wrapper issue rather than a new defect in the added disposition mapping.
- Follow-up: Track cancellation-safe payment-result handoff separately, covering cancellation before JNI entry, during execution, and after native completion.
- Swift initial disposition disagrees with Kotlin/JNI pre-native default — The seed differs, but no reachable pre-native throwing operation inside this catch exposes it. Argument validation and admitNativeOp failures are already mapped to notSent outside the detached task, and the C export writes NOT_SENT immediately on entry before any checked failure. The proposed cancellation example does not apply to this detached closure.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| let result = broadcast_shielded_spend( | ||
| sdk, | ||
| state_transition, | ||
| "transfer", | ||
| SpendResultWait::ExecutionProved, | ||
| ) |
There was a problem hiding this comment.
✅ Resolved at
688c75e1; see the replies below.
🔴 Blocking: Preserve the tip guard after potentially delivered broadcast failures
The guarded tip uses broadcast_shielded_spend, whose broadcast_definitely_failed helper treats gRPC Unavailable as definitive. That helper explicitly documents that Unavailable can follow delivery or hide an earlier delivered retry. If a relay forwards the transfer but its acknowledgement is lost, this becomes ShieldedBroadcastFailed; transfer_with then calls cancel_pending, deleting the durable guard, and ShieldedTipDisposition::of reports NotSent. Retrying with other funded notes can pay the recipient twice: on-chain nullifiers protect the original inputs, not the payment intent. The new tip-level retry guarantee cannot use this note-release heuristic. Retain the guard and report Unresolved for potentially delivered transport failures, and test a delivered request followed by Unavailable.
source: gpt-6.1-sol (phase2-reviewer: security-auditor)
There was a problem hiding this comment.
Fixed in 688c75e. The tip is now broadcast once, with no retries (PutSettings retries: Some(0)). tip_broadcast_definitely_failed (operations.rs) counts as definitive only a consensus rejection from that single node (not NullifierAlreadySpent), or NoAvailableAddresses, where nothing was sent.
- Transport errors stay unresolved.
Unavailable,DeadlineExceeded, and a broadcast error with no verdict keep the guard. The send reportsShieldedSpendUnconfirmed, so the disposition isUnresolved. - Why no retries. A retry could otherwise land on a node that is a block behind, whose
InvalidAnchorrejection would hide a copy another node admitted. - Settlement. The barrier settles as failed only in
transfer_with's definitive-error arm. - Test.
tip_broadcast_is_definitive_only_on_a_consensus_rejectioncoversUnavailable,DeadlineExceeded, a broadcast error with no verdict, andNullifierAlreadySpent, and checks that the ordinary spend rule still releases notes onUnavailable.
This test is a unit test of the classifier. The delivered-then-Unavailable sequence itself needs a live node.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 688c75e1): Your zero-retry broadcast and tip-specific classifier now preserve ambiguity for Unavailable, DeadlineExceeded, cause-less errors, and NullifierAlreadySpent, fixing the reported transport-failure release. The new rejection-authentication finding addresses an endpoint fabricating a verdict, which is distinct from that transport-error sequence.
| .arm_redrive( | ||
| id, | ||
| PendingRedrive { | ||
| activity_id: entry.id, | ||
| anchor, | ||
| nullifiers: notes.iter().map(|n| n.nullifier).collect(), | ||
| st_bytes, | ||
| attempts: MAX_REDRIVE_ATTEMPTS, | ||
| identity_nonce_finalized: false, | ||
| identity_user_abandoned: false, |
There was a problem hiding this comment.
✅ Resolved at
0dfe1513; see the replies below.
🔴 Blocking: Protect unresolved tip guards from account purge and Clear
The payment barrier is an ordinary nullifier-backed PendingRedrive, but existing lifecycle operations can delete those records without resolving the payment. Send an ambiguous tip from ordinary account 0, then rebind from accounts [0, 1] to [1]: register_locked protects only empty-nullifier identity-debit guards, and FileBackedShieldedStore::purge_subwallet deletes this tip's row. A tip from account 1 now passes the unresolved check even though the original transfer may still execute. Clear likewise deletes every recovery row. Preserving recipient confirmations across Clear does not preserve this payment barrier, and asking callers to keep the paying account bound delegates the safety decision back to the host. Retain unresolved tip guards independently of rebuildable account state, or reject destructive lifecycle operations until settlement; serialize that protection with guard creation. Add ambiguous-tip → rebind/Clear → send-from-another-account regression coverage.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 688c75e. The payment barrier is no longer a recovery record. Each tip records a TipBarrier (recipient, nullifiers, anchor, status) in a new shielded_tip_barriers table, separate from note and recovery state.
- Purges.
purge_subwalletandpurge_all_subwalletsleave it in place; onlypurge_walletremoves it. - Rebinds.
register_lockedrefuses a registration that drops or re-keys the paying account of a pending tip (ShieldedRecoveryKeysRequired), including on the first registration after a restart. - Clear.
clear()refuses withShieldedTipPendingwhile any tip is pending. - Sends. The send holds
shielded_config_lockfrom source selection through the spend, so a bind can't drop the account mid-send. - Stuck tips. A tip that never settles can be released only by
abandon_unresolved_shielded_tips, with the user's acknowledgement that it may still execute. This mirrorsabandon_identity_debit. - Tests.
should_keep_a_pending_tips_account_bound_and_refuse_clear_until_it_settleschecks that a bind dropping the account and a Clear are refused, then pass once the user releases the tip.tip_barriers_settle_durably_and_outlive_account_purgeschecks that a barrier survives purges and a reopen.
One known gap, which can't pay twice: a Clear can still run between note selection and the barrier being written, because Clear doesn't take shielded_config_lock. If that tip then never lands, it waits for the user's release.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 688c75e1): Your independent barrier survives account purges and Clear, registration pins pending tips' paying accounts, and sends hold the configuration lock through guard creation. These changes close the reported deletion-based bypass; the new refused-Clear finding instead concerns stopping recovery before returning the refusal.
There was a problem hiding this comment.
The remaining part, a Clear landing before the barrier exists, is fixed in 84cfd41: Clear and account-dropping binds are refused from tip-send admission onward. Details in #5709 (comment).
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 0dfe1513): Your independent barriers survive account purges, registration pins Pending tips' paying accounts, and Clear refuses them. The newer admission count also closes the pre-barrier interval identified in the follow-up discussion.
| let redrives = store | ||
| .pending_redrives_for_wallet(wallet_id) | ||
| .map_err(|e| PlatformWalletError::ShieldedStoreError(e.to_string()))?; |
There was a problem hiding this comment.
✅ Resolved at
688c75e1; see the replies below.
🔴 Blocking: Persist settlement before removing the durable tip guard
An empty redrive set does not establish that the payment's outcome was durably recorded. During reconciliation, apply_scanned_nullifier_spends calls mark_spent, and FileBackedShieldedStore::mark_spent immediately deletes the recovery row from SQLite. The spent-note and Confirmed activity updates reach the host later through the coordinator's changeset fan-out. A process exit between those steps leaves the host's previously flushed Pending activity but no recovery row after restart; this check returns false, permitting another payment from a different funded account even though the original payment executed. The successful-send path also removes the guard without requiring durable settlement persistence. Retain the barrier until settlement is durably recorded, or write a durable settlement marker before deleting it. Add a reopen test interrupted after recovery-row deletion but before host persistence.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 688c75e. FileBackedShieldedStore::mark_spent now writes the barrier's Executed status to SQLite before it deletes the recovery row.
- Ordering. Both are in the same store step, so a crash between them leaves the tip recorded as executed, never forgotten. The host's activity row doesn't matter, because the barrier is the record of truth.
- Write failure. A failed status write leaves the barrier pending, which fails closed.
- Successful send.
finalize_pendingrunsmark_spent, so a successful send settles it the same way. - No settlement from
clear_pending.clear_pendingno longer settles barriers. A barrier is marked failed only on its own send's definitive rejection. - Test.
tip_barriers_settle_durably_and_outlive_account_purgeschecks that the executed status survives a reopen, that a released reservation doesn't settle a barrier, and that executed is never overwritten by failed.
There is no test that interrupts mark_spent between the two writes. The status write comes first, so the case you describe can't occur.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 688c75e1): mark_spent now writes Executed before deleting recovery rows, and failed status writes leave the barrier Pending, closing the original process-exit ordering window. The separate synchronization finding concerns loss of the initial barrier on power or OS failure rather than recovery-row deletion preceding settlement.
| if !matches!( | ||
| StateTransition::deserialize_from_bytes_trusted(&redrive.st_bytes), | ||
| Ok(StateTransition::ShieldedTransfer(_)) | ||
| ) { | ||
| continue; |
There was a problem hiding this comment.
✅ Resolved at
688c75e1; see the replies below.
🔴 Blocking: Fail closed when an unresolved transition cannot be decoded
This condition treats a deserialization failure as equivalent to a successfully decoded non-transfer. FileBackedShieldedStore::rehydrate_pending_spends accepts a nullifier-backed row with valid routing fields, widths and flags even when st_bytes cannot decode. Consequently, damage limited to an interrupted tip's signed bytes leaves its reservation restored but makes review/send ignore its payment barrier; other notes can fund a replacement while the original payment's outcome remains unknown. A parked record also has its attempt budget exhausted, so the redrive loop skips decoding it. Skip only successfully classified non-transfer transitions; return ShieldedRecoveryCorrupted or conservatively block on unreadable bytes. Replace the vec![7; 16] 'unshield-like' fixture with a serialized non-transfer, and test malformed bytes separately.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Fixed in 688c75e. The tip check no longer decodes transition bytes. It reads the barrier rows, which hold the recipient and nullifiers directly.
- Damaged rows. A row that can't be read marks its wallet damaged, or every wallet if its wallet id is unreadable. For that wallet,
tip_barriersfails, so review and send fail withShieldedRecoveryCorrupted. A Clear is refused while any wallet has a damaged row. - Repair. The acknowledged
abandon_unresolved_shielded_tipsclears the damage. - Test.
tip_barriers_settle_durably_and_outlive_account_purgescorrupts one row, checks that only its wallet is blocked, and checks that releasing the wallet's tips repairs it.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 688c75e1): Your unresolved-tip check now reads independent barrier records rather than decoding recovery-transition bytes, and unreadable barriers make the affected wallet's reads fail closed. The separately reported repair defects concern acknowledgement durability and scope, not the original decoding bypass.
| /// ambiguous input (signs, grouping, exponents, sub-credit fractions). | ||
| public static func parseShieldedTipAmount(_ text: String) throws -> UInt64 { | ||
| var credits: UInt64 = 0 | ||
| try text.withCString { try platform_wallet_parse_shielded_tip_amount($0, &credits).check() } |
There was a problem hiding this comment.
✅ Resolved at
0dfe1513; see the replies below.
🟡 Suggestion: Reject embedded NUL before converting an amount to a C string
Swift strings can contain U+0000, but the C export reads this argument with CStr::from_ptr. parseShieldedTipAmount("1\u{0000}e3") therefore parses only "1" and returns 100,000,000,000 credits, although the canonical Rust parser rejects the complete string. The JNI path already rejects embedded NUL through CString::new. Reject an unrepresentable string before this null-terminated ABI conversion, or make the export length-aware, so the Swift binding preserves the parser's input.
| try text.withCString { try platform_wallet_parse_shielded_tip_amount($0, &credits).check() } | |
| guard !text.utf8.contains(0) else { | |
| throw PlatformWalletError.invalidParameter("DASH amount contains an embedded NUL") | |
| } | |
| try text.withCString { try platform_wallet_parse_shielded_tip_amount($0, &credits).check() } |
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
There was a problem hiding this comment.
Fixed in 688c75e: parseShieldedTipAmount now refuses a string containing U+0000 before converting it to a C string.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 688c75e1): parseShieldedTipAmount now rejects embedded U+0000 before withCString, so the binding no longer silently submits only a prefix to the Rust parser.
There was a problem hiding this comment.
Fixed in 84cfd41. resolveShieldedTip, reviewShieldedTip and sendShieldedTip reject a username containing U+0000 before the C call, as JNI does. The send reports it as a .notSent ShieldedTipFailure, before any native op is admitted.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 0dfe1513): Your amount wrapper still rejects embedded NUL before withCString, so Rust receives the complete representable amount. I also verified the subsequent username protections in resolve, review, and send described in your latest reply.
| if let Some(persister) = persister { | ||
| persister | ||
| .flush() | ||
| .map_err(|e| PlatformWalletError::Persistence(e.to_string()))?; | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
5e9971ba; see the replies below.
🟡 Suggestion: Propagate the guarded activity write failure before flushing
A successful flush does not prove that the Pending activity was successfully queued. record_pending_activity returns Some(entry) even when queue_shielded_changeset logs and discards persister.store's error. FFIPersister can roll back its store round and then return success from the independent flush callback, so this new durability gate still broadcasts after a known activity-write failure. On restart the signed guard survives without its recipient row, conservatively blocking every recipient until reconciliation. Keep that fail-closed fallback for unexpected loss, but propagate known write failures before broadcasting through a fallible guarded activity-recording path. Test a persister whose store fails while flush succeeds.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Addressed differently in 688c75e. The pending activity row no longer carries the payment barrier: the TipBarrier holds the recipient itself, in Rust's own store. A lost activity row can't hide the tip or block every recipient.
The flush is kept so the host shows the pending row across a relaunch. Making queue_shielded_changeset fallible would change every shielded operation, so I've left it out of this PR.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 688c75e1): Your independent barrier removes the recipient-loss and wallet-wide blocking consequences, but the flush still succeeds after a known activity-store failure and therefore does not provide the retained restart-history guarantee. A guarded-only fallible writer can address this remaining suggestion without changing every shielded operation's failure policy.
There was a problem hiding this comment.
Fixed in 38bfe13 with a guarded-only fallible writer. arm_parked_spend_guard re-stores the tip's activity changeset through persister.store(...) and then flushes, returning on either error before the barrier is recorded or anything is broadcast. The entry upserts by id, so the second write is idempotent. Other shielded operations keep their current failure policy.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 5e9971ba): Your guarded-only writer explicitly propagates persister.store failure and invokes flush only after store succeeds, with either error returning before barrier creation or broadcast. This fixes the known-write-failure case without changing every shielded operation's persistence policy.
| fn confirmed_tip_recipient( | ||
| &self, | ||
| wallet_id: WalletId, | ||
| label: &str, | ||
| ) -> Result<Option<ConfirmedTipRecipient>, Self::Error>; | ||
|
|
||
| /// Record `recipient` as the one the user confirmed for `label`. | ||
| fn confirm_tip_recipient( | ||
| &mut self, | ||
| wallet_id: WalletId, | ||
| label: &str, | ||
| recipient: ConfirmedTipRecipient, | ||
| ) -> Result<(), Self::Error>; |
There was a problem hiding this comment.
✅ Resolved at
688c75e1; see the replies below.
🟡 Suggestion: Document the public ShieldedStore source-compatibility break
confirmed_tip_recipient and confirm_tip_recipient are new required methods on the existing public ShieldedStore trait, whose documentation explicitly supports consumer-provided implementations. Those implementations stop compiling until both methods are added. The breaking-change description covers only the unreleased send bindings, and the existing Rust API migration notes do not mention these methods. Document the custom-backend migration, including wallet scoping, retention across shielded Clear, and deletion with purge_wallet. This is a Rust API compatibility change, not a PlatformVersion or GroveVersion change.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Documented in the PR description's Breaking Changes section. A custom ShieldedStore backend must now implement the recipient methods (confirmed_tip_recipient, confirm_tip_recipient) and the barrier methods (record_tip_barrier, tip_barriers, has_pending_tip_barriers, settle_tip_barrier, abandon_tip_barriers). It must also settle barriers as executed in mark_spent. The section also covers wallet scoping, the recipient record surviving Clear, and deletion with purge_wallet.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 688c75e1): Your Breaking Changes section now documents both required ShieldedStore method groups, wallet scoping, retention across Clear and account purges, purge_wallet deletion, and mark_spent settlement ordering.
| let _sending = self | ||
| .tip_send_in_progress | ||
| .try_lock() | ||
| .map_err(|_| PlatformWalletError::ShieldedTipPending)?; |
There was a problem hiding this comment.
✅ Resolved at
688c75e1; see the replies below.
🟡 Suggestion: No test proves a second concurrent tip is refused instead of queued
The new try_lock is the wallet-wide mechanism that prevents an overlapping repeat from waiting behind the first send and paying again after it succeeds. The added tests cover persisted guards and disposition mapping, but none exercises contention on this mutex or its sharing across PlatformWallet clones. Add a deterministic test holding the send gate on one handle and invoking send_shielded_tip through a clone; assert ShieldedTipPending/NotSent before recipient resolution or broadcast. This can be tested locally without a funded network.
source: muse-spark-1.3-contributor (phase1-reviewer: rust-quality)
There was a problem hiding this comment.
Added in 688c75e: should_refuse_a_second_tip_while_one_is_being_sent (viewing_key_bind_tests.rs). It holds the send lock on one wallet handle and sends through a clone, and the second send fails with ShieldedTipPending before any resolution. The release path is also refused while a tip is being sent.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 688c75e1): Your new should_refuse_a_second_tip_while_one_is_being_sent test holds the original handle's send lock and sends through a clone, verifying ShieldedTipPending before recipient resolution. The separate disposition mapping covers that refusal as NotSent.
| let wallet_id: [u8; 32] = std::slice::from_raw_parts(wallet_id_bytes, 32) | ||
| .try_into() | ||
| .unwrap(); |
There was a problem hiding this comment.
✅ Resolved at
688c75e1; see the replies below.
💬 Nitpick: New review export uses unwrap() on an infallible conversion in unsafe FFI
This conversion cannot fail because the slice is constructed with exactly 32 bytes, so it is not a reachable panic defect. However, the review conventions prohibit production unwrap and permit expect when its message states the established invariant. Express that invariant here rather than adding another unexplained unwrap to an unsafe export.
| let wallet_id: [u8; 32] = std::slice::from_raw_parts(wallet_id_bytes, 32) | |
| .try_into() | |
| .unwrap(); | |
| let wallet_id: [u8; 32] = std::slice::from_raw_parts(wallet_id_bytes, 32) | |
| .try_into() | |
| .expect("wallet_id slice is exactly 32 bytes by construction"); |
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
There was a problem hiding this comment.
Fixed in 688c75e: both new exports now use expect("wallet_id slice is exactly 32 bytes by construction").
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 688c75e1): Both new exports now use expect with the exact 32-byte construction invariant in the message, addressing the requested change.
…ts outcome is proven The tip's retry guard was the generic recovery record, whose lifetime follows note reservations and account state. Review found four ways it could vanish or be ignored before the original payment's outcome was known, each permitting a second payment. - A tip records a TipBarrier (recipient, notes, anchor) in a new shielded_tip_barriers table, apart from note and recovery state, so account purges, Clear and rescans do not remove it. - The store settles it as executed in mark_spent, before the recovery row is deleted. It settles as failed only on the tip's own definitive rejection: the tip is broadcast once with no retries, so a consensus rejection speaks for the only node that received it; transport errors such as Unavailable, and NullifierAlreadySpent, stay unresolved. The release pass and re-drive rejections no longer settle tips. - An unresolved tip whose outcome stays unknown is released only by abandon_unresolved_shielded_tips with the user's acknowledgement that it may still execute (C, JNI, Swift, Kotlin). - While a tip is pending, a bind that drops or re-keys its paying account and a shielded Clear are refused; the send holds shielded_config_lock. - An unreadable barrier blocks its wallet's tips instead of being skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The independent barrier design addresses the earlier decoding, purge, settlement-ordering, and transport-error failures, but four blocking issues remain: insufficient commit durability, unauthenticated rejection handling, cross-wallet corruption release, and recovery-worker shutdown after a refused Clear. Three suggestions remain, including the narrower host-activity persistence issue from the prior review. Verification was static only; Kotlin build/tests passed, while the supplied CI snapshot showed Rust workspace tests pending and the dedicated wallet job skipped.
🔴 4 blocking | 🟡 3 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/tips.rs and barrier persistence in file_store.rs directly alter funds-movement safety, retry eligibility, note reservations, and durable storage. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1115-1129: Synchronize the payment barrier to durable storage before broadcasting
This INSERT uses pending_conn, which open_path creates through open_tuned_connection with WAL synchronous=NORMAL. A successful execute therefore does not guarantee that the committed barrier has reached stable storage before broadcasting. Power loss or an OS crash after delivery but before WAL synchronization can discard both the barrier and parked recovery row. After reopening, has_unresolved_transfer_to consults only barriers, so another funded account can submit a replacement payment even if the host retained its Pending activity. The host flush precedes these writes and cannot synchronize this separate database. Configure the authoritative recovery/barrier connection with synchronous=FULL or establish an equivalent checked durability boundary before permitting broadcast; the commitment-tree connection can retain its performance settings. Assert the writer's durability configuration and pre-broadcast ordering; graceful close/reopen tests do not establish power-loss durability.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1196-1207: Keep damaged-barrier release within the acknowledgement's wallet scope
abandon_unresolved_shielded_tips obtains acknowledgement for self.wallet_id(), but this branch deletes every barrier whose wallet ID has an invalid length and clears the global damage flag. If an unresolved payment from wallet B has a damaged wallet_id, acknowledging abandonment for wallet A therefore removes B's protection too. B can subsequently submit another payment from other notes without acknowledgement of B's unresolved payment, because tip admission no longer consults pending_redrives. Keep unattributed barriers fail-closed unless ownership can be recovered, or provide a separately explicit coordinator-wide acknowledgement operation. Add a two-wallet regression showing that wallet A's acknowledgement cannot release wallet B's unidentified barrier.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1184-1189: Delete every decoder-rejected barrier during acknowledged repair
The repair predicate is weaker than load_tip_barriers' decoder. A row with account_index = -1 and otherwise valid fields is rejected by row.get::<_, u32>(2), marking its wallet damaged. Abandonment updates its status to Abandoned, but this DELETE retains it because account_index is never checked. The method clears the in-memory damage flag and reports success, yet reopening rejects the same row and blocks tips and Clear again. Storage-class mismatches can likewise pass the SQL length checks while failing blob decoding. Track and remove the exact rejected rows, or share the decoder's complete validity rules rather than maintaining a second approximation. Extend the reopen coverage with an invalid account index.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3169-3175: Do not settle a tip from an unauthenticated DAPI rejection
carries_consensus_rejection identifies a decoded error, not authenticated evidence that the payment cannot execute. rs-sdk/src/error.rs deserializes dash-serialized-consensus-error-bin metadata without authenticating the verdict. A malicious DAPI endpoint can forward the valid signed transfer and return a fabricated consensus rejection other than NullifierAlreadySpent. This classifier then reports definitive failure, and transfer_with marks the barrier Failed, releases the reservation, and reports NotSent; a replacement funded from other notes can pay twice. The guarded wait path has the same problem: wait_for_outcome_with_metadata returns error envelopes before proof verification, and classify_spend_wait_failure treats their serialized causes as definitive. Keep unverified remote rejection claims unresolved unless authenticated evidence or a locally established invariant proves non-execution. Cover fabricated broadcast metadata and wait-error envelopes, asserting that neither releases the barrier.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3124-3128: Propagate the guarded activity write failure before flushing
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437204446)
The independent TipBarrier fixes the earlier recipient-loss and all-recipient blocking consequences, but this flush still cannot establish the promised restart-visible Pending activity. record_pending_activity returns Some after queue_shielded_changeset logs and discards persister.store's error. FFIPersister can roll back that store round and then succeed in its independent flush callback, allowing broadcast despite a known failure to retain the payment history. The barrier contains neither the amount nor the full activity record needed to display that pending payment after relaunch. This does not require changing every shielded operation: add a fallible recording path specifically for guarded tips while preserving ordinary callers' best-effort behavior. Cover a store callback that fails while flush succeeds.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1238-1242: Preserve background recovery when Clear is refused
The public PlatformWalletManager::clear_shielded path calls shielded_sync_manager.quiesce_held before reaching this refusal. That drain calls stop(), cancelling the background worker. Returning ShieldedTipPending leaves the coordinator registered, but dropping the quiesce and clearing guards only reopens admission; neither recreates the worker. Attempting a forbidden Clear therefore silently stops the automatic scans and re-drives needed to settle pending payments, including those belonging to other wallets on this manager. Preserve or restore the previously running worker on this non-destructive refusal, or perform a race-free admission check before stopping it. Test through manager.clear_shielded and verify that a refused Clear leaves an already-running recovery loop running; the added coordinator.clear test bypasses this ordering.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:2081-2082: Carry the native tip disposition through coroutine cancellation
TeardownGate.withOp uses withContext(Dispatchers.IO). Cancelling the caller does not interrupt the blocking native send, and prompt cancellation can discard its completed result when dispatching back to the caller. JNI has already copied Rust's disposition into the array, but this catch throws a bare CancellationException, making cancellation before native entry indistinguishable from cancellation after an executed payment. After success the barrier can already be settled, so another review need not report an unresolved payment either. Preserve the completed native outcome in a cancellation-compatible exception or result without adding a Kotlin retry policy. Add a deterministic completed-send/cancelled-handoff test using the sequence already exercised by TeardownGateTest for discarded secret results.
| conn.execute( | ||
| "INSERT OR REPLACE INTO shielded_tip_barriers \ | ||
| (wallet_id, activity_id, account_index, recipient, nullifiers, anchor, status) \ | ||
| VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)", | ||
| rusqlite::params![ | ||
| wallet_id.as_slice(), | ||
| barrier.activity_id.as_slice(), | ||
| barrier.account_index, | ||
| barrier.recipient.as_slice(), | ||
| barrier.nullifiers.concat(), | ||
| barrier.anchor.as_slice(), | ||
| status | ||
| ], | ||
| ) | ||
| .map_err(|e| FileShieldedStoreError::Storage(format!("write tip barrier: {e}")))?; |
There was a problem hiding this comment.
✅ Resolved at
5e9971ba; see the replies below.
🔴 Blocking: Synchronize the payment barrier to durable storage before broadcasting
This INSERT uses pending_conn, which open_path creates through open_tuned_connection with WAL synchronous=NORMAL. A successful execute therefore does not guarantee that the committed barrier has reached stable storage before broadcasting. Power loss or an OS crash after delivery but before WAL synchronization can discard both the barrier and parked recovery row. After reopening, has_unresolved_transfer_to consults only barriers, so another funded account can submit a replacement payment even if the host retained its Pending activity. The host flush precedes these writes and cannot synchronize this separate database. Configure the authoritative recovery/barrier connection with synchronous=FULL or establish an equivalent checked durability boundary before permitting broadcast; the commitment-tree connection can retain its performance settings. Assert the writer's durability configuration and pre-broadcast ordering; graceful close/reopen tests do not establish power-loss durability.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 38bfe13. FileBackedShieldedStore::open_path now sets PRAGMA synchronous=FULL; PRAGMA fullfsync=ON; on pending_conn, which writes the barrier, recovery and recipient tables. fullfsync makes that sync flush the drive cache on Apple platforms. Each commit there is therefore synced before record_tip_barrier and arm_redrive return, and both precede the broadcast in arm_parked_spend_guard. The tree connection keeps NORMAL.
tip_barriers_settle_durably_and_outlive_account_purges asserts PRAGMA synchronous = 2 (FULL) on that connection. As you say, a graceful reopen test can't demonstrate power-loss durability; the test pins the configuration.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 5e9971ba): Your authoritative pending connection enables synchronous=FULL and fullfsync before barrier and recovery writes, and both writes finish before broadcast. The configuration assertion pins that durability boundary without relying on graceful reopen as a power-loss test.
| fn tip_broadcast_definitely_failed(e: &dash_sdk::Error) -> bool { | ||
| use dash_sdk::dapi_client::DapiClientError; | ||
| (carries_consensus_rejection(e) && !is_nullifier_already_spent(e)) | ||
| || matches!( | ||
| e, | ||
| dash_sdk::Error::DapiClientError(DapiClientError::NoAvailableAddresses) | ||
| ) |
There was a problem hiding this comment.
✅ Resolved at
5e9971ba; see the replies below.
🔴 Blocking: Do not settle a tip from an unauthenticated DAPI rejection
carries_consensus_rejection identifies a decoded error, not authenticated evidence that the payment cannot execute. rs-sdk/src/error.rs deserializes dash-serialized-consensus-error-bin metadata without authenticating the verdict. A malicious DAPI endpoint can forward the valid signed transfer and return a fabricated consensus rejection other than NullifierAlreadySpent. This classifier then reports definitive failure, and transfer_with marks the barrier Failed, releases the reservation, and reports NotSent; a replacement funded from other notes can pay twice. The guarded wait path has the same problem: wait_for_outcome_with_metadata returns error envelopes before proof verification, and classify_spend_wait_failure treats their serialized causes as definitive. Keep unverified remote rejection claims unresolved unless authenticated evidence or a locally established invariant proves non-execution. Cover fabricated broadcast metadata and wait-error envelopes, asserting that neither releases the barrier.
source: gpt-6.1-sol (phase2-reviewer: security-auditor)
There was a problem hiding this comment.
Fixed in 38bfe13. broadcast_guarded_transfer (operations.rs) no longer trusts any rejection a node reports.
- Broadcast. The only definitive case is
NoAvailableAddresses, where nothing was sent (tip_broadcast_never_sent). Every other broadcast error falls through to the wait. - Wait. Every wait error, including a consensus-coded envelope, becomes
ShieldedSpendUnconfirmed. The disposition isUnresolvedand the guard is re-armed for re-drive. - Result. A barrier settles as failed only when the tip never left the device. A fabricated success can't pass the wait's proof verification.
- UX cost. A tip that was genuinely rejected now stays unresolved until its notes settle or the user releases it with
abandon_unresolved_shielded_tips. - Test.
tip_broadcast_is_definitive_only_when_it_reached_no_nodecovers a consensus-coded rejection,NullifierAlreadySpent,Unavailable,DeadlineExceededand a no-verdict broadcast error; all stay unresolved.
The test is a unit test of the classifier. The wait-envelope path is the unconditional mapping of every wait error, so it isn't tested separately.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 5e9971ba): Your foreground guarded send now accepts only locally established NoAvailableAddresses as definitive and maps every wait error to ShieldedSpendUnconfirmed, so remote rejection claims cannot mark its barrier Failed or authorize a replacement. The separate re-drive reservation/activity defect is reported independently because that path does not release the tip barrier.
| if self.unattributed_tip_barrier_damage { | ||
| conn.execute( | ||
| "DELETE FROM shielded_tip_barriers WHERE length(wallet_id) != 32", | ||
| [], | ||
| ) | ||
| .map_err(|e| { | ||
| FileShieldedStoreError::Storage(format!("drop damaged tip barriers: {e}")) | ||
| })?; | ||
| } | ||
| drop(conn); | ||
| self.damaged_tip_barrier_wallets.remove(&wallet_id); | ||
| self.unattributed_tip_barrier_damage = false; |
There was a problem hiding this comment.
✅ Resolved at
5e9971ba; see the replies below.
🔴 Blocking: Keep damaged-barrier release within the acknowledgement's wallet scope
abandon_unresolved_shielded_tips obtains acknowledgement for self.wallet_id(), but this branch deletes every barrier whose wallet ID has an invalid length and clears the global damage flag. If an unresolved payment from wallet B has a damaged wallet_id, acknowledging abandonment for wallet A therefore removes B's protection too. B can subsequently submit another payment from other notes without acknowledgement of B's unresolved payment, because tip admission no longer consults pending_redrives. Keep unattributed barriers fail-closed unless ownership can be recovered, or provide a separately explicit coordinator-wide acknowledgement operation. Add a two-wallet regression showing that wallet A's acknowledgement cannot release wallet B's unidentified barrier.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in 38bfe13. Releasing a wallet's tips no longer touches barriers whose wallet id is unreadable.
- What it deletes.
abandon_tip_barriersdeletes only that wallet's rows rejected at load, by rowid and wallet id. - Unknown owner.
unattributed_tip_barrier_damagestays set, so such a row keeps blocking every wallet's tips and Clear, and no wallet's acknowledgement releases it. - Binds. Such a row no longer fails shielded binds, since an unreadable barrier can't settle through its account anyway.
- Test.
an_unattributed_damaged_tip_barrier_is_not_released_by_another_walletchecks that wallet A's acknowledgement leaves wallet B's unidentified barrier blocking, including after a reopen.
I didn't add a separate coordinator-wide acknowledgement. A row whose wallet id is unreadable can only come from on-disk damage, so it fails closed. If you want an explicit recovery path for that case, I can add one.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 5e9971ba): Your wallet-scoped abandonment leaves unattributed rows and their global damage flag intact, so one wallet's acknowledgement cannot release another wallet's unidentified barrier. The two-wallet reopen regression verifies that fail-closed behavior; a coordinator-wide release API is not required to fix this scope violation.
| // A pending tip settles through the note and recovery state a | ||
| // Clear deletes; clearing first would leave it unresolvable. | ||
| match store.has_pending_tip_barriers() { | ||
| Ok(false) => {} | ||
| Ok(true) => return Err(crate::error::PlatformWalletError::ShieldedTipPending), |
There was a problem hiding this comment.
✅ Resolved at
5e9971ba; see the replies below.
🔴 Blocking: Preserve background recovery when Clear is refused
The public PlatformWalletManager::clear_shielded path calls shielded_sync_manager.quiesce_held before reaching this refusal. That drain calls stop(), cancelling the background worker. Returning ShieldedTipPending leaves the coordinator registered, but dropping the quiesce and clearing guards only reopens admission; neither recreates the worker. Attempting a forbidden Clear therefore silently stops the automatic scans and re-drives needed to settle pending payments, including those belonging to other wallets on this manager. Preserve or restore the previously running worker on this non-destructive refusal, or perform a race-free admission check before stopping it. Test through manager.clear_shielded and verify that a refused Clear leaves an already-running recovery loop running; the added coordinator.clear test bypasses this ordering.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 38bfe13. PlatformWalletManager::clear_shielded checks coordinator.has_pending_tips() before it quiesces, so a Clear refused for a pending tip never stops the worker.
If a tip turns pending between that check and the coordinator's own refusal, the manager restarts the worker. It does so only if the worker was running, and only after the clearing latch and quiesce guard are released (the body moved into quiesce_and_clear_shielded). start() still declines during teardown or a concurrent Clear.
The new test a_refused_shielded_clear_keeps_background_sync_running (manager/mod.rs) starts the worker, records a pending tip, calls manager.clear_shielded(), and asserts ShieldedTipPending with the worker still running.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 5e9971ba): Your manager now refuses existing pending tips before quiescing and restarts a previously running worker on a later ShieldedTipPending refusal after the helper's guards have dropped. The manager-level regression checks that the ordinary refused-Clear path leaves background sync running.
| if self.damaged_tip_barrier_wallets.contains(&wallet_id) { | ||
| conn.execute( | ||
| "DELETE FROM shielded_tip_barriers WHERE wallet_id = ?1 AND NOT (\ | ||
| status IN (0, 1, 2, 3) AND length(activity_id) = 32 \ | ||
| AND length(recipient) = 43 AND length(anchor) = 32 \ | ||
| AND length(nullifiers) > 0 AND length(nullifiers) % 32 = 0)", |
There was a problem hiding this comment.
✅ Resolved at
5e9971ba; see the replies below.
🟡 Suggestion: Delete every decoder-rejected barrier during acknowledged repair
The repair predicate is weaker than load_tip_barriers' decoder. A row with account_index = -1 and otherwise valid fields is rejected by row.get::<_, u32>(2), marking its wallet damaged. Abandonment updates its status to Abandoned, but this DELETE retains it because account_index is never checked. The method clears the in-memory damage flag and reports success, yet reopening rejects the same row and blocks tips and Clear again. Storage-class mismatches can likewise pass the SQL length checks while failing blob decoding. Track and remove the exact rejected rows, or share the decoder's complete validity rules rather than maintaining a second approximation. Extend the reopen coverage with an invalid account index.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 38bfe13. load_tip_barriers now records the rowid of every row its decoder rejects, including an account_index that doesn't fit a u32. The release deletes exactly those rowids instead of approximating the decoder in SQL. tip_barriers_settle_durably_and_outlive_account_purges now includes an account_index = -1 row and checks that the wallet is clean after a reopen.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 5e9971ba): Your loader records the exact decoder-rejected rowids, and abandonment deletes those rows with a wallet-ID qualification rather than approximating validity in SQL. The added negative-account-index fixture and reopen assertion cover the specific repair failure.
| } catch (error: kotlinx.coroutines.CancellationException) { | ||
| throw error |
There was a problem hiding this comment.
✅ Resolved at
5e9971ba; see the replies below.
🟡 Suggestion: Carry the native tip disposition through coroutine cancellation
TeardownGate.withOp uses withContext(Dispatchers.IO). Cancelling the caller does not interrupt the blocking native send, and prompt cancellation can discard its completed result when dispatching back to the caller. JNI has already copied Rust's disposition into the array, but this catch throws a bare CancellationException, making cancellation before native entry indistinguishable from cancellation after an executed payment. After success the barrier can already be settled, so another review need not report an unresolved payment either. Preserve the completed native outcome in a cancellation-compatible exception or result without adding a Kotlin retry policy. Add a deterministic completed-send/cancelled-handoff test using the sequence already exercised by TeardownGateTest for discarded secret results.
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
There was a problem hiding this comment.
Fixed in 38bfe13. PlatformWalletManager.sendShieldedTip runs the gate and the native send under withContext(NonCancellable). A cancelled caller therefore still gets the completed outcome, and a failure still carries the JNI-written disposition. Cancellation takes effect at the caller's next suspension point.
There is no Kotlin test for this. Gradle doesn't run in this environment, and the safety itself doesn't depend on it, because the Rust barrier is what refuses a second payment.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 5e9971ba): Your NonCancellable scope now encloses both teardownGate.op and the native send, so the IO handoff returns into a non-cancelled context and preserves the completed outcome and JNI-written disposition. The deterministic cancellation regression remains absent, but the reported result-discard path is addressed.
- Tip barriers and recovery rows are synced to disk before the tip is broadcast (synchronous=FULL and fullfsync on their connection), so they survive power loss, not only a crash. - A node's rejection, at broadcast or in the wait for the result, is unauthenticated, so it leaves the tip unresolved. A tip settles as failed only when it reached no node at all. - Releasing a wallet's unresolved tips deletes exactly that wallet's unreadable barrier rows; a barrier whose wallet is unknown stays and keeps blocking tips and Clear, but no longer fails shielded binds. - A Clear refused for a pending tip no longer stops background sync, which is what settles the tip. - The tip's activity row is re-stored fallibly before broadcast, and the Kotlin send is not cancellable once started, so its outcome is never lost. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
38bfe13 to
5e9971b
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 5e9971b confirms 14 prior findings are fixed, but the prior lifecycle finding remains partially unresolved: Clear can overtake a tip before its barrier exists. A second blocker affects guarded-tip recovery, which still releases reservations and records failure based on unauthenticated re-drive rejection claims; five nonblocking suggestions also remain. No builds or tests were run; current GitHub checks show Kotlin passing, while the supplied CI snapshot records Rust workspace tests queued and the dedicated wallet job skipped.
🔴 2 blocking | 🟡 5 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes funds-movement safety in send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/operations.rs and adds durable payment-barrier storage in file_store.rs, governing duplicate-payment prevention, note reservation, and recovery across restarts. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1244-1249: Protect unresolved tip guards from account purge and Clear
The independent barrier protects already-recorded tips, but the acknowledged pre-barrier Clear window still violates lifecycle ordering. After transfer_with extracts valid witnesses, proof construction runs before arm_parked_spend_guard records the barrier. During that interval, manager.clear_shielded can find no pending barrier, purge the store, remove registrations, stop background sync, and return success. The sender holds shielded_config_lock, but Clear does not acquire it, and shielded_transfer_with neither holds the coordinator lifecycle guard nor validates clear_generation before recreating activity/barrier/recovery state and broadcasting. Thus a successful Clear can be followed by a payment broadcasting and persistence writes occurring behind the host's wipe; an ambiguous payment also loses its registered recovery account. Retaining duplicate-tip protection does not make those state changes safe. Serialize Clear with pre-barrier send admission, or invalidate stale sends and abort them before they recreate state or broadcast. Add a deterministic manager-level Clear test with the sender paused after witness extraction.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3243-3245: Preserve guarded-tip outcome semantics through re-drive
Resetting this attempt budget hands an unresolved guarded tip to redrive_pending_spends. That path still calls classify_redrive_broadcast, which treats a consensus-coded remote error other than NullifierAlreadySpent as DefinitiveRejection; its rejection arm clears input reservations, deletes the recovery record through clear_pending, and persists Failed activity. The SDK merely deserializes the rejection metadata, so a dishonest endpoint can forward the original payment and fabricate this response, and a lagging node's rejection does not invalidate an earlier accepted relay. The independent tip barrier remains pending, so duplicate-tip admission stays blocked, but the wallet now reports a potentially executing payment as failed and offers its at-risk inputs to other spending paths. Apply the conservative trust policy throughout recovery for barrier-backed tips: retain reservations, recovery state, and unresolved activity on unauthenticated rejection claims. Add an ambiguous guarded send followed by a consensus-coded re-drive rejection regression that checks all four records.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3126-3133: Exercise failures in the guarded host-persistence gate
Every arm_parked_spend_guard invocation in parked_spend_guard_tests passes persister: None, so none exercises the new fallible store-and-flush branch. This branch distinguishes refusing a payment before broadcast from merely logging an activity-write failure. Add deterministic fake-persister cases for a failed store with an otherwise successful flush implementation, and for a successful store followed by a failed flush. Assert the Persistence error, that flush is not called after store fails, and that neither a tip barrier nor a recovery record is armed. These tests require neither proving nor a live network.
In `packages/rs-platform-wallet/src/wallet/shielded/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/store.rs:179-185: Index tip barriers by nullifier on the scan hot path
Both store implementations call this full-map traversal from mark_spent before their ordinary note lookup. apply_scanned_nullifier_spends invokes mark_spent for every scanned network nullifier against every registered subwallet, including nullifiers the wallet does not own. This therefore adds work proportional to scanned actions × registered subwallets × all retained barriers, including unrelated wallets and already-executed rows. Because settled rows remain for distinct recipients, historical tipping activity multiplies future scan work under the shared store write lock even when no tip is pending. Maintain a reverse lookup keyed by wallet and nullifier for barriers that can still transition to Executed, updating it on load, insertion, settlement, replacement, abandonment, and wallet purge while preserving SQL-first settlement ordering.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:727-733: Preserve retry work when barrier settlement cannot be persisted
A settlement-write failure is logged, but mark_spent continues to mark the note spent, remove its recovery record, and return success. The scan can then advance its watermark beyond the confirming action. If the failure persists through that pass, subsequent background passes need not encounter the nullifier again, leaving an already-confirmed tip's barrier Pending and blocking its recipient, account changes, and Clear until another restoration path or explicit abandonment intervenes. Propagate the settlement error before mutating SubwalletState, or retain explicit settlement-retry work. Add a single-subwallet failure-injection test that fails the status update during confirmation and verifies ordinary recovery later settles the barrier without abandonment.
In `packages/rs-platform-wallet/src/wallet/shielded/tips.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:207-213: Share canonical DPNS normalization with recipient resolution
tip_recipient_label independently reproduces extract_dpns_label plus normalize_dpns_label from rs-sdk/src/platform/dpns_usernames/mod.rs, with an additional trim that the resolver does not perform. Recipient confirmations and the rotated-address pending check depend on this key continuing to identify exactly the name used by resolution. The implementations agree for ordinary supported names today, so this is not an established payment-routing defect, but maintaining a second canonicalization implementation creates an avoidable synchronization requirement in a safety-sensitive path. Expose a shared canonical helper from the SDK or its transport-free DPNS owner and use it for resolution and recipient-history keys, with one explicit whitespace policy.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:1748-1750: Reject embedded NUL before converting the review username to a C string
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437204437)
The new review wrapper accepts a Swift String containing U+0000, but username.withCString passes it through a null-terminated ABI and the Rust export reads it with CStr::from_ptr. Consequently, "alice\u{0000}suffix" reviews "alice" and returns the prefix's recipient and confirmation history rather than processing the complete supplied name. JNI rejects the same input through CString::new, so the bindings disagree. Reject embedded NUL before this conversion, as parseShieldedTipAmount already does, or use a length-bearing export. This is ABI representability checking; DPNS validation should remain in Rust.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Extend untrusted-rejection handling to ordinary foreground shielded spends — The ordinary broadcast_shielded_spend and classify_spend_wait_failure paths still treat decoded remote consensus errors as definitive non-execution and release reservations. A malicious endpoint can forward a payment and fabricate that rejection, potentially inducing a replacement payment using different notes. This behavior is present in the exact base revision; remediation of ordinary foreground spends is separate from the in-scope guarded-tip recovery defect above.
- Follow-up: Track a separate security fix for ordinary shielded-send rejection handling, with forwarded-payment/fabricated-rejection regression coverage.
| let mut store = self.store.write().await; | ||
| // A pending tip settles through the note and recovery state a | ||
| // Clear deletes; clearing first would leave it unresolvable. | ||
| match store.has_pending_tip_barriers() { | ||
| Ok(false) => {} | ||
| Ok(true) => return Err(crate::error::PlatformWalletError::ShieldedTipPending), |
There was a problem hiding this comment.
✅ Resolved at
0dfe1513; see the replies below.
🔴 Blocking: Protect unresolved tip guards from account purge and Clear
The independent barrier protects already-recorded tips, but the acknowledged pre-barrier Clear window still violates lifecycle ordering. After transfer_with extracts valid witnesses, proof construction runs before arm_parked_spend_guard records the barrier. During that interval, manager.clear_shielded can find no pending barrier, purge the store, remove registrations, stop background sync, and return success. The sender holds shielded_config_lock, but Clear does not acquire it, and shielded_transfer_with neither holds the coordinator lifecycle guard nor validates clear_generation before recreating activity/barrier/recovery state and broadcasting. Thus a successful Clear can be followed by a payment broadcasting and persistence writes occurring behind the host's wipe; an ambiguous payment also loses its registered recovery account. Retaining duplicate-tip protection does not make those state changes safe. Serialize Clear with pre-barrier send admission, or invalidate stale sends and abort them before they recreate state or broadcast. Add a deterministic manager-level Clear test with the sender paused after witness extraction.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in 84cfd41. send_shielded_tip now takes coordinator.admit_tip_send(wallet_id) before it resolves the recipient or selects notes, and holds it until the send returns. Admission increments a per-wallet count under the coordinator lifecycle mutex, which Clear holds for its whole run, so no send is admitted mid-Clear. While a send is admitted:
clear()returnsShieldedTipPending, andhas_pending_tips()reports it, so the manager pre-check refuses too;register_lockedrefuses a bind that drops or re-keys that wallet's accounts;unregister_wallet(wallet removal) waits for the send to end before purging.
Tests: should_refuse_clear_and_account_drops_while_a_tip_send_is_admitted pauses a real send_shielded_tip right after admission. should_remove_a_wallet_only_after_its_tip_send_ends covers removal. The pause sits before witness extraction rather than after it; admission covers the whole send, so the extraction-to-barrier window is inside it.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 0dfe1513): Your admission begins before resolution and note selection and remains alive through the send, while Clear checks the admitted count under lifecycle serialization. Pausing earlier than witness extraction still covers the entire reported pre-barrier interval; the removal deadlock is a separate finding.
| if let Some(mut record) = parked { | ||
| record.attempts = 0; | ||
| if let Err(e) = store.arm_redrive(id, record) { |
There was a problem hiding this comment.
✅ Resolved at
bc99e1e8; see the replies below.
🔴 Blocking: Preserve guarded-tip outcome semantics through re-drive
Resetting this attempt budget hands an unresolved guarded tip to redrive_pending_spends. That path still calls classify_redrive_broadcast, which treats a consensus-coded remote error other than NullifierAlreadySpent as DefinitiveRejection; its rejection arm clears input reservations, deletes the recovery record through clear_pending, and persists Failed activity. The SDK merely deserializes the rejection metadata, so a dishonest endpoint can forward the original payment and fabricate this response, and a lagging node's rejection does not invalidate an earlier accepted relay. The independent tip barrier remains pending, so duplicate-tip admission stays blocked, but the wallet now reports a potentially executing payment as failed and offers its at-risk inputs to other spending paths. Apply the conservative trust policy throughout recovery for barrier-backed tips: retain reservations, recovery state, and unresolved activity on unauthenticated rejection claims. Add an ambiguous guarded send followed by a consensus-coded re-drive rejection regression that checks all four records.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
There was a problem hiding this comment.
Fixed in 84cfd41. redrive_pending_spends checks is_guarded_tip before re-broadcasting. That holds while a barrier for the activity is pending or abandoned, or when the wallet's barriers are unreadable (fail closed). classify_redrive_broadcast(result, guarded_tip) then maps a consensus rejection to Inconclusive. Reservations, the recovery record, the activity row and the barrier all stay, and the attempt still counts toward the cap, which leaves later handling to the scan or prune passes. NullifierAlreadySpent is still matched first. Tests: the classification matrix gains the guarded case, and should_guard_a_redriven_tip_until_its_barrier_settles covers the barrier states. There is no end-to-end redrive_pending_spends test with a fabricated rejection, because the mock SDK cannot return a broadcast error.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 0dfe1513): Your guarded classifier protects the retained Pending/Abandoned cases, but recording a replacement tip deletes the old Abandoned barrier while its recovery operation remains armed, restoring the unsafe rejection branch. An abandon-then-replace helper regression can demonstrate this remaining gap without extending the mock SDK's broadcast behavior.
There was a problem hiding this comment.
The abandoned-replacement case is fixed in ca92a58. record_tip_barrier now prunes only executed and failed barriers of the recipient, in both stores; pending and abandoned ones stay. An abandoned tip's barrier therefore keeps marking its recovery record as a guarded tip until the tip settles. Tests: should_guard_a_redriven_tip_until_its_barrier_settles now runs arm → abandon → replacement tip and checks that the old activity is still guarded. a_replacement_tip_keeps_an_abandoned_barrier checks the same thing durably across a reopen.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at ca92a584): Your retention change fixes abandon-then-replace for readable barriers, but acknowledged repair deletes a decoder-rejected barrier and clears its damage entry while leaving its valid recovery operation armed. That operation then loses guarded classification and can release reservations and persist Failed on an unauthenticated rejection; the carried finding details the remaining repair sequence.
There was a problem hiding this comment.
Fixed in 9fb9160. Before abandon_tip_barriers deletes a wallet's damaged rows, it writes an abandoned stand-in barrier for each of that wallet's note-spending recovery operations that no readable barrier claims. The stand-in uses the operation's own activity id, account, nullifiers and anchor, with a zero recipient because the real one is unreadable. The stand-ins go in first, so a crash in between cannot lose the marking. is_guarded_tip keeps classifying the operation as a tip, and the stand-in settles as executed once its notes are seen spent. Cost: an ordinary spend in flight at that moment also waits for the prune backstop, but only when the user repairs damage. The ShieldedStore doc now requires this. Test: repairing_a_damaged_barrier_keeps_its_recovery_marked_as_a_tip arms a valid recovery, damages its barrier's account_index, reopens and acknowledges. After a second reopen it checks that the activity has an abandoned barrier with the operation's nullifiers.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at bc99e1e8): Your latest repair writes Abandoned stand-ins for otherwise unclaimed note-spending recovery operations before deleting damaged rows, and the reopen regression verifies that their guarded classification survives acknowledgement. Pending and Abandoned barriers also survive replacement, so the guarded classifier continues treating remote rejections as inconclusive.
| barriers | ||
| .iter() | ||
| .filter(|((wallet, _), barrier)| { | ||
| *wallet == wallet_id | ||
| && barrier.status != TipBarrierStatus::Executed | ||
| && barrier.nullifiers.contains(nullifier) | ||
| }) |
There was a problem hiding this comment.
✅ Withdrawn at
0dfe1513; see the replies below.
🟡 Suggestion: Index tip barriers by nullifier on the scan hot path
Both store implementations call this full-map traversal from mark_spent before their ordinary note lookup. apply_scanned_nullifier_spends invokes mark_spent for every scanned network nullifier against every registered subwallet, including nullifiers the wallet does not own. This therefore adds work proportional to scanned actions × registered subwallets × all retained barriers, including unrelated wallets and already-executed rows. Because settled rows remain for distinct recipients, historical tipping activity multiplies future scan work under the shared store write lock even when no tip is pending. Maintain a reverse lookup keyed by wallet and nullifier for barriers that can still transition to Executed, updating it on load, insertion, settlement, replacement, abandonment, and wallet purge while preserving SQL-first settlement ordering.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Not changing this here. The traversal is over in-memory barriers only: one row per tip, compared by nullifier in memory with no SQL. A wallet would need many thousands of tips before it costs anything next to the trial decryption on the same pass. A reverse index would have to stay consistent across load, insert, settle, abandon and purge in both stores, which adds risk to the settlement ordering for no measured gain. I'll add the index if profiling shows the scan spends time here.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Withdrawn (re-reviewed at 0dfe1513): I verified your point that the traversal performs in-memory comparisons rather than database queries; requiring an additional synchronized index is not justified by the available performance evidence. I withdraw the indexing requirement unless profiling demonstrates a material scan cost.
| for activity_id in tip_barriers_spending(&self.tip_barriers, id.wallet_id, nullifier) { | ||
| if let Err(e) = | ||
| self.set_tip_barrier_status(id.wallet_id, &activity_id, TipBarrierStatus::Executed) | ||
| { | ||
| tracing::warn!(error = %e, "tip barrier settlement not persisted; it stays pending"); | ||
| } | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
0dfe1513; see the replies below.
🟡 Suggestion: Preserve retry work when barrier settlement cannot be persisted
A settlement-write failure is logged, but mark_spent continues to mark the note spent, remove its recovery record, and return success. The scan can then advance its watermark beyond the confirming action. If the failure persists through that pass, subsequent background passes need not encounter the nullifier again, leaving an already-confirmed tip's barrier Pending and blocking its recipient, account changes, and Clear until another restoration path or explicit abandonment intervenes. Propagate the settlement error before mutating SubwalletState, or retain explicit settlement-retry work. Add a single-subwallet failure-injection test that fails the status update during confirmation and verifies ordinary recovery later settles the barrier without abandonment.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Fixed in 84cfd41. A settlement write that fails is kept in unsettled_tip_executions and retried at each watermark advance (set_last_synced_note_index, once per pass rather than once per scanned nullifier); purge_wallet drops the wallet's entries. I didn't propagate the error from mark_spent: the pass may already have marked other notes spent, and on the rescan those report as not newly spent, so the host would never receive their spent records. A restart also settles it again, because cold-start restore calls mark_spent for persisted spent notes. Test: a_failed_tip_settlement_is_retried_until_it_persists fails the UPDATE with a trigger, checks that an unrelated mark_spent does not settle the barrier, and then checks that the watermark advance settles it durably.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 0dfe1513): Your explicit unsettled_tip_executions work survives a failed settlement update and retries on watermark advancement, so progress no longer depends on seeing that nullifier again. The trigger-based regression verifies durable settlement after the fault is removed.
| fn tip_recipient_label(username: &str) -> String { | ||
| let name = username.trim(); | ||
| let label = match name.rfind('.') { | ||
| Some(dot) if name[dot..].eq_ignore_ascii_case(".dash") => &name[..dot], | ||
| _ => name, | ||
| }; | ||
| dash_sdk::platform::dpns_usernames::convert_to_homograph_safe_chars(label) |
There was a problem hiding this comment.
✅ Resolved at
0dfe1513; see the replies below.
🟡 Suggestion: Share canonical DPNS normalization with recipient resolution
tip_recipient_label independently reproduces extract_dpns_label plus normalize_dpns_label from rs-sdk/src/platform/dpns_usernames/mod.rs, with an additional trim that the resolver does not perform. Recipient confirmations and the rotated-address pending check depend on this key continuing to identify exactly the name used by resolution. The implementations agree for ordinary supported names today, so this is not an established payment-routing defect, but maintaining a second canonicalization implementation creates an avoidable synchronization requirement in a safety-sensitive path. Expose a shared canonical helper from the SDK or its transport-free DPNS owner and use it for resolution and recipient-history keys, with one explicit whitespace policy.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 84cfd41. normalize_dpns_label in rs-sdk/src/platform/dpns_usernames is now public, and tip_recipient_label is normalize_dpns_label(username.trim()). That is the function resolve_dpns_name uses, so the key cannot drift from resolution. Whitespace policy: the wallet's review and send trim the username before resolving, and the SDK normalization keeps surrounding whitespace (stated in its doc). Existing keys are unchanged.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 0dfe1513): Your recipient-history key now delegates to the SDK's public normalize_dpns_label, which resolution also uses. Review and send both trim before resolution, preserving one explicit whitespace policy.
| if let Some(persister) = persister { | ||
| persister | ||
| .store(PlatformWalletChangeSet { | ||
| shielded: Some(changeset_for_entry(id, entry.clone())), | ||
| ..Default::default() | ||
| }) | ||
| .and_then(|()| persister.flush()) | ||
| .map_err(|e| PlatformWalletError::Persistence(e.to_string()))?; |
There was a problem hiding this comment.
✅ Resolved at
0dfe1513; see the replies below.
🟡 Suggestion: Exercise failures in the guarded host-persistence gate
Every arm_parked_spend_guard invocation in parked_spend_guard_tests passes persister: None, so none exercises the new fallible store-and-flush branch. This branch distinguishes refusing a payment before broadcast from merely logging an activity-write failure. Add deterministic fake-persister cases for a failed store with an otherwise successful flush implementation, and for a successful store followed by a failed flush. Assert the Persistence error, that flush is not called after store fails, and that neither a tip barrier nor a recovery record is armed. These tests require neither proving nor a live network.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Added in 84cfd41: should_refuse_a_tip_whose_activity_row_cannot_be_persisted runs arm_parked_spend_guard against a fake persister twice. Once the store fails, and flush is never called. Once the store succeeds and the flush fails. Each case asserts a Persistence error and that no barrier or recovery record was armed.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 0dfe1513): Your fake-persister regression exercises both store failure and successful store followed by flush failure, asserting Persistence and no armed barrier or recovery record. It also verifies that a failed store prevents flush.
…balances from the ledger Review of the tip screens found two decisions still made in the apps: which shielded accounts to request when rebinding after identity discovery (Swift and Kotlin differed, and Kotlin dropped ordinary accounts other than 0), and an identity's tip balance, which the apps summed from persisted notes and so showed as zero after a shielded Clear. - refresh_shielded_accounts rebinds the ordinary accounts already bound or persisted (account 0 when none) and adds new identities' tip accounts, under the account-set lock; persisted viewing keys first, the seed only when they do not cover the request. - identity_tip_balance reports Unverified, Unavailable (no established ledger) or Available credits, from slot verification and the coordinator's ledger state. - Review and send trim the username, so both apps resolve the same names. Both are exposed through C, JNI, Swift and Kotlin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and never wait behind a scan identity_tip_balance read a bound account's ledger as Available once the wallet was hydrated, so a tip account bound after a restore showed zero until its first scan. An account with no restored or scanned history is now Unavailable. The FFI read gets the same 100 ms store budget as the local balance snapshot and fails as busy instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The independent durable barriers improve restart safety, but guarded-tip recovery still releases inputs on unauthenticated rejection, Clear can overtake an admitted send, and the new balance API can combine stale identity placement with a newer ledger. Static verification at c293c94 revalidated all 21 prior findings: six remain valid and 15 are fixed; two additional findings are confirmed. Current PR checks report the Kotlin SDK build and tests passing; no builds or tests were run locally.
🔴 3 blocking | 🟡 5 suggestion(s)
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/tips.rs and barrier persistence in file_store.rs directly alter funds-movement safety, restart-safe note reservations, and durable storage. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/tips.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:515-524: Keep identity placement stable through the tip-balance snapshot
identity_tip_account releases the wallet-manager read lock before local_balance_snapshot awaits the coordinator's lifecycle/store locks, and this method does not hold identity_placement. While the snapshot waits, apply, load, or discovery can move the identity to another slot; applying a placeholder can also clear its verified flag. The later ledger is nevertheless returned as Available for the previously selected account. If that ledger changes after the move, the returned ownership and balance states never existed together, potentially attributing a retired or another identity's account to this identity as a verified balance. Hold the existing placement read guard across account selection and ledger acquisition, or revalidate placement and verification before returning. Add a deterministic snapshot-wait, identity re-slot, and ledger-update regression.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:223-230: Share canonical DPNS normalization with recipient resolution
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437608308)
tip_recipient_label independently reproduces the optional case-insensitive .dash suffix handling and homograph normalization owned by rs-sdk's extract_dpns_label/normalize_dpns_label, while recipient resolution uses the SDK implementation. Review and send trim before resolving, so the current supported inputs agree; this is not a demonstrated routing defect. However, the normalized value is now a persistent recipient-history key used for change warnings and the previous-address pending check. A future resolver normalization change can split that history unless both copies change together. Expose and reuse the canonical SDK helper, or return the canonical label with resolution, with one explicit whitespace policy.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift:1819: Reject embedded NUL in the new Swift recipient-review binding
Swift String can contain U+0000, but platform_wallet_manager_review_shielded_tip reads username with CStr::from_ptr. Reviewing "alice\u0000bob" therefore resolves and reviews only "alice", rather than the complete supplied name. The JNI conversion rejects this input through CString::new, and the Swift amount wrapper already handles the same representation problem. Reject embedded NUL before the null-terminated conversion, and check the corresponding username conversion in send as well. This preserves the Rust API's input without duplicating DPNS validation in Swift.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3243-3245: Preserve guarded-tip outcome semantics through re-drive
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437608298)
Resetting the parked record's attempts admits the unresolved tip to redrive_pending_spends. Its classify_redrive_broadcast still treats consensus-coded remote rejection as definitive, and the rejection arm calls clear_pending for the inputs, deletes recovery state, and writes Failed activity. The SDK deserializes that rejection metadata without authenticating it; neither a fabricated rejection nor an honest lagging endpoint establishes that an earlier delivery cannot execute. Keeping the independent barrier Pending prevents another tip to the same recipient, but does not preserve the reservations: balance reads and other spending paths can now treat the at-risk notes as available. Recognize tip-backed recovery records and preserve their reservations, recovery work, and unresolved activity on unauthenticated rejection. Add an ambiguous guarded send followed by a consensus-coded re-drive rejection regression that checks all three records and attempts spending the inputs through another payment.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3124-3133: Exercise failures in the guarded host-persistence gate
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437608311)
The fallible store-then-flush gate now correctly precedes barrier creation, but every arm_parked_spend_guard invocation in parked_spend_guard_tests passes persister: None. These tests therefore do not exercise the durability boundary that must refuse a payment before broadcast. Add a recording persister covering a failed store whose flush would succeed, a successful store followed by a failed flush, and successful ordering. Assert the Persistence error, that store failure skips flush, and that neither failure installs a tip barrier or recovery record. These cases require neither proving nor a live network.
In `packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1244-1249: Protect unresolved tip guards from account purge and Clear
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437608296)
This check protects only barriers already installed. transfer_with releases the store guard after extracting witnesses, then constructs the proof before arm_parked_spend_guard records the barrier. Clear can complete in that interval: its manager and coordinator checks see no pending tip, so it purges account state and registrations and stops background sync. The sender holds shielded_config_lock, but Clear does not take that lock, and the sender neither holds coordinator lifecycle admission nor validates clear_generation before recreating activity/barrier/recovery records and broadcasting. Although the retained barrier prevents the original duplicate-recipient bypass, a successful Clear can still be followed by a payment and persistence writes behind the host's authorized wipe, with the recovery account no longer registered. Serialize Clear with send admission through barrier installation, or invalidate the send before broadcast when Clear changes the generation. Test manager.clear_shielded with a send paused after witness extraction.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:727-733: Preserve retry work when barrier settlement cannot be persisted
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437608305)
A failed Executed-status write is logged, but mark_spent then marks the note spent, removes its recovery record, and returns success. apply_scanned_nullifier_spends therefore succeeds and sync advances the watermark beyond the confirming action. Leaving the barrier Pending is correctly fail-closed, but a transient write failure can strand an already-executed tip for the remainder of the session: later scans need not encounter that nullifier again, and the recovery record that could prompt work has gone. The recipient, paying account, and Clear remain blocked despite execution having been observed. Propagate the settlement error before consuming the note/recovery work, or retain explicit settlement-retry work. Add fault injection demonstrating settlement after storage becomes writable without requiring user abandonment or restoration.
In `packages/rs-platform-wallet/src/wallet/shielded/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/store.rs:174-186: Index tip barriers by nullifier on the scan hot path
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437608302)
tip_barriers_spending traverses the complete cross-wallet barrier map before filtering by wallet, status, and nullifier membership. Both store implementations call it from mark_spent, and apply_scanned_nullifier_spends invokes mark_spent for every scanned network nullifier against every bound subwallet, including unrelated and padding nullifiers. Settled rows remain for distinct recipients, so historical tipping activity adds scan work proportional to scanned actions × bound subwallets × retained barriers under the shared store write lock. Maintain a store-owned reverse lookup keyed by wallet/nullifier for barriers that can still transition to Executed, rebuilding it on load and updating it with insertion, settlement, replacement, and purge.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bind shielded-transfer execution evidence to the submitted outputs — The ShieldedTransfer branch in packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs verifies spent nullifiers, not recipient outputs, while the execution-proof classifier returns true for this transition family. Competing signed transfers with the same complete nullifier set but different destinations therefore share the verified evidence. The same implementation exists on the PR base e8e518f; this is a concrete pre-existing proof-guarantee limitation, separate from the guarded re-drive reservation release reported here.
- Follow-up: Track output-binding execution evidence or an explicitly affected-state-only client workflow separately, respecting shipped-generation versioning constraints.
| let tip = self.identity_tip_account(identity_id).await?; | ||
| if !tip.verified { | ||
| return Ok(IdentityTipBalance::Unverified); | ||
| } | ||
| let ledger = coordinator.local_balance_snapshot(self.wallet_id()).await?; | ||
| let balance = match ledger { | ||
| ShieldedLocalBalanceState::Ready(snapshot) => snapshot | ||
| .accounts | ||
| .get(&tip.account) | ||
| .filter(|account| account.source != ShieldedBalanceSource::NoHistory) |
There was a problem hiding this comment.
✅ Resolved at
0dfe1513; see the replies below.
🔴 Blocking: Keep identity placement stable through the tip-balance snapshot
identity_tip_account releases the wallet-manager read lock before local_balance_snapshot awaits the coordinator's lifecycle/store locks, and this method does not hold identity_placement. While the snapshot waits, apply, load, or discovery can move the identity to another slot; applying a placeholder can also clear its verified flag. The later ledger is nevertheless returned as Available for the previously selected account. If that ledger changes after the move, the returned ownership and balance states never existed together, potentially attributing a retired or another identity's account to this identity as a verified balance. Hold the existing placement read guard across account selection and ledger acquisition, or revalidate placement and verification before returning. Add a deterministic snapshot-wait, identity re-slot, and ledger-update regression.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 0dfe151. identity_tip_balance now holds the identity_placement read guard across both choosing the account and reading the ledger, so discovery, load or apply cannot re-slot the identity, or clear its verification, in between. identity_tip_account takes no placement lock itself, so the read is not nested. Test: should_hold_identity_placement_through_a_tip_balance_read holds the store writer so the ledger read waits. It then checks that a placement write is blocked during that wait and becomes available once the read returns.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 0dfe1513): Your placement guard now covers both account selection and the awaited ledger snapshot, without nesting a placement read inside identity_tip_account. The added contention regression checks that placement remains excluded until the balance read returns.
| return try await Task.detached(priority: .userInitiated) { | ||
| var review = ShieldedTipReviewFFI() | ||
| try walletId.withUnsafeBytes { wallet in | ||
| try username.withCString { name in |
There was a problem hiding this comment.
✅ Resolved at
0dfe1513; see the replies below.
🟡 Suggestion: Reject embedded NUL in the new Swift recipient-review binding
Swift String can contain U+0000, but platform_wallet_manager_review_shielded_tip reads username with CStr::from_ptr. Reviewing "alice^@bob" therefore resolves and reviews only "alice", rather than the complete supplied name. The JNI conversion rejects this input through CString::new, and the Swift amount wrapper already handles the same representation problem. Reject embedded NUL before the null-terminated conversion, and check the corresponding username conversion in send as well. This preserves the Rust API's input without duplicating DPNS validation in Swift.
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
There was a problem hiding this comment.
There was a problem hiding this comment.
Resolved (re-reviewed at 0dfe1513): Your current review and resolve wrappers reject embedded NUL before withCString, and send rejects it before native admission as a notSent ShieldedTipFailure. The C boundary no longer receives only a username prefix.
…se a tip on a re-drive rejection A tip send is now admitted under the coordinator's lifecycle lock before it selects notes. Clear, and a bind dropping or re-keying the wallet's accounts, is refused while one is admitted, so a Clear can no longer land before the tip's barrier exists and purge what it relies on. The sync re-drive treated a node's consensus rejection of a re-broadcast tip as definitive: it released the notes, dropped the recovery record and marked the activity failed. A rejection is unauthenticated, so for a tip whose barrier is unsettled it now counts as inconclusive. Also: - a tip settlement whose write fails is retried by later mark_spent calls instead of leaving the barrier pending; - the recipient-history key uses the SDK's normalize_dpns_label; - Swift refuses a username with an embedded NUL, as JNI does; - tests cover a host that cannot store or flush the tip's activity row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
identity_tip_balance chose the identity's account and then waited for the ledger without the placement lock, so a discovery or load could move the identity in between and the balance would describe an account it no longer held. It now holds the placement read guard across both steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Three blocking issues remain at 0dfe151: a bind/send/removal deadlock, loss of conservative recovery handling after replacing an abandoned tip, and post-removal persistence writes through stale wallet handles. All 23 prior findings were independently reconciled: 21 are fixed, one remains valid, and the indexing suggestion is withdrawn. This was static verification only; current CI confirms Kotlin SDK build/tests pass, but provides no Rust wallet or Swift build/test result.
🔴 3 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/tips.rs and recovery and settlement logic in operations.rs and file_store.rs directly change funds-movement safeguards, note reservations, and durable payment state. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:975-986: Drain admitted sends without holding the lifecycle lock
This drain can deadlock with an admitted tip and a concurrent bind. A bind holds shielded_config_lock and passes its initial attachment check before loading persistence; a tip then obtains admission at tips.rs:626 and waits for that configuration lock. If unregister_wallet acquires lifecycle before the bind reaches install_shielded_views, removal waits here for the tip, the tip waits for the bind, and the bind waits for begin_install's lifecycle lock at platform_wallet.rs:1141. The installation's detached check cannot break the cycle because it runs after acquiring lifecycle. This also blocks other wallets' lifecycle operations on the shared coordinator. Close admission under lifecycle, drain admitted sends without retaining that mutex, then reacquire it for the purge, or establish a consistent acquisition order across all three paths. Add a deterministic bind → admitted-send → removal regression; the existing removal test drops a standalone admission externally and cannot exercise this cycle.
In `packages/rs-platform-wallet/src/wallet/shielded/tips.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:624-632: Reject detached wallet handles during tip admission
Admission never checks whether this wallet generation has already been removed. The C send resolves an Arc<PlatformWallet> before invoking the potentially blocking mnemonic resolver, so removal can finish before this call reaches admission; a Rust caller can also retain the removed handle. For an ordinary Account source, tip_source_account returns the account without checking membership, and recipient resolution uses only network queries. The send then calls confirm_tip_recipient and durably recreates recipient history for the deleted wallet before note selection fails against the purged ledger. Thus successful wallet removal can be followed by new persisted state from its stale handle. Check the detached/generation invariant in the Rust admission path, serialized with unregister and before any store mutation, rather than adding an FFI-only preflight. Add a retained-handle regression that resumes after removal and verifies that recipient and barrier tables stay empty.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3320-3327: Preserve guarded-tip outcome semantics through re-drive
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437608298)
The new classifier protects Pending and Abandoned barriers only while their rows remain present. After an ambiguous tip is abandoned, its recovery record and reservations remain armed. A replacement tip to the same address from other notes calls record_tip_barrier, which deletes every non-Pending predecessor for that recipient in both stores, including the old Abandoned barrier. With its anchor retained and attempt budget remaining, the old recovery operation now returns false here and an unauthenticated consensus-coded rejection enters DefinitiveRejection, releases its reservations, deletes its recovery record, and reports Failed. Acknowledgement permits another payment; it does not prove that the original payment failed. Preserve guarded-tip provenance for the recovery record's lifetime, either on the recovery record itself or by retaining its identifying barrier until recovery resolves. Extend the helper regression with arm → abandon → record replacement → classify old rejection; this sequence does not require a broadcast-error-capable mock.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Apply authenticated-outcome handling to ordinary shielded spends — The inherited non-tip broadcast and recovery paths still treat decoded remote consensus errors as definitive failure. SDK error conversion deserializes those claims without authenticating them, so an endpoint that forwards a payment and fabricates a rejection can cause reservations to be released and potentially executing activity to be reported as failed. This policy exists in the supplied base; the in-scope finding above concerns the new tip path losing its explicitly stronger policy.
- Follow-up: Track a separate security follow-up for conservative outcome handling across ordinary shielded broadcasts, result waits, and recovery, including forwarded-request/fabricated-rejection coverage.
| let _lifecycle = self.lifecycle.lock().await; | ||
| // Wait out the wallet's admitted tip sends (none is admitted while | ||
| // `lifecycle` is held): one finishing after the purge would record a | ||
| // barrier nothing can settle, blocking Clear for good. | ||
| loop { | ||
| let ended = self.tip_send_ended.notified(); | ||
| tokio::pin!(ended); | ||
| ended.as_mut().enable(); | ||
| if !self.tip_send_in_flight(Some(wallet_id)) { | ||
| break; | ||
| } | ||
| ended.await; |
There was a problem hiding this comment.
✅ Resolved at
ca92a584; see the replies below.
🔴 Blocking: Drain admitted sends without holding the lifecycle lock
This drain can deadlock with an admitted tip and a concurrent bind. A bind holds shielded_config_lock and passes its initial attachment check before loading persistence; a tip then obtains admission at tips.rs:626 and waits for that configuration lock. If unregister_wallet acquires lifecycle before the bind reaches install_shielded_views, removal waits here for the tip, the tip waits for the bind, and the bind waits for begin_install's lifecycle lock at platform_wallet.rs:1141. The installation's detached check cannot break the cycle because it runs after acquiring lifecycle. This also blocks other wallets' lifecycle operations on the shared coordinator. Close admission under lifecycle, drain admitted sends without retaining that mutex, then reacquire it for the purge, or establish a consistent acquisition order across all three paths. Add a deterministic bind → admitted-send → removal regression; the existing removal test drops a standalone admission externally and cannot exercise this cycle.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in ca92a58. unregister_wallet now waits for the wallet's admitted sends (wait_for_tip_sends) without holding lifecycle. It then takes lifecycle and checks again, looping if a send was admitted just before the handle was marked detached. The purge runs under lifecycle once no send is admitted. A bind waiting for lifecycle can therefore finish and release its config lock to the waiting send. Admissions after the detach are refused, so the loop ends. Test: should_let_a_bind_proceed_while_removal_drains_a_tip_send pauses an admitted send and starts the removal. It checks that begin_install still gets lifecycle while removal drains (this fails with the old code), then that removal finishes once the send ends.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at ca92a584): Your unregister loop now drains admitted sends without holding lifecycle, then acquires lifecycle and rechecks before purging, breaking the reported bind/send/removal cycle. The added regression checks that begin_install can acquire lifecycle while removal waits.
| // Clear is refused from here until the send returns; afterwards an | ||
| // unresolved tip's barrier keeps refusing it. | ||
| let _admission = coordinator.admit_tip_send(self.wallet_id()).await; | ||
| #[cfg(test)] | ||
| test_hooks::reach(self, test_hooks::Point::AfterTipAdmission).await; | ||
| let _placement = self.identity.identity_placement.read().await; | ||
| // A bind cannot drop or re-key the paying account while its tip is | ||
| // between note selection and recording its barrier. | ||
| let _config = self.shielded_config_lock.lock().await; |
There was a problem hiding this comment.
✅ Resolved at
ca92a584; see the replies below.
🔴 Blocking: Reject detached wallet handles during tip admission
Admission never checks whether this wallet generation has already been removed. The C send resolves an Arc before invoking the potentially blocking mnemonic resolver, so removal can finish before this call reaches admission; a Rust caller can also retain the removed handle. For an ordinary Account source, tip_source_account returns the account without checking membership, and recipient resolution uses only network queries. The send then calls confirm_tip_recipient and durably recreates recipient history for the deleted wallet before note selection fails against the purged ledger. Thus successful wallet removal can be followed by new persisted state from its stale handle. Check the detached/generation invariant in the Rust admission path, serialized with unregister and before any store mutation, rather than adding an FFI-only preflight. Add a retained-handle regression that resumes after removal and verifies that recipient and barrier tables stay empty.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in ca92a58. admit_tip_send now takes the handle's shielded_detached flag and refuses (WalletNotFound) under lifecycle once it is set. Removal sets the flag before unregister_wallet, so the outcome is one of two: removal sees the admission and waits for it, or the send sees the removal and stops before resolving the recipient or touching the store. Test: should_refuse_a_tip_from_a_removed_wallet_handle sends from a removed handle. It asserts WalletNotFound, and that the barrier and recipient tables stay empty.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at ca92a584): Your admission path now checks shielded_detached under lifecycle before recipient resolution or persistence, and removal detaches before draining and purging. The retained-handle regression checks WalletNotFound and empty recipient/barrier state; the separate abandonment mutation is reported independently.
…use removed handles, keep abandoned barriers Wallet removal waited for admitted tip sends while holding the coordinator's lifecycle lock. A send waiting for a bind's config lock, with that bind waiting for lifecycle, deadlocked all three. Removal now drains without the lock, then takes it and re-checks before purging. Admission now refuses a wallet handle already removed from the manager, checked under the lifecycle lock, so a stale handle cannot write recipient history or a barrier after the purge. A replacement tip deleted its recipient's abandoned barrier, so the old tip's re-drive lost its guarded-tip handling. Only executed and failed barriers are pruned now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the combined reviewer evidence against ca92a58 and confirmed two blockers: detached-handle abandonment can release a replacement wallet's barriers, and damaged-barrier repair can remove an active recovery operation's conservative rejection policy. Of the 25 prior findings, 23 are fixed, one remains valid, and one remains withdrawn; validation was static, with no local builds or tests run. Current CI shows Kotlin passing, wallet tests and the Swift SDK build skipped, and Rust workspace tests failing in the unchanged historical TokenHistory activation test (Some(8000) versus Some(108)), which appears unrelated to this wallet-only change.
🔴 2 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 11: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 14: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate changes to send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/operations.rs and durable tip barriers in file_store.rs directly alter funds movement, note reservations, and restart-safe payment settlement. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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/tips.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/tips.rs:588-596: Fence tip abandonment against detached wallet generations
This mutation locks only the handle's tip_send_in_progress mutex, then changes persisted barriers by wallet ID without checking shielded_detached or entering the coordinator's lifecycle boundary. Retain handle G1, remove its wallet, recreate the same seed/network as G2, and record a Pending tip through G2. Abandonment through G1 now marks G2's barrier Abandoned. The replacement has a new send mutex, so G1 can also release that barrier while G2 is actively sending, contrary to this method's contract. A delayed acknowledgement for a removed generation can therefore remove a newer payment's duplicate-send protection. Serialize abandonment with wallet removal in rs-platform-wallet, validate attachment under that serialization, and retain protection through the store mutation while preserving send exclusion. Add a retained-G1 → remove → recreate-G2 regression asserting WalletNotFound from G1 abandonment and an unchanged Pending barrier for G2; an FFI-only preflight would not protect native Rust callers.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:3320-3327: Preserve guarded-tip outcome semantics through re-drive
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4437608298)
Retaining readable Abandoned barriers fixes the replacement sequence in your latest reply, but acknowledged corruption repair still removes the classification while leaving valid recovery armed. For an ambiguous tip with attempts remaining, damage only its barrier's account_index to -1 and reopen. rehydrate_pending_spends restores the valid recovery operation and reservations, while load_tip_barriers rejects the barrier and initially makes this lookup fail closed. abandon_tip_barriers then deletes that rejected row and clears the wallet's damage entry at file_store.rs:1231–1241 without changing the recovery operation. This lookup subsequently returns false. If the anchor remains recorded, a consensus-coded re-drive rejection enters DefinitiveRejection, releases the reservations, removes recovery, and records Failed activity even though the original payment may execute. Acknowledgement permits replacement payments; it does not authenticate a later rejection or prove the original inputs spendable. Preserve durable guarded-tip provenance independently of the repairable barrier, or retain equivalent activity-keyed protection during repair. Add a valid-recovery → damaged-barrier → reopen → acknowledge-repair regression verifying that the retained operation still receives Inconclusive rejection handling.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Apply conservative outcome handling to ordinary shielded spends — The pre-existing non-tip broadcast_shielded_spend and classify_spend_wait_failure paths still treat unauthenticated consensus-error metadata as definitive failure. A hostile endpoint can relay a valid payment and fabricate rejection, causing reservation release and a Failed result despite possible execution. This behavior exists on the PR's base and is not introduced by the guarded-tip implementation.
- Follow-up: Track a separate security fix for ordinary shielded spend submission and recovery, covering fabricated broadcast metadata and wait-error envelopes without expanding this PR.
| let _sending = self | ||
| .tip_send_in_progress | ||
| .try_lock() | ||
| .map_err(|_| PlatformWalletError::ShieldedTipPending)?; | ||
| coordinator | ||
| .store() | ||
| .write() | ||
| .await | ||
| .abandon_tip_barriers(self.wallet_id()) |
There was a problem hiding this comment.
✅ Resolved at
bc99e1e8; see the replies below.
🔴 Blocking: Fence tip abandonment against detached wallet generations
This mutation locks only the handle's tip_send_in_progress mutex, then changes persisted barriers by wallet ID without checking shielded_detached or entering the coordinator's lifecycle boundary. Retain handle G1, remove its wallet, recreate the same seed/network as G2, and record a Pending tip through G2. Abandonment through G1 now marks G2's barrier Abandoned. The replacement has a new send mutex, so G1 can also release that barrier while G2 is actively sending, contrary to this method's contract. A delayed acknowledgement for a removed generation can therefore remove a newer payment's duplicate-send protection. Serialize abandonment with wallet removal in rs-platform-wallet, validate attachment under that serialization, and retain protection through the store mutation while preserving send exclusion. Add a retained-G1 → remove → recreate-G2 regression asserting WalletNotFound from G1 abandonment and an unchanged Pending barrier for G2; an FFI-only preflight would not protect native Rust callers.
source: gpt-6-astra (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 9fb9160. abandon_unresolved_shielded_tips now goes through the same admission as a send: admit_tip_send(wallet_id, &shielded_detached) under lifecycle, held through the store mutation, after the handle's send lock. A removed handle is refused with WalletNotFound. Removal drains an abandonment already admitted, so it can't touch the barriers of a wallet recreated after that removal. The check is in rs-platform-wallet, so native Rust callers are covered as well as the FFI. Test: should_refuse_abandonment_from_a_removed_wallet_handle removes the wallet and records a pending barrier for the same wallet id. It asserts that the old handle's abandonment fails with WalletNotFound and the barrier stays pending.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at bc99e1e8): Your abandonment path now takes attachment-checked coordinator admission after the send mutex and retains it through the store mutation, so removal drains it before the wallet ID can be reused. The retained-handle regression verifies WalletNotFound and leaves the replacement's Pending barrier unchanged.
…and keep repaired tips guarded Abandoning unresolved tips took only the handle's send lock, so a handle kept past its wallet's removal could release a recreated wallet's pending barrier. Abandonment is now admitted like a send: refused for a detached handle under the lifecycle lock, and drained by removal. Repairing damaged barriers deleted the unreadable rows, so a still-armed recovery operation lost its guarded-tip handling and a later re-drive rejection could release its notes. Before deleting them, every unclaimed note-spending recovery operation of the wallet now gets an abandoned stand-in barrier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tor, and bind tip-account classification for Kotlin The Swift refreshShieldedAccounts blocked the main actor while the bind waited for a shielded scan to release the store. It is now async: the native call runs detached, admitted like the other tip calls, and local-balance reads delivered meanwhile are marked obsolete. Kotlin gains PlatformWalletManager.isShieldedTipAccount over the existing C export, so screens can exclude tip accounts without hardcoding the range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…companion It landed on PlatformWalletPersistenceCapabilities' companion, so the example app could not resolve PlatformWalletManager.isShieldedTipAccount. The manager's companion is now public with its constants kept private. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified two outstanding issues at bc99e1e: destructive rebinding can discard unresolved-tip reservations when barrier reads fail, and Kotlin's standalone account classifier can enter JNI before loading the library. The 26 prior findings reconcile as 25 fixed and one remaining withdrawn. This was static verification only; the supplied CI snapshot shows Kotlin passing, Rust workspace tests pending and the dedicated wallet job skipped, while the current checks query reports no failures.
🔴 1 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to shielded/operations.rs, shielded/tips.rs and shielded/file_store.rs directly alter funds-movement safety through durable payment barriers, note reservations, retry decisions and concurrent send lifecycle controls. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:793-800: Preserve pending-account ownership when barrier reads fail
A failed tip_barriers read does not mean there are no pending owners. FileBackedShieldedStore returns Err for the entire wallet when any of its barriers is damaged, or globally when a barrier has an unreadable wallet ID, suppressing otherwise readable Pending barriers too. With a valid unresolved tip and recovery record on account 0, that condition lets a rebind from [0, 1] to [1] bypass this pinning check. The following recovery check explicitly skips nullifier-backed records, and purge_subwallet(0) then deletes the durable recovery row and in-memory reservations. Rebinding account 0 can restore the host's unspent notes without those reservations, making potentially spent inputs available to ordinary balance and spending paths. The independent barrier still blocks another tip, but does not preserve the inputs or recovery. Reject destructive binds when ownership is unreadable, or expose readable ownership separately without interpreting a failed aggregate as empty. Add a regression combining a damaged barrier with a valid pending tip and assert that dropping its account is refused and its recovery record survives.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:2930: Initialize the native library before the static account classifier
This companion method is callable before constructing an Sdk or wallet manager, but immediately invokes JNI. Neither the companion nor FundingNative initializes NativeLoader, so a first SDK call to PlatformWalletManager.isShieldedTipAccount(0) throws UnsatisfiedLinkError even with a correctly bundled shielded library. Initialize the library before entering FundingNative, following Sdk.hasShielded() and Sdk.version(), and cover this helper as the first native call in a fresh-process instrumented test.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Extend conservative remote-outcome handling to ordinary shielded spends — The base revision's ordinary shielded broadcast and wait paths already treat unauthenticated consensus-error claims as definitive and can release reservations; the non-tip re-drive branch preserves that existing policy. A dishonest endpoint could relay a payment and fabricate a rejection, allowing a caller to retry the payment with different notes. This PR removes tips from that unsafe policy without introducing or worsening the ordinary-send exposure; the remaining non-tip behavior warrants a separate security follow-up.
- Follow-up: Track a separate issue or maintainer-requested PR applying conservative unresolved semantics to ordinary shielded sends and recovery, with fabricated broadcast and wait-error coverage.
| // A pending tip settles only through its paying account's scan | ||
| // and prune passes, so that account must stay bound, with its keys. | ||
| // Unreadable barriers cannot settle that way and keep blocking | ||
| // tips and Clear on their own, so they do not block the bind. | ||
| let tips = store.tip_barriers(wallet_id).unwrap_or_default(); | ||
| for tip in tips | ||
| .iter() | ||
| .filter(|tip| tip.status == super::store::TipBarrierStatus::Pending) |
There was a problem hiding this comment.
✅ Resolved at
ffcabad7; see the replies below.
🔴 Blocking: Preserve pending-account ownership when barrier reads fail
A failed tip_barriers read does not mean there are no pending owners. FileBackedShieldedStore returns Err for the entire wallet when any of its barriers is damaged, or globally when a barrier has an unreadable wallet ID, suppressing otherwise readable Pending barriers too. With a valid unresolved tip and recovery record on account 0, that condition lets a rebind from [0, 1] to [1] bypass this pinning check. The following recovery check explicitly skips nullifier-backed records, and purge_subwallet(0) then deletes the durable recovery row and in-memory reservations. Rebinding account 0 can restore the host's unspent notes without those reservations, making potentially spent inputs available to ordinary balance and spending paths. The independent barrier still blocks another tip, but does not preserve the inputs or recovery. Reject destructive binds when ownership is unreadable, or expose readable ownership separately without interpreting a failed aggregate as empty. Add a regression combining a damaged barrier with a valid pending tip and assert that dropping its account is refused and its recovery record survives.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in the commit on top of bc99e1e. register_locked no longer reads a failed tip_barriers lookup as empty. While the wallet's barriers are unreadable, a bind that drops or re-keys an account (stale non-empty) is refused with ShieldedTipPending. An initial registration purges nothing, so it still proceeds and the wallet can bind and sync after a restart. Abandoning the wallet's unresolved tips repairs the damage, which keeps the recovery operations marked as tips, and destructive binds then work again. One effect: barrier damage whose wallet id is unreadable now blocks account drops for every wallet, as it already blocks tips and Clear. Test: should_refuse_dropping_an_account_while_tip_barriers_are_unreadable arms a recovery and a readable pending barrier on account 0, marks the wallet's barriers unreadable, and checks that binding [1] is refused and the recovery survives.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at ffcabad7): Your new match rejects unreadable barriers whenever stale accounts would be purged, while permitting registration that purges nothing. The added regression combines readable pending recovery with barrier-read failure and verifies that dropping the paying account is refused without deleting recovery.
| * Whether [account] is a dedicated tip account, as Rust classifies it | ||
| * (Swift `PlatformWalletManager.isShieldedTipAccount`). Shielded builds only. | ||
| */ | ||
| fun isShieldedTipAccount(account: Int): Boolean = FundingNative.isShieldedTipAccount(account) |
There was a problem hiding this comment.
✅ Resolved at
ffcabad7; see the replies below.
🟡 Suggestion: Initialize the native library before the static account classifier
This companion method is callable before constructing an Sdk or wallet manager, but immediately invokes JNI. Neither the companion nor FundingNative initializes NativeLoader, so a first SDK call to PlatformWalletManager.isShieldedTipAccount(0) throws UnsatisfiedLinkError even with a correctly bundled shielded library. Initialize the library before entering FundingNative, following Sdk.hasShielded() and Sdk.version(), and cover this helper as the first native call in a fresh-process instrumented test.
| fun isShieldedTipAccount(account: Int): Boolean = FundingNative.isShieldedTipAccount(account) | |
| fun isShieldedTipAccount(account: Int): Boolean { | |
| Sdk.initialize() | |
| return FundingNative.isShieldedTipAccount(account) | |
| } |
source: gpt-6.1-sol (phase2-reviewer: ffi-engineer)
There was a problem hiding this comment.
Fixed in the same commit: PlatformWalletManager.isShieldedTipAccount now calls NativeLoader.ensureLoaded() before entering FundingNative, as Sdk.hasShielded() and Sdk.version() do. I haven't added an instrumented first-native-call test; the classifier is a single synchronous JNI call with no state, and the load is the same idempotent call those methods use.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at ffcabad7): Your companion classifier now calls NativeLoader.ensureLoaded() before entering FundingNative, matching the established static-helper initialization order. The omitted fresh-process regression does not leave the reported load-order defect present.
… unreadable register_locked read a failed barrier lookup as "no pending tips", so a damaged row let a bind drop the account paying a readable pending tip and purge its recovery and reservations. A bind that drops or re-keys an account is now refused while the wallet's barriers are unreadable; an initial registration, which purges nothing, still proceeds. Also load the native library before Kotlin's static PlatformWalletManager.isShieldedTipAccount calls JNI. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at ffcabad: 27 prior findings are fixed, and the withdrawn performance suggestion remains withdrawn. One blocking restart-state defect remains: durable execution settlements are not applied to restored input eligibility; two additional reliability suggestions are confirmed. Validation was static only: Kotlin CI passed, while the supplied snapshot showed Rust workspace tests and the JS build pending, with no later completion established by the current checks response.
🔴 1 blocking | 🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/tips.rs and durable barrier handling in file_store.rs directly change funds-movement safety, note reservations, and persisted storage across retries and restarts. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:505-507: Restore input exclusion from durable tip settlements
An Executed barrier is loaded only into tip_barriers, without applying its nullifiers to restored note eligibility. mark_spent durably records Executed and deletes the recovery row before mark_notes_spent queues the host's spent-state changeset; scan-driven confirmation also delivers that changeset later. If the process exits in that interval, reopening restores no recovery reservation, and restore_locked accepts the host's old is_spent=false notes. Both get_unspent_notes and spendable_balance then rely solely on those notes and in-memory reservations, ignoring the durable execution evidence. Consequently identity_tip_balance can report already-consumed inputs as Available, and spending can select them before reconciliation. This is part of the new restart-safe settlement and balance contract, not a binding-layer issue. Persist and restore known-spent or exclusion evidence in rs-platform-wallet, or retain durable exclusion until the host spent update is committed; ensure replacing a settled recipient barrier cannot erase exclusion still needed for recovery. Add an offline reopen regression interrupted after mark_spent but before host persistence, restore the old unspent snapshot, and verify that the inputs are neither counted nor selectable.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:994-997: Bound the pending-tip preflight before quiescing
This preflight can wait behind an entire shielded scan before reaching Clear's bounded drain path. With no send admitted, has_pending_tips awaits the store read lock, while sync_notes_across holds the writer across network streaming and trial decryption. A cold scan can therefore delay Clear for minutes without reaching quiesce_held's 10-second drain budget. Use a nonblocking or bounded barrier check that returns an explicit busy error on contention, preserving the running recovery worker and never treating contention as an empty barrier set. Add a manager-level regression holding the store writer and verifying that Clear returns within a bounded interval without stopping the worker.
In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1955-1964: Retain retry work when a not-sent settlement write fails
A transient failure writing Failed leaves a Pending barrier with no automatic settlement work. For example, NoAvailableAddresses establishes that the tip was not broadcast, but this status update can fail after the barrier was committed. cancel_pending then removes its reservations and recovery record, and the call reports NotSent. Unlike failed execution settlements, this failure is not retained for a later retry. Once storage recovers, scans will not observe these never-spent nullifiers consumed, so the barrier continues blocking its recipient, paying-account removal, and Clear until explicit abandonment. Retain Failed-settlement retry work as well, and add failure-injection coverage proving that ordinary synchronization retries it after the storage fault is removed.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Audit unauthenticated rejection handling in ordinary shielded spends — The ordinary spend and recovery paths still classify decoded remote consensus errors as definitive rejection. The SDK decodes broadcast metadata and wait-error envelopes without authenticating those rejection claims, allowing a malicious relay to forward a payment and then cause reservations to be released by fabricating rejection. A replacement payment funded from different notes can pay twice. This policy is present at the base revision; the new guarded-tip path avoids it and does not introduce or worsen the ordinary-path behavior.
- Follow-up: Track a separate security issue covering ordinary shielded broadcast, wait, and recovery verdict handling, with relay-then-fabricated-rejection regression coverage.
| (Some(wallet_id), Some(barrier)) => { | ||
| self.tip_barriers | ||
| .insert((wallet_id, barrier.activity_id), barrier); |
There was a problem hiding this comment.
✅ Resolved at
1b300991; see the replies below.
🔴 Blocking: Restore input exclusion from durable tip settlements
An Executed barrier is loaded only into tip_barriers, without applying its nullifiers to restored note eligibility. mark_spent durably records Executed and deletes the recovery row before mark_notes_spent queues the host's spent-state changeset; scan-driven confirmation also delivers that changeset later. If the process exits in that interval, reopening restores no recovery reservation, and restore_locked accepts the host's old is_spent=false notes. Both get_unspent_notes and spendable_balance then rely solely on those notes and in-memory reservations, ignoring the durable execution evidence. Consequently identity_tip_balance can report already-consumed inputs as Available, and spending can select them before reconciliation. This is part of the new restart-safe settlement and balance contract, not a binding-layer issue. Persist and restore known-spent or exclusion evidence in rs-platform-wallet, or retain durable exclusion until the host spent update is committed; ensure replacing a settled recipient barrier cannot erase exclusion still needed for recovery. Add an offline reopen regression interrupted after mark_spent but before host persistence, restore the old unspent snapshot, and verify that the inputs are neither counted nor selectable.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in 21d3110. restore_locked now applies the durable evidence: after restoring a subwallet's host notes, it marks every note named by an executed barrier for that account as spent. It also queues those spent marks to the host once the store is released, so the host's rows catch up. Replacing a recipient's barrier now prunes only failed barriers, so an executed one stays as long as the wallet does and a later tip can't erase the exclusion. Test: an_executed_tip_keeps_its_notes_spent_across_a_stale_host_snapshot records an executed barrier and restores a snapshot where its note is still unspent. It then checks that the note is excluded from both the spendable balance and get_unspent_notes.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 21d3110e): Your new overlay and retention fix the readable-store case, but a sibling decoder-rejected row makes tip_barriers fail for the whole wallet and unwrap_or_default discards the readable Executed barrier too. The added regression does not include that combination; with a stale snapshot, restoration still establishes a ledger containing the consumed input without a recovery reservation.
There was a problem hiding this comment.
The remaining case is fixed in 1b30099. mark_spent now records each owned note it sees newly spent, plus any a barrier or recovery operation tracks, so B is recorded even after A settled the barrier and dropped the shared recovery operation. A recovery operation's durable row is now deleted only once every input it reserves is recorded. If the process stops between A and B, the row rehydrates and keeps B reserved, and no unobserved sibling is ever treated as spent. A row left by an input that is never seen spent (a competing payment) is released by the existing re-drive and prune passes. Test: a_two_input_tip_keeps_both_inputs_excluded_across_interruptions marks A, reopens and restores a stale snapshot, and checks that B is still reserved. It then marks B, checks that the row goes, and checks that a further stale restore still excludes both. The failed-tip test that leaves the unobserved sibling spendable still passes.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 1b300991): Your per-nullifier evidence and retained recovery row fix the direct interrupted-settlement/reopen sequence, including the interval between observing A and B. The added two-input regression covers that sequence; the separate account-rebinding path that deletes the retained row is reported above.
| if let Some(coord) = self.shielded_coordinator().await { | ||
| if coord.has_pending_tips().await? { | ||
| return Err(crate::error::PlatformWalletError::ShieldedTipPending); | ||
| } |
There was a problem hiding this comment.
✅ Resolved at
21d3110e; see the replies below.
🟡 Suggestion: Bound the pending-tip preflight before quiescing
This preflight can wait behind an entire shielded scan before reaching Clear's bounded drain path. With no send admitted, has_pending_tips awaits the store read lock, while sync_notes_across holds the writer across network streaming and trial decryption. A cold scan can therefore delay Clear for minutes without reaching quiesce_held's 10-second drain budget. Use a nonblocking or bounded barrier check that returns an explicit busy error on contention, preserving the running recovery worker and never treating contention as an empty barrier set. Add a manager-level regression holding the store writer and verifying that Clear returns within a bounded interval without stopping the worker.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Fixed in 21d3110. has_pending_tips waits at most 100 ms for the store read lock. If a scan holds the store past that, it returns a busy ShieldedStoreError and does not treat contention as an empty barrier set. clear_shielded returns that error before quiescing, so the worker keeps running. Test: the_pending_tip_check_fails_busy_behind_a_scan holds the store writer and checks that the call returns the error within its bound. The manager path is the unchanged ? before quiesce_and_clear_shielded.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 21d3110e): Your 100 ms timeout bounds store-lock acquisition and returns an explicit busy error rather than an empty barrier set. The manager propagates that error before quiescing, and the coordinator regression exercises the contended writer.
| if let (true, Some(entry)) = (guard_before_broadcast, &pending_entry) { | ||
| if let Err(e) = store.write().await.settle_tip_barrier( | ||
| wallet_id, | ||
| &entry.id, | ||
| TipBarrierStatus::Failed, | ||
| ) { | ||
| warn!(error = %e, "tip barrier failure not persisted; it stays pending"); | ||
| } | ||
| } | ||
| cancel_pending(store, id, &selected_notes).await; |
There was a problem hiding this comment.
✅ Resolved at
21d3110e; see the replies below.
🟡 Suggestion: Retain retry work when a not-sent settlement write fails
A transient failure writing Failed leaves a Pending barrier with no automatic settlement work. For example, NoAvailableAddresses establishes that the tip was not broadcast, but this status update can fail after the barrier was committed. cancel_pending then removes its reservations and recovery record, and the call reports NotSent. Unlike failed execution settlements, this failure is not retained for a later retry. Once storage recovers, scans will not observe these never-spent nullifiers consumed, so the barrier continues blocking its recipient, paying-account removal, and Clear until explicit abandonment. Retain Failed-settlement retry work as well, and add failure-injection coverage proving that ordinary synchronization retries it after the storage fault is removed.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Fixed in 21d3110. The file store's retry set now records the intended status (unsettled_tip_settlements, executed winning over failed). settle_tip_barrier adds a failed write to it, and each watermark advance retries it. Test: a_failed_not_sent_settlement_is_retried_until_it_persists fails the UPDATE with a trigger and checks that the barrier stays pending. It then removes the trigger, advances the watermark, and checks the barrier is durably failed after a reopen.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 21d3110e): Your status-aware retry map now retains unsuccessful Failed settlements, and watermark advancement retries them with Executed taking precedence. I verified the trigger-based regression checks both the pending state during failure and durable Failed status after reopening.
…le restore, and retry every settlement An executed barrier was not applied to a restored host snapshot, so a process stopping between mark_spent and the host persisting the spent marks brought the tip's notes back as spendable. Restore now marks every executed barrier's notes of the account spent and sends the host those spent marks. A replacement tip prunes only failed barriers, keeping executed ones as that evidence. A failed (not-sent) settlement whose write failed was never retried; the retry set now keeps both kinds, executed winning. Clear's pending-tip check waits at most 100 ms for the store and fails as busy instead of waiting out a scan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification at 21d3110 confirms 29 prior findings fixed, one still valid, and one remaining withdrawn. Two blocking restoration defects remain: unreadable barriers suppress valid spent-input evidence, and payment-level settlement can incorrectly mark unspent inputs as spent. Current CI checks pass, including Kotlin SDK build/tests; no Rust-wallet or Swift check results are reported, and no local builds or tests were run.
🔴 2 blocking
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/operations.rs and barrier persistence in file_store.rs directly alter funds-movement safety, note reservations, retry decisions, and durable settlement across crashes. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1230-1232: Restore input exclusion from durable tip settlements
The overlay fixes the clean-store case, but `unwrap_or_default()` discards readable execution evidence whenever another barrier is damaged. `FileBackedShieldedStore::tip_barriers` returns an error for the entire wallet when a sibling row is unreadable, and for every wallet when a row has an unreadable wallet ID. Reopen with a valid Executed barrier for note A, an unrelated damaged barrier, and a host snapshot predating A's spent update: initial registration is permitted, restoration inserts A as unspent, this lookup silently supplies no exclusions, and `restore_balance_metadata` establishes the ledger. The executed tip no longer has a recovery reservation, so both ordinary note selection and spendable balances can offer A. Blocking tips and Clear does not protect those consumers; on-chain rejection of a reused nullifier prevents execution but does not correct the reported funds or invalid spend construction. Preserve readable per-nullifier exclusion evidence independently of the aggregate admission lookup, or withhold restored balance/spending eligibility when that evidence is unavailable. Extend the stale-snapshot regression with an unrelated decoder-rejected barrier and assert that A is neither counted nor selectable.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1234-1244: Persist spent evidence per nullifier, not per tip barrier
An Executed barrier does not establish that every original input was consumed. `tip_barriers_spending` matches any single listed nullifier and includes Failed and Abandoned barriers; `mark_spent` promotes each match to Executed. For example, a tip selects A and B, reaches `NoAvailableAddresses`, settles Failed, and releases both reservations. A later ordinary payment spends only A, promoting the retained failed barrier to Executed. On restoration, this loop marks both A and the genuinely unspent B as spent and queues B's false spent flag to host persistence. This also occurs with a current host snapshot: restoring A's legitimate spent flag can promote the barrier immediately before this overlay reads it. Retained Executed barriers then repeat the false exclusion on subsequent restores, hiding real funds from balances and note selection. Persist individually observed spent nullifiers, or distinguish invalidation by a competing spend from proof that the original transfer executed; do not expand one observed spend into spent evidence for the complete input set. Add a two-input failed-tip followed by an ordinary single-input spend and reopen/restore regression that leaves the other input counted and selectable.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Extend conservative rejection handling to ordinary shielded payments — Ordinary shielded broadcast/wait handling and non-guarded re-drive still accept unauthenticated consensus-error metadata as definitive rejection. The SDK deserializes that metadata without authenticating non-execution, so a dishonest endpoint can relay a payment and fabricate a rejection that releases reservations and reports failure. These ordinary-payment decisions already exist at the PR base; the new guarded-tip path avoids them, so the broader trust-policy repair should be tracked separately.
- Follow-up: Track a separate issue or maintainer-requested PR applying authenticated-outcome or conservative unresolved handling to ordinary shielded payments and recovery.
| let executed: Vec<[u8; 32]> = store | ||
| .tip_barriers(wallet_id) | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
✅ Resolved at
c2b4ca92; see the replies below.
🔴 Blocking: Restore input exclusion from durable tip settlements
The overlay fixes the clean-store case, but unwrap_or_default() discards readable execution evidence whenever another barrier is damaged. FileBackedShieldedStore::tip_barriers returns an error for the entire wallet when a sibling row is unreadable, and for every wallet when a row has an unreadable wallet ID. Reopen with a valid Executed barrier for note A, an unrelated damaged barrier, and a host snapshot predating A's spent update: initial registration is permitted, restoration inserts A as unspent, this lookup silently supplies no exclusions, and restore_balance_metadata establishes the ledger. The executed tip no longer has a recovery reservation, so both ordinary note selection and spendable balances can offer A. Blocking tips and Clear does not protect those consumers; on-chain rejection of a reused nullifier prevents execution but does not correct the reported funds or invalid spend construction. Preserve readable per-nullifier exclusion evidence independently of the aggregate admission lookup, or withhold restored balance/spending eligibility when that evidence is unavailable. Extend the stale-snapshot regression with an unrelated decoder-rejected barrier and assert that A is neither counted nor selectable.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering, rust-quality)
There was a problem hiding this comment.
Fixed in c2b4ca9. The restore overlay no longer goes through the aggregate tip_barriers lookup. It reads a separate shielded_known_spent table (known_spent_nullifiers(id)), so a damaged or unattributed barrier row elsewhere can't hide the evidence, and a read error fails the restore instead of silently skipping exclusions. The regression an_observed_spend_stays_applied_across_a_stale_host_snapshot marks the wallet's barriers unreadable before the reopen and restore, then checks that the spent notes are neither counted nor selectable.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at c2b4ca92): Your independent known_spent_nullifiers lookup removes the aggregate tip_barriers decoding dependency, and the regression checks restoration while barrier ownership is unreadable. A failure reading the new evidence table exposes a separate partial-restore defect, reported as a new finding.
| .filter(|barrier| { | ||
| barrier.status == super::store::TipBarrierStatus::Executed | ||
| && barrier.account_index == id.account_index | ||
| }) | ||
| .flat_map(|barrier| barrier.nullifiers) | ||
| .collect(); | ||
| for nullifier in executed { | ||
| if store.mark_spent(*id, &nullifier).map_err(|e| { | ||
| crate::error::PlatformWalletError::ShieldedStoreError(e.to_string()) | ||
| })? { | ||
| executed_spends.record_nullifier_spent(*id, nullifier); |
There was a problem hiding this comment.
✅ Resolved at
c2b4ca92; see the replies below.
🔴 Blocking: Persist spent evidence per nullifier, not per tip barrier
An Executed barrier does not establish that every original input was consumed. tip_barriers_spending matches any single listed nullifier and includes Failed and Abandoned barriers; mark_spent promotes each match to Executed. For example, a tip selects A and B, reaches NoAvailableAddresses, settles Failed, and releases both reservations. A later ordinary payment spends only A, promoting the retained failed barrier to Executed. On restoration, this loop marks both A and the genuinely unspent B as spent and queues B's false spent flag to host persistence. This also occurs with a current host snapshot: restoring A's legitimate spent flag can promote the barrier immediately before this overlay reads it. Retained Executed barriers then repeat the false exclusion on subsequent restores, hiding real funds from balances and note selection. Persist individually observed spent nullifiers, or distinguish invalidation by a competing spend from proof that the original transfer executed; do not expand one observed spend into spent evidence for the complete input set. Add a two-input failed-tip followed by an ordinary single-input spend and reopen/restore regression that leaves the other input counted and selectable.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in c2b4ca9. Spent evidence is now per nullifier. When mark_spent sees a nullifier spent while a tip barrier or a recovery operation tracks it, it records that exact nullifier in shielded_known_spent before deleting the recovery row. If that write fails, the recovery row stays, so a restart still reserves the note. Restore marks only those nullifiers spent, never a barrier's whole input set. Executed barriers are no longer the evidence, so a replacement tip prunes them again. The regression covers your case: a two-input tip settles failed, a later scan sees only its first input spent, and an ordinary payment's recovery-tracked note is also spent. After a reopen with a stale snapshot, the first input and the payment's note are excluded, and the second input stays counted and selectable.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at c2b4ca92): Your restore now applies only individually recorded known-spent nullifiers, so observing A no longer falsely marks the unobserved B from a failed two-input tip spent. The added stale-snapshot regression checks that B remains counted and selectable.
…nput set The restore overlay read executed barriers through the aggregate barrier lookup, so one damaged row dropped all the evidence. It also marked every input of an executed barrier spent, although an executed barrier can come from a failed tip whose one input a later payment spent. mark_spent now records the exact nullifier in a new shielded_known_spent table whenever it matches a tip barrier or drops a recovery operation, and keeps the recovery rows if that write fails. Restore marks exactly those notes spent and sends the host the marks. A replacement tip again prunes executed barriers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the combined reviewer evidence and all 33 prior findings against c2b4ca9. Two blocking wallet-state defects remain: successful multi-input spends lose durable exclusion evidence after their first input, and a failed exclusion lookup exposes partially restored notes to generic balance and selection APIs. This was static review only; current CI confirms Kotlin build/test success but provides no Rust-wallet or Swift validation results.
🔴 2 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The extensive changes to send_shielded_tip in packages/rs-platform-wallet/src/wallet/shielded/tips.rs and barrier persistence in file_store.rs alter funds-movement safety, note reservations, settlement, and restart recovery. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/coordinator.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/coordinator.rs:1230-1232: Keep failed spent-evidence restores out of the spendable store
This error is returned after the preceding loop has already inserted the host's notes. Reopen after a confirmed spend of A with an old unspent host snapshot and a malformed sibling nullifier row in shielded_known_spent for the same account. known_spent_nullifiers collects and validates the entire result before returning, so the malformed row makes this lookup fail without applying even A's valid exclusion. The store lock is then released with A unspent and its recovery reservation already deleted.
The failed bind does not quarantine that state: install_shielded_views publishes shielded_keys before restoration and only clears hydration on failure. Hydration protects local_balance_snapshot, but shielded_balances reads spendable_balance directly, derive_spend_keyset accepts those published keys, and ordinary note selection calls get_unspent_notes without checking hydration. Those APIs can consequently report or select A after the failed bind.
Read and validate exclusion evidence before publishing restored notes, or quarantine affected accounts so every balance and selection consumer refuses access until restoration succeeds. Add a failed-known-spent-read regression that checks generic balance and selection APIs after the bind error, rather than checking only the returned error or local snapshot availability.
In `packages/rs-platform-wallet/src/wallet/shielded/file_store.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:825-827: Restore input exclusion from durable tip settlements
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4438377611)
The per-nullifier overlay fixes restoration only for inputs this condition records. For a successful tip consuming A and B, mark_spent(A) settles the barrier as Executed, removes the entire matching recovery operation through SubwalletState::drop_redrives_containing, records A, and deletes the durable recovery row. When finalization or scan reconciliation subsequently calls mark_spent(B), tip_barriers_spending excludes the already-Executed barrier and no recovery operation remains to populate dropped_redrives. Both operands here are false, so B never enters shielded_known_spent despite being individually observed spent. The in-memory store uses the same condition.
A process exit after both marks but before the host persists its spent-note changeset therefore restores B from the old snapshot without a recovery reservation or spent exclusion. Balances count consumed funds and note selection can build a payment from them; on-chain nullifier checks prevent execution but do not repair the wallet's reported funds. An interruption between the individual marks also loses durable recovery protection for B.
Track individually observed owned spends independently of whether an earlier input already settled the barrier or deleted the shared recovery operation. Preserve protection for remaining inputs across interruptions without treating one observed input as proof that unobserved siblings were spent. Add successful two-input settlement followed by reopen/stale-snapshot restoration, plus interruption after the first mark; retain the existing failed-tip test that leaves an unobserved sibling spendable.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Extend conservative rejection handling to ordinary shielded spends — Ordinary non-tip spends still classify unauthenticated consensus-coded DAPI errors as definitive failures, including in non-guarded re-drive. An endpoint can relay a valid payment and fabricate a rejection, causing reservations to be released and failure to be reported; a retry funded from different notes can pay twice. This behavior is present in the supplied base revision, and the PR's guarded-tip path no longer relies on it, so it is not an additional blocker here.
- Follow-up: Track a separate security fix extending conservative broadcast, wait, and recovery handling to ordinary shielded payments, with fabricated-rejection regression coverage.
| let known_spent = store.known_spent_nullifiers(*id).map_err(|e| { | ||
| crate::error::PlatformWalletError::ShieldedStoreError(e.to_string()) | ||
| })?; |
There was a problem hiding this comment.
✅ Resolved at
1b300991; see the replies below.
🔴 Blocking: Keep failed spent-evidence restores out of the spendable store
This error is returned after the preceding loop has already inserted the host's notes. Reopen after a confirmed spend of A with an old unspent host snapshot and a malformed sibling nullifier row in shielded_known_spent for the same account. known_spent_nullifiers collects and validates the entire result before returning, so the malformed row makes this lookup fail without applying even A's valid exclusion. The store lock is then released with A unspent and its recovery reservation already deleted.
The failed bind does not quarantine that state: install_shielded_views publishes shielded_keys before restoration and only clears hydration on failure. Hydration protects local_balance_snapshot, but shielded_balances reads spendable_balance directly, derive_spend_keyset accepts those published keys, and ordinary note selection calls get_unspent_notes without checking hydration. Those APIs can consequently report or select A after the failed bind.
Read and validate exclusion evidence before publishing restored notes, or quarantine affected accounts so every balance and selection consumer refuses access until restoration succeeds. Add a failed-known-spent-read regression that checks generic balance and selection APIs after the bind error, rather than checking only the returned error or local snapshot availability.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in 1b30099. Known spends are now loaded once at open into the store and written through, so known_spent_nullifiers reads memory and can't fail partway through a restore. Restore also reads them before it restores any of the subwallet's notes. Rows are only ever written with exact sizes, so a row that doesn't decode can only be external damage. The loader skips such a row with an error instead of discarding the valid ones, so A's exclusion still applies when a sibling row is malformed.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 1b300991): Your startup cache preserves A's valid exclusion when a sibling row has an invalid width, and restore_locked reads exclusions before inserting that subwallet's notes. That fixes the reported partial-restore exposure; skipping a damaged input's own exclusion is a separate successful-open defect reported above.
…ow until all its inputs are A two-input tip's second input was never recorded as spent: marking the first settled the barrier and dropped the shared recovery operation, so the second matched neither. A stop between the two marks also lost the second input's durable reservation. mark_spent now records each owned note it sees newly spent, plus tracked ones, and keeps a recovery operation's durable row until every input it reserves is recorded; a restart in between rehydrates it and keeps the rest reserved. Known spends are loaded at open, skipping a damaged row rather than failing the others, and restore reads them before restoring any note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified two blocking input-exclusion defects at head 1b30099: account rebinding can delete partially reconciled recovery protection, and the known-spend loader silently omits damaged exclusion evidence. All 34 prior findings were revalidated: 33 are fixed in their reported scenarios, and the performance suggestion remains withdrawn. This was a static review; git diff --check passed and current CI reports successful Kotlin build/tests, but provides no Rust-wallet or Swift validation results.
🔴 2 blocking
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to shielded/operations.rs, shielded/tips.rs and shielded/file_store.rs alter payment admission, note reservations, spend settlement and durable recovery storage, directly affecting funds movement and storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:900-908: Preserve partially recorded input exclusions across account rebinds
The retained recovery row protects the two-input interruption case only until the paying account is purged. Suppose a tip consumes A and B, but the process stops after recording A and before recording B or updating the host snapshot. On reopen, restoring the stale snapshot applies A's known-spent exclusion, removes the operation from SubwalletState.redrives, and retains its SQL row through lingering_redrive_rows; B remains reserved. However, the barrier is already Executed, so register_locked's Pending-only account pin allows a rebind from [0, 1] to [1]. purge_subwallet then unconditionally deletes nullifier-backed recovery rows and removes the account's reservations. Re-adding account 0 and restoring the same snapshot excludes only A, leaving the already-consumed B countable and selectable.
Make retained input-exclusion ownership authoritative across account purges, or refuse dropping/re-keying an account until its remaining exclusions are reconciled. Extend a_two_input_tip_keeps_both_inputs_excluded_across_interruptions with an offline drop-and-readd after its first reopen, and assert that B remains excluded from both spendable_balance and get_unspent_notes.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:570-578: Do not treat damaged spent evidence as an absent exclusion
This branch detects invalid exclusion evidence but still publishes an apparently complete known-spent set. After mark_spent durably records consumed note A and deletes its single-input recovery row, change A's shielded_known_spent.account_index to -1 and reopen with a host snapshot predating the spent update. The loader logs and skips A's row, open_path succeeds, and known_spent_nullifiers returns Ok without A. restore_locked therefore restores A as unspent and establishes its ledger, allowing spendable_balance and get_unspent_notes to count and select an already-consumed input. The Executed barrier does not compensate because restoration no longer uses barriers as spent evidence.
Preserve readable sibling exclusions, but fail store opening on detected exclusion damage or retain explicit damage state that prevents affected restored accounts from becoming spendable until reconciliation. Add a regression that damages the consumed input's own known-spend row and verifies that reopening/restoration cannot expose it as available.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Extend conservative remote-verdict handling to ordinary shielded spends — The ordinary shielded-spend paths still trust unauthenticated rejection metadata: classify_spend_wait_failure maps consensus-coded errors to definitive failure, and unguarded re-drives release reservations through DefinitiveRejection. A dishonest endpoint can relay a payment and fabricate rejection metadata, causing a potentially executing payment to be reported as failed. The same behavior exists on the specified base; this PR adds conservative handling for guarded tips without introducing or worsening the ordinary-spend behavior.
- Follow-up: Track a separate security fix applying conservative outcome handling to ordinary shielded spends and recovery, with fabricated broadcast and wait-error regression cases.
| for (activity_id, inputs) in resolved { | ||
| self.lingering_redrive_rows | ||
| .insert((id, activity_id), inputs); | ||
| } | ||
| // A recovery row goes once every input it reserves is known spent: | ||
| // one observed input does not prove the others spent (a competing | ||
| // payment may have taken it), and until then a restart rehydrates | ||
| // the row and keeps the rest reserved. A row left behind by an input | ||
| // never seen spent is released by the re-drive and prune passes. |
There was a problem hiding this comment.
✅ Resolved at
50c24fbf; see the replies below.
🔴 Blocking: Preserve partially recorded input exclusions across account rebinds
The retained recovery row protects the two-input interruption case only until the paying account is purged. Suppose a tip consumes A and B, but the process stops after recording A and before recording B or updating the host snapshot. On reopen, restoring the stale snapshot applies A's known-spent exclusion, removes the operation from SubwalletState.redrives, and retains its SQL row through lingering_redrive_rows; B remains reserved. However, the barrier is already Executed, so register_locked's Pending-only account pin allows a rebind from [0, 1] to [1]. purge_subwallet then unconditionally deletes nullifier-backed recovery rows and removes the account's reservations. Re-adding account 0 and restoring the same snapshot excludes only A, leaving the already-consumed B countable and selectable.
Make retained input-exclusion ownership authoritative across account purges, or refuse dropping/re-keying an account until its remaining exclusions are reconciled. Extend a_two_input_tip_keeps_both_inputs_excluded_across_interruptions with an offline drop-and-readd after its first reopen, and assert that B remains excluded from both spendable_balance and get_unspent_notes.
source: gpt-6.1-sol (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 50c24fb. register_locked now refuses a bind that drops or re-keys an account while has_unreconciled_spend(id) holds, with ShieldedRecoveryKeysRequired. That covers two cases: the account still has a recovery row kept for unconfirmed inputs, or one of its armed recovery operations has some inputs known spent and others not (the pre-restore case after a reopen). The purge can therefore no longer release B's last durable reservation. a_two_input_tip_keeps_both_inputs_excluded_across_interruptions now tries to rebind from [0] to [1] after the first reopen. It asserts the refusal and that B is still excluded, then confirms B and checks the row goes.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 50c24fbf): Your register_locked check now refuses dropping or re-keying an account while lingering recovery or a rehydrated operation intersects known-spent evidence. The extended two-input regression verifies that the attempted drop is refused while B remains reserved.
| match ( | ||
| <[u8; 32]>::try_from(wallet_id), | ||
| u32::try_from(account_index), | ||
| <[u8; 32]>::try_from(nullifier), | ||
| ) { | ||
| (Ok(wallet_id), Ok(account_index), Ok(nullifier)) => { | ||
| known.insert((SubwalletId::new(wallet_id, account_index), nullifier)); | ||
| } | ||
| _ => tracing::error!("skipping an unreadable known-spend row"), |
There was a problem hiding this comment.
✅ Resolved at
50c24fbf; see the replies below.
🔴 Blocking: Do not treat damaged spent evidence as an absent exclusion
This branch detects invalid exclusion evidence but still publishes an apparently complete known-spent set. After mark_spent durably records consumed note A and deletes its single-input recovery row, change A's shielded_known_spent.account_index to -1 and reopen with a host snapshot predating the spent update. The loader logs and skips A's row, open_path succeeds, and known_spent_nullifiers returns Ok without A. restore_locked therefore restores A as unspent and establishes its ledger, allowing spendable_balance and get_unspent_notes to count and select an already-consumed input. The Executed barrier does not compensate because restoration no longer uses barriers as spent evidence.
Preserve readable sibling exclusions, but fail store opening on detected exclusion damage or retain explicit damage state that prevents affected restored accounts from becoming spendable until reconciliation. Add a regression that damages the consumed input's own known-spend row and verifies that reopening/restoration cannot expose it as available.
source: gpt-6.1-sol (phase2-reviewer: architecture-layering)
There was a problem hiding this comment.
Fixed in 50c24fb. A known-spend row that doesn't decode now marks its wallet's evidence as damaged, or every wallet's when the wallet id itself is unreadable. Readable sibling rows still load. known_spent_nullifiers then fails for that wallet, and restore reads it before restoring any note, so the bind fails and nothing from the snapshot becomes countable or selectable. A shielded Clear rebuilds every spend from the chain while the host wipes its notes, so it deletes the recorded spends and the damage, and the wallet can restore again. Tests: damaged_known_spends_refuse_reads_until_cleared damages the consumed note's own row, then a row with an unreadable wallet, and checks both refusals and the Clear. damaged_spend_evidence_fails_the_restore_before_publishing_notes checks that the failed restore leaves no unspent notes and a zero spendable balance.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 50c24fbf): Your wallet-scoped and unattributed damage tracking now makes known_spent_nullifiers refuse incomplete evidence before restore inserts notes. That closes the damaged-row case; the separate interruption boundary when Clear deletes the evidence is reported as a new finding.
…restores over damaged spend evidence A bind could drop the account of a recovery operation whose first input was confirmed and second not; the purge deleted the row still reserving the second, and a stale host snapshot then offered it. Such a bind is now refused until the rest is confirmed. A known-spend row that did not decode was skipped, so the consumed note it recorded came back spendable. Its wallet (every wallet, when the wallet id is unreadable) now fails restores until a shielded Clear, which rebuilds spends from the chain, removes the evidence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The prior fixes are present: 35 prior findings are fixed, and the previously withdrawn optimization remains withdrawn. Two blocking persistence issues remain—failed known-spend writes can lose exclusions, and Clear can discard recovery evidence before stale snapshots are durably invalidated—plus a non-blocking recovery-pin cleanup defect. Current CI passes Kotlin build/tests but supplies no Rust or Swift result; verification was static, with no builds or tests run.
🔴 2 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 11: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 14: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to shielded/operations.rs, shielded/tips.rs and shielded/file_store.rs directly alter funds-movement admission, spend reservations, durable recovery and settlement, including new SQLite tables. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:920-925: Keep failed known-spend writes authoritative without a recovery row
Retaining recovery rows protects only notes that already have one. An owned note consumed by another device, or by an ordinary payment whose successful broadcast did not require a durable redrive, can reach this branch with `resolved` empty. `sw.mark_spent` has already mutated memory, but a failed `shielded_known_spent` INSERT leaves no durable exclusion or fallback reservation. This branch returns success, so the scan can advance its watermark. If the process exits before the host persists the spent changeset, reopening and restoring its earlier snapshot makes the consumed note countable and selectable again. Reobserving the nullifier also need not retry the write: the note is no longer newly spent, and there may be no recovery operation or tip barrier tracking it.
Make the spent transition fail-closed independently of an existing recovery row. Persist the exclusion before publishing the mutation, or establish durable quarantine/recovery that prevents restored eligibility until the write succeeds; retry work must not depend solely on `newly_spent`. Add an INSERT-failure regression for an owned note with no recovery row, interrupt before host persistence, and verify that reopening with the stale snapshot cannot expose the note.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1613-1620: Make Clear's exclusion reset fail-atomic and restart-safe
Clear deletes authoritative recovery and spent evidence without durably invalidating the host snapshots that still depend on it. Both host paths reset Rust first and wipe Room/SwiftData afterward. With a settled tip and an older unspent host snapshot, interrupting after this purge but before the host wipe leaves neither the recovery reservation nor the known-spent exclusion. On restart, `restore_locked` accepts the old note and establishes its ledger, making a consumed input countable and selectable. The coordinator's in-memory `clear_generation` does not survive reopening, and an Executed barrier does not supply per-nullifier exclusions.
There is also a failure boundary inside this block: these DELETEs are separate autocommitted statements. If deleting `shielded_known_spent` fails, the recovery rows have already disappeared even though Clear returns an error and the host correctly retains its snapshot. In the two-input interruption case, A retains its recorded exclusion but B loses its last durable reservation and becomes available after reopening.
Keep exclusions and necessary reservations authoritative until pre-Clear snapshots are durably rejected or reconciled. If Clear removes those records, commit the recovery/evidence reset and its durable invalidation state atomically, and apply in-memory cleanup only after commit. A SQLite transaction alone addresses the second failure but not the process interruption before the host wipe. Add regressions for both successful Rust reset followed by interruption before host deletion, and a rejected known-spend DELETE followed by reopen and stale restoration.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1498-1507: Retire lingering account ownership when recovery releases the remainder
`has_unreconciled_spend` now treats every `lingering_redrive_rows` entry as live recovery ownership, but the terminal release paths do not remove those entries. For an operation reserving A and B, observing a competing payment consume only A moves the operation into this map while B remains reserved. After its original anchor is pruned, `release_stranded_spends` calls `clear_pending(B)`, which releases B and deletes the durable recovery row through `delete_redrive_rows_containing`, but leaves the lingering entry. `clear_redrive` likewise never removes it. Since B is genuinely unspent, the all-inputs-known-spent cleanup need never run.
Subsequent binds dropping the account therefore keep failing with `ShieldedRecoveryKeysRequired` after its durable recovery and reservations are gone. Further sync passes and tip abandonment do not clear the obsolete pin; reopening happens to remove it. Retire lingering ownership as part of successful durable recovery deletion, preserving it when deletion fails and preserving A's independent spent exclusion. Add the competing-spend → prune-release → successful account-drop sequence without reopening or clearing the store.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Audit ordinary shielded payments for unauthenticated rejection handling — The unchanged
broadcast_definitely_failedhelper accepts unauthenticated consensus-error metadata and transport statuses as definitive failure, and ordinary re-drives still release recovery on consensus-coded rejection. The SDK returns wait-error envelopes before proof verification. A dishonest endpoint can relay a payment and fabricate a rejection; rebuilding the reported failed payment from different funded notes can pay twice. These ordinary-payment classifications already exist in the base revision, while this PR's guarded-tip path explicitly avoids them.- Follow-up: Track a separate security follow-up for ordinary shielded broadcast, wait, and recovery classification, retaining unresolved outcomes unless authenticated evidence or a local invariant establishes non-execution.
| .is_err() | ||
| { | ||
| for (activity_id, inputs) in resolved { | ||
| self.lingering_redrive_rows.insert((id, activity_id), inputs); | ||
| } | ||
| return Ok(outcome.newly_spent); |
There was a problem hiding this comment.
✅ Resolved at
4654d678; see the replies below.
🔴 Blocking: Keep failed known-spend writes authoritative without a recovery row
Retaining recovery rows protects only notes that already have one. An owned note consumed by another device, or by an ordinary payment whose successful broadcast did not require a durable redrive, can reach this branch with resolved empty. sw.mark_spent has already mutated memory, but a failed shielded_known_spent INSERT leaves no durable exclusion or fallback reservation. This branch returns success, so the scan can advance its watermark. If the process exits before the host persists the spent changeset, reopening and restoring its earlier snapshot makes the consumed note countable and selectable again. Reobserving the nullifier also need not retry the write: the note is no longer newly spent, and there may be no recovery operation or tip barrier tracking it.
Make the spent transition fail-closed independently of an existing recovery row. Persist the exclusion before publishing the mutation, or establish durable quarantine/recovery that prevents restored eligibility until the write succeeds; retry work must not depend solely on newly_spent. Add an INSERT-failure regression for an owned note with no recovery row, interrupt before host persistence, and verify that reopening with the stale snapshot cannot expose the note.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in 4654d67. mark_spent now records the exact nullifier before changing anything. That covers an owned note newly seen spent, with or without a recovery row, plus tracked ones. If the write fails, the call returns the error with nothing mutated. The scan pass then fails before its watermark advances, and the next pass sees the note as newly spent again and retries. Finalization already treats a mark_spent error as best-effort, and the inputs' reservations stay held. Known cost: notes the failed pass marked before the failing one reach the host only at the next restore, which queues the recorded spends to the host. Test: a_failed_spend_record_marks_nothing_and_is_retried fails the INSERT with a trigger for an owned note with no recovery row. It checks the note is still unspent and the call errored, then retries and checks the record persists across a reopen.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 4654d678): For the reported owned-note case without recovery state, your INSERT now precedes note mutation and its error aborts the pass before watermark advancement. The failure-injection regression checks the unchanged note and successful recording on retry; the separate terminal-barrier ordering defect is reported independently.
| .map_err(|e| { | ||
| FileShieldedStoreError::Storage(format!("purge all pending spends: {e}")) | ||
| })?; | ||
| // Clear rebuilds every spend from the chain, and the host wipes | ||
| // its notes, so recorded spends (damaged or not) are not needed. | ||
| conn.execute("DELETE FROM shielded_known_spent", []) | ||
| .map_err(|e| FileShieldedStoreError::Storage(format!("purge known spends: {e}")))?; |
There was a problem hiding this comment.
🔴 Blocking: Make Clear's exclusion reset fail-atomic and restart-safe
Clear deletes authoritative recovery and spent evidence without durably invalidating the host snapshots that still depend on it. Both host paths reset Rust first and wipe Room/SwiftData afterward. With a settled tip and an older unspent host snapshot, interrupting after this purge but before the host wipe leaves neither the recovery reservation nor the known-spent exclusion. On restart, restore_locked accepts the old note and establishes its ledger, making a consumed input countable and selectable. The coordinator's in-memory clear_generation does not survive reopening, and an Executed barrier does not supply per-nullifier exclusions.
There is also a failure boundary inside this block: these DELETEs are separate autocommitted statements. If deleting shielded_known_spent fails, the recovery rows have already disappeared even though Clear returns an error and the host correctly retains its snapshot. In the two-input interruption case, A retains its recorded exclusion but B loses its last durable reservation and becomes available after reopening.
Keep exclusions and necessary reservations authoritative until pre-Clear snapshots are durably rejected or reconciled. If Clear removes those records, commit the recovery/evidence reset and its durable invalidation state atomically, and apply in-memory cleanup only after commit. A SQLite transaction alone addresses the second failure but not the process interruption before the host wipe. Add regressions for both successful Rust reset followed by interruption before host deletion, and a rejected known-spend DELETE followed by reopen and stale restoration.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in 4654d67. Readable recorded spends now survive Clear: a spent nullifier stays spent, so a Clear interrupted before the host wipe still excludes consumed notes on the next restore. purge_all_subwallets deletes the recovery rows and only the damaged spend rows (by rowid) in one BEGIN IMMEDIATE transaction, rolling back on failure, so a failed Clear keeps every recovery row. Clear is also refused (ShieldedTipPending, which keeps the manager's restart-on-refusal) while any spend is partly confirmed, so B's last reservation can't be deleted mid-reconciliation. The pre-check reports the same. Tests: a_clear_keeps_recorded_spends (across a reopen), and the two-input test now asserts Clear is refused while B is unconfirmed.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Still applies (re-reviewed at 4654d678): Your transaction and retention of readable spends fix the SQL-failure and clean-evidence cases, but Clear still deletes a consumed note's damaged exclusion and removes quarantine before the host wipe commits. The partial-confirmation check does not cover a fully settled operation with no recovery row; reopening after that interrupted repair accepts the unchanged unspent snapshot.
There was a problem hiding this comment.
Partly fixed in 158ae6a, with one case left open for a maintainer decision. Clear no longer repairs damaged spend evidence, because the host may still hold the snapshot it protects. A wallet with damaged spend rows keeps refusing restores until the wallet is removed, which deletes the host's snapshot too, and re-added. So no Clear, interrupted or not, can drop a consumed note's only exclusion. Test: damaged_known_spents_refuse_reads_until_the_wallet_is_removed. The note-ingestion finding is fixed in the same commit: recorded spends now exclude their notes from get_unspent_notes and spendable_balance in both stores, so a receipt a rescan saves before the nullifier replay is never counted or selected, and the replay still reports it newly spent to the host. Test: a_recorded_spend_excludes_a_rescanned_receipt_before_replay. Open: a damaged row whose wallet id is unreadable blocks every wallet's restores and has no repair path. Wallet removal can't attribute it, and a Clear-based repair is what this thread showed unsafe. Choosing that recovery path (for example an explicit acknowledged reset that rebuilds every wallet from the chain) is for the maintainers, so I'm raising it with them rather than guessing.
🤖 Posted autonomously by Claude on behalf of pasta.
| fn has_unreconciled_spend(&self, id: SubwalletId) -> bool { | ||
| self.lingering_redrive_rows | ||
| .keys() | ||
| .any(|(owner, _)| *owner == id) | ||
| || self.subwallets.get(&id).is_some_and(|state| { | ||
| state.redrives.values().any(|redrive| { | ||
| redrive | ||
| .nullifiers | ||
| .iter() | ||
| .any(|nullifier| self.known_spent.contains(&(id, *nullifier))) |
There was a problem hiding this comment.
✅ Resolved at
4654d678; see the replies below.
🟡 Suggestion: Retire lingering account ownership when recovery releases the remainder
has_unreconciled_spend now treats every lingering_redrive_rows entry as live recovery ownership, but the terminal release paths do not remove those entries. For an operation reserving A and B, observing a competing payment consume only A moves the operation into this map while B remains reserved. After its original anchor is pruned, release_stranded_spends calls clear_pending(B), which releases B and deletes the durable recovery row through delete_redrive_rows_containing, but leaves the lingering entry. clear_redrive likewise never removes it. Since B is genuinely unspent, the all-inputs-known-spent cleanup need never run.
Subsequent binds dropping the account therefore keep failing with ShieldedRecoveryKeysRequired after its durable recovery and reservations are gone. Further sync passes and tip abandonment do not clear the obsolete pin; reopening happens to remove it. Retire lingering ownership as part of successful durable recovery deletion, preserving it when deletion fails and preserving A's independent spent exclusion. Add the competing-spend → prune-release → successful account-drop sequence without reopening or clearing the store.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Fixed in 4654d67. When clear_pending successfully deletes the durable rows that reserve an input (as release_stranded_spends does), it now drops the lingering entries holding that input. clear_redrive drops its entry after the row delete succeeds. A failed delete keeps the entry, and A's recorded spend is untouched. Test: a_released_remainder_retires_its_account_pin arms A+B, sees A spent by a competing payment, releases B, and checks the account is no longer pinned without reopening and the row is gone after reopen.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved (re-reviewed at 4654d678): Your successful clear_pending deletion now retires matching lingering entries, and clear_redrive removes its entry only after durable deletion succeeds. The released-remainder regression checks that the account pin ends without reopening.
…rded spends through Clear A known-spend write that failed for an owned note with no recovery row left no durable exclusion while the scan went on. mark_spent now records before changing anything and fails the call if it cannot, so the pass is retried. Clear deleted recovery and spend evidence in separate statements and before the host wiped its notes. It now deletes recovery rows and only damaged spend rows in one transaction, keeps readable recorded spends, and is refused while a spend is partly confirmed. A released or cleared recovery row now also ends its account pin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the combined reviewer claims against head 4654d67. Five blocking persistence defects and one non-blocking backend-consistency issue remain; of the 39 prior findings, 37 are fixed, one remains valid, and one remains withdrawn. This was a static review without builds or test execution; the current GitHub checks show the Kotlin SDK build and tests passing.
🔴 5 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Enforce recorded-spend exclusions at the store's note-insertion boundary
packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:854-856
save_note delegates directly to overwrite-by-nullifier storage without applying known_spent, while balance and selection read only note flags and reservations. After Clear retains A's readable spent exclusion but empties note state, a rebuilding scan saves A's decrypted receipt with is_spent=false. All receipts are inserted before nullifier replay. If recording a newly observed spend B fails before replay reaches A—for example, B belongs to the first account and A to the second—the pass returns an error without rolling back inserted notes. A remains countable and selectable despite its intact durable exclusion. The host-restoration overlay never runs on this scan-ingestion path, and a later retry does not protect callers in the interval.
Make recorded exclusions authoritative at note ingestion or eligibility reads for every caller, and preserve the corresponding spent-state delivery to host persistence. Check both store implementations. Add a rescan regression retaining A's exclusion across Clear, inserting its unspent receipt, failing B's INSERT before A's replay, and checking balance and selection immediately after the failed pass.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 13: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate changes to shielded/operations.rs, shielded/tips.rs and shielded/file_store.rs directly alter funds movement, note reservations and durable spend recovery, including the barriers that prevent duplicate payments after restart. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:892-900: Persist spent exclusions before publishing a terminal barrier
mark_spent calls settle_tips before record_known_spent. The status UPDATE can therefore commit Executed while the subsequent known-spend INSERT fails, or the process can stop between those commits. For a previously returned unresolved single-input tip, this leaves its recovery row armed but no recorded input exclusion. has_unreconciled_spend returns false because it requires either a lingering entry or an armed operation containing a recorded input, while register_locked pins only Pending barriers. An account-dropping bind can consequently delete the last recovery reservation; re-adding the account and restoring the old host snapshot offers the consumed note again. Clear also passes its ownership checks in this state.
Persist the exact input exclusion before publishing Executed, or commit both changes atomically, keeping recovery deletion last. The new INSERT-failure regression has no barrier and does not exercise this earlier mutation. Add a barrier-backed INSERT-failure/interruption regression followed by destructive bind or Clear and stale restoration.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1327-1333: Retire damaged-spend row identities when purging their wallet
purge_wallet deletes the wallet's known-spend SQL rows and clears its damage flag, but leaves their identifiers in damaged_known_spent_rows. The table uses ordinary reusable SQLite rowids, not lifetime-stable identities. Load wallet A's sole damaged exclusion, purge A, and record a readable spend for wallet B: B can receive the deleted rowid. A subsequent Clear blindly deletes B's valid exclusion through the stale repair list, violating the new retention contract. The in-memory known_spent set still contains B's entry, hiding the deletion until reopening, when a stale host snapshot can expose B's consumed note.
Retire cached repair targets after their rows are successfully purged, retaining enough ownership information to identify them, and scope or revalidate targets before repair deletion. Add damaged-A → purge-A → record-B → Clear → reopen coverage proving that B's readable exclusion survives.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:854-856: Enforce recorded-spend exclusions at the store's note-insertion boundary
save_note delegates directly to overwrite-by-nullifier storage without applying known_spent, while balance and selection read only note flags and reservations. After Clear retains A's readable spent exclusion but empties note state, a rebuilding scan saves A's decrypted receipt with is_spent=false. All receipts are inserted before nullifier replay. If recording a newly observed spend B fails before replay reaches A—for example, B belongs to the first account and A to the second—the pass returns an error without rolling back inserted notes. A remains countable and selectable despite its intact durable exclusion. The host-restoration overlay never runs on this scan-ingestion path, and a later retry does not protect callers in the interval.
Make recorded exclusions authoritative at note ingestion or eligibility reads for every caller, and preserve the corresponding spent-state delivery to host persistence. Check both store implementations. Add a rescan regression retaining A's exclusion across Clear, inserting its unspent receipt, failing B's INSERT before A's replay, and checking balance and selection immediately after the failed pass.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/file_store.rs:1633-1641: Make Clear's exclusion reset fail-atomic and restart-safe
(existing thread: https://github.com/dashpay/platform/pull/5709#discussion_r4439093353)
Retaining readable spends and transacting the deletions fixes the clean-evidence interruption and failed-DELETE cases. The damaged-evidence repair still removes the durable restore refusal before the host snapshot is invalidated. Take a fully settled consumed note A whose host row still says unspent, damage A's known-spend account_index to -1, and reopen. Restoration correctly refuses initially, but Clear deletes that damaged row and then clears the damage flags without persisting any snapshot invalidation. With no remaining recovery operation or Pending barrier, the partial-confirmation gate does not prevent this Clear.
Both hosts wipe their snapshots afterward: Kotlin calls clearShieldedStorage before the Room deletion transaction, and Swift invokes clearShielded before its SwiftData deletions and save. If that wipe fails or the process stops between the stores, reopening finds neither A's exclusion nor its damage marker. restore_locked accepts the unchanged unspent note and establishes its ledger. The damaged-known-spends regression currently verifies that the evidence becomes empty after repair, but never restores the retained snapshot.
Commit a durable Rust-owned restore fence or rebuild-required marker with damaged-evidence deletion, and enforce it until pre-repair snapshots are durably rejected or authoritative reconstruction completes. Extend the regression through successful Rust Clear, interruption before host deletion, reopen, and stale restoration.
In `packages/rs-platform-wallet/src/wallet/shielded/store.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/store.rs:179-185: Scope tip-settlement matches to the paying subwallet
tip_barriers_spending matches the wallet but ignores barrier.account_index. apply_scanned_nullifier_spends processes every scanned nullifier against each registered subwallet, so processing a tip account T's nullifier against ordinary account 0 settles T's barrier and records its exclusion under account 0. If processing stops or fails before reaching T, T retains its recovery row but has no recorded input of its own. Its barrier is already Executed, and has_unreconciled_spend(T) is false, so dropping T or running Clear can delete its last reservation. Restoring T's stale host notes then exposes the consumed input.
Match the full SubwalletId when selecting barriers for settlement and exclusion recording. Reordering the known-spend write alone does not fix the incorrect account attribution. Add a two-account scan interrupted or failed after the non-owning account, followed by destructive bind/Clear and stale restoration before the owning account is scanned.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/store.rs:1274-1282: In-memory backend always reports no unreconciled spend
Both unreconciled-spend methods return false unconditionally, so coordinator tests using InMemoryShieldedStore cannot exercise the partial-confirmation refusal enforced by the file backend. This is not solely a restart distinction: after one input of a two-input operation is marked spent, its sibling remains reserved within the process, but Clear and destructive binds can purge that reservation. SubwalletState::mark_spent also removes the redrive operation, so merely inspecting the remaining redrive map would not restore parity.
Retain enough in-memory ownership information to implement the same predicate, or explicitly document this backend as a reduced test double and use the file backend for lifecycle tests that depend on partial-confirmation safety.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Extend conservative rejection handling to ordinary shielded recovery — The ordinary, non-tip re-drive path still treats decoded remote consensus-error metadata as definitive and releases reservations. The SDK deserializes that metadata without authenticating non-execution, so an endpoint can relay a signed payment and fabricate a rejection. This behavior is present in the supplied base revision; this PR adds conservative handling for guarded tips rather than introducing or worsening the ordinary-spend behavior.
- Follow-up: Track a separate security follow-up for authenticated outcomes or conservative unresolved handling in ordinary shielded broadcast and recovery.
| let tips = tip_barriers_spending(&self.tip_barriers, id.wallet_id, nullifier); | ||
| let executed = tips | ||
| .iter() | ||
| .map(|activity_id| ((id.wallet_id, *activity_id), TipBarrierStatus::Executed)) | ||
| .collect(); | ||
| self.settle_tips(executed); | ||
| let Some(sw) = self.subwallets.get(&id) else { | ||
| if !tips.is_empty() { | ||
| self.record_known_spent(id, nullifier)?; |
There was a problem hiding this comment.
🔴 Blocking: Persist spent exclusions before publishing a terminal barrier
mark_spent calls settle_tips before record_known_spent. The status UPDATE can therefore commit Executed while the subsequent known-spend INSERT fails, or the process can stop between those commits. For a previously returned unresolved single-input tip, this leaves its recovery row armed but no recorded input exclusion. has_unreconciled_spend returns false because it requires either a lingering entry or an armed operation containing a recorded input, while register_locked pins only Pending barriers. An account-dropping bind can consequently delete the last recovery reservation; re-adding the account and restoring the old host snapshot offers the consumed note again. Clear also passes its ownership checks in this state.
Persist the exact input exclusion before publishing Executed, or commit both changes atomically, keeping recovery deletion last. The new INSERT-failure regression has no barrier and does not exercise this earlier mutation. Add a barrier-backed INSERT-failure/interruption regression followed by destructive bind or Clear and stale restoration.
source: gpt-6-astra (phase2-reviewer: architecture-layering)
There was a problem hiding this comment.
Fixed in 158ae6a. mark_spent now writes in this order: the input's known-spend record, then the barrier's executed settlement, then note and recovery state. A failed record returns before the barrier settles, so it stays pending and keeps pinning its account and Clear. Test: a_failed_spend_record_leaves_the_tip_barrier_pending (barrier-backed INSERT failure, checked after a reopen).
🤖 Posted autonomously by Claude on behalf of pasta.
| barriers | ||
| .iter() | ||
| .filter(|((wallet, _), barrier)| { | ||
| *wallet == wallet_id | ||
| && barrier.status != TipBarrierStatus::Executed | ||
| && barrier.nullifiers.contains(nullifier) | ||
| }) |
There was a problem hiding this comment.
🔴 Blocking: Scope tip-settlement matches to the paying subwallet
tip_barriers_spending matches the wallet but ignores barrier.account_index. apply_scanned_nullifier_spends processes every scanned nullifier against each registered subwallet, so processing a tip account T's nullifier against ordinary account 0 settles T's barrier and records its exclusion under account 0. If processing stops or fails before reaching T, T retains its recovery row but has no recorded input of its own. Its barrier is already Executed, and has_unreconciled_spend(T) is false, so dropping T or running Clear can delete its last reservation. Restoring T's stale host notes then exposes the consumed input.
Match the full SubwalletId when selecting barriers for settlement and exclusion recording. Reordering the known-spend write alone does not fix the incorrect account attribution. Add a two-account scan interrupted or failed after the non-owning account, followed by destructive bind/Clear and stale restoration before the owning account is scanned.
source: gpt-6-astra (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 158ae6a. tip_barriers_spending now takes the full SubwalletId and matches barrier.account_index, so replaying a tip account's nullifier against another account settles and records nothing. Test: a_tip_barrier_settles_only_from_its_paying_account.
🤖 Posted autonomously by Claude on behalf of pasta.
| self.unsettled_tip_settlements | ||
| .retain(|(wallet, _), _| *wallet != wallet_id); | ||
| self.known_spent.retain(|(id, _)| id.wallet_id != wallet_id); | ||
| self.lingering_redrive_rows | ||
| .retain(|(id, _), _| id.wallet_id != wallet_id); | ||
| self.damaged_known_spent.remove(&wallet_id); | ||
| Ok(()) |
There was a problem hiding this comment.
🔴 Blocking: Retire damaged-spend row identities when purging their wallet
purge_wallet deletes the wallet's known-spend SQL rows and clears its damage flag, but leaves their identifiers in damaged_known_spent_rows. The table uses ordinary reusable SQLite rowids, not lifetime-stable identities. Load wallet A's sole damaged exclusion, purge A, and record a readable spend for wallet B: B can receive the deleted rowid. A subsequent Clear blindly deletes B's valid exclusion through the stale repair list, violating the new retention contract. The in-memory known_spent set still contains B's entry, hiding the deletion until reopening, when a stale host snapshot can expose B's consumed note.
Retire cached repair targets after their rows are successfully purged, retaining enough ownership information to identify them, and scope or revalidate targets before repair deletion. Add damaged-A → purge-A → record-B → Clear → reopen coverage proving that B's readable exclusion survives.
source: gpt-6-astra (phase2-reviewer: general)
There was a problem hiding this comment.
Fixed in 158ae6a by removing the repair list: Clear no longer deletes damaged spend rows (see the Clear thread), so no cached rowid can outlive its row. purge_wallet deletes the wallet's rows by wallet_id and clears its damage flag.
🤖 Posted autonomously by Claude on behalf of pasta.
| fn has_unreconciled_spend(&self, _id: SubwalletId) -> bool { | ||
| // Nothing here outlives the process, so a purge loses no durable | ||
| // reservation. | ||
| false | ||
| } | ||
|
|
||
| fn has_unreconciled_spends(&self) -> bool { | ||
| false | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: In-memory backend always reports no unreconciled spend
Both unreconciled-spend methods return false unconditionally, so coordinator tests using InMemoryShieldedStore cannot exercise the partial-confirmation refusal enforced by the file backend. This is not solely a restart distinction: after one input of a two-input operation is marked spent, its sibling remains reserved within the process, but Clear and destructive binds can purge that reservation. SubwalletState::mark_spent also removes the redrive operation, so merely inspecting the remaining redrive map would not restore parity.
Retain enough in-memory ownership information to implement the same predicate, or explicitly document this backend as a reduced test double and use the file backend for lifecycle tests that depend on partial-confirmation safety.
source: muse-spark-1.3-contributor (phase1-reviewer: general)
There was a problem hiding this comment.
Documented in 158ae6a: the in-memory store is a reduced test double. It keeps no lingering-row ownership, and lifecycle tests that depend on partial-confirmation refusals use the file backend, as the existing ones do.
🤖 Posted autonomously by Claude on behalf of pasta.
…ettlement to the paying account, and keep damaged spend evidence through Clear - mark_spent records the input's spend before settling a matching barrier, so a failed record leaves the barrier pending. - A barrier settles only from its paying account, not from another account replaying the same nullifier. - Recorded spends exclude their notes from balance and selection reads, covering a rescan that saved a receipt before replaying its nullifier. - Clear no longer repairs damaged spend evidence, since the host may still hold the snapshot it protects; removing the wallet repairs it. This also drops the rowid repair list that a wallet purge left stale. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue being fixed or feature implemented
Before this change, the shielded-tip screens in the example apps decided four things about a payment themselves, and parsed the amount by their own rules:
The worst consequence: after a relaunch the in-flight guard was gone. A tip whose outcome was still open could be sent again from other notes and pay the recipient twice. Review of #5288 asked for each decision to move into
rs-platform-wallet(#5674 tracked the first and a source-account race that #5288 now fixes).This is the second PR of a stack:
The order is deliberate: the safety logic lands in Rust before the screens that rely on it. This PR stays a draft until #5288 merges.
What was done?
send_shielded_tipflushes the pending activity row and durably records aTipBarrier(recipient, notes, anchor) in a newshielded_tip_barrierstable in the shielded store, Rust's own SQLite. That table is separate from note and recovery state, so account purges, Clear and rescans don't remove it. A parked recovery record keeps the notes reserved across a relaunch.mark_spent, before the recovery row goes. A settlement write that fails is retried at the next sync pass.synchronous=FULL,fullfsync) before the broadcast.abandon_unresolved_shielded_tips, with the user's acknowledgement that it may still execute, asabandon_identity_debitworks today.ShieldedTipPending), including the address last confirmed for the name in case the recipient rotated it. Also refused: a second tip while one is being sent, a bind that drops or re-keys the paying account, and a shielded Clear. These refusals start when a send is admitted, under the coordinator's lifecycle lock and before note selection, so a Clear cannot land before the barrier exists; removing the wallet waits for the send to end. A refused Clear leaves background sync running. An unreadable barrier blocks its wallet's tips instead of being skipped.ShieldedTipDisposition::of(error)returnsNotSentorUnresolved. The C send reports it throughout_disposition(SHIELDED_TIP_DISPOSITION_*); JNI fills an out-array; Swift and Kotlin throwShieldedTipFailure(disposition). Argument errors raised before the native call are thrown before anything starts.review_shielded_tipreturns the resolved recipient, aShieldedTipRecipientChange(New,Unchanged, orChanged { previous }), and whether a payment to it is unresolved. Confirmations are kept per wallet in a newshielded_tip_recipientstable in the shielded store. They are keyed by the normalized DPNS label, recorded by the send, kept across a shielded Clear, and dropped with the wallet.identity_tip_accountreturns the identity's tip account and whether this session proved its slot: discovery, load, or a seed check in preparation or a send.ManagedIdentity::slot_verifiedis session-only, so a restored index, which may be a host placeholder, stays unverified until then.identity_tip_balancereturnsUnverified,Unavailable { account }orAvailable { account, credits }. A balance is available only for a verified slot whose account has a restored or scanned ledger, read from the reservation-aware local snapshot. So a cleared or never-scanned account no longer shows as zero. The C read fails as busy instead of waiting behind a scan, like the local balance snapshot.refresh_shielded_accountskeeps the wallet's ordinary accounts (bound, or persisted, or[0]) and adds the tip accounts of newly discovered identities. It replaces the screens' own account filtering.parse_shielded_tip_amountconverts a typed DASH amount to exact credits, refusing signs, grouping, exponents, zero, and fractions finer than one credit. Before this, Swift and Kotlin each parsed amounts with different rules.platform_wallet_manager_review_shielded_tip,platform_wallet_identity_tip_account,platform_wallet_manager_identity_tip_balance,platform_wallet_manager_refresh_shielded_accounts,platform_wallet_parse_shielded_tip_amount,platform_wallet_manager_abandon_unresolved_shielded_tips), JNI, Swift and Kotlin, which only marshal.How Has This Been Tested?
cargo fmt, the wallet CI clippy command, and every test of the three wallet crates (1749 wallet lib, 567 storage integration, 507 FFI).NOT_SENT/UNRESOLVED;Breaking Changes
There are no consensus or protocol changes.
platform_wallet_manager_send_shielded_tipgains a trailinguint32_t *out_disposition.ShieldedStoretrait: custom backends must implement two groups of new methods.confirmed_tip_recipient,confirm_tip_recipient). The record is kept per wallet, survives a shielded Clear, and is deleted bypurge_wallet.record_tip_barrier,tip_barriers,has_pending_tip_barriers,settle_tip_barrier,abandon_tip_barriers). A barrier must be settled as executed insidemark_spentbefore any recovery row is dropped. It must not be changed byclear_pending, must survive account purges and Clear, and is deleted bypurge_wallet. An unreadable barrier must failtip_barriers.known_spent_nullifiers).mark_spentmust durably record each owned note it sees newly spent, and each tracked one, before changing anything (failing the call if it cannot), and must keep a recovery operation's durable state until every input it reserves is recorded. Restoring a host snapshot marks those notes spent. Recorded spends also exclude their notes fromget_unspent_notesandspendable_balance. Kept untilpurge_wallet; a shielded Clear keeps them. A row that does not decode must failknown_spent_nullifiersfor its wallet (for every wallet when the wallet id is unreadable) rather than read as absent; removing the wallet repairs it.has_unreconciled_spend). A bind may not drop or re-key an account, and a shielded Clear is refused, while a recovery operation has some inputs known spent and others not (has_unreconciled_spends). Releasing or clearing that operation's durable row ends it.sendShieldedTipnow throwsShieldedTipFailure.The C, Swift and Kotlin sends are unreleased (#5288).
Checklist:
For repository code-owners and collaborators only
Closes #5674
🤖 Generated with Claude Code