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 1 commit into
Conversation
…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>
📝 WalkthroughWalkthroughAsset-lock recovery now detects confirmed input conflicts before rebroadcast or proof waiting. Rust exposes the conflict as FFI result code 41, and Swift converts it into a typed wallet error 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 Network
AssetLockRecovery->>WalletTransactionHistory: inspect confirmed transaction inputs
WalletTransactionHistory-->>AssetLockRecovery: return conflicting spender details
AssetLockRecovery->>AssetLockRecovery: create AssetLockInputConflict
AssetLockRecovery-->>Network: skip broadcast and proof wait
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 356c6b1) |
![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
| /// 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.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the Broadcast-state description.
resume_asset_lock returns code 41 for both Built and Broadcast locks. A Broadcast lock was already sent before this resume call. Do not state that nothing was broadcast. State that conflict detection prevents an additional broadcast and proof wait.
packages/rs-platform-wallet-ffi/src/error.rs#L373-L378: replace “nothing was broadcast” with the no-additional-broadcast behavior.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L145-L148: use the same corrected behavior inPlatformWalletResultCode.packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L419-L425: use the same corrected behavior inPlatformWalletError.
📍 Affects 2 files
packages/rs-platform-wallet-ffi/src/error.rs#L373-L378(this comment)packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L145-L148packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift#L419-L425
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-ffi/src/error.rs` around lines 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.
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']
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