fix(swift-sdk): act on swept transactions in the SwiftData store - #4589
Open
romchornyi wants to merge 1 commit into
Open
fix(swift-sdk): act on swept transactions in the SwiftData store#4589romchornyi wants to merge 1 commit into
romchornyi wants to merge 1 commit into
Conversation
romchornyi
requested review from
QuantumExplorer,
ZocoLini,
llbartekll and
shumkov
as code owners
September 2, 2026 20:28
Contributor
|
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: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
6 tasks
Collaborator
|
🕓 Ready for review — 54 ahead in queue (commit 1fb1a17) |
The SwiftData mirror of the storage contract, complicated by two things SQLite does not have: rows shared across wallets, and a round that now spans two callbacks. Shared rows are why a sweep marks rather than deletes. A transaction row can belong to several wallets, so the first wallet's callback cannot remove it — it sets `isGloballySwept`, which excludes the row and its outputs from every restore and enumeration path, and the physical delete is left to housekeeping once every wallet's scoped cleanup has landed. A tombstone must likewise outlive its loser: detach it and the consumed coin reads unspent again. Held inputs become pending-input tombstones carrying the winner and, when it was mined, its height; a chained sweep repoints an earlier tombstone at the new winner rather than stacking a second hold. The release pass is outpoint-keyed, the drain gives tombstones precedence over ordinary observations, and `isSpent` stays monotonic against them: a hold the sweep proved consumed is never downgraded by a later record — not even the winner's own, which can arrive IS-locked, a context below in-block. `autosaveEnabled` goes off on the round context. Sweeps travel in their own callback, so the round spans two calls, and an autosave landing between them would make the watermark and the additive rows durable while the removal is still unstaged — with `rollback()` unable to take back a save that already happened. The handler attests `ATOMIC_CHANGESETS`, and Rust now relies on that to trust the split transport, so the guarantee has to be real. The handler declares `CORE_SWEEP_REMOVAL` and `DASHPAY_PAYMENTS`; before this commit it published the legacy `struct_size`, the negotiated slot read `None`, and Rust fail-closed. ## Schema V4, and the freeze it required The four models that gain a column — `PersistentTransaction`, `PersistentTxo`, `PersistentPendingInput`, `PersistentWallet` — were still referenced live by `DashSchemaV1/V2/V3`. Adding a property to a live model mutates those released versions' checksums in place, so a store written by a shipped binary would match no registered schema and fail to open with Cocoa 134504 instead of migrating. That is exactly the defect `DashSchemaFrozenModels.swift` was introduced to prevent, and its instruction is to freeze the model you change. Freezing those four alone is not possible: a frozen model declares its relationships against frozen counterparts (an `inverse:` key path is typed on the destination model), and following relationships in both directions closes over 24 of the 35 models — one type per entity name is all a schema can hold, so the component travels together. All 24 are frozen here at their V3 shape, shared by V1, V2 and V3, none of which changed any of them. The eleven models outside the component are still live-referenced and still carry the latent defect, unchanged by this. `DashSchemaV4` then registers the live models with a lightweight V3→V4 stage: every new column is additive with a default or optional, so existing rows migrate as not-swept, unsuperseded, ordinary unstamped claims, and a wallet with no chainlock boundary yet. ## Merge with #4356 #4356 landed first and rewrote the same three regions. Its `reconcileSpendObservation` stays the single spend verdict, extended with one sweep term — a stamped hold outranks any observation — and its oldest-first pending-row reconciliation stays, under a tombstone-precedence branch. One correction the merge forced: the "never displace confirmed evidence" rule refused the link when `isSpent` was true with NO spender linked, which is precisely the sweep-hold shape, so the winner's own record could never supply the attribution the hold lacked. With no link there is nothing to displace, so it is adopted. One gap neither PR covered is closed here: `buildUnresolvedAssetLockTxRecordBuffer` now skips globally-swept rows, so the double-spend screen can never be handed a swept loser as the settled spender of a lock's input. Also carries the `ChangesetRoundIndex` per-round fetch cache — the reviewed-but-untested fix for the quadratic SwiftData fetch that put ~99% of CPU on the serial queue. Sweep paths deliberately opt out of it, since they key on mutable columns the index cannot answer stale. Tests: `SweptTransactionPersistTests` (38) — shared losers, detached tombstones with a missing winner row, chained tombstones, cross-round reinstatement, released-pending deadlock, co-swept twins, the throwing-lookup round failure, and the winner's late record against a stamped hold. `DashModelMigrationTests` gains the V3→V4 stage and reads V1/V2 rows through the frozen types. Full suite: 437 tests, the only failures being two `KeychainSignerAdditionalSigningKeysTests` cases that fail identically on an unmodified checkout (the test host cannot write to the keychain).
romchornyi
force-pushed
the
split/4406-3-producer
branch
from
September 3, 2026 08:12
267ecca to
48db83c
Compare
romchornyi
force-pushed
the
split/4406-5-swift
branch
from
September 3, 2026 08:12
8918079 to
1fb1a17
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Until this lands, the Swift host publishes the legacy
struct_size, the negotiated sweeps slot readsNone,CORE_SWEEP_REMOVALis withheld, and Rust fail-closes: an iOS wallet freezes its sync watermark on the first sweep it meets rather than diverging. Funds-safe, but a user-visible stall — this is the PR that ends it.What was done?
The store
SwiftData rows can be shared across wallets, so a sweep marks rather than deletes:
isGloballySweptexcludes the row and its outputs from every restore and enumeration path, and the physical delete is left to housekeeping once every wallet's scoped cleanup has landed. A tombstone must likewise outlive its loser — detach it and the consumed coin reads unspent again.Held inputs become pending-input tombstones carrying the winner and, when it was mined, its height. A chained sweep repoints an earlier tombstone at the new winner rather than stacking a second hold. The release pass is outpoint-keyed, the drain gives tombstones precedence over ordinary observations, and
isSpentstays monotonic against them: a hold the sweep proved consumed is never downgraded by a later record — not even the winner's own, which can arrive IS-locked, a context below in-block.autosaveEnabledgoes off on the round context. Sweeps travel in their own callback, so a round now spans two calls, and an autosave landing between them would make the watermark and the additive rows durable while the removal is still unstaged — withrollback()unable to take back a save that already happened. The handler attestsATOMIC_CHANGESETSand Rust relies on that to trust the split transport, so the guarantee has to be real.Schema V4, and the freeze it required
The four models that gain a column (
PersistentTransaction,PersistentTxo,PersistentPendingInput,PersistentWallet) were still referenced live byDashSchemaV1/V2/V3. Adding a property to a live model mutates those released versions' checksums in place, so a store written by a shipped binary matches no registered schema and fails to open with Cocoa 134504 instead of migrating — exactly whatDashSchemaFrozenModels.swiftwas introduced to prevent, and its instruction is to freeze the model you change.Freezing those four alone is not possible: a frozen model declares its relationships against frozen counterparts (an
inverse:key path is typed on the destination model), and following relationships in both directions closes over 24 of the 35 models — a schema holds one type per entity name, so the component travels together. All 24 are frozen at their V3 shape, shared by V1/V2/V3, none of which changed any of them. The eleven models outside the component remain live-referenced and still carry the latent defect, unchanged by this PR.DashSchemaV4then registers the live models with a lightweight V3→V4 stage: every new column is additive with a default or optional, so existing rows migrate as not-swept, unsuperseded, ordinary unstamped claims, and a wallet with no chainlock boundary yet.Merge with #4356
#4356 landed first and rewrote the same three regions. Its
reconcileSpendObservationstays the single spend verdict, extended with one sweep term — a stamped hold outranks any observation — and its oldest-first pending-row reconciliation stays, under a tombstone-precedence branch.One correction the merge forced: the "never displace confirmed evidence" rule refused the link when
isSpentwas true with no spender linked, which is precisely the sweep-hold shape, so the winner's own record could never supply the attribution the hold lacked. With no link there is nothing to displace, so it is adopted.One gap neither PR covered is closed here:
buildUnresolvedAssetLockTxRecordBufferskips globally-swept rows, so the double-spend screen can never be handed a swept loser as the settled spender of a lock's input.Also carried
The
ChangesetRoundIndexper-round fetch cache — the reviewed-but-untested fix for the quadratic SwiftData fetch that put ~99% of CPU on the serial queue. Sweep paths deliberately opt out of it, since they key on mutable columns the index cannot answer stale. Named explicitly because it is the one piece here without dedicated tests.How Has This Been Tested?
xcodebuild test -scheme SwiftDashSDK -destination 'platform=iOS Simulator,name=iPhone 17'— 437 tests, andswift buildclean under the package's-warnings-as-errors.SweptTransactionPersistTests(38): shared losers, detached tombstones with a missing winner row, chained tombstones, cross-round reinstatement, released-pending deadlock, co-swept twins, the throwing-lookup round failure, and the winner's late record against a stamped hold.DashModelMigrationTests: gainstestV3StoreMigratesToV4AndBackfillsTheSweepColumns, and its V1/V2 cases now write and read through the frozen types.InvitationPersistenceTeststracks the new capability mask.The only failures on this machine are two
KeychainSignerAdditionalSigningKeysTestscases, which fail identically on an unmodified checkout — the barexcodebuildrun has no writable keychain, whichrun_tests.shprovides in CI.Breaking Changes
None at the API surface. Schema V4 is a lightweight migration; the freeze exists specifically so V1/V2/V3 stores keep opening.
Checklist:
For repository code-owners and collaborators only