fix(platform-wallet): fail a double-spending asset lock with a typed terminal error - #4356
fix(platform-wallet): fail a double-spending asset lock with a typed terminal error#4356QuantumExplorer wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAsset-lock recovery now detects confirmed input conflicts before rebroadcast or proof waiting. Rust exposes the conflict as FFI result code 42. Swift and Kotlin convert it into typed wallet errors with diagnostic details. ChangesAsset-lock input conflict handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AssetLockRecovery
participant WalletTransactionHistory
participant PlatformWalletFFI
participant SwiftOrKotlinSDK
AssetLockRecovery->>WalletTransactionHistory: inspect confirmed transaction inputs
WalletTransactionHistory-->>AssetLockRecovery: return conflicting spender details
AssetLockRecovery->>PlatformWalletFFI: return AssetLockInputConflict
PlatformWalletFFI-->>SwiftOrKotlinSDK: map result code 42
SwiftOrKotlinSDK-->>SwiftOrKotlinSDK: preserve terminal diagnostic message
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit 7d9be71) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 373-378: Correct the Broadcast-state description to reflect that
conflict detection prevents any additional broadcast and proof wait, rather than
claiming nothing was broadcast. Apply this wording consistently in
packages/rs-platform-wallet-ffi/src/error.rs lines 373-378,
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
lines 145-148, and the PlatformWalletError description at lines 419-425.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ebe6763-6a36-4b5e-ade5-369cf8c1b463
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new conflict detector can classify a transaction in a reorgable, non-chainlocked block as terminal and authorize the host to discard an asset lock that may become valid after a reorg. The typed error is also flattened by several public FFI paths, omitted from Kotlin's typed hierarchy, and documented incorrectly for locks already in the Broadcast state.
Source: codex general reviewer backend gpt-5.6-sol; codex rust-quality reviewer backend gpt-5.6-sol; codex ffi-engineer reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 3 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 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/asset_lock/sync/recovery.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:239: Require chain-lock finality before declaring the asset lock terminal
`TransactionRecord::is_confirmed()` delegates to `TransactionContext::confirmed()`, which returns true for both `InBlock` and `InChainLockedBlock`. The pinned key-wallet implementation explicitly states that `InBlock` can be reorganized out and exposes `is_chain_locked()` as the finality predicate. A sibling found only in an ordinary block can therefore trigger `AssetLockInputConflict` and authorize permanent deletion of the tracked lock even though a reorg may remove that sibling and make the asset-lock transaction valid again. The positive test currently constructs exactly an `InBlock` context, so it codifies the unsafe terminal verdict. Restrict this destructive classification to chainlocked records and change the positive fixture to `InChainLockedBlock`.
In `packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs:149-152: Manual FFI wrappers erase the new typed conflict code
`asset_lock_manager_catch_up_blocking` explicitly converts every wallet error to `ErrorWalletOperation`, bypassing the new `From<PlatformWalletError>` arm. The shielded funding wrappers repeat this at `shielded_send.rs:1024-1028` and `shielded_send.rs:1290-1294`; the latter is the public resume endpoint used by both Swift and JNI. Consequently, these paths return code 6 instead of code 41, so Swift receives `.walletOperation` and Kotlin receives the generic wallet-operation type rather than the terminal conflict classification. Preserve `AssetLockInputConflict` through `PlatformWalletFFIResult::from` while retaining the existing contextual `ErrorWalletOperation` fallback for unrelated errors, and add endpoint-level conversion tests.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:520-527: Kotlin omits the new terminal error from its public type mapping
JNI's `take_pwffi_error` preserves platform-wallet result codes by adding `PWFFI_CODE_OFFSET`, and the identity and platform-address resume APIs can now surface native code 41 as exception code 1041. `fromPlatformWalletNative` has no code-41 arm, however, so it falls through to `PlatformWallet.Generic`. This error carries destructive, non-retryable semantics and therefore meets this hierarchy's stated criterion for a dedicated type. Add `PlatformWallet.AssetLockInputConflict`, map code 41 to it, and test conversion from `DashSDKException(1041, ...)` so Kotlin callers can catch the terminal condition without inspecting `Generic.nativeCode`.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:373-378: Correct the Broadcast-state description
Code 41 can be returned for both `Built` and `Broadcast` locks. By definition, a `Broadcast` lock was sent during an earlier call, and `resume_asset_lock` normally performs a defensive rebroadcast for that state. The statement that "nothing was broadcast, nothing is in flight" is therefore false and can mislead hosts about the lock's history. State instead that conflict detection prevents the current resume from performing an additional broadcast or entering the proof wait. Apply the same correction to `PlatformWalletResult.swift:145-148` and `PlatformWalletResult.swift:419-425`.
| info.core_wallet | ||
| .transaction_history() | ||
| .into_iter() | ||
| .filter(|record| record.txid != lock_txid && record.is_confirmed()) |
There was a problem hiding this comment.
🔴 Blocking: Require chain-lock finality before declaring the asset lock terminal
TransactionRecord::is_confirmed() delegates to TransactionContext::confirmed(), which returns true for both InBlock and InChainLockedBlock. The pinned key-wallet implementation explicitly states that InBlock can be reorganized out and exposes is_chain_locked() as the finality predicate. A sibling found only in an ordinary block can therefore trigger AssetLockInputConflict and authorize permanent deletion of the tracked lock even though a reorg may remove that sibling and make the asset-lock transaction valid again. The positive test currently constructs exactly an InBlock context, so it codifies the unsafe terminal verdict. Restrict this destructive classification to chainlocked records and change the positive fixture to InChainLockedBlock.
| .filter(|record| record.txid != lock_txid && record.is_confirmed()) | |
| .filter(|record| record.txid != lock_txid && record.context.is_chain_locked()) |
source: ['codex']
| /// TERMINAL, and the only code here that authorises a host to discard | ||
| /// a tracked asset lock: nothing was broadcast, nothing is in flight, | ||
| /// and no retry of this outpoint can ever succeed. The remedy is to | ||
| /// drop the lock and build a new one from currently-unspent inputs. | ||
| /// Contrast `ErrorTransactionBroadcastUnconfirmed`, where the tx may | ||
| /// well be alive and discarding it would strand real funds. |
There was a problem hiding this comment.
🟡 Suggestion: Correct the Broadcast-state description
Code 41 can be returned for both Built and Broadcast locks. By definition, a Broadcast lock was sent during an earlier call, and resume_asset_lock normally performs a defensive rebroadcast for that state. The statement that "nothing was broadcast, nothing is in flight" is therefore false and can mislead hosts about the lock's history. State instead that conflict detection prevents the current resume from performing an additional broadcast or entering the proof wait. Apply the same correction to PlatformWalletResult.swift:145-148 and PlatformWalletResult.swift:419-425.
source: ['coderabbit']
There was a problem hiding this comment.
Resolved in this update — Correct the Broadcast-state description no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…terminal error A tracked asset lock whose funding input was already spent by a different confirmed transaction can never confirm: peers reject it as a double spend at the mempool boundary and relay nothing back, and Core has not sent BIP61 rejects by default since 0.17. `resume_asset_lock` would re-broadcast into that void and then sit in `wait_for_proof` — unbounded for the user-facing funding flows — so the app could not tell a dead lock from a slow network and had no basis to offer discarding it. Screen the `Built` and `Broadcast` arms for a confirmed transaction in the wallet's own history that spends one of the lock's inputs, and return `AssetLockInputConflict` (FFI code 41, mirrored in Swift) naming the input and the transaction that actually spent it. Settled statuses are left alone. The scan is conclusive in one direction only: a hit is a definite verdict, but under the default `keep-finalized-transactions = OFF` feature key-wallet evicts chainlocked records and keeps only their txids, so the oldest conflicts are invisible and the existing timeout stays the backstop for those. Prevention of the underlying build lives in key-wallet's spend-scan frontier gate and arrives with the next pin bump. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…code through every endpoint Review follow-ups. The chain-lock blocker is resolved by rationale rather than by gating: under the default keep-finalized-transactions=OFF build, apply_chain_lock evicts a record the moment a chainlock buries it, so restricting the verdict to is_chain_locked() records would leave the screen firing only in tests. The verdict stays on any confirmed sibling, and that is fund-safe: the conflicting spender is necessarily this wallet's own transaction (only this wallet can sign its outpoints), so discarding the conflicted lock strands nothing — after even a freak reorg the inputs return to the spendable set. The docs on the variant, the detection helper, and both host mirrors now carry this reasoning. - AssetLockInputConflict gains spender_chain_locked, computed from the record's context or the wallet's last_applied_chain_lock watermark (promotion is what evicts a record, so a surviving record is usually still InBlock after the boundary passed it); hosts can phrase their confidence accordingly, and a new fixture pins the chainlocked case. - The catch-up and shielded funding endpoints no longer flatten the conflict to ErrorWalletOperation: asset_lock_manager_catch_up_blocking and map_asset_lock_funding_result preserve code 42 (the catch-up pass is exactly where a restored wallet's dead lock surfaces). - Kotlin gains the typed PlatformWallet.AssetLockInputConflict arm for code 42 with a conversion test; the FFI code is pinned at 42 by test (41 was claimed by the shielded capacity preflight while this PR was open); stale Swift doc claims corrected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
356c6b1 to
7d9be71
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs (1)
150-161: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve typed asset-lock codes without changing timeout semantics.
map_asset_lock_funding_resultpreserves onlyAssetLockAlreadyConsumed(24) andAssetLockInputConflict(42). It mapsAssetLockNotTrackedandAssetLockFundingMismatchtoErrorWalletOperation(6). If catch-up should matchasset_lock_manager_resume, preserve the three remaining typed asset-lock variants explicitly, but keep unrelated timeout and wait errors at code 6. The Swift catch-up caller treats code 6 as an expected failure and discards it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs` around lines 150 - 161, Update the error mapping in map_asset_lock_funding_result to preserve the typed asset-lock result codes for AssetLockAlreadyConsumed, AssetLockInputConflict, AssetLockNotTracked, and AssetLockFundingMismatch. Keep unrelated timeout and wait errors mapped to ErrorWalletOperation (6), preserving the Swift catch-up caller’s existing timeout semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs`:
- Around line 150-161: Update the error mapping in map_asset_lock_funding_result
to preserve the typed asset-lock result codes for AssetLockAlreadyConsumed,
AssetLockInputConflict, AssetLockNotTracked, and AssetLockFundingMismatch. Keep
unrelated timeout and wait errors mapped to ErrorWalletOperation (6), preserving
the Swift catch-up caller’s existing timeout semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c76231a3-5150-4ca9-bbd3-521cc6cd60de
📒 Files selected for processing (8)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/rs-platform-wallet-ffi/src/asset_lock/sync.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-platform-wallet/src/error.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The follow-up preserves the dedicated conflict code through the catch-up, shielded, Swift, and Kotlin surfaces, and it corrects the Broadcast-state documentation. One blocking issue remains: a merely InBlock spender still produces the same terminal code that authorizes callers to discard the tracked asset lock, even though that spender can be removed by a reorganization.
Source: Codex general reviewer backend gpt-5.6-sol; Codex security-auditor reviewer backend gpt-5.6-sol; Codex ffi-engineer reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. Orchestration only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/asset_lock/sync/recovery.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:266: Require chain-lock finality before declaring the asset lock terminal
(existing thread: https://github.com/dashpay/platform/pull/4356#discussion_r3747137306)
`record.is_confirmed()` accepts both `TransactionContext::InBlock` and `InChainLockedBlock`, while the finality calculated at lines 275-278 is only reported and does not gate the result. The Rust, Swift, and Kotlin contracts define code 42 as terminal and explicitly authorize discarding the tracked lock regardless of whether the message reports `chainlocked: false`. An ordinary block can be reorganized out, at which point the sibling no longer spends the input and the previously signed tracked transaction can become valid again; for a `Broadcast` lock, a peer may also retain and replay the already-submitted transaction after the reorganization. The fact that both transactions were signed by this wallet means the value remains wallet-controlled, but it does not make permanent deletion of the original tracking state sound or make the terminal verdict true. Emit this destructive classification only when the record itself or the applied ChainLock boundary proves finality. If a non-final conflict must stop an unbounded wait, expose it through a distinct non-destructive result rather than code 42.
Open PR #4356 defines ErrorAssetLockInputConflict = 42 at its head with complete Swift/Kotlin mappings — the frontier this file advertised was already taken. Number-bearing side references now defer to the frontier note instead of naming a value that can go stale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
A tracked asset lock whose funding input was already spent by a different confirmed transaction can never confirm. Peers reject it as a double spend at the mempool boundary and relay nothing back, and Core has not sent BIP61
rejectmessages by default since 0.17, so the drop is completely silent.resume_asset_lockhad no way to see this. It would re-broadcast into the void and then sit inwait_for_proof— unbounded for the user-facing funding flows — leaving the condition indistinguishable from a slow network. The app had no basis on which to offer discarding the lock, so the funds it was meant to move stayed stranded with no error surfaced anywhere.Seen on testnet: a restored wallet built an identity top-up asset lock spending an outpoint that one of its own earlier asset locks had already consumed at height 1510203.
What was done?
resume_asset_locknow screens itsBuiltandBroadcastarms for a confirmed transaction in the wallet's own history that spends one of the lock's inputs, and returns a new terminalPlatformWalletError::AssetLockInputConflict { out_point, input, spent_by, height }naming the conflicting input and the transaction that actually spent it.InstantSendLocked/ChainLocked/RecoveredFromChain/Consumed) are explicitly excluded and a future status variant forces a decision here.ErrorAssetLockInputConflict, next free above the highest in-tree claim of 40; the nominally-free 28/30 are left vacated per the ledger convention in that file), with the ledger comment extended and a dedicated arm added to theFrom<PlatformWalletError>mapping so it no longer falls through toErrorUnknown. Mirrored throughPlatformWalletResult.swiftto a typed Swift case so a host can key a discard affordance off the case rather than off message text.Known limitation, documented on the detection helper: the scan is conclusive in one direction only. A hit is a definite verdict — confirmed spends of an outpoint are mutually exclusive. A miss proves nothing: under the default
keep-finalized-transactions = OFFfeature, key-wallet evicts the fullTransactionRecordonce a chainlock buries it and retains only the txid, so precisely the oldest and most likely conflicts are invisible. The existing timeout remains the backstop for those, and callers must not treat "no conflict" as proof of liveness.Scope: this makes a dead lock diagnosable and discardable. It does not stop one from being built — that prevention is a spend-scan frontier gate in key-wallet (dashpay/rust-dashcore#937) and arrives with the next pin bump.
How Has This Been Tested?
Unit tests in
recovery.rscovering: aBroadcastlock whose input is spent by a different confirmed record returns the typed error without re-broadcasting or hanging; an unconfirmed conflicting spend does not trigger it; the lock's own confirmed record is not mistaken for a conflict; and settled/proof-carrying locks keep their existing outcome.Each of the three guards was mutation-tested — removed individually, each makes exactly one test fail and no others.
cargo test -p platform-wallet asset_lockpasses (47 tests);cargo clippy -p platform-wallet -p platform-wallet-ffi --all-features --all-targetsandcargo fmt --all --checkclean.Breaking Changes
None. New error variant and a new FFI code in a fresh slot; no existing code or mapping changes meaning.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit