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 5 commits into
Conversation
|
Warning Review limit reachedNext included review available in 18 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 (7)
📝 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)
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 |
|
🕓 Queued for automated review — 18th in line, estimated start in ~2.7 h (commit 86c9033)
|
![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
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.
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.
| 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.
| 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.
…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>
…itative A thrown single-row pending-input fetch was memoized as an empty result, so the rest of the round treated the outpoint as having no pending rows: upsertUtxo could skip deferred-spend reconciliation and removePendingInputs could leave persisted rows behind. Failed lookups now leave the cache unpopulated (reads retry), inserts do not seed an entry that would read as the complete set, and removePendingInputs only writes the authoritative empty after a successful lookup. Also use loadUnaligned for the Data-backed index decode in the round tests. Addresses review feedback from coderabbitai and thepastaclaw. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A thrown row lookup in cachedTransaction / cachedTxo / cachedCoreAddress collapsed into the same nil as a genuinely missing row, and every caller takes nil as license to insert over a .unique column; the duplicate only surfaced as a failed save() at endChangeset. The account lookup in applyAccountChangeset and the wallet lookup in persistWalletChangeset had the same collapse with no unique backstop at all: a thrown account read committed a second account row, a thrown wallet read reported the round as a success while dropping it. Route the round's reads through the ModelFetching seam, record the first thrown lookup on the round cache, stop applying rows once set, and return a non-zero code from the changeset callback so Rust closes the round as failed and endChangeset rolls the staged writes back. Pending inputs keep their retry semantics (no unique column; a duplicate pending row resolves to the same TXO). Track TXO and pending-input prefetch coverage in separate outpoint sets so a thrown pending-input chunk fetch no longer discards the already-fetched TXO chunk. Fold the four per-row apply loops into applyEntries so the per-row autorelease pool and the rejection guard live in one place. Share the FetchFaultInjector seam double between suites and add a regression test for the rejected round. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… wall-clock ratio The 10x wall-clock ratio passed a 2-3x superlinear regression outright and could flake on a loaded CI host. Count reads through the ModelFetching seam instead: a round is the wallet and account lookups plus one bulk fetch per entity per 900-key chunk, so the count is a function of the chunk count and any reintroduced per-row fetch scales it with the record count. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
25dfd8c to
86c9033
Compare
|
Rebased onto
Tests: Resolved: line 949, line 906, line 231, and the pending-input / Deferred to follow-ups, left open on purpose (self-review findings outside the scope of this round):
🤖 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