fix(platform-wallet): give the conflict screen a source that survives the load - #4404
Conversation
… the load The screen reads `core_wallet.transaction_history()`, which the FFI load path deliberately leaves empty apart from the unresolved locks' own funding records. `catchUpStuckAssetLocks` runs at app launch, before block sync repopulates anything — so at the one moment the screen runs it has nothing to scan, and a lock that is provably a double spend sails past it. Measured on a testnet device: a lock at `Built` spending an outpoint that a chain-locked transaction had taken at height 1532949 produced `history_len=1`, and that one record was the lock's own funding tx, which `record.txid != lock_txid` filters out. Zero candidates. The resume then re-broadcast into the void and sat in `wait_for_proof` for the full 300s — the behaviour this screen exists to prevent. The existing tests pass because they populate the history first, so the gap is specific to the load path. The host mirror already knows: the persisted row for a spent outpoint records which transaction took it, with height and context. Carry that across the FFI into `restored_asset_lock_input_spends` and let the screen consult it before falling back to the history scan. Only a spender that reached a block settles an outpoint — a mempool sighting can still be replaced — and that judgement is made Rust-side from the context the host passes through verbatim. In-session behaviour is unchanged; the fallback scan still runs and still wins when the history is populated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
🕓 Ready for review — 3 ahead in queue (commit c82369f) |
`WalletRestoreEntryFFI` gained two fields and the Kotlin JNI host builds that struct too, so the Android build broke on E0063. Null them: the Kotlin persister has no equivalent of the Swift spend-linkage query yet, so the conflict screen keeps its previous transaction-history behaviour on that host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The restore plumbing addresses the startup visibility gap, but the new load query uses a SwiftData relationship predicate that the model explicitly identifies as crash-prone, and the FFI decoder can turn unknown context values into false terminal conflicts. The newly introduced restored-map path and its safety gates also need focused regression coverage.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5457-5459: Avoid the crash-prone relationship predicate during wallet load
`PersistentTxo` explicitly documents that predicates traversing its optional `spendingTransaction` relationship enter a SwiftData nested-optional path that crashes, which is why the scalar `isSpent` column exists. This query runs during wallet loading, and `try?` cannot recover from a process-level SwiftData crash. The conflict reader only accepts restored rows whose spender reached a block, while the persistence handler sets `isSpent` under that same condition, so filtering on the scalar retains every row the current Rust conflict screen can use without touching the unsafe predicate path.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:4893-4895: Do not classify unknown context values as chain-locked
The FFI contract defines only context discriminants 0 through 3, but these ordered comparisons classify every malformed or future value, including 4 and `u32::MAX`, as both confirmed and chain-locked. That is unsafe because `first_confirmed_input_conflict` treats `in_block` as conclusive evidence and returns the terminal conflict code that allows the host to discard the tracked lock. Match only the known block-context discriminants so invalid persisted data degrades to the previous no-evidence behavior instead of manufacturing finality.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:268-275: Cover the newly added empty-history conflict path
The existing conflict tests insert spender records into `transaction_history()`, while every test fixture initializes `restored_asset_lock_input_spends` as empty. Add focused tests with empty history and a restored spend for the funded input: a confirmed different spender must return `AssetLockInputConflict` without broadcasting, while an unconfirmed spender and the lock's own txid must not trigger the terminal error. The FFI decoder should likewise cover unknown context values, because that boundary determines whether persisted evidence can condemn a lock.
| predicate: #Predicate { txo in | ||
| txo.walletId == walletId && txo.spendingTransaction != nil | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Avoid the crash-prone relationship predicate during wallet load
PersistentTxo explicitly documents that predicates traversing its optional spendingTransaction relationship enter a SwiftData nested-optional path that crashes, which is why the scalar isSpent column exists. This query runs during wallet loading, and try? cannot recover from a process-level SwiftData crash. The conflict reader only accepts restored rows whose spender reached a block, while the persistence handler sets isSpent under that same condition, so filtering on the scalar retains every row the current Rust conflict screen can use without touching the unsafe predicate path.
| predicate: #Predicate { txo in | |
| txo.walletId == walletId && txo.spendingTransaction != nil | |
| } | |
| predicate: #Predicate { txo in | |
| txo.walletId == walletId && txo.isSpent == true | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in 48afef9 — Avoid the crash-prone relationship predicate during wallet load 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.
| height: (row.spender_height != 0).then_some(row.spender_height), | ||
| in_block: row.spender_context >= CONTEXT_IN_BLOCK, | ||
| chain_locked: row.spender_context >= CONTEXT_IN_CHAIN_LOCKED_BLOCK, |
There was a problem hiding this comment.
🔴 Blocking: Do not classify unknown context values as chain-locked
The FFI contract defines only context discriminants 0 through 3, but these ordered comparisons classify every malformed or future value, including 4 and u32::MAX, as both confirmed and chain-locked. That is unsafe because first_confirmed_input_conflict treats in_block as conclusive evidence and returns the terminal conflict code that allows the host to discard the tracked lock. Match only the known block-context discriminants so invalid persisted data degrades to the previous no-evidence behavior instead of manufacturing finality.
| height: (row.spender_height != 0).then_some(row.spender_height), | |
| in_block: row.spender_context >= CONTEXT_IN_BLOCK, | |
| chain_locked: row.spender_context >= CONTEXT_IN_CHAIN_LOCKED_BLOCK, | |
| height: (row.spender_height != 0).then_some(row.spender_height), | |
| in_block: matches!(row.spender_context, 2 | 3), | |
| chain_locked: row.spender_context == 3, |
source: ['codex']
There was a problem hiding this comment.
Resolved in 48afef9 — Do not classify unknown context values as chain-locked 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.
| if let Some((input, spend)) = lock_inputs.iter().find_map(|input| { | ||
| info.restored_asset_lock_input_spends | ||
| .get_key_value(input) | ||
| .filter(|(_, spend)| spend.spender != lock_txid && spend.in_block) | ||
| }) { | ||
| return Some((*input, spend.spender, spend.height, spend.chain_locked)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: Cover the newly added empty-history conflict path
The existing conflict tests insert spender records into transaction_history(), while every test fixture initializes restored_asset_lock_input_spends as empty. Add focused tests with empty history and a restored spend for the funded input: a confirmed different spender must return AssetLockInputConflict without broadcasting, while an unconfirmed spender and the lock's own txid must not trigger the terminal error. The FFI decoder should likewise cover unknown context values, because that boundary determines whether persisted evidence can condemn a lock.
source: ['codex']
There was a problem hiding this comment.
Resolved in 48afef9 — Cover the newly added empty-history conflict path 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.
…erage **The spend-linkage query could crash the process.** It filtered on `spendingTransaction != nil`, and `PersistentTxo.isSpent` exists specifically because chasing that optional relationship in a predicate drops SwiftData onto a nested-optional codepath that crashes — which `try?` cannot recover from, during wallet load. Filter on the scalar instead. Nothing is lost: `isSpent` flips under the same in-block condition the conflict screen requires of a spender, so every row the screen can act on is still included. **Unknown context bytes read as final.** `>= 2` and `>= 3` classified every malformed or forward-versioned value, `u32::MAX` included, as both confirmed and chain-locked. The screen treats `in_block` as conclusive and returns a terminal code the host may act on by discarding the lock, so manufacturing that verdict from a corrupt row is unsafe. Match the known discriminants exactly; anything else degrades to no evidence. **Coverage for the path this PR adds.** Three tests: the restored linkage condemning a lock with an empty history (the app-launch shape), a non-final spender and the lock's own txid both declining to condemn it, and the decoder's context boundary across 0..=3 plus unknown values. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current head fixes all three prior findings: the SwiftData query now uses the safe scalar predicate, the Rust decoder accepts only known context discriminants, and focused regression tests cover the empty-history restore path and decoder boundary. Two new Swift load-path blockers remain: the arbitrary 4096-row fetch limit can omit the conflict this PR needs to restore, and cleanup deinitializes more spend-linkage elements than were initialized.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5463-5464: Do not cap an unfiltered set before selecting the lock inputs
This query selects an unspecified first 4096 confirmed spent TXOs for the wallet, while only Rust later intersects those rows with the unresolved asset locks' input outpoints. `PersistentTxo` retains spent outputs for wallet history, so a wallet can legitimately exceed this limit, and neither a predicate nor a sort gives relevant lock inputs priority. If the required outpoint falls outside the returned subset, startup again has no evidence of the confirmed conflict and `resume_asset_lock` enters the full proof wait—the load-path failure this PR is intended to fix. Remove the cap or constrain the Swift query to the unresolved locks' input outpoints before applying a bound.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5469-5493: Track only initialized spend-linkage buffer elements
The allocation is registered with `spent.count` before rows with a missing `spendingTransaction` or malformed txid are skipped. `LoadAllocation.release()` consequently calls `deinitialize(count:)` for the full fetched count even though only `written` elements were initialized. When every row is skipped, the function returns `(nil, 0)` but retains a wholly uninitialized buffer for later deinitialization. This violates `UnsafeMutablePointer`'s initialized-memory contract; the allocation tracker elsewhere in this file explicitly documents that deinitializing uninitialized slots is undefined behavior. Deallocate immediately when no rows were written and otherwise register exactly the initialized count.
| descriptor.relationshipKeyPathsForPrefetching = [\.spendingTransaction] | ||
| descriptor.fetchLimit = Self.assetLockInputSpendCap |
There was a problem hiding this comment.
🔴 Blocking: Do not cap an unfiltered set before selecting the lock inputs
This query selects an unspecified first 4096 confirmed spent TXOs for the wallet, while only Rust later intersects those rows with the unresolved asset locks' input outpoints. PersistentTxo retains spent outputs for wallet history, so a wallet can legitimately exceed this limit, and neither a predicate nor a sort gives relevant lock inputs priority. If the required outpoint falls outside the returned subset, startup again has no evidence of the confirmed conflict and resume_asset_lock enters the full proof wait—the load-path failure this PR is intended to fix. Remove the cap or constrain the Swift query to the unresolved locks' input outpoints before applying a bound.
source: ['codex']
There was a problem hiding this comment.
Resolved in 84eddfc — Do not cap an unfiltered set before selecting the lock inputs 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.
| let buf = UnsafeMutablePointer<AssetLockInputSpendFFI>.allocate(capacity: spent.count) | ||
| allocation.assetLockInputSpendBuffers.append((buf, spent.count)) | ||
| var written = 0 | ||
| for txo in spent { | ||
| guard let spender = txo.spendingTransaction else { continue } | ||
|
|
||
| guard txo.txid.count == 32, spender.txid.count == 32 else { continue } | ||
| var row = AssetLockInputSpendFFI() | ||
| txo.txid.withUnsafeBytes { src in | ||
| Swift.withUnsafeMutableBytes(of: &row.prev_txid) { dst in | ||
| dst.copyMemory(from: src) | ||
| } | ||
| } | ||
| row.vout = txo.vout | ||
| spender.txid.withUnsafeBytes { src in | ||
| Swift.withUnsafeMutableBytes(of: &row.spender_txid) { dst in | ||
| dst.copyMemory(from: src) | ||
| } | ||
| } | ||
| row.spender_height = spender.blockHeight | ||
| row.spender_context = spender.context | ||
| buf[written] = row | ||
| written += 1 | ||
| } | ||
| return written == 0 ? (nil, 0) : (buf, written) |
There was a problem hiding this comment.
🔴 Blocking: Track only initialized spend-linkage buffer elements
The allocation is registered with spent.count before rows with a missing spendingTransaction or malformed txid are skipped. LoadAllocation.release() consequently calls deinitialize(count:) for the full fetched count even though only written elements were initialized. When every row is skipped, the function returns (nil, 0) but retains a wholly uninitialized buffer for later deinitialization. This violates UnsafeMutablePointer's initialized-memory contract; the allocation tracker elsewhere in this file explicitly documents that deinitializing uninitialized slots is undefined behavior. Deallocate immediately when no rows were written and otherwise register exactly the initialized count.
| let buf = UnsafeMutablePointer<AssetLockInputSpendFFI>.allocate(capacity: spent.count) | |
| allocation.assetLockInputSpendBuffers.append((buf, spent.count)) | |
| var written = 0 | |
| for txo in spent { | |
| guard let spender = txo.spendingTransaction else { continue } | |
| guard txo.txid.count == 32, spender.txid.count == 32 else { continue } | |
| var row = AssetLockInputSpendFFI() | |
| txo.txid.withUnsafeBytes { src in | |
| Swift.withUnsafeMutableBytes(of: &row.prev_txid) { dst in | |
| dst.copyMemory(from: src) | |
| } | |
| } | |
| row.vout = txo.vout | |
| spender.txid.withUnsafeBytes { src in | |
| Swift.withUnsafeMutableBytes(of: &row.spender_txid) { dst in | |
| dst.copyMemory(from: src) | |
| } | |
| } | |
| row.spender_height = spender.blockHeight | |
| row.spender_context = spender.context | |
| buf[written] = row | |
| written += 1 | |
| } | |
| return written == 0 ? (nil, 0) : (buf, written) | |
| let buf = UnsafeMutablePointer<AssetLockInputSpendFFI>.allocate(capacity: spent.count) | |
| var written = 0 | |
| for txo in spent { | |
| guard let spender = txo.spendingTransaction else { continue } | |
| guard txo.txid.count == 32, spender.txid.count == 32 else { continue } | |
| var row = AssetLockInputSpendFFI() | |
| txo.txid.withUnsafeBytes { src in | |
| Swift.withUnsafeMutableBytes(of: &row.prev_txid) { dst in | |
| dst.copyMemory(from: src) | |
| } | |
| } | |
| row.vout = txo.vout | |
| spender.txid.withUnsafeBytes { src in | |
| Swift.withUnsafeMutableBytes(of: &row.spender_txid) { dst in | |
| dst.copyMemory(from: src) | |
| } | |
| } | |
| row.spender_height = spender.blockHeight | |
| row.spender_context = spender.context | |
| buf[written] = row | |
| written += 1 | |
| } | |
| if written == 0 { | |
| buf.deallocate() | |
| return (nil, 0) | |
| } | |
| allocation.assetLockInputSpendBuffers.append((buf, written)) | |
| return (buf, written) |
source: ['codex']
There was a problem hiding this comment.
Resolved in 84eddfc — Track only initialized spend-linkage buffer elements 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.
The load callback fetched the wallet's confirmed spent TXOs, capped at 4096, and left Rust to intersect them with the unresolved locks' input outpoints. Nothing orders that set, so a wallet with more spent history than the cap could return a page missing the very outpoint the conflict screen needs — startup back to no evidence and a full proof wait, the failure this branch exists to fix. Resolve the outpoints first, from the locks' persisted funding transactions, and fetch exactly those: one point lookup each on the unique `outpoint` key, so the cap has nothing left to protect. `PersistentTransaction.inputs` cannot answer this, being the inverse of `spendingTransaction` — for the case that matters, an outpoint taken by a different transaction, it names the winner and omits the lock's edge. Every other condition now runs in Swift on the fetched row rather than in the predicate: `isSpent` still gates the row, `spendingTransaction` is read but never chased in a predicate, and no captured collection has to survive SwiftData's translation on the load path. Also register the FFI buffer for the number of rows actually written. It was registered for the fetched count while rows without a spender or with a malformed txid were skipped, so `release()` would deinitialize uninitialized memory.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commit fixes both prior blockers by selecting only unresolved-lock inputs and initializing exactly the tracked buffer length. The restored conflict path still skips valid locks when their standalone transaction row is absent, and its finality report fails to apply the restored chain-lock boundary.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5520-5529: Decode inputs from the asset-lock row instead of requiring a transaction row
Every `PersistentAssetLock` stores the consensus-encoded funding transaction in `transactionBytes`, and `buildAssetLockRestoreBuffer` uses those bytes to restore the tracked lock. This helper instead requires a separate `PersistentTransaction` row for the lock txid and skips the lock when that row is missing or empty. The same file explicitly recognizes that a Built/Broadcast asset-lock row can exist when its own transaction never reached the transaction table. In that state, the input TXO can still be linked to a different confirmed spender, but this early skip prevents the outpoint lookup and leaves the restored conflict map blind, reproducing the startup proof-wait failure this PR is intended to fix. Decode the authoritative bytes already stored on the asset-lock row.
In `packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs:268-273: Apply the restored chain-lock boundary to restored spends
The restored path reports only `spend.chain_locked`, which reflects the persisted transaction context. A transaction row can remain `InBlock` after `last_applied_chain_lock` advances beyond its block height; that is why the history fallback immediately below combines the record context with `chain_locked_height`. The same restored metadata is available here, so an InBlock restored spend at or below that boundary is final even when its row was never promoted to context 3. Without the same fallback, the terminal error incorrectly exposes `spender_chain_locked: false` to the host.
| guard let outpoint = decodeOutPointHex(lock.outPointHex) else { continue } | ||
| let txidData = Data(outpoint.prefix(32)) | ||
| var txDescriptor = FetchDescriptor<PersistentTransaction>( | ||
| predicate: #Predicate { $0.txid == txidData } | ||
| ) | ||
| txDescriptor.fetchLimit = 1 | ||
| guard let txRow = try? backgroundContext.fetch(txDescriptor).first, | ||
| !txRow.transactionData.isEmpty, | ||
| let decoded = try? TransactionDecoder.decode(txRow.transactionData, network: network) | ||
| else { continue } |
There was a problem hiding this comment.
🔴 Blocking: Decode inputs from the asset-lock row instead of requiring a transaction row
Every PersistentAssetLock stores the consensus-encoded funding transaction in transactionBytes, and buildAssetLockRestoreBuffer uses those bytes to restore the tracked lock. This helper instead requires a separate PersistentTransaction row for the lock txid and skips the lock when that row is missing or empty. The same file explicitly recognizes that a Built/Broadcast asset-lock row can exist when its own transaction never reached the transaction table. In that state, the input TXO can still be linked to a different confirmed spender, but this early skip prevents the outpoint lookup and leaves the restored conflict map blind, reproducing the startup proof-wait failure this PR is intended to fix. Decode the authoritative bytes already stored on the asset-lock row.
| guard let outpoint = decodeOutPointHex(lock.outPointHex) else { continue } | |
| let txidData = Data(outpoint.prefix(32)) | |
| var txDescriptor = FetchDescriptor<PersistentTransaction>( | |
| predicate: #Predicate { $0.txid == txidData } | |
| ) | |
| txDescriptor.fetchLimit = 1 | |
| guard let txRow = try? backgroundContext.fetch(txDescriptor).first, | |
| !txRow.transactionData.isEmpty, | |
| let decoded = try? TransactionDecoder.decode(txRow.transactionData, network: network) | |
| else { continue } | |
| guard !lock.transactionBytes.isEmpty, | |
| let decoded = try? TransactionDecoder.decode( | |
| lock.transactionBytes, | |
| network: network | |
| ) | |
| else { continue } |
source: ['codex']
There was a problem hiding this comment.
Resolved in 1fcf166 — Decode inputs from the asset-lock row instead of requiring a transaction row 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.
| if let Some((input, spend)) = lock_inputs.iter().find_map(|input| { | ||
| info.restored_asset_lock_input_spends | ||
| .get_key_value(input) | ||
| .filter(|(_, spend)| spend.spender != lock_txid && spend.in_block) | ||
| }) { | ||
| return Some((*input, spend.spender, spend.height, spend.chain_locked)); |
There was a problem hiding this comment.
🟡 Suggestion: Apply the restored chain-lock boundary to restored spends
The restored path reports only spend.chain_locked, which reflects the persisted transaction context. A transaction row can remain InBlock after last_applied_chain_lock advances beyond its block height; that is why the history fallback immediately below combines the record context with chain_locked_height. The same restored metadata is available here, so an InBlock restored spend at or below that boundary is final even when its row was never promoted to context 3. Without the same fallback, the terminal error incorrectly exposes spender_chain_locked: false to the host.
| if let Some((input, spend)) = lock_inputs.iter().find_map(|input| { | |
| info.restored_asset_lock_input_spends | |
| .get_key_value(input) | |
| .filter(|(_, spend)| spend.spender != lock_txid && spend.in_block) | |
| }) { | |
| return Some((*input, spend.spender, spend.height, spend.chain_locked)); | |
| let spender_chain_locked = spend.chain_locked | |
| || chain_locked_height | |
| .zip(spend.height) | |
| .is_some_and(|(boundary, spender_height)| spender_height <= boundary); | |
| return Some((*input, spend.spender, spend.height, spender_chain_locked)); |
source: ['codex']
There was a problem hiding this comment.
Resolved in 1fcf166 — Apply the restored chain-lock boundary to restored spends 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.
Two findings on 84eddfc. `unresolvedAssetLockInputs` decoded the funding transaction out of a `PersistentTransaction` row, and skipped the lock when that row was missing. A Built / Broadcast asset lock whose own transaction never reached the transaction table is exactly the state this path exists for — its input can still have been taken by a confirmed spender — so the skip left the restored conflict map blind and startup back in the proof wait this branch is fixing. Decode `PersistentAssetLock.transactionBytes` instead: the authoritative copy, the same bytes `buildAssetLockRestoreBuffer` hands Rust, and one fetch fewer. `first_confirmed_input_conflict` reported a restored spender's finality from `spend.chain_locked` alone. A persisted row keeps the context it was stored with, and the promotion that would flip it to chain-locked is precisely what the restore path never runs — so a spender mined below a chainlock the wallet applied later read as not final, understating the evidence the host acts on. Apply the same boundary fallback the history scan a few lines down already uses.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current head resolves both carried findings by decoding unresolved inputs from each asset-lock row's authoritative transaction bytes and applying the persisted chain-lock boundary to restored spends. One blocking load-path issue remains: migrated spent TXOs with an empty denormalized wallet ID are discarded before their valid conflict evidence reaches Rust, reproducing the startup proof wait for existing wallets.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:5465-5470: Accept migrated TXOs whose wallet ID has not been backfilled
`PersistentTxo.walletId` explicitly defaults to empty for rows migrated from the older schema, and the established restore path already resolves those legacy rows through their account relationship. The spend-reconciliation path can set `isSpent` and `spendingTransaction` without backfilling that field, so a confirmed conflicting spender can be present while this strict equality guard still discards the row. Because the point lookup uses the globally unique outpoint decoded from this wallet's unresolved asset-lock transaction, accepting the documented empty migration value cannot introduce an unrelated row; rejecting it leaves the restored conflict map empty and sends startup recovery back into the full proof wait this PR is intended to prevent.
| guard let txo = try? backgroundContext.fetch(descriptor).first, | ||
| txo.walletId == walletId, | ||
| txo.isSpent, | ||
| let spender = txo.spendingTransaction, | ||
| txo.txid.count == 32, | ||
| spender.txid.count == 32 |
There was a problem hiding this comment.
🔴 Blocking: Accept migrated TXOs whose wallet ID has not been backfilled
PersistentTxo.walletId explicitly defaults to empty for rows migrated from the older schema, and the established restore path already resolves those legacy rows through their account relationship. The spend-reconciliation path can set isSpent and spendingTransaction without backfilling that field, so a confirmed conflicting spender can be present while this strict equality guard still discards the row. Because the point lookup uses the globally unique outpoint decoded from this wallet's unresolved asset-lock transaction, accepting the documented empty migration value cannot introduce an unrelated row; rejecting it leaves the restored conflict map empty and sends startup recovery back into the full proof wait this PR is intended to prevent.
| guard let txo = try? backgroundContext.fetch(descriptor).first, | |
| txo.walletId == walletId, | |
| txo.isSpent, | |
| let spender = txo.spendingTransaction, | |
| txo.txid.count == 32, | |
| spender.txid.count == 32 | |
| guard let txo = try? backgroundContext.fetch(descriptor).first, | |
| (txo.walletId == walletId || txo.walletId.isEmpty), | |
| txo.isSpent, | |
| let spender = txo.spendingTransaction, | |
| txo.txid.count == 32, | |
| spender.txid.count == 32 | |
| else { continue } |
source: ['codex']
There was a problem hiding this comment.
Resolved in f962c06 — Accept migrated TXOs whose wallet ID has not been backfilled 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.
…y does The spend-linkage lookup compared `PersistentTxo.walletId` raw. That column is documented as empty on rows written before it existed, and the spend-reconciliation path sets `isSpent` and the spender link without backfilling it — so the comparison discarded exactly the legacy rows a confirmed conflicting spender is recorded on. The restored conflict map came back empty and startup went into the full proof wait this branch exists to prevent. Resolve through `resolvedWalletId`, the fallback `loadWalletList` already uses for the same reason. That is narrower than accepting any empty value: a legacy row whose account belongs to a different wallet is still rejected. Adds the first coverage for this path — the confirmed spender is restored both when the TXO carries its wallet id and when it does not. The second test was confirmed to fail against the raw comparison.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact head fixes the prior migrated-TXO ownership blocker by resolving ownership through the account relationship when the denormalized wallet ID is empty, with a focused Swift test exercising the real load callback. The restored conflict path also conservatively handles context discriminants, applies the chain-lock boundary, and has focused Rust coverage; no in-scope defects remain.
Source: reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
Review — head
|
…pshot The restored map is a load-time snapshot nothing ever demotes, so it must not shadow fresher evidence. The conflict screen now runs the live history scan first; the snapshot fills only the gaps no other source covers (app-launch catch-up, chainlock-evicted spenders), a row whose spender the live history has re-observed unconfirmed is treated as stale rather than trusted, and a snapshot height no longer claims chainlock finality via the boundary fallback — a persisted height cannot prove its block survived to be buried, so only the mirror's own observed chainlock context may carry that confidence tier. The conflict itself still fires either way. Also reattaches PlatformWalletInfo's rustdoc (absorbed by the RestoredSpend insertion), drops the documented-but-nonexistent second reader from RestoredSpend's doc, replaces the stale 3-tuple field docs, and removes two unused imports left over from that revision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…validate them for real WalletRestoreEntryFFI crosses the boundary as a bare pointer with no size or version tag, so appending is the only layout change that keeps every earlier field at its old offset; the two spend-row fields were inserted mid-struct and now sit at the tail, with the append-only discipline documented on the struct. The malformed-row skip previously guarded a Txid parse that cannot fail on a fixed 32-byte array — the branch now rejects the one shape a broken host actually produces (zeroed txids), so the load-safety contract in the doc is real. The restored-row log no longer calls the rows conflicts (the host emits the locks' own spends too; conflict-ness is decided per lock by the screen), the struct docs describe what the host actually sends, the persisted context discriminants are named constants shared by every context_raw decoder, and the identity-restore doc block is reattached to the function it describes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d by the outpoint resolveInputOutpoint's isSpent is now monotonic — a later mempool sighting of a different spender no longer downgrades a flag an in-block spend set, which would also have blanked the spend-linkage evidence the conflict screen restores at the next launch (the guard mirrors the same line on the sweep-persistence branch, so the eventual merge is a no-op). The emitted row's outpoint fields now come from the 36-byte key the fetch matched on rather than the row's computed txid property, so a corrupt relationship cannot turn the keyed exact match into a guess; a row skipped on ownership grounds logs instead of vanishing; a legacy wallet with an unresolved network keeps its evidence (the decoder's network argument only shapes address rendering this caller discards); and the builder doc now says what is actually emitted — settled spends only, the lock's own spend included. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ount A count assertion alone lets a wrong-source copy — swapped txids, a context read off the wrong transaction — ship green on both sides of the FFI. The new test reads the emitted row back and pins every field to the fixture's distinct values, and the fixture's outpoint encoding now goes through PersistentAssetLock.encodeOutPoint so it cannot drift from the format the load path reads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes pushed —
|
4 participants
Issue being fixed or feature implemented
Follow-up to #4356, targeting its branch — see #4356 (comment) for the original report.
The conflict screen #4356 adds cannot fire on the iOS load path, because it reads state that path does not have.
first_confirmed_input_conflictscansinfo.core_wallet.transaction_history(), and the FFI load path deliberately leavestransactions()empty apart from the unresolved locks' own funding records (restore_unresolved_asset_lock_tx_records).catchUpStuckAssetLocksruns at app launch, before block sync repopulates anything — so at the one moment the screen runs, it has nothing to scan.Measured on a testnet device with a temporary diagnostic at the call site:
history_len=1, and that single record is the lock's own funding transaction, whichrecord.txid != lock_txidfilters out. Zero candidates.This was a textbook case for the screen:
8789cd69…:1had already been taken by8c19e1c2…, confirmed and chain-locked at height 1532949 (verified against a testnet node). The screen returnedNone, the resume re-broadcast into the void, andwait_for_proofran the full 300s — the exact behaviour #4356 sets out to eliminate.The PR's own tests pass because they populate the transaction history first, so the gap is specific to the load path rather than the logic.
What was done?
The host mirror already knows the answer — the persisted row for a spent outpoint records which transaction took it, with height and context — it just never crosses the FFI.
AssetLockInputSpendFFIrow onWalletRestoreEntryFFI: outpoint, spender txid, spender height, and the spender'sTransactionContextdiscriminant passed through verbatim. The host marshals; deciding which contexts count as final is Rust's call, perpackages/swift-sdk/CLAUDE.md.PlatformWalletInfo::restored_asset_lock_input_spendsviaClientWalletStartState. Only a spender that reached a block settles an outpoint — a mempool or InstantSend-only sighting can still be replaced, so it is no basis for calling another transaction dead.first_confirmed_input_conflictconsults that map first, then falls back to the existing history scan.PersistentTxorows that carry aspendingTransaction, capped at 4096 rows, freed with the rest of the load allocation.In-session behaviour is unchanged — the fallback scan still runs and still wins whenever the history is populated. A malformed row is skipped rather than failing the load: this is evidence for a screen that degrades to its previous behaviour without it, so a bad row must not cost the user their wallet.
How Has This Been Tested?
cargo test -p platform-wallet --lib asset_lock::— 51 passed, including fix(platform-wallet): fail a double-spending asset lock with a typed terminal error #4356's own conflict suite unchanged.cargo check -p platform-wallet --tests,cargo check -p platform-wallet-fficlean;./build_ios.sh --target simsucceeds (Swift compiles against the regenerated header).That lock was the root of a three-transaction chain holding 1.57 DASH of phantom balance on that wallet, so the screen firing is what lets the whole chain be discarded — it earns its keep well beyond the error message.
Breaking Changes
None. Additive fields on an FFI struct that already grows this way; hosts that do not populate them get the previous behaviour exactly.
Checklist:
Two things I want to flag rather than quietly leave:
No new test. The gap is that a real load produces an empty history, and the existing suite constructs its wallets in memory with the history already populated — so a unit test asserting "the map is consulted" would not reproduce the condition that made the screen blind. Reproducing it properly needs a restore-from-persistence harness. I verified on device instead, and would rather say so than add a test that passes for the wrong reason. Happy to add one if you can point me at the right harness.
I only checked the iOS path. If the Kotlin load path repopulates
transactions()on startup, this is iOS-specific and the framing above is broader than it should be.