fix(swift-sdk): fixed mempool tx categorization after restart#3777
Conversation
📝 WalkthroughWalkthroughGates UTXO spent-state writes on whether the spending transaction is in-block (or later). Adds ChangesTransaction spend-state confirmation gating
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
✅ Review complete (commit d20ddee) |
ZocoLini
force-pushed
the
fix/tx-categorization
branch
from
e3c20ea to
f9c28f8
Compare
![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
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 660-675: The current logic unconditionally derives txo.isSpent
from the incoming spendingTransaction and will clear a previously confirmed
spend when a later unconfirmed conflicting mempool sighting arrives; change the
update so that if the existing txo.spendingTransaction is confirmed (use
Self.spendIsInBlock on txo.spendingTransaction) and the incoming
spendingTransaction is unconfirmed (expectedIsSpent == false) and has a
different txid than txo.spendingTransaction, then skip changing txo.isSpent and
do not overwrite the canonical spending link; only allow the update when the
incoming spender is confirmed, or when the incoming txid equals the existing one
(same-tx downgrade/upgrade) or when an explicit reorg downgrade path is taken.
Apply the same guard to the other similar blocks around the indicated regions
(lines ~843-872 and ~901-922) referencing the same symbols: txo,
spendingTransaction, spendingTxid, inputIndex, and Self.spendIsInBlock.
🪄 Autofix (Beta)
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
Run ID: aa0944c8-4597-4813-ad8c-c8517d9f145c
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "e44fd4b120e798325af07d5e9c97fb7a567acc18869dc0754b3ca8ab9da145cb"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR correctly identifies and partially fixes the mempool self-send restart-misclassification bug. However, spendIsInBlock is too strict: it excludes instantSend, which this codebase already treats as confirmed (CoreBalance.confirmed doc string says "in a block or InstantSend-locked"). Combined with loadWalletList's isSpent == false filter, this regresses InstantSend spends across a restart, re-feeding the spent UTXO to Rust as unspent. A second, narrower concern: markUtxoSpent now flips isSpent back to false whenever the spending tx can't be resolved, including the empty/zero spending_txid guard path, which previously was an authoritative spend signal.
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(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`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:639-641: InstantSend-locked spends regress to unspent across a restart
`TransactionContextType` ordering is `mempool=0, instantSend=1, inBlock=2, inChainLockedBlock=3`, so `tx.context >= inBlock.rawValue` excludes `instantSend`. The stated reason for the new gate — "reversible by RBF or mempool eviction" — does not apply to InstantSend: by Dash design it cannot be reorged out and cannot be replaced. Treating it the same as mempool produces two concrete regressions:
1. While the spending tx is `instantSend` but not yet in a block, the input TXO keeps `isSpent == false`, so the wallet UI/queries that read SwiftData see the row as still spendable.
2. Across a restart in that window, `loadWalletList` (line 2948) filters TXOs by `isSpent == false` and hands the row back to Rust as a live UTXO. Nothing in the restore path rebuilds ordinary spending transactions into Rust's in-memory graph, so the spent input is silently re-credited until a later block update arrives, which can overstate spendable funds and enable a duplicate-spend attempt.
This contradicts the project's own contract — `ManagedCoreWallet.CoreBalance.confirmed`'s doc literally says "Confirmed balance (in a block or InstantSend-locked)" — so the persistence layer should follow the same definition.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:915-921: `markUtxoSpent` can now clear a previously-set `isSpent`
Before this PR, `markUtxoSpent` was an authoritative spend signal: once the TXO was located, `isSpent` was set to `true` unconditionally. After this change the tail-write is `txo.isSpent = spendingTx.map(Self.spendIsInBlock) ?? false`, which can flip `isSpent` from `true` back to `false` in three paths:
1. The guard at line 902 leaves `spendingTx == nil` when the FFI emits an empty/all-zero `spending_txid`. The fact that this guard exists implies Rust can emit that case — and the legacy semantic there was "mark as spent anyway".
2. The spending tx hasn't been persisted yet this flush. The comment promises self-healing on the next flush, but until then the TXO observably toggles back to spendable.
3. A TXO already confirmed-spent in an earlier flush is re-emitted here with the spending tx's record evicted from this `backgroundContext` — `isSpent` goes `true → false`.
The PR's stated intent ("don't permanently flip `isSpent` from a mempool-only sighting") only requires the negative case for mempool. An upgrade-only rule — never clear `isSpent` here, only set `false → true` when the spending tx is confirmed — preserves the fix while keeping the existing authoritative-signal semantics. Same shape should be applied at the deferred-pending drain (line 872) for consistency.
|
I will be taking a look into pasta claw complains to be able to preperly answer them |
ZocoLini
force-pushed
the
fix/tx-categorization
branch
from
f9c28f8 to
95b219d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR's gate on isSpent writes is intentionally scoped to block-confirmed contexts so mempool/IS-locked spends can be re-identified by the catch-up classifier on restart — this is explicitly stated in the doc comment and the linkage comment. Codex's blocking flag on InstantSend exclusion contradicts this documented design and is dropped as a false positive. The remaining valid feedback is a real doc-comment typo (unterminated backtick) plus low-priority suggestions around test coverage and clarifying the IS exclusion rationale.
💬 3 nitpick(s)
1 additional finding(s) omitted (not in diff).
ZocoLini
force-pushed
the
fix/tx-categorization
branch
from
95b219d to
d20ddee
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (1)
659-675:⚠️ Potential issue | 🟠 MajorPreserve an existing confirmed spender against later unconfirmed conflicts.
These paths still derive
isSpent/spendingTransactionfrom the incoming or selected spender only. If a TXO is already linked to confirmed spend A and a later mempool-only conflicting spend B is processed, this code can overwrite the canonical link with B and clearisSpentback tofalse. BecauseloadWalletList()still rebuilds the restore set fromisSpent == falseat Line 2952, a restart can rehydrate a confirmed-spent output as live again.Only allow the overwrite when the incoming spender is confirmed or matches the existing txid; if the existing linked spender is confirmed and the new spender is a different unconfirmed tx, keep the current link/flag.
Also applies to: 843-873, 901-925
🤖 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 659 - 675, The code currently overwrites txo.isSpent and txo.spendingTransaction with any incoming spender, which allows a confirmed existing spender to be replaced by a later unconfirmed conflicting spender; change the logic in the block that computes linkageChanged (and the subsequent mutation around spendIsInBlock(spendingTransaction), txo.spendingTransaction, spendingTxid, and isSpent) so that you only overwrite the existing link if the incoming spendingTransaction is confirmed (Self.spendIsInBlock(...) == true) or the incoming spendingTxid equals the already-linked txo.spendingTransaction?.txid; if the existing txo.spendingTransaction is confirmed and the incoming one is unconfirmed and has a different txid, do not change txo.isSpent or txo.spendingTransaction (preserve the canonical confirmed spender); apply the same guard to the other similar blocks noted (around the other ranges) and ensure loadWalletList rebuilding logic remains consistent with isSpent semantics.
🤖 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.
Duplicate comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 659-675: The code currently overwrites txo.isSpent and
txo.spendingTransaction with any incoming spender, which allows a confirmed
existing spender to be replaced by a later unconfirmed conflicting spender;
change the logic in the block that computes linkageChanged (and the subsequent
mutation around spendIsInBlock(spendingTransaction), txo.spendingTransaction,
spendingTxid, and isSpent) so that you only overwrite the existing link if the
incoming spendingTransaction is confirmed (Self.spendIsInBlock(...) == true) or
the incoming spendingTxid equals the already-linked
txo.spendingTransaction?.txid; if the existing txo.spendingTransaction is
confirmed and the incoming one is unconfirmed and has a different txid, do not
change txo.isSpent or txo.spendingTransaction (preserve the canonical confirmed
spender); apply the same guard to the other similar blocks noted (around the
other ranges) and ensure loadWalletList rebuilding logic remains consistent with
isSpent semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 960f2a02-4a55-4968-b6e0-97d394741117
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (95b219d→d20ddee3) is a single-character doc fix (closing backtick on isSpent). The prior 'backtick typo' finding is FIXED; the prior 'document/clarify InstantSend exclusion' and 'add restart regression test' findings are STILL VALID against the head — they are carried forward below. The de novo concern surfaced this round is the symmetric isSpent = expectedIsSpent write in resolveInputOutpoint, which can now flip a previously-true isSpent to false if a stale/conflicting mempool spender for the same outpoint is observed after a confirmed spender — this is a behavioral regression introduced by this PR (pre-PR code unconditionally wrote true). It does not justify blocking on its own because it depends on an unusual conflict/reorg emit sequence, but it is the most important new observation in the cumulative state. Recommending COMMENT.
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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:667-675: New write path can flip `isSpent` from true back to false on a stale/conflicting mempool spender
The pre-PR code in `resolveInputOutpoint` unconditionally set `txo.isSpent = true`. The new code computes `expectedIsSpent = spendIsInBlock(spendingTransaction)` and writes it whenever `linkageChanged` is true. This is the intended fix when an unconfirmed spender arrives first — but it also means: if a TXO is already linked to a confirmed spender A (`isSpent == true`), and a later stale or conflicting mempool observation B for the same outpoint flows through this path, then `expectedIsSpent` is false, `linkageChanged` is true, and the code clears `isSpent` and reassigns `spendingTransaction` to B. Because `loadWalletList()` restores rows where `isSpent == false`, that TXO becomes re-eligible as spendable, leading to balance overcount and a possible user-driven double-spend attempt. The same shape exists in the pending-input drain path at line 871–872. Consider only allowing the spender to be replaced (and `isSpent` lowered) when the existing linked spender is also unconfirmed, the same txid is changing context, or an explicit reorg downgrade signal is present. Pre-PR behavior masked this because `true` was always rewritten; this PR makes the downward flip newly reachable.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:630-640: [Carried forward] Document the deliberate InstantSend exclusion in `spendIsInBlock`
`TransactionContextType` is `mempool=0, instantSend=1, inBlock=2, inChainLockedBlock=3` (KeyWallet/TransactionContext.swift:4-8). The threshold `tx.context >= inBlock.rawValue` therefore groups `instantSend` with `mempool` as 'don't flip `isSpent`'. The justification given in the comment is reversibility via RBF or mempool eviction — but an InstantSend-locked tx is precisely the case that is NOT reversible by RBF or eviction. Two consequences:
* For the bug being fixed (restart classifier needs the input TXO back as `isSpent == false`), lumping InstantSend with mempool is actually safe and arguably required, since the catch-up classifier still needs the input handed back until the spending tx lands in a block.
* For UI surfaces that read `isSpent == false` as 'spendable', an InstantSend-locked spend will appear spendable until the block confirms — a spendable-balance overcount window.
A future maintainer reading only the comment's RBF/eviction justification could reasonably widen the predicate to `>= instantSend.rawValue` and silently reintroduce the restart-classifier bug. Either widen the comment to explicitly call out InstantSend (intentional, and why), or — if the intent really is 'is this spend safely final' — change the threshold to `> mempool`. Recommend the doc-clarification approach so the restart-classifier contract stays intact.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:628-936: [Carried forward] No regression test for the restart + mempool sighting + classifier sequence
The PR replaces three previously unconditional `isSpent = true` writes with a context-gated path. The behavior change is meaningful in at least four flows: (1) mempool-only sighting then restart then block (the bug being fixed), (2) confirmed sighting in-order, (3) pending-row resolution in `upsertUtxo` when spending tx is mempool vs in-block, (4) `markUtxoSpent` with a missing spending-tx record (now leaves `isSpent` untouched instead of writing `true`). All four are realistic; (4) in particular silently changes a previously-loud signal into a no-op. There is no Swift test in `packages/swift-sdk` exercising the persist→mempool-sighting→restart→block sequence (no `testSelfSendClassificationSurvivesMidFlightRestart` or equivalent). Given the bug being fixed was a production-observed `netAmount` direction flip with inflated value, a regression test asserting `isSpent == true` and correct classification after that sequence would meaningfully harden the fix.
3 participants
After a mid-flight restart between mempool sighting and block confirmation, a self-send tx was misclassified as Incoming with netAmount = +sum_of_outputs. The persister flipped PersistentTxo.isSpent = true on the mempool sighting, and loadWalletList filters by isSpent == false, so the input TXO was dropped from the restored set — the catch-up classifier on the next launch saw an "unknown" input and emitted Incoming.
Fix: gate the isSpent write on the spending tx being in a block (context >= inBlock). Mempool sightings still link the spendingTransaction relationship but keep the row in the unspent set. The next upsert with a confirmed context flips isSpent then.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit