Skip to content

Commit d86a763

Browse files
llbartekllclaude
llbartekll
and
claude
committed
fix(swift-sdk): never claim drift from a comparison that did not happen
`storeSchemaVerdict` returned `driftedRegisteredVersion` — the one verdict that authorizes inferred lightweight migration — for a store whose metadata carried no entity hashes at all: nothing disagreed because nothing was compared, and the store would have been opened by inference and trimmed to the current schema. Drift now requires at least one entity actually compared and at least one actually disagreeing. Stores that cannot be placed no longer borrow the newer-build error either. `no_version_identifier` is an old or truncated store as much as a new one, and `storeFromNewerBuild` tells the user to update the app or reset the wallet — destructive advice on a store that is fine. A new `.unplaceable` verdict rethrows SwiftData's own error instead, with the verdict in the log. `shutdown()` raised the diagnostics cancellation after the handle guard, so a pass running for a never-configured manager — the branch that deliberately runs its database half without a handle — could not be told to stop, and held the persistence queue across teardown. Cancel before the guard. The #4438 audit reported a clean, complete result when the persisted BIP44 address pool was empty: every output fell through as unattributed and nothing reached the missing-TXO check, on exactly the wallet whose address rows went missing. An empty pool is now an incompleteness like an undecodable transaction. Deliberately not `unattributed_output_count > 0`: a CoinJoin transaction pays its peers, so every healthy audit has some. Also: an autorelease pool per account in the memory half, matching the database half — the whole loop is one GCD work item, so the peak was the sum of every account rather than the largest; and the restore snapshot no longer classifies every row on the errored path, where it faulted a relationship per row, at launch, under the queue, to describe a load being discarded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 668a707 commit d86a763

5 files changed

Lines changed: 176 additions & 68 deletions

File tree

‎packages/swift-sdk/Sources/SwiftDashSDK/Persistence/DashModelContainer.swift‎

