fix: case-insensitive .dash suffix and UTXO double-spend prevention (re-target of #3466)#3585
fix: case-insensitive .dash suffix and UTXO double-spend prevention (re-target of #3466)#3585Claudius-Maginificent wants to merge 14 commits intov3.1-devfrom
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
📝 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 Possibly related PRs
Suggested reviewers
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 5d4a61b) |
![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: "1e182c0fabbd9ddf45b709215b7b777228fa9027d0e34d8037e5815115bf0436"
)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
Two-fix PR (case-insensitive .dash DPNS suffix; broadcast-first UTXO ordering in CoreWallet::send_to_addresses) is structurally sound at a3a5d96. No blocking issues. Five non-blocking observations remain: an asymmetric early-return in resolve_dpns_name, a misnamed/unreachable subset check, the new typed ConcurrentSpendConflict variant being erased at the FFI boundary, a silent post-broadcast !is_relevant warning that masks a wallet-internal invariant break for self-built txs, and missing unit coverage for the broadcast-ordering contract despite the broadcaster trait already being injectable.
Reviewed commit: a3a5d96
🟡 3 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: New `ConcurrentSpendConflict` variant is flattened to `ErrorUnknown` over FFI
packages/rs-platform-wallet-ffi/src/error.rs (lines 157-161)
The blanket impl From<PlatformWalletError> for PlatformWalletFFIResult at lines 157-161 maps every PlatformWalletError variant — including the newly-introduced ConcurrentSpendConflict (CMT-008) — to PlatformWalletFFIResultCode::ErrorUnknown plus a free-form error.to_string() message. The whole motivation for adding the typed variant (per the commit message) was to let FFI/UI/retry-loop callers distinguish a retryable concurrent-spend race from a genuine builder failure without parsing English error text. As implemented, Swift consumers of core_wallet_send_to_addresses (e.g. ManagedCoreWallet.sendToAddresses) still see only ErrorUnknown and must substring-match the localized-unsafe message "...concurrent spend); retry" to drive retries — exactly the brittleness the variant was meant to eliminate. Replace the blanket From with a per-variant match (or special-case ConcurrentSpendConflict first), and add a dedicated PlatformWalletFFIResultCode (e.g. ErrorConcurrentSpendConflict or a generic ErrorRetryable) so the typed Rust outcome survives the C ABI.
🤖 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-ffi/src/error.rs`:
- [SUGGESTION] lines 157-161: New `ConcurrentSpendConflict` variant is flattened to `ErrorUnknown` over FFI
The blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` at lines 157-161 maps every `PlatformWalletError` variant — including the newly-introduced `ConcurrentSpendConflict` (CMT-008) — to `PlatformWalletFFIResultCode::ErrorUnknown` plus a free-form `error.to_string()` message. The whole motivation for adding the typed variant (per the commit message) was to let FFI/UI/retry-loop callers distinguish a retryable concurrent-spend race from a genuine builder failure without parsing English error text. As implemented, Swift consumers of `core_wallet_send_to_addresses` (e.g. `ManagedCoreWallet.sendToAddresses`) still see only `ErrorUnknown` and must substring-match the localized-unsafe message `"...concurrent spend); retry"` to drive retries — exactly the brittleness the variant was meant to eliminate. Replace the blanket `From` with a per-variant match (or special-case `ConcurrentSpendConflict` first), and add a dedicated `PlatformWalletFFIResultCode` (e.g. `ErrorConcurrentSpendConflict` or a generic `ErrorRetryable`) so the typed Rust outcome survives the C ABI.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 227-246: Post-broadcast `!is_relevant` for self-built tx is silently downgraded to a warning
After a successful broadcast, the only path that records the spend in local state is `info.check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true)` at lines 230-232. For a transaction this wallet just built from its own `spendable` UTXOs (lines 78-82) and its own derived change address (lines 113-117), `check_result.is_relevant == false` is not a transient sync gap — it indicates a wallet-internal invariant break (xpub mismatch, derivation drift, account-map staleness). The branch at lines 233-238 emits only a `tracing::warn!` and returns `Ok(tx)`, so the next `send_to_addresses` from the same handle can reselect the same inputs and only learn of the desync via a network duplicate-spend rejection — exactly the failure mode this PR is meant to improve. The comment block at lines 215-226 explicitly bundles this with the wallet-missing branch as 'transient', but they are categorically different: wallet-missing is best-effort cleanup, while `!is_relevant` for an own-built tx is an internal contract violation. Surface it via a structured metric or distinct `tracing::error!` so operators can distinguish it from genuine sync-lag warnings in production telemetry.
- [SUGGESTION] lines 34-249: Broadcast-first ordering and rollback-on-failure contract have no automated coverage
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 #3466. `CoreWallet` is generic over `B: TransactionBroadcaster + ?Sized`, so the seam for deterministic unit tests already exists, but a repository-wide grep confirms no test under `packages/rs-platform-wallet` exercises `send_to_addresses` (only the implementation file matches). 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. Without coverage, only code review prevents a future refactor from re-introducing the original mark-spent-before-broadcast bug.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reviewed current head 79843e32 after the four CMT follow-up commits. No blockers. The empty-label DPNS early return is good, the revalidation/change-address wording is much clearer, and the post-broadcast !is_relevant path now emits a structured error event instead of a quiet warning.
Two non-blocking notes remain for maintainability:
- The new
broadcast_failure_keeps_inputs_spendable/broadcast_success_marks_inputs_unavailabletests only exercisebroadcast_transactionas a pass-through wrapper. They do not callsend_to_addresses, do not inspect the spendable UTXO set before/after, and do not verifycheck_core_transactionregistration on success. That still leaves the actual PR contract mostly covered by code shape rather than a regression test; I would either rename/narrow these tests or add a realsend_to_addressesfixture when practical. - The top-level recovery comment still says
!check_result.is_relevantis an “expected transient,” while the new structurederror!branch correctly treats an own-built tx being unrelated as an internal invariant violation. The code behavior is fine; the surrounding comment should be aligned so future readers do not downgrade that event again.
Validation run locally: git diff --check origin/v3.1-dev...HEAD; cargo fmt -p platform-wallet -p dash-sdk -- --check; cargo test -p dash-sdk --lib platform::dpns_usernames:: (5 passed / 0 failed / 6 ignored); cargo test -p platform-wallet --lib wallet::core::broadcast::tests:: (2 passed); cargo check -p platform-wallet --lib; cargo clippy -p platform-wallet --lib -- -D warnings.
Reviewed commit: 79843e3
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/rs-sdk/src/platform/dpns_usernames/mod.rs (1)
522-554: 💤 Low valueConsider adding a multi-label
.dashedge case totest_extract_dpns_label.
extract_dpns_label("alice.bob.dash")returns"alice.bob"(viarfind, which targets the last dot). The result is then normalized to"a11ce.b0b"— a lookup that will never match, so the runtime behavior is safe. But this case is currently undocumented and untested, and a future reader might wonder whether it was considered.✏️ Proposed addition to
test_extract_dpns_labelassert_eq!(extract_dpns_label(".dash"), ""); + // Multi-label: only the terminal `.dash` is stripped; the intermediate + // dot is kept as part of the returned label slice. + assert_eq!(extract_dpns_label("alice.bob.dash"), "alice.bob"); + assert_eq!(extract_dpns_label("alice.bob.eth"), "alice.bob.eth"); }🤖 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 522 - 554, Add a test asserting the multi-label `.dash` edge case for extract_dpns_label: call extract_dpns_label("alice.bob.dash") and assert it returns "alice.bob" (so behavior via rfind is covered); optionally add a complementary assertion that normalize_dpns_label("alice.bob.dash") yields "a11ce.b0b" to document the normalized multi-label result. Reference extract_dpns_label and normalize_dpns_label when adding these assertions to the existing test_extract_dpns_label / test_normalize_dpns_label blocks.packages/rs-platform-wallet/src/wallet/core/broadcast.rs (2)
168-188: 💤 Low valueOptional: extract repeated
managed_account_mutlookup into a helper.The
match account_type { … standard_bip44_accounts.get_mut(&account_index) … }pattern now appears three times in this function (lines 60-69, 102-107, 168-179) with identical error mapping at lines 180-185. A small helper onWalletInfo(or a free fn taking&mut WalletInfo) would eliminate the duplication and centralize the not-found error message; the borrow-checker rationale for re-lookup remains, but the syntactic noise drops significantly.Not blocking — the current code is correct and the duplication is local.
🤖 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 168 - 188, Extract the repeated lookup into a single helper (either an impl method on WalletInfo like managed_account_mut(&mut self, account_type: StandardAccountType, account_index: u32) -> Result<&mut ManagedAccount, PlatformWalletError> or a free fn taking &mut WalletInfo) that encapsulates the match over account_type and the ok_or_else error mapping used when calling next_change_address; replace the three occurrences that directly access standard_bip44_accounts/standard_bip32_accounts (the blocks referencing account_type, account_index, and .get_mut(...).ok_or_else(...)) with this helper to reduce duplication while preserving the same error text and borrow semantics.
282-440: ⚡ Quick winTest names overstate the contract; they cover the wrapper, not the regression.
The two test functions are named after
send_to_addressesinvariants (broadcast_failure_keeps_inputs_spendable,broadcast_success_marks_inputs_unavailable) but only exercisebroadcast_transaction, a one-line passthrough toself.broadcaster.broadcast(..). Neither test:
- runs
send_to_addresses,- observes any spendable-UTXO state before/after, nor
- asserts that
check_core_transactionis/isn't invoked depending on broadcast outcome.A future refactor that moves the post-broadcast
check_core_transaction(..)block above thebroadcast_transaction(..).await?line — i.e. the exact#3466regression — would still leave both tests green. The module-level rationale at lines 296-298 essentially says the same thing in reverse, but anchoring the regression to the trivial wrapper is brittle.Either rename the tests to describe the actual scope (e.g.
broadcast_transaction_propagates_broadcaster_err/_returns_broadcaster_txid) or expand coverage tosend_to_addressesso the broadcast-first ordering is genuinely pinned. GivenCoreWalletis generic overB: TransactionBroadcaster + ?Sized— the seam already exists — at minimum a renamed pair removes the misleading framing.♻️ Minimal rename to match actual scope
- /// Broadcast failure: `broadcast_transaction` propagates the - /// underlying `Err`, so callers (notably `send_to_addresses`) bail out - /// via `?` *before* the post-broadcast `check_core_transaction` block - /// can mark any input as spent. This is the rollback half of the - /// `#3466` contract: a network rejection must leave UTXOs spendable. - #[tokio::test] - async fn broadcast_failure_keeps_inputs_spendable() { + /// `broadcast_transaction` propagates the broadcaster's `Err` unchanged, + /// so `send_to_addresses`'s `?` short-circuits before + /// `check_core_transaction` runs. This pins the wrapper contract that + /// the broadcast-first ordering relies on — full state-mutation + /// coverage requires a `send_to_addresses` test (see follow-up). + #[tokio::test] + async fn broadcast_transaction_propagates_broadcaster_err() { @@ - #[tokio::test] - async fn broadcast_success_marks_inputs_unavailable() { + #[tokio::test] + async fn broadcast_transaction_returns_broadcaster_txid_on_ok() {🤖 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 282 - 440, The test names (broadcast_failure_keeps_inputs_spendable, broadcast_success_marks_inputs_unavailable) overstate coverage because they only exercise CoreWallet::broadcast_transaction (a one-line passthrough to self.broadcaster.broadcast) and do not call send_to_addresses or check UTXO state; either rename the tests to reflect the actual scope (e.g. broadcast_transaction_propagates_broadcaster_err and broadcast_transaction_returns_broadcaster_txid) or expand them to exercise CoreWallet::send_to_addresses and assert pre/post spendable-UTXO state and that check_core_transaction is/n't invoked depending on broadcaster outcome; update the test function identifiers accordingly and keep the existing MockBroadcaster usage to drive Ok/Err branches.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 168-188: Extract the repeated lookup into a single helper (either
an impl method on WalletInfo like managed_account_mut(&mut self, account_type:
StandardAccountType, account_index: u32) -> Result<&mut ManagedAccount,
PlatformWalletError> or a free fn taking &mut WalletInfo) that encapsulates the
match over account_type and the ok_or_else error mapping used when calling
next_change_address; replace the three occurrences that directly access
standard_bip44_accounts/standard_bip32_accounts (the blocks referencing
account_type, account_index, and .get_mut(...).ok_or_else(...)) with this helper
to reduce duplication while preserving the same error text and borrow semantics.
- Around line 282-440: The test names (broadcast_failure_keeps_inputs_spendable,
broadcast_success_marks_inputs_unavailable) overstate coverage because they only
exercise CoreWallet::broadcast_transaction (a one-line passthrough to
self.broadcaster.broadcast) and do not call send_to_addresses or check UTXO
state; either rename the tests to reflect the actual scope (e.g.
broadcast_transaction_propagates_broadcaster_err and
broadcast_transaction_returns_broadcaster_txid) or expand them to exercise
CoreWallet::send_to_addresses and assert pre/post spendable-UTXO state and that
check_core_transaction is/n't invoked depending on broadcaster outcome; update
the test function identifiers accordingly and keep the existing MockBroadcaster
usage to drive Ok/Err branches.
In `@packages/rs-sdk/src/platform/dpns_usernames/mod.rs`:
- Around line 522-554: Add a test asserting the multi-label `.dash` edge case
for extract_dpns_label: call extract_dpns_label("alice.bob.dash") and assert it
returns "alice.bob" (so behavior via rfind is covered); optionally add a
complementary assertion that normalize_dpns_label("alice.bob.dash") yields
"a11ce.b0b" to document the normalized multi-label result. Reference
extract_dpns_label and normalize_dpns_label when adding these assertions to the
existing test_extract_dpns_label / test_normalize_dpns_label blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87d3678a-ba22-4a8a-a89e-70de1a6beee9
📒 Files selected for processing (3)
packages/rs-platform-wallet/src/error.rspackages/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
Re-review at 79843e3: prior items (CMT-001/004/005/007/008 and DPNS API symmetry) are resolved. Three carry-forward review items remain — none blocking. The newly-introduced typed ConcurrentSpendConflict variant is collapsed to ErrorUnknown at the FFI boundary, defeating its purpose for Swift consumers; the new CMT-003 tests pin only the one-line broadcast_transaction pass-through rather than the send_to_addresses ordering contract their docstrings advertise; and the runtime ConcurrentSpendConflict branch is unreachable under current code paths, so its retryable framing conflates a builder-internal regression with the genuine race the variant promises.
Reviewed commit: 79843e3
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🟡 suggestion: `ConcurrentSpendConflict` is flattened to `ErrorUnknown` across the FFI boundary
packages/rs-platform-wallet-ffi/src/error.rs (lines 157-161)
The blanket impl From<PlatformWalletError> for PlatformWalletFFIResult maps every variant — including the newly-introduced ConcurrentSpendConflict (rs-platform-wallet/src/error.rs:63-64) — to PlatformWalletFFIResultCode::ErrorUnknown plus error.to_string(). The whole motivation for adding the typed variant per CMT-008 and the INTENTIONAL(CMT-002) note in broadcast.rs:154-161 was to let FFI/UI/retry-loop callers distinguish a retryable concurrent-spend race from a generic builder failure without parsing English error text. As implemented, Swift consumers of core_wallet_send_to_addresses (e.g. ManagedCoreWallet.sendToAddresses) still see only ErrorUnknown and would have to substring-match the localization-unsafe message "...concurrent spend); retry" to drive a retry — exactly the brittleness the variant was meant to eliminate. Replace the blanket From with a per-variant match (or special-case ConcurrentSpendConflict first), and add a dedicated PlatformWalletFFIResultCode (e.g. ErrorConcurrentSpendConflict or a more general ErrorRetryable) so the typed Rust outcome survives the C ABI. Note that adding a new #[repr(C)] enum discriminant is itself an ABI change requiring the C header to be regenerated and Swift consumers rebuilt.
🤖 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-ffi/src/error.rs`:
- [SUGGESTION] lines 157-161: `ConcurrentSpendConflict` is flattened to `ErrorUnknown` across the FFI boundary
The blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` maps every variant — including the newly-introduced `ConcurrentSpendConflict` (`rs-platform-wallet/src/error.rs:63-64`) — to `PlatformWalletFFIResultCode::ErrorUnknown` plus `error.to_string()`. The whole motivation for adding the typed variant per CMT-008 and the `INTENTIONAL(CMT-002)` note in `broadcast.rs:154-161` was to let FFI/UI/retry-loop callers distinguish a retryable concurrent-spend race from a generic builder failure without parsing English error text. As implemented, Swift consumers of `core_wallet_send_to_addresses` (e.g. `ManagedCoreWallet.sendToAddresses`) still see only `ErrorUnknown` and would have to substring-match the localization-unsafe message `"...concurrent spend); retry"` to drive a retry — exactly the brittleness the variant was meant to eliminate. Replace the blanket `From` with a per-variant match (or special-case `ConcurrentSpendConflict` first), and add a dedicated `PlatformWalletFFIResultCode` (e.g. `ErrorConcurrentSpendConflict` or a more general `ErrorRetryable`) so the typed Rust outcome survives the C ABI. Note that adding a new `#[repr(C)]` enum discriminant is itself an ABI change requiring the C header to be regenerated and Swift consumers rebuilt.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 282-441: CMT-003 tests pin `broadcast_transaction`'s pass-through, not the `send_to_addresses` rollback contract
The module docstring (lines 284-299) and per-test docstrings (e.g. 388-392, 415-420) describe these as the regression-prevention tests for the #3466 broadcast-first / rollback contract: "a failed broadcast must propagate Err so callers short-circuit before any spendable-set mutation" and "broadcast success must hand the txid back so the caller can register the tx as a mempool spend." But `broadcast_transaction` is a one-line wrapper (`self.broadcaster.broadcast(transaction).await` at line 26), and both tests call it directly (lines 401, 428) — they assert only that an `Err` matches `TransactionBroadcast(_)` and that `call_count == 1`. They never construct a wallet with funded UTXOs, never invoke `send_to_addresses`, and never observe the spendable-UTXO set or the post-broadcast `check_core_transaction` block before vs. after. `make_core_wallet` uses an empty `WalletManager::new(Network::Testnet)`, so `send_to_addresses` would short-circuit at line 51 with `WalletNotFound` before reaching the broadcaster anyway. A future refactor that re-orders the post-broadcast registration block back above `self.broadcast_transaction(&tx).await?` on line 200 — re-introducing #3466's mark-spent-before-broadcast bug — would leave both tests green. This is the canonical "testing the mock, not the code" anti-pattern. Either rewrite the tests to drive `send_to_addresses` end-to-end (fixture UTXOs through `WalletManager`, then compare `account.spendable_utxos(...)` across an `Err` and `Ok` mock broadcaster) or rename them and tighten the docstrings to reflect that they only pin the wrapper's pass-through.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two-fix PR (case-insensitive .dash DPNS suffix; broadcast-then-mark-spent reorder in CoreWallet::send_to_addresses) is structurally sound. Three convergent suggestions remain across both reviewers: the new ConcurrentSpendConflict variant is erased to ErrorUnknown at the FFI boundary (defeating its purpose for Swift retry logic); the new tests pin broadcast_transaction pass-through, not the send_to_addresses ordering they document; and the change-address derivation index is still advanced before the fallible broadcast step, partially undermining the rollback contract. A few smaller nits accompany them.
Reviewed commit: 79843e3
🟡 3 suggestion(s) | 💬 3 nitpick(s)
1 additional finding
🟡 suggestion: `ConcurrentSpendConflict` is flattened to `ErrorUnknown` at the FFI boundary
packages/rs-platform-wallet-ffi/src/error.rs (lines 157-161)
The new typed variant PlatformWalletError::ConcurrentSpendConflict (rs-platform-wallet/src/error.rs:63-64) was added so foreign callers can branch on a retryable UTXO race instead of substring-matching English text. But the blanket impl From<PlatformWalletError> for PlatformWalletFFIResult here maps every variant — including the new one — to PlatformWalletFFIResultCode::ErrorUnknown plus error.to_string(). core_wallet_send_to_addresses routes errors through unwrap_result_or_return!, so Swift consumers (e.g. ManagedCoreWallet.sendToAddresses) receive only errorUnknown plus the localizable-unsafe message "...concurrent spend); retry". The Rust-level distinction never reaches the iOS retry path it was added for. Either special-case ConcurrentSpendConflict in this From (mapping to a dedicated PlatformWalletFFIResultCode such as ErrorRetryable / ErrorConcurrentSpendConflict), or land a follow-up that adds the code before any caller starts depending on the current behaviour.
🤖 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-ffi/src/error.rs`:
- [SUGGESTION] lines 157-161: `ConcurrentSpendConflict` is flattened to `ErrorUnknown` at the FFI boundary
The new typed variant `PlatformWalletError::ConcurrentSpendConflict` (rs-platform-wallet/src/error.rs:63-64) was added so foreign callers can branch on a retryable UTXO race instead of substring-matching English text. But the blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` here maps every variant — including the new one — to `PlatformWalletFFIResultCode::ErrorUnknown` plus `error.to_string()`. `core_wallet_send_to_addresses` routes errors through `unwrap_result_or_return!`, so Swift consumers (e.g. `ManagedCoreWallet.sendToAddresses`) receive only `errorUnknown` plus the localizable-unsafe message `"...concurrent spend); retry"`. The Rust-level distinction never reaches the iOS retry path it was added for. Either special-case `ConcurrentSpendConflict` in this `From` (mapping to a dedicated `PlatformWalletFFIResultCode` such as `ErrorRetryable` / `ErrorConcurrentSpendConflict`), or land a follow-up that adds the code before any caller starts depending on the current behaviour.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 282-441: New tests pin `broadcast_transaction` pass-through, not the `send_to_addresses` ordering contract they document
The module docstring frames these tests as the regression pin for the #3466 ordering contract in `send_to_addresses` ("a failed `broadcast_transaction` must propagate `Err` so callers short-circuit before any spendable-set mutation"), but both tests call `wallet.broadcast_transaction(&tx)` (lines 401, 428), which is a one-line forwarder to `self.broadcaster.broadcast(...)` (lines 22-27). They assert pass-through of `Result<Txid, _>` and a call count, never construct a wallet with spendable UTXOs, never invoke `send_to_addresses`, and never observe spendable-set state before/after broadcast. A future refactor that moves the post-broadcast `check_core_transaction(.., Mempool, ..)` block above the `?` on line 200 would re-introduce the original mark-spent-before-broadcast bug while keeping these tests green. The seam exists (`CoreWallet` is generic over `B: TransactionBroadcaster + ?Sized`); the missing piece is a wallet fixture with one spendable UTXO that drives `send_to_addresses` end-to-end and asserts the spendable-set delta against the broadcaster outcome. At minimum, the docstring should be downgraded from "central correctness" to "wrapper passthrough" so future readers don't over-trust these names.
- [SUGGESTION] lines 164-200: Change-address derivation index is advanced before the fallible broadcast step
CMT-007 defers the change advance until after revalidation passes, but the commit (`next_change_address(Some(&xpub), true)` at line 187) still runs inside the lock-held block before `self.broadcast_transaction(&tx).await?` at line 200. A network-rejection broadcast failure therefore still burns the change index and leaves a derived-but-never-used gap address. The comment at lines 109-112 frames the deferral as protection against widening the gap-limit window on retry, but the broadcast-failure path produces exactly that outcome — the rollback contract the PR documents ("if the network rejects we leave wallet state untouched", line 193-194) is only partial. Either advance the index after `broadcast_transaction(..)` returns `Ok(_)`, or compute the change address from a peek and persist the index advance only on overall success. Since the index advance is wallet-local mutable state and the advance call is presumably cheap, deferring it past the `?` on line 200 is the straightforward fix.
#3585 The recent CMT-001/003/004/005/007/002 fixes added overlapping commentary across broadcast.rs and dpns_usernames/mod.rs — INTENTIONAL annotations, rewritten revalidation comment, structured-event surrounds, test docstrings. Trim redundancy while preserving the INTENTIONAL marker pattern, structured tracing fields, and all CMT-NNN cross-references. 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
Re-review at 391768c. Most prior carry-forwards are addressed: DPNS API symmetry, CMT-007 change-address peek, CMT-004 structured-event upgrade, and the typed ConcurrentSpendConflict variant all landed cleanly. Three non-blocking issues remain and are convergently flagged by both reviewers: the new typed variant is still flattened to ErrorUnknown across the FFI boundary; the new CMT-003 tests pin only the one-line broadcast_transaction wrapper rather than the send_to_addresses ordering contract their docstring/names advertise; and the change-address derivation index is still committed before the fallible broadcast, so a network-rejected broadcast burns an index even though the spendable-set state is preserved. Two minor Rust-quality nitpicks retained.
Reviewed commit: 391768c
🟡 3 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🟡 suggestion: New ConcurrentSpendConflict variant is flattened to ErrorUnknown across the FFI boundary
packages/rs-platform-wallet-ffi/src/error.rs (lines 157-161)
The blanket impl From<PlatformWalletError> for PlatformWalletFFIResult maps every wallet error — including the newly-added PlatformWalletError::ConcurrentSpendConflict (packages/rs-platform-wallet/src/error.rs:63-64) — to PlatformWalletFFIResultCode::ErrorUnknown plus error.to_string(). The whole motivation for adding the typed variant, per the INTENTIONAL(CMT-002) annotation at broadcast.rs:151-153 ("forward compatibility with cross-process concurrent-spend surfacing"), is to let foreign callers branch on a retryable UTXO race without parsing English text. As wired, Swift consumers of core_wallet_send_to_addresses (e.g. ManagedCoreWallet.sendToAddresses) still receive only .errorUnknown plus the localization-unsafe message "...concurrent spend); retry" and would have to substring-match it to drive a retry — exactly the brittleness the typed variant was meant to eliminate. Special-case ConcurrentSpendConflict ahead of the blanket conversion and add a dedicated PlatformWalletFFIResultCode discriminant (e.g. ErrorRetryable or ErrorConcurrentSpendConflict). Note that adding a new #[repr(C)] discriminant is itself a C-ABI change requiring header regeneration and Swift consumer rebuilds, so it is best landed before any caller starts depending on the current ErrorUnknown shape.
🤖 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-ffi/src/error.rs`:
- [SUGGESTION] lines 157-161: New ConcurrentSpendConflict variant is flattened to ErrorUnknown across the FFI boundary
The blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` maps every wallet error — including the newly-added `PlatformWalletError::ConcurrentSpendConflict` (`packages/rs-platform-wallet/src/error.rs:63-64`) — to `PlatformWalletFFIResultCode::ErrorUnknown` plus `error.to_string()`. The whole motivation for adding the typed variant, per the `INTENTIONAL(CMT-002)` annotation at `broadcast.rs:151-153` ("forward compatibility with cross-process concurrent-spend surfacing"), is to let foreign callers branch on a retryable UTXO race without parsing English text. As wired, Swift consumers of `core_wallet_send_to_addresses` (e.g. `ManagedCoreWallet.sendToAddresses`) still receive only `.errorUnknown` plus the localization-unsafe message `"...concurrent spend); retry"` and would have to substring-match it to drive a retry — exactly the brittleness the typed variant was meant to eliminate. Special-case `ConcurrentSpendConflict` ahead of the blanket conversion and add a dedicated `PlatformWalletFFIResultCode` discriminant (e.g. `ErrorRetryable` or `ErrorConcurrentSpendConflict`). Note that adding a new `#[repr(C)]` discriminant is itself a C-ABI change requiring header regeneration and Swift consumer rebuilds, so it is best landed before any caller starts depending on the current `ErrorUnknown` shape.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 264-397: CMT-003 tests pin broadcast_transaction's pass-through, not the send_to_addresses ordering contract their names advertise
The module docstring at lines 266-271 frames these as the regression pin for the #3466 broadcast-first / rollback contract, and the test names `broadcast_failure_keeps_inputs_spendable` (line 354) and `broadcast_success_marks_inputs_unavailable` (line 378) directly invoke spendable-set semantics. But `broadcast_transaction` is a one-line wrapper around `self.broadcaster.broadcast(transaction).await` (lines 22-27), and both tests call `wallet.broadcast_transaction(&tx)` (lines 361, 384) — they only assert that an `Err` matches `TransactionBroadcast(_)`, that `Ok(txid)` is forwarded unchanged, and that `call_count == 1`. They never construct a wallet with funded UTXOs, never invoke `send_to_addresses`, and never observe the spendable set or change-address state before vs. after either branch. `make_core_wallet` (lines 335-349) builds an empty `WalletManager::new(Network::Testnet)`, so calling `send_to_addresses` would short-circuit at line 51 with `WalletNotFound` long before reaching the broadcaster anyway. Net effect: a future refactor that re-orders the post-broadcast `check_core_transaction(.., update_state = true, ..)` block at lines 230-258 above the `?` on line 193 — re-introducing the exact mark-spent-before-broadcast bug from #3466 — would leave both tests green. Either rewrite them to drive `send_to_addresses` end-to-end (build a wallet with one spendable UTXO, run the call, assert the spendable-set delta against broadcaster outcome), or rename them and tighten the docstring to reflect that they only pin the wrapper's pass-through and leave contract pinning to a follow-up.
- [SUGGESTION] lines 179-193: Change-address derivation index is committed before the fallible broadcast, partially undermining the rollback contract
CMT-007 correctly defers the change-address advance past the post-build revalidation: lines 113-115 use `next_change_address(Some(&xpub), false)` to peek without advancing, and the comment at lines 109-112 captures that narrow case. But the actual `next_change_address(Some(&xpub), true)` commit at lines 179-181 still runs inside the closure that ends at line 184, while `self.broadcast_transaction(&tx).await?` at line 193 runs after — so a network-rejection broadcast failure still advances the change index and leaves a derived-but-never-used gap address. That contradicts the broader rollback claim two lines below: "if the network rejects we leave wallet state untouched so the caller can retry without manual sync repair" (lines 186-187). Each retry against a flaky broadcaster widens the gap further. (Verified at upstream `key-wallet/src/managed_account/address_pool.rs:509`: with `add_to_state=true`, `addresses` is inserted and `highest_generated` is bumped; with `false` neither happens, so the peek itself is a true peek but the commit absolutely mutates pool state.) Either move the commit advance after `broadcast_transaction(..)` returns `Ok(_)` (re-acquiring the lock once for both the index commit and the `check_core_transaction` registration block at lines 230-258), or compute the change address from a peek and persist the advance only on overall success. Bounded impact (BIP44 gap limit ≥ 20 absorbs many failures), so suggestion rather than blocking.
| #[cfg(test)] | ||
| mod tests { | ||
| //! Broadcast ordering / rollback contract tests (CMT-003). | ||
| //! | ||
| //! Pin `broadcast_transaction`'s pass-through behaviour: `Err` propagates | ||
| //! so `send_to_addresses` short-circuits before any spendable-set | ||
| //! mutation, and `Ok(txid)` is forwarded unchanged so the post-broadcast | ||
| //! mempool registration runs on a confirmed-success signal. See #3466. | ||
| use std::sync::atomic::{AtomicUsize, Ordering}; | ||
| use std::sync::Arc; | ||
|
|
||
| use async_trait::async_trait; | ||
| use dashcore::consensus::deserialize; | ||
| use dashcore::{Transaction, Txid}; | ||
| use tokio::sync::RwLock; | ||
|
|
||
| use crate::broadcaster::TransactionBroadcaster; | ||
| use crate::wallet::core::balance::WalletBalance; | ||
| use crate::wallet::core::CoreWallet; | ||
| use crate::PlatformWalletError; | ||
| use key_wallet::Network; | ||
| use key_wallet_manager::WalletManager; | ||
|
|
||
| /// Records every call and returns a canned outcome. | ||
| struct MockBroadcaster { | ||
| outcome: BroadcastOutcome, | ||
| calls: AtomicUsize, | ||
| } | ||
|
|
||
| enum BroadcastOutcome { | ||
| Ok(Txid), | ||
| Err(String), | ||
| } | ||
|
|
||
| impl MockBroadcaster { | ||
| fn new(outcome: BroadcastOutcome) -> Self { | ||
| Self { | ||
| outcome, | ||
| calls: AtomicUsize::new(0), | ||
| } | ||
| } | ||
|
|
||
| fn call_count(&self) -> usize { | ||
| self.calls.load(Ordering::SeqCst) | ||
| } | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl TransactionBroadcaster for MockBroadcaster { | ||
| async fn broadcast(&self, _transaction: &Transaction) -> Result<Txid, PlatformWalletError> { | ||
| self.calls.fetch_add(1, Ordering::SeqCst); | ||
| match &self.outcome { | ||
| BroadcastOutcome::Ok(txid) => Ok(*txid), | ||
| BroadcastOutcome::Err(msg) => { | ||
| Err(PlatformWalletError::TransactionBroadcast(msg.clone())) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Minimal serialized tx (1 input, 1 output, 0 value) — only the | ||
| /// broadcaster's Err/Ok branch matters here, not the shape. | ||
| fn dummy_transaction() -> Transaction { | ||
| let bytes = hex::decode( | ||
| "010000000100000000000000000000000000000000000000000000000000000000000000\ | ||
| 00ffffffff00ffffffff0100000000000000000000000000", | ||
| ) | ||
| .expect("valid hex"); | ||
| deserialize(&bytes).expect("deserializable tx") | ||
| } | ||
|
|
||
| fn make_core_wallet<B: TransactionBroadcaster + ?Sized>(broadcaster: Arc<B>) -> CoreWallet<B> { | ||
| let sdk = Arc::new( | ||
| dash_sdk::SdkBuilder::new_mock() | ||
| .build() | ||
| .expect("mock sdk build"), | ||
| ); | ||
| let wallet_manager = Arc::new(RwLock::new(WalletManager::new(Network::Testnet))); | ||
| CoreWallet::new( | ||
| sdk, | ||
| wallet_manager, | ||
| [0u8; 32], | ||
| broadcaster, | ||
| Arc::new(WalletBalance::new()), | ||
| ) | ||
| } | ||
|
|
||
| /// Rollback half of the #3466 contract: a broadcast `Err` propagates so | ||
| /// callers `?`-out before any spendable-set mutation. | ||
| #[tokio::test] | ||
| async fn broadcast_failure_keeps_inputs_spendable() { | ||
| let broadcaster = Arc::new(MockBroadcaster::new(BroadcastOutcome::Err( | ||
| "simulated network rejection".to_string(), | ||
| ))); | ||
| let wallet = make_core_wallet(Arc::clone(&broadcaster)); | ||
| let tx = dummy_transaction(); | ||
|
|
||
| let result = wallet.broadcast_transaction(&tx).await; | ||
|
|
||
| assert!( | ||
| matches!(result, Err(PlatformWalletError::TransactionBroadcast(_))), | ||
| "expected broadcast Err to propagate, got {:?}", | ||
| result | ||
| ); | ||
| assert_eq!( | ||
| broadcaster.call_count(), | ||
| 1, | ||
| "broadcaster must be called exactly once on a failed broadcast" | ||
| ); | ||
| } | ||
|
|
||
| /// Success half of the #3466 contract: the broadcaster's `Txid` is | ||
| /// passed through unchanged so the mempool-registration block fires. | ||
| #[tokio::test] | ||
| async fn broadcast_success_marks_inputs_unavailable() { | ||
| let expected_txid = dummy_transaction().txid(); | ||
| let broadcaster = Arc::new(MockBroadcaster::new(BroadcastOutcome::Ok(expected_txid))); | ||
| let wallet = make_core_wallet(Arc::clone(&broadcaster)); | ||
| let tx = dummy_transaction(); | ||
|
|
||
| let result = wallet.broadcast_transaction(&tx).await; | ||
|
|
||
| assert_eq!( | ||
| result.expect("broadcast Ok"), | ||
| expected_txid, | ||
| "broadcast_transaction must pass the broadcaster's Txid through unchanged" | ||
| ); | ||
| assert_eq!( | ||
| broadcaster.call_count(), | ||
| 1, | ||
| "broadcaster must be called exactly once on a successful broadcast" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: CMT-003 tests pin broadcast_transaction's pass-through, not the send_to_addresses ordering contract their names advertise
The module docstring at lines 266-271 frames these as the regression pin for the #3466 broadcast-first / rollback contract, and the test names broadcast_failure_keeps_inputs_spendable (line 354) and broadcast_success_marks_inputs_unavailable (line 378) directly invoke spendable-set semantics. But broadcast_transaction is a one-line wrapper around self.broadcaster.broadcast(transaction).await (lines 22-27), and both tests call wallet.broadcast_transaction(&tx) (lines 361, 384) — they only assert that an Err matches TransactionBroadcast(_), that Ok(txid) is forwarded unchanged, and that call_count == 1. They never construct a wallet with funded UTXOs, never invoke send_to_addresses, and never observe the spendable set or change-address state before vs. after either branch. make_core_wallet (lines 335-349) builds an empty WalletManager::new(Network::Testnet), so calling send_to_addresses would short-circuit at line 51 with WalletNotFound long before reaching the broadcaster anyway. Net effect: a future refactor that re-orders the post-broadcast check_core_transaction(.., update_state = true, ..) block at lines 230-258 above the ? on line 193 — re-introducing the exact mark-spent-before-broadcast bug from #3466 — would leave both tests green. Either rewrite them to drive send_to_addresses end-to-end (build a wallet with one spendable UTXO, run the call, assert the spendable-set delta against broadcaster outcome), or rename them and tighten the docstring to reflect that they only pin the wrapper's pass-through and leave contract pinning to a follow-up.
source: ['claude', 'codex']
🤖 Fix this 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 264-397: CMT-003 tests pin broadcast_transaction's pass-through, not the send_to_addresses ordering contract their names advertise
The module docstring at lines 266-271 frames these as the regression pin for the #3466 broadcast-first / rollback contract, and the test names `broadcast_failure_keeps_inputs_spendable` (line 354) and `broadcast_success_marks_inputs_unavailable` (line 378) directly invoke spendable-set semantics. But `broadcast_transaction` is a one-line wrapper around `self.broadcaster.broadcast(transaction).await` (lines 22-27), and both tests call `wallet.broadcast_transaction(&tx)` (lines 361, 384) — they only assert that an `Err` matches `TransactionBroadcast(_)`, that `Ok(txid)` is forwarded unchanged, and that `call_count == 1`. They never construct a wallet with funded UTXOs, never invoke `send_to_addresses`, and never observe the spendable set or change-address state before vs. after either branch. `make_core_wallet` (lines 335-349) builds an empty `WalletManager::new(Network::Testnet)`, so calling `send_to_addresses` would short-circuit at line 51 with `WalletNotFound` long before reaching the broadcaster anyway. Net effect: a future refactor that re-orders the post-broadcast `check_core_transaction(.., update_state = true, ..)` block at lines 230-258 above the `?` on line 193 — re-introducing the exact mark-spent-before-broadcast bug from #3466 — would leave both tests green. Either rewrite them to drive `send_to_addresses` end-to-end (build a wallet with one spendable UTXO, run the call, assert the spendable-set delta against broadcaster outcome), or rename them and tighten the docstring to reflect that they only pin the wrapper's pass-through and leave contract pinning to a follow-up.
| change_account | ||
| .next_change_address(Some(&xpub), true) | ||
| .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; | ||
|
|
||
| tx | ||
| }; | ||
|
|
||
| // Broadcast first; if the network rejects we leave wallet state | ||
| // untouched so the caller can retry without manual sync repair. | ||
| // This is intentional even if the remote accepted the transaction | ||
| // but the broadcast path returned an error: in that ambiguous case | ||
| // later attempts may reuse the same inputs locally, but the network | ||
| // rejects the duplicate spend instead of us marking UTXOs spent for | ||
| // a transaction that might not have propagated. | ||
| self.broadcast_transaction(&tx).await?; |
There was a problem hiding this comment.
🟡 Suggestion: Change-address derivation index is committed before the fallible broadcast, partially undermining the rollback contract
CMT-007 correctly defers the change-address advance past the post-build revalidation: lines 113-115 use next_change_address(Some(&xpub), false) to peek without advancing, and the comment at lines 109-112 captures that narrow case. But the actual next_change_address(Some(&xpub), true) commit at lines 179-181 still runs inside the closure that ends at line 184, while self.broadcast_transaction(&tx).await? at line 193 runs after — so a network-rejection broadcast failure still advances the change index and leaves a derived-but-never-used gap address. That contradicts the broader rollback claim two lines below: "if the network rejects we leave wallet state untouched so the caller can retry without manual sync repair" (lines 186-187). Each retry against a flaky broadcaster widens the gap further. (Verified at upstream key-wallet/src/managed_account/address_pool.rs:509: with add_to_state=true, addresses is inserted and highest_generated is bumped; with false neither happens, so the peek itself is a true peek but the commit absolutely mutates pool state.) Either move the commit advance after broadcast_transaction(..) returns Ok(_) (re-acquiring the lock once for both the index commit and the check_core_transaction registration block at lines 230-258), or compute the change address from a peek and persist the advance only on overall success. Bounded impact (BIP44 gap limit ≥ 20 absorbs many failures), so suggestion rather than blocking.
source: ['claude', 'codex']
🤖 Fix this 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 179-193: Change-address derivation index is committed before the fallible broadcast, partially undermining the rollback contract
CMT-007 correctly defers the change-address advance past the post-build revalidation: lines 113-115 use `next_change_address(Some(&xpub), false)` to peek without advancing, and the comment at lines 109-112 captures that narrow case. But the actual `next_change_address(Some(&xpub), true)` commit at lines 179-181 still runs inside the closure that ends at line 184, while `self.broadcast_transaction(&tx).await?` at line 193 runs after — so a network-rejection broadcast failure still advances the change index and leaves a derived-but-never-used gap address. That contradicts the broader rollback claim two lines below: "if the network rejects we leave wallet state untouched so the caller can retry without manual sync repair" (lines 186-187). Each retry against a flaky broadcaster widens the gap further. (Verified at upstream `key-wallet/src/managed_account/address_pool.rs:509`: with `add_to_state=true`, `addresses` is inserted and `highest_generated` is bumped; with `false` neither happens, so the peek itself is a true peek but the commit absolutely mutates pool state.) Either move the commit advance after `broadcast_transaction(..)` returns `Ok(_)` (re-acquiring the lock once for both the index commit and the `check_core_transaction` registration block at lines 230-258), or compute the change address from a peek and persist the advance only on overall success. Bounded impact (BIP44 gap limit ≥ 20 absorbs many failures), so suggestion rather than blocking.
| let selected: BTreeSet<OutPoint> = | ||
| tx.input.iter().map(|txin| txin.previous_output).collect(); | ||
| let spendable_outpoints: BTreeSet<OutPoint> = | ||
| spendable.iter().map(|utxo| utxo.outpoint).collect(); |
There was a problem hiding this comment.
💬 Nitpick: Two BTreeSet allocations on every successful build for an unreachable check
Per the comment block at lines 141-145, the subset check is unreachable under normal operation. The two BTreeSet<OutPoint> allocations at lines 146-149 — O(n log n) per build — therefore run on every happy-path send purely to detect a builder-internal regression. For wallets with many small UTXOs the spendable-side allocation dominates and is wasted work. Cheaper alternatives that keep the regression catch: downgrade to debug_assert! so the cost is paid only in test/dev builds, or iterate tx.input once and look up each previous_output in a single pre-built HashSet of spendable outpoints (eliminating the second set construction).
source: ['claude']
| let change_account = match account_type { | ||
| StandardAccountType::BIP44Account => info | ||
| .core_wallet | ||
| .accounts | ||
| .standard_bip44_accounts | ||
| .get_mut(&account_index), | ||
| StandardAccountType::BIP32Account => info | ||
| .core_wallet | ||
| .accounts | ||
| .standard_bip32_accounts | ||
| .get_mut(&account_index), | ||
| } | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::TransactionBuild(format!( | ||
| "{:?} managed account {} not found", | ||
| account_type, account_index | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
💬 Nitpick: Re-borrow lookup surfaces an unreachable invariant break as a user-facing TransactionBuild error
After revalidation passes, the closure re-acquires &mut to the same managed account in order to commit the change-address advance. The .ok_or_else(...) at lines 173-178 returns PlatformWalletError::TransactionBuild(format!("{:?} managed account {} not found", ...)) if the lookup fails — but reaching this branch would mean managed_accounts.get_mut(&account_index) succeeded on line 102 yet failed here, which is impossible while the same wm.write() guard from line 50 is still held. The .ok_or_else exists to satisfy the borrow checker, not because the lookup can realistically fail. Surfacing this as a user-facing TransactionBuild("... not found") conflates an impossible internal invariant break with genuine builder failures. Consider .expect("managed account vanished while wallet_manager write lock held — invariant break") or routing through a dedicated internal-error variant.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reviewed the delta from 79843e32 to 391768ce.
This commit is comment/docs-only: it tightens the CMT notes in broadcast.rs and the empty-label DPNS comment without changing the DPNS normalization or wallet broadcast behavior. I don’t see any new blockers in this delta.
Carry-forward: the existing non-blocking feedback on the broadcast_transaction tests still applies in substance — the docs are less overstated now, but the test names still read like end-to-end send_to_addresses/UTXO-state coverage while they only exercise the wrapper pass-through. That can be handled as a follow-up/nit and doesn’t block this PR.
Validation on current head:
git diff --check origin/v3.1-dev...HEADcargo fmt -p platform-wallet -p dash-sdk -- --checkcargo test -p dash-sdk --lib platform::dpns_usernames::— 5 passed / 0 failed / 6 ignoredcargo test -p platform-wallet --lib wallet::core::broadcast::tests::— 2 passedcargo check -p platform-wallet --lib
Reviewed commit: 391768c
…CMT-001) next_change_address(.., true) ran inside the write-lock block before the fallible broadcast_transaction call, so a broadcast failure left the derivation index advanced — burning a gap-limit address with no on-chain record. Move the commit past the broadcast ? so the index only advances on broadcast success. Reapplies CMT-007's intent properly — the earlier fix (23d8943) used the peek-then-commit shape but the commit was still before broadcast. Thread: PRRT_kwDOGUlHz85_9Neu Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h scope (CMT-002) The tests drive CoreWallet::broadcast_transaction directly with a MockBroadcaster, pinning Err/Ok pass-through. The previous names and docstring framed them as #3466 send_to_addresses rollback regression pins, which they aren't (they don't drive send_to_addresses). Rename to describe what they actually pin. Thread: PRRT_kwDOGUlHz85_9Nej Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/core/broadcast.rs (1)
109-112: ⚡ Quick winUpdate the rollback comments to match the implemented behavior.
Line 109 says the change index is committed once revalidation passes, but the code now commits it only after a successful broadcast. Lines 194-196 also call
!check_result.is_relevantan expected transient, while Lines 243-246 treat the same condition as an internal invariant violation and emiterror!. These comments sit on the tricky broadcast-first path, so they should tell one consistent story.Suggested comment-only cleanup
- // Peek at the next change address without advancing the derivation - // index. We commit the advance only after post-build revalidation - // succeeds, so a revalidation failure does not burn an index and - // widen the gap-limit window on retry. + // Peek at the next change address without advancing the derivation + // index. We commit the advance only after a successful broadcast, + // so revalidation or broadcast failures do not burn an index and + // widen the gap-limit window on retry. @@ - // * `!check_result.is_relevant` is the expected transient: the - // wallet just hasn't ingested the tx yet (or some derivation - // path/script is unrecognised), and a later sync will fix it. + // * `!check_result.is_relevant` for this own-built transaction is + // an internal wallet inconsistency. We log it as an error and + // rely on later sync to reconcile local state.Also applies to: 194-196, 243-246
🤖 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 109 - 112, Update the comment blocks around the broadcast-first path to accurately reflect current behavior: state that the change derivation index is only committed after a successful broadcast (not merely post-build revalidation), and make the commentary for the check_result.is_relevant condition consistent across sites (treat !check_result.is_relevant as an expected transient, not an internal invariant error). Locate the comments near the "Peek at the next change address..." block and the places referencing check_result.is_relevant (the branches around the broadcast/revalidation flow) and rewrite them to say the index advance is deferred until broadcast succeeds and that non-relevance is transient/expected on retry, removing the erroneous "error!" framing.packages/rs-sdk/src/platform/dpns_usernames/mod.rs (1)
520-553: 💤 Low valueTests are thorough; one edge case worth adding for completeness.
The existing cases cover the main contract well. Consider adding a test for a name with an embedded dot where the last segment is
.dash(e.g."alice.bob.dash"), sincerfindmakesextract_dpns_labelreturn"alice.bob"there. The behavior is deterministic and the helper is private, but having it documented in the test suite would guard against surprising behaviour if a caller ever passes a subdomain-style string.✅ Suggested additional assertions
// Empty / suffix-only inputs normalize to an empty label. assert_eq!(normalize_dpns_label(""), ""); assert_eq!(normalize_dpns_label(".dash"), ""); assert_eq!(normalize_dpns_label(".DASH"), ""); + + // Subdomain-style input: only the last .dash segment is stripped. + assert_eq!(normalize_dpns_label("alice.bob.dash"), "a11ce.b0b"); }assert_eq!(extract_dpns_label(".dash"), ""); + // Multiple dots — only the last .dash is stripped. + assert_eq!(extract_dpns_label("alice.bob.dash"), "alice.bob"); + assert_eq!(extract_dpns_label("alice.dash.eth"), "alice.dash.eth"); }🤖 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 520 - 553, Add a unit test covering a name with an embedded dot where the final segment is ".dash" (e.g. "alice.bob.dash") to assert the deterministic behavior of extract_dpns_label and normalize_dpns_label; specifically, verify extract_dpns_label("alice.bob.dash") returns "alice.bob" and normalize_dpns_label("alice.bob.dash") returns the normalized form of "alice.bob" (so the embedded-dot part is preserved and only the trailing .dash is stripped case-insensitively). Place the assertions near the existing tests (test_extract_dpns_label and test_normalize_dpns_label_strips_dash_suffix_case_insensitively) to document and guard this subdomain-style input behavior.
🤖 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.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 109-112: Update the comment blocks around the broadcast-first path
to accurately reflect current behavior: state that the change derivation index
is only committed after a successful broadcast (not merely post-build
revalidation), and make the commentary for the check_result.is_relevant
condition consistent across sites (treat !check_result.is_relevant as an
expected transient, not an internal invariant error). Locate the comments near
the "Peek at the next change address..." block and the places referencing
check_result.is_relevant (the branches around the broadcast/revalidation flow)
and rewrite them to say the index advance is deferred until broadcast succeeds
and that non-relevance is transient/expected on retry, removing the erroneous
"error!" framing.
In `@packages/rs-sdk/src/platform/dpns_usernames/mod.rs`:
- Around line 520-553: Add a unit test covering a name with an embedded dot
where the final segment is ".dash" (e.g. "alice.bob.dash") to assert the
deterministic behavior of extract_dpns_label and normalize_dpns_label;
specifically, verify extract_dpns_label("alice.bob.dash") returns "alice.bob"
and normalize_dpns_label("alice.bob.dash") returns the normalized form of
"alice.bob" (so the embedded-dot part is preserved and only the trailing .dash
is stripped case-insensitively). Place the assertions near the existing tests
(test_extract_dpns_label and
test_normalize_dpns_label_strips_dash_suffix_case_insensitively) to document and
guard this subdomain-style input behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80ad6729-3aa7-4303-a2fb-8e139c62f7f9
📒 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
Two-fix PR (case-insensitive .dash DPNS suffix; broadcast-then-mark-spent reorder in CoreWallet::send_to_addresses) is structurally sound at 5d4a61b. The latest commit honestly downgrades the broadcast-wrapper test docstring to match what's actually exercised. Three carry-forwards remain: the new ConcurrentSpendConflict variant is still erased to ErrorUnknown across the FFI boundary; the send_to_addresses rollback contract has no automated coverage despite the broadcaster trait being trivially injectable; and deferring the change-address state commit past the broadcast opens a narrow crash window where a successful tx pays to an address the wallet did not yet record (recoverable since highest_generated isn't advanced). One performance nit on a defensive subset check.
Reviewed commit: 5d4a61b
🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🟡 suggestion: ConcurrentSpendConflict is flattened to ErrorUnknown across the FFI boundary
packages/rs-platform-wallet-ffi/src/error.rs (lines 157-161)
The blanket impl From<PlatformWalletError> for PlatformWalletFFIResult maps every variant — including the newly-introduced ConcurrentSpendConflict (packages/rs-platform-wallet/src/error.rs:63-64) — to PlatformWalletFFIResultCode::ErrorUnknown plus error.to_string(). The INTENTIONAL(CMT-002) note at wallet/core/broadcast.rs:151-153 says the typed variant exists for forward compatibility with cross-process concurrent-spend surfacing, but as wired today Swift consumers of core_wallet_send_to_addresses (e.g. ManagedCoreWallet.sendToAddresses) only see errorUnknown plus the localization-unsafe English message "...concurrent spend); retry", which they would have to substring-match to drive a retry — exactly the brittleness the typed variant was meant to eliminate. Special-case ConcurrentSpendConflict ahead of the blanket conversion (or replace the blanket impl with a per-variant match) and add a dedicated #[repr(C)] PlatformWalletFFIResultCode discriminant (e.g. ErrorRetryable / ErrorConcurrentSpendConflict). Because adding a new C-ABI variant requires header regeneration and Swift consumer rebuilds, it is best landed before any caller starts depending on the current ErrorUnknown shape.
🤖 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-ffi/src/error.rs`:
- [SUGGESTION] lines 157-161: ConcurrentSpendConflict is flattened to ErrorUnknown across the FFI boundary
The blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` maps every variant — including the newly-introduced `ConcurrentSpendConflict` (`packages/rs-platform-wallet/src/error.rs:63-64`) — to `PlatformWalletFFIResultCode::ErrorUnknown` plus `error.to_string()`. The `INTENTIONAL(CMT-002)` note at `wallet/core/broadcast.rs:151-153` says the typed variant exists for forward compatibility with cross-process concurrent-spend surfacing, but as wired today Swift consumers of `core_wallet_send_to_addresses` (e.g. `ManagedCoreWallet.sendToAddresses`) only see `errorUnknown` plus the localization-unsafe English message `"...concurrent spend); retry"`, which they would have to substring-match to drive a retry — exactly the brittleness the typed variant was meant to eliminate. Special-case `ConcurrentSpendConflict` ahead of the blanket conversion (or replace the blanket impl with a per-variant match) and add a dedicated `#[repr(C)]` `PlatformWalletFFIResultCode` discriminant (e.g. `ErrorRetryable` / `ErrorConcurrentSpendConflict`). Because adding a new C-ABI variant requires header regeneration and Swift consumer rebuilds, it is best landed before any caller starts depending on the current `ErrorUnknown` shape.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] lines 270-403: send_to_addresses rollback / re-selection contract is still uncovered by automated tests
The renamed test module now correctly admits at lines 272-277 that it pins only `broadcast_transaction`'s one-line pass-through — both tests call `wallet.broadcast_transaction(&tx)` directly, and `make_core_wallet` constructs an empty `WalletManager::new(Network::Testnet)` with no funded accounts. That is the integrity fix the prior over-claiming names needed, but it leaves the PR's central correctness claim entirely unpinned: broadcast failure must leave the spendable UTXO set untouched, and broadcast success must make those inputs non-spendable for the next caller via the post-broadcast `check_core_transaction(.., update_state=true, ..)` block at lines 239-241. `CoreWallet` is generic over `B: TransactionBroadcaster + ?Sized`, so the broadcaster seam already exists; the missing piece is a fixture wallet with a funded account driven through `WalletManager`. Two short async tests would lock this in: (1) inject `Err(...)` and assert the spendable-UTXO set, the change-derivation index, and the absence of any mempool registration are byte-identical before vs. after `send_to_addresses`; (2) inject `Ok(...)` and assert `account.spendable_utxos(..)` no longer contains the spent inputs and the change-index has advanced exactly once. Without coverage, only review prevents a future refactor (e.g. moving the post-broadcast registration block above the `?` on line 167) from silently re-introducing the original mark-spent-before-broadcast bug from #3466 with every existing test still green.
- [SUGGESTION] lines 109-236: Deferred change-address commit can pay a successful tx to an address the wallet never recorded
`send_to_addresses` peeks the next change address with `next_change_address(Some(&xpub), false)` at lines 113-115 and only persists it via `next_change_address(Some(&xpub), true)` at line 224, after `self.broadcast_transaction(&tx).await?` at line 167 returns success. In the underlying key-wallet implementation (verified in `key-wallet/src/managed_account/address_pool.rs:509-518`), `add_to_state = false` skips inserting the address into `addresses`, `address_index`, and `script_pubkey_index`, and does not advance `highest_generated`. The signed/broadcast transaction therefore pays change to an address the wallet does not yet track. If the process is cancelled, panics, or hits the warning paths at lines 223-235 / 255-262 between broadcast acceptance and a successful re-lock + commit, that change output is on-chain but unrecorded locally until the next send through the same handle (since `highest_generated` is unchanged, the same index is re-derived deterministically) or until gap-limit lookahead during sync rediscovers it. The trade-off vs. committing before broadcast is documented inline (avoiding gap-limit widening on broadcast rejection) and recovery is mechanically possible, so this is not a fund-loss risk — but the comment block at lines 207-210 should explicitly own the crash-window failure mode, and a small recovery test (post-crash next-send re-derives the same change address and persists it) would lock in the mitigation the design relies on.
| #[cfg(test)] | ||
| mod tests { | ||
| //! `broadcast_transaction` pass-through contract. | ||
| //! | ||
| //! Pins that the wrapper does not transform `Err` or modify the success | ||
| //! result — the `Txid` returned by the broadcaster is forwarded unchanged. | ||
| //! The higher-level `send_to_addresses` rollback contract (#3466) is not | ||
| //! covered here; pinning it would require live wallet fixtures. | ||
| use std::sync::atomic::{AtomicUsize, Ordering}; | ||
| use std::sync::Arc; | ||
|
|
||
| use async_trait::async_trait; | ||
| use dashcore::consensus::deserialize; | ||
| use dashcore::{Transaction, Txid}; | ||
| use tokio::sync::RwLock; | ||
|
|
||
| use crate::broadcaster::TransactionBroadcaster; | ||
| use crate::wallet::core::balance::WalletBalance; | ||
| use crate::wallet::core::CoreWallet; | ||
| use crate::PlatformWalletError; | ||
| use key_wallet::Network; | ||
| use key_wallet_manager::WalletManager; | ||
|
|
||
| /// Records every call and returns a canned outcome. | ||
| struct MockBroadcaster { | ||
| outcome: BroadcastOutcome, | ||
| calls: AtomicUsize, | ||
| } | ||
|
|
||
| enum BroadcastOutcome { | ||
| Ok(Txid), | ||
| Err(String), | ||
| } | ||
|
|
||
| impl MockBroadcaster { | ||
| fn new(outcome: BroadcastOutcome) -> Self { | ||
| Self { | ||
| outcome, | ||
| calls: AtomicUsize::new(0), | ||
| } | ||
| } | ||
|
|
||
| fn call_count(&self) -> usize { | ||
| self.calls.load(Ordering::SeqCst) | ||
| } | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl TransactionBroadcaster for MockBroadcaster { | ||
| async fn broadcast(&self, _transaction: &Transaction) -> Result<Txid, PlatformWalletError> { | ||
| self.calls.fetch_add(1, Ordering::SeqCst); | ||
| match &self.outcome { | ||
| BroadcastOutcome::Ok(txid) => Ok(*txid), | ||
| BroadcastOutcome::Err(msg) => { | ||
| Err(PlatformWalletError::TransactionBroadcast(msg.clone())) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Minimal serialized tx (1 input, 1 output, 0 value) — only the | ||
| /// broadcaster's Err/Ok branch matters here, not the shape. | ||
| fn dummy_transaction() -> Transaction { | ||
| let bytes = hex::decode( | ||
| "010000000100000000000000000000000000000000000000000000000000000000000000\ | ||
| 00ffffffff00ffffffff0100000000000000000000000000", | ||
| ) | ||
| .expect("valid hex"); | ||
| deserialize(&bytes).expect("deserializable tx") | ||
| } | ||
|
|
||
| fn make_core_wallet<B: TransactionBroadcaster + ?Sized>(broadcaster: Arc<B>) -> CoreWallet<B> { | ||
| let sdk = Arc::new( | ||
| dash_sdk::SdkBuilder::new_mock() | ||
| .build() | ||
| .expect("mock sdk build"), | ||
| ); | ||
| let wallet_manager = Arc::new(RwLock::new(WalletManager::new(Network::Testnet))); | ||
| CoreWallet::new( | ||
| sdk, | ||
| wallet_manager, | ||
| [0u8; 32], | ||
| broadcaster, | ||
| Arc::new(WalletBalance::new()), | ||
| ) | ||
| } | ||
|
|
||
| /// `broadcast_transaction` forwards a broadcaster `Err` to the caller | ||
| /// without transformation. | ||
| #[tokio::test] | ||
| async fn broadcast_transaction_passes_through_err_unchanged() { | ||
| let broadcaster = Arc::new(MockBroadcaster::new(BroadcastOutcome::Err( | ||
| "simulated network rejection".to_string(), | ||
| ))); | ||
| let wallet = make_core_wallet(Arc::clone(&broadcaster)); | ||
| let tx = dummy_transaction(); | ||
|
|
||
| let result = wallet.broadcast_transaction(&tx).await; | ||
|
|
||
| assert!( | ||
| matches!(result, Err(PlatformWalletError::TransactionBroadcast(_))), | ||
| "expected broadcast Err to propagate, got {:?}", | ||
| result | ||
| ); | ||
| assert_eq!( | ||
| broadcaster.call_count(), | ||
| 1, | ||
| "broadcaster must be called exactly once on a failed broadcast" | ||
| ); | ||
| } | ||
|
|
||
| /// `broadcast_transaction` forwards the broadcaster's `Txid` to the | ||
| /// caller without transformation. | ||
| #[tokio::test] | ||
| async fn broadcast_transaction_passes_through_ok_unchanged() { | ||
| let expected_txid = dummy_transaction().txid(); | ||
| let broadcaster = Arc::new(MockBroadcaster::new(BroadcastOutcome::Ok(expected_txid))); | ||
| let wallet = make_core_wallet(Arc::clone(&broadcaster)); | ||
| let tx = dummy_transaction(); | ||
|
|
||
| let result = wallet.broadcast_transaction(&tx).await; | ||
|
|
||
| assert_eq!( | ||
| result.expect("broadcast Ok"), | ||
| expected_txid, | ||
| "broadcast_transaction must pass the broadcaster's Txid through unchanged" | ||
| ); | ||
| assert_eq!( | ||
| broadcaster.call_count(), | ||
| 1, | ||
| "broadcaster must be called exactly once on a successful broadcast" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: send_to_addresses rollback / re-selection contract is still uncovered by automated tests
The renamed test module now correctly admits at lines 272-277 that it pins only broadcast_transaction's one-line pass-through — both tests call wallet.broadcast_transaction(&tx) directly, and make_core_wallet constructs an empty WalletManager::new(Network::Testnet) with no funded accounts. That is the integrity fix the prior over-claiming names needed, but it leaves the PR's central correctness claim entirely unpinned: broadcast failure must leave the spendable UTXO set untouched, and broadcast success must make those inputs non-spendable for the next caller via the post-broadcast check_core_transaction(.., update_state=true, ..) block at lines 239-241. CoreWallet is generic over B: TransactionBroadcaster + ?Sized, so the broadcaster seam already exists; the missing piece is a fixture wallet with a funded account driven through WalletManager. Two short async tests would lock this in: (1) inject Err(...) and assert the spendable-UTXO set, the change-derivation index, and the absence of any mempool registration are byte-identical before vs. after send_to_addresses; (2) inject Ok(...) and assert account.spendable_utxos(..) no longer contains the spent inputs and the change-index has advanced exactly once. Without coverage, only review prevents a future refactor (e.g. moving the post-broadcast registration block above the ? on line 167) from silently re-introducing the original mark-spent-before-broadcast bug from #3466 with every existing test still green.
source: ['claude', 'codex']
🤖 Fix this 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 270-403: send_to_addresses rollback / re-selection contract is still uncovered by automated tests
The renamed test module now correctly admits at lines 272-277 that it pins only `broadcast_transaction`'s one-line pass-through — both tests call `wallet.broadcast_transaction(&tx)` directly, and `make_core_wallet` constructs an empty `WalletManager::new(Network::Testnet)` with no funded accounts. That is the integrity fix the prior over-claiming names needed, but it leaves the PR's central correctness claim entirely unpinned: broadcast failure must leave the spendable UTXO set untouched, and broadcast success must make those inputs non-spendable for the next caller via the post-broadcast `check_core_transaction(.., update_state=true, ..)` block at lines 239-241. `CoreWallet` is generic over `B: TransactionBroadcaster + ?Sized`, so the broadcaster seam already exists; the missing piece is a fixture wallet with a funded account driven through `WalletManager`. Two short async tests would lock this in: (1) inject `Err(...)` and assert the spendable-UTXO set, the change-derivation index, and the absence of any mempool registration are byte-identical before vs. after `send_to_addresses`; (2) inject `Ok(...)` and assert `account.spendable_utxos(..)` no longer contains the spent inputs and the change-index has advanced exactly once. Without coverage, only review prevents a future refactor (e.g. moving the post-broadcast registration block above the `?` on line 167) from silently re-introducing the original mark-spent-before-broadcast bug from #3466 with every existing test still green.
| @@ -127,12 +134,270 @@ impl<B: TransactionBroadcaster + ?Sized> CoreWallet<B> { | |||
| ) | |||
| .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; | |||
|
|
|||
| builder | |||
| let tx = builder | |||
| .build() | |||
| .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))? | |||
| .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; | |||
|
|
|||
| // Defense-in-depth: by builder contract `tx.input` outpoints are | |||
| // a subset of the height-aware `spendable` set we passed to | |||
| // `select_inputs`, so this branch is unreachable in normal | |||
| // operation. Marking inputs spent is deferred to after broadcast | |||
| // (see #3466) regardless. | |||
| let selected: BTreeSet<OutPoint> = | |||
| tx.input.iter().map(|txin| txin.previous_output).collect(); | |||
| let spendable_outpoints: BTreeSet<OutPoint> = | |||
| spendable.iter().map(|utxo| utxo.outpoint).collect(); | |||
| if !selected.is_subset(&spendable_outpoints) { | |||
| // INTENTIONAL(CMT-002): typed variant kept user-retryable for | |||
| // forward compatibility with cross-process concurrent-spend | |||
| // surfacing — even though today only builder regression hits. | |||
| return Err(PlatformWalletError::ConcurrentSpendConflict); | |||
| } | |||
|
|
|||
| (tx, xpub) | |||
| }; | |||
|
|
|||
| // Broadcast first; if the network rejects we leave wallet state | |||
| // untouched so the caller can retry without manual sync repair. | |||
| // This is intentional even if the remote accepted the transaction | |||
| // but the broadcast path returned an error: in that ambiguous case | |||
| // later attempts may reuse the same inputs locally, but the network | |||
| // rejects the duplicate spend instead of us marking UTXOs spent for | |||
| // a transaction that might not have propagated. | |||
| self.broadcast_transaction(&tx).await?; | |||
|
|
|||
| // Now that the tx is in flight, register it as a mempool transaction | |||
| // so subsequent callers see the inputs as spent and don't reselect | |||
| // them. The trade-off is that two callers racing between the lock | |||
| // drop above and the broadcast can both pick the same UTXOs; the | |||
| // network resolves that race exactly as it does on `v3.1-dev` | |||
| // today, but neither caller corrupts local state on a transient | |||
| // broadcast failure. | |||
| // | |||
| // Broadcast-first semantics: by the time we get here the network has | |||
| // already accepted the transaction, so the two warning paths below | |||
| // intentionally do NOT convert into a post-success `Err`. They | |||
| // simply mean local wallet state did not get updated to reflect the | |||
| // mempool spend / change output. Recovery in both cases: | |||
| // | |||
| // * The next `send_to_addresses` from the same handle may reselect | |||
| // the same UTXOs because they still look spendable locally. That | |||
| // follow-up transaction will be rejected by the network as a | |||
| // duplicate spend (the broadcaster surfaces that as an error to | |||
| // the caller), so funds are never double-spent on-chain. | |||
| // * Once mempool/block sync catches up, the wallet will see the | |||
| // original transaction and reconcile its UTXO set, after which | |||
| // subsequent sends pick up the correct change outputs. | |||
| // | |||
| // The two cases differ in what they imply: | |||
| // | |||
| // * `!check_result.is_relevant` is the expected transient: the | |||
| // wallet just hasn't ingested the tx yet (or some derivation | |||
| // path/script is unrecognised), and a later sync will fix it. | |||
| // * The `else` branch (wallet missing in the manager) is NOT a | |||
| // normal transient — the broadcast succeeded against a | |||
| // `CoreWallet` handle whose underlying wallet entry is gone | |||
| // from the manager. That is a broken/inconsistent local handle | |||
| // and the warning exists so operators can spot it; future | |||
| // sends through the same handle will keep failing the lookup | |||
| // above and surface a clean `WalletNotFound` error. | |||
| { | |||
| let mut wm = self.wallet_manager.write().await; | |||
| if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&self.wallet_id) { | |||
| // Broadcast succeeded — commit the change-address advance now | |||
| // so a future send picks up a fresh index. Doing this before | |||
| // the broadcast would burn a derivation index on a network | |||
| // rejection, widening the gap-limit window on retry. | |||
| let change_account = match account_type { | |||
| StandardAccountType::BIP44Account => info | |||
| .core_wallet | |||
| .accounts | |||
| .standard_bip44_accounts | |||
| .get_mut(&account_index), | |||
| StandardAccountType::BIP32Account => info | |||
| .core_wallet | |||
| .accounts | |||
| .standard_bip32_accounts | |||
| .get_mut(&account_index), | |||
| }; | |||
| if let Some(change_account) = change_account { | |||
| if let Err(e) = change_account.next_change_address(Some(&xpub), true) { | |||
| // Broadcast already succeeded; surface as a warning | |||
| // rather than an error so the caller still sees the | |||
| // tx hash. A later sync reconciles the index. | |||
| tracing::warn!( | |||
| target: "platform_wallet::broadcast", | |||
| event = "post_broadcast_change_index_advance_failed", | |||
| txid = %tx.txid(), | |||
| wallet_id = %hex::encode(self.wallet_id), | |||
| error = %e, | |||
| "failed to advance change-address index after successful broadcast" | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: Deferred change-address commit can pay a successful tx to an address the wallet never recorded
send_to_addresses peeks the next change address with next_change_address(Some(&xpub), false) at lines 113-115 and only persists it via next_change_address(Some(&xpub), true) at line 224, after self.broadcast_transaction(&tx).await? at line 167 returns success. In the underlying key-wallet implementation (verified in key-wallet/src/managed_account/address_pool.rs:509-518), add_to_state = false skips inserting the address into addresses, address_index, and script_pubkey_index, and does not advance highest_generated. The signed/broadcast transaction therefore pays change to an address the wallet does not yet track. If the process is cancelled, panics, or hits the warning paths at lines 223-235 / 255-262 between broadcast acceptance and a successful re-lock + commit, that change output is on-chain but unrecorded locally until the next send through the same handle (since highest_generated is unchanged, the same index is re-derived deterministically) or until gap-limit lookahead during sync rediscovers it. The trade-off vs. committing before broadcast is documented inline (avoiding gap-limit widening on broadcast rejection) and recovery is mechanically possible, so this is not a fund-loss risk — but the comment block at lines 207-210 should explicitly own the crash-window failure mode, and a small recovery test (post-crash next-send re-derives the same change address and persists it) would lock in the mitigation the design relies on.
source: ['codex']
🤖 Fix this 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 109-236: Deferred change-address commit can pay a successful tx to an address the wallet never recorded
`send_to_addresses` peeks the next change address with `next_change_address(Some(&xpub), false)` at lines 113-115 and only persists it via `next_change_address(Some(&xpub), true)` at line 224, after `self.broadcast_transaction(&tx).await?` at line 167 returns success. In the underlying key-wallet implementation (verified in `key-wallet/src/managed_account/address_pool.rs:509-518`), `add_to_state = false` skips inserting the address into `addresses`, `address_index`, and `script_pubkey_index`, and does not advance `highest_generated`. The signed/broadcast transaction therefore pays change to an address the wallet does not yet track. If the process is cancelled, panics, or hits the warning paths at lines 223-235 / 255-262 between broadcast acceptance and a successful re-lock + commit, that change output is on-chain but unrecorded locally until the next send through the same handle (since `highest_generated` is unchanged, the same index is re-derived deterministically) or until gap-limit lookahead during sync rediscovers it. The trade-off vs. committing before broadcast is documented inline (avoiding gap-limit widening on broadcast rejection) and recovery is mechanically possible, so this is not a fund-loss risk — but the comment block at lines 207-210 should explicitly own the crash-window failure mode, and a small recovery test (post-crash next-send re-derives the same change address and persists it) would lock in the mitigation the design relies on.
| // Defense-in-depth: by builder contract `tx.input` outpoints are | ||
| // a subset of the height-aware `spendable` set we passed to | ||
| // `select_inputs`, so this branch is unreachable in normal | ||
| // operation. Marking inputs spent is deferred to after broadcast | ||
| // (see #3466) regardless. | ||
| let selected: BTreeSet<OutPoint> = | ||
| tx.input.iter().map(|txin| txin.previous_output).collect(); | ||
| let spendable_outpoints: BTreeSet<OutPoint> = | ||
| spendable.iter().map(|utxo| utxo.outpoint).collect(); | ||
| if !selected.is_subset(&spendable_outpoints) { | ||
| // INTENTIONAL(CMT-002): typed variant kept user-retryable for | ||
| // forward compatibility with cross-process concurrent-spend | ||
| // surfacing — even though today only builder regression hits. | ||
| return Err(PlatformWalletError::ConcurrentSpendConflict); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Two BTreeSet allocations on every successful build for an unreachable check
Per the comment block at lines 141-145, select_inputs(&spendable, ...) is the only input source the builder receives and the wallet_manager.write().await guard from line 50 is held continuously through builder.build() and the subset check, so by builder contract selected.is_subset(&spendable_outpoints) is always true under normal operation — the only path to the Err on line 154 is an upstream key-wallet builder regression, not the cross-process concurrent spend the variant name and Display text advertise. Yet the two BTreeSet<OutPoint> constructions on lines 146-149 (O(n log n) in spendable-UTXO count) run on every happy-path send. Cheaper alternatives that keep the regression catch: (a) downgrade to debug_assert! so the cost is paid only in test/dev builds; or (b) iterate tx.input once and look up each previous_output in a single pre-built HashSet of spendable outpoints, eliminating the second set construction. Non-blocking — outputs.len() is typically small — but worth doing alongside any future hot-path tightening, especially since today this branch fires only on a deterministic builder bug and would surface to FFI callers as a (currently mis-typed) retryable error.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Reviewed the delta from 391768ce to 5d4a61bf.
The new commits move the change-address index commit to after successful broadcast and rename the broadcast wrapper tests so they no longer imply full send_to_addresses/UTXO-state coverage. I don’t see any new blockers in this delta.
Carry-forward non-blocking note: the broadcast recovery comment still describes !check_result.is_relevant as an expected transient, while the code below correctly treats an own-built transaction not being recognized by the wallet checker as an internal invariant violation (error!). That comment should be made consistent with the code, but it doesn’t block the PR.
Validation on current head:
cargo fmt -p platform-wallet -p dash-sdk -- --checkgit diff --check origin/v3.1-dev...HEADcargo test -p platform-wallet --lib wallet::core::broadcast::tests::— 2 passedcargo test -p dash-sdk --lib platform::dpns_usernames::— 5 passed / 0 failed / 6 ignoredcargo check -p platform-wallet --libcargo clippy -p platform-wallet --lib -- -D warnings
Reviewed commit: 5d4a61b
3 participants
Summary
Re-implementation of PR #3466 against the current
v3.1-dev. The original PR's base branch (feat/platform-wallet) is defunct after the platform-wallet refactor merge; rather than rebase that PR through a tangle of merge commits, this is a fresh start.Both fixes were originally backported from dash-evo-tool. Triage at the time of authoring confirmed both bugs are still present on
v3.1-dev.Fix 1 — Case-insensitive
.dashsuffix in DPNS resolutionSdk::resolve_dpns_namestripped the.dashsuffix using exact-match. Inputs like"Alice.DASH"fell into the else branch — the entire string was treated as a label and DPNS lookup missed.Verbatim port from PR #3466 fix 1 / dash-evo-tool PR #810.
Fix 2 — UTXO double-spend prevention (Option A: mark-spent after broadcast)
CoreWallet::send_to_addresseshad a TOCTOU window between dropping the write lock (after build/select/sign) and broadcasting. Concurrent callers could both select the same UTXOs; the loser got an opaque network rejection.Adapted from PR #3466's original
send_transactionpatch because the wallet was rewritten aroundTransactionBuilderpost-#3466 — same bug, different call site, same optimistic-validation cure.Addresses thepastaclaw's blocking comment from PR #3466 about state corruption on broadcast failure. We deliberately mark UTXOs spent only after successful broadcast: re-validate selected outpoints under the write lock, drop the lock, broadcast, then re-acquire the lock and call
check_core_transaction(.., Mempool, .., update_state=true)only on broadcast success. A transient broadcast failure now leaves wallet state untouched instead of falsely marking UTXOs spent until manual sync.Trade-off documented in the code comment: two callers racing between drop-lock and broadcast can both pick the same UTXOs — the network resolves that race exactly as it does on
v3.1-devtoday, but at least neither caller corrupts local state on transient failure.Companion
Supersedes #3466 (which was against the defunct
feat/platform-walletbase). Old PR will be closed as superseded once this opens.Test plan
cargo fmt --checkgreencargo check -p platform-wallet -p dash-sdk --testsgreencargo test -p dash-sdk --lib(117 passed)cargo test -p platform-wallet --lib(115 passed)cargo clippy -p dash-sdk -p platform-wallet --tests -- -D warnings— clean for files this PR touches; 3 pre-existing warnings inpackages/rs-platform-wallet/src/manager/accessors.rs(manual_unwrap_or_default,unnecessary_cast,redundant_closure) are already onv3.1-devand need a separate cleanup PR🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Bug Fixes
.dashsuffix in any letter case and rejects empty normalized labels.Improvements