perf(swift-sdk): linear wallet-changeset rounds via per-round bulk-prefetch cache - #4392
perf(swift-sdk): linear wallet-changeset rounds via per-round bulk-prefetch cache#4392PastaPastaPasta wants to merge 2 commits into
Conversation
…efetch cache A single persister store() round can carry thousands of transaction records (an SPV catch-up folds many blocks into one round), and the apply helpers issued an individual ModelContext.fetch per row, per input, and per UTXO. Each fetch re-evaluates its predicate against every object staged in the open begin/end changeset bracket, so round cost grew quadratically - hours of pinned CPU for an 8k-record round on a large wallet, stalling the persistence drain behind the incident where a ~900k-txcount wallet reached 59 GB. persistWalletChangeset now walks the changeset once, bulk-fetches every transaction / TXO / pending-input / core-address row the round could touch with chunked IN predicates, and the helpers hit per-round dictionaries; inserts and deletes update the cache in place so later rows in the batch observe them. persistAccountAddresses gets the same treatment for its per-address row and TXO-backfill fetches. A 4k-record round drops from minutes to under a second, verified by a scaling regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…drop unused prevout-txid prefetch A thrown chunk fetch previously left its keys in the prefetched sets, turning the error into an authoritative 'row does not exist' for ~900 keys at once - the upsert paths would then insert duplicates over unique columns. A failed chunk now removes its keys from the prefetched set (round cache) or records the addresses for a single-row fallback fetch (persistAccountAddresses), restoring the pre-cache behavior on error. Also stop collecting input prevout txids into the transaction prefetch: the apply helpers look inputs up as TXOs / pending rows, never as transactions, so those keys only inflated the IN queries (hundreds of foreign parents per CoinJoin record). Addresses review feedback from coderabbitai and thepastaclaw on PR 4385. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesWallet changeset persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🔵 Low · up to The PR substantially improves wallet restoration performance, but merge readiness has two bounded follow-ups: a fallback lookup failure can be treated as a missing pending row and temporarily affect spend resolution, and one regression test may trap on unaligned data before exercising its assertion. Sequence Diagram(s)sequenceDiagram
participant PlatformWalletPersistenceHandler
participant WalletChangesetRoundCache
participant SwiftData
PlatformWalletPersistenceHandler->>WalletChangesetRoundCache: build cache for wallet changeset round
WalletChangesetRoundCache->>SwiftData: bulk-fetch transactions, TXOs, pending inputs, and addresses
PlatformWalletPersistenceHandler->>WalletChangesetRoundCache: reconcile changeset entries
WalletChangesetRoundCache-->>PlatformWalletPersistenceHandler: return cached rows or authoritative misses
PlatformWalletPersistenceHandler->>SwiftData: persist reconciled rows and relationships
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 25dfd8c) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 989-1004: Update cachedPendingInputs so a failed
backgroundContext.fetch does not cache an empty result in cache.pendingInputs;
only store successfully fetched rows, while preserving the existing cached and
prefetched-outpoint behavior so subsequent lookups retry after failure.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/WalletChangesetRoundTests.swift`:
- Line 166: Update the fundingIndex extraction in WalletChangesetRoundTests to
use Swift 6’s unaligned byte-loading API instead of load(as:), preserving the
UInt64 conversion while avoiding alignment-dependent traps for Data storage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3b5270f-0b8b-42a3-9b2c-fe636464b939
📒 Files selected for processing (5)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/BulkFetchPredicateTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/DashPayPersistenceTests.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/FFIFixtures.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/WalletChangesetRoundTests.swift
| private func cachedPendingInputs( | ||
| outpoint: Data, | ||
| cache: WalletChangesetRoundCache | ||
| ) -> [PersistentPendingInput] { | ||
| if let rows = cache.pendingInputs[outpoint] { return rows } | ||
| if cache.prefetchedOutpoints.contains(outpoint) { | ||
| cache.pendingInputs[outpoint] = [] | ||
| return [] | ||
| } | ||
| let descriptor = FetchDescriptor<PersistentPendingInput>( | ||
| predicate: #Predicate { $0.outpoint == outpoint } | ||
| ) | ||
| let rows = (try? backgroundContext.fetch(descriptor)) ?? [] | ||
| cache.pendingInputs[outpoint] = rows | ||
| return rows | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
A failed single-row fetch becomes an authoritative "no pending rows" for the whole round.
Line 1001 collapses a thrown fetch into [], and line 1002 stores that result. Every later call for the same outpoint then returns [] without retrying. resolveInputOutpoint can therefore insert a second pending row for an outpoint that already has one, and upsertUtxo skips the deferred-spend resolve for that outpoint in this round.
The impact is bounded: duplicate pending rows resolve to the same TXO, and PersistentPendingInput has no unique column. The prefetch path deliberately avoids this pattern (it subtracts the chunk instead of recording an authoritative miss), so the fallback path is inconsistent with it. Consider not caching on failure so the next lookup retries.
♻️ Proposed change to keep a failed fetch non-authoritative
let descriptor = FetchDescriptor<PersistentPendingInput>(
predicate: `#Predicate` { $0.outpoint == outpoint }
)
- let rows = (try? backgroundContext.fetch(descriptor)) ?? []
- cache.pendingInputs[outpoint] = rows
- return rows
+ guard let rows = try? backgroundContext.fetch(descriptor) else {
+ // Leave the key uncached so a later lookup retries instead of
+ // reading the failure as "no pending rows for this outpoint".
+ return []
+ }
+ cache.pendingInputs[outpoint] = rows
+ return rows
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private func cachedPendingInputs( | |
| outpoint: Data, | |
| cache: WalletChangesetRoundCache | |
| ) -> [PersistentPendingInput] { | |
| if let rows = cache.pendingInputs[outpoint] { return rows } | |
| if cache.prefetchedOutpoints.contains(outpoint) { | |
| cache.pendingInputs[outpoint] = [] | |
| return [] | |
| } | |
| let descriptor = FetchDescriptor<PersistentPendingInput>( | |
| predicate: #Predicate { $0.outpoint == outpoint } | |
| ) | |
| let rows = (try? backgroundContext.fetch(descriptor)) ?? [] | |
| cache.pendingInputs[outpoint] = rows | |
| return rows | |
| } | |
| private func cachedPendingInputs( | |
| outpoint: Data, | |
| cache: WalletChangesetRoundCache | |
| ) -> [PersistentPendingInput] { | |
| if let rows = cache.pendingInputs[outpoint] { return rows } | |
| if cache.prefetchedOutpoints.contains(outpoint) { | |
| cache.pendingInputs[outpoint] = [] | |
| return [] | |
| } | |
| let descriptor = FetchDescriptor<PersistentPendingInput>( | |
| predicate: #Predicate { $0.outpoint == outpoint } | |
| ) | |
| guard let rows = try? backgroundContext.fetch(descriptor) else { | |
| // Leave the key uncached so a later lookup retries instead of | |
| // reading the failure as "no pending rows for this outpoint". | |
| return [] | |
| } | |
| cache.pendingInputs[outpoint] = rows | |
| return rows | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`
around lines 989 - 1004, Update cachedPendingInputs so a failed
backgroundContext.fetch does not cache an empty result in cache.pendingInputs;
only store successfully fetched rows, while preserving the existing cached and
prefetched-outpoint behavior so subsequent lookups retry after failure.
There was a problem hiding this comment.
The failure is valid, but changing only cachedPendingInputs is incomplete. resolveInputOutpoint can immediately cache only its newly inserted row, and removePendingInputs unconditionally replaces the failed lookup with []; both callers must preserve the non-authoritative failure state so a later lookup can retry and discover persisted rows.
There was a problem hiding this comment.
@thepastaclaw, correct. The cache must preserve the failed-fetch state across all three paths.
resolveInputOutpoint must not append the newly staged row to cache.pendingInputs when the fallback fetch failed. removePendingInputs must not replace that failed state with an empty authoritative entry. A later lookup can then retry and merge persisted rows with staged rows from the ModelContext.
- Update the pending-input cache contract to distinguish a successful empty lookup from a failed lookup.
- Preserve the non-authoritative state in
resolveInputOutpointandremovePendingInputs. - Add regression coverage for fallback failure followed by another lookup in the same changeset round.
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/platform PR: 4392
File: packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:989-1004
Timestamp: 2026-08-13T02:25:04.855Z
Learning: For `PlatformWalletPersistenceHandler` pending-input caching, a failed SwiftData fallback fetch is non-authoritative. `cachedPendingInputs(outpoint:cache:)`, `resolveInputOutpoint(outpoint:inputIndex:spendingTransaction:spendingTxid:walletId:cache:)`, and `removePendingInputs(for:cache:)` must preserve that state so a later lookup retries.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Failed to handle agent chat message. Please try again.
You are interacting with an AI system.
| let txos = try fetchAll(PersistentTxo.self, in: container) | ||
| XCTAssertEqual(txos.count, count) | ||
| for txo in txos { | ||
| let fundingIndex = txo.outpoint.withUnsafeBytes { $0.load(as: UInt64.self) } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the repository-declared Swift toolchain and existing unaligned-load usage.
fd -HI -t f -g 'Package.swift' -g '.swift-version' -g '.tool-versions' -g '*.pbxproj' . \
| sort \
| xargs -r rg -n -C2 'swift-tools-version|SWIFT_VERSION|loadUnaligned'
rg -n -C2 '\.loadUnaligned\(as:' packages/swift-sdkRepository: dashpay/platform
Length of output: 393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Swift package/toolchain declarations ---'
find . -type f \( -name 'Package.swift' -o -name '.swift-version' -o -name '.tool-versions' -o -name '*.pbxproj' \) -print0 |
sort -z |
xargs -0 -r rg -n -C2 'swift-tools-version|SWIFT_VERSION|IPHONEOS_DEPLOYMENT_TARGET'
printf '%s\n' '--- Relevant test context ---'
sed -n '145,180p' packages/swift-sdk/SwiftTests/SwiftDashSDKTests/WalletChangesetRoundTests.swift
printf '%s\n' '--- Existing unaligned-load usage ---'
rg -n -C2 '\.loadUnaligned\(as:' packages/swift-sdk || true
printf '%s\n' '--- Outpoint construction and related decoding ---'
rg -n -C3 'outpoint|fundingIndex' packages/swift-sdk/SwiftTests packages/swift-sdk --glob '*.swift' --glob '*.rs' |
head -n 240Repository: dashpay/platform
Length of output: 37310
Use an unaligned load for Data bytes.
load(as:) requires eight-byte-aligned storage. Data does not guarantee this alignment, so the test can trap before it checks spend linkage. Swift 6 supports loadUnaligned(as:).
Proposed fix
- let fundingIndex = txo.outpoint.withUnsafeBytes { $0.load(as: UInt64.self) }
+ let fundingIndex = txo.outpoint.withUnsafeBytes {
+ $0.loadUnaligned(as: UInt64.self)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let fundingIndex = txo.outpoint.withUnsafeBytes { $0.load(as: UInt64.self) } | |
| let fundingIndex = txo.outpoint.withUnsafeBytes { | |
| $0.loadUnaligned(as: UInt64.self) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/WalletChangesetRoundTests.swift`
at line 166, Update the fundingIndex extraction in WalletChangesetRoundTests to
use Swift 6’s unaligned byte-loading API instead of load(as:), preserving the
UInt64 conversion while avoiding alignment-dependent traps for Data storage.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The per-round cache preserves the intended reconciliation behavior and removes the main quadratic lookup path, but two minor issues remain: failed pending-input fallback fetches are cached as authoritative misses, and a new test performs an alignment-dependent typed load from Data. Neither issue is blocking, but both should be corrected before relying on the fallback and regression coverage.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is 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)
🟡 2 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`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:1001-1002: Do not make a failed pending-input fetch authoritative
The bulk-prefetch path removes a failed chunk from `prefetchedOutpoints` so later accesses can fall back to individual fetches, but this fallback converts its own fetch failure into `[]` and caches that value. Subsequent operations therefore treat the outpoint as having no pending rows: `upsertUtxo` can skip deferred-spend reconciliation, while `removePendingInputs` can leave persisted rows behind. Preserve a distinct fetch-failure state and only cache successfully fetched rows. The callers also need to avoid replacing that failure state with an authoritative empty or partial entry: `resolveInputOutpoint` may insert a new staged row without claiming it is the complete set, and `removePendingInputs` should only cache `[]` after a successful lookup and deletion.
In `packages/swift-sdk/SwiftTests/SwiftDashSDKTests/WalletChangesetRoundTests.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftTests/SwiftDashSDKTests/WalletChangesetRoundTests.swift:166: Use an unaligned load when decoding the Data-backed txid
`UnsafeRawBufferPointer.load(as:)` requires the buffer address to satisfy `UInt64` alignment, which `Data.withUnsafeBytes` does not guarantee. The test can therefore trap before checking the spend linkage. The package uses Swift tools 6.0, where `loadUnaligned(as:)` is available, so decode these bytes without imposing an alignment precondition.
| let rows = (try? backgroundContext.fetch(descriptor)) ?? [] | ||
| cache.pendingInputs[outpoint] = rows |
There was a problem hiding this comment.
🟡 Suggestion: Do not make a failed pending-input fetch authoritative
The bulk-prefetch path removes a failed chunk from prefetchedOutpoints so later accesses can fall back to individual fetches, but this fallback converts its own fetch failure into [] and caches that value. Subsequent operations therefore treat the outpoint as having no pending rows: upsertUtxo can skip deferred-spend reconciliation, while removePendingInputs can leave persisted rows behind. Preserve a distinct fetch-failure state and only cache successfully fetched rows. The callers also need to avoid replacing that failure state with an authoritative empty or partial entry: resolveInputOutpoint may insert a new staged row without claiming it is the complete set, and removePendingInputs should only cache [] after a successful lookup and deletion.
source: ['coderabbit']
There was a problem hiding this comment.
Fixed in 9219275: a thrown fallback fetch now leaves the cache unpopulated (reads retry), the insert path skips seeding an entry for a failed-fetch outpoint so a staged row never reads as the complete set, and removePendingInputs only writes the authoritative empty after a successful lookup.
🤖 Posted autonomously by Claude on behalf of pasta.
| let txos = try fetchAll(PersistentTxo.self, in: container) | ||
| XCTAssertEqual(txos.count, count) | ||
| for txo in txos { | ||
| let fundingIndex = txo.outpoint.withUnsafeBytes { $0.load(as: UInt64.self) } |
There was a problem hiding this comment.
🟡 Suggestion: Use an unaligned load when decoding the Data-backed txid
UnsafeRawBufferPointer.load(as:) requires the buffer address to satisfy UInt64 alignment, which Data.withUnsafeBytes does not guarantee. The test can therefore trap before checking the spend linkage. The package uses Swift tools 6.0, where loadUnaligned(as:) is available, so decode these bytes without imposing an alignment precondition.
| let fundingIndex = txo.outpoint.withUnsafeBytes { $0.load(as: UInt64.self) } | |
| let fundingIndex = txo.outpoint.withUnsafeBytes { | |
| $0.loadUnaligned(as: UInt64.self) | |
| } |
source: ['coderabbit']
There was a problem hiding this comment.
Fixed in 9219275 — switched to loadUnaligned(as:).
🤖 Posted autonomously by Claude on behalf of pasta.
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Automated deep review of this PR (8 independent finder angles, each candidate adversarially re-verified against the PR head before posting). 9 findings survived verification and are posted inline below; the remaining candidates were dropped as duplicates of existing review threads or as unconfirmed on inspection (notably a claimed prefetch-coverage gap on upsertUtxo's pending-resolve path — the chosen.spendingTransaction relationship preference makes that fallback fetch rare in practice).
🤖 Posted autonomously by Claude on behalf of pasta.
| let descriptor = FetchDescriptor<PersistentTransaction>( | ||
| predicate: #Predicate { $0.txid == txid } | ||
| ) | ||
| guard let row = try? backgroundContext.fetch(descriptor).first else { return nil } |
There was a problem hiding this comment.
🟡 Thrown fallback fetches in cachedTransaction/cachedTxo/cachedCoreAddress read as "row absent" and license inserts over .unique columns
All three single-row fallbacks use guard let row = try? backgroundContext.fetch(descriptor).first else { return nil }, which collapses a thrown fetch into the same nil as a genuinely missing row. Callers take nil as license to insert (upsertTransaction's new-record branch, upsertUtxo's TXO and stub-tx inserts), and PersistentTransaction.txid, PersistentTxo.outpoint, and PersistentCoreAddress.address are all @Attribute(.unique) — so a transient fetch error becomes a duplicate-key insert whose failure only surfaces at endChangeset's save(), rolling back the entire round and reporting a persistence failure to Rust; if the underlying error recurs, the replayed round wedges the same way. The bulk-path comment above (lines 882-888) names exactly this hazard and routes failures to this fallback, but the fallback has the same collapse, and the resolution being applied to cachedPendingInputs in the existing thread doesn't extend here. Consider distinguishing a thrown fetch from an empty result in these three as well (fail the round early, or track the key as unresolved rather than absent).
🤖 Posted autonomously by Claude on behalf of pasta.
| let pendingDescriptor = FetchDescriptor<PersistentPendingInput>( | ||
| predicate: #Predicate { chunk.contains($0.outpoint) } | ||
| ) | ||
| if let txoRows = try? backgroundContext.fetch(txoDescriptor), |
There was a problem hiding this comment.
💬 A thrown pending-input fetch discards the successful TXO chunk results
The joint if let txoRows = try? ..., let pendingRows = try? ... couples the two fetches: when the pending fetch throws, the already-fetched txoRows are thrown away and the whole chunk is subtracted from prefetchedOutpoints, demoting ~900 keys of both entities to per-row fallback fetches for the rest of the round. Since one prefetchedOutpoints set serves both caches, confining the damage means splitting coverage per entity — e.g. separate prefetched-outpoint sets for TXOs and pending inputs, so a transient error in one fetch doesn't reinstate per-row fetching for the other.
🤖 Posted autonomously by Claude on behalf of pasta.
| record.coreAddress = coreAddr | ||
| } | ||
| if record.coreAddress == nil, !record.address.isEmpty, | ||
| let coreAddr = cachedCoreAddress(address: record.address, cache: cache) { |
There was a problem hiding this comment.
💬 Existing-TXO core-address lookups can miss the prefetch and fall back per row
prefetchedAddresses is collected from utxo.address (FFI value, only when non-nil), but this lookup keys on record.address — the stored row value. When a known outpoint is re-emitted without an FFI address (or with a different one) and record.coreAddress is still nil, the stored address misses the prefetched set and takes a single-row fallback fetch; a negative result is never memoized, so every such TXO in the round re-fetches. Since the TXO bulk fetch runs before the core-address chunk loop in buildWalletChangesetRoundCache, unioning the fetched TXO rows' address values into prefetchedAddresses there (or memoizing negative fallback results) would close the gap.
🤖 Posted autonomously by Claude on behalf of pasta.
| cache.prefetchedOutpoints.insert( | ||
| PersistentTxo.makeOutpoint(txid: txid, vout: entry.outpoint.vout) | ||
| ) | ||
| cache.prefetchedTxids.insert(hashData(entry.spending_txid)) |
There was a problem hiding this comment.
💬 All-zero spending_txid sentinel is seeded into prefetchedTxids
The consumer (markUtxoSpent) guards the lookup with !spendingTxid.allSatisfy { $0 == 0 }, but this insert is unconditional, so the "no spending tx" sentinel lands in the bulk IN fetch and the authoritative-miss set. Harmless today because the only guarded consumer never looks it up, but mirroring the zero-check here keeps the key set meaningful and the fetch lists minimal.
🤖 Posted autonomously by Claude on behalf of pasta.
| // restore emits thousands of entries per round, and each | ||
| // per-row fetch would re-scan the round's staged objects | ||
| // (same quadratic the wallet-changeset round cache removes). | ||
| let allAddresses = entries.map(\.address) |
There was a problem hiding this comment.
🟡 persistPlatformPaymentAddresses still does one fetch per entry — the same n² this block removes
This bulk prefetch fixes the base58 branch, but the platform branch this function delegates to a few lines up (persistPlatformPaymentAddresses, ~line 3527) still runs a FetchDescriptor<PersistentPlatformAddress> per entry against a context full of staged rows. The justification in this comment applies verbatim: a DIP-17 platform-account restore emits thousands of entries per round, and each per-entry fetch re-scans the round's staged objects — keeping exactly the quadratic this PR removes for core addresses. Applying the same chunked-IN prefetch keyed on PersistentPlatformAddress.address there would finish the job.
🤖 Posted autonomously by Claude on behalf of pasta.
| let txoDescriptor = FetchDescriptor<PersistentTxo>( | ||
| predicate: #Predicate { chunk.contains($0.address) } | ||
| ) | ||
| for txo in (try? backgroundContext.fetch(txoDescriptor)) ?? [] { |
There was a problem hiding this comment.
💬 Failed TXO-backfill chunk now silently skips ~900 addresses; allAddresses keeps duplicates
The comment says a failed TXO-backfill fetch "match[es] the old per-row try?", but the granularity changed: the old code lost one address's backfill per thrown fetch, while a thrown chunk here drops it for up to 900 addresses, with no unresolvedAddresses-style fallback like the address-row fetch immediately above. Separately, entries.map(\.address) keeps duplicate addresses in the chunk IN lists (a Set would be minimal by construction). Both are minor since the backfill is a display-relationship sweep, but a symmetric per-address fallback set would restore parity with the row fetch beside it.
🤖 Posted autonomously by Claude on behalf of pasta.
| // insert duplicates over `.unique` columns. Dropping the | ||
| // chunk's keys instead routes every lookup through the | ||
| // single-row fallback fetch — the pre-cache behavior. | ||
| for chunk in Self.chunked(Array(cache.prefetchedTxids)) { |
There was a problem hiding this comment.
🟡 Round cache pins every key and fetched row for the whole round — memory peak lands on exactly the rounds this PR targets
Each prefetched outpoint is a 36-byte Data, and the builder collects every input outpoint (CoinJoin records carry hundreds of foreign parents each), so a large round can hold 10^5–10^6 keys across the prefetched* sets plus a reference to every fetched/inserted row in the dictionaries until the round ends; Self.chunked(Array(...)) then materializes the full key set again as arrays. Staged inserts are pinned by the ModelContext regardless, so the delta is the key sets, the fetched-row maps, and the array copies — concentrated on the huge rounds the PR optimizes, which is where iOS memory pressure (jetsam) bites. Draining the cache per account/sub-batch and chunking over ArraySlice instead of copied arrays would flatten the peak without giving up the linear fetch count.
🤖 Posted autonomously by Claude on behalf of pasta.
|
|
||
| let small = try measureRound(count: 1_000) | ||
| let large = try measureRound(count: 4_000) | ||
| XCTAssertLessThan( |
There was a problem hiding this comment.
🟡 Wall-clock ratio assertion is simultaneously loose and flaky — consider counting fetches instead
large < max(small, 0.05) * 10 passes a 2–3× superlinear regression outright, while on a loaded CI host large can spike past 10× of a fast small run and flake — and the natural response to flakes is loosening the threshold, eroding the only regression guard this PR adds. A deterministic proxy would serve better: a fetch-call counter on the handler (increment around each backgroundContext.fetch) asserting the round executes O(chunks + fallback misses) fetches rather than O(rows). That fails on any reintroduced per-row fetch, runs in milliseconds, and is immune to host load.
🤖 Posted autonomously by Claude on behalf of pasta.
| try context.save() | ||
|
|
||
| var fetched: [Data: PersistentTxo] = [:] | ||
| for chunk in stride(from: 0, to: outpoints.count, by: 900).map({ |
There was a problem hiding this comment.
💬 Test re-implements the chunking instead of exercising chunked(_:size:)
Because PlatformWalletPersistenceHandler.chunked is private, this test hand-rolls the same stride/slice logic inline — so it validates a copy of the algorithm, not the shipped helper, and the two can drift (say, a future chunk-size or slicing change) without this pin noticing. Widening chunked to internal (the suite already imports @testable) and calling it here would make the contract test bind to the real code; this PR's new FFIFixtures.swift shows the pattern of promoting shared test plumbing when a second user appears.
🤖 Posted autonomously by Claude on behalf of pasta.
2 participants
Issue being fixed or feature implemented
Restoring a wallet with a large transaction history made the app pin a CPU core for hours and grow memory without bound until the OS killed it (observed: 59 GB footprint on a mainnet wallet whose SPV scan matches ~8,000 transactions, with only 3,884 of them ever reaching disk).
The root cause is how a persistence round applies its rows. Each Rust
store()round maps to onebeginChangeset→ per-kind callbacks →endChangesetbracket, with a singlesave()at the end. During SPV catch-up one round can carry thousands of transaction records, and the apply helpers (upsertTransaction,resolveInputOutpoint,upsertUtxo,markUtxoSpent, …) issued an individualModelContext.fetchfor every row, every input, and every UTXO. SwiftData evaluates each of those fetches against all objects staged so far in the unsaved round, so the more rows a round had already staged, the more expensive every following fetch became:What was done?
One idea, applied consistently: fetch once per round, not once per row.
PlatformWalletPersistenceHandler.persistWalletChangesetnow builds aWalletChangesetRoundCachebefore applying anything: it walks the changeset once, collects every txid / outpoint / address the round could touch, and bulk-fetches the matchingPersistentTransaction/PersistentTxo/PersistentPendingInput/PersistentCoreAddressrows with chunkedINpredicates (≤900 keys per chunk, under SQLite's bind-variable limit).upsertTransaction,resolveInputOutpoint,removePendingInputs,upsertUtxo,markUtxoSpent,markUtxoInstantLocked) look rows up in the cache dictionaries instead of fetching. Inserts and deletes update the cache in place, so later rows in the same batch observe them exactly as they previously observed staged objects through per-row fetches.spendingTxidfrom a prior session) falls back to a single-row fetch.persistAccountAddressesgets the same treatment — its per-address row fetch and per-address TXO-backfill fetch (a second hot loop in the same rounds during restore) are now two chunked bulk fetches.Result: a 4,000-record round drops from minutes to under a second, and the end-to-end restore that previously died at 59 GB completes a full mainnet genesis→tip sync in ~16 minutes with a ~1.2 GB peak (header download, not persistence; ~430 MB settled).
How Has This Been Tested?
New unit tests (
swift test, 354 passing):BulkFetchPredicateTests— pins the two SwiftData behaviors the cache depends on:[Data].contains($0.column)translating to SQLINwith >900 keys chunked, and staged (unsaved) rows staying visible to bulk fetches.WalletChangesetRoundTests— drives realWalletChangeSetFFIstructs through a full begin→persist→end round: a same-round chain of spends resolves every TXO↔spender linkage and drains all pending-input rows; an input with unknown funding still writes its pending-input row (the out-of-order spend-repair mechanism); and a scaling regression test asserts a 4× larger round costs near-linearly more (fails on any quadratic regression).FFIFixtures— shared test helpers (deduplicatestuple32copies that existed inDashPayPersistenceTests).Manual end-to-end: restored a mainnet wallet reproducing the incident workload (~8k matched transactions) in SwiftExampleApp on the iOS simulator. Full chain scan completed in ~16 minutes; all matched transactions and TXOs durably persisted; sync watermark reached the chain tip; memory sampled every 30 s never exceeded ~1.25 GB; app restart came back clean with the watermark intact.
Breaking Changes
None. No public API or schema changes; the persistence semantics (round atomicity, pending-input repair, spend gating) are unchanged — only the lookup strategy inside a round.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Reliability
Tests