fix: case-insensitive .dash resolution + UTXO double-spend race hardening#3585
fix: case-insensitive .dash resolution + UTXO double-spend race hardening#3585Claudius-Maginificent wants to merge 43 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 1ac8c5d) |
![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: "a2599efbc055a8b4389d94395447fd6f8a3f70c0902b3753453d4ec63a2d3a92"
)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>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Current head resolves three earlier issues in this area: DashPay now shares the core reservation set, pending change addresses are tracked to avoid reuse, and post-broadcast reconciliation no longer drops reservations on the success-but-unreconciled path. One carried-forward prior finding remains valid in send_to_addresses, and this delta adds two new suggestion-level issues: leak_until_sync makes leaked reservations permanent for the life of the process, and the duplicated DashPay send path still has no dedicated regression coverage.
🟡 3 suggestion(s)
3 additional finding(s)
suggestion: `send_to_addresses` still classifies retryable coin-selection failures by `Display` text
packages/rs-platform-wallet/src/wallet/core/broadcast.rs (line 190)
TransactionBuilder::build_signed returns a typed BuilderError, and the pinned key-wallet dependency exposes BuilderError::CoinSelection(SelectionError::InsufficientFunds|NoUtxosAvailable). This code discards that structure and decides whether to return PlatformWalletError::NoSpendableInputs by substring-matching e.to_string(). That makes the wallet's public retryable/non-retryable contract depend on upstream wording instead of the actual error variant. A wording change in key-wallet silently turns a retryable selection failure into TransactionBuild, and the nearby test explicitly notes that this mapping path is still unpinned.
suggestion: `leak_until_sync` permanently strands reservations with no in-process recovery path
packages/rs-platform-wallet/src/wallet/core/reservations.rs (line 148)
OutpointReservationGuard::leak_until_sync calls std::mem::forget(self), so the only owner capable of removing the reserved outpoints and pending change address never runs Drop. There is no API in OutpointReservations or any sync path in this crate that rebuilds or clears ReservationsInner afterward, and the test suite now pins the behavior as "held until process restart." If check_core_transaction returns a false negative or the wallet handle is missing after a successful broadcast, those outpoints and change addresses remain unavailable for the lifetime of the process, causing false NoSpendableInputs results and permanently skipped change indices until restart.
suggestion: The duplicated DashPay send path has no regression tests for its new reservation and reconciliation logic
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (line 93)
IdentityWallet::send_payment now duplicates the same lock/build/reserve/broadcast/reconcile flow as CoreWallet::send_to_addresses, including shared outpoint reservations, pending-change tracking, and the sticky reservation path on unreconciled broadcasts. There is no test coverage for send_payment itself: a repo-wide search finds only the implementation and docs, not a test that exercises concurrent send_payment vs. send_to_addresses, rollback on broadcast failure, or the leak_until_sync branch. Because this logic is duplicated instead of factored into a shared helper, the existing core-wallet tests do not protect the DashPay path from regressions.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:190-205: `send_to_addresses` still classifies retryable coin-selection failures by `Display` text
`TransactionBuilder::build_signed` returns a typed `BuilderError`, and the pinned `key-wallet` dependency exposes `BuilderError::CoinSelection(SelectionError::InsufficientFunds|NoUtxosAvailable)`. This code discards that structure and decides whether to return `PlatformWalletError::NoSpendableInputs` by substring-matching `e.to_string()`. That makes the wallet's public retryable/non-retryable contract depend on upstream wording instead of the actual error variant. A wording change in `key-wallet` silently turns a retryable selection failure into `TransactionBuild`, and the nearby test explicitly notes that this mapping path is still unpinned.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/core/reservations.rs`:148-161: `leak_until_sync` permanently strands reservations with no in-process recovery path
`OutpointReservationGuard::leak_until_sync` calls `std::mem::forget(self)`, so the only owner capable of removing the reserved outpoints and pending change address never runs `Drop`. There is no API in `OutpointReservations` or any sync path in this crate that rebuilds or clears `ReservationsInner` afterward, and the test suite now pins the behavior as "held until process restart." If `check_core_transaction` returns a false negative or the wallet handle is missing after a successful broadcast, those outpoints and change addresses remain unavailable for the lifetime of the process, causing false `NoSpendableInputs` results and permanently skipped change indices until restart.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:93-357: The duplicated DashPay send path has no regression tests for its new reservation and reconciliation logic
`IdentityWallet::send_payment` now duplicates the same lock/build/reserve/broadcast/reconcile flow as `CoreWallet::send_to_addresses`, including shared outpoint reservations, pending-change tracking, and the sticky reservation path on unreconciled broadcasts. There is no test coverage for `send_payment` itself: a repo-wide search finds only the implementation and docs, not a test that exercises concurrent `send_payment` vs. `send_to_addresses`, rollback on broadcast failure, or the `leak_until_sync` branch. Because this logic is duplicated instead of factored into a shared helper, the existing core-wallet tests do not protect the DashPay path from regressions.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the wallet-related findings against b8043f9aabc26594f1bd796dc78dcaa4460a3b46. The three prior findings are still valid on this head, and there is one additional latest-delta regression in the duplicated DashPay send path: it does not preserve the new retryable NoSpendableInputs contract when transaction building fails.
🟡 4 suggestion(s)
4 additional finding(s)
suggestion: `send_to_addresses` still classifies coin-selection failures by matching `Display` text
packages/rs-platform-wallet/src/wallet/core/broadcast.rs (line 190)
send_to_addresses still converts build_signed failures into NoSpendableInputs by searching e.to_string() for "Insufficient funds" and "No UTXOs available". That is a brittle protocol boundary: the pinned key-wallet dependency exposes typed BuilderError::CoinSelection(SelectionError) and SelectionError::{NoUtxosAvailable, InsufficientFunds}, so this code is discarding stable enum variants and depending on human-readable wording instead. A harmless upstream wording change would silently reclassify the same retryable condition into TransactionBuild, breaking the API contract this PR introduced.
suggestion: `leak_until_sync` makes unreconciled reservations permanent for the lifetime of the process
packages/rs-platform-wallet/src/wallet/core/reservations.rs (line 148)
OutpointReservationGuard::leak_until_sync only calls std::mem::forget(self), so the only code that can remove the reserved outpoints and pending change address never runs. I could not find any compensating API on OutpointReservations, and the crate’s own test now pins this as "held until process restart", so the documented "full sync will reconcile" path does not exist in-process. If post-broadcast reconciliation returns a false negative or the wallet handle goes stale after broadcast, those UTXOs and change addresses remain locally blocked until restart, causing persistent NoSpendableInputs results and skipped change indices.
suggestion: The duplicated DashPay send path still has no focused regression coverage for reservation and reconciliation behavior
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (line 93)
IdentityWallet::send_payment duplicates the same reserve/build/broadcast/reconcile flow as CoreWallet::send_to_addresses, including shared outpoint reservations, pending change tracking, and the leak_until_sync branch. A repo-wide search at this SHA found no tests that exercise send_payment itself for the concurrency loser case, broadcast-failure rollback, or unreconciled-success behavior. Because this logic is duplicated instead of factored behind a shared helper, the existing broadcast.rs tests do not protect the DashPay path from diverging.
suggestion: `send_payment` collapses retryable coin-selection failures into generic `TransactionBuild` errors
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (line 257)
Unlike CoreWallet::send_to_addresses, the DashPay path maps every build_signed failure directly to PlatformWalletError::TransactionBuild(e.to_string()). That regresses the new wallet error contract for retryable depletion cases: a normal BuilderError::CoinSelection(SelectionError::InsufficientFunds | NoUtxosAvailable) now surfaces as a generic builder error instead of NoSpendableInputs. Callers of the two send paths therefore receive inconsistent error semantics for the same underlying failure and are forced back to string inspection on the DashPay path.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:190-205: `send_to_addresses` still classifies coin-selection failures by matching `Display` text
`send_to_addresses` still converts `build_signed` failures into `NoSpendableInputs` by searching `e.to_string()` for `"Insufficient funds"` and `"No UTXOs available"`. That is a brittle protocol boundary: the pinned `key-wallet` dependency exposes typed `BuilderError::CoinSelection(SelectionError)` and `SelectionError::{NoUtxosAvailable, InsufficientFunds}`, so this code is discarding stable enum variants and depending on human-readable wording instead. A harmless upstream wording change would silently reclassify the same retryable condition into `TransactionBuild`, breaking the API contract this PR introduced.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/core/reservations.rs`:148-161: `leak_until_sync` makes unreconciled reservations permanent for the lifetime of the process
`OutpointReservationGuard::leak_until_sync` only calls `std::mem::forget(self)`, so the only code that can remove the reserved outpoints and pending change address never runs. I could not find any compensating API on `OutpointReservations`, and the crate’s own test now pins this as `"held until process restart"`, so the documented `"full sync will reconcile"` path does not exist in-process. If post-broadcast reconciliation returns a false negative or the wallet handle goes stale after broadcast, those UTXOs and change addresses remain locally blocked until restart, causing persistent `NoSpendableInputs` results and skipped change indices.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:93-360: The duplicated DashPay send path still has no focused regression coverage for reservation and reconciliation behavior
`IdentityWallet::send_payment` duplicates the same reserve/build/broadcast/reconcile flow as `CoreWallet::send_to_addresses`, including shared outpoint reservations, pending change tracking, and the `leak_until_sync` branch. A repo-wide search at this SHA found no tests that exercise `send_payment` itself for the concurrency loser case, broadcast-failure rollback, or unreconciled-success behavior. Because this logic is duplicated instead of factored behind a shared helper, the existing `broadcast.rs` tests do not protect the DashPay path from diverging.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:257-262: `send_payment` collapses retryable coin-selection failures into generic `TransactionBuild` errors
Unlike `CoreWallet::send_to_addresses`, the DashPay path maps every `build_signed` failure directly to `PlatformWalletError::TransactionBuild(e.to_string())`. That regresses the new wallet error contract for retryable depletion cases: a normal `BuilderError::CoinSelection(SelectionError::InsufficientFunds | NoUtxosAvailable)` now surfaces as a generic builder error instead of `NoSpendableInputs`. Callers of the two send paths therefore receive inconsistent error semantics for the same underlying failure and are forced back to string inspection on the DashPay path.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
…per (CMT-001, CMT-004)
Replace brittle `e.to_string()` substring matches in `send_to_addresses`
and `send_payment` with a typed `classify_build_error` helper that
pattern-matches `BuilderError::InsufficientFunds` and
`BuilderError::CoinSelection(SelectionError::{NoUtxosAvailable,
InsufficientFunds})` into the retryable
`PlatformWalletError::NoSpendableInputs`. Every other build failure
falls through to fatal `PlatformWalletError::TransactionBuild` with the
upstream Display text preserved in `context`.
Unit-pinned with six assertions over the typed variants so a future
upstream rewording of the `Display` impl cannot silently downgrade
`NoSpendableInputs` back to `TransactionBuild`.
Drops the stale `TODO(CMT-005, #3585)` comment.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pinned behavior (CMT-002) The previous docstring claimed "a wallet restart or full sync will reconcile" the leaked reservation, but no in-process reclaim API exists today — `leak_until_sync` calls `mem::forget(self)` and the outpoints stay pinned in `OutpointReservations` until the process exits. The unit test `leak_until_sync_keeps_reservation_held` pins exactly that behavior. Rewrite the doc to plainly state "held until process restart", keep the safer-of-two-bad-outcomes rationale, and add a follow-up TODO referencing @thepastaclaw's PR #3585 review for a future `OutpointReservations::reclaim_confirmed` API hooked into periodic sync. No behaviour change — docs only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Wallet/IdentityWallet (CMT-003) Extract `broadcast_and_reconcile` — the broadcast → check_core_transaction → release-or-leak decision tree shared by `CoreWallet::send_to_addresses` and `IdentityWallet::send_payment`. The path-specific bits stay inlined at each call site (UTXO snapshot/filter, change-address peek-burn-commit loop, builder construction, post-broadcast reservation reconcile target account layout) — the helper covers only the genuinely shared invariant code where divergence would cause double-spend or leaked-reservation bugs. A `FnOnce(&mut PlatformWalletInfo, &Txid, bool /*reconciled*/)` hook runs inside the post-broadcast write lock so call-site-specific bookkeeping (e.g. `record_dashpay_payment`) sees the same locked critical section as the reconcile. Pin the helper's three invariants directly with new unit tests: * `broadcast_and_reconcile_drops_reservation_on_broadcast_failure` — the hook is NOT invoked and the reservation is released so the caller can retry. * `broadcast_and_reconcile_leaks_when_wallet_missing` — stale wallet handle leaks the reservation (no concurrent re-selection of an already-broadcast outpoint). * `send_to_addresses_success_invokes_hook_and_releases_reservation` — end-to-end happy path through the shared helper. The three pre-existing CMT-003 concurrency/rollback tests in `broadcast.rs` (`concurrent_same_utxo_sends_resolve_via_reservation_set`, `broadcast_failure_releases_reservation_for_retry`, `unreconciled_broadcast_keeps_reservation_held`) continue to exercise the same shared helper — no separate `send_payment` ports are needed because the helper IS the shared seam those tests target. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@thepastaclaw — addressing your 2026-05-25 review (the one posted as review body due to inline HTTP 422). Three commits on top of `b8043f9a`: CMT-001 + CMT-004 — typed coin-selection mapper (`8de859a6`) CMT-002 — `leak_until_sync` docstring corrected (`4d017b8a`) CMT-003 — shared broadcast/reconcile helper (`cd2ade5e`) Validations: `cargo fmt` clean, `clippy --tests -- -D warnings` clean, `cargo test --lib` 146 passed. 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two prior concerns remain on cd2ade5: unreconciled successful broadcasts still pin reservations until process restart, and send_payment still has no direct regression coverage for its DashPay-specific path after the helper refactor. The latest delta does fix the earlier typed-error-mapping and shared post-broadcast-tail issues, and I did not confirm the new claim that send_payment has a current reachable concurrent-spend bug from omitting the extra post-build recheck.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: `leak_until_sync` still leaves reservations stuck for the lifetime of the process
packages/rs-platform-wallet/src/wallet/core/reservations.rs (line 148)
This branch still calls mem::forget(self) with no matching in-process reclaim path. broadcast_and_reconcile reaches it after a successful broadcast whenever post-broadcast reconciliation fails or the wallet handle is missing, then returns Ok(txid) to the caller. At that point the reserved outpoints and pending change address remain pinned in OutpointReservations until the PlatformWalletManager is dropped, so later sends on the same wallet can keep failing with NoSpendableInputs or skip change indices even though the send already succeeded. The updated docstring and test now explicitly confirm that behavior instead of fixing it.
suggestion: `send_payment` still has no focused regression coverage for its own build, reconcile, and payment-recording path
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (line 93)
There are still no tests that call IdentityWallet::send_payment on this head. The new coverage in wallet/core/broadcast.rs exercises broadcast_and_reconcile directly and through CoreWallet::send_to_addresses, but it does not cover the DashPay-specific work that remains inside send_payment: external-account lookup, recipient address derivation, change-address handling, and the on_reconcile hook that records the outgoing PaymentEntry. A regression in that path would still pass the current suite.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/core/reservations.rs`:148-167: `leak_until_sync` still leaves reservations stuck for the lifetime of the process
This branch still calls `mem::forget(self)` with no matching in-process reclaim path. `broadcast_and_reconcile` reaches it after a successful broadcast whenever post-broadcast reconciliation fails or the wallet handle is missing, then returns `Ok(txid)` to the caller. At that point the reserved outpoints and pending change address remain pinned in `OutpointReservations` until the `PlatformWalletManager` is dropped, so later sends on the same wallet can keep failing with `NoSpendableInputs` or skip change indices even though the send already succeeded. The updated docstring and test now explicitly confirm that behavior instead of fixing it.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:93-327: `send_payment` still has no focused regression coverage for its own build, reconcile, and payment-recording path
There are still no tests that call `IdentityWallet::send_payment` on this head. The new coverage in `wallet/core/broadcast.rs` exercises `broadcast_and_reconcile` directly and through `CoreWallet::send_to_addresses`, but it does not cover the DashPay-specific work that remains inside `send_payment`: external-account lookup, recipient address derivation, change-address handling, and the `on_reconcile` hook that records the outgoing `PaymentEntry`. A regression in that path would still pass the current suite.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
|
One remaining non-blocking concern I still see in the exact head: When post-broadcast reconciliation fails, the code intentionally calls Why it matters: any false-negative Suggested follow-up: add a sync-driven reclaim path keyed by observed outpoints/txid, and cover the reconcile-failure branch with a regression test proving the wallet can recover without restart. The main targeted fixes look good to me: case-insensitive |
…-utxo-race-v3.1-dev # Conflicts: # packages/rs-platform-wallet-ffi/src/error.rs # packages/rs-platform-wallet/src/error.rs # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
…g_mut` in tests Post-merge interface drift: clippy 1.92's `manual_dangling_ptr` flags the existing test-helper pattern. Swap to the idiom clippy now expects — same non-null sentinel pointer, no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…electableInputs (14) Follow upstream #3651's umbrella intent: all three "can't-pick-UTXOs" wallet variants — `NoSpendableInputs` (incl. race-loser path), `OnlyOutputAddressesFunded`, `OnlyDustInputs` — now route to `ErrorNoSelectableInputs (14)`. The dedicated `ErrorNoSpendableInputs (30)` slot becomes dead and is removed from both the FFI enum and the Swift mirror (case, switch arm, `errorDescription` arm). The typed `PlatformWalletError::NoSpendableInputs` Display rendering (account_type / account_index / context + race-loser breadcrumb) still survives verbatim in the result message so callers can distinguish the underlying cause. `ErrorConcurrentSpendConflict (31)` is unchanged. Pinned FFI test updated to assert all three variants collapse onto code 14; added a separate pin for `ConcurrentSpendConflict (31)` keeping its dedicated code so future churn doesn't accidentally fold it into the umbrella too.
Claudius-Maginificent
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest incremental push at 87d24e3 is limited to the FFI/Swift error-code remap and I did not find a new in-scope defect in that delta. Two prior wallet-core concerns from earlier commits in PR #3585 remain valid on this head and should still be carried forward: the post-broadcast leak path permanently pins reservations in-process, and the rewritten DashPay send_payment path still lacks focused regression coverage.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: Successful broadcasts can still leave UTXOs and change addresses reserved for the life of the process
packages/rs-platform-wallet/src/wallet/core/reservations.rs (line 148)
broadcast_and_reconcile() still sends every successful-but-unreconciled broadcast down the reservation.leak_until_sync() branch at packages/rs-platform-wallet/src/wallet/core/broadcast.rs:130-146, and leak_until_sync() still implements that by mem::forget(self) with an explicit note that there is no in-process reclaim path. That means this PR's shared send-flow can leave a transaction that was already accepted by the network permanently pinning its selected outpoints and pending change address inside OutpointReservations until the wallet process restarts. The branch avoids an immediate double-spend attempt, but the resulting stuck reservations can still make later sends fail with NoSpendableInputs or burn change indices indefinitely on a long-lived wallet handle.
suggestion: The rewritten `IdentityWallet::send_payment` path still has no direct regression test
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (line 93)
IdentityWallet::send_payment now contains substantial PR-specific logic of its own: contact external-account lookup, recipient address derivation, funding-account change-address advancement, reservation/filtering against concurrent sends, the call into broadcast_and_reconcile, and the on_reconcile hook that records the outgoing PaymentEntry. A repository-wide search on this head finds no test that calls send_payment directly; current coverage only exercises the shared helper and CoreWallet::send_to_addresses in wallet/core/broadcast.rs. Because of that gap, a regression in the DashPay-specific build/reconcile/payment-recording path introduced by this PR can still pass the suite.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/core/reservations.rs`:148-172: Successful broadcasts can still leave UTXOs and change addresses reserved for the life of the process
`broadcast_and_reconcile()` still sends every successful-but-unreconciled broadcast down the `reservation.leak_until_sync()` branch at `packages/rs-platform-wallet/src/wallet/core/broadcast.rs:130-146`, and `leak_until_sync()` still implements that by `mem::forget(self)` with an explicit note that there is no in-process reclaim path. That means this PR's shared send-flow can leave a transaction that was already accepted by the network permanently pinning its selected outpoints and pending change address inside `OutpointReservations` until the wallet process restarts. The branch avoids an immediate double-spend attempt, but the resulting stuck reservations can still make later sends fail with `NoSpendableInputs` or burn change indices indefinitely on a long-lived wallet handle.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:93-327: The rewritten `IdentityWallet::send_payment` path still has no direct regression test
`IdentityWallet::send_payment` now contains substantial PR-specific logic of its own: contact external-account lookup, recipient address derivation, funding-account change-address advancement, reservation/filtering against concurrent sends, the call into `broadcast_and_reconcile`, and the `on_reconcile` hook that records the outgoing `PaymentEntry`. A repository-wide search on this head finds no test that calls `send_payment` directly; current coverage only exercises the shared helper and `CoreWallet::send_to_addresses` in `wallet/core/broadcast.rs`. Because of that gap, a regression in the DashPay-specific build/reconcile/payment-recording path introduced by this PR can still pass the suite.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
… (Found-026) [backport] (#3658) Co-authored-by: Claude Opus 4.7 (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
The latest incremental patch at b9a3be72c7a0b5cc7c14e2338ca26d0335c90281 correctly fixes the receive-address handout race in PlatformAddressWallet and adds focused regression coverage for that path; I did not find a new defect in the two files changed by this last commit. In the cumulative PR range for #3585, however, both carried-forward prior findings remain valid: the post-broadcast unreconciled-success path still pins reservations for the rest of the process, and the rewritten DashPay send_payment path still has no direct regression test covering its call-site-specific behavior.
🟡 2 suggestion(s)
2 additional finding(s)
suggestion: Successful-but-unreconciled broadcasts still pin reservations until process restart
packages/rs-platform-wallet/src/wallet/core/reservations.rs (line 139)
ReservationGuard::leak_until_sync() still preserves reservations by skipping Drop with mem::forget(self), and the helper used by both send paths still takes that branch whenever broadcast succeeds but post-broadcast reconciliation does not (packages/rs-platform-wallet/src/wallet/core/broadcast.rs:130-146). That means send_to_addresses and send_payment can return success while leaving selected outpoints and pending change addresses reserved for the lifetime of the process if the wallet is missing or check_core_transaction(..) reports is_relevant = false. The double-spend risk is avoided, but the wallet can then degrade into persistent NoSpendableInputs or permanently burned change slots until restart because there is still no sync-time reclaim path.
suggestion: `IdentityWallet::send_payment` still lacks a direct regression test for its rewritten path
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (line 93)
A repository search on this head still finds no test that calls IdentityWallet::send_payment directly. The added coverage in wallet/core/broadcast.rs validates the shared broadcast_and_reconcile() helper, but it does not execute the DashPay-specific logic that this PR moved into send_payment: external-account lookup, recipient-address derivation, funding-account change-address advancement, reservation filtering against concurrent sends, and the on_reconcile hook that records the outgoing PaymentEntry. Because those behaviors remain untested end-to-end, a regression in the send_payment call site can still pass the current suite even though the shared helper is covered.
🤖 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.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/core/reservations.rs`:139-164: Successful-but-unreconciled broadcasts still pin reservations until process restart
`ReservationGuard::leak_until_sync()` still preserves reservations by skipping `Drop` with `mem::forget(self)`, and the helper used by both send paths still takes that branch whenever broadcast succeeds but post-broadcast reconciliation does not (`packages/rs-platform-wallet/src/wallet/core/broadcast.rs:130-146`). That means `send_to_addresses` and `send_payment` can return success while leaving selected outpoints and pending change addresses reserved for the lifetime of the process if the wallet is missing or `check_core_transaction(..)` reports `is_relevant = false`. The double-spend risk is avoided, but the wallet can then degrade into persistent `NoSpendableInputs` or permanently burned change slots until restart because there is still no sync-time reclaim path.
- [SUGGESTION] In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:93-316: `IdentityWallet::send_payment` still lacks a direct regression test for its rewritten path
A repository search on this head still finds no test that calls `IdentityWallet::send_payment` directly. The added coverage in `wallet/core/broadcast.rs` validates the shared `broadcast_and_reconcile()` helper, but it does not execute the DashPay-specific logic that this PR moved into `send_payment`: external-account lookup, recipient-address derivation, funding-account change-address advancement, reservation filtering against concurrent sends, and the `on_reconcile` hook that records the outgoing `PaymentEntry`. Because those behaviors remain untested end-to-end, a regression in the `send_payment` call site can still pass the current suite even though the shared helper is covered.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
# Conflicts: # packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs
The v3.1-dev merge added a required asset_locks parameter to PlatformAddressWallet::new. The Found-026 receive-address test helper wallet_with_platform_account still used the pre-merge 4-arg signature, breaking the --tests build. Construct a stub AssetLockManager (never exercised by these hand-out tests) and pass it through, mirroring the upstream build_short_circuit_wallet idiom in transfer.rs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Prior finding reconciliation for d6bf1b1: carried forward STILL VALID prior findings: #1:successful-unreconciled-broadcasts-pin-reservations, #2:identity-send-payment-missing-direct-regression-test; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.
Cumulative review of PR #3585 at d6bf1b1. The latest delta (b9a3be7..d6bf1b1) is small: threads AssetLockManager into PlatformAddressWallet::new, fixes is_dpns_name_available to treat proof-verified non-existence as available, and removes an unused helper. No new defects introduced. Both prior findings remain STILL VALID and are carried forward as suggestions: the leak_until_sync reservation-pinning behavior is now explicitly acknowledged via a TODO referencing this PR (reservations.rs:159), and IdentityWallet::send_payment still has no direct regression test (file has no #[cfg(test)] block, ends at line 329).
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/reservations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/reservations.rs:147-170: Successful-but-unreconciled broadcasts pin reservations until process restart
`ReservationGuard::leak_until_sync()` skips Drop via `mem::forget(self)`, and `broadcast_and_reconcile` takes that branch whenever the broadcast succeeded but `check_core_transaction` reported `is_relevant == false` (wallet-handle eviction, transient recognition failure, sync regression). The transaction may already be on the network, but the wallet keeps the outpoints and chosen change address reserved in memory until the process restarts — there is no `reclaim_committed`/sync-reconciliation hook. The TODO at lines 159–162 explicitly references this PR review thread and acknowledges the gap. The deliberate choice to hold rather than risk a double-spend is sound, but a transient reconciliation failure can semi-permanently shrink the spendable set, and the same `leak_until_sync` path is used unconditionally by `next_unused_receive_address` (platform_addresses/wallet.rs), where a long-lived session that hands out addresses without ever receiving payments grows the in-memory set without bound. Either land the periodic-sync-driven reclaim hook before merge, or surface a metric/log so operators notice leaked state before it starves new spends. Status: STILL VALID.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:93-329: `IdentityWallet::send_payment` lacks a direct regression test for its rewritten path
The PR rewrote the DashPay-specific `send_payment` orchestration end-to-end (write-lock-scoped snapshot/filter, change-address peek-and-burn loop, contact-address derivation, defense-in-depth subset check, reservation handoff into `broadcast_and_reconcile`, in-write-lock `record_dashpay_payment` hook). Coverage at the current head ships only via the shared `broadcast_and_reconcile` helper tests; `payments.rs` contains no `#[cfg(test)]` block (file ends at 329) and no `#[test]` / `#[tokio::test]` attribute. A future change to funding-account resolution, the change-address contention burn-loop, the `selected.is_subset(&spendable_outpoints)` tripwire, or the order-sensitive interaction between recipient-address derivation and change-address commit would leave the helper tests green while silently regressing the only DashPay outgoing-payment path. The burn-loop and reservation handoff are also the only defense against same-UTXO selection between concurrent DashPay payments, so a silent regression would resurrect the race that motivated the PR. Add at least one regression test that drives `send_payment` end-to-end with a stubbed broadcaster, asserting (a) the `PaymentEntry` is recorded on the sending `ManagedIdentity`, (b) outpoints are reserved during build and released after reconcile, and (c) a concurrent `send_payment`/`send_to_addresses` race short-circuits with `NoSpendableInputs`. Status: STILL VALID.
| /// Keep the reservation held until the process exits. | ||
| /// | ||
| /// Use this when the operation succeeded but wallet state could not | ||
| /// be reconciled, OR when the keys must remain reserved across an | ||
| /// asynchronous follow-up event (chain sync, balance flip) that | ||
| /// will eventually commit them out-of-band. | ||
| /// | ||
| /// **No in-process reclaim path exists today.** Keys stay pinned in | ||
| /// [`Reservations`] for the lifetime of the holding manager; only | ||
| /// a process restart (which drops the whole reservations set) | ||
| /// releases them. | ||
| /// | ||
| /// TODO(@thepastaclaw, PR #3585): a periodic-sync-driven | ||
| /// `Reservations::reclaim_committed(&[K])` API would let the next | ||
| /// confirmation / balance-sync pass reconcile leaked reservations | ||
| /// without a restart. Tracked on the PR review thread. | ||
| pub(crate) fn leak_until_sync(self) { | ||
| // `mem::forget` skips `Drop`, leaving the reservation entries | ||
| // in the shared set. The guard's only owned heap allocation is | ||
| // the `Vec<K>`, which is small and never freed for the lifetime | ||
| // of the process — acceptable given this is a rare error / async | ||
| // commit branch. | ||
| std::mem::forget(self); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Successful-but-unreconciled broadcasts pin reservations until process restart
ReservationGuard::leak_until_sync() skips Drop via mem::forget(self), and broadcast_and_reconcile takes that branch whenever the broadcast succeeded but check_core_transaction reported is_relevant == false (wallet-handle eviction, transient recognition failure, sync regression). The transaction may already be on the network, but the wallet keeps the outpoints and chosen change address reserved in memory until the process restarts — there is no reclaim_committed/sync-reconciliation hook. The TODO at lines 159–162 explicitly references this PR review thread and acknowledges the gap. The deliberate choice to hold rather than risk a double-spend is sound, but a transient reconciliation failure can semi-permanently shrink the spendable set, and the same leak_until_sync path is used unconditionally by next_unused_receive_address (platform_addresses/wallet.rs), where a long-lived session that hands out addresses without ever receiving payments grows the in-memory set without bound. Either land the periodic-sync-driven reclaim hook before merge, or surface a metric/log so operators notice leaked state before it starves new spends. Status: STILL VALID / INTENTIONALLY DEFERRED.
source: ['claude', 'codex']
| "DashPay payment broadcast" | ||
| ); | ||
|
|
||
| // --- 4. Record the outgoing payment on the sender's ManagedIdentity. --- | ||
| let entry = crate::wallet::identity::types::dashpay::payment::PaymentEntry::new_sent( | ||
| *to_contact_id, | ||
| amount_duffs, | ||
| memo, | ||
| ); | ||
| { | ||
| let mut wm = self.wallet_manager.write().await; | ||
| if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { | ||
| if let Some(managed) = info.identity_manager.managed_identity_mut(from_identity_id) | ||
| { | ||
| managed.record_dashpay_payment( | ||
| txid.to_string(), | ||
| entry.clone(), | ||
| &self.persister, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok((txid, entry)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: IdentityWallet::send_payment lacks a direct regression test for its rewritten path
The PR rewrote the DashPay-specific send_payment orchestration end-to-end (write-lock-scoped snapshot/filter, change-address peek-and-burn loop, contact-address derivation, defense-in-depth subset check, reservation handoff into broadcast_and_reconcile, in-write-lock record_dashpay_payment hook). Coverage at the current head ships only via the shared broadcast_and_reconcile helper tests; payments.rs contains no #[cfg(test)] block (file ends at 329) and no #[test] / #[tokio::test] attribute. A future change to funding-account resolution, the change-address contention burn-loop, the selected.is_subset(&spendable_outpoints) tripwire, or the order-sensitive interaction between recipient-address derivation and change-address commit would leave the helper tests green while silently regressing the only DashPay outgoing-payment path. The burn-loop and reservation handoff are also the only defense against same-UTXO selection between concurrent DashPay payments, so a silent regression would resurrect the race that motivated the PR. Add at least one regression test that drives send_payment end-to-end with a stubbed broadcaster, asserting (a) the PaymentEntry is recorded on the sending ManagedIdentity, (b) outpoints are reserved during build and released after reconcile, and (c) a concurrent send_payment/send_to_addresses race short-circuits with NoSpendableInputs. Status: STILL VALID.
source: ['claude', 'codex']
… defer to proper Reserved tri-state The receive-address hand-out race (Found-026) was being closed with a side-band `AddressReservations` set (generic `Reservations<K>` keyed by `(account, index)`, leaked via `leak_until_sync` on hand-out). That overloads the two-state `used` address model with an out-of-band "reserved" notion — a missing-state workaround that belongs upstream in `key-wallet` as a real `Unused -> Reserved -> Used` tri-state. This slims PR #3585 to the two independent, sound fixes: * case-insensitive `.dash` DPNS resolution (rs-sdk) * the `OutpointReservations` UTXO double-spend guard (platform-wallet) `OutpointReservations` and the generic `Reservations<K>`/`ReservationGuard<K>` core it builds on stay — only the address-reservation alias, key type, guard alias, and the on-hand-out reserve threading are removed. `next_unused_receive_address` reverts to mainline `next_unused(..)`, matching v3.1-dev with no reserve delta. The Found-026 white-box tests and the address-reservation unit tests are removed; they return with the follow-up tri-state PR. Co-Authored-By: Claude Opus 4.8 (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.
| /// Keep the reservation held until the process exits. | ||
| /// | ||
| /// Use this when the operation succeeded but wallet state could not | ||
| /// be reconciled, OR when the keys must remain reserved across an | ||
| /// asynchronous follow-up event (chain sync, balance flip) that | ||
| /// will eventually commit them out-of-band. | ||
| /// | ||
| /// **No in-process reclaim path exists today.** Keys stay pinned in | ||
| /// [`Reservations`] for the lifetime of the holding manager; only | ||
| /// a process restart (which drops the whole reservations set) | ||
| /// releases them. | ||
| /// | ||
| /// TODO(@thepastaclaw, PR #3585): a periodic-sync-driven | ||
| /// `Reservations::reclaim_committed(&[K])` API would let the next | ||
| /// confirmation / balance-sync pass reconcile leaked reservations | ||
| /// without a restart. Tracked on the PR review thread. | ||
| pub(crate) fn leak_until_sync(self) { | ||
| // `mem::forget` skips `Drop`, leaving the reservation entries | ||
| // in the shared set. The guard's only owned heap allocation is | ||
| // the `Vec<K>`, which is small and never freed for the lifetime | ||
| // of the process — acceptable given this is a rare error / async | ||
| // commit branch. | ||
| std::mem::forget(self); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['claude', 'codex']
5 participants
Issue being fixed or feature implemented
Two independent correctness bugs in the wallet / DPNS path:
Fix 1 — case-insensitive
.dashname resolution..dashusername lookups were case-sensitive, soAlice.dashfailed to resolvealice.dash. DPNS names are normalized to lowercase; the lookup path must normalize too.Fix 2 — UTXO double-spend race in concurrent sends. Two concurrent
send_to_addresses/send_paymentcalls on the same wallet could both select the same UTXO before either broadcast, producing a double-spend the network rejects — and could both pick the same change address.OutpointReservationsreserves the selected outpoints and the chosen change address under the wallet write lock so a concurrent caller skips them.Note
The receive-address hand-out reservation and the multi-pool
address_reservebridge that earlier drafts of this PR carried have been moved to the stacked follow-up #3770 (which is based on this branch). This PR no longer containsaddress_reserve.rs, thePoolKindmodel, ornext_unused_receive_addressreservation.What was done?
Fix 1 — DPNS
.dashcase-fold (wallet/identity/network/dpns.rs)Fix 2 —
OutpointReservations(wallet/core/reservations.rs,broadcast.rs,wallet.rs,identity/network/payments.rs)Reservations<K>core: aMutex-guardedHashSet<K>with an RAIIReservationGuard<K>that releases keys on drop, plusrelease_after_commit/leak_until_syncfor the broadcast-success / unrecoverable-leak outcomes.OutpointReservationscomposesReservations<OutPoint>(selected UTXOs) withReservations<Address>(pending_change— change addresses peeked but not yet committed to a confirmed broadcast).send_to_addresses/send_paymentsnapshot reserved outpoints, filter them from coin selection, pick a change address not already inpending_change, and reserve the selected set + change address before dropping the write lock; the post-broadcast reconcile releases (success) or leaks-until-sync (unrecoverable).FFI alignment (
rs-platform-wallet-ffi/src/core_wallet/*, incl.reservations_ffi.rs)How Has This Been Tested?
cargo test -p platform-wallet— green, includingconcurrent_callers_get_no_spendable_inputs(the loser getsNoSpendableInputs, never a broadcast error) and the outpoint / change-address reservation tests.cargo check -p platform-wallet-ffi— rc=0 (FFI parity).Breaking Changes
None. No serialized/persisted form changed; reservations are ephemeral.
Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent