fix(swift-sdk): stop born-spent TXO rows at the persistence seam and reconcile the store after a full scan - #4638
fix(swift-sdk): stop born-spent TXO rows at the persistence seam and reconcile the store after a full scan#4638llbartekll wants to merge 3 commits into
Conversation
… persistence seam and expose a store-reconcile inventory A persister that derives its UTXO rows from record roles writes an UNSPENT row for an output the engine never credited — the coin was spent by a transaction with no wallet-owned output (a CoinJoin collateral burn) that was discarded before the coin was known (rust-dashcore#992) — and its own restore path then hands the phantom back to the engine on every launch (#4575). - `CoreChangeSet::utxo_credit_verdicts`: for every Received/Change output the owning account does not hold, why (observed spent at a height, doomed, uncredited), computed by the event bridge under its existing read lock. Absence means credited: today's behaviour. - A size-negotiated persistence extension slot, `on_persist_wallet_changeset_utxo_verdicts_fn`, fired BEFORE the changeset callback so the host has the verdicts while it materialises the round's `utxos_added`. `WalletChangeSetFFI` is frozen. - `wallet_utxos_page` / `classify_outpoints` accessors and their FFI, for a store reconcile after a full scan: a paged wallet inventory carrying the owning-account tuple, and a per-row verdict whose only actionable class — known-uncredited-owned — is the engine's own decision, not an absence. Both take the wallet lock outside the handle registry guard. Depends on dashpay/rust-dashcore#979 for the primary engine-side repair; compiles against the current pin. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ore against the engine after a full scan Consumes the credit-verdict slot: an output the engine skipped because a block was observed spending it (or whose record is doomed) is written spent at creation, and any verdict vetoes the redelivery clear that would otherwise resurrect it. Adds `reconcileCoreTxoStore(for:)`, gated on the SPV steady state, no latched sync fault, and the wallet's own watermark; it inserts validated, owned, mature engine coins the store lacks and marks a row spent only on the engine's known-uncredited-owned verdict. Never deletes, never un-marks, never acts on absence; idempotent, wallet-scoped, paged, deferred behind open Rust rounds, stopped by shutdown and delete. Runs automatically on the steady-state transition and every 30 minutes. Swift half not yet compiled on this branch: the xcframework build was interrupted by a full disk. Tests are written for the seam, the reconcile and its shutdown behaviour; a privacy test over the events is still to be added. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 49 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Makes the reconcile constants nonisolated so the synchronous runner can read them off the main actor, calls the static wallet resolver through the type, and advances the classify walk by the rows a page really left behind — a page that flipped entirely re-reads the same offset, which now holds rows the walk has not seen. Adds the privacy test over every new event (no address, txid, outpoint, script, long hex or Base58 run) and the shutdown/race tests, and makes the fixtures restorable the way the load path requires. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
🕓 Queued for automated review — 4th in line, estimated start in ~55 min (commit d7515ac)
|
romchornyi
left a comment
romchornyi
left a comment
There was a problem hiding this comment.
Full pass over the three commits (c098ac1c80, b6197b6114, d7515acefd) — the Rust accessors, the FFI surface, the changeset seam and the Swift reconcile.
The core mechanism holds up. The verdict is computed under the read lock the bridge already takes, the extension slot is size-negotiated and a slotless host behaves exactly as before, the heal pass is insert-only and the classify pass flip-only, and isSpent stays monotonic. I also checked the parts that are easy to get wrong and found them clean: wallet_utxos_page cursor semantics (Bound::Excluded, has_more across empty and trailing accounts, the limit == 0 / over-max clamps) terminate correctly and the Swift hasMore && rows.last loop cannot spin; the WalletUtxoEntryFFI address/script allocations are freed symmetrically with no leak on the early-return-empty path; OutPointFFI byte order round-trips through the new From<&OutPointFFI>; there is no onQueue-within-onQueue reentrancy and no lock cycle between serialQueue and the engine's wallet lock; and a verdict-spent row still gets its spender link later, since adoptSpendObservation does not gate on isSpent.
Two findings are marked inline. Both end the same way — a live coin written or flipped spent, and a spent row is never restored — so they are worth settling before this lands.
The rest are non-blocking, take them or leave them:
PlatformWalletPersistenceHandler.swift:1404—persistWalletChangesetUtxoVerdictswas inserted between@discardableResultand thepersistWalletChangesetSweepsdoc comment and signature it belonged to. The attribute now applies to the new function, sweeps lost it, and the sweeps doc block ("Returnsfalseto fail the round…") sits above an unrelated attribute. It compiles only because no caller currently discards the sweeps result.PlatformWalletPersistenceHandler.swift:10938—reconcileUnspentTxoPagehas noisWatchOnlyContactAccountfilter, unlike the heal pass at 10860. A row filed under a DashPay external account (tag 13), which pre-#926 builds did persist, goes toclassify_outpoints; the owning account recognises the script and knows the txid, so if the coin is not in that account'sfunds.utxosthe answer isKnownUncreditedand the row is flipped. The asymmetry between the two passes looks unintended whichever way you resolve it.PlatformWalletManagerTxoReconcile.swift:322—coreTxoReconcileLastRunAt[walletId] = nowis stamped at schedule time, before any gate runs. A wallet that skips for a transient reason is then not retried for 30 minutes — andwalletBehindTipis a likely skip on the steady-state rising edge, since the durable watermark commonly trailsfilters.currentHeightby more than 6 at that moment. If the rising edge does not recur, the trigger only fires whenspvProgresschanges value (PlatformWalletManager.swift:3039), so it may not run at all that session. Stamping on the.reconciledpath only would make the retry immediate.PlatformWalletPersistenceHandler.swift:10947— the classify walk is an offset page ordered bySortDescriptor(\.createdAt)with no unique tiebreaker. Rows from one changeset round sharecreatedAtclosely enough for ties, and SwiftData's order among ties is unspecified between fetches, sooffset += page.fetched - flippedcan step over rows that reshuffled across a page boundary. Harmless per run — the pass is idempotent and re-runs — but a single run does not actually cover the whole store. A secondary sort onoutpointmakes it exact.CoreTxoReconcileTypes.swift:43—dashpayExternalAccountTag: UInt8 = 13is hardcoded rather than read fromACCOUNT_TYPE_TAG_FFI_DASHPAY_EXTERNAL_ACCOUNT. Correct today (wallet_restore_types.rs:56), but this is the single gate keeping the heal pass from filing a contact's coins as the user's, and a renumbering would silently repoint it atPlatformPayment.
| // Credit verdicts: newest wins per outpoint. Every verdict in a | ||
| // drain is computed against the same wallet snapshot, so two | ||
| // batches folding together cannot disagree about a coin. | ||
| self.utxo_credit_verdicts.extend(other.utxo_credit_verdicts); |
There was a problem hiding this comment.
The merge's premise does not hold: verdicts folded into one round come from different wallet snapshots, so a stale ObservedSpent can survive and write a live coin spent.
The comment above this line says every verdict in a drain is computed against the same wallet snapshot. It isn't. utxo_credit_verdicts (core_bridge.rs:1261) takes its own wallet_manager.read().await, and it runs inside build_core_changeset, which is per event — so two events folded into one round are projected against two snapshots taken at different times.
extend is newest-wins only for outpoints the newer map mentions. A coin that was uncredited when event A was projected and credited by the time event B was projected is simply absent from B's map, because the verdict function only records outputs the account does not hold. So A's ObservedSpent survives the fold.
Downstream that is not a cosmetic disagreement: upsertUtxo writes the row spent at creation, nothing later un-marks a spent row, and loadWalletList restores only isSpent == false rows — so the coin leaves the store for good at the next launch. This is the same class of loss the PR exists to fix, arriving through the new path.
Two ways out, either fine: take the wallet snapshot once per drain and project every event against it, or make the merge erase a verdict when a later batch that covers the same record carries none for that outpoint.
| if !page.rows.isEmpty { | ||
| let classes: [CoreOutpointClass] | ||
| do { | ||
| classes = try engine.classify(page.rows.map(\.query)) |
There was a problem hiding this comment.
The class is read on one queue and applied on another; a coin re-credited in the gap is flipped spent on a stale verdict.
engine.classify runs here on coreTxoReconcileQueue, and the result is applied in a separate serialQueue closure (reconcileApplyEngineClasses, line 253). Between the two the engine can legitimately re-credit one of these coins — a reorg of the spender delivers it back in utxos_added as unspent.
The apply side guards only guard let txo = fetchTxoRow(...), !txo.isSpent, which is still false for such a row, so the stale knownUncredited flips a coin the engine currently holds. inChangeset does not cover it either: it defers while a round is open, and the dangerous case is a round that opened and completed inside the gap.
The flip is one-way — nothing un-marks a spent row, and the restore only replays isSpent == false — so this is a durable loss, not a transient miscount.
Re-checking the class inside the same serialQueue closure would close it; so would carrying an engine generation (or the row's lastUpdated as read at classify time) across the gap and refusing the flip when it moved.
3 participants
Issue being fixed or feature implemented
Tracking: #4575. Residual engine class: dashpay/rust-dashcore#992. Android precedent: #4439.
A mainnet CoinJoin-heavy wallet ends every full historical scan with the Rust engine correct and SwiftData wrong, and every relaunch re-injects the wrong SwiftData rows into the engine:
Reproduced, on a support wallet, from the diagnostics export (three SDK sessions,
platform-wallet 4.2.0-dev.8, engine built with #979):utxo_count=0; SwiftDatadatabase_only_count=4(19,549 duffs each, heights 2,391,743 / 2,391,786 / 2,402,896 / 2,402,986); owned-output audittotal_anomaly_count=0Blocks: processed: 0)emitted_count=4 emitted_value_duffs=78196; engineutxo_count=4 confirmed_duffs=78196; diffcommon_count=4; UI shows 0.00078196 DASHSo even a clean rebuild from seed with #979 produces the four phantom rows: they are born wrong in the persister and then become its authoritative restore source. Each of the four coins was spent on-chain by a CoinJoin collateral burn — a transaction whose sole output is
OP_RETURN— that the engine processed while the coin was not yet in its UTXO set. The burn matched nothing and was discarded (rust-dashcore#992); the funding record, (re)emitted later, still classified the outputReceived; the FFI projection derivesutxos_addedfrom record roles (record_new_utxos_ffi), so the persister wrote an unspent row for a coin the engine — guarded by the #649observed_spentmap inupdate_utxos— never credited. Nothing later corrects it: the spender has no record, no spend emit, no sweep, and iOS had no store↔engine reconcile.Where the evidence lives, and why an inventory-only reconcile is not enough
Verified in the pinned engine:
observed_spent_outpoints(feat(wasm-dpp): implement asset lock proof bindings #649) records every input of every block transaction, with its height, and is pruned at the finality boundary (prune_finalized_observed_spends), so historical spends are gone long before a scan ends.spent_outpointsset survives the scan but only ever receives inputs of transactions that matched the account — the nightly: fix masternode identities and other fixes. #992 spender never did — and is context-free (mempool spends included), which is exactly why the reviewer of fix(kotlin-sdk): reconcile the TXO store against the engine and repair restored address pools #4439 blocked flipping on it.At end-of-scan the engine therefore holds no spend evidence for the four rows. What it does hold is its own verdict on the coin: the owning account knows the funding txid (
has_transaction/transaction_is_finalized), recognises the output's script as its own (contains_script_pub_key), and does not hold the coin. Underupdate_utxos's rules an owned output of a known record is absent only because the engine skipped it for a spent/doomed reason or consumed it. That verdict is available at the moment the funding record is emitted (Part 1) and for the rest of the scanning session, until a restart empties the finalized set (Part 2). Neither needs #979's accessor.What was done?
Part 1 — credit verdicts at the changeset seam (prevention)
CoreChangeSet.utxo_credit_verdicts: BTreeMap<OutPoint, UtxoCreditVerdict>(ObservedSpent { height },Doomed,Uncredited), computed by the event bridge for everyReceived/Changeoutput of the round's owned slices that the owning account does not hold, under the read lock the bridge already takes per event. Absence means credited — today's behaviour, byte for byte.on_persist_wallet_changeset_utxo_verdicts_fn(WalletChangeSetFFIis frozen), fired inside the round before the changeset callback, only on rounds that carry a verdict. A host without the slot behaves exactly as before.upsertUtxoconsults the round's verdicts:observed_spent/doomedrows are written spent at creation (no spender link — the spender was never recorded), any verdict vetoes the redelivery "recovery clear",uncreditedchanges nothing else. Onepersistence_txo_credit_verdictsevent per round, counts only.For the field sequence above this makes the rows spent at creation wherever the engine saw the spending block before the funding block (verified on the support wallet, see the manual verification below); it cannot cover a spending block dash-spv never delivers (dashpay/rust-dashcore#1006).
Part 2 — post-scan store reconcile (safety net + heal), Swift SDK
wallet_utxos_page(paged(AccountType, OutPoint)walk with the owning-account tuple, address and confirmation flags; page cap enforced natively — the inventory's size is chain-controlled) andclassify_outpoints(Unknown/Unspent/KnownUncredited/NotOwned, costqueries × accounts, never inventory size). Names and shapes follow fix(kotlin-sdk): reconcile the TXO store against the engine and repair restored address pools #4439's Rust side so the two PRs converge. FFI:platform_wallet_wallet_utxos_page(+_free),platform_wallet_classify_outpoints; both take the wallet lock outside the handle registry's guard (thesync_progressshape), so a caller parked behind block processing never stallsdestroy.PlatformWalletManager.reconcileCoreTxoStore(for:)— gated on SPV running and in steady state (dash-spv's fully synced state iswaitForEventswith the filter phase at its target;.syncedis transient), no latched sync fault (syncFaultDetected()), and the wallet's own durable watermark within 6 blocks of the scan tip. Runs automatically on the steady-state transition and every 30 minutes; hosts may also call it. Engine reads on a dedicated queue; store steps on the persistence queue, each its own closure, deferred while a Rust round is open; stops between pages whenshutdown()ordeleteWalletbumps its epoch.upsertUtxoinserts it, only when validated (32-byte txid, script, address), owned (account row resolved by the seven-field tuple; never filed unowned — the restore loader routes by account), not a contact's watch-only chain, and ≥ 100 confirmations.isSpent == falserows are classified in batches; onlyknownUncreditedmarks a row spent (and drops pending-input claims on it).unspent,unknown,notOwnedare counted, never acted on..referencedigests only — no txid, outpoint, address or script.Repair path for already-affected devices
A from-seed rebuild is fixed by Part 1 for the spend-before-funding ordering; the never-delivered-spender class (dashpay/rust-dashcore#1006) survives a from-seed rebuild until the engine-side fix dashpay/rust-dashcore#1007 lands. A store that already holds phantom rows is healed by an in-place from-birth rescan (verified below): the phantom is restored into
utxos, so the collateral burn now matches by input and its spend reaches the store through the ordinaryutxos_spentchannel; Part 2 covers silent leftovers in the same session.How Has This Been Tested?
cargo test -p platform-wallet --lib: 987 + 7 new — bridge (the nightly: fix masternode identities and other fixes. #992 shape: burn processed before its funding ⇒ObservedSpentat the burn height; doomed mempool record; credited ⇒ no verdict; end to end throughbuild_core_changesetwith an unknown wallet yielding nothing), changeset merge, inventory paging, classification of unspent / known-uncredited / not-owned / unknown across the arrival orders that produce each.cargo test -p platform-wallet-ffi --lib: 335, including the new slot (fires before the changeset callback, never when empty, slotless host still succeeds), the extension layout pin (the verdict slot is now terminal), and struct-size gating of the new slot.SwiftTests/SwiftDashSDKTests):BornSpentTxoPersistTests(verdict ⇒ row spent and absent from the restore; doomed; uncredited leaves it unspent; verdict vetoes the redelivery clear; round-scoped; rolled-back round leaves no row; restart: a file-backed store reopened twice restores zero coins),CoreTxoReconcileTests(positive verdict ⇒ flipped; absent from both ⇒ unchanged; missing engine coin ⇒ inserted with stub parent, account, address; immature / foreign / unresolved / malformed refused; steady-state gate; idempotent; wallet-scoped; repeated relaunch restores nothing; consistent store ⇒ zero mutations; never un-marked; failed read stops the run; paging across both passes),CoreTxoReconcileShutdownTests(cancelled before / mid-run; deferred behind an open round and completed after it; refused after shutdown),CoreTxoReconcilePrivacyTests(every new event rendered through the SDK's file sink with realistic fixtures: no address, txid or outpoint in either byte orientation, no script, no 32+ hex run, no address-length Base58 run).swift build -Xswiftc -warnings-as-errorsclean;swift test: 512 tests, 0 failures (14 pre-existing skips).xcodebuild build -scheme SwiftExampleApp -destination 'generic/platform=iOS Simulator' ARCHS=arm64: BUILD SUCCEEDED against the rebuiltDashSDKFFI.xcframework(dev profile, sim + mac slices).Manual verification on the support wallet (2026-09-09, iOS Simulator, release-ios FFI with rust-dashcore#979 applied locally)
rescan_filtersfrom birth, uninterruptedutxos_spentchannel with a spender link; balance 0;persistence_txo_reconcile_summary:engine_row_count=0 store_row_count=0, zero mutationspersistence_txo_credit_verdicts: 2 756 outputs written spent at creation (observed_spent), 592 already spent — Part 1 works wherever the spending block was applied before the funding block. 11 rows (one 0.1 + ten 0.001 CoinJoin denominations, 0.11 DASH) remain unspent in both the engine and the store. Their 4 spending blocks were never matched or applied by dash-spv in that session, so the engine holds no evidence; the reconcile correctly reportsengine_row_count=11 store_row_count=11 unspent_count=11and flips nothing. Root cause: dash-spv dropped the scripts derived by re-applied blocks (297 of 3 645), fixed in dashpay/rust-dashcore#1007engine_row_count=0 store_row_count=0, 0 unspent rows, balance 0; restart restores nothingrescan_filtersfrom birthSo on the current pin the from-seed rebuild is not fully fixed by Part 1: the residual class is an engine-side discovery gap (dash-spv never delivers the spending block, so neither the #649 map nor
spent_outpointsever sees the spend), dashpay/rust-dashcore#1006, fixed by dashpay/rust-dashcore#1007 — with that fix applied the from-seed rebuild converges to zero (last row above). This PR is correct and safe on that class — it never marks anything on absence and reports the disagreement in counts — but it cannot repair it;rescan_filtersfrom birth does, in one pass. The paragraph "A from-seed rebuild is fixed by Part 1 alone" above is therefore too strong: Part 1 fixes the spend-before-funding ordering (verified: the rows for the one spending block that was applied early were born spent and stayed spent), not the never-delivered-spender case.Known gap (not addressed here): an initial scan interrupted mid-sweep resumes into the same state, see the same issue.
Acceptance
After a from-seed rebuild and a restart of the fixture wallet, SwiftData and the engine both hold zero unspent TXOs for the four burned coins and the UI stays at zero; a second reconcile run reports zero mutations.
Breaking Changes
None. One additive persistence-extension slot (size-negotiated, ignored by older hosts), two additive FFI functions, one public Swift API.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code