From fe6d7c1507a2c419f5c72ccf480131ac008ce2ea Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 5 May 2026 11:02:25 +0200 Subject: [PATCH 01/33] fix(rs-sdk): case-insensitive .dash suffix in resolve_dpns_name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- packages/rs-sdk/src/platform/dpns_usernames/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/rs-sdk/src/platform/dpns_usernames/mod.rs b/packages/rs-sdk/src/platform/dpns_usernames/mod.rs index 58c1b4a9792..e38a984238e 100644 --- a/packages/rs-sdk/src/platform/dpns_usernames/mod.rs +++ b/packages/rs-sdk/src/platform/dpns_usernames/mod.rs @@ -426,8 +426,8 @@ impl Sdk { // Handle both "alice" and "alice.dash" formats let label = if let Some(dot_pos) = name.rfind('.') { let (label_part, suffix) = name.split_at(dot_pos); - // Only strip the suffix if it's exactly ".dash" - if suffix == ".dash" { + // 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 From 26f13d96537d9b7c401d921780f09ae5b11cebc4 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 5 May 2026 11:02:43 +0200 Subject: [PATCH 02/33] fix(rs-platform-wallet): prevent UTXO double-spend race in send_to_addresses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent --- .../src/wallet/core/broadcast.rs | 61 +++++++++++++++++-- 1 file changed, 57 insertions(+), 4 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 10578a7a682..f3c9f0ae525 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -1,5 +1,9 @@ -use dashcore::{Address as DashAddress, Transaction}; +use std::collections::BTreeSet; + +use dashcore::{Address as DashAddress, OutPoint, Transaction}; use key_wallet::account::account_type::StandardAccountType; +use key_wallet::transaction_checking::{TransactionContext, WalletTransactionChecker}; +use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use crate::broadcaster::TransactionBroadcaster; use crate::{CoreWallet, PlatformWalletError}; @@ -35,7 +39,6 @@ impl CoreWallet { ) -> Result { use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy; use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder; - use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; if outputs.is_empty() { return Err(PlatformWalletError::TransactionBuild( @@ -127,12 +130,62 @@ impl CoreWallet { ) .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()))?; + + // Re-validate the selected outpoints are still spendable while + // we still hold the write lock. The lock makes our build atomic + // against other callers on this handle, but external mempool / + // block events processed before we acquired the lock may have + // invalidated UTXOs that were still in the spendable set when + // `select_inputs` ran. + // + // We deliberately do NOT mark the inputs as spent here — that + // happens after a successful broadcast (see #3466 review). A + // failed broadcast must not leave UTXOs falsely marked spent. + let selected: BTreeSet = + tx.input.iter().map(|txin| txin.previous_output).collect(); + let still_spendable: BTreeSet = info + .get_spendable_utxos() + .into_iter() + .map(|utxo| utxo.outpoint) + .collect(); + if !selected.is_subset(&still_spendable) { + return Err(PlatformWalletError::TransactionBuild( + "Selected UTXOs are no longer available (concurrent transaction). \ + Please retry." + .to_string(), + )); + } + + tx }; + // Broadcast first; if the network rejects we leave wallet state + // untouched so the caller can retry without manual sync repair. 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. + { + 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; + } + Ok(tx) } } From 1bd306a05736f4ab84c56f59ee51e6e118dc7ef2 Mon Sep 17 00:00:00 2001 From: Lil Claw Date: Wed, 6 May 2026 05:21:22 -0500 Subject: [PATCH 03/33] fix: improve platform wallet UTXO checks and DPNS parsing (#3595) Co-authored-by: PastaClaw --- .../src/wallet/core/broadcast.rs | 87 +++++++++++------ .../rs-sdk/src/platform/dpns_usernames/mod.rs | 97 ++++++++++++++----- 2 files changed, 133 insertions(+), 51 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index f3c9f0ae525..47193fcd34d 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -134,28 +134,18 @@ impl CoreWallet { .build() .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; - // Re-validate the selected outpoints are still spendable while - // we still hold the write lock. The lock makes our build atomic - // against other callers on this handle, but external mempool / - // block events processed before we acquired the lock may have - // invalidated UTXOs that were still in the spendable set when - // `select_inputs` ran. - // - // We deliberately do NOT mark the inputs as spent here — that - // happens after a successful broadcast (see #3466 review). A - // failed broadcast must not leave UTXOs falsely marked spent. + // Sanity-check that the builder only selected outpoints from + // the same height-aware spendable set we handed to input + // selection. We deliberately do NOT mark the inputs as spent here + // — that happens after a successful broadcast (see #3466 review). + // A failed broadcast must not leave UTXOs falsely marked spent. let selected: BTreeSet = tx.input.iter().map(|txin| txin.previous_output).collect(); - let still_spendable: BTreeSet = info - .get_spendable_utxos() - .into_iter() - .map(|utxo| utxo.outpoint) - .collect(); - if !selected.is_subset(&still_spendable) { + let spendable_outpoints: BTreeSet = + spendable.iter().map(|utxo| utxo.outpoint).collect(); + if !selected.is_subset(&spendable_outpoints) { return Err(PlatformWalletError::TransactionBuild( - "Selected UTXOs are no longer available (concurrent transaction). \ - Please retry." - .to_string(), + "Transaction builder selected an unavailable UTXO. Please retry.".to_string(), )); } @@ -164,6 +154,11 @@ impl CoreWallet { // 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 @@ -173,17 +168,53 @@ impl CoreWallet { // 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; - 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) { + let check_result = info + .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) + .await; + if !check_result.is_relevant { + tracing::warn!( + txid = %tx.txid(), + "broadcast transaction was not relevant during post-broadcast wallet registration" + ); + } + } else { + tracing::warn!( + wallet_id = %hex::encode(self.wallet_id), + txid = %tx.txid(), + "wallet missing during post-broadcast transaction registration" + ); + } } Ok(tx) diff --git a/packages/rs-sdk/src/platform/dpns_usernames/mod.rs b/packages/rs-sdk/src/platform/dpns_usernames/mod.rs index e38a984238e..3f00fc0fa45 100644 --- a/packages/rs-sdk/src/platform/dpns_usernames/mod.rs +++ b/packages/rs-sdk/src/platform/dpns_usernames/mod.rs @@ -35,6 +35,27 @@ pub fn convert_to_homograph_safe_chars(input: &str) -> String { .collect() } +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 +} + +/// Strip an optional case-insensitive `.dash` suffix and apply DPNS +/// homograph-safe normalization, producing a value suitable for matching +/// against the `normalizedLabel` field of `domain` documents. +/// +/// Accepts either a bare label (e.g. `"alice"`) or a full DPNS name +/// (e.g. `"alice.dash"`, `"Alice.DASH"`) and returns the normalized label +/// (e.g. `"a11ce"`). +fn normalize_dpns_label(input: &str) -> String { + convert_to_homograph_safe_chars(extract_dpns_label(input)) +} + /// Check if a username is valid according to DPNS rules /// /// A username is valid if: @@ -365,19 +386,31 @@ impl Sdk { /// /// # Arguments /// - /// * `label` - The username label to check (e.g., "alice") + /// * `name` - The username label (e.g., "alice") or full DPNS name + /// (e.g., "alice.dash"). The `.dash` suffix is matched + /// case-insensitively and stripped before normalization, mirroring + /// [`Sdk::resolve_dpns_name`]. /// /// # Returns /// /// Returns `true` if the name is available, `false` if it's taken - pub async fn is_dpns_name_available(&self, label: &str) -> Result { + pub async fn is_dpns_name_available(&self, name: &str) -> Result { use crate::platform::documents::document_query::DocumentQuery; use drive::query::WhereClause; use drive::query::WhereOperator; - let dpns_contract = self.fetch_dpns_contract().await?; + let normalized_label = normalize_dpns_label(name); + + // An empty normalized label (e.g. `""`, `".dash"`, `".DASH"`) is not + // a registrable DPNS name, so report it as unavailable rather than + // doing a network round-trip that would query for + // `normalizedLabel == ""`. This mirrors the early-return guard in + // `resolve_dpns_name` so the two APIs agree on malformed input. + if normalized_label.is_empty() { + return Ok(false); + } - let normalized_label = convert_to_homograph_safe_chars(label); + let dpns_contract = self.fetch_dpns_contract().await?; // Query for existing domain with this label let query = DocumentQuery { @@ -422,29 +455,13 @@ impl Sdk { let dpns_contract = self.fetch_dpns_contract().await?; - // Extract label from full name if needed - // Handle both "alice" and "alice.dash" formats - 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 normalized_label = normalize_dpns_label(name); - // Validate the label before proceeding - if label.is_empty() { + // Validate the normalized label before proceeding + if normalized_label.is_empty() { return Ok(None); } - let normalized_label = convert_to_homograph_safe_chars(label); - // Query for domain with this label let query = DocumentQuery { data_contract: dpns_contract, @@ -499,6 +516,40 @@ mod tests { assert_eq!(convert_to_homograph_safe_chars("test123"), "test123"); } + #[test] + fn test_normalize_dpns_label_strips_dash_suffix_case_insensitively() { + // Bare label and full name normalize to the same value, regardless + // of the case of the .dash suffix. This is the contract that + // `is_dpns_name_available` and `resolve_dpns_name` share so that + // queries against `normalizedLabel` agree. + let expected = "a11ce"; + assert_eq!(normalize_dpns_label("alice"), expected); + assert_eq!(normalize_dpns_label("alice.dash"), expected); + assert_eq!(normalize_dpns_label("alice.DASH"), expected); + assert_eq!(normalize_dpns_label("Alice.DaSh"), expected); + assert_eq!(normalize_dpns_label("ALICE.DASH"), expected); + + // Non-.dash suffixes are not stripped (they are treated as part of + // the label and normalized whole). + assert_eq!(normalize_dpns_label("alice.eth"), "a11ce.eth"); + + // 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"), ""); + } + + #[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("alice.eth"), "alice.eth"); + assert_eq!(extract_dpns_label(".dash"), ""); + } + #[test] fn test_is_valid_username() { // Valid usernames From 23d8943c38d8cb01a46fc1c38d3f492e94d37f14 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 12:38:29 +0200 Subject: [PATCH 04/33] fix(rs-platform-wallet): defer change-address advance until after revalidation (CMT-007) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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: https://github.com/dashpay/platform/pull/3585#discussion_r3194660629 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/wallet/core/broadcast.rs | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 47193fcd34d..2c984a4c8d5 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -106,8 +106,12 @@ impl CoreWallet { )) })?; + // 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. let change_addr = change_account - .next_change_address(Some(&xpub), true) + .next_change_address(Some(&xpub), false) .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; builder = builder.set_change_address(change_addr); @@ -149,6 +153,32 @@ impl CoreWallet { )); } + // Revalidation passed; now commit the change-address advance so + // the next send picks up the next index. Re-borrow the managed + // account because `select_inputs` above borrowed + // `info.core_wallet.accounts` and ended the earlier reborrow. + 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 + )) + })?; + change_account + .next_change_address(Some(&xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + tx }; From a3a5d9653c17b304ed55c72ad3d8015630fd277a Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 12:40:08 +0200 Subject: [PATCH 05/33] fix(rs-platform-wallet): typed ConcurrentSpendConflict variant for retryable UTXO race (CMT-008) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/dashpay/platform/pull/3585#discussion_r3194660635 Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/rs-platform-wallet/src/error.rs | 3 +++ packages/rs-platform-wallet/src/wallet/core/broadcast.rs | 4 +--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 006e9b01331..ed724b1f161 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -60,6 +60,9 @@ pub enum PlatformWalletError { #[error("Transaction building failed: {0}")] TransactionBuild(String), + #[error("Transaction builder selected an unavailable UTXO (concurrent spend); retry")] + ConcurrentSpendConflict, + #[error("Asset lock proof waiting failed: {0}")] AssetLockProofWait(String), diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 2c984a4c8d5..a7649971461 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -148,9 +148,7 @@ impl CoreWallet { let spendable_outpoints: BTreeSet = spendable.iter().map(|utxo| utxo.outpoint).collect(); if !selected.is_subset(&spendable_outpoints) { - return Err(PlatformWalletError::TransactionBuild( - "Transaction builder selected an unavailable UTXO. Please retry.".to_string(), - )); + return Err(PlatformWalletError::ConcurrentSpendConflict); } // Revalidation passed; now commit the change-address advance so From 41c9493632e57cfde87246bccb81288c4b4b31cc Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 13:31:35 +0200 Subject: [PATCH 06/33] fix(rs-sdk): skip DPNS contract fetch when label is empty (CMT-001) 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) --- packages/rs-sdk/src/platform/dpns_usernames/mod.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/rs-sdk/src/platform/dpns_usernames/mod.rs b/packages/rs-sdk/src/platform/dpns_usernames/mod.rs index 3f00fc0fa45..7a6f56959c2 100644 --- a/packages/rs-sdk/src/platform/dpns_usernames/mod.rs +++ b/packages/rs-sdk/src/platform/dpns_usernames/mod.rs @@ -453,15 +453,18 @@ impl Sdk { use drive::query::WhereClause; use drive::query::WhereOperator; - let dpns_contract = self.fetch_dpns_contract().await?; - let normalized_label = normalize_dpns_label(name); - // Validate the normalized label before proceeding + // An empty normalized label (e.g. `""`, `".dash"`, `".DASH"`) cannot + // resolve to a registered identity. Skip the contract fetch and + // return early so the API mirrors `is_dpns_name_available` and + // doesn't perform a wasted RPC round-trip on malformed input. if normalized_label.is_empty() { return Ok(None); } + let dpns_contract = self.fetch_dpns_contract().await?; + // Query for domain with this label let query = DocumentQuery { data_contract: dpns_contract, From 2c7e22a072b70daa0aefb35162c6dffc333ea629 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 13:32:11 +0200 Subject: [PATCH 07/33] docs(rs-platform-wallet): rewrite revalidation comment to match builder 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) --- .../src/wallet/core/broadcast.rs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index a7649971461..873c6023c6a 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -138,16 +138,26 @@ impl CoreWallet { .build() .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; - // Sanity-check that the builder only selected outpoints from - // the same height-aware spendable set we handed to input - // selection. We deliberately do NOT mark the inputs as spent here - // — that happens after a successful broadcast (see #3466 review). - // A failed broadcast must not leave UTXOs falsely marked spent. + // `select_inputs` is the only source of UTXOs for this builder, + // so `tx.input` outpoints must be a subset of the height-aware + // `spendable` set by the builder's contract. The check below is + // a defense-in-depth runtime guard for builder regressions; + // under normal operation this branch is unreachable. Inputs are + // not marked spent here either way — that happens after a + // successful broadcast (see #3466 review): a failed broadcast + // must not leave UTXOs falsely marked spent. let selected: BTreeSet = tx.input.iter().map(|txin| txin.previous_output).collect(); let spendable_outpoints: BTreeSet = spendable.iter().map(|utxo| utxo.outpoint).collect(); if !selected.is_subset(&spendable_outpoints) { + // INTENTIONAL(CMT-002): The `ConcurrentSpendConflict` variant + // is named and framed as user-retryable for forward + // compatibility. The current code path is only reachable on + // a builder-internal regression, but the typed variant is + // preserved so future work that surfaces real concurrent-spend + // conflicts (e.g. from cross-process wallets) can route + // through the same handler without an API churn. return Err(PlatformWalletError::ConcurrentSpendConflict); } From 97d153201c21edc632ea0d9daca4517aaa633305 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 13:32:31 +0200 Subject: [PATCH 08/33] fix(rs-platform-wallet): structured event for post-broadcast !is_relevant own-built tx (CMT-004, CMT-005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/wallet/core/broadcast.rs | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 873c6023c6a..9da16918f9a 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -241,12 +241,32 @@ impl CoreWallet { .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) .await; if !check_result.is_relevant { - tracing::warn!( + // CMT-004: The wallet just built and signed this + // transaction from its own spendable inputs, so a + // `!is_relevant` post-broadcast check is an + // internal-invariant violation, not a transient. Emit a + // structured `error!` event with stable field names so + // operators can alert on it independent of the message + // text. We still return `Ok(tx)`: broadcast already + // succeeded, and rolling back here would mislead the + // caller into thinking the network rejected the tx. + tracing::error!( + target: "platform_wallet::broadcast", + event = "post_broadcast_unrelated_to_own_wallet", txid = %tx.txid(), - "broadcast transaction was not relevant during post-broadcast wallet registration" + wallet_id = %hex::encode(self.wallet_id), + "Internal invariant violation: own-built broadcast not recognized by post-broadcast check" ); } } else { + // INTENTIONAL(CMT-005): The wallet-missing branch indicates + // the wallet entry was removed from the manager between the + // lock drop and re-acquisition. Broadcast already succeeded, + // so converting to `Err` would be wrong (caller would think + // the tx failed). Observability via a single structured log + // line is acceptable for current operator workflows — + // promote to a metric only when monitoring infrastructure is + // in place to consume one. tracing::warn!( wallet_id = %hex::encode(self.wallet_id), txid = %tx.txid(), From 79843e325fed222c8623b96a08e1d40f30608c1b Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 13:32:46 +0200 Subject: [PATCH 09/33] test(rs-platform-wallet): broadcast ordering + rollback contract (CMT-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) --- .../src/wallet/core/broadcast.rs | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 9da16918f9a..138e7bcee64 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -278,3 +278,164 @@ impl CoreWallet { Ok(tx) } } + +#[cfg(test)] +mod tests { + //! Broadcast ordering / rollback contract tests (CMT-003). + //! + //! The PR's central correctness claim is: + //! + //! * a failed `broadcast_transaction` must propagate `Err` so callers + //! short-circuit before any spendable-set mutation, and + //! * a successful `broadcast_transaction` must hand the txid back so + //! the caller can register the tx as a mempool spend. + //! + //! `CoreWallet::send_to_addresses` enforces this with a single `?` on + //! the `broadcast_transaction` call before the post-broadcast + //! `check_core_transaction(.., Mempool, ..)` block runs. Anything that + //! breaks the wrapper's pass-through behaviour silently moves the + //! "register as spent" block above the broadcast line and reintroduces + //! the regression flagged on #3466. These tests pin that pass-through + //! contract. + 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; + + /// Mock broadcaster that records every call and returns a configured + /// canned outcome. Generic over the configured outcome so tests can + /// drive both the success and failure branches without importing a + /// real network broadcaster. + 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 { + self.calls.fetch_add(1, Ordering::SeqCst); + match &self.outcome { + BroadcastOutcome::Ok(txid) => Ok(*txid), + BroadcastOutcome::Err(msg) => { + Err(PlatformWalletError::TransactionBroadcast(msg.clone())) + } + } + } + } + + /// Coinbase-style transaction good enough to round-trip through + /// `broadcast_transaction`'s pass-through. The shape doesn't matter + /// for these tests — only the broadcaster's Err/Ok branch does. + fn dummy_transaction() -> Transaction { + // Minimal serialized regtest coinbase tx (1 input, 1 output, 0 value). + // Hex was generated from a `Transaction { version: 1, lock_time: 0, + // input: vec![TxIn::default()], output: vec![TxOut { value: 0, + // script_pubkey: ScriptBuf::new() }], special_transaction_payload: None }` + // round-trip; embedded here so the test stays free of fixture I/O. + let bytes = hex::decode( + "010000000100000000000000000000000000000000000000000000000000000000000000\ + 00ffffffff00ffffffff0100000000000000000000000000", + ) + .expect("valid hex"); + deserialize(&bytes).expect("deserializable tx") + } + + fn make_core_wallet(broadcaster: Arc) -> CoreWallet { + 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 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() { + 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 success: `broadcast_transaction` hands the txid back + /// untouched. `send_to_addresses` then re-acquires the wallet lock + /// and registers the tx as a mempool spend; that registration is + /// gated on this Ok return. If the wrapper ever swallows or remaps + /// the txid, the spent-input tracking on the success path silently + /// breaks. + #[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" + ); + } +} From 391768cefdc1d2509464ba4dc76dd15fea273cfd Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 14:46:18 +0200 Subject: [PATCH 10/33] docs(rs-platform-wallet): tighten and deduplicate inline comments on PR #3585 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/wallet/core/broadcast.rs | 94 +++++-------------- .../rs-sdk/src/platform/dpns_usernames/mod.rs | 6 +- 2 files changed, 27 insertions(+), 73 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 138e7bcee64..c05c55e8ac0 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -138,26 +138,19 @@ impl CoreWallet { .build() .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; - // `select_inputs` is the only source of UTXOs for this builder, - // so `tx.input` outpoints must be a subset of the height-aware - // `spendable` set by the builder's contract. The check below is - // a defense-in-depth runtime guard for builder regressions; - // under normal operation this branch is unreachable. Inputs are - // not marked spent here either way — that happens after a - // successful broadcast (see #3466 review): a failed broadcast - // must not leave UTXOs falsely marked spent. + // 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 = tx.input.iter().map(|txin| txin.previous_output).collect(); let spendable_outpoints: BTreeSet = spendable.iter().map(|utxo| utxo.outpoint).collect(); if !selected.is_subset(&spendable_outpoints) { - // INTENTIONAL(CMT-002): The `ConcurrentSpendConflict` variant - // is named and framed as user-retryable for forward - // compatibility. The current code path is only reachable on - // a builder-internal regression, but the typed variant is - // preserved so future work that surfaces real concurrent-spend - // conflicts (e.g. from cross-process wallets) can route - // through the same handler without an API churn. + // 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); } @@ -241,15 +234,10 @@ impl CoreWallet { .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) .await; if !check_result.is_relevant { - // CMT-004: The wallet just built and signed this - // transaction from its own spendable inputs, so a - // `!is_relevant` post-broadcast check is an - // internal-invariant violation, not a transient. Emit a - // structured `error!` event with stable field names so - // operators can alert on it independent of the message - // text. We still return `Ok(tx)`: broadcast already - // succeeded, and rolling back here would mislead the - // caller into thinking the network rejected the tx. + // CMT-004: own-built tx unrecognised by our own checker + // is an internal-invariant violation, not a transient. + // Structured `error!` with stable fields so operators can + // alert independent of message text. tracing::error!( target: "platform_wallet::broadcast", event = "post_broadcast_unrelated_to_own_wallet", @@ -259,14 +247,8 @@ impl CoreWallet { ); } } else { - // INTENTIONAL(CMT-005): The wallet-missing branch indicates - // the wallet entry was removed from the manager between the - // lock drop and re-acquisition. Broadcast already succeeded, - // so converting to `Err` would be wrong (caller would think - // the tx failed). Observability via a single structured log - // line is acceptable for current operator workflows — - // promote to a metric only when monitoring infrastructure is - // in place to consume one. + // INTENTIONAL(CMT-005): log-only is sufficient until metrics + // infrastructure exists; see broadcast-first rationale above. tracing::warn!( wallet_id = %hex::encode(self.wallet_id), txid = %tx.txid(), @@ -283,20 +265,10 @@ impl CoreWallet { mod tests { //! Broadcast ordering / rollback contract tests (CMT-003). //! - //! The PR's central correctness claim is: - //! - //! * a failed `broadcast_transaction` must propagate `Err` so callers - //! short-circuit before any spendable-set mutation, and - //! * a successful `broadcast_transaction` must hand the txid back so - //! the caller can register the tx as a mempool spend. - //! - //! `CoreWallet::send_to_addresses` enforces this with a single `?` on - //! the `broadcast_transaction` call before the post-broadcast - //! `check_core_transaction(.., Mempool, ..)` block runs. Anything that - //! breaks the wrapper's pass-through behaviour silently moves the - //! "register as spent" block above the broadcast line and reintroduces - //! the regression flagged on #3466. These tests pin that pass-through - //! contract. + //! 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; @@ -312,10 +284,7 @@ mod tests { use key_wallet::Network; use key_wallet_manager::WalletManager; - /// Mock broadcaster that records every call and returns a configured - /// canned outcome. Generic over the configured outcome so tests can - /// drive both the success and failure branches without importing a - /// real network broadcaster. + /// Records every call and returns a canned outcome. struct MockBroadcaster { outcome: BroadcastOutcome, calls: AtomicUsize, @@ -352,15 +321,9 @@ mod tests { } } - /// Coinbase-style transaction good enough to round-trip through - /// `broadcast_transaction`'s pass-through. The shape doesn't matter - /// for these tests — only the broadcaster's Err/Ok branch does. + /// 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 { - // Minimal serialized regtest coinbase tx (1 input, 1 output, 0 value). - // Hex was generated from a `Transaction { version: 1, lock_time: 0, - // input: vec![TxIn::default()], output: vec![TxOut { value: 0, - // script_pubkey: ScriptBuf::new() }], special_transaction_payload: None }` - // round-trip; embedded here so the test stays free of fixture I/O. let bytes = hex::decode( "010000000100000000000000000000000000000000000000000000000000000000000000\ 00ffffffff00ffffffff0100000000000000000000000000", @@ -385,11 +348,8 @@ mod tests { ) } - /// 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. + /// 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( @@ -412,12 +372,8 @@ mod tests { ); } - /// Broadcast success: `broadcast_transaction` hands the txid back - /// untouched. `send_to_addresses` then re-acquires the wallet lock - /// and registers the tx as a mempool spend; that registration is - /// gated on this Ok return. If the wrapper ever swallows or remaps - /// the txid, the spent-input tracking on the success path silently - /// breaks. + /// 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(); diff --git a/packages/rs-sdk/src/platform/dpns_usernames/mod.rs b/packages/rs-sdk/src/platform/dpns_usernames/mod.rs index 7a6f56959c2..c3468db0d0c 100644 --- a/packages/rs-sdk/src/platform/dpns_usernames/mod.rs +++ b/packages/rs-sdk/src/platform/dpns_usernames/mod.rs @@ -455,10 +455,8 @@ impl Sdk { let normalized_label = normalize_dpns_label(name); - // An empty normalized label (e.g. `""`, `".dash"`, `".DASH"`) cannot - // resolve to a registered identity. Skip the contract fetch and - // return early so the API mirrors `is_dpns_name_available` and - // doesn't perform a wasted RPC round-trip on malformed input. + // Empty normalized label (e.g. `""`, `".dash"`) can't resolve to an + // identity; bail before the contract fetch. Mirrors `is_dpns_name_available`. if normalized_label.is_empty() { return Ok(None); } From 43e3f9d1cbcec5e424eaf5cfc72fcdc2a668e068 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 15:17:32 +0200 Subject: [PATCH 11/33] fix(rs-platform-wallet): defer change-address commit past broadcast (CMT-001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (23d8943c38) 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) --- .../src/wallet/core/broadcast.rs | 62 ++++++++++--------- 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index c05c55e8ac0..bad9d1f3bc7 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -46,7 +46,7 @@ impl CoreWallet { )); } - let tx = { + let (tx, xpub) = { let mut wm = self.wallet_manager.write().await; let (wallet, info) = wm.get_wallet_and_info_mut(&self.wallet_id).ok_or_else(|| { crate::error::PlatformWalletError::WalletNotFound( @@ -154,33 +154,7 @@ impl CoreWallet { return Err(PlatformWalletError::ConcurrentSpendConflict); } - // Revalidation passed; now commit the change-address advance so - // the next send picks up the next index. Re-borrow the managed - // account because `select_inputs` above borrowed - // `info.core_wallet.accounts` and ended the earlier reborrow. - 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 - )) - })?; - change_account - .next_change_address(Some(&xpub), true) - .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; - - tx + (tx, xpub) }; // Broadcast first; if the network rejects we leave wallet state @@ -230,6 +204,38 @@ impl CoreWallet { { 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" + ); + } + } + let check_result = info .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) .await; From 5d4a61bf0cdaa31b4c57ef48d207e87f7fe927cd Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 6 May 2026 15:19:40 +0200 Subject: [PATCH 12/33] test(rs-platform-wallet): rename broadcast pass-through tests to match 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) --- .../src/wallet/core/broadcast.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index bad9d1f3bc7..76f00764382 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -269,12 +269,12 @@ impl CoreWallet { #[cfg(test)] mod tests { - //! Broadcast ordering / rollback contract tests (CMT-003). + //! `broadcast_transaction` pass-through contract. //! - //! 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. + //! 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; @@ -354,10 +354,10 @@ mod tests { ) } - /// Rollback half of the #3466 contract: a broadcast `Err` propagates so - /// callers `?`-out before any spendable-set mutation. + /// `broadcast_transaction` forwards a broadcaster `Err` to the caller + /// without transformation. #[tokio::test] - async fn broadcast_failure_keeps_inputs_spendable() { + async fn broadcast_transaction_passes_through_err_unchanged() { let broadcaster = Arc::new(MockBroadcaster::new(BroadcastOutcome::Err( "simulated network rejection".to_string(), ))); @@ -378,10 +378,10 @@ mod tests { ); } - /// Success half of the #3466 contract: the broadcaster's `Txid` is - /// passed through unchanged so the mempool-registration block fires. + /// `broadcast_transaction` forwards the broadcaster's `Txid` to the + /// caller without transformation. #[tokio::test] - async fn broadcast_success_marks_inputs_unavailable() { + 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)); From 4dd55d2f42fcd337cdbf0bfb41b92815c3334f39 Mon Sep 17 00:00:00 2001 From: lklimek <842586+lklimek@users.noreply.github.com> Date: Fri, 8 May 2026 14:59:17 +0200 Subject: [PATCH 13/33] fix: close same-UTXO concurrent-selection race in send_to_addresses (#3622) Co-authored-by: Claude Opus 4.7 (1M context) --- packages/rs-platform-wallet/src/error.rs | 6 + .../src/wallet/core/broadcast.rs | 424 +++++++++++++++--- .../rs-platform-wallet/src/wallet/core/mod.rs | 1 + .../src/wallet/core/reservations.rs | 139 ++++++ .../src/wallet/core/wallet.rs | 7 + 5 files changed, 516 insertions(+), 61 deletions(-) create mode 100644 packages/rs-platform-wallet/src/wallet/core/reservations.rs diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index ed724b1f161..ce505753b9b 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -63,6 +63,12 @@ pub enum PlatformWalletError { #[error("Transaction builder selected an unavailable UTXO (concurrent spend); retry")] ConcurrentSpendConflict, + #[error( + "no spendable inputs available for {context} \ + (other in-flight transactions reserved the wallet's UTXOs; retry once they confirm)" + )] + NoSpendableInputs { context: String }, + #[error("Asset lock proof waiting failed: {0}")] AssetLockProofWait(String), diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 732e398eb59..636427244d2 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -29,9 +29,11 @@ impl CoreWallet { /// Build, sign, and broadcast a payment to the given addresses. /// - /// Uses key-wallet's [`TransactionBuilder`] for UTXO selection, fee - /// estimation, and signing. Change is sent to the next internal address - /// of the specified account. + /// Uses key-wallet's [`TransactionBuilder`] for UTXO selection, fee estimation, and signing. + /// Change is sent to the next internal address of the specified account. Concurrent calls on + /// the same wallet handle are race-safe via the reservation set in [`super::reservations`]: + /// the second caller short-circuits with [`PlatformWalletError::NoSpendableInputs`] before + /// touching the network if all UTXOs are reserved by an in-flight broadcast. pub async fn send_to_addresses( &self, account_type: StandardAccountType, @@ -47,7 +49,7 @@ impl CoreWallet { )); } - let (tx, xpub) = { + let (tx, xpub, _reservation) = { let mut wm = self.wallet_manager.write().await; let (wallet, info) = wm.get_wallet_and_info_mut(&self.wallet_id).ok_or_else(|| { crate::error::PlatformWalletError::WalletNotFound( @@ -76,12 +78,26 @@ impl CoreWallet { )) })?; + // Snapshot spendable UTXOs minus any in-flight reservations from + // a concurrent `send_to_addresses` on this handle. Single lock + // acquisition for the whole filter pass. + let reserved = self.reservations.snapshot(); let spendable: Vec<_> = account .spendable_utxos(current_height) .into_iter() + .filter(|utxo| !reserved.contains(&utxo.outpoint)) .cloned() .collect(); + if spendable.is_empty() { + return Err(PlatformWalletError::NoSpendableInputs { + context: format!( + "{:?} account {} (all UTXOs reserved by in-flight transactions)", + account_type, account_index + ), + }); + } + let xpub = wallet_accounts .get(&account_index) .map(|a| a.account_xpub) @@ -141,17 +157,29 @@ impl CoreWallet { None }, ) - .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + .map_err(|e| { + // Map coin-selection failures to `NoSpendableInputs`. String-match pinned by + // `builder_error_text_contract_for_no_inputs`. + // TODO(typed-wrapper): drop once upstream exposes `SelectionError` typed. + let msg = e.to_string(); + if msg.contains("Insufficient funds") || msg.contains("No UTXOs available") { + PlatformWalletError::NoSpendableInputs { + context: format!( + "{:?} account {} ({})", + account_type, account_index, msg + ), + } + } else { + PlatformWalletError::TransactionBuild(msg) + } + })?; let tx = builder .build() .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. + // Defense-in-depth: unreachable under normal builder contract but guards against + // a future regression where `select_inputs` picks an outpoint outside `spendable`. let selected: BTreeSet = tx.input.iter().map(|txin| txin.previous_output).collect(); let spendable_outpoints: BTreeSet = @@ -163,60 +191,27 @@ impl CoreWallet { return Err(PlatformWalletError::ConcurrentSpendConflict); } - (tx, xpub) + // Reserve before releasing the lock so the next caller sees these outpoints + // filtered out. Guard held until `check_core_transaction` marks them spent + // (success) or the error unwinds (failure → outpoints released for retry). + let reservation = self.reservations.reserve(selected.into_iter().collect()); + + (tx, xpub, reservation) }; - // 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. + // Broadcast first — on error we leave wallet state untouched so the caller can retry. + // If the network accepted but the call errored (ambiguous outcome), a retry will be + // rejected as a duplicate spend rather than us marking UTXOs spent prematurely. 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. + // Mark inputs spent under the write lock, transitioning them from "reserved" to "spent" + // before the reservation guard drops — no observable gap for concurrent callers. + // Warning paths below do NOT return Err: the network already accepted the tx. { 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. + // Commit the change-address advance post-broadcast; doing it before would burn + // a derivation index on network rejection, widening the gap-limit window. let change_account = match account_type { StandardAccountType::BIP44Account => info .core_wallet @@ -249,10 +244,8 @@ impl CoreWallet { .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) .await; if !check_result.is_relevant { - // CMT-004: own-built tx unrecognised by our own checker - // is an internal-invariant violation, not a transient. - // Structured `error!` with stable fields so operators can - // alert independent of message text. + // CMT-004: own-built tx unrecognised by our checker — internal invariant + // violation, not a transient. Stable event field for operator alerting. tracing::error!( target: "platform_wallet::broadcast", event = "post_broadcast_unrelated_to_own_wallet", @@ -272,6 +265,10 @@ impl CoreWallet { } } + // Explicit drop: inputs are already marked spent above; no gap between + // "reservation released" and "spent visible" to concurrent callers. + drop(_reservation); + Ok(tx) } } @@ -409,4 +406,309 @@ mod tests { "broadcaster must be called exactly once on a successful broadcast" ); } + + // Race-closing tests: same-UTXO concurrent `send_to_addresses`. + // B must short-circuit with `NoSpendableInputs` before the network — a `TransactionBroadcast` + // failure from B would mean the bug is still open. + + use std::collections::BTreeMap; + + use dashcore::hashes::Hash; + use dashcore::{Address as DashAddress, OutPoint, TxOut}; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::wallet::Wallet; + use key_wallet::Utxo; + use tokio::sync::Notify; + + use crate::wallet::platform_wallet::PlatformWalletInfo; + use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; + + /// Mock broadcaster that gates the broadcast on an external `Notify`. + /// `entered` fires the moment `broadcast()` is awaited — by then the + /// caller has reserved its outpoints and dropped the wallet write lock. + struct GatedBroadcaster { + gate: Arc, + entered: Arc, + calls: AtomicUsize, + succeed: bool, + } + + #[async_trait] + impl TransactionBroadcaster for GatedBroadcaster { + async fn broadcast(&self, transaction: &Transaction) -> Result { + self.calls.fetch_add(1, Ordering::SeqCst); + self.entered.notify_one(); + self.gate.notified().await; + if self.succeed { + Ok(transaction.txid()) + } else { + Err(PlatformWalletError::TransactionBroadcast( + "mock failure".to_string(), + )) + } + } + } + + /// Always-failing mock broadcaster — used to assert that a failed + /// broadcast releases the reservation so a retry can pick up the + /// same UTXO. + struct FailingBroadcaster; + + #[async_trait] + impl TransactionBroadcaster for FailingBroadcaster { + async fn broadcast(&self, _transaction: &Transaction) -> Result { + Err(PlatformWalletError::TransactionBroadcast( + "always fails".to_string(), + )) + } + } + + /// Build a single-wallet `WalletManager` containing one BIP-44 + /// account (index 0) funded with one large UTXO at the account's + /// first receive address. Returns the wallet manager handle, the + /// wallet id, and a recipient address (a separate derived address + /// in the same account — funding/sending to the same address is + /// not the property under test). + fn build_funded_wallet_manager( + utxo_value: u64, + ) -> ( + Arc>>, + crate::wallet::platform_wallet::WalletId, + DashAddress, + ) { + let wallet = Wallet::new_random(Network::Testnet, WalletAccountCreationOptions::Default) + .expect("test wallet"); + + let xpub = wallet + .accounts + .standard_bip44_accounts + .get(&0) + .expect("bip44 account 0") + .account_xpub; + let mut wallet_info = ManagedWalletInfo::from_wallet(&wallet, 0); + + // Height must be well past UTXO height: `select_coins_with_size` enforces + // `min_confirmations >= 1`, which requires synced_height > utxo_height. + use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface as _; + wallet_info.update_synced_height(100); + + let funding_address = wallet_info + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("managed bip44 account 0") + .next_receive_address(Some(&xpub), true) + .expect("derive receive address"); + + let outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); + let mut utxo = Utxo::new( + outpoint, + TxOut { + value: utxo_value, + script_pubkey: funding_address.script_pubkey(), + }, + funding_address, + 1, + false, + ); + utxo.is_confirmed = true; + wallet_info + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("managed bip44 account 0") + .utxos + .insert(outpoint, utxo); + + let info = PlatformWalletInfo { + core_wallet: wallet_info, + balance: Arc::new(WalletBalance::new()), + identity_manager: crate::wallet::identity::IdentityManager::new(), + tracked_asset_locks: BTreeMap::new(), + }; + + let mut wm: WalletManager = WalletManager::new(Network::Testnet); + let wallet_id = wm.insert_wallet(wallet, info).expect("insert"); + + // Recipient — use the second receive address as a stable target. + let recipient = { + let info = wm.get_wallet_info_mut(&wallet_id).expect("info"); + info.core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&0) + .expect("acc") + .next_receive_address(Some(&xpub), true) + .expect("derive recipient") + }; + + (Arc::new(RwLock::new(wm)), wallet_id, recipient) + } + + fn make_core_wallet_for_manager( + wm: Arc>>, + wallet_id: crate::wallet::platform_wallet::WalletId, + broadcaster: Arc, + ) -> CoreWallet { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + CoreWallet::new( + sdk, + wm, + wallet_id, + broadcaster, + Arc::new(WalletBalance::new()), + ) + } + + /// Two concurrent `send_to_addresses` calls on one wallet with one UTXO must yield exactly + /// one broadcast. The loser must get [`PlatformWalletError::NoSpendableInputs`] — never + /// `TransactionBroadcast` (that would mean it reached the network, which is the bug closed). + #[tokio::test] + async fn concurrent_same_utxo_sends_resolve_via_reservation_set() { + use key_wallet::account::account_type::StandardAccountType; + + let (wm, wallet_id, recipient) = build_funded_wallet_manager(2_000_000); + let gate = Arc::new(Notify::new()); + let entered = Arc::new(Notify::new()); + let broadcaster = Arc::new(GatedBroadcaster { + gate: Arc::clone(&gate), + entered: Arc::clone(&entered), + calls: AtomicUsize::new(0), + succeed: true, + }); + let core = make_core_wallet_for_manager( + wm, + wallet_id, + Arc::clone(&broadcaster) as Arc, + ); + + let send_value = 100_000; + let outputs_a = vec![(recipient.clone(), send_value)]; + let outputs_b = vec![(recipient.clone(), send_value)]; + + // Spawn caller A. It will reserve the only spendable outpoint + // under the wallet write lock, drop the lock, and block on the + // broadcast `Notify`. + let core_a = core.clone(); + let a_handle = tokio::spawn(async move { + core_a + .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs_a) + .await + }); + + // Deterministic handshake: wait until A has reached the broadcast gate. + // By that point A has reserved the outpoint and dropped the wallet write lock. + entered.notified().await; + + // Caller B starts now. The wallet's only UTXO is reserved by A, + // so B's spendable snapshot is empty → `NoSpendableInputs`. + let b_result = core + .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs_b) + .await; + + match &b_result { + Err(PlatformWalletError::NoSpendableInputs { context }) => { + assert!( + context.contains("reserved") + || context.contains("Insufficient") + || context.contains("No UTXOs"), + "B's NoSpendableInputs context should mention reservation \ + or insufficient/no-utxos; got: {context}" + ); + } + other => panic!( + "B must short-circuit with NoSpendableInputs (the race-loser \ + must not reach the broadcaster); got: {other:?}" + ), + } + + // Now release A's broadcast. + gate.notify_one(); + + let a_result = a_handle.await.expect("a task panicked"); + assert!( + a_result.is_ok(), + "A must succeed once its broadcast gate fires; got: {a_result:?}" + ); + + // Pin "loser never reached the network" directly: only A invoked the broadcaster. + assert_eq!( + broadcaster.calls.load(Ordering::SeqCst), + 1, + "broadcaster must be called exactly once across both concurrent senders" + ); + } + + /// On broadcast failure, the reservation must be released so the + /// caller can retry. This is the regression-tripwire for the + /// reservation guard's Drop semantics. + #[tokio::test] + async fn broadcast_failure_releases_reservation_for_retry() { + use key_wallet::account::account_type::StandardAccountType; + + let (wm, wallet_id, recipient) = build_funded_wallet_manager(2_000_000); + let broadcaster: Arc = Arc::new(FailingBroadcaster); + let core = make_core_wallet_for_manager(wm, wallet_id, broadcaster); + + let outputs = vec![(recipient.clone(), 100_000)]; + + // First call fails at the broadcast step → guard drops → + // reservation released. The change-address index is also rolled + // back by virtue of #3585's peek-then-commit pattern. + let first = core + .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs.clone()) + .await; + assert!( + matches!(first, Err(PlatformWalletError::TransactionBroadcast(_))), + "first call must surface broadcast failure; got: {first:?}" + ); + + // Reservation released: the second call must reach the broadcaster (same UTXO visible), + // not short-circuit with `NoSpendableInputs` (which would indicate a leaked reservation). + let second = core + .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs) + .await; + match second { + Err(PlatformWalletError::TransactionBroadcast(_)) => { + // Expected — reservation released, coin selection + // succeeded, broadcaster rejected as designed. + } + Err(PlatformWalletError::NoSpendableInputs { .. }) => { + panic!( + "reservation leaked after broadcast failure — second \ + call should have selected the released UTXO" + ); + } + other => panic!("unexpected second call result: {other:?}"), + } + } + + /// Pins the upstream error text the production string-match in + /// `send_to_addresses` depends on. If `key-wallet` ever rephrases + /// "Insufficient funds" / "No UTXOs available", this test breaks + /// loudly so the matcher can be updated (or, ideally, replaced + /// with a typed `SelectionError` once upstream exposes it). + #[test] + fn builder_error_text_contract_for_no_inputs() { + use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy; + use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder; + + let (_, _, recipient) = build_funded_wallet_manager(2_000_000); + + let result = TransactionBuilder::new() + .add_output(&recipient, 100_000) + .expect("add_output") + .select_inputs(&[], SelectionStrategy::LargestFirst, 100, |_| None); + + let err = match result { + Ok(_) => panic!("empty UTXO slice must fail coin selection"), + Err(e) => e, + }; + let msg = err.to_string(); + assert!( + msg.contains("Insufficient funds") || msg.contains("No UTXOs available"), + "production string-match in send_to_addresses depends on these tokens; \ + got: {msg}" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/core/mod.rs b/packages/rs-platform-wallet/src/wallet/core/mod.rs index 106a4108c22..e068dfacb4d 100644 --- a/packages/rs-platform-wallet/src/wallet/core/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/core/mod.rs @@ -1,6 +1,7 @@ pub mod balance; pub mod balance_handler; mod broadcast; +mod reservations; pub mod wallet; pub use balance::WalletBalance; diff --git a/packages/rs-platform-wallet/src/wallet/core/reservations.rs b/packages/rs-platform-wallet/src/wallet/core/reservations.rs new file mode 100644 index 00000000000..070c60e96a3 --- /dev/null +++ b/packages/rs-platform-wallet/src/wallet/core/reservations.rs @@ -0,0 +1,139 @@ +//! Per-wallet outpoint reservation set for [`CoreWallet::send_to_addresses`](super::broadcast). +//! +//! Closes the same-UTXO concurrent-selection race: the first caller reserves its selected +//! outpoints under the write lock; subsequent callers filter them out and short-circuit with +//! [`PlatformWalletError::NoSpendableInputs`](crate::PlatformWalletError) before hitting the +//! network. Reservations are released by an RAII guard on success, error, or panic. + +use std::collections::HashSet; +use std::sync::{Arc, Mutex}; + +use dashcore::OutPoint; + +/// Per-wallet set of outpoints that have been selected for an in-flight +/// broadcast but not yet marked spent in `ManagedWalletInfo`. +/// +/// Cheaply cloneable: holds an `Arc>` internally. All clones share +/// the same set. +#[derive(Debug, Default, Clone)] +pub(crate) struct OutpointReservations { + inner: Arc>>, +} + +impl OutpointReservations { + pub(crate) fn new() -> Self { + Self::default() + } + + /// Test whether `outpoint` is currently reserved. + #[cfg(test)] + pub(crate) fn contains(&self, outpoint: &OutPoint) -> bool { + let guard = self + .inner + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + guard.contains(outpoint) + } + + /// Clone the current reservation set under a single lock acquisition. + /// + /// Callers filter spendable UTXOs against the returned snapshot to + /// avoid one mutex lock per candidate outpoint. + pub(crate) fn snapshot(&self) -> HashSet { + let guard = self + .inner + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + guard.clone() + } + + /// Reserve `outpoints`, returning an RAII guard that releases them on + /// drop. The guard must be held until the broadcast outcome is + /// reconciled into wallet state (success → `check_core_transaction` + /// has run; failure → caller has propagated the error). + pub(crate) fn reserve(&self, outpoints: Vec) -> OutpointReservationGuard { + { + let mut guard = self + .inner + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + for op in &outpoints { + guard.insert(*op); + } + } + OutpointReservationGuard { + reservations: Arc::clone(&self.inner), + outpoints, + } + } +} + +/// RAII guard releasing reservations on drop. +/// +/// Drop is infallible and panic-safe — the underlying `Mutex` is recovered +/// from poisoning so a panicking caller still releases its outpoints. +#[must_use = "dropping the guard immediately releases the reservation"] +pub(crate) struct OutpointReservationGuard { + reservations: Arc>>, + outpoints: Vec, +} + +impl Drop for OutpointReservationGuard { + fn drop(&mut self) { + let mut guard = self + .reservations + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + for op in &self.outpoints { + guard.remove(op); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use dashcore::hashes::Hash; + use dashcore::Txid; + + fn op(n: u32) -> OutPoint { + OutPoint::new(Txid::all_zeros(), n) + } + + #[test] + fn reserve_then_drop_releases() { + let res = OutpointReservations::new(); + let a = op(1); + { + let _g = res.reserve(vec![a]); + assert!(res.contains(&a)); + } + assert!(!res.contains(&a)); + } + + #[test] + fn second_reservation_is_disjoint() { + let res = OutpointReservations::new(); + let a = op(1); + let b = op(2); + let _g1 = res.reserve(vec![a]); + let _g2 = res.reserve(vec![b]); + assert!(res.contains(&a)); + assert!(res.contains(&b)); + } + + #[test] + fn poisoned_mutex_still_releases() { + let res = OutpointReservations::new(); + let a = op(7); + let res_clone = res.clone(); + let _ = std::thread::spawn(move || { + let _g = res_clone.reserve(vec![a]); + panic!("intentional"); + }) + .join(); + // Guard dropped during unwind — outpoint must be released even + // though the mutex was poisoned. + assert!(!res.contains(&a)); + } +} diff --git a/packages/rs-platform-wallet/src/wallet/core/wallet.rs b/packages/rs-platform-wallet/src/wallet/core/wallet.rs index 5a29db29002..83a4a662a88 100644 --- a/packages/rs-platform-wallet/src/wallet/core/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/core/wallet.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use super::balance::WalletBalance; +use super::reservations::OutpointReservations; use dashcore::Address as DashAddress; use tokio::sync::RwLock; @@ -31,6 +32,10 @@ pub struct CoreWallet { pub(crate) broadcaster: Arc, /// Lock-free balance for UI reads. balance: Arc, + /// Outpoints currently reserved by an in-flight `send_to_addresses` + /// call on this handle. Closes the same-UTXO concurrent-selection + /// race — see [`super::reservations`]. + pub(crate) reservations: OutpointReservations, } impl CoreWallet { @@ -47,6 +52,7 @@ impl CoreWallet { wallet_id, broadcaster, balance, + reservations: OutpointReservations::new(), } } @@ -244,6 +250,7 @@ impl Clone for CoreWallet { wallet_id: self.wallet_id, broadcaster: Arc::clone(&self.broadcaster), balance: Arc::clone(&self.balance), + reservations: self.reservations.clone(), } } } From 349b95b8c8499b7a07db506591d84848c58896bd Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Fri, 8 May 2026 15:10:23 +0200 Subject: [PATCH 14/33] feat(rs-platform-wallet): attach outpoint context to ConcurrentSpendConflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ConcurrentSpendConflict` was unit-only — if the defense-in-depth subset check ever fired, operators would have no diagnostic content. Carry the selected outpoints in the variant so the construction site (and downstream log lines) surface them automatically via `Display`. Strip the `INTENTIONAL(CMT-002)` review-thread tag from the same site — git history is the record for review provenance. Refs PR #3585 review (F-001, F-004). Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/rs-platform-wallet/src/error.rs | 8 ++++++-- packages/rs-platform-wallet/src/wallet/core/broadcast.rs | 9 +++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index ce505753b9b..99450c93d3a 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -1,3 +1,4 @@ +use dashcore::OutPoint; use dpp::identifier::Identifier; use key_wallet::Network; @@ -60,8 +61,11 @@ pub enum PlatformWalletError { #[error("Transaction building failed: {0}")] TransactionBuild(String), - #[error("Transaction builder selected an unavailable UTXO (concurrent spend); retry")] - ConcurrentSpendConflict, + #[error( + "Transaction builder selected an unavailable UTXO (concurrent spend); retry. \ + Selected outpoints: {selected:?}" + )] + ConcurrentSpendConflict { selected: Vec }, #[error( "no spendable inputs available for {context} \ diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 636427244d2..69373a8c76c 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -185,10 +185,11 @@ impl CoreWallet { let spendable_outpoints: BTreeSet = 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); + // Typed retryable variant: forward-compatible with cross-process + // concurrent-spend surfacing; today only a builder regression hits it. + return Err(PlatformWalletError::ConcurrentSpendConflict { + selected: selected.into_iter().collect(), + }); } // Reserve before releasing the lock so the next caller sees these outpoints From 6239fda21f924562590f48f624cfcc6dd1731909 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Fri, 8 May 2026 15:10:38 +0200 Subject: [PATCH 15/33] chore(rs-platform-wallet): drop CMT-NNN review tombstones from broadcast.rs Strip the `CMT-004` review-thread prefix from the post-broadcast checker comment in `send_to_addresses`. The surrounding prose already documents the present-state semantics; the review-comment ID is git-history noise. Refs PR #3585 review (F-001). Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/rs-platform-wallet/src/wallet/core/broadcast.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 69373a8c76c..f54ed2992b0 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -245,7 +245,7 @@ impl CoreWallet { .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) .await; if !check_result.is_relevant { - // CMT-004: own-built tx unrecognised by our checker — internal invariant + // Own-built tx unrecognised by our checker is an internal invariant // violation, not a transient. Stable event field for operator alerting. tracing::error!( target: "platform_wallet::broadcast", From 4d204cdaef79c490caaeb5cff0fa2259e3c2e71e Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Fri, 8 May 2026 15:10:51 +0200 Subject: [PATCH 16/33] fix(rs-platform-wallet): structured tracing fields on wallet-missing warn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Align the post-broadcast wallet-missing `tracing::warn!` with its two sibling sites in `send_to_addresses` by adding `target: "platform_wallet::broadcast"` and `event = "post_broadcast_wallet_missing"`. Operators alerting on stable event names now catch all three post-broadcast observability paths without parsing free-text. Strip the `INTENTIONAL(CMT-005)` review-thread tag from the same site — the rewritten present-state comment already explains why log-only is sufficient on this path. Refs PR #3585 review (F-001, F-003). Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/rs-platform-wallet/src/wallet/core/broadcast.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index f54ed2992b0..7f4edfc04bd 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -256,9 +256,11 @@ impl CoreWallet { ); } } else { - // INTENTIONAL(CMT-005): log-only is sufficient until metrics - // infrastructure exists; see broadcast-first rationale above. + // Log-only: broadcast already succeeded; the wallet handle is stale and + // future sends will surface a clean `WalletNotFound` from the lookup above. tracing::warn!( + target: "platform_wallet::broadcast", + event = "post_broadcast_wallet_missing", wallet_id = %hex::encode(self.wallet_id), txid = %tx.txid(), "wallet missing during post-broadcast transaction registration" From 0bacd25279d87f777a80277e60f23ac2c6f05ff3 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 12 May 2026 13:21:13 +0200 Subject: [PATCH 17/33] chore: improve error type --- packages/rs-platform-wallet/src/error.rs | 13 +++++++------ .../src/wallet/core/broadcast.rs | 16 +++++++--------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 99450c93d3a..3944036f342 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -1,6 +1,6 @@ use dashcore::OutPoint; use dpp::identifier::Identifier; -use key_wallet::Network; +use key_wallet::{account::StandardAccountType, Network}; /// Errors that can occur in platform wallet operations #[derive(Debug, thiserror::Error)] @@ -67,11 +67,12 @@ pub enum PlatformWalletError { )] ConcurrentSpendConflict { selected: Vec }, - #[error( - "no spendable inputs available for {context} \ - (other in-flight transactions reserved the wallet's UTXOs; retry once they confirm)" - )] - NoSpendableInputs { context: String }, + #[error("no spendable inputs available on {account_type} account {account_index}: {context}")] + NoSpendableInputs { + account_type: StandardAccountType, + account_index: u32, + context: String, + }, #[error("Asset lock proof waiting failed: {0}")] AssetLockProofWait(String), diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index f9e8908f774..99944bb2dac 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -121,10 +121,9 @@ impl CoreWallet { if spendable.is_empty() { return Err(PlatformWalletError::NoSpendableInputs { - context: format!( - "{:?} account {} (all UTXOs reserved by in-flight transactions)", - account_type, account_index - ), + account_index, + account_type, + context: "all UTXOs used or reserved by in-flight transactions".to_string(), }); } @@ -157,10 +156,9 @@ impl CoreWallet { let msg = e.to_string(); if msg.contains("Insufficient funds") || msg.contains("No UTXOs available") { PlatformWalletError::NoSpendableInputs { - context: format!( - "{:?} account {} ({})", - account_type, account_index, msg - ), + account_type, + account_index, + context: msg, } } else { PlatformWalletError::TransactionBuild(msg) @@ -599,7 +597,7 @@ mod tests { .await; match &b_result { - Err(PlatformWalletError::NoSpendableInputs { context }) => { + Err(PlatformWalletError::NoSpendableInputs { context, .. }) => { assert!( context.contains("reserved") || context.contains("Insufficient") From 0188fa97f82fd357987d4ed9963788f1c04e0bb2 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 12 May 2026 14:09:05 +0200 Subject: [PATCH 18/33] chore: improve docs --- .../src/wallet/core/broadcast.rs | 43 +++++++++---------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 99944bb2dac..631ebc1dd14 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -150,8 +150,8 @@ impl CoreWallet { }) .await .map_err(|e| { - // Map coin-selection failures to `NoSpendableInputs`. String-match pinned by - // `builder_error_text_contract_for_no_inputs`. + // Map coin-selection failures to `NoSpendableInputs`. The string-match is + // brittle against upstream rephrasing and is currently unpinned by tests. // TODO(typed-wrapper): drop once upstream exposes `SelectionError` typed via BuilderError. let msg = e.to_string(); if msg.contains("Insufficient funds") || msg.contains("No UTXOs available") { @@ -265,12 +265,17 @@ impl CoreWallet { #[cfg(test)] mod tests { - //! `broadcast_transaction` pass-through contract. + //! Broadcast and `send_to_addresses` contracts. //! - //! 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. + //! Pins: + //! - `broadcast_transaction` forwards the broadcaster's `Ok`/`Err` unchanged. + //! - Concurrent `send_to_addresses` on the same wallet handle resolves via + //! the reservation set: the loser short-circuits with `NoSpendableInputs` + //! before reaching the broadcaster. + //! - A broadcast failure releases the reservation so a retry sees the same + //! UTXO as spendable again. + //! - An empty spendable snapshot (e.g. all UTXOs reserved) maps to + //! `NoSpendableInputs` via the early-exit guard. use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; @@ -673,10 +678,12 @@ mod tests { } } - /// Pins the upstream error text the production string-match in - /// `send_to_addresses` depends on. If `key-wallet` ever rephrases - /// its coin-selection errors, this test breaks loudly so the matcher - /// can be updated (or replaced with typed `SelectionError` matching). + /// Pins the early-exit guard: when the spendable snapshot is empty + /// (e.g. all UTXOs reserved by in-flight broadcasts), `send_to_addresses` + /// surfaces `NoSpendableInputs` without invoking the builder. + /// + /// Note: the upstream coin-selection string-match in `send_to_addresses` + /// is not exercised here — that path is currently unpinned. #[tokio::test] async fn builder_error_text_contract_for_no_inputs() { use key_wallet::account::account_type::StandardAccountType; @@ -685,20 +692,10 @@ mod tests { let broadcaster: Arc = Arc::new(FailingBroadcaster); let core = make_core_wallet_for_manager(wm, wallet_id, broadcaster); - // Drain the UTXO by marking it spent via a successful reservation then - // never releasing, simulating a zero-spendable wallet. We verify the - // production error-message contract by checking `send_to_addresses` - // surfaces `NoSpendableInputs` when the builder returns no-inputs. - // - // The simplest way: call `send_to_addresses` on a wallet whose only - // UTXO has been removed. We rebuild with zero UTXOs by using the - // `build_funded_wallet_manager(0)` path — but that fails UTXO height. - // Instead, verify directly that `NoSpendableInputs` is mapped when - // the spendable set is empty before building (the early-exit guard). let outputs = vec![(recipient.clone(), 100_000)]; - // Reserve the wallet's only outpoint externally to make the spendable - // set empty for the next caller. Use the reservation API directly. + // Reserve the wallet's only outpoint so the spendable snapshot is + // empty for the next caller, exercising the early-exit guard. let outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); let _guard = core.reservations.reserve(vec![outpoint]); From 9902cbd6da33641b55044b13d0722861e78633ca Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 12 May 2026 15:19:14 +0200 Subject: [PATCH 19/33] chore: fix build --- packages/rs-platform-wallet-ffi/tests/integration_tests.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/tests/integration_tests.rs b/packages/rs-platform-wallet-ffi/tests/integration_tests.rs index 09adb69a20f..de826eddf43 100644 --- a/packages/rs-platform-wallet-ffi/tests/integration_tests.rs +++ b/packages/rs-platform-wallet-ffi/tests/integration_tests.rs @@ -50,7 +50,6 @@ fn test_wallet_from_mnemonic() { let result = platform_wallet_info_create_from_mnemonic( Network::Testnet.into(), mnemonic.as_ptr(), - std::ptr::null(), &mut handle, ); @@ -266,7 +265,6 @@ fn test_full_workflow() { let result = platform_wallet_info_create_from_mnemonic( Network::Testnet.into(), mnemonic.as_ptr(), - std::ptr::null(), &mut wallet_handle, ); assert_eq!(result.code, PlatformWalletFFIResultCode::Success); From 371e2c37426f2fbd778c3fbcc476f53b4bbadac3 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 13 May 2026 13:14:32 +0200 Subject: [PATCH 20/33] fix(rs-platform-wallet): restore defensive post-build UTXO revalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA-001 (LOW) from Marvin's #3585 merge audit: #3585's `OutpointReservations` pre-build filter closes the in-process concurrent-caller race entirely, and the existing post-build `selected.is_subset(&spendable_outpoints)` check catches builder regressions. But that check re-uses the SAME `spendable` snapshot captured BEFORE `build_signed`, so a future mutator that touches UTXOs outside the wallet write lock (mempool listener, chain reorg subsystem, cross-process spend) would slip through. Restore the defense-in-depth pattern from the obsolete commit `603b444425`: after `build_signed` returns, re-call `managed_account.spendable_utxos` within the same lock-acquisition block and confirm every selected outpoint is still present in the fresh view. If not, surface `PlatformWalletError::ConcurrentSpendConflict` (the typed retryable variant #3585 introduced) carrying the missing outpoints — semantically correct post-build, distinct from the pre-build `NoSpendableInputs` failure. Today no code path mutates UTXOs without holding the wallet write lock we hold across build, so this is unreachable by construction. The reservations guard remains the primary in-process race defense; this is the cross- subsystem / future-proofing net. Without it, someone introducing a parallel UTXO mutator later would re-open the race silently. No unit test: injecting a UTXO disappearance between `build_signed` and the fresh re-fetch requires test-only plumbing inside the same lock-acquisition block (no clean seam to mock). The two #3585 concurrency tests (`concurrent_same_utxo_sends_resolve_via_reservation_set`, `broadcast_failure_releases_reservation_for_retry`) still pass — semantics of the reservation path are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/wallet/core/broadcast.rs | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 631ebc1dd14..7c10baac243 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -179,6 +179,26 @@ impl CoreWallet { }); } + // Defense-in-depth: re-snapshot spendable UTXOs after `build_signed` and confirm + // every selected outpoint is still present. Today every UTXO mutator goes through + // the wallet write lock that we hold across build, so this is unreachable — but + // a future mutator running outside the lock (mempool listener, chain reorg, etc.) + // would slip through the pre-build `spendable` snapshot above; this fresh re-fetch + // catches it before broadcast. The reservations guard remains the primary in-process + // race defense; this is the cross-process / cross-subsystem net. + let fresh_spendable_outpoints: BTreeSet = managed_account + .spendable_utxos(current_height) + .into_iter() + .map(|utxo| utxo.outpoint) + .collect(); + if !selected.is_subset(&fresh_spendable_outpoints) { + let missing: Vec = selected + .difference(&fresh_spendable_outpoints) + .copied() + .collect(); + return Err(PlatformWalletError::ConcurrentSpendConflict { selected: missing }); + } + // Reserve before releasing the lock so the next caller sees these outpoints // filtered out. Guard held until `check_core_transaction` marks them spent // (success) or the error unwinds (failure → outpoints released for retry). @@ -276,18 +296,18 @@ mod tests { //! UTXO as spendable again. //! - An empty spendable snapshot (e.g. all UTXOs reserved) maps to //! `NoSpendableInputs` via the early-exit guard. - use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; + use std::sync::atomic::{AtomicUsize, Ordering}; use async_trait::async_trait; use dashcore::consensus::deserialize; use dashcore::{Transaction, Txid}; use tokio::sync::RwLock; + use crate::PlatformWalletError; use crate::broadcaster::TransactionBroadcaster; - use crate::wallet::core::balance::WalletBalance; use crate::wallet::core::CoreWallet; - use crate::PlatformWalletError; + use crate::wallet::core::balance::WalletBalance; use key_wallet::Network; use key_wallet_manager::WalletManager; @@ -410,9 +430,9 @@ mod tests { use dashcore::hashes::Hash; use dashcore::{Address as DashAddress, OutPoint, TxOut}; - use key_wallet::wallet::initialization::WalletAccountCreationOptions; - use key_wallet::wallet::Wallet; use key_wallet::Utxo; + use key_wallet::wallet::Wallet; + use key_wallet::wallet::initialization::WalletAccountCreationOptions; use tokio::sync::Notify; use crate::wallet::platform_wallet::PlatformWalletInfo; From ff56c56979903f939003dc93962f4d99db537567 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 13 May 2026 12:24:45 +0200 Subject: [PATCH 21/33] chore(rs-platform-wallet-ffi): use Result::is_err in group_info tests Pre-existing `matches!(result, Err(_))` patterns trip `clippy::redundant_pattern_matching` under the workspace's `-D warnings` gate. Swap to `result.is_err()` so the clippy step stays green for the crates this PR touches. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/rs-platform-wallet-ffi/src/tokens/group_info.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/tokens/group_info.rs b/packages/rs-platform-wallet-ffi/src/tokens/group_info.rs index 78595b5050c..b5c75a01a09 100644 --- a/packages/rs-platform-wallet-ffi/src/tokens/group_info.rs +++ b/packages/rs-platform-wallet-ffi/src/tokens/group_info.rs @@ -94,7 +94,7 @@ mod tests { fn test_decode_other_signer_null_action_id() { unsafe { let result = decode_group_info(2, 0, std::ptr::null(), false); - assert!(matches!(result, Err(_)), "expected Err(NullPointer)"); + assert!(result.is_err(), "expected Err(NullPointer)"); } } @@ -120,7 +120,7 @@ mod tests { fn test_decode_invalid_kind() { unsafe { let result = decode_group_info(99, 0, std::ptr::null(), false); - assert!(matches!(result, Err(_)), "expected Err(InvalidParameter)"); + assert!(result.is_err(), "expected Err(InvalidParameter)"); } } } From 54665018962348027c601aa1af270907418aa765 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Wed, 13 May 2026 15:52:49 +0200 Subject: [PATCH 22/33] chore: fmt --- .../rs-platform-wallet/src/wallet/core/broadcast.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 7c10baac243..5254d2d7318 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -296,18 +296,18 @@ mod tests { //! UTXO as spendable again. //! - An empty spendable snapshot (e.g. all UTXOs reserved) maps to //! `NoSpendableInputs` via the early-exit guard. - use std::sync::Arc; 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::PlatformWalletError; use crate::broadcaster::TransactionBroadcaster; - use crate::wallet::core::CoreWallet; use crate::wallet::core::balance::WalletBalance; + use crate::wallet::core::CoreWallet; + use crate::PlatformWalletError; use key_wallet::Network; use key_wallet_manager::WalletManager; @@ -430,9 +430,9 @@ mod tests { use dashcore::hashes::Hash; use dashcore::{Address as DashAddress, OutPoint, TxOut}; - use key_wallet::Utxo; - use key_wallet::wallet::Wallet; use key_wallet::wallet::initialization::WalletAccountCreationOptions; + use key_wallet::wallet::Wallet; + use key_wallet::Utxo; use tokio::sync::Notify; use crate::wallet::platform_wallet::PlatformWalletInfo; From 25bb3596788a2d3f6dca381c1cca477faa31beea Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 21 May 2026 15:35:29 +0200 Subject: [PATCH 23/33] fix(rs-platform-wallet-ffi): typed FFI codes for retryable errors + safe slice marshalling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CMT-002: map PlatformWalletError::NoSpendableInputs and ConcurrentSpendConflict to dedicated PlatformWalletFFIResultCode variants (30, 31) so Swift can branch on numeric codes instead of substring-matching messages. Mirrors the new cases in PlatformWalletResultCode + PlatformWalletError on the Swift side. The ConcurrentSpendConflict.selected: Vec payload is still stringified into the message field (TODO: propagate structured payload when a generic FFI sidecar exists). CMT-004: harden core_wallet_send_to_addresses input marshalling. slice::from_raw_parts requires a non-null aligned pointer even for len == 0, and Swift's empty Array.withUnsafeBufferPointer.baseAddress returns nil — guard the zero-count path. Also null-check every addr_ptrs[i] before CStr::from_ptr instead of trusting the caller. Tests cover both count==0 paths (null and non-null arrays) plus a null-element-inside-array case. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/core_wallet/broadcast.rs | 125 ++++++++++++++++-- packages/rs-platform-wallet-ffi/src/error.rs | 26 +++- .../PlatformWallet/PlatformWalletResult.swift | 19 +++ 3 files changed, 159 insertions(+), 11 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs index 24d54bb0776..f10d71848ea 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs @@ -70,17 +70,32 @@ pub unsafe extern "C" fn core_wallet_send_to_addresses( check_ptr!(out_tx_bytes); check_ptr!(out_tx_len); - let mut outputs = Vec::with_capacity(count); - let addr_ptrs = std::slice::from_raw_parts(addresses, count); - let amount_slice = std::slice::from_raw_parts(amounts, count); + // `std::slice::from_raw_parts` requires a non-null, properly + // aligned pointer even for `len == 0`. Swift's empty + // `Array.withUnsafeBufferPointer.baseAddress` returns `nil`, so + // the `count == 0` path is allowed to ship null `addresses` / + // `amounts` — guard against constructing the slice in that case. + let outputs: Vec<(dashcore::Address, u64)> = if count == 0 { + Vec::new() + } else { + let addr_ptrs = std::slice::from_raw_parts(addresses, count); + let amount_slice = std::slice::from_raw_parts(amounts, count); - for i in 0..count { - let c_str = unwrap_result_or_return!(std::ffi::CStr::from_ptr(addr_ptrs[i]).to_str()); - - let addr = unwrap_result_or_return!(dashcore::Address::from_str(c_str)).assume_checked(); - - outputs.push((addr, amount_slice[i])); - } + let mut outputs = Vec::with_capacity(count); + for i in 0..count { + if addr_ptrs[i].is_null() { + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorNullPointer, + format!("null address pointer at index {i}"), + ); + } + let c_str = unwrap_result_or_return!(std::ffi::CStr::from_ptr(addr_ptrs[i]).to_str()); + let addr = + unwrap_result_or_return!(dashcore::Address::from_str(c_str)).assume_checked(); + outputs.push((addr, amount_slice[i])); + } + outputs + }; use key_wallet::account::account_type::StandardAccountType; let std_account_type = match account_type { @@ -134,3 +149,93 @@ pub unsafe extern "C" fn core_wallet_free_tx_bytes(bytes: *mut u8, len: usize) { let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(bytes, len)); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::handle::NULL_HANDLE; + + /// `count == 0` MUST NOT touch `addresses` / `amounts` — Swift's + /// empty `Array.withUnsafeBufferPointer.baseAddress` gives `nil`, + /// and `slice::from_raw_parts` is UB on a null pointer regardless + /// of length. Pass `NULL_HANDLE` so the storage lookup short- + /// circuits to `NotFound` before any wallet code runs — we only + /// care that input marshalling did not blow up. + #[test] + fn send_to_addresses_zero_count_null_pointers_is_safe() { + // Use a non-null but fake signer pointer; the closure that + // would dereference it is never entered because `NULL_HANDLE` + // makes `with_item` return `None`. + let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let mut out_tx: *mut u8 = std::ptr::null_mut(); + let mut out_len: usize = 0; + + let result = unsafe { + core_wallet_send_to_addresses( + NULL_HANDLE, + 0, // BIP44Account + 0, + std::ptr::null(), // null addresses — allowed because count == 0 + std::ptr::null(), // null amounts — allowed because count == 0 + 0, + fake_signer, + &mut out_tx, + &mut out_len, + ) + }; + assert_eq!(result.code, PlatformWalletFFIResultCode::NotFound); + } + + /// Non-null `addresses` array with `count == 0` is also fine. + #[test] + fn send_to_addresses_zero_count_nonnull_pointers_is_safe() { + let dummy_addr: *const c_char = std::ptr::null(); + let dummy_amount: u64 = 0; + let addrs: [*const c_char; 1] = [dummy_addr]; + let amts: [u64; 1] = [dummy_amount]; + let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let mut out_tx: *mut u8 = std::ptr::null_mut(); + let mut out_len: usize = 0; + + let result = unsafe { + core_wallet_send_to_addresses( + NULL_HANDLE, + 0, + 0, + addrs.as_ptr(), + amts.as_ptr(), + 0, // count = 0 → array contents ignored + fake_signer, + &mut out_tx, + &mut out_len, + ) + }; + assert_eq!(result.code, PlatformWalletFFIResultCode::NotFound); + } + + /// A null entry inside the address array must surface as + /// `ErrorNullPointer`, not UB. + #[test] + fn send_to_addresses_null_element_is_rejected() { + let addrs: [*const c_char; 2] = [std::ptr::null(), std::ptr::null()]; + let amts: [u64; 2] = [1, 2]; + let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let mut out_tx: *mut u8 = std::ptr::null_mut(); + let mut out_len: usize = 0; + + let result = unsafe { + core_wallet_send_to_addresses( + NULL_HANDLE, + 0, + 0, + addrs.as_ptr(), + amts.as_ptr(), + 2, + fake_signer, + &mut out_tx, + &mut out_len, + ) + }; + assert_eq!(result.code, PlatformWalletFFIResultCode::ErrorNullPointer); + } +} diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index adbe771c8c0..9a65d703740 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -77,6 +77,21 @@ pub enum PlatformWalletFFIResultCode { ErrorMemoryAllocation = 11, ErrorUtf8Conversion = 12, + /// No spendable inputs available on the account — wait for sync / + /// new UTXOs and retry, or surface a depleted-wallet message. + /// Carries the typed [`PlatformWalletError::NoSpendableInputs`] + /// (account_type / account_index / context) stringified in the + /// `message` field. + ErrorNoSpendableInputs = 30, + /// Transaction builder selected an outpoint that another in-flight + /// build had already reserved — retryable. The originating + /// [`PlatformWalletError::ConcurrentSpendConflict`] carries a + /// `Vec` of the colliding inputs; that payload is + /// stringified into the `message` field for now (TODO: propagate + /// the structured outpoint list across the FFI when a generic + /// payload sidecar exists). + ErrorConcurrentSpendConflict = 31, + NotFound = 98, // Used exclusively for all the Option that are retuned as errors ErrorUnknown = 99, } @@ -156,7 +171,16 @@ impl From> for PlatformWalletFFIResult { impl From for PlatformWalletFFIResult { fn from(error: PlatformWalletError) -> Self { - PlatformWalletFFIResult::err(PlatformWalletFFIResultCode::ErrorUnknown, error.to_string()) + let code = match &error { + PlatformWalletError::NoSpendableInputs { .. } => { + PlatformWalletFFIResultCode::ErrorNoSpendableInputs + } + PlatformWalletError::ConcurrentSpendConflict { .. } => { + PlatformWalletFFIResultCode::ErrorConcurrentSpendConflict + } + _ => PlatformWalletFFIResultCode::ErrorUnknown, + }; + PlatformWalletFFIResult::err(code, error.to_string()) } } diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift index 199d07bd5e3..f1340a4fc20 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift @@ -18,6 +18,8 @@ public enum PlatformWalletResultCode: Int32, Sendable { case errorInvalidIdentifier = 10 case errorMemoryAllocation = 11 case errorUtf8Conversion = 12 + case errorNoSpendableInputs = 30 + case errorConcurrentSpendConflict = 31 case notFound = 98 case errorUnknown = 99 @@ -49,6 +51,10 @@ public enum PlatformWalletResultCode: Int32, Sendable { self = .errorMemoryAllocation case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_UTF8_CONVERSION: self = .errorUtf8Conversion + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_NO_SPENDABLE_INPUTS: + self = .errorNoSpendableInputs + case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_CONCURRENT_SPEND_CONFLICT: + self = .errorConcurrentSpendConflict case PLATFORM_WALLET_FFI_RESULT_CODE_NOT_FOUND: self = .notFound case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_UNKNOWN: @@ -124,6 +130,15 @@ public enum PlatformWalletError: LocalizedError { case serialization(String) case deserialization(String) case memoryAllocation(String) + /// No spendable inputs on the requested account — retryable after + /// sync, or surface a depleted-wallet message. Mirrors the Rust + /// `PlatformWalletError::NoSpendableInputs` variant. + case noSpendableInputs(String) + /// Transaction builder picked an outpoint another concurrent + /// build had already selected. Retry — the underlying reservation + /// will have cleared. Mirrors the Rust + /// `PlatformWalletError::ConcurrentSpendConflict` variant. + case concurrentSpendConflict(String) case notFound(String) case unknown(String) @@ -136,6 +151,7 @@ public enum PlatformWalletError: LocalizedError { .invalidIdentifier(let m), .invalidNetwork(let m), .walletOperation(let m), .identityNotFound(let m), .contactNotFound(let m), .utf8Conversion(let m), .serialization(let m), .deserialization(let m), .memoryAllocation(let m), + .noSpendableInputs(let m), .concurrentSpendConflict(let m), .notFound(let m), .unknown(let m): return m } @@ -160,6 +176,9 @@ public enum PlatformWalletError: LocalizedError { case .errorInvalidIdentifier: self = .invalidIdentifier(detail) case .errorMemoryAllocation: self = .memoryAllocation(detail) case .errorUtf8Conversion: self = .utf8Conversion(detail) + case .errorNoSpendableInputs: self = .noSpendableInputs(detail) + case .errorConcurrentSpendConflict: + self = .concurrentSpendConflict(detail) case .notFound: self = .notFound(detail) case .errorUnknown: self = .unknown(detail) } From c29b4ba6a4bd9993a4dbb20d0aed3624582a7d21 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Thu, 21 May 2026 15:42:10 +0200 Subject: [PATCH 24/33] fix(platform-wallet): close UTXO race in DashPay + harden change-index and post-broadcast paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CMT-001: `IdentityWallet::send_payment` now consults the shared `OutpointReservations` set used by `CoreWallet::send_to_addresses`, reserving its selected outpoints + change address under the same write lock and releasing them only after `check_core_transaction` reconciles the broadcast into wallet state. A concurrent core send + DashPay send on the same wallet can no longer pick the same UTXO. - CMT-003: `send_to_addresses` no longer releases the reservation unconditionally after a successful broadcast. If the post-broadcast reconcile path fails (wallet handle stale, or own-built tx not recognised by `check_core_transaction`), the reservation is leaked until a wallet restart / full sync; releasing it would let a concurrent caller re-select an already-broadcast outpoint and produce a double-spend the network would reject. Added a unit test (`unreconciled_broadcast_keeps_reservation_held`). - CMT-006: peek-without-commit change-address reuse closed by committing the change-index advance inside the same write lock as the outpoint reservation, and skipping past any change address still pending from a concurrent in-flight send. A single index per failure is burned — acceptable; reuse is not. - CMT-005: stronger deferral comment for the brittle coin-selection substring match (typed `BuilderError::CoinSelection(SelectionError::…)` upstream). Refs: PR #3585. Co-Authored-By: Bilby the Dev <842586+lklimek@users.noreply.github.com> --- .../src/wallet/core/broadcast.rs | 210 +++++++++++++----- .../rs-platform-wallet/src/wallet/core/mod.rs | 2 +- .../src/wallet/core/reservations.rs | 201 ++++++++++++++--- .../identity/network/identity_handle.rs | 8 + .../src/wallet/identity/network/payments.rs | 164 ++++++++++++-- .../src/wallet/platform_wallet.rs | 3 + 6 files changed, 486 insertions(+), 102 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index 9ac1e941029..bd4ba64d90f 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -64,7 +64,7 @@ impl CoreWallet { )); } - let (tx, xpub, _reservation) = { + let (tx, _reservation) = { let mut wm = self.wallet_manager.write().await; let (wallet, info) = wm.get_wallet_and_info_mut(&self.wallet_id).ok_or_else(|| { crate::error::PlatformWalletError::WalletNotFound( @@ -142,18 +142,41 @@ impl CoreWallet { }); } - // 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. - let change_addr = managed_account - .next_change_address(Some(&xpub), false) - .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + // Pick the next change address. Peek (advance=false) first; if the + // peeked address is already pending from a concurrent in-flight + // send (CMT-006), advance the derivation index and peek again + // until we find one that is not pending. The final chosen + // address is committed (advance=true) inside this same write + // lock and inserted into the reservation set so a concurrent + // caller can never select the same change address. Advancing + // burns at most one index per concurrent in-flight send — a + // bounded, acceptable cost for privacy. + let pending_change = self.reservations.pending_change_snapshot(); + let change_addr = loop { + let peeked = managed_account + .next_change_address(Some(&xpub), false) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + if !pending_change.contains(&peeked) { + // Commit the advance now (under the same write lock as + // the outpoint reservation below). On broadcast failure + // a single index is burned — acceptable; on success the + // pending-change entry is released when the guard drops + // post-`check_core_transaction`. + let _ = managed_account + .next_change_address(Some(&xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + break peeked; + } + // Burn this index by advancing past it and try again. + let _ = managed_account + .next_change_address(Some(&xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + }; let mut builder = TransactionBuilder::new() .set_current_height(current_height) .set_selection_strategy(SelectionStrategy::LargestFirst) - .set_change_address(change_addr) + .set_change_address(change_addr.clone()) .add_inputs(spendable.iter().cloned()); for (addr, amount) in &outputs { builder = builder.add_output(addr, *amount); @@ -165,9 +188,11 @@ impl CoreWallet { }) .await .map_err(|e| { - // Map coin-selection failures to `NoSpendableInputs`. The string-match is - // brittle against upstream rephrasing and is currently unpinned by tests. - // TODO(typed-wrapper): drop once upstream exposes `SelectionError` typed via BuilderError. + // TODO(CMT-005, #3585): Brittle substring match against upstream Display impl. + // Pinned key-wallet exposes typed BuilderError::CoinSelection(SelectionError::…) + // (InsufficientFunds, NoUtxosAvailable). Pattern-match the enum here once the + // classification can be threaded through `build_signed`'s error type. Deferred + // pending a focused refactor with test coverage for the typed variants. let msg = e.to_string(); if msg.contains("Insufficient funds") || msg.contains("No UTXOs available") { PlatformWalletError::NoSpendableInputs { @@ -215,11 +240,14 @@ impl CoreWallet { } // Reserve before releasing the lock so the next caller sees these outpoints - // filtered out. Guard held until `check_core_transaction` marks them spent - // (success) or the error unwinds (failure → outpoints released for retry). - let reservation = self.reservations.reserve(selected.into_iter().collect()); - - (tx, xpub, reservation) + // filtered out *and* skips the peeked change address. Guard held until + // `check_core_transaction` marks them spent (success) or the error + // unwinds (failure → outpoints released for retry). + let reservation = self + .reservations + .reserve(selected.into_iter().collect(), Some(change_addr.clone())); + + (tx, reservation) }; // Broadcast first — on error we leave wallet state untouched so the caller can retry. @@ -230,43 +258,25 @@ impl CoreWallet { // Mark inputs spent under the write lock, transitioning them from "reserved" to "spent" // before the reservation guard drops — no observable gap for concurrent callers. // Warning paths below do NOT return Err: the network already accepted the tx. + // + // CMT-003: the reservation is released *only* when both + // (a) the wallet lookup succeeded, and + // (b) `check_core_transaction` recognised the tx as relevant (i.e. it marked + // the inputs as spent in `managed_account.utxos`). + // If either fails, releasing the reservation would let a concurrent caller + // select the same UTXO and produce a double-spend on the network. The change + // index was already committed inside the build write lock above, so a single + // gap is acceptable — leak the reservation until a full sync reconciles. + let mut reconciled = false; { let mut wm = self.wallet_manager.write().await; if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&self.wallet_id) { - // Commit the change-address advance post-broadcast; doing it before would burn - // a derivation index on network rejection, widening the gap-limit window. - 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" - ); - } - } - let check_result = info .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) .await; - if !check_result.is_relevant { + if check_result.is_relevant { + reconciled = true; + } else { // Own-built tx unrecognised by our checker is an internal invariant // violation, not a transient. Stable event field for operator alerting. tracing::error!( @@ -290,9 +300,23 @@ impl CoreWallet { } } - // Explicit drop: inputs are already marked spent above; no gap between - // "reservation released" and "spent visible" to concurrent callers. - drop(_reservation); + if reconciled { + // Inputs are now marked spent; safe to release reservation. + _reservation.release_after_commit(); + } else { + // Broadcast succeeded but state could not be reconciled. Releasing the + // reservation now risks a concurrent send re-selecting the same UTXO and + // producing a double-spend the network would reject. Keep it held until + // a future sync reconciles — a wallet restart is the eventual relief. + tracing::warn!( + target: "platform_wallet::broadcast", + event = "post_broadcast_reservation_leaked_until_sync", + txid = %tx.txid(), + wallet_id = %hex::encode(self.wallet_id), + "leaking outpoint reservation: post-broadcast reconciliation failed" + ); + _reservation.leak_until_sync(); + } Ok(tx) } @@ -808,6 +832,88 @@ mod tests { } } + /// CMT-003: if `check_core_transaction` returns `is_relevant = false` + /// after a successful broadcast (an internal invariant violation but a + /// real-world possibility on a corrupted/stale wallet state), the + /// reservation must stay held — releasing it could let a concurrent + /// caller select the same already-broadcast outpoint and produce a + /// double-spend the network would reject. + /// + /// We force `is_relevant = false` by clearing the funding account's + /// address-pool entries between the broadcast and the reconcile step; + /// the post-broadcast `check_core_transaction` then matches nothing. + #[tokio::test] + async fn unreconciled_broadcast_keeps_reservation_held() { + use key_wallet::account::account_type::StandardAccountType; + + let (wm, wallet_id, recipient, signer) = build_funded_wallet_manager(2_000_000); + let signer = Arc::new(signer); + + let gate = Arc::new(Notify::new()); + let entered = Arc::new(Notify::new()); + let broadcaster = Arc::new(GatedBroadcaster { + gate: Arc::clone(&gate), + entered: Arc::clone(&entered), + calls: AtomicUsize::new(0), + succeed: true, + }); + + let core = make_core_wallet_for_manager( + Arc::clone(&wm), + wallet_id, + Arc::clone(&broadcaster) as Arc, + ); + + let outputs = vec![(recipient.clone(), 100_000)]; + + let core_send = core.clone(); + let signer_send = Arc::clone(&signer); + let handle = tokio::spawn(async move { + core_send + .send_to_addresses( + StandardAccountType::BIP44Account, + 0, + outputs, + signer_send.as_ref(), + ) + .await + }); + + // Wait until the broadcast call has been entered — by then the + // outpoint is reserved, the change index is committed, and the + // wallet write lock has been released. + entered.notified().await; + + // Sabotage the wallet so `check_core_transaction` cannot match + // anything. Clearing the account-collection map entirely is the + // surest way: with no accounts the checker walks zero entries. + { + let mut wm_guard = wm.write().await; + let info = wm_guard.get_wallet_info_mut(&wallet_id).expect("info"); + info.core_wallet.accounts.standard_bip44_accounts.clear(); + } + + // Capture the reservation's outpoint *before* releasing the gate + // so the assertion target is stable. + let funding_outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); + + // Release the broadcast — the post-broadcast reconcile sees + // `is_relevant=false` and (per CMT-003) leaks the reservation. + gate.notify_one(); + + let result = handle.await.expect("task panicked"); + assert!( + result.is_ok(), + "send_to_addresses must succeed when broadcast succeeds; got: {result:?}" + ); + + assert!( + core.reservations.contains(&funding_outpoint), + "reservation must remain held when post-broadcast reconcile fails (is_relevant=false), \ + otherwise a concurrent caller could re-select the same already-broadcast outpoint" + ); + } + /// Pins the early-exit guard: when the spendable snapshot is empty /// (e.g. all UTXOs reserved by in-flight broadcasts), `send_to_addresses` /// surfaces `NoSpendableInputs` without invoking the builder. @@ -827,7 +933,7 @@ mod tests { // Reserve the wallet's only outpoint so the spendable snapshot is // empty for the next caller, exercising the early-exit guard. let outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); - let _guard = core.reservations.reserve(vec![outpoint]); + let _guard = core.reservations.reserve(vec![outpoint], None); let result = core .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs, &signer) diff --git a/packages/rs-platform-wallet/src/wallet/core/mod.rs b/packages/rs-platform-wallet/src/wallet/core/mod.rs index e068dfacb4d..d53ea482fb9 100644 --- a/packages/rs-platform-wallet/src/wallet/core/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/core/mod.rs @@ -1,7 +1,7 @@ pub mod balance; pub mod balance_handler; mod broadcast; -mod reservations; +pub(crate) mod reservations; pub mod wallet; pub use balance::WalletBalance; diff --git a/packages/rs-platform-wallet/src/wallet/core/reservations.rs b/packages/rs-platform-wallet/src/wallet/core/reservations.rs index 070c60e96a3..6c8461c48b7 100644 --- a/packages/rs-platform-wallet/src/wallet/core/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/core/reservations.rs @@ -8,16 +8,35 @@ use std::collections::HashSet; use std::sync::{Arc, Mutex}; -use dashcore::OutPoint; +use dashcore::{Address, OutPoint}; + +/// Inner state shared between an [`OutpointReservations`] handle and every +/// outstanding [`OutpointReservationGuard`]. Held behind a single `Mutex` +/// so reservation + change-address tracking commit atomically. +#[derive(Debug, Default)] +struct ReservationsInner { + outpoints: HashSet, + /// Change addresses already committed (`advance=true`) by an + /// in-flight `send_to_addresses` whose broadcast has not yet + /// completed. Concurrent senders that peek a change address still + /// present here advance past it under the same write lock so two + /// disjoint-UTXO sends do not both broadcast with the same change + /// address (privacy regression — CMT-006). Address-keyed rather than + /// `(account, index)` because the upstream pool API exposes addresses + /// but not indices. + pending_change: HashSet
, +} /// Per-wallet set of outpoints that have been selected for an in-flight -/// broadcast but not yet marked spent in `ManagedWalletInfo`. +/// broadcast but not yet marked spent in `ManagedWalletInfo`, plus any +/// change addresses peeked but not yet reconciled with a confirmed +/// broadcast. /// /// Cheaply cloneable: holds an `Arc>` internally. All clones share /// the same set. #[derive(Debug, Default, Clone)] pub(crate) struct OutpointReservations { - inner: Arc>>, + inner: Arc>, } impl OutpointReservations { @@ -32,38 +51,66 @@ impl OutpointReservations { .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.contains(outpoint) + guard.outpoints.contains(outpoint) } - /// Clone the current reservation set under a single lock acquisition. - /// - /// Callers filter spendable UTXOs against the returned snapshot to - /// avoid one mutex lock per candidate outpoint. + /// Test whether a change address is currently pending. + #[cfg(test)] + pub(crate) fn change_address_pending(&self, addr: &Address) -> bool { + let guard = self + .inner + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + guard.pending_change.contains(addr) + } + + /// Clone the current outpoint reservation set under a single lock + /// acquisition. Callers filter spendable UTXOs against the returned + /// snapshot to avoid one mutex lock per candidate outpoint. pub(crate) fn snapshot(&self) -> HashSet { let guard = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.clone() + guard.outpoints.clone() + } + + /// Clone the current pending-change-address set so callers can skip + /// past in-flight peeks without holding the reservation mutex. + pub(crate) fn pending_change_snapshot(&self) -> HashSet
{ + let guard = self + .inner + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + guard.pending_change.clone() } - /// Reserve `outpoints`, returning an RAII guard that releases them on - /// drop. The guard must be held until the broadcast outcome is - /// reconciled into wallet state (success → `check_core_transaction` - /// has run; failure → caller has propagated the error). - pub(crate) fn reserve(&self, outpoints: Vec) -> OutpointReservationGuard { + /// Reserve `outpoints` and (optionally) a chosen change address in + /// the same lock acquisition, returning an RAII guard that releases + /// both on drop. The guard must be held until the broadcast outcome + /// is reconciled into wallet state. + pub(crate) fn reserve( + &self, + outpoints: Vec, + change_address: Option
, + ) -> OutpointReservationGuard { { let mut guard = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); for op in &outpoints { - guard.insert(*op); + guard.outpoints.insert(*op); + } + if let Some(addr) = &change_address { + guard.pending_change.insert(addr.clone()); } } OutpointReservationGuard { reservations: Arc::clone(&self.inner), outpoints, + pending_change: change_address, + released: false, } } } @@ -71,25 +118,72 @@ impl OutpointReservations { /// RAII guard releasing reservations on drop. /// /// Drop is infallible and panic-safe — the underlying `Mutex` is recovered -/// from poisoning so a panicking caller still releases its outpoints. +/// from poisoning so a panicking caller still releases its outpoints +/// and pending change index (if any). #[must_use = "dropping the guard immediately releases the reservation"] pub(crate) struct OutpointReservationGuard { - reservations: Arc>>, + reservations: Arc>, outpoints: Vec, + /// Pending change address reserved for this in-flight send (if any). + pending_change: Option
, + /// Set after a successful `release_after_commit` so `Drop` is a no-op. + released: bool, } -impl Drop for OutpointReservationGuard { - fn drop(&mut self) { - let mut guard = self +impl OutpointReservationGuard { + /// Release outpoints and any pending change index now, marking the + /// guard inert so its `Drop` is a no-op. Called by the broadcast + /// path after `check_core_transaction` has transitioned the inputs + /// from "reserved" to "spent" and the change index has been + /// committed via `next_change_address(..., true)`. The deliberate + /// release point exists so the same code path that *succeeded* the + /// broadcast also relinquishes the reservation — separating it from + /// the panic/drop path keeps post-broadcast-failure handling + /// (CMT-003) on the implicit `Drop` branch. + pub(crate) fn release_after_commit(mut self) { + self.do_release(); + self.released = true; + } + + /// Keep the reservation held for the lifetime of the process by + /// leaking the guard. Use this when the broadcast succeeded but + /// wallet state could not be reconciled (e.g., own-built tx not + /// recognised by `check_core_transaction`, or the wallet handle + /// went stale post-broadcast). Releasing the outpoints in that + /// scenario would let a concurrent caller select the same UTXO and + /// produce a double-spend the network would reject — keeping the + /// reservation is the safer of two bad outcomes; a wallet restart + /// or full sync will reconcile. + pub(crate) fn leak_until_sync(self) { + // `Box::leak` is the standard way to drop the ownership without + // running `Drop`. We don't actually heap-allocate — `mem::forget` + // is equivalent and avoids the allocation. + std::mem::forget(self); + } + + fn do_release(&mut self) { + let mut inner = self .reservations .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); for op in &self.outpoints { - guard.remove(op); + inner.outpoints.remove(op); + } + if let Some(addr) = self.pending_change.take() { + inner.pending_change.remove(&addr); } } } +impl Drop for OutpointReservationGuard { + fn drop(&mut self) { + if self.released { + return; + } + self.do_release(); + } +} + #[cfg(test)] mod tests { use super::*; @@ -100,12 +194,21 @@ mod tests { OutPoint::new(Txid::all_zeros(), n) } + fn addr(byte: u8) -> Address { + use dashcore::secp256k1::{PublicKey, Secp256k1, SecretKey}; + let secp = Secp256k1::new(); + let sk = SecretKey::from_slice(&[byte; 32]).expect("valid sk"); + let pk = PublicKey::from_secret_key(&secp, &sk); + let cpk = dashcore::PublicKey::new(pk); + Address::p2pkh(&cpk, dashcore::Network::Testnet) + } + #[test] fn reserve_then_drop_releases() { let res = OutpointReservations::new(); let a = op(1); { - let _g = res.reserve(vec![a]); + let _g = res.reserve(vec![a], None); assert!(res.contains(&a)); } assert!(!res.contains(&a)); @@ -116,8 +219,8 @@ mod tests { let res = OutpointReservations::new(); let a = op(1); let b = op(2); - let _g1 = res.reserve(vec![a]); - let _g2 = res.reserve(vec![b]); + let _g1 = res.reserve(vec![a], None); + let _g2 = res.reserve(vec![b], None); assert!(res.contains(&a)); assert!(res.contains(&b)); } @@ -128,7 +231,7 @@ mod tests { let a = op(7); let res_clone = res.clone(); let _ = std::thread::spawn(move || { - let _g = res_clone.reserve(vec![a]); + let _g = res_clone.reserve(vec![a], None); panic!("intentional"); }) .join(); @@ -136,4 +239,52 @@ mod tests { // though the mutex was poisoned. assert!(!res.contains(&a)); } + + #[test] + fn change_address_reserved_and_released_on_drop() { + let res = OutpointReservations::new(); + let ch = addr(0x42); + { + let _g = res.reserve(vec![op(1)], Some(ch.clone())); + assert!(res.change_address_pending(&ch)); + } + assert!(!res.change_address_pending(&ch)); + } + + #[test] + fn pending_change_snapshot_reflects_reservations() { + let res = OutpointReservations::new(); + let ch1 = addr(0x11); + let ch2 = addr(0x22); + let _g1 = res.reserve(vec![op(1)], Some(ch1.clone())); + let _g2 = res.reserve(vec![op(2)], Some(ch2.clone())); + let snap = res.pending_change_snapshot(); + assert!(snap.contains(&ch1)); + assert!(snap.contains(&ch2)); + } + + #[test] + fn release_after_commit_is_drop_noop() { + let res = OutpointReservations::new(); + let a = op(11); + let ch = addr(0x55); + let g = res.reserve(vec![a], Some(ch.clone())); + assert!(res.contains(&a)); + assert!(res.change_address_pending(&ch)); + g.release_after_commit(); + assert!(!res.contains(&a)); + assert!(!res.change_address_pending(&ch)); + } + + #[test] + fn leak_until_sync_keeps_reservation_held() { + let res = OutpointReservations::new(); + let a = op(13); + let g = res.reserve(vec![a], None); + g.leak_until_sync(); + assert!( + res.contains(&a), + "leak_until_sync must keep the outpoint reserved until process restart" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs b/packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs index 83a94d30117..268c56f89d1 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs @@ -274,6 +274,13 @@ pub struct IdentityWallet { /// `SpvBroadcaster`-pinned, while this one picks the broadcaster /// used by `send_payment` (static dispatch per call). pub(crate) broadcaster: Arc, + /// Shared outpoint reservation set (cloned from the sibling + /// [`crate::CoreWallet`]). DashPay `send_payment` and core + /// `send_to_addresses` both fund from the same BIP-44 account 0 + /// UTXOs, so they must consult the same reservation set to avoid + /// the same-UTXO concurrent-selection race (CMT-001). See + /// [`crate::wallet::core::reservations`]. + pub(crate) reservations: crate::wallet::core::reservations::OutpointReservations, } // Manual `Debug`: the derive would require `B: Debug`, which is not part @@ -296,6 +303,7 @@ impl Clone for IdentityWallet { asset_locks: Arc::clone(&self.asset_locks), persister: self.persister.clone(), broadcaster: Arc::clone(&self.broadcaster), + reservations: self.reservations.clone(), } } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index c135e04e6fc..4fed7fbca6c 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -103,6 +103,9 @@ impl IdentityWallet { ), PlatformWalletError, > { + use std::collections::BTreeSet; + + use dashcore::OutPoint; use key_wallet::account::account_collection::DashpayAccountKey; use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionStrategy; use key_wallet::wallet::managed_wallet_info::transaction_builder::TransactionBuilder; @@ -110,16 +113,14 @@ impl IdentityWallet { let account_index: u32 = 0; - let (payment_address, tx) = { + // Build, sign, reserve — all under one write-lock acquisition. Mirrors + // `CoreWallet::send_to_addresses` so concurrent calls between this and + // core `send_to_addresses` cannot select the same UTXO (CMT-001 / #3585). + let (payment_address, tx, _reservation) = { let mut wm = self.wallet_manager.write().await; // Resolve the external account's xpub so we can derive addresses. let contact_xpub = { - // Look up the external account in the *immutable* AccountCollection on - // `Wallet`. The ManagedAccountCollection only stores the managed state; - // the xpub lives on the immutable Account in `wallet.accounts`. - // For a watch-only external account we stored the contact's xpub directly - // as `account_xpub` on the Account struct — look it up via DashpayAccountKey. let wallet = wm.get_wallet(&self.wallet_id).ok_or_else(|| { PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)) })?; @@ -145,7 +146,74 @@ impl IdentityWallet { .get_wallet_and_info_mut(&self.wallet_id) .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; - // Derive the next unused address from the external account's address pool. + // Resolve the funding-account xpub up front so we can advance the + // change-address derivation under the same lock. + let funding_xpub = wallet + .accounts + .standard_bip44_accounts + .get(&0) + .map(|a| a.account_xpub) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild( + "BIP-44 account 0 not found in wallet".to_string(), + ) + })?; + + let current_height = info.core_wallet.synced_height(); + + let managed_account = info + .core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&0) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild( + "BIP-44 managed account 0 not found".to_string(), + ) + })?; + + // Snapshot spendable UTXOs minus any in-flight reservations from + // a concurrent `send_to_addresses`/`send_payment` on this wallet. + let reserved = self.reservations.snapshot(); + let spendable: Vec<_> = managed_account + .spendable_utxos(current_height) + .into_iter() + .filter(|utxo| !reserved.contains(&utxo.outpoint)) + .cloned() + .collect(); + if spendable.is_empty() { + return Err(PlatformWalletError::NoSpendableInputs { + account_index, + account_type: + key_wallet::account::account_type::StandardAccountType::BIP44Account, + context: "all UTXOs used or reserved by in-flight transactions".to_string(), + }); + } + + // Pick a change address that no concurrent send has already + // peeked. Commit the advance inside this write lock so the + // privacy property holds even if broadcast fails later (one + // burned index per failure is acceptable; reuse is not). + let pending_change = self.reservations.pending_change_snapshot(); + let change_addr = loop { + let peeked = managed_account + .next_change_address(Some(&funding_xpub), false) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + if !pending_change.contains(&peeked) { + let _ = managed_account + .next_change_address(Some(&funding_xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + break peeked; + } + let _ = managed_account + .next_change_address(Some(&funding_xpub), true) + .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + }; + + // Derive the recipient's payment address from the external pool. + // Done *after* the change-address pick so a derivation failure + // doesn't leave a committed funding-side change advance dangling + // without a matching outpoint reservation. let key = DashpayAccountKey { index: account_index, user_identity_id: from_identity_id.to_buffer(), @@ -162,13 +230,12 @@ impl IdentityWallet { to_contact_id )) })?; - let payment_address = external_account .next_address(Some(&contact_xpub), true) .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; - let current_height = info.core_wallet.synced_height(); - + // Re-borrow the managed funding account for the builder (the + // external_account borrow above ended at `payment_address`). let managed_account = info .core_wallet .accounts @@ -179,20 +246,12 @@ impl IdentityWallet { "BIP-44 managed account 0 not found".to_string(), ) })?; - let account = wallet - .accounts - .standard_bip44_accounts - .get(&0) - .ok_or_else(|| { - PlatformWalletError::TransactionBuild( - "BIP-44 account 0 not found in wallet".to_string(), - ) - })?; let builder = TransactionBuilder::new() .set_current_height(current_height) .set_selection_strategy(SelectionStrategy::LargestFirst) - .set_funding(managed_account, account) + .set_change_address(change_addr.clone()) + .add_inputs(spendable.iter().cloned()) .add_output(&payment_address, amount_duffs); let (tx, _fee) = builder @@ -202,10 +261,26 @@ impl IdentityWallet { .await .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; - (payment_address, tx) + // Defense-in-depth: confirm the builder picked only outpoints + // from our pre-filtered spendable snapshot. + let selected: BTreeSet = + tx.input.iter().map(|txin| txin.previous_output).collect(); + let spendable_outpoints: BTreeSet = + spendable.iter().map(|utxo| utxo.outpoint).collect(); + if !selected.is_subset(&spendable_outpoints) { + return Err(PlatformWalletError::ConcurrentSpendConflict { + selected: selected.into_iter().collect(), + }); + } + + let reservation = self + .reservations + .reserve(selected.into_iter().collect(), Some(change_addr)); + + (payment_address, tx, reservation) }; - // --- 3. Broadcast the transaction. --- + // --- 3. Broadcast the transaction (lock released). --- let txid = self .broadcaster .broadcast(&tx) @@ -221,15 +296,35 @@ impl IdentityWallet { "DashPay payment broadcast" ); - // --- 4. Record the outgoing payment on the sender's ManagedIdentity. --- + // --- 4. Record the outgoing payment + reconcile UTXO state. --- let entry = crate::wallet::identity::types::dashpay::payment::PaymentEntry::new_sent( *to_contact_id, amount_duffs, memo, ); + let mut reconciled = false; { let mut wm = self.wallet_manager.write().await; - if let Some(info) = wm.get_wallet_info_mut(&self.wallet_id) { + if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&self.wallet_id) { + // Transition inputs from "reserved" → "spent" via the same + // post-broadcast checker the core path uses, so the + // reservation can be safely released below. + use key_wallet::transaction_checking::{ + TransactionContext, WalletTransactionChecker, + }; + let check_result = info + .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) + .await; + reconciled = check_result.is_relevant; + if !reconciled { + tracing::error!( + target: "platform_wallet::broadcast", + event = "post_broadcast_unrelated_to_own_wallet", + txid = %txid, + wallet_id = %hex::encode(self.wallet_id), + "Internal invariant violation: own-built DashPay payment not recognized by post-broadcast check" + ); + } if let Some(managed) = info.identity_manager.managed_identity_mut(from_identity_id) { managed.record_dashpay_payment( @@ -238,9 +333,30 @@ impl IdentityWallet { &self.persister, ); } + } else { + tracing::warn!( + target: "platform_wallet::broadcast", + event = "post_broadcast_wallet_missing", + wallet_id = %hex::encode(self.wallet_id), + %txid, + "wallet missing during post-broadcast DashPay payment registration" + ); } } + if reconciled { + _reservation.release_after_commit(); + } else { + tracing::warn!( + target: "platform_wallet::broadcast", + event = "post_broadcast_reservation_leaked_until_sync", + %txid, + wallet_id = %hex::encode(self.wallet_id), + "leaking outpoint reservation: DashPay post-broadcast reconciliation failed" + ); + _reservation.leak_until_sync(); + } + Ok((txid, entry)) } } diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs index dcd9486798e..f5d4b11d90a 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs @@ -267,6 +267,9 @@ impl PlatformWallet { asset_locks: Arc::clone(&asset_locks), persister: wallet_persister.clone(), broadcaster: dashpay_broadcaster, + // Shared with `CoreWallet::send_to_addresses` so DashPay + // payments do not race the same UTXO. See CMT-001 / #3585. + reservations: core.reservations.clone(), }; let platform = PlatformAddressWallet::new( From 8de859a64c44f25af3c91c87121a00607dbfb87e Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Mon, 25 May 2026 14:44:01 +0200 Subject: [PATCH 25/33] =?UTF-8?q?fix(rs-platform-wallet):=20typed=20Builde?= =?UTF-8?q?rError=20=E2=86=92=20PlatformWalletError=20mapper=20(CMT-001,?= =?UTF-8?q?=20CMT-004)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace brittle `e.to_string()` substring matches in `send_to_addresses` and `send_payment` with a typed `classify_build_error` helper that pattern-matches `BuilderError::InsufficientFunds` and `BuilderError::CoinSelection(SelectionError::{NoUtxosAvailable, InsufficientFunds})` into the retryable `PlatformWalletError::NoSpendableInputs`. Every other build failure falls through to fatal `PlatformWalletError::TransactionBuild` with the upstream Display text preserved in `context`. Unit-pinned with six assertions over the typed variants so a future upstream rewording of the `Display` impl cannot silently downgrade `NoSpendableInputs` back to `TransactionBuild`. Drops the stale `TODO(CMT-005, #3585)` comment. Co-Authored-By: Claude Opus 4.6 --- .../src/wallet/core/broadcast.rs | 160 ++++++++++++++++-- .../rs-platform-wallet/src/wallet/core/mod.rs | 2 +- .../src/wallet/identity/network/payments.rs | 8 +- 3 files changed, 151 insertions(+), 19 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index bd4ba64d90f..c34c19bb7ac 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -5,11 +5,50 @@ use key_wallet::account::account_type::StandardAccountType; use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; use key_wallet::signer::Signer; use key_wallet::transaction_checking::{TransactionContext, WalletTransactionChecker}; +use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionError; +use key_wallet::wallet::managed_wallet_info::transaction_builder::BuilderError; use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use crate::broadcaster::TransactionBroadcaster; use crate::{CoreWallet, PlatformWalletError}; +/// Map a key-wallet [`BuilderError`] from `TransactionBuilder::build_signed` into a +/// [`PlatformWalletError`], distinguishing depleted-UTXO conditions (retryable +/// [`PlatformWalletError::NoSpendableInputs`]) from every other build failure +/// (fatal [`PlatformWalletError::TransactionBuild`]). +/// +/// The variants treated as "no spendable inputs" are: +/// +/// * [`BuilderError::InsufficientFunds`] — the builder's own pre-selection +/// shortfall check. +/// * [`BuilderError::CoinSelection`] wrapping either +/// [`SelectionError::NoUtxosAvailable`] or +/// [`SelectionError::InsufficientFunds`] — the coin-selector's two +/// depleted-UTXO outcomes. +/// +/// `account_type` and `account_index` are threaded through unchanged so the +/// resulting `NoSpendableInputs` carries the same operator-facing context the +/// upstream call sites already populate. +pub(crate) fn classify_build_error( + err: BuilderError, + account_type: StandardAccountType, + account_index: u32, +) -> PlatformWalletError { + let context = err.to_string(); + match err { + BuilderError::InsufficientFunds { .. } + | BuilderError::CoinSelection(SelectionError::NoUtxosAvailable) + | BuilderError::CoinSelection(SelectionError::InsufficientFunds { .. }) => { + PlatformWalletError::NoSpendableInputs { + account_type, + account_index, + context, + } + } + _ => PlatformWalletError::TransactionBuild(context), + } +} + impl CoreWallet { /// Broadcast a signed transaction to the network. /// @@ -187,23 +226,7 @@ impl CoreWallet { managed_account.address_derivation_path(&addr) }) .await - .map_err(|e| { - // TODO(CMT-005, #3585): Brittle substring match against upstream Display impl. - // Pinned key-wallet exposes typed BuilderError::CoinSelection(SelectionError::…) - // (InsufficientFunds, NoUtxosAvailable). Pattern-match the enum here once the - // classification can be threaded through `build_signed`'s error type. Deferred - // pending a focused refactor with test coverage for the typed variants. - let msg = e.to_string(); - if msg.contains("Insufficient funds") || msg.contains("No UTXOs available") { - PlatformWalletError::NoSpendableInputs { - account_type, - account_index, - context: msg, - } - } else { - PlatformWalletError::TransactionBuild(msg) - } - })?; + .map_err(|e| classify_build_error(e, account_type, account_index))?; // Defense-in-depth: unreachable under normal builder contract but guards against // a future regression where coin selection picks an outpoint outside `spendable`. @@ -944,4 +967,107 @@ mod tests { "send_to_addresses must map a fully-reserved wallet to NoSpendableInputs; got: {result:?}" ); } + + // ---- classify_build_error: typed BuilderError → PlatformWalletError ---- + // + // The mapper replaces the previous brittle Display-substring match + // (CMT-001 / CMT-004). These tests pin the typed contract directly so + // a future rename or rewording of the upstream `Display` impl cannot + // silently downgrade `NoSpendableInputs` back to `TransactionBuild`. + + use super::classify_build_error; + use key_wallet::account::account_type::StandardAccountType; + use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionError; + use key_wallet::wallet::managed_wallet_info::transaction_builder::BuilderError; + + #[test] + fn classify_builder_insufficient_funds_maps_to_no_spendable_inputs() { + let err = BuilderError::InsufficientFunds { + available: 1_000, + required: 10_000, + }; + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 7); + match mapped { + PlatformWalletError::NoSpendableInputs { + account_type, + account_index, + context, + } => { + assert!(matches!(account_type, StandardAccountType::BIP44Account)); + assert_eq!(account_index, 7); + assert!( + context.contains("Insufficient funds"), + "context should preserve the upstream Display; got: {context}" + ); + } + other => panic!("expected NoSpendableInputs, got: {other:?}"), + } + } + + #[test] + fn classify_coin_selection_no_utxos_available_maps_to_no_spendable_inputs() { + let err = BuilderError::CoinSelection(SelectionError::NoUtxosAvailable); + let mapped = classify_build_error(err, StandardAccountType::BIP32Account, 3); + match mapped { + PlatformWalletError::NoSpendableInputs { + account_type, + account_index, + .. + } => { + assert!(matches!(account_type, StandardAccountType::BIP32Account)); + assert_eq!(account_index, 3); + } + other => panic!("expected NoSpendableInputs, got: {other:?}"), + } + } + + #[test] + fn classify_coin_selection_insufficient_funds_maps_to_no_spendable_inputs() { + let err = BuilderError::CoinSelection(SelectionError::InsufficientFunds { + available: 5, + required: 100, + }); + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 0); + assert!( + matches!(mapped, PlatformWalletError::NoSpendableInputs { .. }), + "CoinSelection(InsufficientFunds) must map to NoSpendableInputs; got: {mapped:?}" + ); + } + + #[test] + fn classify_no_inputs_falls_through_to_transaction_build() { + let err = BuilderError::NoInputs; + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 0); + assert!( + matches!(mapped, PlatformWalletError::TransactionBuild(_)), + "non-depleted BuilderError must fall through to TransactionBuild; got: {mapped:?}" + ); + } + + #[test] + fn classify_signing_failed_falls_through_to_transaction_build() { + let err = BuilderError::SigningFailed("bad signer".to_string()); + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 0); + match mapped { + PlatformWalletError::TransactionBuild(msg) => { + assert!( + msg.contains("bad signer"), + "context should preserve message" + ); + } + other => panic!("expected TransactionBuild, got: {other:?}"), + } + } + + #[test] + fn classify_coin_selection_failed_falls_through_to_transaction_build() { + let err = + BuilderError::CoinSelection(SelectionError::SelectionFailed("wraparound".to_string())); + let mapped = classify_build_error(err, StandardAccountType::BIP44Account, 0); + assert!( + matches!(mapped, PlatformWalletError::TransactionBuild(_)), + "CoinSelection(SelectionFailed) is not a depleted-UTXO outcome; \ + must fall through to TransactionBuild; got: {mapped:?}" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/core/mod.rs b/packages/rs-platform-wallet/src/wallet/core/mod.rs index d53ea482fb9..48fcfbfd8d8 100644 --- a/packages/rs-platform-wallet/src/wallet/core/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/core/mod.rs @@ -1,6 +1,6 @@ pub mod balance; pub mod balance_handler; -mod broadcast; +pub(crate) mod broadcast; pub(crate) mod reservations; pub mod wallet; diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index 4fed7fbca6c..ab44e23a97c 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -259,7 +259,13 @@ impl IdentityWallet { managed_account.address_derivation_path(&addr) }) .await - .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; + .map_err(|e| { + crate::wallet::core::broadcast::classify_build_error( + e, + key_wallet::account::account_type::StandardAccountType::BIP44Account, + account_index, + ) + })?; // Defense-in-depth: confirm the builder picked only outpoints // from our pre-filtered spendable snapshot. From 4d017b8ac144774937a06080d4cb119a68c4618e Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Mon, 25 May 2026 14:44:26 +0200 Subject: [PATCH 26/33] docs(rs-platform-wallet): correct leak_until_sync docstring to match pinned behavior (CMT-002) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous docstring claimed "a wallet restart or full sync will reconcile" the leaked reservation, but no in-process reclaim API exists today — `leak_until_sync` calls `mem::forget(self)` and the outpoints stay pinned in `OutpointReservations` until the process exits. The unit test `leak_until_sync_keeps_reservation_held` pins exactly that behavior. Rewrite the doc to plainly state "held until process restart", keep the safer-of-two-bad-outcomes rationale, and add a follow-up TODO referencing @thepastaclaw's PR #3585 review for a future `OutpointReservations::reclaim_confirmed` API hooked into periodic sync. No behaviour change — docs only. Co-Authored-By: Claude Opus 4.6 --- .../src/wallet/core/reservations.rs | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/reservations.rs b/packages/rs-platform-wallet/src/wallet/core/reservations.rs index 6c8461c48b7..2d869e12580 100644 --- a/packages/rs-platform-wallet/src/wallet/core/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/core/reservations.rs @@ -145,19 +145,30 @@ impl OutpointReservationGuard { self.released = true; } - /// Keep the reservation held for the lifetime of the process by - /// leaking the guard. Use this when the broadcast succeeded but - /// wallet state could not be reconciled (e.g., own-built tx not - /// recognised by `check_core_transaction`, or the wallet handle - /// went stale post-broadcast). Releasing the outpoints in that - /// scenario would let a concurrent caller select the same UTXO and - /// produce a double-spend the network would reject — keeping the - /// reservation is the safer of two bad outcomes; a wallet restart - /// or full sync will reconcile. + /// Keep the reservation held until the process exits. + /// + /// Use this when the broadcast succeeded but wallet state could not be + /// reconciled (e.g., own-built tx not recognised by + /// `check_core_transaction`, or the wallet handle went stale + /// post-broadcast). Releasing the outpoints in that scenario would let a + /// concurrent caller select the same already-broadcast UTXO and produce + /// a double-spend the network would reject — keeping the reservation is + /// the safer of two bad outcomes. + /// + /// **No in-process reclaim path exists today.** The outpoints stay + /// pinned in [`OutpointReservations`] for the lifetime of the + /// `PlatformWalletManager`; only a wallet-process restart (which drops + /// the whole reservations set) releases them. + /// + /// TODO(@thepastaclaw, PR #3585): a periodic-sync-driven + /// `OutpointReservations::reclaim_confirmed(&[OutPoint])` API would let + /// the next confirmation pass reconcile leaked reservations without a + /// restart. Tracked on the PR review thread. pub(crate) fn leak_until_sync(self) { - // `Box::leak` is the standard way to drop the ownership without - // running `Drop`. We don't actually heap-allocate — `mem::forget` - // is equivalent and avoids the allocation. + // `mem::forget` skips `Drop`, leaving the reservation entries in + // the shared set. The guard's only owned heap allocation is the + // `Vec`, which is small and never freed for the lifetime + // of the process — acceptable given this is a rare error branch. std::mem::forget(self); } From cd2ade5e9cd4aff7058595e168fd697d40918805 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Mon, 25 May 2026 14:50:47 +0200 Subject: [PATCH 27/33] refactor(rs-platform-wallet): factor shared send-flow helper for CoreWallet/IdentityWallet (CMT-003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract `broadcast_and_reconcile` — the broadcast → check_core_transaction → release-or-leak decision tree shared by `CoreWallet::send_to_addresses` and `IdentityWallet::send_payment`. The path-specific bits stay inlined at each call site (UTXO snapshot/filter, change-address peek-burn-commit loop, builder construction, post-broadcast reservation reconcile target account layout) — the helper covers only the genuinely shared invariant code where divergence would cause double-spend or leaked-reservation bugs. A `FnOnce(&mut PlatformWalletInfo, &Txid, bool /*reconciled*/)` hook runs inside the post-broadcast write lock so call-site-specific bookkeeping (e.g. `record_dashpay_payment`) sees the same locked critical section as the reconcile. Pin the helper's three invariants directly with new unit tests: * `broadcast_and_reconcile_drops_reservation_on_broadcast_failure` — the hook is NOT invoked and the reservation is released so the caller can retry. * `broadcast_and_reconcile_leaks_when_wallet_missing` — stale wallet handle leaks the reservation (no concurrent re-selection of an already-broadcast outpoint). * `send_to_addresses_success_invokes_hook_and_releases_reservation` — end-to-end happy path through the shared helper. The three pre-existing CMT-003 concurrency/rollback tests in `broadcast.rs` (`concurrent_same_utxo_sends_resolve_via_reservation_set`, `broadcast_failure_releases_reservation_for_retry`, `unreconciled_broadcast_keeps_reservation_held`) continue to exercise the same shared helper — no separate `send_payment` ports are needed because the helper IS the shared seam those tests target. Co-Authored-By: Claude Opus 4.6 --- .../src/wallet/core/broadcast.rs | 333 ++++++++++++++---- .../src/wallet/identity/network/payments.rs | 95 ++--- 2 files changed, 293 insertions(+), 135 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs index c34c19bb7ac..17f13cdc257 100644 --- a/packages/rs-platform-wallet/src/wallet/core/broadcast.rs +++ b/packages/rs-platform-wallet/src/wallet/core/broadcast.rs @@ -1,6 +1,7 @@ use std::collections::BTreeSet; +use std::sync::Arc; -use dashcore::{Address as DashAddress, OutPoint, Transaction}; +use dashcore::{Address as DashAddress, OutPoint, Transaction, Txid}; use key_wallet::account::account_type::StandardAccountType; use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; use key_wallet::signer::Signer; @@ -8,8 +9,12 @@ use key_wallet::transaction_checking::{TransactionContext, WalletTransactionChec use key_wallet::wallet::managed_wallet_info::coin_selection::SelectionError; use key_wallet::wallet::managed_wallet_info::transaction_builder::BuilderError; use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use key_wallet_manager::WalletManager; +use tokio::sync::RwLock; +use super::reservations::OutpointReservationGuard; use crate::broadcaster::TransactionBroadcaster; +use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use crate::{CoreWallet, PlatformWalletError}; /// Map a key-wallet [`BuilderError`] from `TransactionBuilder::build_signed` into a @@ -49,6 +54,101 @@ pub(crate) fn classify_build_error( } } +/// Shared send-flow tail used by [`CoreWallet::send_to_addresses`] and +/// `IdentityWallet::send_payment`. +/// +/// Broadcasts `tx`, then under a single write lock acquisition reconciles +/// wallet state and either releases or leaks the reservation guard +/// according to the post-broadcast invariant (CMT-003): +/// +/// * On broadcast failure the reservation is implicitly dropped — the +/// guard is moved into this function and unwinds via `Drop` when the +/// early `?` returns, releasing the outpoints so the caller can retry. +/// * On broadcast success, `check_core_transaction` is run inside the +/// write lock. If `is_relevant == true` the reservation is released via +/// [`OutpointReservationGuard::release_after_commit`]; otherwise it is +/// leaked via [`OutpointReservationGuard::leak_until_sync`] to prevent a +/// concurrent caller from re-selecting an already-broadcast outpoint. +/// * `on_reconcile` is invoked inside the write lock *after* +/// `check_core_transaction` so call-site-specific bookkeeping (e.g. +/// `IdentityWallet::send_payment` recording a `PaymentEntry`) sees the +/// same locked wallet state and runs in the same critical section. +/// +/// Returns the broadcasted [`Txid`]. +pub(crate) async fn broadcast_and_reconcile( + wallet_manager: &Arc>>, + wallet_id: WalletId, + broadcaster: &Arc, + tx: &Transaction, + reservation: OutpointReservationGuard, + on_reconcile: F, +) -> Result +where + B: TransactionBroadcaster + ?Sized, + F: FnOnce(&mut PlatformWalletInfo, &Txid, bool), +{ + // Broadcast first — on error the `reservation` guard is dropped via + // `?` unwind, releasing the outpoints so the caller can retry. If the + // network accepted but the call errored (ambiguous outcome), a retry + // will be rejected as a duplicate spend rather than us marking UTXOs + // spent prematurely. + let txid = broadcaster.broadcast(tx).await?; + + // Mark inputs spent under the write lock, transitioning them from + // "reserved" to "spent" before the reservation guard drops. The guard + // is consumed below by either `release_after_commit` (success) or + // `leak_until_sync` (post-broadcast reconcile failure). + let mut reconciled = false; + { + let mut wm = wallet_manager.write().await; + if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&wallet_id) { + let check_result = info + .check_core_transaction(tx, TransactionContext::Mempool, wallet, true, true) + .await; + reconciled = check_result.is_relevant; + if !reconciled { + tracing::error!( + target: "platform_wallet::broadcast", + event = "post_broadcast_unrelated_to_own_wallet", + txid = %txid, + wallet_id = %hex::encode(wallet_id), + "Internal invariant violation: own-built broadcast not recognized by post-broadcast check" + ); + } + on_reconcile(info, &txid, reconciled); + } else { + tracing::warn!( + target: "platform_wallet::broadcast", + event = "post_broadcast_wallet_missing", + wallet_id = %hex::encode(wallet_id), + %txid, + "wallet missing during post-broadcast transaction registration" + ); + } + } + + if reconciled { + // Inputs are now marked spent; safe to release reservation. + reservation.release_after_commit(); + } else { + // Broadcast succeeded but state could not be reconciled. Releasing + // the reservation now risks a concurrent send re-selecting the + // same UTXO and producing a double-spend the network would + // reject. Keep it held until process restart — see + // `OutpointReservationGuard::leak_until_sync`. + tracing::warn!( + target: "platform_wallet::broadcast", + event = "post_broadcast_reservation_leaked_until_sync", + %txid, + wallet_id = %hex::encode(wallet_id), + "leaking outpoint reservation: post-broadcast reconciliation failed" + ); + reservation.leak_until_sync(); + } + + Ok(txid) +} + impl CoreWallet { /// Broadcast a signed transaction to the network. /// @@ -273,73 +373,18 @@ impl CoreWallet { (tx, reservation) }; - // Broadcast first — on error we leave wallet state untouched so the caller can retry. - // If the network accepted but the call errored (ambiguous outcome), a retry will be - // rejected as a duplicate spend rather than us marking UTXOs spent prematurely. - self.broadcast_transaction(&tx).await?; - - // Mark inputs spent under the write lock, transitioning them from "reserved" to "spent" - // before the reservation guard drops — no observable gap for concurrent callers. - // Warning paths below do NOT return Err: the network already accepted the tx. - // - // CMT-003: the reservation is released *only* when both - // (a) the wallet lookup succeeded, and - // (b) `check_core_transaction` recognised the tx as relevant (i.e. it marked - // the inputs as spent in `managed_account.utxos`). - // If either fails, releasing the reservation would let a concurrent caller - // select the same UTXO and produce a double-spend on the network. The change - // index was already committed inside the build write lock above, so a single - // gap is acceptable — leak the reservation until a full sync reconciles. - let mut reconciled = false; - { - let mut wm = self.wallet_manager.write().await; - if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&self.wallet_id) { - let check_result = info - .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) - .await; - if check_result.is_relevant { - reconciled = true; - } else { - // Own-built tx unrecognised by our checker is an internal invariant - // violation, not a transient. Stable event field for operator alerting. - tracing::error!( - target: "platform_wallet::broadcast", - event = "post_broadcast_unrelated_to_own_wallet", - txid = %tx.txid(), - wallet_id = %hex::encode(self.wallet_id), - "Internal invariant violation: own-built broadcast not recognized by post-broadcast check" - ); - } - } else { - // Log-only: broadcast already succeeded; the wallet handle is stale and - // future sends will surface a clean `WalletNotFound` from the lookup above. - tracing::warn!( - target: "platform_wallet::broadcast", - event = "post_broadcast_wallet_missing", - wallet_id = %hex::encode(self.wallet_id), - txid = %tx.txid(), - "wallet missing during post-broadcast transaction registration" - ); - } - } - - if reconciled { - // Inputs are now marked spent; safe to release reservation. - _reservation.release_after_commit(); - } else { - // Broadcast succeeded but state could not be reconciled. Releasing the - // reservation now risks a concurrent send re-selecting the same UTXO and - // producing a double-spend the network would reject. Keep it held until - // a future sync reconciles — a wallet restart is the eventual relief. - tracing::warn!( - target: "platform_wallet::broadcast", - event = "post_broadcast_reservation_leaked_until_sync", - txid = %tx.txid(), - wallet_id = %hex::encode(self.wallet_id), - "leaking outpoint reservation: post-broadcast reconciliation failed" - ); - _reservation.leak_until_sync(); - } + broadcast_and_reconcile( + &self.wallet_manager, + self.wallet_id, + &self.broadcaster, + &tx, + _reservation, + |_info, _txid, _reconciled| { + // No path-specific post-reconcile bookkeeping for the + // raw send-to-addresses flow. + }, + ) + .await?; Ok(tx) } @@ -1070,4 +1115,156 @@ mod tests { must fall through to TransactionBuild; got: {mapped:?}" ); } + + // ---- broadcast_and_reconcile: shared post-broadcast helper (CMT-003) ---- + // + // The helper centralises the broadcast → reconcile → release-or-leak + // decision tree used by both `CoreWallet::send_to_addresses` and + // `IdentityWallet::send_payment`. These tests pin the helper's three + // invariants directly so the shared path stays honest: + // + // 1. Broadcast failure unwinds the reservation (caller can retry). + // 2. The `on_reconcile` hook is only invoked when the wallet lookup + // succeeded, and is passed the same `is_relevant` flag the helper + // used to decide release-vs-leak. + // 3. When the wallet is missing post-broadcast, the hook is NOT + // invoked and the reservation is leaked (no double-spend risk). + + use std::sync::atomic::AtomicBool; + + use super::broadcast_and_reconcile; + + /// On broadcast failure, the reservation guard moved into the helper + /// must be dropped via early-return, releasing the outpoints so the + /// caller can retry. The `on_reconcile` hook must NOT fire. + #[tokio::test] + async fn broadcast_and_reconcile_drops_reservation_on_broadcast_failure() { + use crate::wallet::core::reservations::OutpointReservations; + use dashcore::hashes::Hash; + use dashcore::{OutPoint, Txid}; + + let reservations = OutpointReservations::new(); + let outpoint = OutPoint::new(Txid::from_byte_array([42u8; 32]), 0); + let guard = reservations.reserve(vec![outpoint], None); + assert!(reservations.contains(&outpoint)); + + let broadcaster: Arc = Arc::new(FailingBroadcaster); + let wm = Arc::new(RwLock::new(WalletManager::< + crate::wallet::platform_wallet::PlatformWalletInfo, + >::new(Network::Testnet))); + let tx = dummy_transaction(); + + let hook_called = Arc::new(AtomicBool::new(false)); + let hook_called_inner = Arc::clone(&hook_called); + + let result = broadcast_and_reconcile( + &wm, + [0u8; 32], + &broadcaster, + &tx, + guard, + move |_info, _txid, _reconciled| { + hook_called_inner.store(true, Ordering::SeqCst); + }, + ) + .await; + + assert!( + matches!(result, Err(PlatformWalletError::TransactionBroadcast(_))), + "broadcast failure must propagate to the caller; got: {result:?}" + ); + assert!( + !hook_called.load(Ordering::SeqCst), + "on_reconcile must NOT be invoked when the broadcast itself fails" + ); + assert!( + !reservations.contains(&outpoint), + "broadcast failure must release the reservation so a retry can succeed" + ); + } + + /// When the wallet is absent from the manager post-broadcast (stale + /// handle), the hook must NOT fire and the reservation must be leaked + /// (held until process restart) so a concurrent caller cannot + /// re-select the same already-broadcast outpoint. + #[tokio::test] + async fn broadcast_and_reconcile_leaks_when_wallet_missing() { + use crate::wallet::core::reservations::OutpointReservations; + use dashcore::hashes::Hash; + use dashcore::{OutPoint, Txid}; + + let reservations = OutpointReservations::new(); + let outpoint = OutPoint::new(Txid::from_byte_array([99u8; 32]), 0); + let guard = reservations.reserve(vec![outpoint], None); + + // OK-broadcasting mock so we reach the post-broadcast write lock. + let broadcaster: Arc = Arc::new(MockBroadcaster::new( + BroadcastOutcome::Ok(dummy_transaction().txid()), + )); + // Empty wallet manager — `get_wallet_mut_and_info_mut` returns `None`. + let wm = Arc::new(RwLock::new(WalletManager::< + crate::wallet::platform_wallet::PlatformWalletInfo, + >::new(Network::Testnet))); + let tx = dummy_transaction(); + + let hook_called = Arc::new(AtomicBool::new(false)); + let hook_called_inner = Arc::clone(&hook_called); + + let result = broadcast_and_reconcile( + &wm, + [0u8; 32], + &broadcaster, + &tx, + guard, + move |_info, _txid, _reconciled| { + hook_called_inner.store(true, Ordering::SeqCst); + }, + ) + .await + .expect("broadcast succeeded; missing wallet is a log-only branch"); + assert_eq!( + result, + tx.txid(), + "helper must surface the broadcaster's Txid" + ); + assert!( + !hook_called.load(Ordering::SeqCst), + "hook must not fire when the wallet handle is stale" + ); + assert!( + reservations.contains(&outpoint), + "missing-wallet post-broadcast must leak the reservation \ + (concurrent re-selection of an already-broadcast outpoint \ + would race the network)" + ); + } + + /// End-to-end through `send_to_addresses` with the full funded + /// fixture: `is_relevant = true` post-broadcast → hook receives + /// `true` and the reservation is released. Pins the success path of + /// the shared helper as exercised by the core call site. + #[tokio::test] + async fn send_to_addresses_success_invokes_hook_and_releases_reservation() { + let (wm, wallet_id, recipient, signer) = build_funded_wallet_manager(2_000_000); + let broadcaster: Arc = Arc::new(MockBroadcaster::new( + BroadcastOutcome::Ok(dummy_transaction().txid()), + )); + let core = make_core_wallet_for_manager(wm, wallet_id, broadcaster); + + let outpoint = OutPoint::new(Txid::from_byte_array([7u8; 32]), 0); + let outputs = vec![(recipient.clone(), 100_000)]; + + let result = core + .send_to_addresses(StandardAccountType::BIP44Account, 0, outputs, &signer) + .await; + assert!( + result.is_ok(), + "happy-path send must succeed; got: {result:?}" + ); + + assert!( + !core.reservations.contains(&outpoint), + "successful send + relevant tx must release the reservation" + ); + } } diff --git a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs index ab44e23a97c..54f3f73a651 100644 --- a/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs +++ b/packages/rs-platform-wallet/src/wallet/identity/network/payments.rs @@ -286,12 +286,34 @@ impl IdentityWallet { (payment_address, tx, reservation) }; - // --- 3. Broadcast the transaction (lock released). --- - let txid = self - .broadcaster - .broadcast(&tx) - .await - .map_err(|e| PlatformWalletError::TransactionBroadcast(e.to_string()))?; + // Broadcast + reconcile via the shared post-broadcast helper. + // The hook records the outgoing `PaymentEntry` on the sender's + // `ManagedIdentity` inside the same write lock the reconcile uses + // — keeping the entry recording in the same critical section + // ensures it cannot race a concurrent state mutation between + // `check_core_transaction` and `record_dashpay_payment`. + let entry = crate::wallet::identity::types::dashpay::payment::PaymentEntry::new_sent( + *to_contact_id, + amount_duffs, + memo, + ); + let entry_for_hook = entry.clone(); + let persister = self.persister.clone(); + let from_identity = *from_identity_id; + + let txid = crate::wallet::core::broadcast::broadcast_and_reconcile( + &self.wallet_manager, + self.wallet_id, + &self.broadcaster, + &tx, + _reservation, + move |info, txid, _reconciled| { + if let Some(managed) = info.identity_manager.managed_identity_mut(&from_identity) { + managed.record_dashpay_payment(txid.to_string(), entry_for_hook, &persister); + } + }, + ) + .await?; tracing::info!( from_identity = %from_identity_id, @@ -302,67 +324,6 @@ impl IdentityWallet { "DashPay payment broadcast" ); - // --- 4. Record the outgoing payment + reconcile UTXO state. --- - let entry = crate::wallet::identity::types::dashpay::payment::PaymentEntry::new_sent( - *to_contact_id, - amount_duffs, - memo, - ); - let mut reconciled = false; - { - let mut wm = self.wallet_manager.write().await; - if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&self.wallet_id) { - // Transition inputs from "reserved" → "spent" via the same - // post-broadcast checker the core path uses, so the - // reservation can be safely released below. - use key_wallet::transaction_checking::{ - TransactionContext, WalletTransactionChecker, - }; - let check_result = info - .check_core_transaction(&tx, TransactionContext::Mempool, wallet, true, true) - .await; - reconciled = check_result.is_relevant; - if !reconciled { - tracing::error!( - target: "platform_wallet::broadcast", - event = "post_broadcast_unrelated_to_own_wallet", - txid = %txid, - wallet_id = %hex::encode(self.wallet_id), - "Internal invariant violation: own-built DashPay payment not recognized by post-broadcast check" - ); - } - if let Some(managed) = info.identity_manager.managed_identity_mut(from_identity_id) - { - managed.record_dashpay_payment( - txid.to_string(), - entry.clone(), - &self.persister, - ); - } - } else { - tracing::warn!( - target: "platform_wallet::broadcast", - event = "post_broadcast_wallet_missing", - wallet_id = %hex::encode(self.wallet_id), - %txid, - "wallet missing during post-broadcast DashPay payment registration" - ); - } - } - - if reconciled { - _reservation.release_after_commit(); - } else { - tracing::warn!( - target: "platform_wallet::broadcast", - event = "post_broadcast_reservation_leaked_until_sync", - %txid, - wallet_id = %hex::encode(self.wallet_id), - "leaking outpoint reservation: DashPay post-broadcast reconciliation failed" - ); - _reservation.leak_until_sync(); - } - Ok((txid, entry)) } } From 96c1d4bfb27aa443d29bdaa158df9b6f0429904d Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 26 May 2026 14:39:10 +0200 Subject: [PATCH 28/33] fix(rs-platform-wallet-ffi): replace `0x1 as *mut` with `ptr::dangling_mut` in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge interface drift: clippy 1.92's `manual_dangling_ptr` flags the existing test-helper pattern. Swap to the idiom clippy now expects — same non-null sentinel pointer, no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../rs-platform-wallet-ffi/src/core_wallet/broadcast.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs index f10d71848ea..c96cfcf5851 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs @@ -166,7 +166,7 @@ mod tests { // Use a non-null but fake signer pointer; the closure that // would dereference it is never entered because `NULL_HANDLE` // makes `with_item` return `None`. - let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let fake_signer = std::ptr::dangling_mut::(); let mut out_tx: *mut u8 = std::ptr::null_mut(); let mut out_len: usize = 0; @@ -193,7 +193,7 @@ mod tests { let dummy_amount: u64 = 0; let addrs: [*const c_char; 1] = [dummy_addr]; let amts: [u64; 1] = [dummy_amount]; - let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let fake_signer = std::ptr::dangling_mut::(); let mut out_tx: *mut u8 = std::ptr::null_mut(); let mut out_len: usize = 0; @@ -219,7 +219,7 @@ mod tests { fn send_to_addresses_null_element_is_rejected() { let addrs: [*const c_char; 2] = [std::ptr::null(), std::ptr::null()]; let amts: [u64; 2] = [1, 2]; - let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let fake_signer = std::ptr::dangling_mut::(); let mut out_tx: *mut u8 = std::ptr::null_mut(); let mut out_len: usize = 0; From 87d24e32526ec4e77236ff55d7667d564250921b Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 26 May 2026 14:49:48 +0200 Subject: [PATCH 29/33] fix(rs-platform-wallet-ffi): collapse NoSpendableInputs onto ErrorNoSelectableInputs (14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow upstream #3651's umbrella intent: all three "can't-pick-UTXOs" wallet variants — `NoSpendableInputs` (incl. race-loser path), `OnlyOutputAddressesFunded`, `OnlyDustInputs` — now route to `ErrorNoSelectableInputs (14)`. The dedicated `ErrorNoSpendableInputs (30)` slot becomes dead and is removed from both the FFI enum and the Swift mirror (case, switch arm, `errorDescription` arm). The typed `PlatformWalletError::NoSpendableInputs` Display rendering (account_type / account_index / context + race-loser breadcrumb) still survives verbatim in the result message so callers can distinguish the underlying cause. `ErrorConcurrentSpendConflict (31)` is unchanged. Pinned FFI test updated to assert all three variants collapse onto code 14; added a separate pin for `ConcurrentSpendConflict (31)` keeping its dedicated code so future churn doesn't accidentally fold it into the umbrella too. --- packages/rs-platform-wallet-ffi/src/error.rs | 124 +++++++++--------- .../PlatformWallet/PlatformWalletResult.swift | 20 ++- 2 files changed, 73 insertions(+), 71 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index b65112eabf1..f8019bc7515 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -80,22 +80,18 @@ pub enum PlatformWalletFFIResultCode { /// no in-tree producer today. Holding the slot here keeps language-mirror /// enums (Swift, Kotlin) numerically aligned with the eventual producer. ErrorArithmeticOverflow = 13, - /// Auto-select had no candidate inputs. Covers all three "can't-select-inputs" - /// wallet variants: `NoSpendableInputs` (account has nothing spendable), - /// `OnlyOutputAddressesFunded` (every funded address is also a destination), - /// and `OnlyDustInputs` (every funded address is below `min_input_amount`). - /// The typed Display rendering survives via the result message so callers - /// can distinguish the underlying cause. Caller must rotate to a fresh + /// Auto-select had no candidate inputs. Umbrella for every + /// "can't-pick-UTXOs" wallet variant: `NoSpendableInputs` (account + /// has nothing spendable, including the race-loser path), + /// `OnlyOutputAddressesFunded` (every funded address is also a + /// destination), and `OnlyDustInputs` (every funded address is + /// below `min_input_amount`). The typed Display rendering survives + /// via the result message so callers can distinguish the underlying + /// cause. Caller must wait for sync / new UTXOs, rotate to a fresh /// receive address, consolidate sub-min balances, or fall back to /// `InputSelection::Explicit`. ErrorNoSelectableInputs = 14, - /// No spendable inputs available on the account — wait for sync / - /// new UTXOs and retry, or surface a depleted-wallet message. - /// Carries the typed [`PlatformWalletError::NoSpendableInputs`] - /// (account_type / account_index / context) stringified in the - /// `message` field. - ErrorNoSpendableInputs = 30, /// Transaction builder selected an outpoint that another in-flight /// build had already reserved — retryable. The originating /// [`PlatformWalletError::ConcurrentSpendConflict`] carries a @@ -190,21 +186,21 @@ impl From for PlatformWalletFFIResult { // dedicated code yet — those still carry the typed Display // rendering as the message. // - // `NoSpendableInputs` has its own dedicated code (30) since the - // race-loser path needs to be distinguishable from the broader - // "no selectable inputs" umbrella (14) used for the two - // address-shape variants below. + // All three "can't-select-inputs" wallet variants + // (`NoSpendableInputs`, `OnlyOutputAddressesFunded`, + // `OnlyDustInputs`) collapse onto the umbrella + // `ErrorNoSelectableInputs` code; the typed Display rendering + // in the message lets callers distinguish the underlying cause + // (including the race-loser breadcrumb on `NoSpendableInputs`). let code = match &error { - PlatformWalletError::NoSpendableInputs { .. } => { - PlatformWalletFFIResultCode::ErrorNoSpendableInputs + PlatformWalletError::NoSpendableInputs { .. } + | PlatformWalletError::OnlyOutputAddressesFunded { .. } + | PlatformWalletError::OnlyDustInputs { .. } => { + PlatformWalletFFIResultCode::ErrorNoSelectableInputs } PlatformWalletError::ConcurrentSpendConflict { .. } => { PlatformWalletFFIResultCode::ErrorConcurrentSpendConflict } - PlatformWalletError::OnlyOutputAddressesFunded { .. } - | PlatformWalletError::OnlyDustInputs { .. } => { - PlatformWalletFFIResultCode::ErrorNoSelectableInputs - } _ => PlatformWalletFFIResultCode::ErrorUnknown, }; PlatformWalletFFIResult::err(code, error.to_string()) @@ -428,52 +424,40 @@ mod tests { assert!(!r.message.is_null()); } - /// The "can't-select-inputs" wallet variants all map to dedicated FFI - /// codes (never `ErrorUnknown`) and the typed Display rendering survives - /// across the boundary so callers can distinguish the underlying cause - /// from the message string. - /// - /// `NoSpendableInputs` owns the dedicated `ErrorNoSpendableInputs` (30) - /// — the race-loser path is the canonical retry signal — while the two - /// address-shape variants (`OnlyOutputAddressesFunded`, `OnlyDustInputs`) - /// share the umbrella `ErrorNoSelectableInputs` (14). + /// All three "can't-select-inputs" wallet variants collapse onto the + /// umbrella `ErrorNoSelectableInputs` (14) FFI code, and the typed + /// Display rendering survives across the boundary so callers can + /// distinguish the underlying cause (including the race-loser + /// breadcrumb on `NoSpendableInputs`) from the message string. #[test] - fn no_selectable_inputs_maps_to_dedicated_code() { + fn no_selectable_inputs_maps_to_umbrella_code() { use dpp::address_funds::PlatformAddress; use key_wallet::account::StandardAccountType; - let cases: Vec<(PlatformWalletError, PlatformWalletFFIResultCode)> = vec![ - ( - PlatformWalletError::NoSpendableInputs { - account_type: StandardAccountType::BIP44Account, - account_index: 0, - context: "wallet empty in test".to_string(), - }, - PlatformWalletFFIResultCode::ErrorNoSpendableInputs, - ), - ( - PlatformWalletError::OnlyOutputAddressesFunded { - funded_outputs: Vec::::new(), - min_input_amount: 1_000, - }, - PlatformWalletFFIResultCode::ErrorNoSelectableInputs, - ), - ( - PlatformWalletError::OnlyDustInputs { - sub_min_count: 3, - sub_min_aggregate: 500, - min_input_amount: 1_000, - }, - PlatformWalletFFIResultCode::ErrorNoSelectableInputs, - ), + let cases: Vec = vec![ + PlatformWalletError::NoSpendableInputs { + account_type: StandardAccountType::BIP44Account, + account_index: 0, + context: "wallet empty in test".to_string(), + }, + PlatformWalletError::OnlyOutputAddressesFunded { + funded_outputs: Vec::::new(), + min_input_amount: 1_000, + }, + PlatformWalletError::OnlyDustInputs { + sub_min_count: 3, + sub_min_aggregate: 500, + min_input_amount: 1_000, + }, ]; - for (err, expected_code) in cases { + for err in cases { let rendered = err.to_string(); let result: PlatformWalletFFIResult = err.into(); assert_eq!( - result.code, expected_code, - "variant must map to its dedicated code (rendered: {rendered})" + result.code, + PlatformWalletFFIResultCode::ErrorNoSelectableInputs, + "variant must collapse onto the umbrella code (rendered: {rendered})" ); assert!(!result.message.is_null()); let msg = unsafe { std::ffi::CStr::from_ptr(result.message) } @@ -486,6 +470,28 @@ mod tests { } } + /// `ConcurrentSpendConflict` keeps its dedicated FFI code (31) — + /// race-loser breadcrumb on the typed `NoSpendableInputs` Display + /// is not the same thing as the builder picking an already-reserved + /// outpoint, and downstream callers (retry logic) must be able to + /// tell them apart by code. + #[test] + fn concurrent_spend_conflict_keeps_dedicated_code() { + let err = PlatformWalletError::ConcurrentSpendConflict { + selected: Vec::new(), + }; + let rendered = err.to_string(); + let result: PlatformWalletFFIResult = err.into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorConcurrentSpendConflict, + ); + let msg = unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_string_lossy() + .into_owned(); + assert_eq!(msg, rendered); + } + /// Other wallet-error variants without a dedicated FFI arm still /// fall through to `ErrorUnknown` while carrying the typed /// Display rendering as the message. Pin this so the catch-all diff --git a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift index 53c59ba1266..de6ff6a0cf9 100644 --- a/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift +++ b/packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift @@ -20,7 +20,6 @@ public enum PlatformWalletResultCode: Int32, Sendable { case errorUtf8Conversion = 12 case errorArithmeticOverflow = 13 case errorNoSelectableInputs = 14 - case errorNoSpendableInputs = 30 case errorConcurrentSpendConflict = 31 case notFound = 98 case errorUnknown = 99 @@ -57,8 +56,6 @@ public enum PlatformWalletResultCode: Int32, Sendable { self = .errorArithmeticOverflow case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_NO_SELECTABLE_INPUTS: self = .errorNoSelectableInputs - case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_NO_SPENDABLE_INPUTS: - self = .errorNoSpendableInputs case PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_CONCURRENT_SPEND_CONFLICT: self = .errorConcurrentSpendConflict case PLATFORM_WALLET_FFI_RESULT_CODE_NOT_FOUND: @@ -137,14 +134,14 @@ public enum PlatformWalletError: LocalizedError { case deserialization(String) case memoryAllocation(String) case arithmeticOverflow(String) - /// Umbrella for the two address-shape "can't-select-inputs" variants - /// (`OnlyOutputAddressesFunded`, `OnlyDustInputs`). Mirrors the Rust - /// `ErrorNoSelectableInputs` FFI code. + /// Umbrella for every "can't-select-inputs" wallet variant + /// (`NoSpendableInputs`, `OnlyOutputAddressesFunded`, + /// `OnlyDustInputs`). Mirrors the Rust `ErrorNoSelectableInputs` + /// FFI code; the originating Rust variant's Display rendering is + /// preserved in the associated message so callers can distinguish + /// the underlying cause (including the race-loser breadcrumb on + /// `NoSpendableInputs`). case noSelectableInputs(String) - /// No spendable inputs on the requested account — retryable after - /// sync, or surface a depleted-wallet message. Mirrors the Rust - /// `PlatformWalletError::NoSpendableInputs` variant. - case noSpendableInputs(String) /// Transaction builder picked an outpoint another concurrent /// build had already selected. Retry — the underlying reservation /// will have cleared. Mirrors the Rust @@ -163,7 +160,7 @@ public enum PlatformWalletError: LocalizedError { .identityNotFound(let m), .contactNotFound(let m), .utf8Conversion(let m), .serialization(let m), .deserialization(let m), .memoryAllocation(let m), .arithmeticOverflow(let m), .noSelectableInputs(let m), - .noSpendableInputs(let m), .concurrentSpendConflict(let m), + .concurrentSpendConflict(let m), .notFound(let m), .unknown(let m): return m } @@ -190,7 +187,6 @@ public enum PlatformWalletError: LocalizedError { case .errorUtf8Conversion: self = .utf8Conversion(detail) case .errorArithmeticOverflow: self = .arithmeticOverflow(detail) case .errorNoSelectableInputs: self = .noSelectableInputs(detail) - case .errorNoSpendableInputs: self = .noSpendableInputs(detail) case .errorConcurrentSpendConflict: self = .concurrentSpendConflict(detail) case .notFound: self = .notFound(detail) From b9a3be72c7a0b5cc7c14e2338ca26d0335c90281 Mon Sep 17 00:00:00 2001 From: lklimek <842586+lklimek@users.noreply.github.com> Date: Tue, 26 May 2026 15:24:21 +0200 Subject: [PATCH 30/33] fix(platform-wallet): reserve receive address via AddressReservations (Found-026) [backport] (#3658) Co-authored-by: Claude Opus 4.7 (1M context) --- .../src/wallet/core/reservations.rs | 423 +++++++++++++----- .../src/wallet/platform_addresses/wallet.rs | 229 +++++++++- 2 files changed, 532 insertions(+), 120 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/reservations.rs b/packages/rs-platform-wallet/src/wallet/core/reservations.rs index 2d869e12580..dabcf979a56 100644 --- a/packages/rs-platform-wallet/src/wallet/core/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/core/reservations.rs @@ -1,115 +1,116 @@ -//! Per-wallet outpoint reservation set for [`CoreWallet::send_to_addresses`](super::broadcast). +//! Generic key reservation set with an RAII guard, plus domain-specific +//! aliases / wrappers used across the platform-wallet race-protection paths: //! -//! Closes the same-UTXO concurrent-selection race: the first caller reserves its selected -//! outpoints under the write lock; subsequent callers filter them out and short-circuit with -//! [`PlatformWalletError::NoSpendableInputs`](crate::PlatformWalletError) before hitting the -//! network. Reservations are released by an RAII guard on success, error, or panic. +//! * [`OutpointReservations`] — closes the same-UTXO concurrent-selection +//! race in [`CoreWallet::send_to_addresses`](super::broadcast). Composes +//! a generic `Reservations` with a parallel +//! `Reservations
` tracking change addresses peeked but not yet +//! committed to a confirmed broadcast. +//! * [`AddressReservations`] — closes the back-to-back +//! `next_unused_receive_address` hand-out race (Found-026) without +//! eagerly advancing `highest_used` in the upstream `AddressPool`. Keys +//! are `(account_index, address_index)` pairs. +//! +//! Both paths share the [`Reservations`] core: a single `Mutex`-protected +//! `HashSet` with an RAII [`ReservationGuard`] that releases keys on +//! drop, with `release_after_commit` and `leak_until_sync` escape hatches +//! mirroring the broadcast-success / unrecoverable-leak distinction +//! introduced in PR #3585. use std::collections::HashSet; +use std::hash::Hash; use std::sync::{Arc, Mutex}; use dashcore::{Address, OutPoint}; -/// Inner state shared between an [`OutpointReservations`] handle and every -/// outstanding [`OutpointReservationGuard`]. Held behind a single `Mutex` -/// so reservation + change-address tracking commit atomically. -#[derive(Debug, Default)] -struct ReservationsInner { - outpoints: HashSet, - /// Change addresses already committed (`advance=true`) by an - /// in-flight `send_to_addresses` whose broadcast has not yet - /// completed. Concurrent senders that peek a change address still - /// present here advance past it under the same write lock so two - /// disjoint-UTXO sends do not both broadcast with the same change - /// address (privacy regression — CMT-006). Address-keyed rather than - /// `(account, index)` because the upstream pool API exposes addresses - /// but not indices. - pending_change: HashSet
, +/// Inner shared state for [`Reservations`]. Held behind a `Mutex` so +/// reserve / release commit atomically. +#[derive(Debug)] +struct ReservationsInner { + keys: HashSet, +} + +impl Default for ReservationsInner { + fn default() -> Self { + Self { + keys: HashSet::new(), + } + } } -/// Per-wallet set of outpoints that have been selected for an in-flight -/// broadcast but not yet marked spent in `ManagedWalletInfo`, plus any -/// change addresses peeked but not yet reconciled with a confirmed -/// broadcast. +/// Generic per-wallet set of keys reserved by an in-flight operation but +/// not yet committed to durable wallet state. Cheaply cloneable: holds +/// an `Arc>`. All clones share the same set. /// -/// Cheaply cloneable: holds an `Arc>` internally. All clones share -/// the same set. -#[derive(Debug, Default, Clone)] -pub(crate) struct OutpointReservations { - inner: Arc>, +/// `K` must be hashable and cloneable (the guard stores its own copy of +/// the reserved keys to release on drop) and `Debug` so the guard prints +/// usefully in `tracing` events. +#[derive(Debug)] +pub(crate) struct Reservations { + inner: Arc>>, } -impl OutpointReservations { - pub(crate) fn new() -> Self { - Self::default() +impl Default for Reservations { + fn default() -> Self { + Self { + inner: Arc::new(Mutex::new(ReservationsInner::default())), + } } +} - /// Test whether `outpoint` is currently reserved. - #[cfg(test)] - pub(crate) fn contains(&self, outpoint: &OutPoint) -> bool { - let guard = self - .inner - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.outpoints.contains(outpoint) +impl Clone for Reservations { + fn clone(&self) -> Self { + Self { + inner: Arc::clone(&self.inner), + } } +} - /// Test whether a change address is currently pending. - #[cfg(test)] - pub(crate) fn change_address_pending(&self, addr: &Address) -> bool { - let guard = self - .inner - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.pending_change.contains(addr) +impl Reservations { + pub(crate) fn new() -> Self { + Self::default() } - /// Clone the current outpoint reservation set under a single lock - /// acquisition. Callers filter spendable UTXOs against the returned - /// snapshot to avoid one mutex lock per candidate outpoint. - pub(crate) fn snapshot(&self) -> HashSet { + /// Test whether `key` is currently reserved. Only used by tests and + /// the [`OutpointReservations`] wrapper's `#[cfg(test)]` accessors — + /// production paths use [`snapshot`](Self::snapshot) to filter many + /// candidates under one lock acquisition. + #[cfg(test)] + pub(crate) fn contains(&self, key: &K) -> bool { let guard = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.outpoints.clone() + guard.keys.contains(key) } - /// Clone the current pending-change-address set so callers can skip - /// past in-flight peeks without holding the reservation mutex. - pub(crate) fn pending_change_snapshot(&self) -> HashSet
{ + /// Clone the current reservation set under a single lock acquisition. + /// Callers filter spendable candidates against the returned snapshot + /// to avoid one mutex lock per candidate. + pub(crate) fn snapshot(&self) -> HashSet { let guard = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.pending_change.clone() + guard.keys.clone() } - /// Reserve `outpoints` and (optionally) a chosen change address in - /// the same lock acquisition, returning an RAII guard that releases - /// both on drop. The guard must be held until the broadcast outcome - /// is reconciled into wallet state. - pub(crate) fn reserve( - &self, - outpoints: Vec, - change_address: Option
, - ) -> OutpointReservationGuard { + /// Reserve `keys`, returning an RAII guard that releases them on drop. + /// The guard must be held until the operation outcome has been + /// reconciled into durable wallet state. + pub(crate) fn reserve(&self, keys: Vec) -> ReservationGuard { { let mut guard = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - for op in &outpoints { - guard.outpoints.insert(*op); - } - if let Some(addr) = &change_address { - guard.pending_change.insert(addr.clone()); + for k in &keys { + guard.keys.insert(k.clone()); } } - OutpointReservationGuard { + ReservationGuard { reservations: Arc::clone(&self.inner), - outpoints, - pending_change: change_address, + keys, released: false, } } @@ -117,29 +118,27 @@ impl OutpointReservations { /// RAII guard releasing reservations on drop. /// -/// Drop is infallible and panic-safe — the underlying `Mutex` is recovered -/// from poisoning so a panicking caller still releases its outpoints -/// and pending change index (if any). +/// Drop is infallible and panic-safe — the underlying `Mutex` is +/// recovered from poisoning so a panicking caller still releases its +/// keys. #[must_use = "dropping the guard immediately releases the reservation"] -pub(crate) struct OutpointReservationGuard { - reservations: Arc>, - outpoints: Vec, - /// Pending change address reserved for this in-flight send (if any). - pending_change: Option
, +pub(crate) struct ReservationGuard { + reservations: Arc>>, + keys: Vec, /// Set after a successful `release_after_commit` so `Drop` is a no-op. released: bool, } -impl OutpointReservationGuard { - /// Release outpoints and any pending change index now, marking the - /// guard inert so its `Drop` is a no-op. Called by the broadcast - /// path after `check_core_transaction` has transitioned the inputs - /// from "reserved" to "spent" and the change index has been - /// committed via `next_change_address(..., true)`. The deliberate - /// release point exists so the same code path that *succeeded* the - /// broadcast also relinquishes the reservation — separating it from - /// the panic/drop path keeps post-broadcast-failure handling - /// (CMT-003) on the implicit `Drop` branch. +impl ReservationGuard { + /// Release the reserved keys now, marking the guard inert so its + /// `Drop` is a no-op. Called by the caller path after the wallet + /// state has transitioned the keys from "reserved" to "committed" + /// (e.g., an outpoint marked spent, an address marked used). + /// + /// The deliberate release point exists so the same code path that + /// *succeeded* the operation also relinquishes the reservation — + /// separating it from the panic/drop path keeps post-failure + /// handling on the implicit `Drop` branch. pub(crate) fn release_after_commit(mut self) { self.do_release(); self.released = true; @@ -147,28 +146,26 @@ impl OutpointReservationGuard { /// Keep the reservation held until the process exits. /// - /// Use this when the broadcast succeeded but wallet state could not be - /// reconciled (e.g., own-built tx not recognised by - /// `check_core_transaction`, or the wallet handle went stale - /// post-broadcast). Releasing the outpoints in that scenario would let a - /// concurrent caller select the same already-broadcast UTXO and produce - /// a double-spend the network would reject — keeping the reservation is - /// the safer of two bad outcomes. + /// Use this when the operation succeeded but wallet state could not + /// be reconciled, OR when the keys must remain reserved across an + /// asynchronous follow-up event (chain sync, balance flip) that + /// will eventually commit them out-of-band. /// - /// **No in-process reclaim path exists today.** The outpoints stay - /// pinned in [`OutpointReservations`] for the lifetime of the - /// `PlatformWalletManager`; only a wallet-process restart (which drops - /// the whole reservations set) releases them. + /// **No in-process reclaim path exists today.** Keys stay pinned in + /// [`Reservations`] for the lifetime of the holding manager; only + /// a process restart (which drops the whole reservations set) + /// releases them. /// /// TODO(@thepastaclaw, PR #3585): a periodic-sync-driven - /// `OutpointReservations::reclaim_confirmed(&[OutPoint])` API would let - /// the next confirmation pass reconcile leaked reservations without a - /// restart. Tracked on the PR review thread. + /// `Reservations::reclaim_committed(&[K])` API would let the next + /// confirmation / balance-sync pass reconcile leaked reservations + /// without a restart. Tracked on the PR review thread. pub(crate) fn leak_until_sync(self) { - // `mem::forget` skips `Drop`, leaving the reservation entries in - // the shared set. The guard's only owned heap allocation is the - // `Vec`, which is small and never freed for the lifetime - // of the process — acceptable given this is a rare error branch. + // `mem::forget` skips `Drop`, leaving the reservation entries + // in the shared set. The guard's only owned heap allocation is + // the `Vec`, which is small and never freed for the lifetime + // of the process — acceptable given this is a rare error / async + // commit branch. std::mem::forget(self); } @@ -177,16 +174,13 @@ impl OutpointReservationGuard { .reservations .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - for op in &self.outpoints { - inner.outpoints.remove(op); - } - if let Some(addr) = self.pending_change.take() { - inner.pending_change.remove(&addr); + for k in &self.keys { + inner.keys.remove(k); } } } -impl Drop for OutpointReservationGuard { +impl Drop for ReservationGuard { fn drop(&mut self) { if self.released { return; @@ -195,6 +189,126 @@ impl Drop for OutpointReservationGuard { } } +// ===================================================================== +// Domain aliases / wrappers +// ===================================================================== + +/// `(account_index, address_index)` key for [`AddressReservations`]. +pub(crate) type AddressReservationKey = (u32, u32); + +/// Per-wallet set of `(account_index, address_index)` slots handed out +/// by [`PlatformAddressWallet::next_unused_receive_address`](crate::wallet::platform_addresses::PlatformAddressWallet::next_unused_receive_address) +/// but not yet flipped to `used` by a positive-balance sync hit. +/// Closes the Found-026 back-to-back hand-out race without prematurely +/// advancing the upstream `AddressPool`'s `highest_used`. +pub(crate) type AddressReservations = Reservations; + +/// RAII guard for [`AddressReservations`]. The platform-payment +/// hand-out path leaks this via `leak_until_sync` after a successful +/// hand-out — exposed as a named alias so any future caller that wants +/// to thread the guard onto its own state (e.g., a transfer builder +/// dropping the guard on broadcast failure) can spell the type. +#[allow(dead_code)] // surfaced for API completeness; no in-tree caller yet +pub(crate) type AddressReservationGuard = ReservationGuard; + +/// Per-wallet set of outpoints reserved by an in-flight +/// `send_to_addresses` plus any change addresses peeked but not yet +/// reconciled with a confirmed broadcast. Cheaply cloneable. +/// +/// Composes a [`Reservations`] (outpoint slot) with a +/// [`Reservations
`] (pending change address). Two distinct +/// generic instances are used rather than one keyed by an enum because +/// the call-sites filter on each independently (`snapshot` returns just +/// the outpoints, `pending_change_snapshot` just the addresses) and the +/// two sets never overlap. Holding them under separate mutexes is safe +/// — `reserve` always takes them in `(outpoints, change)` order, the +/// only acquisition order in the module. +#[derive(Debug, Default, Clone)] +pub(crate) struct OutpointReservations { + outpoints: Reservations, + pending_change: Reservations
, +} + +impl OutpointReservations { + pub(crate) fn new() -> Self { + Self::default() + } + + /// Test whether `outpoint` is currently reserved. + #[cfg(test)] + pub(crate) fn contains(&self, outpoint: &OutPoint) -> bool { + self.outpoints.contains(outpoint) + } + + /// Test whether a change address is currently pending. + #[cfg(test)] + pub(crate) fn change_address_pending(&self, addr: &Address) -> bool { + self.pending_change.contains(addr) + } + + /// Clone the current outpoint reservation set under a single lock + /// acquisition. + pub(crate) fn snapshot(&self) -> HashSet { + self.outpoints.snapshot() + } + + /// Clone the current pending-change-address set so callers can skip + /// past in-flight peeks without holding the reservation mutex. + pub(crate) fn pending_change_snapshot(&self) -> HashSet
{ + self.pending_change.snapshot() + } + + /// Reserve `outpoints` and (optionally) a chosen change address, + /// returning an RAII guard that releases both on drop. The guard + /// must be held until the broadcast outcome is reconciled into + /// wallet state. + pub(crate) fn reserve( + &self, + outpoints: Vec, + change_address: Option
, + ) -> OutpointReservationGuard { + let outpoint_guard = self.outpoints.reserve(outpoints); + let change_guard = change_address.map(|addr| self.pending_change.reserve(vec![addr])); + OutpointReservationGuard { + outpoint_guard, + change_guard, + } + } +} + +/// RAII guard releasing outpoint + (optional) change-address +/// reservations together. Mirrors the `release_after_commit` / +/// `leak_until_sync` API of the generic guard but composes two inner +/// guards under one type so callers don't have to thread both. +#[must_use = "dropping the guard immediately releases the reservation"] +pub(crate) struct OutpointReservationGuard { + outpoint_guard: ReservationGuard, + change_guard: Option>, +} + +impl OutpointReservationGuard { + /// Release outpoints and any pending change index now, marking the + /// guard inert so its `Drop` is a no-op. Called by the broadcast + /// path after `check_core_transaction` has transitioned the inputs + /// from "reserved" to "spent" and the change index has been + /// committed via `next_change_address(..., true)`. + pub(crate) fn release_after_commit(self) { + self.outpoint_guard.release_after_commit(); + if let Some(g) = self.change_guard { + g.release_after_commit(); + } + } + + /// Keep the reservation held until the process exits. See + /// [`ReservationGuard::leak_until_sync`] for the rationale. + pub(crate) fn leak_until_sync(self) { + self.outpoint_guard.leak_until_sync(); + if let Some(g) = self.change_guard { + g.leak_until_sync(); + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -298,4 +412,81 @@ mod tests { "leak_until_sync must keep the outpoint reserved until process restart" ); } + + // ===================================================================== + // Generic `Reservations` tests — exercise the address-reservation + // alias the platform-payment hand-out path uses (Found-026). + // ===================================================================== + + #[test] + fn address_reservation_same_key_twice_is_redundant_release() { + // Reserving the same `(account, index)` from two distinct guards + // is *allowed* (HashSet idempotency) but dropping one releases + // the slot — the second guard would then mistakenly hold a key + // the set no longer contains. The platform-address hand-out + // path guards against this by **checking `contains` before + // reserving** and advancing past any reserved index, so the + // duplicate-reserve case never arises in practice. This test + // documents the invariant rather than the (would-be-broken) + // overlapping-guards behaviour. + let res: AddressReservations = AddressReservations::new(); + let k = (0u32, 5u32); + let g = res.reserve(vec![k]); + assert!(res.contains(&k)); + assert!( + res.contains(&k), + "second `contains` would also return true — callers must skip past it" + ); + drop(g); + assert!(!res.contains(&k)); + } + + #[test] + fn address_reservation_distinct_keys_independent() { + // Mirrors the outpoint `second_reservation_is_disjoint` test + // for the address-reservation alias: two distinct + // `(account, address_index)` slots can be reserved + // concurrently and both remain held until each guard drops. + let res: AddressReservations = AddressReservations::new(); + let k1 = (0u32, 0u32); + let k2 = (0u32, 1u32); + let g1 = res.reserve(vec![k1]); + let g2 = res.reserve(vec![k2]); + assert!(res.contains(&k1)); + assert!(res.contains(&k2)); + drop(g1); + assert!(!res.contains(&k1)); + assert!(res.contains(&k2)); + drop(g2); + assert!(!res.contains(&k2)); + } + + #[test] + fn address_reservation_leak_until_sync_holds_slot() { + // The hand-out path leaks the guard so the slot stays reserved + // until the next balance-positive sync flips `used` on the + // pool — at which point the in-memory reservation is harmless + // because `next_unused` will skip the index regardless. This + // test pins that semantic for the address alias. + let res: AddressReservations = AddressReservations::new(); + let k = (0u32, 7u32); + let g = res.reserve(vec![k]); + g.leak_until_sync(); + assert!( + res.contains(&k), + "leak_until_sync must keep the address slot reserved until process restart" + ); + } + + #[test] + fn address_reservation_snapshot_reflects_state() { + let res: AddressReservations = AddressReservations::new(); + let k1 = (0u32, 0u32); + let k2 = (1u32, 3u32); + let _g = res.reserve(vec![k1, k2]); + let snap = res.snapshot(); + assert!(snap.contains(&k1)); + assert!(snap.contains(&k2)); + assert_eq!(snap.len(), 2); + } } diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index 8364e2c7333..0e11a607957 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -7,6 +7,7 @@ use dpp::fee::Credits; use tokio::sync::RwLock; use crate::error::PlatformWalletError; +use crate::wallet::core::reservations::AddressReservations; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use key_wallet_manager::WalletManager; @@ -29,6 +30,14 @@ pub struct PlatformAddressWallet { pub(crate) provider: Arc>>, /// Per-wallet persistence handle for queuing changesets. pub(crate) persister: WalletPersister, + /// `(account_index, address_index)` slots handed out by + /// [`next_unused_receive_address`](Self::next_unused_receive_address) + /// but not yet flipped to `used` by a positive-balance sync hit. + /// Closes the Found-026 back-to-back hand-out race without + /// eagerly advancing the upstream `AddressPool`'s `highest_used` — + /// the upstream flip happens when a real payment lands on the + /// address. + pub(crate) reservations: AddressReservations, } impl PlatformAddressWallet { @@ -47,6 +56,7 @@ impl PlatformAddressWallet { wallet_id, provider: Arc::new(RwLock::new(None)), persister, + reservations: AddressReservations::new(), } } @@ -265,10 +275,80 @@ impl PlatformAddressWallet { key_wallet::KeySource::Public(xpub) }; - let address = managed_account - .addresses - .next_unused(&key_source, true) - .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?; + // Found-026 / #3658-review: reserve the slot in an in-memory + // `AddressReservations` set rather than flipping the upstream + // pool's `used` flag. Hand-out must not advance `highest_used` + // (gap-limit accounting) until the address actually receives a + // payment — chain sync flips `used` on the positive-balance + // hit, at which point the reservation becomes redundant. + // + // The reservation must filter the pool's `next_unused_with_info` + // result: that function only knows about the pool's own `used` + // flag, so we scan forward from index 0 picking the first slot + // that is neither `used` nor reserved. If every generated index + // is taken, mint a new one via `address_range`. + let reserved = self.reservations.snapshot(); + let account_index = account_key.account; + let pool = &mut managed_account.addresses; + + let pick = (0..=pool.highest_generated.unwrap_or(0)).find_map(|i| { + pool.addresses.get(&i).and_then(|info| { + if info.used || reserved.contains(&(account_index, i)) { + None + } else { + Some(info.clone()) + } + }) + }); + + let address_info = match pick { + Some(info) => info, + None => { + // No unused-and-unreserved index exists in the + // already-generated range: mint a fresh one strictly + // beyond `highest_generated`. Two cases: + // + // 1. `highest_generated` is `Some(h)` — `address_range(h+1, h+2)` + // generates exactly one new address at index `h+1`. + // 2. `highest_generated` is `None` (pool empty) — + // `address_range`'s `unwrap_or(0)` for the "current + // highest" collides with the "no indices exist" + // state, so a `(0, 1)` range yields an empty vec. + // Fall back to the upstream `next_unused_with_info`, + // which is correct in the empty case (no `!used && + // !reserved` index can exist if no indices exist). + match pool.highest_generated { + Some(h) => { + let next_index = h + 1; + pool.address_range(next_index, next_index + 1, &key_source) + .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?; + pool.info_at_index(next_index).cloned().ok_or_else(|| { + PlatformWalletError::AddressSync(format!( + "Failed to retrieve generated address info at index {next_index}" + )) + })? + } + None => pool + .next_unused_with_info(&key_source, true) + .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?, + } + } + }; + + // INTENTIONAL(CMT-001 / #3658 review): the reservation is + // in-memory only by design. A crash before the next sync AND + // no payment received by restart manifests as address-reuse + // (a privacy/accounting nuisance, not fund loss). Addresses + // requested but never paid to are freed for reuse on the next + // session, avoiding unbounded index-gap growth from + // speculative or abandoned hand-outs. The eager + // `mark_index_used` flip was removed at QuantumExplorer's + // request because it advanced `highest_used` permanently — + // see #3658 review thread. + self.reservations + .reserve(vec![(account_index, address_info.index)]) + .leak_until_sync(); + let address = address_info.address; PlatformAddress::try_from(address).map_err(|e| { PlatformWalletError::AddressSync(format!("Failed to convert to PlatformAddress: {e}")) @@ -327,3 +407,144 @@ impl std::fmt::Debug for PlatformAddressWallet { .finish() } } + +#[cfg(test)] +mod found_026_tests { + use super::*; + use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; + use key_wallet::account::account_collection::PlatformPaymentAccountKey; + use key_wallet::wallet::initialization::{ + PlatformPaymentAccountSpec, WalletAccountCreationOptions, + }; + use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; + use key_wallet::{Network, Wallet}; + use key_wallet_manager::WalletManager; + use std::collections::{BTreeMap, BTreeSet}; + + const ACCOUNT_KEY: PlatformPaymentAccountKey = PlatformPaymentAccountKey { + account: 0, + key_class: 0, + }; + + /// Build a network-free `PlatformAddressWallet` over one DIP-17 + /// platform-payment account (account 0, key_class 0). Mirrors the + /// `register_wallet` path: `ManagedWalletInfo::from_wallet` + + /// `insert_wallet`, no SPV / no funding. + fn wallet_with_platform_account() -> PlatformAddressWallet { + let mut pp = BTreeSet::new(); + pp.insert(PlatformPaymentAccountSpec { + account: 0, + key_class: 0, + }); + let opts = WalletAccountCreationOptions::AllAccounts( + BTreeSet::new(), + BTreeSet::new(), + BTreeSet::new(), + BTreeSet::new(), + pp, + ); + let wallet = Wallet::new_random(Network::Testnet, opts).expect("wallet"); + + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let info = PlatformWalletInfo { + core_wallet: ManagedWalletInfo::from_wallet(&wallet, 0), + balance: Arc::new(crate::wallet::core::WalletBalance::new()), + identity_manager: crate::wallet::identity::IdentityManager::new(), + tracked_asset_locks: BTreeMap::new(), + }; + let wallet_manager = Arc::new(RwLock::new(WalletManager::new(Network::Testnet))); + let wallet_id = wallet_manager + .try_write() + .expect("uncontended") + .insert_wallet(wallet, info) + .expect("insert"); + let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); + PlatformAddressWallet::new(sdk, wallet_manager, wallet_id, persister) + } + + /// Found-026 durable guard: two `next_unused_receive_address` calls + /// with NO intervening sync/balance update must return DISTINCT + /// addresses. Pre-fix, `next_unused` re-hands index 0 (its `used` + /// flag only flips on a positive synced balance) → identical + /// addresses → this assertion fails. Post-fix the first call + /// reserves index 0 in the in-memory `AddressReservations` set, + /// so the second call skips it and yields index 1. The upstream + /// pool's `highest_used` is left untouched until a real payment + /// lands — see #3658 review thread. + #[tokio::test] + async fn found_026_back_to_back_handout_returns_distinct_addresses() { + let wallet = wallet_with_platform_account(); + + let a = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("first hand-out"); + let b = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("second hand-out"); + + assert_ne!( + a, b, + "back-to-back hand-out with no sync re-handed the same address (Found-026)" + ); + } + + /// `next_unused_receive_address` must not advance `highest_used` + /// in the upstream `AddressPool` — that was @QuantumExplorer's + /// review concern on #3658. The reservation should be in-memory + /// only (gap-limit accounting stays anchored to actually-paid + /// addresses). + #[tokio::test] + async fn next_unused_receive_address_does_not_advance_highest_used() { + let wallet = wallet_with_platform_account(); + + let _a = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("first hand-out"); + let _b = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("second hand-out"); + + let wm = wallet.wallet_manager.read().await; + let info = wm.get_wallet_info(&wallet.wallet_id).expect("wallet info"); + let account = info + .core_wallet + .platform_payment_managed_account_at_index(ACCOUNT_KEY.account) + .expect("account"); + assert_eq!( + account.addresses.highest_used, None, + "hand-out must not advance `highest_used` — reservations are in-memory only" + ); + } + + /// Three consecutive hand-outs (no sync, no payment) must yield + /// three distinct indices. Pre-fix with `mark_index_used` removed, + /// a naive "scan for first `!used`" would re-hand index 0 because + /// `used` never flips. The `AddressReservations` filter must skip + /// reserved-but-not-used slots to maintain the distinct-handout + /// invariant across more than two calls. + #[tokio::test] + async fn handout_skips_reserved_but_not_used_indices() { + let wallet = wallet_with_platform_account(); + + let a = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("first hand-out"); + let b = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("second hand-out"); + let c = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("third hand-out"); + + assert_ne!(a, b, "1st vs 2nd hand-out collided"); + assert_ne!(a, c, "1st vs 3rd hand-out collided"); + assert_ne!(b, c, "2nd vs 3rd hand-out collided"); + } +} From d6bf1b17321190ac02dcf1eecb053ab2f7c20895 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Fri, 29 May 2026 12:02:51 +0200 Subject: [PATCH 31/33] test(platform-wallet): thread asset_locks into Found-026 test helper The v3.1-dev merge added a required asset_locks parameter to PlatformAddressWallet::new. The Found-026 receive-address test helper wallet_with_platform_account still used the pre-merge 4-arg signature, breaking the --tests build. Construct a stub AssetLockManager (never exercised by these hand-out tests) and pass it through, mirroring the upstream build_short_circuit_wallet idiom in transfer.rs. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/wallet/platform_addresses/wallet.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index c7bc212f95d..38f0f7125aa 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -479,7 +479,24 @@ mod found_026_tests { .insert_wallet(wallet, info) .expect("insert"); let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); - PlatformAddressWallet::new(sdk, wallet_manager, wallet_id, persister) + + // Asset-lock manager is required by the constructor but never + // exercised here — these tests only drive receive-address hand-out. + let event_manager = Arc::new(crate::events::PlatformEventManager::new(Vec::new())); + let spv = Arc::new(crate::spv::SpvRuntime::new( + Arc::clone(&wallet_manager), + event_manager, + )); + let broadcaster = Arc::new(SpvBroadcaster::new(spv)); + let asset_locks = Arc::new(AssetLockManager::new( + Arc::clone(&sdk), + Arc::clone(&wallet_manager), + wallet_id, + Arc::new(tokio::sync::Notify::new()), + broadcaster, + persister.clone(), + )); + PlatformAddressWallet::new(sdk, wallet_manager, wallet_id, asset_locks, persister) } /// Found-026 durable guard: two `next_unused_receive_address` calls From 2c08626fcc6c35f88ac6dda68ae2229ba00e087d Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Fri, 29 May 2026 14:09:32 +0200 Subject: [PATCH 32/33] refactor(platform-wallet): drop receive-address reservation from #3585; defer to proper Reserved tri-state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The receive-address hand-out race (Found-026) was being closed with a side-band `AddressReservations` set (generic `Reservations` keyed by `(account, index)`, leaked via `leak_until_sync` on hand-out). That overloads the two-state `used` address model with an out-of-band "reserved" notion — a missing-state workaround that belongs upstream in `key-wallet` as a real `Unused -> Reserved -> Used` tri-state. This slims PR #3585 to the two independent, sound fixes: * case-insensitive `.dash` DPNS resolution (rs-sdk) * the `OutpointReservations` UTXO double-spend guard (platform-wallet) `OutpointReservations` and the generic `Reservations`/`ReservationGuard` core it builds on stay — only the address-reservation alias, key type, guard alias, and the on-hand-out reserve threading are removed. `next_unused_receive_address` reverts to mainline `next_unused(..)`, matching v3.1-dev with no reserve delta. The Found-026 white-box tests and the address-reservation unit tests are removed; they return with the follow-up tri-state PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/wallet/core/reservations.rs | 127 ++------- .../src/wallet/platform_addresses/wallet.rs | 246 +----------------- 2 files changed, 19 insertions(+), 354 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/reservations.rs b/packages/rs-platform-wallet/src/wallet/core/reservations.rs index dabcf979a56..6fcc9880286 100644 --- a/packages/rs-platform-wallet/src/wallet/core/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/core/reservations.rs @@ -1,21 +1,18 @@ -//! Generic key reservation set with an RAII guard, plus domain-specific -//! aliases / wrappers used across the platform-wallet race-protection paths: +//! Generic key reservation set with an RAII guard, plus the +//! [`OutpointReservations`] wrapper used by the platform-wallet +//! UTXO race-protection path. //! -//! * [`OutpointReservations`] — closes the same-UTXO concurrent-selection -//! race in [`CoreWallet::send_to_addresses`](super::broadcast). Composes -//! a generic `Reservations` with a parallel -//! `Reservations
` tracking change addresses peeked but not yet -//! committed to a confirmed broadcast. -//! * [`AddressReservations`] — closes the back-to-back -//! `next_unused_receive_address` hand-out race (Found-026) without -//! eagerly advancing `highest_used` in the upstream `AddressPool`. Keys -//! are `(account_index, address_index)` pairs. +//! [`OutpointReservations`] closes the same-UTXO concurrent-selection +//! race in [`CoreWallet::send_to_addresses`](super::broadcast). It +//! composes a generic `Reservations` with a parallel +//! `Reservations
` tracking change addresses peeked but not yet +//! committed to a confirmed broadcast. //! -//! Both paths share the [`Reservations`] core: a single `Mutex`-protected -//! `HashSet` with an RAII [`ReservationGuard`] that releases keys on -//! drop, with `release_after_commit` and `leak_until_sync` escape hatches -//! mirroring the broadcast-success / unrecoverable-leak distinction -//! introduced in PR #3585. +//! Both inner sets share the [`Reservations`] core: a single +//! `Mutex`-protected `HashSet` with an RAII [`ReservationGuard`] +//! that releases keys on drop, with `release_after_commit` and +//! `leak_until_sync` escape hatches mirroring the broadcast-success / +//! unrecoverable-leak distinction. use std::collections::HashSet; use std::hash::Hash; @@ -67,6 +64,7 @@ impl Clone for Reservations { } impl Reservations { + #[cfg(test)] pub(crate) fn new() -> Self { Self::default() } @@ -190,27 +188,9 @@ impl Drop for ReservationGuard { } // ===================================================================== -// Domain aliases / wrappers +// Domain wrapper // ===================================================================== -/// `(account_index, address_index)` key for [`AddressReservations`]. -pub(crate) type AddressReservationKey = (u32, u32); - -/// Per-wallet set of `(account_index, address_index)` slots handed out -/// by [`PlatformAddressWallet::next_unused_receive_address`](crate::wallet::platform_addresses::PlatformAddressWallet::next_unused_receive_address) -/// but not yet flipped to `used` by a positive-balance sync hit. -/// Closes the Found-026 back-to-back hand-out race without prematurely -/// advancing the upstream `AddressPool`'s `highest_used`. -pub(crate) type AddressReservations = Reservations; - -/// RAII guard for [`AddressReservations`]. The platform-payment -/// hand-out path leaks this via `leak_until_sync` after a successful -/// hand-out — exposed as a named alias so any future caller that wants -/// to thread the guard onto its own state (e.g., a transfer builder -/// dropping the guard on broadcast failure) can spell the type. -#[allow(dead_code)] // surfaced for API completeness; no in-tree caller yet -pub(crate) type AddressReservationGuard = ReservationGuard; - /// Per-wallet set of outpoints reserved by an in-flight /// `send_to_addresses` plus any change addresses peeked but not yet /// reconciled with a confirmed broadcast. Cheaply cloneable. @@ -412,81 +392,4 @@ mod tests { "leak_until_sync must keep the outpoint reserved until process restart" ); } - - // ===================================================================== - // Generic `Reservations` tests — exercise the address-reservation - // alias the platform-payment hand-out path uses (Found-026). - // ===================================================================== - - #[test] - fn address_reservation_same_key_twice_is_redundant_release() { - // Reserving the same `(account, index)` from two distinct guards - // is *allowed* (HashSet idempotency) but dropping one releases - // the slot — the second guard would then mistakenly hold a key - // the set no longer contains. The platform-address hand-out - // path guards against this by **checking `contains` before - // reserving** and advancing past any reserved index, so the - // duplicate-reserve case never arises in practice. This test - // documents the invariant rather than the (would-be-broken) - // overlapping-guards behaviour. - let res: AddressReservations = AddressReservations::new(); - let k = (0u32, 5u32); - let g = res.reserve(vec![k]); - assert!(res.contains(&k)); - assert!( - res.contains(&k), - "second `contains` would also return true — callers must skip past it" - ); - drop(g); - assert!(!res.contains(&k)); - } - - #[test] - fn address_reservation_distinct_keys_independent() { - // Mirrors the outpoint `second_reservation_is_disjoint` test - // for the address-reservation alias: two distinct - // `(account, address_index)` slots can be reserved - // concurrently and both remain held until each guard drops. - let res: AddressReservations = AddressReservations::new(); - let k1 = (0u32, 0u32); - let k2 = (0u32, 1u32); - let g1 = res.reserve(vec![k1]); - let g2 = res.reserve(vec![k2]); - assert!(res.contains(&k1)); - assert!(res.contains(&k2)); - drop(g1); - assert!(!res.contains(&k1)); - assert!(res.contains(&k2)); - drop(g2); - assert!(!res.contains(&k2)); - } - - #[test] - fn address_reservation_leak_until_sync_holds_slot() { - // The hand-out path leaks the guard so the slot stays reserved - // until the next balance-positive sync flips `used` on the - // pool — at which point the in-memory reservation is harmless - // because `next_unused` will skip the index regardless. This - // test pins that semantic for the address alias. - let res: AddressReservations = AddressReservations::new(); - let k = (0u32, 7u32); - let g = res.reserve(vec![k]); - g.leak_until_sync(); - assert!( - res.contains(&k), - "leak_until_sync must keep the address slot reserved until process restart" - ); - } - - #[test] - fn address_reservation_snapshot_reflects_state() { - let res: AddressReservations = AddressReservations::new(); - let k1 = (0u32, 0u32); - let k2 = (1u32, 3u32); - let _g = res.reserve(vec![k1, k2]); - let snap = res.snapshot(); - assert!(snap.contains(&k1)); - assert!(snap.contains(&k2)); - assert_eq!(snap.len(), 2); - } } diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index 38f0f7125aa..a4bb1bd1e53 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -9,7 +9,6 @@ use tokio::sync::RwLock; use crate::broadcaster::SpvBroadcaster; use crate::error::PlatformWalletError; use crate::wallet::asset_lock::manager::AssetLockManager; -use crate::wallet::core::reservations::AddressReservations; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use key_wallet_manager::WalletManager; @@ -37,14 +36,6 @@ pub struct PlatformAddressWallet { pub(crate) asset_locks: Arc>, /// Per-wallet persistence handle for queuing changesets. pub(crate) persister: WalletPersister, - /// `(account_index, address_index)` slots handed out by - /// [`next_unused_receive_address`](Self::next_unused_receive_address) - /// but not yet flipped to `used` by a positive-balance sync hit. - /// Closes the Found-026 back-to-back hand-out race without - /// eagerly advancing the upstream `AddressPool`'s `highest_used` — - /// the upstream flip happens when a real payment lands on the - /// address. - pub(crate) reservations: AddressReservations, } impl PlatformAddressWallet { @@ -65,7 +56,6 @@ impl PlatformAddressWallet { provider: Arc::new(RwLock::new(None)), asset_locks, persister, - reservations: AddressReservations::new(), } } @@ -295,80 +285,10 @@ impl PlatformAddressWallet { key_wallet::KeySource::Public(xpub) }; - // Found-026 / #3658-review: reserve the slot in an in-memory - // `AddressReservations` set rather than flipping the upstream - // pool's `used` flag. Hand-out must not advance `highest_used` - // (gap-limit accounting) until the address actually receives a - // payment — chain sync flips `used` on the positive-balance - // hit, at which point the reservation becomes redundant. - // - // The reservation must filter the pool's `next_unused_with_info` - // result: that function only knows about the pool's own `used` - // flag, so we scan forward from index 0 picking the first slot - // that is neither `used` nor reserved. If every generated index - // is taken, mint a new one via `address_range`. - let reserved = self.reservations.snapshot(); - let account_index = account_key.account; - let pool = &mut managed_account.addresses; - - let pick = (0..=pool.highest_generated.unwrap_or(0)).find_map(|i| { - pool.addresses.get(&i).and_then(|info| { - if info.used || reserved.contains(&(account_index, i)) { - None - } else { - Some(info.clone()) - } - }) - }); - - let address_info = match pick { - Some(info) => info, - None => { - // No unused-and-unreserved index exists in the - // already-generated range: mint a fresh one strictly - // beyond `highest_generated`. Two cases: - // - // 1. `highest_generated` is `Some(h)` — `address_range(h+1, h+2)` - // generates exactly one new address at index `h+1`. - // 2. `highest_generated` is `None` (pool empty) — - // `address_range`'s `unwrap_or(0)` for the "current - // highest" collides with the "no indices exist" - // state, so a `(0, 1)` range yields an empty vec. - // Fall back to the upstream `next_unused_with_info`, - // which is correct in the empty case (no `!used && - // !reserved` index can exist if no indices exist). - match pool.highest_generated { - Some(h) => { - let next_index = h + 1; - pool.address_range(next_index, next_index + 1, &key_source) - .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?; - pool.info_at_index(next_index).cloned().ok_or_else(|| { - PlatformWalletError::AddressSync(format!( - "Failed to retrieve generated address info at index {next_index}" - )) - })? - } - None => pool - .next_unused_with_info(&key_source, true) - .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?, - } - } - }; - - // INTENTIONAL(CMT-001 / #3658 review): the reservation is - // in-memory only by design. A crash before the next sync AND - // no payment received by restart manifests as address-reuse - // (a privacy/accounting nuisance, not fund loss). Addresses - // requested but never paid to are freed for reuse on the next - // session, avoiding unbounded index-gap growth from - // speculative or abandoned hand-outs. The eager - // `mark_index_used` flip was removed at QuantumExplorer's - // request because it advanced `highest_used` permanently — - // see #3658 review thread. - self.reservations - .reserve(vec![(account_index, address_info.index)]) - .leak_until_sync(); - let address = address_info.address; + let address = managed_account + .addresses + .next_unused(&key_source, true) + .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?; PlatformAddress::try_from(address).map_err(|e| { PlatformWalletError::AddressSync(format!("Failed to convert to PlatformAddress: {e}")) @@ -427,161 +347,3 @@ impl std::fmt::Debug for PlatformAddressWallet { .finish() } } - -#[cfg(test)] -mod found_026_tests { - use super::*; - use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; - use key_wallet::account::account_collection::PlatformPaymentAccountKey; - use key_wallet::wallet::initialization::{ - PlatformPaymentAccountSpec, WalletAccountCreationOptions, - }; - use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; - use key_wallet::{Network, Wallet}; - use key_wallet_manager::WalletManager; - use std::collections::{BTreeMap, BTreeSet}; - - const ACCOUNT_KEY: PlatformPaymentAccountKey = PlatformPaymentAccountKey { - account: 0, - key_class: 0, - }; - - /// Build a network-free `PlatformAddressWallet` over one DIP-17 - /// platform-payment account (account 0, key_class 0). Mirrors the - /// `register_wallet` path: `ManagedWalletInfo::from_wallet` + - /// `insert_wallet`, no SPV / no funding. - fn wallet_with_platform_account() -> PlatformAddressWallet { - let mut pp = BTreeSet::new(); - pp.insert(PlatformPaymentAccountSpec { - account: 0, - key_class: 0, - }); - let opts = WalletAccountCreationOptions::AllAccounts( - BTreeSet::new(), - BTreeSet::new(), - BTreeSet::new(), - BTreeSet::new(), - pp, - ); - let wallet = Wallet::new_random(Network::Testnet, opts).expect("wallet"); - - let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); - let info = PlatformWalletInfo { - core_wallet: ManagedWalletInfo::from_wallet(&wallet, 0), - balance: Arc::new(crate::wallet::core::WalletBalance::new()), - identity_manager: crate::wallet::identity::IdentityManager::new(), - tracked_asset_locks: BTreeMap::new(), - }; - let wallet_manager = Arc::new(RwLock::new(WalletManager::new(Network::Testnet))); - let wallet_id = wallet_manager - .try_write() - .expect("uncontended") - .insert_wallet(wallet, info) - .expect("insert"); - let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); - - // Asset-lock manager is required by the constructor but never - // exercised here — these tests only drive receive-address hand-out. - let event_manager = Arc::new(crate::events::PlatformEventManager::new(Vec::new())); - let spv = Arc::new(crate::spv::SpvRuntime::new( - Arc::clone(&wallet_manager), - event_manager, - )); - let broadcaster = Arc::new(SpvBroadcaster::new(spv)); - let asset_locks = Arc::new(AssetLockManager::new( - Arc::clone(&sdk), - Arc::clone(&wallet_manager), - wallet_id, - Arc::new(tokio::sync::Notify::new()), - broadcaster, - persister.clone(), - )); - PlatformAddressWallet::new(sdk, wallet_manager, wallet_id, asset_locks, persister) - } - - /// Found-026 durable guard: two `next_unused_receive_address` calls - /// with NO intervening sync/balance update must return DISTINCT - /// addresses. Pre-fix, `next_unused` re-hands index 0 (its `used` - /// flag only flips on a positive synced balance) → identical - /// addresses → this assertion fails. Post-fix the first call - /// reserves index 0 in the in-memory `AddressReservations` set, - /// so the second call skips it and yields index 1. The upstream - /// pool's `highest_used` is left untouched until a real payment - /// lands — see #3658 review thread. - #[tokio::test] - async fn found_026_back_to_back_handout_returns_distinct_addresses() { - let wallet = wallet_with_platform_account(); - - let a = wallet - .next_unused_receive_address(ACCOUNT_KEY) - .await - .expect("first hand-out"); - let b = wallet - .next_unused_receive_address(ACCOUNT_KEY) - .await - .expect("second hand-out"); - - assert_ne!( - a, b, - "back-to-back hand-out with no sync re-handed the same address (Found-026)" - ); - } - - /// `next_unused_receive_address` must not advance `highest_used` - /// in the upstream `AddressPool` — that was @QuantumExplorer's - /// review concern on #3658. The reservation should be in-memory - /// only (gap-limit accounting stays anchored to actually-paid - /// addresses). - #[tokio::test] - async fn next_unused_receive_address_does_not_advance_highest_used() { - let wallet = wallet_with_platform_account(); - - let _a = wallet - .next_unused_receive_address(ACCOUNT_KEY) - .await - .expect("first hand-out"); - let _b = wallet - .next_unused_receive_address(ACCOUNT_KEY) - .await - .expect("second hand-out"); - - let wm = wallet.wallet_manager.read().await; - let info = wm.get_wallet_info(&wallet.wallet_id).expect("wallet info"); - let account = info - .core_wallet - .platform_payment_managed_account_at_index(ACCOUNT_KEY.account) - .expect("account"); - assert_eq!( - account.addresses.highest_used, None, - "hand-out must not advance `highest_used` — reservations are in-memory only" - ); - } - - /// Three consecutive hand-outs (no sync, no payment) must yield - /// three distinct indices. Pre-fix with `mark_index_used` removed, - /// a naive "scan for first `!used`" would re-hand index 0 because - /// `used` never flips. The `AddressReservations` filter must skip - /// reserved-but-not-used slots to maintain the distinct-handout - /// invariant across more than two calls. - #[tokio::test] - async fn handout_skips_reserved_but_not_used_indices() { - let wallet = wallet_with_platform_account(); - - let a = wallet - .next_unused_receive_address(ACCOUNT_KEY) - .await - .expect("first hand-out"); - let b = wallet - .next_unused_receive_address(ACCOUNT_KEY) - .await - .expect("second hand-out"); - let c = wallet - .next_unused_receive_address(ACCOUNT_KEY) - .await - .expect("third hand-out"); - - assert_ne!(a, b, "1st vs 2nd hand-out collided"); - assert_ne!(a, c, "1st vs 3rd hand-out collided"); - assert_ne!(b, c, "2nd vs 3rd hand-out collided"); - } -} From 1ac8c5d9acca9e7fd6a5380f8211acf3f850b17d Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Fri, 29 May 2026 14:16:28 +0200 Subject: [PATCH 33/33] chore(platform-wallet): drop unused generic Reservations::new MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Reservations::::new()` had no callers after the address-reservation removal — `OutpointReservations` constructs its inner sets via `Reservations::default()`. Drop it to clear the dead_code warning. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/rs-platform-wallet/src/wallet/core/reservations.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/reservations.rs b/packages/rs-platform-wallet/src/wallet/core/reservations.rs index 6fcc9880286..ec89cd92770 100644 --- a/packages/rs-platform-wallet/src/wallet/core/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/core/reservations.rs @@ -64,11 +64,6 @@ impl Clone for Reservations { } impl Reservations { - #[cfg(test)] - pub(crate) fn new() -> Self { - Self::default() - } - /// Test whether `key` is currently reserved. Only used by tests and /// the [`OutpointReservations`] wrapper's `#[cfg(test)]` accessors — /// production paths use [`snapshot`](Self::snapshot) to filter many