Lines changed: 37 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -79,13 +79,22 @@ public enum DashModelContainer {
7979
/// wrote, so it must not run; the pre-fallback crash was the safe
8080
/// outcome here.
8181
case newerThanRegistered(reason: String)
82+
/// The metadata reads but does not place the store against any
83+
/// registered version: no declared version identifier, or nothing to
84+
/// compare it by. Inferred migration must not answer this either — an
85+
/// unplaced store opened by inference is trimmed to the current schema
86+
/// exactly like a downgrade — but it is NOT evidence of a newer build,
87+
/// so the host must not be told to update or reset. SwiftData's own
88+
/// error is passed through instead.
89+
case unplaceable(reason: String)
8290

8391
var logLabel: String {
8492
switch self {
8593
case .unreadable: return "unreadable"
8694
case .matchesRegisteredVersion: return "matches_registered_version"
8795
case .driftedRegisteredVersion: return "drifted_registered_version"
8896
case .newerThanRegistered(let reason): return "newer_than_registered:\(reason)"
97+
case .unplaceable(let reason): return "unplaceable:\(reason)"
8998
}
9099
}
91100
}
@@ -168,8 +177,19 @@ public enum DashModelContainer {
168177
if let unknown = storeVersionIdentifiers.first(where: { !registeredIdentifiers.contains($0) }) {
169178
return .newerThanRegistered(reason: "unregistered_version_identifier=\(unknown)")
170179
}
180+
// No identifier at all places the store nowhere. Older stores and
181+
// stores with truncated metadata land here too, so this is not a
182+
// newer build and must not be reported to the user as one.
171183
guard !storeVersionIdentifiers.isEmpty else {
172-
return .newerThanRegistered(reason: "no_version_identifier")
184+
return .unplaceable(reason: "no_version_identifier")
185+
}
186+
// Drift is a statement about hashes that disagree. With no hashes to
187+
// read there is nothing to disagree, and every check below would pass
188+
// vacuously — `disagreeing` empty, so `unknownShapes` empty, so
189+
// `driftedRegisteredVersion` from a comparison that never happened,
190+
// authorizing inferred migration over a store nothing is known about.
191+
guard !storeEntityHashes.isEmpty else {
192+
return .unplaceable(reason: "no_entity_hashes")
173193
}
174194

175195
// An entity the current schema does not have can only have been
@@ -195,11 +215,20 @@ public enum DashModelContainer {
195215
let unknownShapes = Set(disagreeing.compactMap { name, hash in
196216
knownDriftedEntityHashes[name] == hash ? nil : name
197217
})
198-
if unknownShapes.isEmpty {
218+
// `!disagreeing.isEmpty` is the load-bearing half: drift is what
219+
// the fallback answers, and a store that agrees on every hash it
220+
// carries and still is not compatible differs by something these
221+
// hashes do not describe — an entity the store lacks entirely,
222+
// say. Whatever that is, it is not the drift the pinned hashes
223+
// authorize, so it does not get inferred migration.
224+
if !disagreeing.isEmpty, unknownShapes.isEmpty {
199225
return .driftedRegisteredVersion
200226
}
201227
unexpectedDrift.formUnion(unknownShapes)
202228
}
229+
guard !unexpectedDrift.isEmpty else {
230+
return .unplaceable(reason: "no_entity_disagreement")
231+
}
203232
return .newerThanRegistered(
204233
reason: "unexpected_entity_drift=\(unexpectedDrift.sorted().joined(separator: "|"))"
205234
)
@@ -475,9 +504,12 @@ public enum DashModelContainer {
475504
guard case .driftedRegisteredVersion = verdict else {
476505
report(succeeded: false, migrationPath: .staged, error: error, storeVerdict: verdict)
477506
// A newer build's store is the one refusal the host can act
478-
// on (tell the user to update or reset), so it gets a typed
479-
// error; everything else is SwiftData's own failure, passed
480-
// through untouched.
507+
// on, so it gets a typed error — and only it, because that
508+
// error's text tells the user to update the app or reset the
509+
// wallet, and resetting is destructive on a store that is
510+
// merely unplaceable. `.unplaceable` and `.unreadable` are
511+
// SwiftData's own failure, passed through untouched with the
512+
// verdict in the log.
481513
if case .newerThanRegistered(let reason) = verdict {
482514
throw DashModelContainerError.storeFromNewerBuild(reason: reason)
483515
}

‎packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWalletDiagnosticAnalyzers.swift‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,10 +240,17 @@ enum CoreWalletDiagnosticAnalyzer {
240240
/// the launch restore path while the persistence queue is held, so a large
241241
/// CoinJoin wallet must not pay for a dozen full-length `filter`/`map`
242242
/// allocations, and nothing here retains a per-row array.
243+
/// - Parameter candidateCountOverride: reported instead of the number of
244+
/// candidates walked. For the errored path, where classifying each row
245+
/// would fault a relationship per row to describe a load that is being
246+
/// discarded: the caller passes the row count it already has and an
247+
/// empty sequence, so `candidate_count` stays truthful and every other
248+
/// counter is honestly zero.
243249
static func summarizeRestoreBuffer<S: Sequence>(
244250
candidates: S,
245251
emittedCount: Int,
246-
errored: Bool
252+
errored: Bool,
253+
candidateCountOverride: Int? = nil
247254
) -> RestoreBufferSummary where S.Element == RestoreCandidate {
248255
// Rust is handed the first `emittedCount` rows that passed validation,
249256
// in order; an errored build deallocated the whole buffer, so none of
@@ -318,7 +325,7 @@ enum CoreWalletDiagnosticAnalyzer {
318325
}
319326

320327
return RestoreBufferSummary(
321-
candidateCount: candidateCount,
328+
candidateCount: candidateCountOverride ?? candidateCount,
322329
candidateValueDuffs: candidateValue,
323330
candidateBip44Count: candidateBip44Count,
324331
candidateBip44ValueDuffs: candidateBip44Value,

‎packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -673,6 +673,14 @@ public class PlatformWalletManager: ObservableObject {
673673
if let task = shutdownTask {
674674
return await task.value
675675
}
676+
// Before the handle guard, not after it: a diagnostic pass can be
677+
// running on the persistence queue for a manager that was never
678+
// configured (`emitCoreWalletDiagnostics` runs its database half
679+
// with no handle), and the early return below would leave it with
680+
// no way to be told to stop — holding the queue, and every Rust
681+
// persister callback entering through it, across teardown. The
682+
// flag is one-way and costs nothing on the no-op path.
683+
coreDiagnosticsCancellation.cancel()
676684
guard handle != NULL_HANDLE else {
677685
// Never configured (or a test double without a handle):
678686
// nothing to tear down. Do not cache this no-op: a manager

‎packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerCoreDiagnostics.swift‎

Lines changed: 107 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -720,19 +720,41 @@ extension PlatformWalletPersistenceHandler {
720720
rejectionReason: rejection
721721
)
722722
}
723-
// Lazily, and with the built rows first: the summary's emission window
724-
// is positional, so the rejected rows must not shift it. Nothing here
725-
// materializes a per-row array — this runs while the launch restore
726-
// holds the persistence queue.
727-
let candidates = [rows, accountLessRows].lazy.flatMap { $0 }.map(candidate)
728723
// A validation error deallocates the compact buffer and aborts the
729724
// whole callback, so zero rows were actually handed to Rust even if
730725
// some valid rows preceded the corrupt one.
731-
let summary = CoreWalletDiagnosticAnalyzer.summarizeRestoreBuffer(
732-
candidates: candidates,
733-
emittedCount: emittedCount,
734-
errored: errored
735-
)
726+
//
727+
// It also means classifying the rows would cost more than it is worth:
728+
// `buildUtxoRestoreBuffer` can bail at row 0 having faulted nothing,
729+
// and `candidate` touches `account` and `txid` — to-one relationships
730+
// — on every row, so the walk would issue a fault per row, at launch,
731+
// with the persistence queue held, to describe a load that is about to
732+
// be discarded. The row that failed is already named with its reason
733+
// by `persistence_wallet_load_validation_failed`; here the count is
734+
// what is left to say, and the positional emission window the walk
735+
// exists for is moot at zero emitted.
736+
let summary: CoreWalletDiagnosticAnalyzer.RestoreBufferSummary
737+
if errored {
738+
summary = CoreWalletDiagnosticAnalyzer.summarizeRestoreBuffer(
739+
candidates: EmptyCollection<
740+
CoreWalletDiagnosticAnalyzer.RestoreCandidate
741+
>(),
742+
emittedCount: 0,
743+
errored: true,
744+
candidateCountOverride: rows.count + accountLessRows.count
745+
)
746+
} else {
747+
// Lazily, and with the built rows first: the summary's emission
748+
// window is positional, so the rejected rows must not shift it.
749+
// Nothing here materializes a per-row array — this runs while the
750+
// launch restore holds the persistence queue, and every row it
751+
// touches was already faulted by the build it is reconciling.
752+
summary = CoreWalletDiagnosticAnalyzer.summarizeRestoreBuffer(
753+
candidates: [rows, accountLessRows].lazy.flatMap { $0 }.map(candidate),
754+
emittedCount: emittedCount,
755+
errored: false
756+
)
757+
}
736758
let hasRejectedRows = summary.missingAccountCount > 0
737759
|| summary.invalidTxidCount > 0
738760
|| summary.invalidAccountTypeCount > 0
@@ -1057,15 +1079,29 @@ extension PlatformWalletPersistenceHandler {
10571079
let missingValue = diagnosticSaturatingSum(anomalies.compactMap {
10581080
$0.reason == "missing_txo" ? $0.amount : nil
10591081
})
1082+
// With no persisted BIP44 addresses nothing can be attributed to this
1083+
// wallet: every decoded output falls into `unattributedOutputCount`,
1084+
// no output reaches the `missing_txo` check, and the summary would
1085+
// otherwise read as a clean, complete audit — on a wallet whose
1086+
// address rows are exactly what went missing. The pool being empty is
1087+
// an incompleteness of the same kind as an undecodable transaction.
1088+
//
1089+
// `unattributedOutputCount > 0` is deliberately NOT part of this: a
1090+
// CoinJoin-spending transaction pays its peers, and their outputs are
1091+
// unattributable by construction, so every healthy audit has some.
1092+
// A partially lost pool is not distinguishable from a small one here;
1093+
// `bip44_address_pool_size` sits beside this flag for that reading.
1094+
let addressPoolEmpty = bip44Addresses.isEmpty
1095+
let auditIncomplete = decodeFailureCount > 0
1096+
|| transactionBytesMissingCount > 0
1097+
|| addressPoolEmpty
10601098
SDKLogger.event(
10611099
"core_owned_output_audit_summary",
10621100
category: .persistence,
1063-
severity: anomalies.isEmpty && decodeFailureCount == 0
1064-
&& transactionBytesMissingCount == 0 ? .info : .warning,
1101+
severity: anomalies.isEmpty && !auditIncomplete ? .info : .warning,
10651102
fields: [
1066-
"audit_incomplete": .boolean(
1067-
decodeFailureCount > 0 || transactionBytesMissingCount > 0
1068-
),
1103+
"audit_incomplete": .boolean(auditIncomplete),
1104+
"bip44_address_pool_empty": .boolean(addressPoolEmpty),
10691105
"bip44_address_pool_size": .integer(Int64(bip44Addresses.count)),
10701106
"candidate_transaction_count": .integer(Int64(candidateCount)),
10711107
"checkpoint": .publicText(checkpoint.rawValue),
@@ -1456,62 +1492,73 @@ extension PlatformWalletManager {
14561492
)
14571493
}
14581494
for balance in sortedBalances {
1459-
let key = Self.diagnosticAccountKey(balance)
14601495
if shutdownBegan(before: "account_utxos") { return }
1461-
let query = diagnosticAccountUtxos(
1462-
managerHandle: managerHandle,
1463-
walletId: walletId,
1464-
balance: balance
1465-
)
1466-
guard case .success(let utxos) = query else {
1467-
unavailableAccounts.insert(key)
1496+
// One pool per account, matching `emitCoreWalletDatabaseDiagnostics`.
1497+
// libdispatch drains its own pool once per work item, and this
1498+
// whole loop is one work item: without this, every account's
1499+
// per-UTXO txid and scriptPubKey copies, its fingerprint material
1500+
// and the formatter each log event allocates all stay resident
1501+
// until the export ends, so the peak is the sum of every account
1502+
// rather than the largest one. `memoryTxos` is returned out of the
1503+
// pool on purpose — `compareDatabase` needs the whole set.
1504+
let accountTxos: [CoreWalletDatabaseDiagnosticSnapshot.Txo]? = autoreleasepool {
1505+
let key = Self.diagnosticAccountKey(balance)
1506+
let query = diagnosticAccountUtxos(
1507+
managerHandle: managerHandle,
1508+
walletId: walletId,
1509+
balance: balance
1510+
)
1511+
guard case .success(let utxos) = query else {
1512+
unavailableAccounts.insert(key)
1513+
SDKLogger.event(
1514+
"core_memory_account_snapshot",
1515+
category: .persistence,
1516+
severity: .warning,
1517+
fields: [
1518+
"account_reference": .reference(key.referenceMaterial),
1519+
"account_type": .unsignedInteger(UInt64(key.typeTag)),
1520+
"checkpoint": .publicText(checkpoint.rawValue),
1521+
"query_available": .boolean(false),
1522+
"wallet_reference": .reference(walletId),
1523+
]
1524+
)
1525+
return nil
1526+
}
1527+
let materials = utxos.map {
1528+
diagnosticTxoFingerprint(
1529+
outpoint: $0.outpoint,
1530+
amount: $0.amount,
1531+
height: $0.height,
1532+
scriptPubKey: $0.scriptPubKey,
1533+
isLocked: $0.isLocked,
1534+
account: key
1535+
)
1536+
}
14681537
SDKLogger.event(
14691538
"core_memory_account_snapshot",
14701539
category: .persistence,
1471-
severity: .warning,
14721540
fields: [
1541+
"account_index": .unsignedInteger(UInt64(balance.index)),
14731542
"account_reference": .reference(key.referenceMaterial),
1474-
"account_type": .unsignedInteger(UInt64(key.typeTag)),
1543+
"account_type": .unsignedInteger(UInt64(balance.typeTag)),
14751544
"checkpoint": .publicText(checkpoint.rawValue),
1476-
"query_available": .boolean(false),
1545+
"confirmed_duffs": .unsignedInteger(balance.confirmed),
1546+
"immature_duffs": .unsignedInteger(balance.immature),
1547+
"locked_duffs": .unsignedInteger(balance.locked),
1548+
"query_available": .boolean(true),
1549+
"standard_tag": .unsignedInteger(UInt64(balance.standardTag)),
1550+
"unconfirmed_duffs": .unsignedInteger(balance.unconfirmed),
1551+
"utxo_count": .integer(Int64(utxos.count)),
1552+
"utxo_fingerprint": .reference(diagnosticFingerprint(materials)),
1553+
"utxo_value_duffs": .unsignedInteger(
1554+
diagnosticSaturatingSum(utxos.map(\.amount))
1555+
),
14771556
"wallet_reference": .reference(walletId),
14781557
]
14791558
)
1480-
continue
1559+
return utxos
14811560
}
1482-
let materials = utxos.map {
1483-
diagnosticTxoFingerprint(
1484-
outpoint: $0.outpoint,
1485-
amount: $0.amount,
1486-
height: $0.height,
1487-
scriptPubKey: $0.scriptPubKey,
1488-
isLocked: $0.isLocked,
1489-
account: key
1490-
)
1491-
}
1492-
SDKLogger.event(
1493-
"core_memory_account_snapshot",
1494-
category: .persistence,
1495-
fields: [
1496-
"account_index": .unsignedInteger(UInt64(balance.index)),
1497-
"account_reference": .reference(key.referenceMaterial),
1498-
"account_type": .unsignedInteger(UInt64(balance.typeTag)),
1499-
"checkpoint": .publicText(checkpoint.rawValue),
1500-
"confirmed_duffs": .unsignedInteger(balance.confirmed),
1501-
"immature_duffs": .unsignedInteger(balance.immature),
1502-
"locked_duffs": .unsignedInteger(balance.locked),
1503-
"query_available": .boolean(true),
1504-
"standard_tag": .unsignedInteger(UInt64(balance.standardTag)),
1505-
"unconfirmed_duffs": .unsignedInteger(balance.unconfirmed),
1506-
"utxo_count": .integer(Int64(utxos.count)),
1507-
"utxo_fingerprint": .reference(diagnosticFingerprint(materials)),
1508-
"utxo_value_duffs": .unsignedInteger(
1509-
diagnosticSaturatingSum(utxos.map(\.amount))
1510-
),
1511-
"wallet_reference": .reference(walletId),
1512-
]
1513-
)
1514-
memoryTxos.append(contentsOf: utxos)
1561+
if let accountTxos { memoryTxos.append(contentsOf: accountTxos) }
15151562
}
15161563
compareDatabase(
15171564
database,

‎packages/swift-sdk/SwiftTests/SwiftDashSDKTests/Dev1StoreUpgradeTests.swift‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -357,9 +357,23 @@ final class Dev1StoreUpgradeTests: XCTestCase {
357357
verdict(["PersistentWallet": a], identifiers: ["9.0.0"]),
358358
.newerThanRegistered(reason: "unregistered_version_identifier=9.0.0")
359359
)
360+
// Unplaceable, NOT a newer build: `open` must rethrow SwiftData's own
361+
// error for these rather than tell the user their wallet came from a
362+
// newer app and offer a reset.
360363
XCTAssertEqual(
361364
verdict(["PersistentWallet": a], identifiers: []),
362-
.newerThanRegistered(reason: "no_version_identifier")
365+
.unplaceable(reason: "no_version_identifier")
366+
)
367+
XCTAssertEqual(
368+
verdict([:]),
369+
.unplaceable(reason: "no_entity_hashes"),
370+
"no hashes means nothing was compared; drift may not be claimed"
371+
)
372+
XCTAssertEqual(
373+
verdict(["PersistentWallet": a]),
374+
.unplaceable(reason: "no_entity_disagreement"),
375+
"every hash the store carries agrees and it still is not compatible — "
376+
+ "it differs by something these hashes do not describe, not by the pinned drift"
363377
)
364378
XCTAssertEqual(
365379
verdict(["PersistentWallet": a, "FutureOnlyModel": a]),

0 commit comments

Comments
 (0)