perf(swift-sdk): linear wallet-changeset rounds via per-round bulk-prefetch cache - #4385
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>
|
Warning Review limit reached
Next review available in: 42 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Swift wallet persistence handler adds round-scoped bulk lookup caches for wallet changesets and account addresses. New tests cover chunked predicates, spend linkage, pending inputs, FFI fixtures, and processing scale. ChangesWallet persistence cache
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟠 High · up to The new bulk-fetch path can treat failed database reads as missing records, potentially overwriting existing wallet data or skipping related TXO backfills. The scaling regression test may also fail on slower CI due to a tight timing bound. Merge should wait for the fetch-error handling to be corrected and the timing assertion to be made reliable. Sequence Diagram(s)sequenceDiagram
participant Changeset as Wallet changeset
participant Handler as PlatformWalletPersistenceHandler
participant Cache as WalletChangesetRoundCache
participant SwiftData as SwiftData context
Changeset->>Handler: persistWalletChangeset
Handler->>Cache: prefetch lookup keys
Cache->>SwiftData: fetch rows in chunks
Handler->>Cache: resolve transaction and UTXO relationships
Cache-->>Handler: cached rows or misses
Handler->>SwiftData: persist reconciled rows
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🕓 Ready for review — 2 ahead in queue (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 875-904: Update the bulk prefetch loops around the cache
population logic to handle chunk fetch failures explicitly: when transaction,
outpoint, or address fetches fail, remove the affected keys from their
corresponding prefetched sets so cachedTransaction, cachedTxo,
cachedPendingInputs, and cachedCoreAddress perform per-row fallback; remove
outpoint keys only after both PersistentTxo and PersistentPendingInput fetches
complete. Also update the PersistentCoreAddress and PersistentTxo chunk fetch
handling near lines 3397-3410 to explicitly handle errors instead of swallowing
them, preserving existing rows and ensuring coreAddress backfill processes their
TXOs. Apply these changes in
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
at ranges 875-904 and 3397-3410.
In
`@packages/swift-sdk/SwiftTests/SwiftDashSDKTests/WalletChangesetRoundTests.swift`:
- Around line 211-236: Update testRoundCostScalesNearLinearly so the ratio
assertion only runs when the 1,000-record baseline is sufficiently measurable,
or increase the baseline floor to provide a realistic budget for the
4,000-record round. Preserve the scaling check while avoiding failures caused by
an unrealistically tight sub-0.5-second absolute threshold on loaded CI runners.
🪄 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: 87030835-78be-497f-b59b-56f91ebf9602
📒 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
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The cache preserves the staged-row behavior on normal reads, but two in-scope suggestions remain: bulk-fetch failures must not become authoritative misses, and input funding transaction IDs should not widen transaction prefetches when only their outpoints are consumed. No blocking issue was verified.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol; CodeRabbit inline findings independently verified. 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)
🟡 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:875-904: Do not turn failed bulk fetches into authoritative cache misses
Each bulk fetch uses `try?` and substitutes an empty array, while the chunk's keys remain in the corresponding `prefetched*` set. A fetch failure is therefore indistinguishable from a successful query that found no rows: `cachedTransaction`, `cachedTxo`, `cachedPendingInputs`, and `cachedCoreAddress` suppress their fallback lookups and can insert duplicates, leave existing pending inputs unresolved, or omit address relationships. The address-persistence prefetch at lines 3397–3410 has the same amplification: one failed chunk is treated as hundreds of missing addresses or TXOs. Handle each error explicitly by either failing the persistence round or removing the affected keys from authoritative coverage so row-wise lookups can retry; for outpoints, retain coverage only when both the TXO and pending-input queries succeeded.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:833-834: Avoid prefetching transaction rows for every input parent
An input's previous txid is only used to construct the outpoint passed to `cachedTxo`; none of the input-resolution helpers looks up the funding `PersistentTransaction`. Transaction rows that are actually consumed are already covered by transaction records, added UTXOs, and `spending_txid` values. Adding every input parent therefore widens the chunked transaction `IN` queries and can materialize unrelated historical transaction rows on the input-heavy restore path this PR is optimizing. Keep the outpoint prefetch but omit the transaction-prefetch insertion.
| for chunk in Self.chunked(Array(cache.prefetchedTxids)) { | ||
| let descriptor = FetchDescriptor<PersistentTransaction>( | ||
| predicate: #Predicate { chunk.contains($0.txid) } | ||
| ) | ||
| for row in (try? backgroundContext.fetch(descriptor)) ?? [] { | ||
| cache.transactions[row.txid] = row | ||
| } | ||
| } | ||
| for chunk in Self.chunked(Array(cache.prefetchedOutpoints)) { | ||
| let txoDescriptor = FetchDescriptor<PersistentTxo>( | ||
| predicate: #Predicate { chunk.contains($0.outpoint) } | ||
| ) | ||
| for row in (try? backgroundContext.fetch(txoDescriptor)) ?? [] { | ||
| cache.txos[row.outpoint] = row | ||
| } | ||
| let pendingDescriptor = FetchDescriptor<PersistentPendingInput>( | ||
| predicate: #Predicate { chunk.contains($0.outpoint) } | ||
| ) | ||
| for row in (try? backgroundContext.fetch(pendingDescriptor)) ?? [] { | ||
| cache.pendingInputs[row.outpoint, default: []].append(row) | ||
| } | ||
| } | ||
| for chunk in Self.chunked(Array(cache.prefetchedAddresses)) { | ||
| let descriptor = FetchDescriptor<PersistentCoreAddress>( | ||
| predicate: #Predicate { chunk.contains($0.address) } | ||
| ) | ||
| for row in (try? backgroundContext.fetch(descriptor)) ?? [] { | ||
| cache.coreAddresses[row.address] = row | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Do not turn failed bulk fetches into authoritative cache misses
Each bulk fetch uses try? and substitutes an empty array, while the chunk's keys remain in the corresponding prefetched* set. A fetch failure is therefore indistinguishable from a successful query that found no rows: cachedTransaction, cachedTxo, cachedPendingInputs, and cachedCoreAddress suppress their fallback lookups and can insert duplicates, leave existing pending inputs unresolved, or omit address relationships. The address-persistence prefetch at lines 3397–3410 has the same amplification: one failed chunk is treated as hundreds of missing addresses or TXOs. Handle each error explicitly by either failing the persistence round or removing the affected keys from authoritative coverage so row-wise lookups can retry; for outpoints, retain coverage only when both the TXO and pending-input queries succeeded.
source: ['coderabbit']
There was a problem hiding this comment.
Fixed in 25dfd8c. A failed chunk fetch now removes its keys from the prefetched set (round cache) — or records the addresses for a single-row fallback fetch in persistAccountAddresses — so a thrown bulk fetch degrades to the pre-cache per-row behavior instead of reading as an authoritative miss.
🤖 Posted autonomously by Claude on behalf of pasta.
| let prevTxid = hashData(entry.txid) | ||
| cache.prefetchedTxids.insert(prevTxid) |
There was a problem hiding this comment.
🟡 Suggestion: Avoid prefetching transaction rows for every input parent
An input's previous txid is only used to construct the outpoint passed to cachedTxo; none of the input-resolution helpers looks up the funding PersistentTransaction. Transaction rows that are actually consumed are already covered by transaction records, added UTXOs, and spending_txid values. Adding every input parent therefore widens the chunked transaction IN queries and can materialize unrelated historical transaction rows on the input-heavy restore path this PR is optimizing. Keep the outpoint prefetch but omit the transaction-prefetch insertion.
| let prevTxid = hashData(entry.txid) | |
| cache.prefetchedTxids.insert(prevTxid) | |
| let prevTxid = hashData(entry.txid) |
source: ['codex']
There was a problem hiding this comment.
Done in 25dfd8c — input prevout txids are no longer collected into the transaction prefetch. They were only needed by the CoinJoin funding-known gate, which was split out of this PR into a follow-up; that PR will reintroduce the collection alongside its consumer.
🤖 Posted autonomously by Claude on behalf of pasta.
…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>
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
Testing