perf(home): coalesce transaction-list reloads and share the CrowdNode restore scan - #977
Closed
romchornyi wants to merge 2 commits into
Closed
perf(home): coalesce transaction-list reloads and share the CrowdNode restore scan#977romchornyi wants to merge 2 commits into
romchornyi wants to merge 2 commits into
Conversation
Every reload trigger enqueued its own full pass on the view model's serial queue. A pass costs seconds once the wallet has a few thousand transactions, while the trigger throttle upstream is one second, so during a restore the queue took work faster than it finished it and the backlog grew for as long as the sync wrote rows. Measured on a mainnet restore of a ~6.6k-transaction wallet: 221 passes, 6252s of queued work inside a 17-minute session, and a request-to-render latency that climbed 6s -> 33s -> 122s -> 219s while the transaction count moved only 1537 -> 1854. The home list was rendering history up to three and a half minutes stale, and roughly seven of every eight passes rebuilt a snapshot the next one had already superseded. Gate on a running pass instead: at most one runs, at most one trailing pass is owed, and every trigger in between collapses into that trailing pass. Nothing is lost because the trailing pass reads the store fresh — including the filter selection, so a filter change during a pass still renders the newest selection.
A restore pass scans the persisted history twice with identical
arguments — tryRestoreSignUp, then getApiAddressConfirmationTx. Both
walk the same rows to answer questions about the same wallet, and each
walk is a SwiftData fetch across the TXO join plus a full decode, paid
inline on the calling thread.
On a mainnet wallet with ~6.6k transactions each walk took ~12.4s
(8.1s fetch, 4.3s decode), so one pass spent ~25s scanning, and the
passes repeat whenever the persisted row count moves — which, during a
restore, is constantly:
20:37:12 restoring CrowdNode state
20:37:24 CrowdNode scan: fetch 6183 rows in 8536ms, decode in 4286ms
20:37:37 CrowdNode scan: fetch 6183 rows in 8114ms, decode in 4289ms
20:37:37 CrowdNode: account not found
withSharedScan serves identical fetchObserved calls made inside it from
one fetch and decode, keyed by wallet, network and the scan's arguments.
It is deliberately scoped to the pass rather than cached across passes:
a row's context and blockHeight change as it confirms without the row
count moving, so a longer-lived memo would answer isChainAccepted from
stale data.
Also corrects the claim on the confirmation scan's floor. It bounds
firstSeen, which the SDK stamps when it first observes a row, so on a
restored wallet every row is "first seen" now and the floor admits the
whole history regardless of chain date — recorded as a TODO rather than
fixed here, since bounding on blockTimestamp needs a SwiftData predicate
change that has to be verified against the store.
📝 WalkthroughWalkthroughThe PR adds scoped transaction-scan reuse to CrowdNode restoration and coalesces concurrent ChangesCrowdNode restore scan sharing
Home reload coordination
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@DashWallet/Sources/Models/CrowdNode/Services/TransactionObserver.swift`:
- Around line 168-197: Replace the process-global
sharedScanDepth/sharedScanResults state with an invocation-local scope
propagated through withSharedScan, so nested calls reuse the same results while
concurrent restore passes remain isolated. Update the fetchObserved lookup path
to access the current scope, and revise the surrounding comments to describe the
implemented scope and behavior accurately.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad46132c-8066-4ce6-955e-8586e2354729
📒 Files selected for processing (3)
DashWallet/Sources/Models/CrowdNode/CrowdNode.swiftDashWallet/Sources/Models/CrowdNode/Services/TransactionObserver.swiftDashWallet/Sources/UI/Home/Views/HomeViewModel.swift
Comment on lines
+168
to
+197
| private static let sharedScanLock = NSLock() | ||
| private static var sharedScanDepth = 0 | ||
| private static var sharedScanResults: [SharedScanKey: [ObservedTransaction]] = [:] | ||
|
|
||
| /// Serves identical `fetchObserved` calls made inside `body` from a single | ||
| /// fetch + decode. | ||
| /// | ||
| /// A CrowdNode restore asks the same question twice — `tryRestoreSignUp` | ||
| /// and `getApiAddressConfirmationTx` scan with identical arguments — and | ||
| /// on a wallet with thousands of transactions each pass cost ~12s of fetch | ||
| /// and decode, paid inline by the caller's thread. | ||
| /// | ||
| /// Scoped to the call rather than cached across passes on purpose: a row's | ||
| /// `context` / `blockHeight` change as it confirms without the row count | ||
| /// moving, so a longer-lived memo would answer `isChainAccepted` from | ||
| /// stale data. | ||
| static func withSharedScan<T>(_ body: () throws -> T) rethrows -> T { | ||
| sharedScanLock.lock() | ||
| sharedScanDepth += 1 | ||
| sharedScanLock.unlock() | ||
| defer { | ||
| sharedScanLock.lock() | ||
| sharedScanDepth -= 1 | ||
| if sharedScanDepth == 0 { | ||
| sharedScanResults.removeAll() | ||
| } | ||
| sharedScanLock.unlock() | ||
| } | ||
| return try body() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Scope shared scan results to one withSharedScan invocation.
sharedScanDepth and sharedScanResults are process-global. If two restore passes overlap, the second pass can reuse the first pass's result for the same key. A row can change between the passes, but the second pass then evaluates stale confirmation data.
Use an invocation-local scan scope. Nested calls in the same restore pass must share that scope. Concurrent restore passes must use separate scopes.
As per coding guidelines, comments must describe behavior that the code actually implements.
🤖 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 `@DashWallet/Sources/Models/CrowdNode/Services/TransactionObserver.swift`
around lines 168 - 197, Replace the process-global
sharedScanDepth/sharedScanResults state with an invocation-local scope
propagated through withSharedScan, so nested calls reuse the same results while
concurrent restore passes remain isolated. Update the fetchObserved lookup path
to access the current scope, and revise the surrounding comments to describe the
implemented scope and behavior accurately.
Source: Coding guidelines
Contributor
Author
|
Closing as duplicate. Both commits here are already covered on
One correction to this PR's description, for anyone reading it later: it claimed the 2022 floor on the confirmation scan does not bound anything because The measurements from the restore that prompted this are still worth keeping as evidence for #976: 221 full reload passes, 6252s of queued work inside a 17-minute session, and request-to-render latency climbing 6s -> 33s -> 122s -> 219s while the transaction count moved only 1537 -> 1854. |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Restoring a wallet with a large history made the home screen look stuck: the sync percentage kept climbing while the transaction list still showed months-old history, and the app burned CPU throughout. Two independent causes, both in app code, both found in a diagnostic log from a mainnet restore of a ~6,600-transaction wallet.
1. The transaction-list reload queue took work faster than it finished it. Every reload trigger enqueued its own full pass on
HomeViewModel's serial queue. A pass costs seconds at that history size while the trigger throttle upstream is one second, so the backlog grew for as long as the sync wrote rows. From the log: 221 passes, 6,252s of queued work inside a 17-minute session, and a request-to-render latency that climbed 6s → 33s → 122s → 219s while the transaction count moved only 1,537 → 1,854. The list was rendering history up to three and a half minutes stale, and roughly seven of every eight passes rebuilt a snapshot the next one had already superseded.2. A CrowdNode restore pass scanned the whole history twice.
tryRestoreSignUpandgetApiAddressConfirmationTxscan with identical arguments, each a SwiftData fetch across the TXO join plus a full decode, paid inline on the calling thread:That is ~25s per pass, and passes repeat whenever the persisted row count moves — during a restore, constantly.
What was done?
HomeViewModelnow coalesces reloads instead of queueing them: at most one pass runs, at most one trailing pass is owed, and every trigger in between collapses into that trailing pass. Nothing is lost, because the trailing pass reads the store fresh — including the filter selection, so a filter change made during a pass still renders the newest selection.TransactionObserver.withSharedScanserves identicalfetchObservedcalls made inside it from one fetch and decode, keyed by wallet, network and the scan's arguments;CrowdNode.restoreStatewraps its pass in it. The sharing is deliberately scoped to the pass rather than cached across passes: a row'scontextandblockHeightchange as it confirms without the row count moving, so a longer-lived memo would answerisChainAcceptedfrom stale data.Also corrects a comment on the confirmation scan's floor. It claimed the floor keeps the scan off the wallet's full history; it bounds
firstSeen, which the SDK stamps when it first observes a row, so on a restored wallet every row is "first seen" now and the floor admits everything regardless of chain date. Recorded asTODO(crowdnode-restore-floor)rather than fixed here — bounding onblockTimestampneeds a SwiftData predicate change that should be verified against the store rather than reasoned about, and the rows it would exclude are a small share of a restored history.Neither change alters what the list or the restore concludes, only how many times the work is done.
How Has This Been Tested?
Clean
dashpaybuild for the simulator (xcodebuild -workspace DashWallet.xcworkspace -scheme dashpay -sdk iphonesimulator -destination 'generic/platform=iOS Simulator' ARCHS=arm64 build) —** BUILD SUCCEEDED **, no new warnings in the touched files.Not yet exercised on device against a large restored wallet, which is where the numbers above came from and where the effect should be confirmed; the unit-test target is currently broken repo-wide, so there is no automated coverage to add against it.
Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Performance Improvements
Bug Fixes