fix(platform-wallet): prevent concurrent UTXO double-spend via outpoint reservations#3585
fix(platform-wallet): prevent concurrent UTXO double-spend via outpoint reservations#3585Claudius-Maginificent wants to merge 52 commits into
Conversation
`Sdk::resolve_dpns_name` stripped the `.dash` suffix using exact byte-match. Inputs like "Alice.DASH" or "alice.Dash" fell into the else branch and the entire string was treated as the label, missing the DPNS lookup even though DPNS itself stores `normalizedLabel` lowercased. Backport from dash-evo-tool PR #810 / platform PR #3466 fix 1. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
…dresses
`CoreWallet::send_to_addresses` had a TOCTOU window between dropping
the wallet write lock (after build/select/sign) and broadcasting the
transaction. Mempool / block events processed before the build lock
was acquired could invalidate selected UTXOs, leaving the caller with
an opaque network rejection.
Pattern (Option A — defer-mark-spent):
1. While still holding the write lock used to build the transaction,
re-validate that every selected outpoint is still in the spendable
set. If any are gone, return `TransactionBuild("Selected UTXOs are
no longer available (concurrent transaction). Please retry.")` so
callers can retry on a fresh UTXO snapshot.
2. Drop the lock and broadcast.
3. Only on broadcast success, re-acquire the write lock and call
`check_core_transaction(.., TransactionContext::Mempool, .., true,
true)` to mark the inputs spent in the local wallet view.
Marking spent strictly after broadcast addresses the review concern
on PR #3466 that the original "mark spent before broadcast" ordering
would corrupt local state on transient broadcast failures.
The original PR #3466 patched `CoreWallet::send_transaction`. That
function no longer exists post-rewrite around `TransactionBuilder`
(see the `feat(platform-wallet): CoreWallet FFI ... TransactionBuilder
integration` and `refactor(platform-wallet): collapse 7+ locks into
single RwLock` migrations). Same bug, different call site, same
optimistic-validation cure.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds runtime UTXO revalidation with a ConcurrentSpendConflict error, changes change-address handling to a two‑phase (peek then advance) flow around broadcast, registers mempool spends after broadcasting, and introduces DPNS helpers to strip a case‑insensitive ChangesWallet Broadcast UTXO Revalidation & Mempool Registration
DPNS Case-Insensitive Suffix Parsing
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TxBuilder
participant WalletState
participant Broadcaster
participant MempoolChecker
Caller->>TxBuilder: build transaction & select inputs
TxBuilder-->>Caller: built tx + selected outpoints
Caller->>WalletState: peek change address (no advance)
Caller->>WalletState: query current spendable outpoints
WalletState-->>Caller: spendable set
Caller->>Caller: compare selected outpoints vs spendable
alt any selected UTXO unavailable
Caller-->>Caller: return ConcurrentSpendConflict
else all available
Caller->>Broadcaster: broadcast raw tx
Broadcaster-->>Caller: broadcast result (txid)
Caller->>MempoolChecker: check_core_transaction(TransactionContext::Mempool)
MempoolChecker-->>WalletState: register mempool spend / advance change address
WalletState-->>Caller: updated state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 e952a05) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rs-platform-wallet/src/wallet/core/broadcast.rs (1)
177-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't surface a post-broadcast bookkeeping miss as a send failure.
After Line 167 the transaction may already be on the network, but Lines 177-184 can still return
WalletNotFound, so the caller can observe an error for a payment that may already have succeeded. This post-broadcast registration step should be best-effort instead of changing the outcome ofsend_to_addresses. Please also verify thatcheck_core_transactionis truly infallible here; if it returns a status orResult, swallowing it would leave local spend-state stale until the next sync.Suggested direction
{ let mut wm = self.wallet_manager.write().await; - let (wallet, info) = - wm.get_wallet_mut_and_info_mut(&self.wallet_id) - .ok_or_else(|| { - crate::error::PlatformWalletError::WalletNotFound( - "Wallet not found in wallet manager".to_string(), - ) - })?; - info.check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) - .await; + if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&self.wallet_id) { + info.check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) + .await; + } }#!/bin/bash set -euo pipefail # Verify the contract of WalletTransactionChecker::check_core_transaction. # Expected result: ideally this resolves to `-> ()`. If it returns a status or # Result, handle/log that outcome here instead of discarding it. rg -n -C4 --type=rust 'trait\s+WalletTransactionChecker|fn\s+check_core_transaction\s*\('🤖 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/rs-platform-wallet/src/wallet/core/broadcast.rs` around lines 177 - 186, After broadcasting, do not let post-broadcast bookkeeping failures turn into a send failure: change the block that acquires self.wallet_manager, calls get_wallet_mut_and_info_mut(&self.wallet_id) and invokes info.check_core_transaction(...) so that a missing wallet (WalletNotFound) or any error/status from check_core_transaction is treated as best-effort—log the condition via crate::error or process logger and continue returning success from send_to_addresses; specifically, catch the None from wm.get_wallet_mut_and_info_mut(&self.wallet_id) and do not map it into an Err return, and inspect WalletTransactionChecker::check_core_transaction's signature (if it returns Result/Status, await and log any Err/negative status instead of discarding or propagating it).
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/dpns_usernames/mod.rs (1)
427-439: ⚡ Quick winConsider extracting label parsing into a tested helper.
The label-extraction block (lines 427–439) is the heart of the bug fix being shipped in this PR, but there is no unit test covering the new case-insensitive path. Because the logic is purely synchronous and has no dependency on
Sdkor the network, it can be extracted into a private helper and tested trivially alongside the other#[test]cases in the same file.♻️ Suggested extraction + test
+/// Extract the DPNS label from a full domain name or bare label. +/// +/// Strips the `.dash` suffix case-insensitively (e.g. "alice.DASH" → "alice"). +/// If the suffix is not `.dash`, the whole input is returned as-is. +fn extract_dpns_label(name: &str) -> &str { + if let Some(dot_pos) = name.rfind('.') { + let (label_part, suffix) = name.split_at(dot_pos); + if suffix.eq_ignore_ascii_case(".dash") { + return label_part; + } + } + name +}Then in
resolve_dpns_name:- let label = if let Some(dot_pos) = name.rfind('.') { - let (label_part, suffix) = name.split_at(dot_pos); - // Strip ".dash" / ".DASH" / mixed case — DPNS itself is case-insensitive. - if suffix.eq_ignore_ascii_case(".dash") { - label_part - } else { - // If it's not ".dash", treat the whole thing as the label - name - } - } else { - // No dot found, use the whole name as the label - name - }; + let label = extract_dpns_label(name);And in the test module:
+ #[test] + fn test_extract_dpns_label() { + assert_eq!(extract_dpns_label("alice.dash"), "alice"); + assert_eq!(extract_dpns_label("alice.DASH"), "alice"); + assert_eq!(extract_dpns_label("alice.Dash"), "alice"); + assert_eq!(extract_dpns_label("Alice.DASH"), "Alice"); + assert_eq!(extract_dpns_label("alice"), "alice"); + assert_eq!(extract_dpns_label(".dash"), ""); + assert_eq!(extract_dpns_label("alice.bob"), "alice.bob"); + }🤖 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/rs-sdk/src/platform/dpns_usernames/mod.rs` around lines 427 - 439, Extract the label-extraction block inside resolve_dpns_name into a private function (e.g., fn extract_dpns_label(name: &str) -> &str) and replace the inline logic in resolve_dpns_name with a call to that helper; ensure the helper implements the same behavior (uses rfind('.') then split_at, treats suffix.eq_ignore_ascii_case(".dash") as stripping the suffix, otherwise returns the whole name). Add unit tests in the same test module that call extract_dpns_label directly to cover cases: names without dot, names with non-.dash suffix, and mixed-case ".DaSh" suffix to verify case-insensitive stripping. Ensure the helper is private (no public API change) and update resolve_dpns_name to use it.
🤖 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/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 177-186: After broadcasting, do not let post-broadcast bookkeeping
failures turn into a send failure: change the block that acquires
self.wallet_manager, calls get_wallet_mut_and_info_mut(&self.wallet_id) and
invokes info.check_core_transaction(...) so that a missing wallet
(WalletNotFound) or any error/status from check_core_transaction is treated as
best-effort—log the condition via crate::error or process logger and continue
returning success from send_to_addresses; specifically, catch the None from
wm.get_wallet_mut_and_info_mut(&self.wallet_id) and do not map it into an Err
return, and inspect WalletTransactionChecker::check_core_transaction's signature
(if it returns Result/Status, await and log any Err/negative status instead of
discarding or propagating it).
---
Nitpick comments:
In `@packages/rs-sdk/src/platform/dpns_usernames/mod.rs`:
- Around line 427-439: Extract the label-extraction block inside
resolve_dpns_name into a private function (e.g., fn extract_dpns_label(name:
&str) -> &str) and replace the inline logic in resolve_dpns_name with a call to
that helper; ensure the helper implements the same behavior (uses rfind('.')
then split_at, treats suffix.eq_ignore_ascii_case(".dash") as stripping the
suffix, otherwise returns the whole name). Add unit tests in the same test
module that call extract_dpns_label directly to cover cases: names without dot,
names with non-.dash suffix, and mixed-case ".DaSh" suffix to verify
case-insensitive stripping. Ensure the helper is private (no public API change)
and update resolve_dpns_name to use it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec32c25e-b849-4962-aaf6-717d236950e7
📒 Files selected for processing (2)
packages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-sdk/src/platform/dpns_usernames/mod.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR fixes the prior #3466 blocking finding: the wallet now broadcasts before mutating spend state, and only marks inputs spent on broadcast success. Remaining items are non-blocking suggestions — a discarded TransactionCheckResult, an in-lock revalidation whose stated rationale doesn't match the code (the lock is held continuously), and no automated tests for either path.
Reviewed commit: 0d17a63
🟡 4 suggestion(s)
1 additional finding
🟡 suggestion: DPNS case-insensitive suffix stripping has no unit test
packages/rs-sdk/src/platform/dpns_usernames/mod.rs (lines 427-439)
The suffix-stripping logic at lines 427-439 is purely string-based and trivially unit-testable, but the only existing tests in packages/rs-sdk/tests/dpns_queries_test.rs are network-gated and only exercise lowercase inputs. Add unit cases for "alice.DASH", "alice.Dash", "alice.eth" (treated as full label), and ".dash" (empty label → Ok(None)) — extracting the label-extraction step into a private helper if needed — to lock in the case-insensitive behavior against regression.
🤖 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/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 185-186: TransactionCheckResult is silently discarded after broadcast
`info.check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true).await` returns a `TransactionCheckResult` (see `packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs:170-182`) but the value is dropped at the `;`. This is the call whose entire purpose is to mark the just-broadcast inputs as spent. If it reports the transaction as not relevant (e.g., due to accounting drift between the selection lock and the post-broadcast lock acquisition), the UTXOs are not actually marked spent and the function still returns `Ok(tx)` — silently re-creating the very re-selection scenario this PR aims to prevent. Since the wallet itself constructed `tx` from its own UTXOs, a non-relevant result here is an invariant violation: at minimum log a warning, and ideally bind the result and assert / surface a tracing event when relevance is unexpected.
- [SUGGESTION] lines 137-160: In-lock revalidation's stated rationale doesn't match the code, and uses a different spendable view than selection
Two related issues with this defensive block:
1. The comment claims the check guards against "external mempool / block events processed before we acquired the lock" — but the `wallet_manager.write().await` guard from line 50 is held continuously through `select_inputs` (line 116) and this revalidation, so any such event would have applied *before* `select_inputs` ran and cannot invalidate the just-selected inputs. By construction, `selected ⊆ still_spendable` for any concurrent mutation path that goes through the wallet manager.
2. The only thing the check can actually catch is a disagreement between the two spendable views: selection at lines 78-82 uses `account.spendable_utxos(current_height)` (per-account, height-aware — coinbase maturity, lock heights), while revalidation at lines 149-153 uses `info.get_spendable_utxos()` which forwards to `core_wallet.get_spendable_utxos()` (wallet-wide, no `current_height` argument; see `platform_wallet_traits.rs:101-103`). If those views disagree on membership, this block produces a spurious "concurrent transaction, please retry" error rather than meaningful protection.
Either delete the block (the real race the PR addresses — lock-drop / broadcast / lock-reacquire — is correctly handled by broadcast-then-mark on line 167-186), or rewrite the comment to describe the concrete filter-disagreement scenario it guards against, and switch the revalidation to query the same per-account, height-aware view used during selection.
- [SUGGESTION] lines 34-190: No automated tests for the new race-prevention ordering
The PR test plan defers concurrent-broadcast and broadcast-failure verification to manual testing. The two contracts this PR establishes are both unit-testable against a mocked `TransactionBroadcaster`:
1. On broadcast `Err`, no UTXO state is mutated — i.e., `check_core_transaction` is never invoked. This is the core regression-prevention guarantee for #3466.
2. If a selected outpoint is independently marked spent between selection and the post-build revalidation, `send_to_addresses` returns `PlatformWalletError::TransactionBuild("Selected UTXOs are no longer available...")` and never reaches the broadcaster.
Without these tests, a future refactor that reorders broadcast and `check_core_transaction` would silently re-introduce #3466.
In `packages/rs-sdk/src/platform/dpns_usernames/mod.rs`:
- [SUGGESTION] lines 427-439: DPNS case-insensitive suffix stripping has no unit test
The suffix-stripping logic at lines 427-439 is purely string-based and trivially unit-testable, but the only existing tests in `packages/rs-sdk/tests/dpns_queries_test.rs` are network-gated and only exercise lowercase inputs. Add unit cases for `"alice.DASH"`, `"alice.Dash"`, `"alice.eth"` (treated as full label), and `".dash"` (empty label → `Ok(None)`) — extracting the label-extraction step into a private helper if needed — to lock in the case-insensitive behavior against regression.
|
Opened draft follow-up PR to address the review feedback here: It covers the intentional ambiguous broadcast-error comment, makes post-broadcast wallet bookkeeping best-effort with warnings, binds/checks |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two unrelated fixes bundled: a one-line case-insensitive .dash suffix in resolve_dpns_name (correct), and a UTXO double-spend prevention reorder in send_to_addresses that broadcasts first and marks inputs spent only on success (correct in spirit, but with several rough edges). No blockers. Main gaps are missing tests and a few state-divergence/error-modeling cleanups in the wallet broadcast path.
Reviewed commit: 0d17a63
🟡 5 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🟡 suggestion: No unit test for the case-insensitive `.dash` suffix fix
packages/rs-sdk/src/platform/dpns_usernames/mod.rs (lines 427-439)
The bug is a one-line case-sensitivity defect, the fix is a one-line change to eq_ignore_ascii_case, and a pure-logic unit test exercising "Alice.dash", "alice.DASH", "Alice.Dash", "alice", and "alice.eth" (whole-string label fallback) would cost ~10 lines and prevent regression of exactly this class of bug. Lifting the label-extraction logic into a small helper would make it directly testable without an SDK harness. Given the PR description explicitly defers manual verification, an offline unit test is the cheapest insurance available.
🤖 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/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 165-187: Post-broadcast state update can silently fail to mark inputs spent
Two failure modes in this block are silent from the perspective of local UTXO state, both of which re-introduce exactly the double-spend the PR is trying to prevent:
1. `check_core_transaction(..)` returns a `TransactionCheckResult` that is discarded. If `is_relevant` comes back `false`, no state mutation happens and the next `send_to_addresses` call will happily reselect the same UTXOs. Since this transaction was just built, signed, and broadcast by *this* wallet, `is_relevant` must always be true; if it isn't, that's a real correctness defect that should be observable.
2. `wm.get_wallet_mut_and_info_mut(..)` may return `None` if the wallet was concurrently removed/replaced. Today this surfaces as `WalletNotFound`, but the tx is already in flight on the network, so local accounting is permanently desynchronized for any still-present wallet handle.
At minimum, log/assert on the `TransactionCheckResult` and emit a distinct error/log when post-broadcast lookup fails so callers know a manual sync may be required.
- [SUGGESTION] lines 109-160: Change-address index is advanced before the new early-return path
`next_change_address(Some(&xpub), true)` at line 110 advances the change-address derivation index (the `true` is the mark-used/advance flag). With the new revalidation branch, control can take a fresh `return Err(...)` at line 154 after that index has already been advanced, leaving a derived-but-never-used change address — i.e. a gap address. A retry then derives yet another address, growing the gap. This same shape exists for the pre-existing build/select/sign error paths, but the PR adds a new branch that exercises it. Defer the change-address advance until after revalidation succeeds (or after broadcast), or compute the address without advancing and commit only on success.
- [SUGGESTION] lines 154-159: Retryable UTXO-race collapsed into a generic `TransactionBuild(String)` error
This branch is the new retry contract introduced by the PR, but it surfaces as `PlatformWalletError::TransactionBuild("Selected UTXOs are no longer available...")`. Callers (FFI layers, UI code, retry loops) can only distinguish it by parsing the message, which is brittle across refactors and any localization changes. Model it as a dedicated enum variant — e.g. `ConcurrentSpendConflict` or `RetryableUtxoConflict` — so retry logic can match on it directly and the rest of the codebase keeps treating `TransactionBuild` as a true builder failure.
- [SUGGESTION] lines 137-187: No test for UTXO race protection or broadcast-failure rollback
Both behaviors the PR claims to deliver are testable with a mocked `TransactionBroadcaster` — `CoreWallet` is already generic over it, so the seam exists:
1. On broadcast failure, the wallet's UTXO set must be unchanged afterwards (the key invariant fixed in this PR vs. the original mark-spent-before-broadcast version in #3466).
2. Two interleaved `send_to_addresses` calls against a single-UTXO wallet must produce the documented retry error rather than corrupted state.
3. After a successful broadcast, the inputs must be observable as spent on the next `get_spendable_utxos()` call.
Without these, regressions to either ordering bug will not show up in unit coverage, and the manual checkboxes in the PR description are deferred. A couple of targeted async tests here would fit naturally alongside the rest of `rs-platform-wallet`.
In `packages/rs-sdk/src/platform/dpns_usernames/mod.rs`:
- [SUGGESTION] lines 427-439: No unit test for the case-insensitive `.dash` suffix fix
The bug is a one-line case-sensitivity defect, the fix is a one-line change to `eq_ignore_ascii_case`, and a pure-logic unit test exercising `"Alice.dash"`, `"alice.DASH"`, `"Alice.Dash"`, `"alice"`, and `"alice.eth"` (whole-string label fallback) would cost ~10 lines and prevent regression of exactly this class of bug. Lifting the label-extraction logic into a small helper would make it directly testable without an SDK harness. Given the PR description explicitly defers manual verification, an offline unit test is the cheapest insurance available.
Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two-bug fix PR validates cleanly: DPNS suffix change is correct and tested; the wallet broadcast reorder addresses the prior #3466 state-corruption issue by broadcasting before mutating local state. No blockers. Remaining items are non-blocking: a small DPNS API ordering inconsistency, a defensive subset-check that is effectively unreachable, missing automated coverage for the new broadcast-first ordering (flagged by both agents), and refinements around the post-broadcast wallet-registration paths (silent !is_relevant, untyped retryable error, race window, telemetry).
Reviewed commit: 1bd306a
🟡 3 suggestion(s) | 💬 4 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/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 146-149: Retryable UTXO-conflict path is encoded as a generic TransactionBuild(String)
If the subset check is kept as a runtime check (or repurposed for an actual concurrent-spend race), surfacing it as PlatformWalletError::TransactionBuild("...Please retry.") forces FFI/UI/retry callers to string-match a human message to distinguish a retryable conflict from a real construction failure. PlatformWalletError has no structured variant for this condition (see packages/rs-platform-wallet/src/error.rs). Add a typed variant (e.g. ConcurrentUtxoConflict) so consumers can branch on it programmatically.
- [SUGGESTION] lines 154-220: No automated test for the broadcast-first ordering or the failure-rollback contract
The PR's central correctness claim — broadcast failure leaves spendable UTXOs untouched, broadcast success makes them non-spendable for the next caller via post-broadcast check_core_transaction — is exactly the regression flagged on the original #3466. CoreWallet is generic over B: TransactionBroadcaster + ?Sized, so the seam for deterministic unit tests already exists. Two short async tests would lock this in:
1. Inject a broadcaster that returns Err(...) and assert the spendable-UTXO set is byte-identical before vs. after the failed send_to_addresses (no check_core_transaction applied).
2. Inject a broadcaster that returns Ok(...) and assert get_spendable_utxos() afterward no longer contains the spent inputs and includes the change output.
No test in packages/rs-platform-wallet currently exercises send_to_addresses or broadcast_transaction (verified via grep across src/ and tests/). The PR's manual checkboxes for both behaviors are deferred to a running node, which is exactly what a unit test should cover. Without coverage, only code review prevents a future refactor from re-introducing the original mark-spent-before-broadcast bug.
- [SUGGESTION] lines 199-217: Post-broadcast !is_relevant is treated as a transient even for transactions built from this wallet's own UTXOs
After broadcast_transaction succeeds, the only place that records the spend in local state is the check_core_transaction(.., Mempool, ..) call on line 203. Both the !is_relevant path (lines 205-210) and the wallet-missing path (lines 211-217) downgrade to tracing::warn! and the function still returns Ok(tx). For a transaction the wallet just built using its own spendable UTXOs and its own derived change address, !is_relevant indicates a wallet-internal invariant break (xpub mismatch, derivation drift, account map staleness) — not the kind of transient the comment treats it as. Letting it pass silently means the next send_to_addresses can reselect the same inputs and only discover the problem via a network rejection later. Consider distinguishing the two warning branches: keep the wallet-missing branch as best-effort logging, but treat !is_relevant for an own-built transaction as an internal error (or at minimum surface a counter/structured error field) so operators can see it independently of free-form log lines.
…alidation (CMT-007) `send_to_addresses` advanced the change-address derivation index before the post-build revalidation early-return introduced by PR #3585. When revalidation detected a UTXO conflict and bailed out, the change index was still bumped — the derived-but-unused address widened the gap-limit window on every retry. Switch the first call to `next_change_address(Some(&xpub), false)` (peek without persisting), and only commit the advance with `add_to_state = true` after revalidation passes. The peek is idempotent: `next_unused` is deterministic on the locked state, so the commit call returns the same address. The mutable account reborrow is reacquired after `select_inputs` ends its borrow on `info.core_wallet.accounts`. Scope: limited to the new revalidation early-return path; pre-existing build/select/sign error paths still advance early but are out of scope for this PR. Ref: #3585 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tryable UTXO race (CMT-008)
The post-build revalidation early-return surfaced as
`PlatformWalletError::TransactionBuild("Transaction builder selected an
unavailable UTXO. Please retry.")`. FFI/UI/retry-loop callers could only tell
this apart from genuine builder failures by string-matching the message —
brittle across refactors and incompatible with localisation.
Add a dedicated unit variant `PlatformWalletError::ConcurrentSpendConflict`
and use it at the early-return site instead of `TransactionBuild(...)`.
`TransactionBuild` is left for true builder-failure cases.
No callers were string-matching the old "Please retry" wording, so no
caller updates were needed.
Ref: #3585 (comment)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reviewed current head 4616cbae after the v3.1-dev merge. The PR diff against the current base is still limited to the intended wallet broadcast and DPNS username files; the merge commit did not introduce new branch-specific concerns.
No new blockers. Prior non-blocking suggestions from the 1bd306a0 review still stand, but current head validates cleanly for the focused paths.
Validation: git diff --check origin/v3.1-dev...HEAD, cargo test -p dash-sdk --lib platform::dpns_usernames:: (5 passed / 0 failed / 6 ignored), cargo check -p platform-wallet --lib, and gh pr checks (CodeRabbit, PR title, semantic title passing).
|
✅ 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: "a0ffcffd7c335089e751673bedd89cbfda89561bf39096cf3cc06ffe0ffc298a"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reviewed current head a3a5d965 after the two new wallet follow-up commits (CMT-007 / CMT-008). No blockers. The typed ConcurrentSpendConflict error is a useful improvement over stringly retry signaling, and the change-address peek avoids generating/monitoring a fresh address when the post-build UTXO revalidation fails.
Remaining feedback is non-blocking documentation/semantic polish: next_change_address(..., true) does not really “advance” past an unused generated change address in key-wallet; it inserts/keeps that address in the monitored pool, and the later transaction registration is what marks the address used so a future send can move on. So the new “commit the change-address advance” / “next send picks up the next index” comments slightly overstate the state transition. Relatedly, the later “Broadcast first; if the network rejects we leave wallet state untouched” comment should be scoped to spend/UTXO state, since the change address may already have been pre-registered before broadcast. I don’t think either is a correctness blocker, but tightening the wording would make this tricky path easier to maintain.
Validation run locally:
git diff --check origin/v3.1-dev...HEADcargo check -p platform-wallet --libcargo clippy -p platform-wallet --lib -- -D warningscargo test -p dash-sdk --lib platform::dpns_usernames::(5 passed / 0 failed / 6 ignored)
GitHub checks for the new head are still in progress; CodeRabbit, title, and semantic-title checks are passing.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two-fix PR: case-insensitive .dash suffix handling in DPNS and broadcast-first ordering in CoreWallet::send_to_addresses. Both fixes are correct and security-neutral. No blockers. Carry-forward suggestions: API ordering asymmetry in resolve_dpns_name (still does network I/O before empty-label guard), an effectively unreachable subset-check framed as user-retryable, the retryable error encoded as a stringly-typed variant that flattens to ErrorUnknown over FFI, the post-broadcast !is_relevant path silently desyncing local state for self-built transactions, change-address index advanced before paths that may now Err, and missing automated coverage for the new broadcast-first ordering.
Reviewed commit: 4616cba
Fresh dispatcher run for the claimed queue item. A same-SHA automated review already existed, so I am posting this as a top-level review to avoid duplicating inline threads while still recording the fresh verification.
🟡 4 suggestion(s) | 💬 2 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/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 146-149: Retryable UTXO-conflict path is stringly-typed and indistinguishable across FFI
If this branch is kept (or repurposed for a real concurrent-spend race), surfacing it as `PlatformWalletError::TransactionBuild("...Please retry.")` forces consumers to string-match the human message to distinguish a retryable conflict from a true builder failure. `PlatformWalletError` has no structured variant for this condition (see `packages/rs-platform-wallet/src/error.rs`). The problem compounds at the FFI boundary: `impl From<PlatformWalletError> for PlatformWalletFFIResult` in `packages/rs-platform-wallet-ffi/src/error.rs:157-160` flattens every variant to `ErrorUnknown` plus a free-form string, so Swift callers (`ManagedCoreWallet.sendToAddresses`) cannot programmatically branch on this new outcome at all. Add a typed Rust variant (e.g. `ConcurrentUtxoConflict` / `RetryableUtxoConflict`) and a corresponding FFI result code so foreign callers can distinguish retryable from terminal failures without parsing English strings.
- [SUGGESTION] lines 199-218: Post-broadcast !is_relevant is treated as a transient even for self-built transactions
After `broadcast_transaction` succeeds, the only place that records the spend in local state is the `check_core_transaction(.., Mempool, ..)` call at line 203. The `!check_result.is_relevant` branch (lines 205–210) only emits a `tracing::warn!` and still returns `Ok(tx)`. For a transaction this wallet just built using its own `spendable` UTXOs and its own derived change address (lines 78–153), `is_relevant=false` is not a transient — it indicates a wallet-internal invariant break (xpub mismatch, derivation drift, account-map staleness). Letting it pass silently means the next `send_to_addresses` from the same handle can reselect the same inputs and only discover the conflict via a network duplicate-spend rejection later — exactly the user-visible failure mode this PR is trying to improve. Distinguish it from the wallet-missing branch: keep wallet-missing as best-effort logging, but treat `!is_relevant` for an own-built transaction as an internal error (or at minimum surface a structured metric/field) so operators can detect it independently of free-form log output.
- [SUGGESTION] lines 109-152: Change-address derivation index is advanced before paths that may now Err out
`next_change_address(Some(&xpub), true)` at lines 109–111 advances (and persists) the change-address derivation index — the second argument is the mark-used flag. With the new subset check at lines 146–150, control can return `Err(...)` after the index has already been advanced, leaving a derived-but-never-used change address (a gap address). Each retry derives yet another, growing the gap. The same shape exists for the pre-existing build/select/sign Err paths inside the lock, but this PR adds another branch that exercises it. Compute the change address without advancing the index, and commit the advance only after the broadcast (or final pre-broadcast checks) succeeds — or rewind the index on the Err paths. Cosmetic for users, but accumulates wallet-state churn over time.
- [SUGGESTION] lines 154-220: Broadcast-first ordering and failure-rollback contract are not covered by automated tests
The PR's central correctness claim — broadcast failure leaves the spendable UTXO set untouched, broadcast success makes those inputs non-spendable for the next caller via post-broadcast `check_core_transaction` — is exactly the regression flagged on the original #3466. `CoreWallet` is generic over `B: TransactionBroadcaster + ?Sized`, so the seam for deterministic unit tests already exists, and a repository-wide grep confirms no test under `packages/rs-platform-wallet` exercises `send_to_addresses`. Two short async tests would lock this in: (1) inject a broadcaster that returns `Err(...)` and assert the spendable-UTXO set is byte-identical before vs. after the failed call (and `check_core_transaction` is never invoked); (2) inject a broadcaster that returns `Ok(...)` and assert `get_spendable_utxos` afterward no longer contains the spent inputs and includes the change output. The PR's manual checkboxes for both behaviors defer to a running node — the very thing a unit test should cover. Without coverage, only code review prevents a future refactor from re-introducing the original mark-spent-before-broadcast bug.
resolve_dpns_name was fetching the DPNS contract before checking the normalized-label guard, performing a wasted RPC round-trip on empty / .dash inputs. Mirror is_dpns_name_available's order: empty-label guard first, contract fetch second. Thread: PRRT_kwDOGUlHz85_7TFE Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…er invariant (CMT-007, CMT-002) The comment framed the subset check as race-prevention against concurrent spends, but the path is only reachable on builder regression. Rewrite to describe the builder-invariant guarantee accurately and label the runtime check as defense-in-depth. Keep the runtime check intact (per project convention against debug_assert!). Also document the CMT-002 INTENTIONAL stance: keep the typed ConcurrentSpendConflict variant for forward compatibility with future cross-process concurrent-spend surfacing, even though today's code path is only reachable on builder regression. Threads: PRRT_kwDOGUlHz85_6_co (CMT-007), PRRT_kwDOGUlHz85_6_cf (CMT-002) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…vant own-built tx (CMT-004, CMT-005) The wallet-missing branch and the !is_relevant branch were both swallowed into a single tracing::warn! call, indistinguishable from each other in production telemetry. Emit a structured tracing::error event for the own-built !is_relevant path with txid + wallet_id fields so operators can alert on internal invariant violations independent of free-form message text. Also document the CMT-005 INTENTIONAL stance: the wallet-missing branch stays as a single structured log line — converting to Err would lie to callers (broadcast already succeeded), and a metric promotion is gated on monitoring infrastructure that doesn't yet exist. Threads: PRRT_kwDOGUlHz85_7TFY (CMT-004), PRRT_kwDOGUlHz85_7TFh (CMT-005) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-003) Add two #[cfg(test)] tests for the broadcast.rs central correctness claim: - broadcast_failure_keeps_inputs_spendable: mock broadcaster returns Err, assert the error propagates from broadcast_transaction so callers short-circuit before any spendable-set mutation runs. - broadcast_success_marks_inputs_unavailable: mock broadcaster returns Ok(txid), assert broadcast_transaction passes the txid through unchanged so the post-broadcast Mempool registration block in send_to_addresses can run on a confirmed-success signal. Closes the same regression class flagged on the original #3466. Thread: PRRT_kwDOGUlHz85_7TFR Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Slimmed #3585 to two independent fixes — receive-address race deferred to a proper Reserved tri-stateG'day reviewers. After digging through the wallet's address machinery I've pulled the receive-address hand-out fix (Found-026) out of this PR. Here's the why and the what. Architecture findingThe back-to-back DecisionThe proper fix is a real
This keeps the platform side honest (one named function, one mental model) and avoids leaving a reservation set that drifts from the pool's own notion of "used". What this PR now contains#3585 is slimmed to the two independent, sound fixes:
The receive-address fix is deferred to a follow-up PR. Test-preservation contract for the follow-upSo nobody loses sleep about what must stay green once the tri-state lands, the downstream distinct-hand-out coverage to keep is:
One expected shift under the tri-state: the " 🤖 Co-authored by Claudius the Magnificent AI Agent |
`Reservations::<K>::new()` had no callers after the address-reservation removal — `OutpointReservations` constructs its inner sets via `Reservations::default()`. Drop it to clear the dead_code warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claudius-Maginificent
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 1ac8c5d. The latest delta (d6bf1b1..1ac8c5d) is a deliberate scope reduction — commit 2c08626 removes the AddressReservations-based Found-026 fix and defers it to a follow-up tri-state PR; commit 1ac8c5d drops the now-unused generic Reservations::new. No new defects introduced. Two prior suggestions remain valid against current head and are carried forward: (1) leak_until_sync on the post-broadcast reconcile path still has no in-process reclaim hook (the parallel address-handout leak path is gone), and (2) IdentityWallet::send_payment still lacks a direct regression test. Codex's blocking/security framing of the receive-address revert is dropped: per the explicit deferral commit, current head matches v3.1-dev mainline behavior and the PR no longer claims to fix Found-026, so it is out-of-scope pre-existing behavior, not a regression introduced here.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/rs-platform-wallet/src/wallet/core/reservations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/reservations.rs:140-163: Post-broadcast reconcile failure pins outpoint reservations until process restart
`ReservationGuard::leak_until_sync()` skips `Drop` via `mem::forget(self)`, and `broadcast_and_reconcile` (core/broadcast.rs:146) takes that branch whenever the broadcast succeeded but `check_core_transaction` returned `is_relevant == false` or the wallet entry was missing. The transaction is on the network, but the selected outpoints and change address stay reserved in memory for the lifetime of the process — there is no in-process reclaim hook tied to later sync/confirmation. The TODO at reservations.rs:152-155 explicitly acknowledges this gap and references this PR. Holding rather than risking a double-spend is the correct first-order choice, but a transient false-negative from `check_core_transaction` (wallet eviction, sync regression, race) semi-permanently shrinks the spendable set of a long-lived wallet handle. The new `tracing::warn!` (broadcast.rs:139-145) and `tracing::error!` (broadcast.rs:110-116) help observability but don't repair the state. Either land a periodic-sync-driven `Reservations::reclaim_committed(&[K])` before merge, or at minimum surface an aggregated metric on `post_broadcast_reservation_leaked_until_sync` so operators can detect leaked state before it starves new spends. Note: the parallel address-handout instance of this pattern is gone in this push — only the broadcast path remains.
# Conflicts: # packages/rs-platform-wallet-ffi/src/error.rs # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
# Conflicts: # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
Claudius-Maginificent
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at 2899d6c. Carried-forward findings #1 (leak_until_sync pins outpoint reservations until process restart) and #2 (no direct IdentityWallet::send_payment regression test) remain VALID — verified against the current head; both are in-scope for the PR's race-guard work. One new in-scope finding from the latest delta: send_payment is missing the post-build fresh-spendable re-snapshot check that send_to_addresses has, creating an asymmetric defense level between two paths that share the same reservation contract. The codex blocking findings (validate_update_v0 versioning, gRPC Unimplemented ban behavior) target code introduced by the v3.1-dev sync merge (separate already-merged PRs #3868, etc.), not this PR's scope. The codex-ffi withExtendedLifetime findings target pre-existing functions (deriveIdentityAuthKeyAtSlot, prePersistIdentityKeysForRegistration) that were not modified in this PR's delta — confirmed by git diff 5071903ed9..2899d6c28c, those hunks are untouched at lines 902-1100. Review action: COMMENT.
🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/rs-platform-wallet/src/wallet/core/reservations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/reservations.rs:140-163: leak_until_sync pins reservations until process restart with no in-process reclaim path
`ReservationGuard::leak_until_sync` calls `std::mem::forget(self)`, intentionally bypassing `Drop` and leaving entries pinned in the shared `Reservations<K>` set for the manager's lifetime. `broadcast_and_reconcile` (broadcast.rs:130-147) takes this branch in two cases: (a) the broadcast succeeded but `check_core_transaction` returned `is_relevant == false`, and (b) the wallet was missing from the manager during reconciliation. Choosing leak-over-release is the strictly safer half of the trade-off here — releasing risks a concurrent caller re-selecting an already-broadcast outpoint and producing a double-spend the network would reject. However, no in-process reclaim path exists; even a later resync that confirms the broadcast cannot free the reservation. A long-lived daemon that hits repeated `is_relevant == false` events (corrupted/stale address pool, transient wallet-manager misses) monotonically leaks outpoints and burned change-address indices until restart, silently reducing spendable balance. The code carries an explicit `TODO(@thepastaclaw, PR #3585)` on the leak path proposing a `Reservations::reclaim_committed(&[K])` API; surfacing this so the follow-up does not drop off-radar after merge. INTENTIONALLY DEFERRED in the current head — carry-forward as a documented follow-up.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:270-280: send_payment is missing the post-build fresh-spendable re-snapshot that send_to_addresses uses
`CoreWallet::send_to_addresses` (broadcast.rs:332-363) performs two defense-in-depth checks after `build_signed`: (1) `selected ⊆ pre-build spendable_outpoints`, and (2) `selected ⊆` a fresh re-fetch of `managed_account.spendable_utxos(current_height)`, surfacing `ConcurrentSpendConflict` with the missing outpoints. The comment on (2) explicitly calls it out as the forward-compat net for a future mutator running outside the wallet write lock (mempool listener, chain reorg, etc.) that would slip through the pre-build snapshot. `IdentityWallet::send_payment` (payments.rs:270-280) implements only check (1) and funds from the same shared BIP-44 account 0. Today the two paths have the same lock-scope guarantees so check (2) is unreachable in both, but the asymmetry quietly drifts the moment a future change moves a UTXO mutator outside the wallet write lock — the comment on the absent check is what guides that future reader, and only `send_to_addresses` has it. Either lift check (2) into a shared helper alongside `classify_build_error` / `broadcast_and_reconcile`, or add an explicit comment on the `send_payment` site documenting why the second check is unnecessary here.
…3770 TODO, scrub stale CMT IDs Adds an end-to-end regression test for the DashPay `send_payment` path (it previously had none): drives a stubbed-broadcaster send and asserts (a) a Sent PaymentEntry is recorded on the sending ManagedIdentity, (b) the selected outpoint is reserved during build and released after a successful reconcile, and (c) a concurrent send holding the same UTXO makes send_payment short-circuit with NoSpendableInputs without reaching the broadcaster. Also documents the planned change-address-reservation switch in #3770 with a TODO at the peek-and-advance loop, and scrubs dead ephemeral CMT-NNN review IDs from the files this PR already touches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… fresh-spendable re-snapshot send_payment runs under the same wallet write-lock scope and funds from the same shared BIP-44 account 0 as CoreWallet::send_to_addresses, so the second fresh-spendable re-snapshot check that send_to_addresses performs would be unreachable here today. The comment records that invariant and points future readers to add the same check if a UTXO mutator is ever moved outside the write lock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t into shared helper; use in send_payment Extracts the post-build defense-in-depth check (re-fetch the funding account's spendable UTXOs and confirm every selected outpoint is still present, else ConcurrentSpendConflict listing the missing ones) out of CoreWallet::send_to_addresses into a pure shared helper `assert_selected_still_spendable`, alongside classify_build_error / broadcast_and_reconcile. send_to_addresses now calls it (no behavior change), and send_payment calls it after its own subset check for parity — so a future out-of-lock UTXO mutator is caught on both send paths. This supersedes the comment-only commit d13949c: the explanatory comment in send_payment is replaced by the actual check plus a pointer to the helper's canonical doc. Tests: unit tests on the helper (subset -> Ok; missing -> conflict listing the vanished outpoint). A send_payment-path trip test is not added — build_signed and the re-snapshot run under one held write lock, so no in-process task can drop a selected UTXO between them today; a TODO in the payments test module records the gap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Re-validation at cf9db28 against 2899d6c: prior #2 (test coverage for IdentityWallet::send_payment) is FIXED by the new #[cfg(test)] mod in payments.rs. Prior #1 (leak_until_sync has no in-process reclaim path) and #3 (send_payment lacks the post-build fresh-spendable re-snapshot that send_to_addresses uses) remain STILL VALID and are carried forward as suggestions. No new latest-delta findings — the delta is comment cleanup (CMT-* tag removal), a TODO(#3770) on the change-address peek loop, and the new direct send_payment regression tests.
🟡 2 carried-forward suggestion(s)
No duplicate inline comments were added here: review_poster.py matched both still-valid findings to existing review threads/comments. This top-level review records exact-SHA coverage for cf9db286 and preserves the cumulative reconciliation.
Prior finding fixed
- send_payment lacks direct regression test coverage: Fixed in cf9db28 — the new #[cfg(test)] mod tests in packages/rs-platform-wallet/src/wallet/identity/network/payments.rs adds direct happy-path and race-loss coverage for IdentityWallet::send_payment, addressing the prior test-coverage gap.
🤖 Prompt for carried-forward findings
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:270-284: send_payment is missing the post-build fresh-spendable re-snapshot that send_to_addresses uses
At cf9db286, CoreWallet::send_to_addresses (broadcast.rs:334-366) performs two defense-in-depth checks after build_signed: (a) `selected ⊆ pre-build spendable` snapshot, and (b) `selected ⊆ fresh managed_account.spendable_utxos(current_height)` re-fetch, surfacing `ConcurrentSpendConflict { selected: missing }`. The inline comment at broadcast.rs:348-354 explicitly justifies (b) as the cross-process / cross-subsystem net for any future UTXO mutator (mempool listener, chain reorg, watch-only update) that does not go through the wallet write lock. IdentityWallet::send_payment (payments.rs:270-280) implements only (a) before reserving and broadcasting. Both paths fund from the same BIP-44 account 0 UTXOs and now share the same `OutpointReservations` instance, so they share the same cross-subsystem exposure. Today both run under the same wallet write lock so there is no observed race, but the symmetric defense matters: a future regression that introduces an out-of-lock UTXO mutator would silently bypass the DashPay send path while still fail-fast on the core send. Either mirror the fresh re-snapshot here, or add a comment at payments.rs:270 explaining why send_payment is exempt so the asymmetry is intentional and discoverable.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/reservations.rs:140-163: leak_until_sync pins reservations until process restart with no in-process reclaim path
ReservationGuard::leak_until_sync (reservations.rs:156-163) intentionally calls `std::mem::forget(self)`, bypassing Drop and leaving entries pinned in the shared Reservations<K> set for the lifetime of the wallet manager. broadcast_and_reconcile takes this branch whenever broadcast succeeded but post-broadcast `check_core_transaction` returns `is_relevant == false`, or the wallet handle was evicted between broadcast and reconcile. The fail-closed behavior prevents an immediate double-spend retry, but it also means a stale or repeatedly-misclassified reconcile path could progressively shrink the in-process spendable set until restart — a soft DoS against the wallet's own sends. The current head explicitly documents the limitation (reservations.rs:147-150) and leaves a TODO at reservations.rs:152-155 tied to this PR thread for a future `Reservations::reclaim_committed(&[K])` API, so this is INTENTIONALLY DEFERRED for #3585. Carrying forward as a suggestion so the periodic-sync-driven reclaim follow-up is captured on a tracking issue (e.g. #3770) rather than living only as an inline TODO.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta cleanly resolves prior finding #1 by extracting assert_selected_still_spendable and wiring it into both send paths; helper has direct unit coverage and the prior asymmetry is gone. Two carry-forward suggestions remain: (a) leak_until_sync still has no in-process reclaim path (intentionally deferred per in-tree TODO, but worth tracking on a separate issue), and (b) the new Reservations::reserve primitive does not make same-key ownership exclusive — safe today only because send paths serialize through the wallet write lock. No blocking issues.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/rs-platform-wallet/src/wallet/core/reservations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/reservations.rs:94-109: Reservations::reserve does not make same-key ownership exclusive
The new reservation primitive (`Reservations::reserve`, reservations.rs:94-109) blindly inserts every requested key into the shared `HashSet<K>` and returns a guard for the full input vector, even if one or more keys were already reserved by another live guard. If two guards ever cover the same key, dropping either guard's `do_release` will remove the key from the set while the other operation still believes it owns the reservation — the set then temporarily reports the key as free, defeating the RAII race-guard contract. Today the call sites (`CoreWallet::send_to_addresses` and `IdentityWallet::send_payment`) prefilter their candidate sets against `snapshot()` under the wallet-manager write lock, so the overlap case is unreachable in production — but `Reservations`/`ReservationGuard` are `pub(crate)` and described as the core race-guard abstraction. Either make `reserve` atomically reject overlaps (return an error or `Option<Guard>`), or track per-key reference counts so `do_release` only removes a key when the last guard drops, so the ownership invariant lives inside the type instead of every caller maintaining it externally.
…erlapping keys Enforce the race-guard exclusivity invariant in the type itself: reserve now inserts all-or-nothing and returns None when any requested key is already held by a live guard, so two guards can never cover the same key. The composed OutpointReservations::reserve is atomic across both sub-sets and rolls back the outpoint reservation if the change-address reserve fails. Call sites map rejection to the retryable ConcurrentSpendConflict (unreachable today since both prefilter under the write lock). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at bee3f49. Latest delta is Swift UI (multi-recipient Core send) + a TODO refresh in reservations.rs pointing at follow-up #3770 — no new in-scope defects introduced. Two prior reservations.rs findings remain STILL VALID and are carried forward as suggestions: leak_until_sync still has no in-process reclaim path (intentionally deferred to #3770), and Reservations::reserve still does not enforce same-key ownership exclusivity at the type level (safe today because both production call sites filter against snapshot() under the wallet write lock). One Codex FFI observation about core_wallet_send_to_addresses using assume_checked() instead of require_network(wallet_network) is real, but the function pre-exists this PR and the SwiftExampleApp caller pre-validates — recorded out-of-scope.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/rs-platform-wallet/src/wallet/core/reservations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/reservations.rs:140-163: leak_until_sync still has no in-process reclaim path (intentionally deferred to #3770)
`ReservationGuard::leak_until_sync` calls `std::mem::forget(self)`, bypassing `Drop` and leaving the reserved keys pinned in the shared `Reservations<K>` set for the lifetime of the `PlatformWalletManager`. The latest delta only rewrites the TODO comment to point at follow-up #3770; the runtime behaviour is unchanged. The branch is taken on the `broadcast_and_reconcile` paths where broadcast succeeded but `check_core_transaction` returned `is_relevant == false`, or when the wallet handle was evicted before reconcile — meaning each occurrence permanently shrinks the wallet's spendable set until process restart. Security/availability impact is bounded (the leak conservatively prevents re-selection of an already-broadcast outpoint, which is the correct safety trade-off), but a peer/RPC source able to influence post-broadcast reconcile outcomes can repeatedly trigger leaks and slow-burn wallet spendability. This is acknowledged in-tree and explicitly tracked as follow-up #3770, so not a blocker for this PR — confirm #3770 is open and assigned before merging so this does not regress into a silent leak across releases, and make sure any operational runbook for `ConcurrentSpendConflict` knows that the only in-process recovery today is a restart.
The DPNS case-insensitive .dash resolution change now lives in its own PR (#3914); this branch is scoped to the OutpointReservations UTXO-spend race guard and its FFI/Swift surface only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Claudius-Maginificent
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward leak_until_sync follow-up remains present and is explicitly deferred to #3770 — accepted as a tracked follow-up, not a blocker. One new in-scope quality gap: the race-loser path in CoreWallet::send_to_addresses (broadcast.rs:301-307) and IdentityWallet::send_payment (payments.rs:184-190) returns NoSpendableInputs even when the wallet has spendable UTXOs that were filtered out solely by in-flight reservations, despite the PR description promising the new ConcurrentSpendConflict variant for exactly that case — the typed FFI code 31 / Swift .concurrentSpendConflict are effectively never produced in the normal race scenario. The asset-lock funding race called out by codex is real but is a pre-existing path the PR did not claim to cover; recorded as out-of-scope follow-up.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:293-307: Race-loser path returns NoSpendableInputs instead of the new ConcurrentSpendConflict
New finding in the cumulative PR (not carried-forward). The PR description states `ConcurrentSpendConflict` is `returned when every candidate outpoint is already reserved, so callers can distinguish a retryable contention from a fatal error`, and FFI result code 31 plus Swift `concurrentSpendConflict` exist solely to surface that distinction. However, this branch builds `spendable` as `spendable_utxos(...).filter(|utxo| !reserved.contains(&utxo.outpoint))` and then returns `NoSpendableInputs` whenever `spendable.is_empty()`, collapsing two semantically distinct cases:
1. The wallet truly has no spendable UTXOs (fatal — no retry will help).
2. The wallet has spendable UTXOs but every one is held by an in-flight reservation (retryable — the very case the new variant was added for).
Both surface as `NoSpendableInputs` with the same context string, and over FFI both map to `ErrorNoSelectableInputs`, not `ErrorConcurrentSpendConflict`. The only place `ConcurrentSpendConflict` actually fires (broadcast.rs:368) is the post-build subset-check defense-in-depth, which the comment itself notes is `unreachable under normal builder contract` — i.e. the new typed signal never fires in the normal race scenario it was added to communicate.
The same pattern exists in `wallet/identity/network/payments.rs:184-190`. Recommended fix: keep an unfiltered `all_spendable` candidate list and, when `spendable.is_empty()`, branch on whether `all_spendable` was empty — empty → `NoSpendableInputs`, non-empty → `ConcurrentSpendConflict { selected: reserved-intersect-candidates }`. This restores the documented retryable/fatal distinction at both the Rust and FFI/Swift layers.
The underlying race-protection layer is correct as-is; this is a typed-error-signal gap rather than a safety bug, so flagged as a suggestion.
| let reserved = self.reservations.snapshot(); | ||
| let spendable: Vec<_> = managed_account | ||
| .spendable_utxos(current_height) | ||
| .into_iter() | ||
| .filter(|utxo| !reserved.contains(&utxo.outpoint)) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| if spendable.is_empty() { | ||
| return Err(PlatformWalletError::NoSpendableInputs { | ||
| account_index, | ||
| account_type, | ||
| context: "all UTXOs used or reserved by in-flight transactions".to_string(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Race-loser path returns NoSpendableInputs instead of the new ConcurrentSpendConflict
New finding in the cumulative PR (not carried-forward). The PR description states ConcurrentSpendConflict is returned when every candidate outpoint is already reserved, so callers can distinguish a retryable contention from a fatal error, and FFI result code 31 plus Swift concurrentSpendConflict exist solely to surface that distinction. However, this branch builds spendable as spendable_utxos(...).filter(|utxo| !reserved.contains(&utxo.outpoint)) and then returns NoSpendableInputs whenever spendable.is_empty(), collapsing two semantically distinct cases:
- The wallet truly has no spendable UTXOs (fatal — no retry will help).
- The wallet has spendable UTXOs but every one is held by an in-flight reservation (retryable — the very case the new variant was added for).
Both surface as NoSpendableInputs with the same context string, and over FFI both map to ErrorNoSelectableInputs, not ErrorConcurrentSpendConflict. The only place ConcurrentSpendConflict actually fires (broadcast.rs:368) is the post-build subset-check defense-in-depth, which the comment itself notes is unreachable under normal builder contract — i.e. the new typed signal never fires in the normal race scenario it was added to communicate.
The same pattern exists in wallet/identity/network/payments.rs:184-190. Recommended fix: keep an unfiltered all_spendable candidate list and, when spendable.is_empty(), branch on whether all_spendable was empty — empty → NoSpendableInputs, non-empty → ConcurrentSpendConflict { selected: reserved-intersect-candidates }. This restores the documented retryable/fatal distinction at both the Rust and FFI/Swift layers.
The underlying race-protection layer is correct as-is; this is a typed-error-signal gap rather than a safety bug, so flagged as a suggestion.
source: ['codex']
|
We have decided to implement this on rust-dashcore. |
github-project-automation
Bot
moved this from In review / testing
to Done
in Platform team
5 participants
Issue being fixed or feature implemented
X→ the second broadcast is rejected as a double-spend → the user sees a generic, non-actionable failure.What was done?
A reservation layer that makes concurrent input selection mutually exclusive:
OutpointReservationsRAII guard (wallet/core/reservations.rs): a process-global, mutex-guarded table that reserves the outpoints a transaction selects for the duration of its build-and-broadcast cycle and releases them on drop. Concurrent selectors skip already-reserved outpoints.CoreWallet::send_to_addresses(wallet/core/broadcast.rs) andIdentityWallet::send_payment(wallet/identity/network/payments.rs) acquire reservations around input selection + broadcast.IdentityWalletshares theCoreWallet's reservation table (wallet/platform_wallet.rs,identity/network/identity_handle.rs,wallet/core/wallet.rs,wallet/core/mod.rs).PlatformWalletError::ConcurrentSpendConflict(src/error.rs) returned when every candidate outpoint is already reserved, so callers can distinguish a retryable contention from a fatal error.ErrorConcurrentSpendConflict = 31(rs-platform-wallet-ffi/src/error.rs), collapse of input-selection errors with a retryable/fatal distinction, and the Swift mirrorconcurrentSpendConflict(swift-sdk/.../PlatformWalletResult.swift).How Has This Been Tested?
cargo build -p dash-sdk -p platform-wallet -p platform-wallet-ffi— clean.reservations.rs(10 tests), reservation-aware broadcast inbroadcast.rs(17 tests), andsend_paymentpaths inpayments.rs; FFI error-mapping tests assert result code31round-trips.Breaking Changes
Additive only. A new
PlatformWalletError::ConcurrentSpendConflictvariant and a new FFI result code31are introduced; existing call signatures are unchanged. Downstream code that matchesPlatformWalletErrorexhaustively must handle the new variant.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent