fix: case-insensitive .dash suffix and UTXO double-spend prevention (re-target of #3466)

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),
 

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<B: TransactionBroadcaster + ?Sized> CoreWallet<B> {
     ) -> Result<Transaction, PlatformWalletError> {
         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(
@@ -103,8 +106,12 @@ impl<B: TransactionBroadcaster + ?Sized> CoreWallet<B> {
                 ))
             })?;
 
+            // 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);
@@ -127,12 +134,117 @@ impl<B: TransactionBroadcaster + ?Sized> CoreWallet<B> {
                 )
                 .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?;
 
-            builder
+            let tx = builder
                 .build()
-                .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?
+                .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?;
+
+            // 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<OutPoint> =
+                tx.input.iter().map(|txin| txin.previous_output).collect();
+            let spendable_outpoints: BTreeSet<OutPoint> =
+                spendable.iter().map(|utxo| utxo.outpoint).collect();
+            if !selected.is_subset(&spendable_outpoints) {
+                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
         };
 
+        // Broadcast first; if the network rejects we leave wallet state
+        // untouched so the caller can retry without manual sync repair.
+        // This is intentional even if the remote accepted the transaction
+        // but the broadcast path returned an error: in that ambiguous case
+        // later attempts may reuse the same inputs locally, but the network
+        // rejects the duplicate spend instead of us marking UTXOs spent for
+        // a transaction that might not have propagated.
         self.broadcast_transaction(&tx).await?;
+
+        // Now that the tx is in flight, register it as a mempool transaction
+        // so subsequent callers see the inputs as spent and don't reselect
+        // them. The trade-off is that two callers racing between the lock
+        // drop above and the broadcast can both pick the same UTXOs; the
+        // network resolves that race exactly as it does on `v3.1-dev`
+        // today, but neither caller corrupts local state on a transient
+        // broadcast failure.
+        //
+        // Broadcast-first semantics: by the time we get here the network has
+        // already accepted the transaction, so the two warning paths below
+        // intentionally do NOT convert into a post-success `Err`. They
+        // simply mean local wallet state did not get updated to reflect the
+        // mempool spend / change output. Recovery in both cases:
+        //
+        //   * The next `send_to_addresses` from the same handle may reselect
+        //     the same UTXOs because they still look spendable locally. That
+        //     follow-up transaction will be rejected by the network as a
+        //     duplicate spend (the broadcaster surfaces that as an error to
+        //     the caller), so funds are never double-spent on-chain.
+        //   * Once mempool/block sync catches up, the wallet will see the
+        //     original transaction and reconcile its UTXO set, after which
+        //     subsequent sends pick up the correct change outputs.
+        //
+        // The two cases differ in what they imply:
+        //
+        //   * `!check_result.is_relevant` is the expected transient: the
+        //     wallet just hasn't ingested the tx yet (or some derivation
+        //     path/script is unrecognised), and a later sync will fix it.
+        //   * The `else` branch (wallet missing in the manager) is NOT a
+        //     normal transient — the broadcast succeeded against a
+        //     `CoreWallet` handle whose underlying wallet entry is gone
+        //     from the manager. That is a broken/inconsistent local handle
+        //     and the warning exists so operators can spot it; future
+        //     sends through the same handle will keep failing the lookup
+        //     above and surface a clean `WalletNotFound` error.
+        {
+            let mut wm = self.wallet_manager.write().await;
+            if let Some((wallet, info)) = wm.get_wallet_mut_and_info_mut(&self.wallet_id) {
+                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)
     }
 }

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<bool, Error> {
+    pub async fn is_dpns_name_available(&self, name: &str) -> Result<bool, Error> {
         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);
-            // Only strip the suffix if it's exactly ".dash"
-            if suffix == ".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