fix(wallet): reload the runtime when an unconfirmed transaction is removed - #1122
Open
llbartekll wants to merge 2 commits into
Open
fix(wallet): reload the runtime when an unconfirmed transaction is removed#1122llbartekll wants to merge 2 commits into
llbartekll wants to merge 2 commits into
Conversation
…moved Removing a never-accepted transaction edits the persistence layer and relies on a full runtime reload to make the Rust wallet forget it — there is no removal API at any FFI layer. That reload was requested through `rearmPlatformSync()`, whose `.platformSyncRearm` trigger `shouldSkipRefresh` elides whenever `isRuntimeReady` holds. At removal time the host, Core SPV and BLAST are all running, so the refresh was always elided and no reload ran: the Rust wallet kept the transaction in its in-memory tx set and its inputs stayed marked spent — so the coins the action promises to free stayed unspendable — while dash-spv's mempool tracker kept rebroadcasting, until the next app launch. Give the remover its own `.walletRowsChanged` trigger on the never-elide arm of `shouldSkipRefresh`, next to `.walletMaterialChanged` and `.walletDidChange`, reached through `reloadAfterWalletRowsChanged()`. `rearmPlatformSync` keeps its elision, which is correct for its own caller: `syncNow` reaches it only when BLAST is already down. Deriving the rescan depth moves ahead of the reload. `fullReset` stops SPV, and `resetPublishedState` zeroes the coordinator's `tipHeight`; only a later progress tick re-seeds it. Reading the tip after a reload that now really happens would find 0 and arm no rescan on any removal — and on the bulk path that rescan is the only safety rail, because it never explorer-checks. Anchoring on the pre-reload tip rewinds deeper, never shallower. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe wallet runtime now reloads after SwiftData wallet-row changes. Unconfirmed transaction removal returns explicit recovery outcomes. Sync status and transaction detail screens display messages for armed rescans, unavailable rescans, and stopped runtimes. ChangesWallet recovery flow
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change reloads the wallet runtime after transaction-row removal and presents recovery-specific status messages. No current merge-blocking risk is identified. Sequence Diagram(s)sequenceDiagram
participant UserInterface
participant UnconfirmedTransactionRemover
participant SwiftDashSDKWalletRuntime
participant SPVCoordinator
UserInterface->>UnconfirmedTransactionRemover: remove unconfirmed transactions
UnconfirmedTransactionRemover->>SPVCoordinator: capture pre-reload tipHeight
UnconfirmedTransactionRemover->>SwiftDashSDKWalletRuntime: reloadAfterWalletRowsChanged()
SwiftDashSDKWalletRuntime-->>UnconfirmedTransactionRemover: runtime readiness
UnconfirmedTransactionRemover->>SPVCoordinator: arm rescan when available
SPVCoordinator-->>UnconfirmedTransactionRemover: RemovalOutcome
UnconfirmedTransactionRemover-->>UserInterface: outcome-specific message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 — 17th in line, estimated start in ~5 h (commit 9cb02e4)
|
romchornyi
reviewed
Sep 9, 2026
romchornyi
left a comment
romchornyi
left a comment
Contributor
There was a problem hiding this comment.
Reviewed at head 4b15786f. The core fix is sound, and I verified the load-bearing claims rather than taking them on trust:
shouldSkipRefreshreally did elide.platformSyncRearmwheneverisRuntimeReadyheld, so the removal really was a no-op in the healthy case. The diagnosis is right.performStop→resetPublishedState()really does zerotipHeight(SwiftDashSDKSPVCoordinator.swift:624-627), and it is re-seeded only from$spvProgress(:582) — so moving the tip read before the reload is required, not incidental, and anchoring on the older tip is the conservative direction.- The rewind is safe on the Rust side too:
spv_rescan_filters_blocking(rs-platform-wallet/src/manager/accessors.rs:595) ignores anyfrom_height >= current_height, so a stale tip cannot advance the checkpoint and skip filters. manager.shutdown()does not flush in-memory wallet state to SwiftData, so the reload cannot resurrect the deleted rows.RefreshTriggerhas exactly oneswitchover it, so the new case breaks no other exhaustive switch, and no test references these symbols.
One inline comment: making the reload real also means a failed reload is now real, and that path currently ends with the user stranded.
Non-blocking recommendation:
SwiftDashSDKWalletRuntime.swift:553— the PR body notes the missingWalletLifecycleTransitionStategate and concludes the serial queue makes it harmless. There is one consequence that survives the ordering argument.switchNetworkwrites the network key (:253) before enqueueing its own.networkDidChangerefresh (:258). If a lifecycle op is already running when a removal enqueues its rows-changed refresh, that refresh is still queued when the user starts a network switch; it then resolves the already-flipped key, never elides, and rebuilds for the destination network.switchNetwork's own refresh then findsisRuntimeReady(destination) == trueand elides — sopublishActiveWalletDidChange(reason: "network-changed")never fires, even though the switch reports success andfinish()es the transition. That publish is deliberately deferred until the destination runtime is bound (see the comment at :464-470) so consumers re-read destination state, andSwiftDashSDKContactsService(:136) observes onlyactiveWalletDidChangeNotification— it would keep the pre-switch contact/ownerId snapshot until some unrelated SwiftData save refreshed it. Taking the same admission gate the switches use, or publishing for.walletRowsChangedas well, closes it.
🤖 Reviewed with Claude Code
Making the reload real also made a failed reload real. `refresh` starts Core SPV and then BLAST and falls back to `fullReset` if either throws, so a Platform outage — routine on a flaky DAPI path, and unrelated to the Core-side row being repaired — stops the host and Core SPV as well. The removal still reported plain success and sent the user to Rescan Filters, which refuses while SPV is stopped: a stopped wallet, a zero balance, and an instruction that cannot be followed until relaunch. Replace the `rescanArmed` boolean with a `RemovalOutcome` that separates `.runtimeStopped` from `.rescanUnavailable`, decided by `isRuntimeReady(for:)` after the reload. Both callers now name the remedy that actually works in that state — reopen the app, or Sync Now, which rebuilds the runtime in-session — instead of Rescan Filters. Also publish `activeWalletDidChange` for `.walletRowsChanged`. A queued rows-changed refresh can resolve a network key an interactive switch has already flipped and rebuild for the destination; the switch's own refresh then finds that runtime ready and elides, dropping its "network-changed" publish, and consumers that observe only this notification keep their pre-switch snapshot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
3 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
"Remove if Not on Network" (tx detail sheet, #954) and "Drop unconfirmed" (Core Sync Status, #982) never actually reloaded the runtime, so the coins they promise to free stayed unspendable until the app was relaunched.
There is no removal API at any FFI layer, so removal is done as persistence surgery plus a full runtime reload — on load the Rust wallet rehydrates its tx set, UTXOs and
spent_outpointsfrom the rows.UnconfirmedTransactionRemover's header states this contract, and the comment directly above the call describes the reload.The reload was requested via
rearmPlatformSync():isRuntimeReadyis true when the host has a bound wallet and BLAST runs on the network and Core SPV runs. At removal time all three hold, so every removal logged "refresh is already satisfied" and returned without a reload. Consequences until the next launch:The UI meanwhile said "Transaction removed", and the confirmation dialog promised "the coins it was trying to spend become available again".
This is a seam between two individually-correct changes:
rearmPlatformSyncwas added indde57909dforPlatformAddressSyncCoordinator.syncNow(), which reaches it only when BLAST is down — there the elision is right and desirable. The remover reused it two weeks later for a case where the same elision defeats the operation. Nothing in the name or signature signals that the call can legitimately do nothing.What was done?
1. A trigger that is never elided.
.walletRowsChangedjoins.walletMaterialChangedand.walletDidChangeon the never-elide arm ofshouldSkipRefresh, reached through a newreloadAfterWalletRowsChanged(). The semantics mirror.walletDidChange: there the runtime looks ready but the registry points at another wallet; here it looks ready but the rows changed underneath it. In both cases the "ready" runtime is exactly the one holding stale state.rearmPlatformSynckeeps its elision — unchanged for its own caller. Both entry points now share oneawaitRefresh(trigger:)helper rather than a second copy of the enqueue-and-await shape.2. Rescan depth is derived before the reload. This is required for the fix, not incidental. Once the reload really runs,
fullResetstops SPV andresetPublishedStatezeroes the coordinator's publishedtipHeight, which only a later$spvProgresstick re-seeds. The existing code read the tip after the call — fine while nothing reloaded, but it would now find0and skip the rescan on every removal, returningrescanArmed = falseand telling users to run Rescan Filters by hand.That rescan is the recovery step that restores a wrongly-removed on-chain transaction, and on the bulk path it is the only safety rail because that path never explorer-checks. Fixing the reload without this would have traded one broken guarantee for another. Anchoring on the pre-reload tip is strictly conservative: the tip only advances, so the older value rewinds deeper, never shallower, inside a window already floored at 720 blocks with a 576-block margin and documented as uncapped in depth.
The skip log now names which of the two causes fired instead of asserting "SPV not running after reload", which stopped being true for one of them.
Scope: iOS only. No platform/FFI changes and no
DashSDKFFI.xcframeworkrebuild — this fixes only when the app reloads its own runtime.How Has This Been Tested?
Build:
dashpayscheme, Debug, iPhone 17 simulator,ARCHS=arm64—BUILD SUCCEEDED. Both changed files appear in the compiler invocations. The only warning in either file (entryQueue/dispatchOnPipeline,SwiftDashSDKWalletRuntime.swift:378) is pre-existing and identical ondevelop, several hundred lines from any change here.Unit tests: could not be run.
DashWalletTestsdoes not compile ondevelop, independent of this PR. Thedashwallet-dashpayscheme builds onlydashpay.app+DashWalletTests.xctest— thedashwallettarget never builds — yet 14 of the test files@testable import dashwallet(39 importdashpay), so the module cannot resolve:That file carries the same import on clean
develop, and this PR touches no test file and no scheme.-only-testingdoes not help, since the whole target must compile first. Worth a separate fix.Not yet done — manual smoke. Reproducing a genuinely stuck unconfirmed transaction takes a real network-dropped send, so the end-to-end proof is still outstanding. What to check:
🧭 RUNTIME :: refreshing runtime for walletRowsChangednot followed by "refresh is already satisfied", bracketed by🛰️ SPVCOORD :: stopped/:: started🗑️ TX-REMOVE :: filter rescan armed from height Nstill appears (proves the pre-reload tip ordering)Happy to hold the merge until someone runs that.
Breaking Changes
None.
rearmPlatformSyncand its caller are behaviourally unchanged.The intended behaviour change: removing an unconfirmed transaction now really tears down and restarts the runtime, which takes seconds. Both callers already cover it (progress HUD on the tx sheet, button spinner on the Core Sync screen) and both already describe the await as spanning the reload.
Noted, not fixed here: neither caller takes the
WalletLifecycleTransitionStateadmission gate that network and wallet switches use, so a removal can overlap a switch. The serial lifecycle queue still orders them, so this is not a correctness bug, but the overlay can now show a switch while a removal reload is in flight. Harmless while the reload was a no-op; worth its own fix now that it isn't.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
User Experience