From 3b924c70979fbcb3c1a3377f71ef76168a675ef2 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:49:09 -0400 Subject: [PATCH 1/7] feat(platform-wallet): union-funding build_signed_payment core primitive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add CoreWallet::build_signed_payment — a first-class "send" primitive that selects inputs across the UNION of every signable funds account (BIP44 + BIP32 + CoinJoin + DashPay receiving), builds and signs a standard L1 payment, and returns the signed transaction plus its fee and change amount. This is the build-only half of the Android send-path cutover: during the dashj->SDK transition the app keeps dashj's transaction bookkeeping (maybeCommitTx drives CrowdNode, memos, confidence listeners), so the SDK must build + sign from the bound wallet and hand back the bytes while dashj commits/broadcasts. The method therefore does NOT broadcast and does NOT persist a debit — the only state touched is the in-memory ReservationSet that set_funding/build_signed use to stop a concurrent SDK build from re-selecting the same coins (released when the spend is later observed by sync or by the reservation-TTL backstop). Reuses the shielded asset-lock union-funding machinery (all_funding_accounts + spendable_utxos + a spanning path resolver + LargestFirst selection to keep CoinJoin's many small denominations from blowing up BranchAndBound), but excludes watch-only DashpayExternalAccounts (a contact's addresses, which this wallet cannot sign). Fee and change are derived from the transaction itself (inputs - outputs), the always self-consistent ground truth, rather than from build_signed's signed-size fee recomputation which can drift by a few duffs from what is actually paid. Adds the typed PaymentInsufficientFunds { available, required } error so a shortfall carries the exact union-wide selectable total. Tests: correct output/change/fee, BIP44+CoinJoin union selection, typed union shortfall, watch-only exclusion, and input validation. cargo test -p platform-wallet --lib: 442 passed. Co-Authored-By: Claude Fable 5 (cherry picked from commit 38012d149f66a13c21efb81d8fff271ece8708d4) --- packages/rs-platform-wallet/src/error.rs | 13 + packages/rs-platform-wallet/src/lib.rs | 1 + .../rs-platform-wallet/src/wallet/core/mod.rs | 2 + .../src/wallet/core/send.rs | 619 ++++++++++++++++++ 4 files changed, 635 insertions(+) create mode 100644 packages/rs-platform-wallet/src/wallet/core/send.rs diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 6e11514bea..09e99cd0b1 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -71,6 +71,19 @@ pub enum PlatformWalletError { #[error("Asset lock transaction failed: {0}")] AssetLockTransaction(String), + /// A general Core L1 payment build (`CoreWallet::build_signed_payment`) + /// could not cover the requested outputs plus fee from the union of the + /// wallet's *signable* funds accounts (BIP44 + BIP32 + CoinJoin + DashPay + /// receiving; watch-only DashPay external accounts are excluded). `available` + /// is the total selectable value across those accounts, `required` the + /// outputs-plus-fee target — carried as exact duff amounts (instead of being + /// flattened into a string) so callers can render a precise shortfall. + #[error( + "payment coin selection is short: available {available} duffs, \ + required {required} duffs" + )] + PaymentInsufficientFunds { available: u64, required: u64 }, + #[error("Transaction broadcast failed: {0}")] TransactionBroadcast(String), diff --git a/packages/rs-platform-wallet/src/lib.rs b/packages/rs-platform-wallet/src/lib.rs index e91c5ccee0..04c034eb50 100644 --- a/packages/rs-platform-wallet/src/lib.rs +++ b/packages/rs-platform-wallet/src/lib.rs @@ -56,6 +56,7 @@ pub use spv::SpvRuntime; pub use wallet::asset_lock::manager::AssetLockManager; pub use wallet::asset_lock::tracked::{AssetLockStatus, TrackedAssetLock}; pub use wallet::asset_lock::AssetLockFunding; +pub use wallet::core::SignedCorePayment; pub use wallet::core::WalletBalance; pub use wallet::core::{CoreWallet, SignedCoreTransaction}; // DashPay types + crypto helpers re-exported through the identity diff --git a/packages/rs-platform-wallet/src/wallet/core/mod.rs b/packages/rs-platform-wallet/src/wallet/core/mod.rs index 5481362ae8..953b1a500f 100644 --- a/packages/rs-platform-wallet/src/wallet/core/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/core/mod.rs @@ -1,10 +1,12 @@ pub mod balance; pub mod balance_handler; mod broadcast; +mod send; mod transaction; pub mod wallet; pub use balance::WalletBalance; pub use balance_handler::BalanceUpdateHandler; +pub use send::SignedCorePayment; pub use transaction::SignedCoreTransaction; pub use wallet::CoreWallet; diff --git a/packages/rs-platform-wallet/src/wallet/core/send.rs b/packages/rs-platform-wallet/src/wallet/core/send.rs new file mode 100644 index 0000000000..4d74fa37fc --- /dev/null +++ b/packages/rs-platform-wallet/src/wallet/core/send.rs @@ -0,0 +1,619 @@ +//! General Core L1 payment building. +//! +//! [`CoreWallet::build_signed_payment`] is the first-class "send" primitive: +//! it selects inputs across every **signable** funds account, builds and signs +//! a standard payment transaction, and returns the **signed serialized bytes** +//! plus the computed fee and change amount — WITHOUT broadcasting and WITHOUT +//! persisting a debit. +//! +//! ## Why build-only / no-broadcast +//! +//! During the dashj→SDK transition the Android app keeps its own transaction +//! bookkeeping (dashj's `maybeCommitTx` drives CrowdNode, memos, and confidence +//! listeners). The app therefore wants the SDK to *build + sign* a payment from +//! the bound wallet and hand back the raw bytes, then commit + broadcast them +//! through dashj itself. Post-transition a separate SDK-broadcast mode will own +//! broadcasting and the debit persistence that goes with it; this primitive is +//! the permanent, generally-useful "give me signed bytes" half of that split. +//! +//! ## Persistence semantics (deliberate) +//! +//! Building does **not** persist a debit and does not write UTXOs, balances, or +//! transaction records back to the wallet. The only in-memory mutation is the +//! key-wallet `ReservationSet` bookkeeping that `set_funding` + +//! `TransactionBuilder::build_signed` perform on the primary funding account: +//! the selected inputs are marked *reserved* so a concurrent SDK build does not +//! re-select the same coins. That reservation is in-memory only (never +//! serialized) and is released when the spend is later processed back into the +//! wallet by sync, or by the reservation-TTL backstop, or explicitly via +//! [`ManagedCoreFundsAccount::release_reservation`] for an abandoned build. No +//! balance is debited until the transaction actually confirms — exactly what +//! the transition flow needs, since dashj owns commit/broadcast. +//! +//! [`ManagedCoreFundsAccount::release_reservation`]: +//! key_wallet::managed_account::ManagedCoreFundsAccount::release_reservation + +use std::collections::{HashMap, HashSet}; + +use dashcore::{Address as DashAddress, OutPoint, Transaction}; +use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; +use key_wallet::managed_account::ManagedCoreFundsAccount; +use key_wallet::ManagedAccountType; +use key_wallet::signer::Signer; +use key_wallet::wallet::managed_wallet_info::coin_selection::{SelectionError, SelectionStrategy}; +use key_wallet::wallet::managed_wallet_info::fee::FeeRate; +use key_wallet::wallet::managed_wallet_info::transaction_builder::{BuilderError, TransactionBuilder}; +use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; +use key_wallet::Utxo; + +use crate::broadcaster::TransactionBroadcaster; +use crate::error::PlatformWalletError; +use crate::wallet::core::CoreWallet; + +/// key-wallet's default fee rate (duffs per kB). Matches the asset-lock +/// builder's `DEFAULT_FEE_PER_KB` and `FeeRate::normal()`. +const DEFAULT_FEE_PER_KB: u64 = 1000; + +/// The BIP44 account that supplies the change output (and whose reservation +/// ledger gates concurrent primary-account builds). The union of every other +/// signable funds account is added as explicit inputs on top of it. +const PRIMARY_BIP44_ACCOUNT_INDEX: u32 = 0; + +/// A built-and-signed Core L1 payment, ready to be committed/broadcast by the +/// caller (dashj during the transition, or a later SDK-broadcast mode). +#[derive(Debug, Clone)] +pub struct SignedCorePayment { + /// The signed transaction. Serialize with + /// [`consensus::serialize`](dashcore::consensus::serialize) for the raw + /// wire bytes the caller hands to its broadcaster. + pub transaction: Transaction, + /// The fee paid, in duffs, computed from the encoded size of the *signed* + /// transaction. + pub fee: u64, + /// Duffs returned to the wallet's change address (0 when the build produced + /// no change output — an exact-match selection or a dust-only remainder + /// folded into the fee). + pub change_amount: u64, +} + +/// True for funds accounts the bound wallet cannot sign for. Only +/// `DashpayExternalAccount`s are watch-only: they hold a *contact's* receiving +/// addresses (we keep the contact's xpub to build payments *to* them and to +/// watch that side), so their UTXOs must never be selected as spend inputs — +/// signing would fail because no private key of ours derives them. Every other +/// funds account (BIP44/BIP32/CoinJoin/DashPay receiving) is derived from our +/// own seed and is signable. +fn is_watch_only_funds_account(account: &ManagedCoreFundsAccount) -> bool { + matches!( + account.managed_account_type(), + ManagedAccountType::DashpayExternalAccount { .. } + ) +} + +impl CoreWallet { + /// Build and sign a standard Core L1 payment to `outputs`, funding it from + /// the union of every **signable** funds account, and return the signed + /// transaction plus its fee and change amount. Does **not** broadcast and + /// does **not** persist a debit (see the module docs for the persistence + /// contract). + /// + /// ## Coin selection — union of signable accounts + /// + /// Inputs are selected across BIP44 + BIP32 + CoinJoin + DashPay-receiving + /// accounts (the wallet-wide spendable set), reusing the same union-funding + /// machinery the shielded asset-lock path uses + /// ([`AssetLockManager::build_asset_lock_tx_from_all_funding_accounts`]): + /// BIP44 account 0 is the PRIMARY account (it supplies the change output and + /// its reservation ledger gates concurrent primary-account builds), and the + /// spendable UTXOs of every other signable account are added as explicit + /// builder inputs. Watch-only `DashpayExternalAccount`s are excluded — their + /// coins belong to a contact and cannot be signed by this wallet. + /// + /// `LargestFirst` selection is used deliberately (not the builder default + /// `BranchAndBound`): a CoinJoin account can hold many small mixed + /// denominations, and `BranchAndBound`'s exact-match subset-sum is + /// exponential over them (the same hang the asset-lock union path avoids). + /// `LargestFirst`'s linear greedy accumulator also minimizes the input + /// count — fewer signer round-trips and a smaller tx/fee. + /// + /// ## Parameters + /// + /// * `outputs` — the recipient `(address, amount_duffs)` pairs. Must be + /// non-empty and every amount must be positive. + /// * `fee_per_kb` — fee rate in duffs/kB, or `None` for the default + /// (`1000`). + /// * `signer` — the ECDSA signer that produces each input's P2PKH signature + /// (the Keychain/Keystore-backed `MnemonicResolverCoreSigner` in + /// production). No private key crosses the boundary. + /// + /// [`AssetLockManager::build_asset_lock_tx_from_all_funding_accounts`]: + /// crate::wallet::asset_lock + pub async fn build_signed_payment( + &self, + outputs: Vec<(DashAddress, u64)>, + fee_per_kb: Option, + signer: &S, + ) -> Result { + if outputs.is_empty() { + return Err(PlatformWalletError::TransactionBuild( + "at least one output is required".to_string(), + )); + } + if outputs.iter().any(|(_, amount)| *amount == 0) { + return Err(PlatformWalletError::TransactionBuild( + "every output amount must be greater than zero".to_string(), + )); + } + let outputs_total: u64 = outputs.iter().map(|(_, amount)| *amount).sum(); + + let mut wm = self.wallet_manager.write().await; + let (wallet, info) = wm + .get_wallet_and_info_mut(&self.wallet_id) + .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; + + let height = info.core_wallet.last_processed_height(); + let fee_rate = FeeRate::new(fee_per_kb.unwrap_or(DEFAULT_FEE_PER_KB)); + + // The PRIMARY account (change destination). Clone the xpub-bearing + // account so no immutable borrow of `wallet` is held across the mutable + // `info` borrow / signer await below. + let primary_account = wallet + .get_bip44_account(PRIMARY_BIP44_ACCOUNT_INDEX) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "BIP44 account {PRIMARY_BIP44_ACCOUNT_INDEX} not found for payment funding" + )) + })? + .clone(); + + // Snapshot the primary account's spendable outpoints so the union sweep + // does not double-add them: `set_funding` already seeds them, and + // `add_inputs` must contribute only the OTHER signable accounts. + let primary_outpoints: HashSet = info + .core_wallet + .accounts + .standard_bip44_accounts + .get(&PRIMARY_BIP44_ACCOUNT_INDEX) + .map(|a| { + a.spendable_utxos(height) + .into_iter() + .map(|u| u.outpoint) + .collect() + }) + .unwrap_or_default(); + + // Single immutable pass over every signable funds account, building: + // (a) an owned `Address -> DerivationPath` resolver spanning all + // signable inputs, so signing resolves a key for an input drawn + // from any account; + // (b) the explicit extra inputs (all signable non-primary accounts); + // (c) an `OutPoint -> value` map for the post-build change figure; + // (d) the total selectable value, for a typed shortfall error. + let mut path_map: HashMap = HashMap::new(); + let mut input_value: HashMap = HashMap::new(); + let mut extra_inputs: Vec = Vec::new(); + let mut selectable_value: u64 = 0; + for account in info.core_wallet.accounts.all_funding_accounts() { + if is_watch_only_funds_account(account) { + continue; + } + for utxo in account.spendable_utxos(height) { + selectable_value = selectable_value.saturating_add(utxo.value()); + input_value.insert(utxo.outpoint, utxo.value()); + if let Some(path) = account.address_derivation_path(&utxo.address) { + path_map.insert(utxo.address.clone(), path); + } + if !primary_outpoints.contains(&utxo.outpoint) { + extra_inputs.push(utxo.clone()); + } + } + } + + // Seed the primary account (inputs + change address + reservations), + // append the union of the other signable accounts' inputs, then add the + // real recipient outputs. The `&mut` borrow of the primary account is + // scoped to this block; the returned builder owns cloned inputs / + // reservations / change address, so no account borrow is held across + // the signer await below. + let builder = { + let primary_funds = info + .core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&PRIMARY_BIP44_ACCOUNT_INDEX) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "managed BIP44 account {PRIMARY_BIP44_ACCOUNT_INDEX} not found for \ + payment funding" + )) + })?; + let mut builder = TransactionBuilder::new() + .set_fee_rate(fee_rate) + .set_current_height(height) + // See the doc-comment: LargestFirst, not the default + // BranchAndBound, to keep CoinJoin's many small denominations + // from blowing up the exact-match subset-sum search. + .set_selection_strategy(SelectionStrategy::LargestFirst) + .set_funding(primary_funds, &primary_account) + .add_inputs(extra_inputs); + for (address, amount) in &outputs { + builder = builder.add_output(address, *amount); + } + builder + }; + + let (transaction, _estimated_fee) = builder + .build_signed(signer, move |addr| path_map.get(&addr).cloned()) + .await + .map_err(|e| map_send_builder_error(e, selectable_value, outputs_total))?; + + // Derive fee and change from the transaction itself — the ground truth + // that is always self-consistent (`fee + outputs + change == inputs`). + // We do NOT use `build_signed`'s returned fee: it recomputes the fee + // from the *signed* size, but the change output was already sized with + // the pre-sign estimate, and ECDSA signatures vary in encoded length — + // so the recomputed figure can differ by a few duffs from the fee the + // wallet actually pays (`inputs − outputs`). + // + // `total_out` is the sum of every output; the only non-recipient output + // a plain payment (no special payload) can carry is the single change + // output back to the primary account, so `change = total_out − outputs`. + // Any selected input we somehow can't price (impossible — every + // spendable UTXO was recorded above) counts as 0, so `fee` is over- + // rather than under-reported. + let selected_input_value: u64 = transaction + .input + .iter() + .map(|txin| input_value.get(&txin.previous_output).copied().unwrap_or(0)) + .sum(); + let total_out: u64 = transaction.output.iter().map(|o| o.value).sum(); + let fee = selected_input_value.saturating_sub(total_out); + let change_amount = total_out.saturating_sub(outputs_total); + + Ok(SignedCorePayment { + transaction, + fee, + change_amount, + }) + } +} + +/// Map a key-wallet [`BuilderError`] to a [`PlatformWalletError`], promoting the +/// two shortfall shapes to the typed [`PlatformWalletError::PaymentInsufficientFunds`] +/// so the exact `available`/`required` duff amounts survive. The builder's own +/// `InsufficientFunds` figures cover only what the primary-account selector saw, +/// so we substitute the union-wide selectable total (`available`) and the +/// outputs-plus-fee-ish target — `required` is at least the outputs total; a +/// coin-selection error already carries the fee-inclusive figure, which we +/// prefer when present. +fn map_send_builder_error( + error: BuilderError, + union_available: u64, + outputs_total: u64, +) -> PlatformWalletError { + match error { + BuilderError::InsufficientFunds { required, .. } => { + PlatformWalletError::PaymentInsufficientFunds { + available: union_available, + required: required.max(outputs_total), + } + } + BuilderError::CoinSelection(SelectionError::InsufficientFunds { required, .. }) => { + PlatformWalletError::PaymentInsufficientFunds { + available: union_available, + required: required.max(outputs_total), + } + } + BuilderError::CoinSelection(SelectionError::NoUtxosAvailable) => { + PlatformWalletError::PaymentInsufficientFunds { + available: union_available, + required: outputs_total, + } + } + other => PlatformWalletError::TransactionBuild(format!("payment build failed: {other}")), + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + use std::sync::Arc; + + use dashcore::hashes::Hash; + use dashcore::{Address as DashAddress, Network, OutPoint, TxOut, Txid}; + use key_wallet::account::account_type::StandardAccountType; + use key_wallet::account::AccountType; + use key_wallet::managed_account::ManagedCoreFundsAccount; + use key_wallet::Utxo; + + use crate::test_support::{ + funded_wallet_manager, split_funded_wallet_manager, AlwaysRejectedBroadcaster, + }; + use crate::wallet::core::balance::WalletBalance; + use crate::wallet::core::CoreWallet; + use crate::wallet::platform_wallet::WalletId; + use crate::PlatformWalletError; + + use super::SignedCorePayment; + + /// A `CoreWallet` over a manager fixture. The send path never broadcasts, + /// so the broadcaster is irrelevant (and the balance handle is unused by + /// build — a fresh one is fine for the split fixtures that don't return it). + fn core_wallet( + wallet_manager: Arc>>, + wallet_id: WalletId, + balance: Arc, + ) -> CoreWallet { + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + CoreWallet::new( + sdk, + wallet_manager, + wallet_id, + Arc::new(AlwaysRejectedBroadcaster), + balance, + ) + } + + fn recipient(seed: u8) -> DashAddress { + DashAddress::dummy(Network::Testnet, seed as usize) + } + + /// Every input of a signed tx must carry a non-empty scriptSig (proof each + /// selected input was actually signed by the per-account resolver). + fn assert_all_inputs_signed(payment: &SignedCorePayment) { + for (i, txin) in payment.transaction.input.iter().enumerate() { + assert!( + !txin.script_sig.is_empty(), + "input {i} was left unsigned (empty scriptSig)" + ); + } + } + + /// A single-account BIP44 payment: the recipient output is present with the + /// exact value, a fee is charged, and the change amount is exactly + /// selected_input − output − fee (here the whole 0.1 DASH rides on one + /// input, so change ≈ 0.1 − amount − fee). + #[tokio::test] + async fn bip44_payment_has_correct_output_change_and_fee() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(wm, wallet_id, balance); + + let to = recipient(42); + let amount = 1_000_000u64; + let payment = core + .build_signed_payment(vec![(to.clone(), amount)], None, &signer) + .await + .expect("build should succeed with 0.1 DASH funded"); + + // Recipient output present with the exact value. + let recipient_out = payment + .transaction + .output + .iter() + .find(|o| o.script_pubkey == to.script_pubkey()); + assert_eq!( + recipient_out.map(|o| o.value), + Some(amount), + "recipient output must carry the requested amount" + ); + + // A fee was charged and change is exactly input − output − fee. + assert!(payment.fee > 0, "a non-zero fee should be charged"); + assert_eq!( + payment.change_amount, + 10_000_000 - amount - payment.fee, + "change must be the single input minus the output minus the fee" + ); + // The change output pays the leftover back to the wallet. + assert!( + payment + .transaction + .output + .iter() + .any(|o| o.value == payment.change_amount), + "a change output equal to change_amount should exist" + ); + assert_all_inputs_signed(&payment); + } + + /// Coin selection spans the UNION of signable funds accounts: a payment + /// that exceeds either the BIP44 slice or the CoinJoin slice alone pulls + /// inputs from BOTH, and every mixed-account input is signed. + #[tokio::test] + async fn payment_funds_from_bip44_and_coinjoin_union() { + // 0.09 DASH on BIP44, 0.09 on CoinJoin; ask 0.15 → needs both. + let (wm, wallet_id, signer) = split_funded_wallet_manager(9_000_000, 9_000_000).await; + + // Snapshot each account's outpoints before building. + let (bip44_ops, coinjoin_ops): (HashSet, HashSet) = { + let guard = wm.read().await; + let (_, info) = guard.get_wallet_and_info(&wallet_id).expect("wallet present"); + let bip44 = info + .core_wallet + .accounts + .standard_bip44_accounts + .get(&0) + .map(|a| a.utxos.keys().copied().collect()) + .unwrap_or_default(); + let coinjoin = info + .core_wallet + .accounts + .coinjoin_accounts + .get(&0) + .map(|a| a.utxos.keys().copied().collect()) + .unwrap_or_default(); + (bip44, coinjoin) + }; + + let core = core_wallet(wm, wallet_id, Arc::new(WalletBalance::new())); + let payment = core + .build_signed_payment(vec![(recipient(7), 15_000_000)], None, &signer) + .await + .expect("0.15 DASH must be fundable from the 0.18 DASH union"); + + let spent: HashSet = payment + .transaction + .input + .iter() + .map(|i| i.previous_output) + .collect(); + assert!( + spent.iter().any(|op| bip44_ops.contains(op)), + "at least one BIP44 input should be selected" + ); + assert!( + spent.iter().any(|op| coinjoin_ops.contains(op)), + "at least one CoinJoin input should be selected" + ); + assert_all_inputs_signed(&payment); + } + + /// A shortfall across the whole signable union surfaces as the typed + /// [`PlatformWalletError::PaymentInsufficientFunds`], with `available` + /// reflecting the union total (not just the primary BIP44 slice). + #[tokio::test] + async fn union_shortfall_is_typed() { + let (wm, wallet_id, signer) = split_funded_wallet_manager(9_000_000, 9_000_000).await; + let core = core_wallet(wm, wallet_id, Arc::new(WalletBalance::new())); + + let result = core + .build_signed_payment(vec![(recipient(7), 100_000_000)], None, &signer) + .await; + + match result { + Err(PlatformWalletError::PaymentInsufficientFunds { + available, + required, + }) => { + assert!( + (9_000_000..=18_000_000).contains(&available), + "available {available} should reflect the union (9M<..<=18M)" + ); + assert!( + required >= 100_000_000, + "required {required} should be at least the requested amount" + ); + } + other => panic!("expected PaymentInsufficientFunds, got {other:?}"), + } + } + + /// A watch-only `DashpayExternalAccount` (a contact's addresses, which this + /// wallet cannot sign) is EXCLUDED from coin selection: its UTXO is never + /// spent, and its value is not counted toward the selectable total. + #[tokio::test] + async fn watch_only_external_account_is_excluded() { + // BIP44 holds 0.1 DASH; a watch-only external account holds 1.0 DASH. + let (wm, wallet_id, _balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + + let watch_only_outpoint = OutPoint { + txid: Txid::from_byte_array([0x9au8; 32]), + vout: 0, + }; + { + let mut guard = wm.write().await; + let (wallet, info) = guard + .get_wallet_mut_and_info_mut(&wallet_id) + .expect("wallet present"); + + // Reuse the wallet's own BIP44 xpub as a stand-in "contact xpub": + // the exclusion happens before any address derivation, so any valid + // xpub suffices to construct the funds-bearing external account. + let contact_xpub = wallet + .accounts + .standard_bip44_accounts + .get(&0) + .expect("bip44 account 0") + .account_xpub; + let account_type = AccountType::DashpayExternalAccount { + index: 0, + user_identity_id: [1u8; 32], + friend_identity_id: [2u8; 32], + }; + let account = key_wallet::Account { + parent_wallet_id: Some(wallet_id), + account_type, + network: Network::Testnet, + account_xpub: contact_xpub, + is_watch_only: true, + }; + let mut managed = ManagedCoreFundsAccount::from_account(&account); + + // Insert a large spendable UTXO directly (arbitrary address — the + // account is skipped before its addresses are ever consulted). + let addr = recipient(200); + let utxo = Utxo { + outpoint: watch_only_outpoint, + txout: TxOut { + value: 100_000_000, + script_pubkey: addr.script_pubkey(), + }, + address: addr, + height: 1, + is_coinbase: false, + is_confirmed: true, + is_instantlocked: false, + is_locked: false, + is_trusted: false, + }; + managed.utxos.insert(utxo.outpoint, utxo); + info.core_wallet + .accounts + .insert_funds_bearing_account(managed) + .expect("insert watch-only external account"); + } + + let core = core_wallet(wm, wallet_id, Arc::new(WalletBalance::new())); + + // Ask for 0.5 DASH: covered only if the 1.0-DASH watch-only UTXO were + // spendable. Since it is excluded, the build must fail — and the + // reported `available` must be just the 0.1-DASH BIP44 slice. + let result = core + .build_signed_payment(vec![(recipient(7), 50_000_000)], None, &signer) + .await; + match result { + Err(PlatformWalletError::PaymentInsufficientFunds { available, .. }) => { + assert_eq!( + available, 10_000_000, + "watch-only value must be excluded from the selectable total" + ); + } + other => panic!("expected PaymentInsufficientFunds, got {other:?}"), + } + + // And a payment that the 0.1-DASH BIP44 slice CAN cover must never spend + // the watch-only outpoint. + let payment = core + .build_signed_payment(vec![(recipient(7), 1_000_000)], None, &signer) + .await + .expect("0.01 DASH is fundable from the BIP44 slice alone"); + assert!( + payment + .transaction + .input + .iter() + .all(|i| i.previous_output != watch_only_outpoint), + "the watch-only UTXO must never be selected as an input" + ); + assert_all_inputs_signed(&payment); + } + + /// Input validation: empty outputs and zero-amount outputs are rejected + /// before any wallet work. + #[tokio::test] + async fn rejects_empty_and_zero_outputs() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(wm, wallet_id, balance); + + let empty = core.build_signed_payment(vec![], None, &signer).await; + assert!(matches!(empty, Err(PlatformWalletError::TransactionBuild(_)))); + + let zero = core + .build_signed_payment(vec![(recipient(7), 0)], None, &signer) + .await; + assert!(matches!(zero, Err(PlatformWalletError::TransactionBuild(_)))); + } +} From 9a104d54d6ca481d71706d0810e7a1cc9d412a85 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:49:34 -0400 Subject: [PATCH 2/7] feat(sdk): plumb build_signed_payment through FFI, JNI, and Kotlin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expose the union-funding send primitive as a public Kotlin suspend fun that returns the signed raw transaction bytes (plus fee + change) WITHOUT broadcasting — the last SDK gap for the Android wallet's send-path cutover. - FFI (rs-platform-wallet-ffi): core_wallet_build_signed_payment, a one-shot call over CoreWallet::build_signed_payment. Recipients cross as a big-endian blob (u32 count; per row u32 addrLen, addr utf8, u64 amount), each address parsed + network-checked; returns the consensus-serialized signed bytes via out-pointers plus out_fee/out_change, freed with core_wallet_free_payment_bytes. cbindgen exports both automatically. - JNI (rs-unified-sdk-jni): WalletManagerNative.coreWalletBuildSignedPayment returns a byte[] packed big-endian as (u64 fee, u64 change, tx bytes); the FFI-owned bytes are freed before returning. - Kotlin (kotlin-sdk): ManagedPlatformWallet.buildSignedPayment(recipients, coreSignerHandle, feePerKb) -> SignedCorePayment(txBytes, fee, change), serialized under the same shared per-wallet coreSendMutex as sendToAddresses so a concurrent build cannot select the same UTXO. Unlike sendToAddresses it neither broadcasts nor picks a funding account (coin selection auto-spans every signable account). ManagedCoreWallet gains the matching native call on the transient core handle. cargo check/test green across platform-wallet-ffi (149) and rs-unified-sdk-jni (2); :sdk:compileDebugKotlin BUILD SUCCESSFUL. Co-Authored-By: Claude Fable 5 (cherry picked from commit d18c9c0b4082ff13e7a7771fd8ca280c0a88a915) [port to v4.2-dev] ManagedPlatformWallet.buildSignedPayment is serialized through the manager's TeardownGate (gate.op {}), matching sendToAddresses and every other native op on this branch, instead of the pre-refactor per-wallet `coreSendMutex` (which no longer exists on v4.2-dev). Concurrent builds still cannot select the same UTXO: CoreWallet::build_signed_payment holds the wallet-manager write lock across coin selection and signing. --- .../dashsdk/ffi/WalletManagerNative.kt | 21 +++ .../dashsdk/wallet/ManagedCoreWallet.kt | 20 +++ .../dashsdk/wallet/ManagedPlatformWallet.kt | 106 +++++++++++ .../src/core_wallet/mod.rs | 2 + .../src/core_wallet/send.rs | 169 ++++++++++++++++++ .../rs-unified-sdk-jni/src/wallet_manager.rs | 91 ++++++++++ 6 files changed, 409 insertions(+) create mode 100644 packages/rs-platform-wallet-ffi/src/core_wallet/send.rs diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt index 0dfbbedc89..bf05825406 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt @@ -211,6 +211,27 @@ internal object WalletManagerNative { */ external fun platformWalletGetCore(walletHandle: Long): Long + /** + * `core_wallet_build_signed_payment` — build + sign a standard L1 payment + * funded from the UNION of the wallet's signable funds accounts (watch-only + * DashPay external accounts excluded), WITHOUT broadcasting. + * + * [coreHandle] is a core-wallet handle from [platformWalletGetCore]. + * [outputsBlob] encodes the recipients big-endian as `u32 count` then per + * row `u32 addrLen, addr utf8, u64 amount`. [feePerKb] is duffs/kB (0 = + * default). [coreSignerHandle] is the manager's `MnemonicResolverHandle`. + * + * Returns a `byte[]` packed big-endian as `u64 fee, u64 change,` then the + * consensus-serialized signed transaction bytes (0-length / null after + * throwing). Does NOT broadcast and does NOT persist a debit. + */ + external fun coreWalletBuildSignedPayment( + coreHandle: Long, + outputsBlob: ByteArray, + feePerKb: Long, + coreSignerHandle: Long, + ): ByteArray + /** * `core_wallet_broadcast_transaction` — broadcast a transaction built by * [coreTxBuilderBuildSigned]. [accountType]/[accountIndex] identify the diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt index 8a0e661d0e..b7f6764b4a 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt @@ -55,6 +55,26 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { ) } + /** + * Build + sign a standard L1 payment funded from the UNION of the wallet's + * signable funds accounts, WITHOUT broadcasting. Returns the packed native + * result (`u64 fee, u64 change,` then the signed tx bytes, big-endian) — + * decoded by [ManagedPlatformWallet.buildSignedPayment]. See that method + * for the full contract; drive this through it (it serializes concurrent + * builds), not directly. + */ + internal fun buildSignedPayment( + outputsBlob: ByteArray, + feePerKb: Long, + coreSignerHandle: Long, + ): ByteArray = + WalletManagerNative.coreWalletBuildSignedPayment( + handle, + outputsBlob, + feePerKb, + coreSignerHandle, + ) + override fun close() { cleanable.clean() } diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt index ba05a9ff7f..ae40b8cb4c 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt @@ -172,6 +172,77 @@ class ManagedPlatformWallet internal constructor( } } + /** + * A built-and-signed Core L1 payment that was NOT broadcast — the output of + * [buildSignedPayment]. [txBytes] is the consensus-serialized signed + * transaction the caller commits/broadcasts itself (dashj during the + * dashj→SDK transition; the SDK's own broadcast afterwards). [fee] and + * [change] are duffs. + */ + data class SignedCorePayment( + val txBytes: ByteArray, + val fee: Long, + val change: Long, + ) { + override fun equals(other: Any?): Boolean = + other is SignedCorePayment && + txBytes.contentEquals(other.txBytes) && + fee == other.fee && + change == other.change + + override fun hashCode(): Int = + (31 * txBytes.contentHashCode() + fee.hashCode()) * 31 + change.hashCode() + } + + /** + * Build and sign a Core L1 payment to [recipients], funding it from the + * UNION of this wallet's signable funds accounts (BIP44 + BIP32 + CoinJoin + * + DashPay receiving; watch-only DashPay external accounts excluded), and + * return the signed raw transaction bytes plus the fee and change — + * **WITHOUT broadcasting**. + * + * This is the transition-era "give me signed bytes" primitive: the Android + * wallet hands [SignedCorePayment.txBytes] to dashj for commit + broadcast + * (keeping dashj's `maybeCommitTx` bookkeeping — CrowdNode, memos, + * confidence listeners), while the SDK owns coin selection and signing. It + * does not broadcast and does not persist a debit; the selected inputs are + * only reserved in memory (released when the spend is later observed by sync + * or by the reservation-TTL backstop). Coin selection auto-spans every + * signable account, so — unlike [sendToAddresses] — the caller does not + * pick a funding account. + * + * Runs through the manager's [TeardownGate] like every other native op. + * A concurrent build cannot select the same UTXO because the underlying + * `build_signed_payment` holds the wallet-manager write lock across coin + * selection and signing (the same native serialization [sendToAddresses] + * relies on). + * + * @param recipients `(address, amountDuffs)` pairs; must be non-empty and + * every amount positive. + * @param coreSignerHandle the manager's `MnemonicResolverHandle` + * (`PlatformWalletManager.mnemonicResolverHandle`); no private key crosses + * the boundary. + * @param feePerKb fee rate in duffs/kB, or 0 for the SDK default. + */ + suspend fun buildSignedPayment( + recipients: List>, + coreSignerHandle: Long, + feePerKb: Long = 0, + ): SignedCorePayment = gate.op { + require(recipients.isNotEmpty()) { "recipients must not be empty" } + require(recipients.all { it.second > 0 }) { "every recipient amount must be positive" } + require(feePerKb >= 0) { "feePerKb must be non-negative, got $feePerKb" } + + val outputsBlob = encodePaymentOutputs(recipients) + mapNativeErrors { + coreWallet().use { core -> + decodeSignedPayment( + core.buildSignedPayment(outputsBlob, feePerKb, coreSignerHandle), + ) + } + } + } + /** * The wallet's Platform-payment addresses that currently hold credits, * each as a [FundingInput] whose `credits` is the full cached balance — @@ -617,6 +688,41 @@ class ManagedPlatformWallet internal constructor( return out.toByteArray() } + /** + * Encode [recipients] to the payment-outputs blob + * `core_wallet_build_signed_payment` reads: `u32 count` then per row + * `u32 addrLen, addr utf8 bytes, u64 amount` (all big-endian, matching + * `DataOutputStream`'s wire order and the Rust `from_be_bytes` decoder). + */ + private fun encodePaymentOutputs(recipients: List>): ByteArray { + val out = java.io.ByteArrayOutputStream() + val dos = java.io.DataOutputStream(out) + dos.writeInt(recipients.size) + for ((address, amount) in recipients) { + val addrBytes = address.toByteArray(Charsets.UTF_8) + dos.writeInt(addrBytes.size) + dos.write(addrBytes) + dos.writeLong(amount) + } + return out.toByteArray() + } + + /** + * Decode the packed [SignedCorePayment] the native build returns: + * `u64 fee, u64 change,` then the signed transaction bytes (big-endian). + */ + private fun decodeSignedPayment(packed: ByteArray): SignedCorePayment { + require(packed.size >= 16) { + "signed-payment result too short (${packed.size} bytes, need >= 16)" + } + val buffer = java.nio.ByteBuffer.wrap(packed) // big-endian by default + val fee = buffer.long + val change = buffer.long + val txBytes = ByteArray(buffer.remaining()) + buffer.get(txBytes) + return SignedCorePayment(txBytes = txBytes, fee = fee, change = change) + } + /** * Encode [recipients] to the funding-recipients blob the FFI reads: * `u32 rowCount` then per row `u8 addressType, u8[20] hash, diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs index 8e12ebc178..126ce1a050 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs @@ -4,10 +4,12 @@ mod addresses; mod broadcast; +mod send; mod transaction_builder; mod wallet; pub use addresses::*; pub use broadcast::*; +pub use send::*; pub use transaction_builder::*; pub use wallet::*; diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs new file mode 100644 index 0000000000..90724a7ca0 --- /dev/null +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs @@ -0,0 +1,169 @@ +//! FFI binding for the union-funding "build a signed payment" primitive. +//! +//! Unlike the step-by-step `core_wallet_tx_builder_*` builder (which funds from +//! a single caller-chosen account), this is a one-shot call that funds a +//! standard L1 payment from the UNION of every signable funds account +//! (BIP44 + BIP32 + CoinJoin + DashPay receiving; watch-only DashPay external +//! accounts are excluded) and returns the **signed serialized transaction +//! bytes** plus the computed fee and change amount. It does NOT broadcast and +//! does NOT persist a debit — the caller commits/broadcasts the returned bytes +//! itself (dashj during the Android transition; a later SDK-broadcast mode +//! afterwards). See `platform_wallet::wallet::core::send` for the semantics. + +use crate::error::*; +use crate::handle::{Handle, CORE_WALLET_STORAGE}; +use crate::runtime::runtime; +use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; +use dashcore::Address as DashAddress; +use platform_wallet::PlatformWalletError; +use rs_sdk_ffi::{MnemonicResolverCoreSigner, MnemonicResolverHandle}; +use std::str::FromStr; + +/// Decode the recipients blob the caller passes to +/// [`core_wallet_build_signed_payment`]. Layout (big-endian): +/// +/// ```text +/// u32 count +/// count × ( u32 address_len, address_len bytes (UTF-8), u64 amount_duffs ) +/// ``` +/// +/// Each address is parsed and checked against `network`; a malformed blob or a +/// wrong-network / unparseable address is a decode error. +fn decode_payment_outputs( + blob: &[u8], + network: dashcore::Network, +) -> Result, PlatformWalletError> { + let err = |m: String| PlatformWalletError::TransactionBuild(m); + let mut cursor = 0usize; + let read_u32 = |buf: &[u8], at: &mut usize| -> Result { + let end = *at + 4; + if end > buf.len() { + return Err(PlatformWalletError::TransactionBuild( + "truncated recipients blob (u32)".to_string(), + )); + } + let v = u32::from_be_bytes([buf[*at], buf[*at + 1], buf[*at + 2], buf[*at + 3]]); + *at = end; + Ok(v) + }; + let read_u64 = |buf: &[u8], at: &mut usize| -> Result { + let end = *at + 8; + if end > buf.len() { + return Err(PlatformWalletError::TransactionBuild( + "truncated recipients blob (u64)".to_string(), + )); + } + let mut b = [0u8; 8]; + b.copy_from_slice(&buf[*at..end]); + *at = end; + Ok(u64::from_be_bytes(b)) + }; + + let count = read_u32(blob, &mut cursor)? as usize; + let mut outputs = Vec::with_capacity(count); + for _ in 0..count { + let addr_len = read_u32(blob, &mut cursor)? as usize; + let end = cursor + addr_len; + if end > blob.len() { + return Err(err("truncated recipients blob (address)".to_string())); + } + let addr_str = std::str::from_utf8(&blob[cursor..end]) + .map_err(|e| err(format!("recipient address is not valid UTF-8: {e}")))?; + cursor = end; + let amount = read_u64(blob, &mut cursor)?; + + let parsed = DashAddress::from_str(addr_str) + .map_err(|e| err(format!("invalid recipient address {addr_str:?}: {e}")))?; + let address = parsed + .require_network(network) + .map_err(|e| err(format!("recipient address {addr_str:?} network mismatch: {e}")))?; + outputs.push((address, amount)); + } + Ok(outputs) +} + +/// Build and sign a standard L1 payment from the wallet's signable funds +/// accounts (union coin selection) and return the signed bytes + fee + change. +/// +/// * `handle` — a core-wallet handle (`platform_wallet_get_core`). +/// * `outputs_blob`/`outputs_blob_len` — the recipients, encoded as documented +/// on [`decode_payment_outputs`]. +/// * `fee_per_kb` — fee rate in duffs/kB, or `0` for the default (1000). +/// * `core_signer_handle` — the caller's `MnemonicResolverHandle`; ownership is +/// retained by the caller (this function does NOT destroy it). +/// * `out_tx_bytes`/`out_tx_len` — receive the consensus-serialized signed +/// transaction. Free with [`core_wallet_free_payment_bytes`]. +/// * `out_fee` — receives the fee paid, in duffs. +/// * `out_change` — receives the change returned to the wallet, in duffs (0 if +/// the build produced no change output). +/// +/// # Safety +/// All pointers must be valid; `outputs_blob` must be readable for +/// `outputs_blob_len` bytes; the out-pointers must be writable. +#[no_mangle] +#[allow(clippy::too_many_arguments)] +pub unsafe extern "C" fn core_wallet_build_signed_payment( + handle: Handle, + outputs_blob: *const u8, + outputs_blob_len: usize, + fee_per_kb: u64, + core_signer_handle: *mut MnemonicResolverHandle, + out_tx_bytes: *mut *mut u8, + out_tx_len: *mut usize, + out_fee: *mut u64, + out_change: *mut u64, +) -> PlatformWalletFFIResult { + check_ptr!(outputs_blob); + check_ptr!(core_signer_handle); + check_ptr!(out_tx_bytes); + check_ptr!(out_tx_len); + check_ptr!(out_fee); + check_ptr!(out_change); + + let blob = std::slice::from_raw_parts(outputs_blob, outputs_blob_len); + let signer_addr = core_signer_handle as usize; + let fee = if fee_per_kb == 0 { + None + } else { + Some(fee_per_kb) + }; + + let option = CORE_WALLET_STORAGE.with_item(handle, |wallet| { + let network = wallet.network(); + let outputs = decode_payment_outputs(blob, network)?; + let wallet_id = wallet.wallet_id(); + // SAFETY: `signer_addr` came from `core_signer_handle`, which the caller + // pinned alive for this call; the `MnemonicResolverCoreSigner` lives + // only on this stack frame and is dropped before returning. + let signer = MnemonicResolverCoreSigner::new( + signer_addr as *mut MnemonicResolverHandle, + wallet_id, + network, + ); + runtime().block_on(wallet.build_signed_payment(outputs, fee, &signer)) + }); + + let result = unwrap_option_or_return!(option); + let payment = unwrap_result_or_return!(result); + + let serialized = dashcore::consensus::serialize(&payment.transaction); + let len = serialized.len(); + *out_tx_bytes = Box::into_raw(serialized.into_boxed_slice()) as *mut u8; + *out_tx_len = len; + *out_fee = payment.fee; + *out_change = payment.change_amount; + + PlatformWalletFFIResult::ok() +} + +/// Free the signed-payment bytes returned by [`core_wallet_build_signed_payment`]. +/// +/// # Safety +/// `bytes`/`len` must be the exact pair written to `out_tx_bytes`/`out_tx_len` +/// by [`core_wallet_build_signed_payment`] (or null / 0). +#[no_mangle] +pub unsafe extern "C" fn core_wallet_free_payment_bytes(bytes: *mut u8, len: usize) { + if !bytes.is_null() && len > 0 { + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(bytes, len)); + } +} diff --git a/packages/rs-unified-sdk-jni/src/wallet_manager.rs b/packages/rs-unified-sdk-jni/src/wallet_manager.rs index 8e0f3e676b..f3731ef23c 100644 --- a/packages/rs-unified-sdk-jni/src/wallet_manager.rs +++ b/packages/rs-unified-sdk-jni/src/wallet_manager.rs @@ -1013,6 +1013,97 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_c }) } +/// `core_wallet_build_signed_payment` — build + sign a standard L1 payment +/// funded from the UNION of the wallet's signable funds accounts (BIP44 + +/// BIP32 + CoinJoin + DashPay receiving; watch-only DashPay external accounts +/// excluded), and return the result WITHOUT broadcasting. +/// +/// `core_handle` is the transient core-wallet `Handle` from +/// [platformWalletGetCore]. `outputs_blob` is the recipients, encoded +/// big-endian as `u32 count` then per row `u32 addrLen, addr utf8, u64 amount` +/// (`ManagedPlatformWallet.encodePaymentOutputs`). `fee_per_kb` is duffs/kB, or +/// 0 for the default. `core_signer_handle` is the manager's +/// `MnemonicResolverHandle`. +/// +/// Returns a `byte[]` packed big-endian as `u64 fee, u64 change,` then the +/// consensus-serialized signed transaction bytes (`fee` and `change` in duffs), +/// or null after throwing. The FFI-owned tx bytes are freed here before +/// returning; Kotlin decodes the packed array via +/// `ManagedPlatformWallet.decodeSignedPayment`. +#[no_mangle] +pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_coreWalletBuildSignedPayment( + mut env: JNIEnv, + _class: JClass, + core_handle: jlong, + outputs_blob: JByteArray, + fee_per_kb: jlong, + core_signer_handle: jlong, +) -> jbyteArray { + guard(&mut env, ptr::null_mut(), |env| { + if core_handle == 0 { + throw_sdk_exception(env, 1, "core handle is 0"); + return ptr::null_mut(); + } + if core_signer_handle == 0 { + throw_sdk_exception(env, 1, "coreSignerHandle is 0"); + return ptr::null_mut(); + } + if fee_per_kb < 0 { + throw_sdk_exception(env, 1, "feePerKb must be non-negative"); + return ptr::null_mut(); + } + let blob = match env.convert_byte_array(&outputs_blob) { + Ok(b) => b, + Err(_) => { + let _ = env.exception_clear(); + throw_sdk_exception(env, 1, "outputs byte[] was invalid"); + return ptr::null_mut(); + } + }; + + let mut out_tx_bytes: *mut u8 = ptr::null_mut(); + let mut out_tx_len: usize = 0; + let mut out_fee: u64 = 0; + let mut out_change: u64 = 0; + let result = unsafe { + platform_wallet_ffi::core_wallet_build_signed_payment( + core_handle as Handle, + blob.as_ptr(), + blob.len(), + fee_per_kb as u64, + core_signer_handle as *mut rs_sdk_ffi::MnemonicResolverHandle, + &mut out_tx_bytes, + &mut out_tx_len, + &mut out_fee, + &mut out_change, + ) + }; + if take_pwffi_error(env, result) { + return ptr::null_mut(); + } + + // Copy the FFI-owned tx bytes out, then free them, then pack the + // metadata-prefixed result for Kotlin. `fee` and `change` are written + // big-endian ahead of the raw tx bytes. + let tx_bytes: &[u8] = if out_tx_bytes.is_null() || out_tx_len == 0 { + &[] + } else { + unsafe { std::slice::from_raw_parts(out_tx_bytes, out_tx_len) } + }; + let mut packed = Vec::with_capacity(16 + tx_bytes.len()); + packed.extend_from_slice(&out_fee.to_be_bytes()); + packed.extend_from_slice(&out_change.to_be_bytes()); + packed.extend_from_slice(tx_bytes); + unsafe { + platform_wallet_ffi::core_wallet_free_payment_bytes(out_tx_bytes, out_tx_len); + } + + env.byte_array_from_slice(&packed) + .map(|a| a.into_raw()) + .unwrap_or(ptr::null_mut()) + }) +} + /// `platform_wallet_get_core` — resolve the transient core-wallet `Handle` /// (as `jlong`) from a `PlatformWallet` handle, for [coreWalletBroadcastTransaction]. /// Free with [coreWalletDestroy]. Returns 0 after throwing. From 0809290e43ec56b35e21412bf58d9bac081e0685 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 29 Jul 2026 23:49:35 -0400 Subject: [PATCH 3/7] fix(platform-wallet): re-scope build_signed_payment to single-account funding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build_signed_payment funded from the union of all signable funds accounts (BIP44 + CoinJoin + …) with BIP44 change — the privacy-domain-crossing design blocked on #4184 (shumkov, 2026-07-21) and replaced there by single-account selection. This code predated that re-scope. - add funding_path: Option; None = unmixed BIP44 account 0, Some(path) = strictly the one funds account whose account path matches. No union, no cross-account accumulation; shortfall returns PaymentInsufficientFunds for that account only. Change routes to BIP44 (explicit change addr when a non-Standard account funds). - new wallet::funding_privacy guardrail: crate-wide static test fails the build if any wallet-wide funds-account iteration lacks a PRIVACY-DOMAIN-OK marker. - replace the union-asserting test with default-never-crosses-domains and explicit-path-selects-strictly tests. - review-fix hardening: bounded FFI allocation + checked cursor math, output-total overflow guard, fee-rate bound, typed PaymentInsufficientFunds (code 22). - thread funding_path through FFI/JNI/Kotlin (null = unmixed BIP44). Co-Authored-By: Claude Opus 4.8 --- .../dashsdk/ffi/WalletManagerNative.kt | 11 +- .../dashsdk/wallet/ManagedCoreWallet.kt | 4 +- .../dashsdk/wallet/ManagedPlatformWallet.kt | 30 +- .../src/core_wallet/send.rs | 101 ++- packages/rs-platform-wallet-ffi/src/error.rs | 12 +- packages/rs-platform-wallet-ffi/src/utils.rs | 38 ++ .../rs-platform-wallet/src/test_support.rs | 88 +++ .../src/wallet/core/send.rs | 628 +++++++++++++----- .../src/wallet/funding_privacy.rs | 359 ++++++++++ packages/rs-platform-wallet/src/wallet/mod.rs | 1 + packages/rs-unified-sdk-jni/src/funding.rs | 34 + .../rs-unified-sdk-jni/src/wallet_manager.rs | 30 +- 12 files changed, 1125 insertions(+), 211 deletions(-) create mode 100644 packages/rs-platform-wallet/src/wallet/funding_privacy.rs diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt index bf05825406..60f66658aa 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt @@ -213,13 +213,19 @@ internal object WalletManagerNative { /** * `core_wallet_build_signed_payment` — build + sign a standard L1 payment - * funded from the UNION of the wallet's signable funds accounts (watch-only - * DashPay external accounts excluded), WITHOUT broadcasting. + * funded from ONE of the wallet's signable funds accounts, WITHOUT + * broadcasting. * * [coreHandle] is a core-wallet handle from [platformWalletGetCore]. * [outputsBlob] encodes the recipients big-endian as `u32 count` then per * row `u32 addrLen, addr utf8, u64 amount`. [feePerKb] is duffs/kB (0 = * default). [coreSignerHandle] is the manager's `MnemonicResolverHandle`. + * [fundingPath] is an optional UTF-8 BIP32 derivation-path string + * (dashpay/platform#4184) naming the single funds account whose UTXOs fund + * the payment: null (the default) funds from the unmixed BIP44 account; an + * explicit account-level path (e.g. the DIP-9 CoinJoin account path) funds + * strictly from that one account, with no union across accounts and no + * consent gate. * * Returns a `byte[]` packed big-endian as `u64 fee, u64 change,` then the * consensus-serialized signed transaction bytes (0-length / null after @@ -230,6 +236,7 @@ internal object WalletManagerNative { outputsBlob: ByteArray, feePerKb: Long, coreSignerHandle: Long, + fundingPath: String?, ): ByteArray /** diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt index b7f6764b4a..bb40200f49 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt @@ -56,7 +56,7 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { } /** - * Build + sign a standard L1 payment funded from the UNION of the wallet's + * Build + sign a standard L1 payment funded from ONE of the wallet's * signable funds accounts, WITHOUT broadcasting. Returns the packed native * result (`u64 fee, u64 change,` then the signed tx bytes, big-endian) — * decoded by [ManagedPlatformWallet.buildSignedPayment]. See that method @@ -67,12 +67,14 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { outputsBlob: ByteArray, feePerKb: Long, coreSignerHandle: Long, + fundingPath: String?, ): ByteArray = WalletManagerNative.coreWalletBuildSignedPayment( handle, outputsBlob, feePerKb, coreSignerHandle, + fundingPath, ) override fun close() { diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt index ae40b8cb4c..05b0c409c5 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt @@ -195,11 +195,9 @@ class ManagedPlatformWallet internal constructor( } /** - * Build and sign a Core L1 payment to [recipients], funding it from the - * UNION of this wallet's signable funds accounts (BIP44 + BIP32 + CoinJoin - * + DashPay receiving; watch-only DashPay external accounts excluded), and - * return the signed raw transaction bytes plus the fee and change — - * **WITHOUT broadcasting**. + * Build and sign a Core L1 payment to [recipients], funding it from a + * **single** funds account, and return the signed raw transaction bytes + * plus the fee and change — **WITHOUT broadcasting**. * * This is the transition-era "give me signed bytes" primitive: the Android * wallet hands [SignedCorePayment.txBytes] to dashj for commit + broadcast @@ -207,9 +205,20 @@ class ManagedPlatformWallet internal constructor( * confidence listeners), while the SDK owns coin selection and signing. It * does not broadcast and does not persist a debit; the selected inputs are * only reserved in memory (released when the spend is later observed by sync - * or by the reservation-TTL backstop). Coin selection auto-spans every - * signable account, so — unlike [sendToAddresses] — the caller does not - * pick a funding account. + * or by the reservation-TTL backstop). + * + * **Funding-domain isolation (dashpay/platform#4184).** Coin selection never + * spans accounts. [fundingPath] names the one funds account to draw from; + * `null` (the default) draws from the unmixed BIP44 account. Passing an + * explicit account-level path — e.g. the DIP-9 CoinJoin account path — spends + * previously-mixed coins deliberately, and only those. Unioning ordinary, + * CoinJoin, and DashPay-receiving coins into one transaction would + * irreversibly link those privacy domains on chain, so it is never done + * implicitly: if the named account cannot cover the payment this throws + * [org.dashfoundation.dashsdk.errors.DashSdkError.PlatformWallet.CoreInsufficientFunds] + * rather than reaching into another account — whose `available` figure is + * that ONE account's balance, so the actionable response is to pick a + * different [fundingPath], not to retry the same one. * * Runs through the manager's [TeardownGate] like every other native op. * A concurrent build cannot select the same UTXO because the underlying @@ -223,11 +232,14 @@ class ManagedPlatformWallet internal constructor( * (`PlatformWalletManager.mnemonicResolverHandle`); no private key crosses * the boundary. * @param feePerKb fee rate in duffs/kB, or 0 for the SDK default. + * @param fundingPath optional UTF-8 BIP32 derivation-path string naming the + * single funds account to fund from; `null` = the unmixed BIP44 account. */ suspend fun buildSignedPayment( recipients: List>, coreSignerHandle: Long, feePerKb: Long = 0, + fundingPath: String? = null, ): SignedCorePayment = gate.op { require(recipients.isNotEmpty()) { "recipients must not be empty" } require(recipients.all { it.second > 0 }) { "every recipient amount must be positive" } @@ -237,7 +249,7 @@ class ManagedPlatformWallet internal constructor( mapNativeErrors { coreWallet().use { core -> decodeSignedPayment( - core.buildSignedPayment(outputsBlob, feePerKb, coreSignerHandle), + core.buildSignedPayment(outputsBlob, feePerKb, coreSignerHandle, fundingPath), ) } } diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs index 90724a7ca0..57033e0376 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs @@ -1,24 +1,34 @@ -//! FFI binding for the union-funding "build a signed payment" primitive. +//! FFI binding for the single-account "build a signed payment" primitive. //! -//! Unlike the step-by-step `core_wallet_tx_builder_*` builder (which funds from -//! a single caller-chosen account), this is a one-shot call that funds a -//! standard L1 payment from the UNION of every signable funds account -//! (BIP44 + BIP32 + CoinJoin + DashPay receiving; watch-only DashPay external -//! accounts are excluded) and returns the **signed serialized transaction -//! bytes** plus the computed fee and change amount. It does NOT broadcast and -//! does NOT persist a debit — the caller commits/broadcasts the returned bytes -//! itself (dashj during the Android transition; a later SDK-broadcast mode -//! afterwards). See `platform_wallet::wallet::core::send` for the semantics. +//! Like the step-by-step `core_wallet_tx_builder_*` builder, this funds from a +//! single caller-chosen account — but as a one-shot call that also signs, and +//! it names the account by BIP32 derivation path (so a DIP-9 CoinJoin or +//! DashPay-receiving account can be selected, not just BIP44/BIP32). It returns +//! the **signed serialized transaction bytes** plus the computed fee and change +//! amount. It does NOT broadcast and does NOT persist a debit — the caller +//! commits/broadcasts the returned bytes itself (dashj during the Android +//! transition; a later SDK-broadcast mode afterwards). +//! +//! Coin selection never unions funding accounts: `funding_path` names exactly +//! one, defaulting to the unmixed BIP44 account. See +//! `platform_wallet::wallet::funding_privacy` for the invariant and +//! `platform_wallet::wallet::core::send` for the semantics. use crate::error::*; use crate::handle::{Handle, CORE_WALLET_STORAGE}; use crate::runtime::runtime; +use crate::utils::parse_optional_derivation_path; use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; use dashcore::Address as DashAddress; use platform_wallet::PlatformWalletError; use rs_sdk_ffi::{MnemonicResolverCoreSigner, MnemonicResolverHandle}; use std::str::FromStr; +/// Smallest number of bytes one encoded output row can occupy: `u32 addr_len` +/// (4) + at least one address byte + `u64 amount` (8). Used to reject an +/// impossible `count` before any allocation. +const MIN_ENCODED_OUTPUT_LEN: usize = 4 + 1 + 8; + /// Decode the recipients blob the caller passes to /// [`core_wallet_build_signed_payment`]. Layout (big-endian): /// @@ -35,38 +45,49 @@ fn decode_payment_outputs( ) -> Result, PlatformWalletError> { let err = |m: String| PlatformWalletError::TransactionBuild(m); let mut cursor = 0usize; + // Checked cursor arithmetic throughout: `cursor + n` on a 32-bit target + // (Android armeabi-v7a) can overflow and panic inside this `extern "C"` + // frame, where the JNI guard cannot safely recover it. let read_u32 = |buf: &[u8], at: &mut usize| -> Result { - let end = *at + 4; - if end > buf.len() { - return Err(PlatformWalletError::TransactionBuild( - "truncated recipients blob (u32)".to_string(), - )); - } + let end = at.checked_add(4).filter(|e| *e <= buf.len()).ok_or_else(|| { + PlatformWalletError::TransactionBuild("truncated recipients blob (u32)".to_string()) + })?; let v = u32::from_be_bytes([buf[*at], buf[*at + 1], buf[*at + 2], buf[*at + 3]]); *at = end; Ok(v) }; let read_u64 = |buf: &[u8], at: &mut usize| -> Result { - let end = *at + 8; - if end > buf.len() { - return Err(PlatformWalletError::TransactionBuild( - "truncated recipients blob (u64)".to_string(), - )); - } + let end = at.checked_add(8).filter(|e| *e <= buf.len()).ok_or_else(|| { + PlatformWalletError::TransactionBuild("truncated recipients blob (u64)".to_string()) + })?; let mut b = [0u8; 8]; b.copy_from_slice(&buf[*at..end]); *at = end; Ok(u64::from_be_bytes(b)) }; + // Bound `count` by what the blob could actually contain BEFORE reserving. + // `count` is a caller-controlled `u32`: passing `u32::MAX` in a four-byte + // blob would otherwise ask `Vec::with_capacity` for ~64 GiB and take the + // process-aborting allocation-failure path instead of returning this + // decode error. let count = read_u32(blob, &mut cursor)? as usize; - let mut outputs = Vec::with_capacity(count); + let max_possible = blob.len().saturating_sub(cursor) / MIN_ENCODED_OUTPUT_LEN; + if count > max_possible { + return Err(err(format!( + "recipients blob declares {count} outputs but holds at most {max_possible}" + ))); + } + let mut outputs = Vec::new(); + outputs + .try_reserve_exact(count) + .map_err(|e| err(format!("cannot allocate {count} recipient outputs: {e}")))?; for _ in 0..count { let addr_len = read_u32(blob, &mut cursor)? as usize; - let end = cursor + addr_len; - if end > blob.len() { - return Err(err("truncated recipients blob (address)".to_string())); - } + let end = cursor + .checked_add(addr_len) + .filter(|e| *e <= blob.len()) + .ok_or_else(|| err("truncated recipients blob (address)".to_string()))?; let addr_str = std::str::from_utf8(&blob[cursor..end]) .map_err(|e| err(format!("recipient address is not valid UTF-8: {e}")))?; cursor = end; @@ -82,8 +103,8 @@ fn decode_payment_outputs( Ok(outputs) } -/// Build and sign a standard L1 payment from the wallet's signable funds -/// accounts (union coin selection) and return the signed bytes + fee + change. +/// Build and sign a standard L1 payment from ONE of the wallet's signable funds +/// accounts and return the signed bytes + fee + change. /// /// * `handle` — a core-wallet handle (`platform_wallet_get_core`). /// * `outputs_blob`/`outputs_blob_len` — the recipients, encoded as documented @@ -91,6 +112,14 @@ fn decode_payment_outputs( /// * `fee_per_kb` — fee rate in duffs/kB, or `0` for the default (1000). /// * `core_signer_handle` — the caller's `MnemonicResolverHandle`; ownership is /// retained by the caller (this function does NOT destroy it). +/// * `funding_path_ptr`/`funding_path_len` — an optional UTF-8 BIP32 +/// derivation-path string (e.g. `"m/44'/5'/0'"`) naming the SINGLE funds +/// account whose UTXOs fund the payment (dashpay/platform#4184). Pass +/// `null` / `0` for the default — the unmixed BIP44 account. Pass an explicit +/// account-level path (e.g. the DIP-9 CoinJoin account path) to spend +/// previously-mixed coins deliberately. There is no union across accounts and +/// no consent gate: exactly one funding source participates, and if it cannot +/// cover the payment the call fails with the typed insufficient-funds code. /// * `out_tx_bytes`/`out_tx_len` — receive the consensus-serialized signed /// transaction. Free with [`core_wallet_free_payment_bytes`]. /// * `out_fee` — receives the fee paid, in duffs. @@ -99,7 +128,9 @@ fn decode_payment_outputs( /// /// # Safety /// All pointers must be valid; `outputs_blob` must be readable for -/// `outputs_blob_len` bytes; the out-pointers must be writable. +/// `outputs_blob_len` bytes; `funding_path_ptr`, when non-null, must point to +/// `funding_path_len` readable bytes for the duration of the call; the +/// out-pointers must be writable. #[no_mangle] #[allow(clippy::too_many_arguments)] pub unsafe extern "C" fn core_wallet_build_signed_payment( @@ -108,6 +139,8 @@ pub unsafe extern "C" fn core_wallet_build_signed_payment( outputs_blob_len: usize, fee_per_kb: u64, core_signer_handle: *mut MnemonicResolverHandle, + funding_path_ptr: *const u8, + funding_path_len: usize, out_tx_bytes: *mut *mut u8, out_tx_len: *mut usize, out_fee: *mut u64, @@ -120,6 +153,11 @@ pub unsafe extern "C" fn core_wallet_build_signed_payment( check_ptr!(out_fee); check_ptr!(out_change); + let funding_path = match parse_optional_derivation_path(funding_path_ptr, funding_path_len) { + Ok(p) => p, + Err(result) => return result, + }; + let blob = std::slice::from_raw_parts(outputs_blob, outputs_blob_len); let signer_addr = core_signer_handle as usize; let fee = if fee_per_kb == 0 { @@ -131,6 +169,7 @@ pub unsafe extern "C" fn core_wallet_build_signed_payment( let option = CORE_WALLET_STORAGE.with_item(handle, |wallet| { let network = wallet.network(); let outputs = decode_payment_outputs(blob, network)?; + let funding_path = funding_path.clone(); let wallet_id = wallet.wallet_id(); // SAFETY: `signer_addr` came from `core_signer_handle`, which the caller // pinned alive for this call; the `MnemonicResolverCoreSigner` lives @@ -140,7 +179,7 @@ pub unsafe extern "C" fn core_wallet_build_signed_payment( wallet_id, network, ); - runtime().block_on(wallet.build_signed_payment(outputs, fee, &signer)) + runtime().block_on(wallet.build_signed_payment(outputs, fee, &signer, funding_path)) }); let result = unwrap_option_or_return!(option); diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 44532de863..edccbe300f 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -324,7 +324,17 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::AddressNonceMismatch { .. } => { PlatformWalletFFIResultCode::ErrorAddressNonceMismatch } - PlatformWalletError::CoreInsufficientFunds { .. } => { + // Both Core-send selector shortfalls share code 22: the atomic + // builder's (`CoreInsufficientFunds`) and the one-shot signed-payment + // primitive's (`PaymentInsufficientFunds`). Without this second arm + // the payment shortfall flattened to `ErrorUnknown`, so the typed + // `available`/`required` amounts `build_signed_payment` computes + // never reached the host as an actionable code — and after the + // dashpay/platform#4184 re-scope those amounts are SINGLE-ACCOUNT + // figures the host must be able to act on (the signal is "pick a + // different funding account", not "retry the same one"). + PlatformWalletError::CoreInsufficientFunds { .. } + | PlatformWalletError::PaymentInsufficientFunds { .. } => { PlatformWalletFFIResultCode::ErrorCoreInsufficientFunds } PlatformWalletError::AssetLockNotTracked(..) => { diff --git a/packages/rs-platform-wallet-ffi/src/utils.rs b/packages/rs-platform-wallet-ffi/src/utils.rs index bf84c88170..9d62627bb4 100644 --- a/packages/rs-platform-wallet-ffi/src/utils.rs +++ b/packages/rs-platform-wallet-ffi/src/utils.rs @@ -2,6 +2,44 @@ use crate::error::*; use crate::{check_ptr, unwrap_result_or_return}; use std::os::raw::{c_char, c_uchar}; +/// Decode an OPTIONAL BIP32 derivation-path string from a raw `(ptr, len)` pair +/// over the C ABI — the shared `funding_path` decoder for every entry point +/// that names a SINGLE funding account (dashpay/platform#4184). +/// +/// A null pointer or zero length is `None` (the default: fund from the unmixed +/// BIP44 account). Otherwise the bytes are parsed as a UTF-8 BIP32 path (e.g. +/// `"m/44'/5'/0'"`); invalid UTF-8 or a malformed path is a hard +/// `ErrorInvalidParameter` — never a silent fallback to the default account, +/// which would fund the transaction from coins the caller did not choose. +/// +/// # Safety +/// `ptr`, when non-null, must point to `len` readable bytes for the duration of +/// the call. +pub(crate) unsafe fn parse_optional_derivation_path( + ptr: *const u8, + len: usize, +) -> Result, PlatformWalletFFIResult> { + use std::str::FromStr; + if ptr.is_null() || len == 0 { + return Ok(None); + } + let bytes = std::slice::from_raw_parts(ptr, len); + let text = std::str::from_utf8(bytes).map_err(|e| { + PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + format!("funding_path is not valid UTF-8: {e}"), + ) + })?; + key_wallet::bip32::DerivationPath::from_str(text) + .map(Some) + .map_err(|e| { + PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + format!("invalid funding_path derivation path {text:?}: {e}"), + ) + }) +} + /// RAII guard that scrubs a `secp256k1::SecretKey`'s scalar on drop. `from_slice` /// allocates a 32-byte scalar copy of the caller's private key, and `SecretKey` /// has no `Drop` wipe of its own — so without this the copy would survive on the diff --git a/packages/rs-platform-wallet/src/test_support.rs b/packages/rs-platform-wallet/src/test_support.rs index 8fd146dc77..381fa30b8a 100644 --- a/packages/rs-platform-wallet/src/test_support.rs +++ b/packages/rs-platform-wallet/src/test_support.rs @@ -257,6 +257,94 @@ pub(crate) async fn funded_wallet_manager_with_outputs( (Arc::new(RwLock::new(wm)), wallet_id, balance, signer) } +/// Builds a testnet wallet manager whose balance is split across TWO privacy +/// domains: `bip44_duffs` on BIP44 account 0 and `coinjoin_duffs` on the DIP-9 +/// CoinJoin account 0. Lets the funding-domain tests prove that coin selection +/// never crosses from one account into the other. +/// +/// Returns the manager, the wallet id, and a soft signer over the wallet's seed +/// (which can derive keys for BOTH accounts, so per-account signing can be +/// exercised end-to-end). +#[cfg(test)] +pub(crate) async fn split_funded_wallet_manager( + bip44_duffs: u64, + coinjoin_duffs: u64, +) -> ( + Arc>>, + WalletId, + WalletSigner, +) { + use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait as _; + + let mut ctx = TestWalletContext::new_random(); + + // Fund BIP44 account 0 (the default funding account) at its pre-derived + // receive address. + let bip44_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[bip44_duffs]); + let bip44_result = ctx + .check_transaction( + &bip44_tx, + TransactionContext::InChainLockedBlock(BlockInfo::new( + 1, + BlockHash::all_zeros(), + 1_700_000_000, + )), + ) + .await; + assert!( + bip44_result.is_relevant && bip44_result.is_new_transaction, + "BIP44 funding tx should be recognized" + ); + + // Derive a fresh CoinJoin receive address (registering it in the CoinJoin + // pool so the checker recognizes the funding), then fund CoinJoin account 0. + let coinjoin_xpub = ctx + .wallet + .get_coinjoin_account(0) + .expect("default wallet has CoinJoin account 0") + .account_xpub; + // CoinJoin is a single-pool (non-standard) account, so it derives via + // `next_address` rather than `next_receive_address`. + let coinjoin_address = ctx + .managed_wallet + .first_coinjoin_managed_account_mut() + .expect("default wallet has a managed CoinJoin account 0") + .next_address(Some(&coinjoin_xpub), true) + .expect("CoinJoin receive address"); + let coinjoin_tx = Transaction::dummy(&coinjoin_address, 0..1, &[coinjoin_duffs]); + let coinjoin_result = ctx + .check_transaction( + &coinjoin_tx, + TransactionContext::InChainLockedBlock(BlockInfo::new( + 2, + BlockHash::all_zeros(), + 1_700_000_100, + )), + ) + .await; + assert!( + coinjoin_result.is_relevant && coinjoin_result.is_new_transaction, + "CoinJoin funding tx should be recognized" + ); + + let signer = WalletSigner { + wallet: ctx.wallet.clone(), + }; + + let balance = Arc::new(WalletBalance::new()); + let info = PlatformWalletInfo { + core_wallet: ctx.managed_wallet, + balance, + identity_manager: IdentityManager::new(), + tracked_asset_locks: BTreeMap::new(), + }; + + let mut wm = WalletManager::::new(Network::Testnet); + let wallet_id = wm.insert_wallet(ctx.wallet, info).expect("insert wallet"); + + (Arc::new(RwLock::new(wm)), wallet_id, signer) +} + /// Funded SPV-backed Core wallet for downstream FFI lifecycle tests. The SPV /// runtime is intentionally not started; abandon/free only need wallet state. pub async fn funded_spv_core_wallet( diff --git a/packages/rs-platform-wallet/src/wallet/core/send.rs b/packages/rs-platform-wallet/src/wallet/core/send.rs index 4d74fa37fc..f4d4e3a554 100644 --- a/packages/rs-platform-wallet/src/wallet/core/send.rs +++ b/packages/rs-platform-wallet/src/wallet/core/send.rs @@ -1,11 +1,19 @@ //! General Core L1 payment building. //! //! [`CoreWallet::build_signed_payment`] is the first-class "send" primitive: -//! it selects inputs across every **signable** funds account, builds and signs +//! it selects inputs from **one** caller-named funds account, builds and signs //! a standard payment transaction, and returns the **signed serialized bytes** //! plus the computed fee and change amount — WITHOUT broadcasting and WITHOUT //! persisting a debit. //! +//! ## Funding-domain isolation +//! +//! Selection is confined to a single funding account, defaulting to the unmixed +//! BIP44 account — never a union across accounts. See +//! [`crate::wallet::funding_privacy`] for the invariant, the +//! dashpay/platform#4073 → #4184 history behind it, and the guardrail that +//! enforces it. +//! //! ## Why build-only / no-broadcast //! //! During the dashj→SDK transition the Android app keeps its own transaction @@ -21,9 +29,14 @@ //! Building does **not** persist a debit and does not write UTXOs, balances, or //! transaction records back to the wallet. The only in-memory mutation is the //! key-wallet `ReservationSet` bookkeeping that `set_funding` + -//! `TransactionBuilder::build_signed` perform on the primary funding account: -//! the selected inputs are marked *reserved* so a concurrent SDK build does not -//! re-select the same coins. That reservation is in-memory only (never +//! `TransactionBuilder::build_signed` perform on the **selected** funding +//! account: the selected inputs are marked *reserved* so a concurrent SDK build +//! does not re-select the same coins. Because selection is confined to one +//! account, every selected input is reserved in the ledger that all funding +//! paths consult for that account — there are no unreserved "secondary-account" +//! inputs (dashpay/platform#4247 review finding, now structurally impossible). +//! +//! That reservation is in-memory only (never //! serialized) and is released when the spend is later processed back into the //! wallet by sync, or by the reservation-TTL backstop, or explicitly via //! [`ManagedCoreFundsAccount::release_reservation`] for an abandoned build. No @@ -33,31 +46,51 @@ //! [`ManagedCoreFundsAccount::release_reservation`]: //! key_wallet::managed_account::ManagedCoreFundsAccount::release_reservation -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use dashcore::{Address as DashAddress, OutPoint, Transaction}; +use key_wallet::bip32::DerivationPath; use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; use key_wallet::managed_account::ManagedCoreFundsAccount; -use key_wallet::ManagedAccountType; use key_wallet::signer::Signer; use key_wallet::wallet::managed_wallet_info::coin_selection::{SelectionError, SelectionStrategy}; use key_wallet::wallet::managed_wallet_info::fee::FeeRate; use key_wallet::wallet::managed_wallet_info::transaction_builder::{BuilderError, TransactionBuilder}; use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; -use key_wallet::Utxo; use crate::broadcaster::TransactionBroadcaster; use crate::error::PlatformWalletError; use crate::wallet::core::CoreWallet; +use crate::wallet::funding_privacy::is_signable_funding_account; /// key-wallet's default fee rate (duffs per kB). Matches the asset-lock /// builder's `DEFAULT_FEE_PER_KB` and `FeeRate::normal()`. const DEFAULT_FEE_PER_KB: u64 = 1000; -/// The BIP44 account that supplies the change output (and whose reservation -/// ledger gates concurrent primary-account builds). The union of every other -/// signable funds account is added as explicit inputs on top of it. -const PRIMARY_BIP44_ACCOUNT_INDEX: u32 = 0; +/// Consensus cap on any single amount this primitive will accept or aggregate. +const MAX_MONEY: u64 = dashcore::blockdata::constants::MAX_MONEY; + +/// Upper bound on the caller-supplied fee rate, in duffs/kB. +/// +/// Derived so that even a maximum-size standard transaction cannot produce a +/// fee above [`MAX_MONEY`]: Dash's standard-transaction limit is 100_000 bytes, +/// i.e. 100 kB, and `FeeRate::calculate_fee` computes +/// `sat_per_kb * size_bytes / 1000` — so `MAX_MONEY / 100` also keeps the +/// intermediate `sat_per_kb * size_bytes` product (≤ 2.1e18) inside `u64`. +const MAX_FEE_PER_KB: u64 = MAX_MONEY / 100; + +/// The unmixed BIP44 account this primitive is pinned to, in both of its roles: +/// +/// * the **default funding account** — where `funding_path: None` selects from; +/// * the **change sink** — key-wallet derives change addresses only for +/// *Standard* accounts, so a payment funded from an explicitly-named +/// non-Standard account (CoinJoin / DashPay-receiving) must route change +/// here. See [`crate::wallet::funding_privacy`]. +/// +/// Account 0 rather than a caller-chosen index: the transition-era send path +/// has exactly one BIP44 account, and the asset-lock builder's `account_index` +/// serves the same pinned role there. +const BIP44_ACCOUNT_INDEX: u32 = 0; /// A built-and-signed Core L1 payment, ready to be committed/broadcast by the /// caller (dashj during the transition, or a later SDK-broadcast mode). @@ -76,45 +109,44 @@ pub struct SignedCorePayment { pub change_amount: u64, } -/// True for funds accounts the bound wallet cannot sign for. Only -/// `DashpayExternalAccount`s are watch-only: they hold a *contact's* receiving -/// addresses (we keep the contact's xpub to build payments *to* them and to -/// watch that side), so their UTXOs must never be selected as spend inputs — -/// signing would fail because no private key of ours derives them. Every other -/// funds account (BIP44/BIP32/CoinJoin/DashPay receiving) is derived from our -/// own seed and is signable. -fn is_watch_only_funds_account(account: &ManagedCoreFundsAccount) -> bool { - matches!( - account.managed_account_type(), - ManagedAccountType::DashpayExternalAccount { .. } - ) -} - impl CoreWallet { /// Build and sign a standard Core L1 payment to `outputs`, funding it from - /// the union of every **signable** funds account, and return the signed - /// transaction plus its fee and change amount. Does **not** broadcast and - /// does **not** persist a debit (see the module docs for the persistence + /// the **single** funds account named by `funding_path`, and return the + /// signed transaction plus its fee and change amount. Does **not** broadcast + /// and does **not** persist a debit (see the module docs for the persistence /// contract). /// - /// ## Coin selection — union of signable accounts + /// ## Coin selection — one account, never a union + /// + /// Inputs come from exactly one funds account: `None` (the default) funds + /// from the unmixed BIP44 account at [`BIP44_ACCOUNT_INDEX`], and + /// `Some(path)` funds strictly from the one funds account whose + /// account-level derivation path equals `path` (e.g. the DIP-9 CoinJoin + /// account, to spend previously-mixed coins deliberately). There is **no + /// union across accounts and no privacy-domain consent gate** — the caller + /// names exactly one funding source, so there is nothing to consent to. If + /// that account cannot cover the payment (+ fee) the build fails with + /// [`PlatformWalletError::PaymentInsufficientFunds`] rather than silently + /// topping up from another account; that failure is the point, not a + /// limitation. See [`crate::wallet::funding_privacy`] for why + /// (dashpay/platform#4073, blocked and re-scoped by #4184). /// - /// Inputs are selected across BIP44 + BIP32 + CoinJoin + DashPay-receiving - /// accounts (the wallet-wide spendable set), reusing the same union-funding - /// machinery the shielded asset-lock path uses - /// ([`AssetLockManager::build_asset_lock_tx_from_all_funding_accounts`]): - /// BIP44 account 0 is the PRIMARY account (it supplies the change output and - /// its reservation ledger gates concurrent primary-account builds), and the - /// spendable UTXOs of every other signable account are added as explicit - /// builder inputs. Watch-only `DashpayExternalAccount`s are excluded — their - /// coins belong to a contact and cannot be signed by this wallet. + /// Watch-only `DashpayExternalAccount`s can never fund a payment — their + /// coins belong to a contact and the local mnemonic holds no key for + /// them — so naming one explicitly is refused rather than silently ignored. + /// + /// Change routes to the BIP44 account at [`BIP44_ACCOUNT_INDEX`], which for + /// the default funding path is the funding account itself. When an explicit + /// non-Standard account (CoinJoin / DashPay-receiving) funds the payment, + /// key-wallet cannot derive change on it at all, so the BIP44 sink is + /// structural — the same change model the asset-lock builder uses. /// /// `LargestFirst` selection is used deliberately (not the builder default /// `BranchAndBound`): a CoinJoin account can hold many small mixed /// denominations, and `BranchAndBound`'s exact-match subset-sum is - /// exponential over them (the same hang the asset-lock union path avoids). - /// `LargestFirst`'s linear greedy accumulator also minimizes the input - /// count — fewer signer round-trips and a smaller tx/fee. + /// exponential over them. `LargestFirst`'s linear greedy accumulator also + /// minimizes the input count — fewer signer round-trips and a smaller + /// tx/fee. /// /// ## Parameters /// @@ -125,14 +157,15 @@ impl CoreWallet { /// * `signer` — the ECDSA signer that produces each input's P2PKH signature /// (the Keychain/Keystore-backed `MnemonicResolverCoreSigner` in /// production). No private key crosses the boundary. - /// - /// [`AssetLockManager::build_asset_lock_tx_from_all_funding_accounts`]: - /// crate::wallet::asset_lock + /// * `funding_path` — the account-level derivation path of the SINGLE funds + /// account whose UTXOs fund the payment. `None` (the default) funds from + /// the unmixed BIP44 account (dashpay/platform#4184). pub async fn build_signed_payment( &self, outputs: Vec<(DashAddress, u64)>, fee_per_kb: Option, signer: &S, + funding_path: Option, ) -> Result { if outputs.is_empty() { return Err(PlatformWalletError::TransactionBuild( @@ -144,7 +177,37 @@ impl CoreWallet { "every output amount must be greater than zero".to_string(), )); } - let outputs_total: u64 = outputs.iter().map(|(_, amount)| *amount).sum(); + + // Checked aggregation, bounded by MAX_MONEY. key-wallet sums the same + // amounts with unchecked `u64` arithmetic while building, so an + // unchecked total here would wrap in release builds (four outputs of + // `1 << 62` sum to exactly 2^64) and let selection fund only the fee + // while retaining four enormous outputs — a signed transaction + // consensus rejects, with meaningless fee/change metadata. In an + // overflow-checking build the same input panics inside the `extern "C"` + // FFI frame, where the JNI guard cannot recover it. + let outputs_total = outputs + .iter() + .try_fold(0u64, |total, (_, amount)| total.checked_add(*amount)) + .filter(|total| *total <= MAX_MONEY) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "output amounts overflow or exceed MAX_MONEY ({MAX_MONEY} duffs)" + )) + })?; + + // Bound the caller-supplied fee rate for the same reason: key-wallet's + // `FeeRate::calculate_fee` computes `sat_per_kb * size_bytes` with + // unchecked `u64` multiplication, so a rate near `u64::MAX` (the public + // Kotlin/FFI APIs accept any non-negative `Long`) panics in an + // overflow-checking Android build, or wraps in release — turning an + // astronomical requested rate into a tiny fee. + let fee_per_kb = fee_per_kb.unwrap_or(DEFAULT_FEE_PER_KB); + if fee_per_kb > MAX_FEE_PER_KB { + return Err(PlatformWalletError::TransactionBuild(format!( + "fee rate {fee_per_kb} duffs/kB exceeds the maximum {MAX_FEE_PER_KB}" + ))); + } let mut wm = self.wallet_manager.write().await; let (wallet, info) = wm @@ -152,81 +215,177 @@ impl CoreWallet { .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; let height = info.core_wallet.last_processed_height(); - let fee_rate = FeeRate::new(fee_per_kb.unwrap_or(DEFAULT_FEE_PER_KB)); + let network = info.core_wallet.network(); + let fee_rate = FeeRate::new(fee_per_kb); - // The PRIMARY account (change destination). Clone the xpub-bearing - // account so no immutable borrow of `wallet` is held across the mutable - // `info` borrow / signer await below. - let primary_account = wallet - .get_bip44_account(PRIMARY_BIP44_ACCOUNT_INDEX) + // ------------------------------------------------------------------ + // REGRESSION NOTE (dashpay/platform#4073 → #4184 → #4247) + // + // This selection block previously unioned every signable funds account + // and ran LargestFirst over the combined set, with BIP44 change — which + // irreversibly links ordinary, CoinJoin, and DashPay-receiving coins in + // one on-chain transaction. Reviewer shumkov blocked exactly that on + // PR #4184 (2026-07-21); the single-selected-account redesign (commit + // 4d3e1322bc) was signed off 2026-07-23. + // + // The send-raw-tx code was written on an older integration line BEFORE + // that re-scope, and shipped the blocked union behavior into the general + // send path — with a test asserting the union as correct behavior. A + // compile-clean, review-passed change is NOT sufficient evidence of + // correctness here. + // + // INVARIANT: single selected account; never union funding accounts; + // default unmixed BIP44. See `crate::wallet::funding_privacy` and its + // guardrail tests. + // ------------------------------------------------------------------ + + // Resolve the account-level path of the unmixed BIP44 account: both the + // default funding source and the change sink. + let bip44_path = info + .core_wallet + .accounts + .standard_bip44_accounts + .get(&BIP44_ACCOUNT_INDEX) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "BIP44 account {BIP44_ACCOUNT_INDEX} not found for payment funding" + )) + })? + .managed_account_type() + .to_account_type() + .derivation_path(network) + .map_err(|e| { + PlatformWalletError::TransactionBuild(format!( + "failed to derive the unmixed BIP44 account-level path: {e}" + )) + })?; + let funding_path = funding_path.unwrap_or_else(|| bip44_path.clone()); + let funds_from_change_account = funding_path == bip44_path; + + // The xpub-bearing BIP44 account: the change sink, and the fallback + // signing-side account. Cloned so no immutable borrow of `wallet` is + // held across the mutable `info` borrow below. + let bip44_acc = wallet + .get_bip44_account(BIP44_ACCOUNT_INDEX) .ok_or_else(|| { PlatformWalletError::TransactionBuild(format!( - "BIP44 account {PRIMARY_BIP44_ACCOUNT_INDEX} not found for payment funding" + "BIP44 account {BIP44_ACCOUNT_INDEX} not found for payment change routing" )) })? .clone(); - // Snapshot the primary account's spendable outpoints so the union sweep - // does not double-add them: `set_funding` already seeds them, and - // `add_inputs` must contribute only the OTHER signable accounts. - let primary_outpoints: HashSet = info - .core_wallet - .accounts - .standard_bip44_accounts - .get(&PRIMARY_BIP44_ACCOUNT_INDEX) - .map(|a| { - a.spendable_utxos(height) - .into_iter() - .map(|u| u.outpoint) - .collect() + // Derive an explicit BIP44 change address ONLY when the funding account + // is not the BIP44 sink itself: `set_funding` already derives change on + // the funding account, which is correct (and consumes no extra pool + // index) in the default case, but fails and is swallowed to `None` for a + // non-Standard CoinJoin / DashPay account. Taken before the funding + // account's `&mut` below — two accounts of the same collection cannot + // both be borrowed mutably at once. + let change_addr: Option = if funds_from_change_account { + None + } else { + let change_acc = info + .core_wallet + .accounts + .standard_bip44_accounts + .get_mut(&BIP44_ACCOUNT_INDEX) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "managed BIP44 account {BIP44_ACCOUNT_INDEX} not found for payment \ + change routing" + )) + })?; + Some( + change_acc + .next_change_address(Some(&bip44_acc.account_xpub), true) + .map_err(|e| { + PlatformWalletError::TransactionBuild(format!( + "failed to derive change address on BIP44 account \ + {BIP44_ACCOUNT_INDEX}: {e}" + )) + })?, + ) + }; + + // The funding account's OWN wallet-level `Account`. `set_funding` calls + // `funds_acc.next_change_address(Some(&acc.account_xpub))` before the + // `set_change_address` override, so `acc` must be the funding account — + // passing the BIP44 xpub for an explicitly-selected BIP32 account would + // record a change entry derived from the wrong xpub into that account's + // pool (dashpay/platform#4184 review). Falls back to `bip44_acc` when no + // wallet-level account matches, preserving the default behavior. + let funding_wallet_acc = wallet + .all_accounts() + .into_iter() + .find(|a| { + a.derivation_path() + .map(|p| p == funding_path) + .unwrap_or(false) }) - .unwrap_or_default(); + .unwrap_or(&bip44_acc); - // Single immutable pass over every signable funds account, building: - // (a) an owned `Address -> DerivationPath` resolver spanning all - // signable inputs, so signing resolves a key for an input drawn - // from any account; - // (b) the explicit extra inputs (all signable non-primary accounts); - // (c) an `OutPoint -> value` map for the post-build change figure; - // (d) the total selectable value, for a typed shortfall error. - let mut path_map: HashMap = HashMap::new(); - let mut input_value: HashMap = HashMap::new(); - let mut extra_inputs: Vec = Vec::new(); - let mut selectable_value: u64 = 0; - for account in info.core_wallet.accounts.all_funding_accounts() { - if is_watch_only_funds_account(account) { + // Locate the ONE managed funds account whose account-level path equals + // `funding_path`, MUTABLY, so `set_funding` reserves the selected inputs + // in that account's OWN reservation ledger. Watch-only + // `DashpayExternalAccount`s are never fundable (the local mnemonic + // cannot sign them) — refuse even when named explicitly. + // + // PRIVACY-DOMAIN-OK: this iterates funds accounts only to LOOK ONE UP by + // derivation path. Exactly one account is selected and it alone funds + // the transaction; nothing is accumulated across accounts. + let mut selected: Option<&mut ManagedCoreFundsAccount> = None; + for acc in info.core_wallet.accounts.all_funding_accounts_mut() { + let acc_path = acc + .managed_account_type() + .to_account_type() + .derivation_path(network) + .map_err(|e| { + PlatformWalletError::TransactionBuild(format!( + "failed to derive account-level path for a funds account: {e}" + )) + })?; + if acc_path != funding_path { continue; } - for utxo in account.spendable_utxos(height) { - selectable_value = selectable_value.saturating_add(utxo.value()); - input_value.insert(utxo.outpoint, utxo.value()); - if let Some(path) = account.address_derivation_path(&utxo.address) { - path_map.insert(utxo.address.clone(), path); - } - if !primary_outpoints.contains(&utxo.outpoint) { - extra_inputs.push(utxo.clone()); - } + if !is_signable_funding_account(acc.managed_account_type()) { + return Err(PlatformWalletError::TransactionBuild(format!( + "funding derivation path {funding_path} names a watch-only account whose \ + coins the local wallet cannot sign; choose a signable funds account" + ))); + } + selected = Some(acc); + break; + } + let selected = selected.ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "no spendable funds account matches funding derivation path {funding_path}" + )) + })?; + + // One immutable pass over the SELECTED account, building: + // (a) an owned `Address -> DerivationPath` resolver, so signing can + // resolve a key for every selected input without holding an + // account borrow across the signer await; + // (b) an `OutPoint -> value` map for the post-build fee/change figures; + // (c) the account's selectable total, for a typed shortfall error. + let mut path_map: HashMap = HashMap::new(); + let mut input_value: HashMap = HashMap::new(); + let mut selectable_value: u64 = 0; + for utxo in selected.spendable_utxos(height) { + selectable_value = selectable_value.saturating_add(utxo.value()); + input_value.insert(utxo.outpoint, utxo.value()); + if let Some(path) = selected.address_derivation_path(&utxo.address) { + path_map.insert(utxo.address.clone(), path); } } - // Seed the primary account (inputs + change address + reservations), - // append the union of the other signable accounts' inputs, then add the - // real recipient outputs. The `&mut` borrow of the primary account is - // scoped to this block; the returned builder owns cloned inputs / - // reservations / change address, so no account borrow is held across - // the signer await below. + // Seed the selected account (inputs + reservations + its own change + // address), override the change sink when the funding account cannot + // derive change, then add the recipient outputs. The `&mut` borrow ends + // with `set_funding`; the returned builder owns cloned inputs / + // reservations / change address, so no account borrow is held across the + // signer await below. let builder = { - let primary_funds = info - .core_wallet - .accounts - .standard_bip44_accounts - .get_mut(&PRIMARY_BIP44_ACCOUNT_INDEX) - .ok_or_else(|| { - PlatformWalletError::TransactionBuild(format!( - "managed BIP44 account {PRIMARY_BIP44_ACCOUNT_INDEX} not found for \ - payment funding" - )) - })?; let mut builder = TransactionBuilder::new() .set_fee_rate(fee_rate) .set_current_height(height) @@ -234,8 +393,10 @@ impl CoreWallet { // BranchAndBound, to keep CoinJoin's many small denominations // from blowing up the exact-match subset-sum search. .set_selection_strategy(SelectionStrategy::LargestFirst) - .set_funding(primary_funds, &primary_account) - .add_inputs(extra_inputs); + .set_funding(selected, funding_wallet_acc); + if let Some(addr) = change_addr { + builder = builder.set_change_address(addr); + } for (address, amount) in &outputs { builder = builder.add_output(address, *amount); } @@ -257,7 +418,7 @@ impl CoreWallet { // // `total_out` is the sum of every output; the only non-recipient output // a plain payment (no special payload) can carry is the single change - // output back to the primary account, so `change = total_out − outputs`. + // output back to the BIP44 sink, so `change = total_out − outputs`. // Any selected input we somehow can't price (impossible — every // spendable UTXO was recorded above) counts as 0, so `fee` is over- // rather than under-reported. @@ -280,33 +441,36 @@ impl CoreWallet { /// Map a key-wallet [`BuilderError`] to a [`PlatformWalletError`], promoting the /// two shortfall shapes to the typed [`PlatformWalletError::PaymentInsufficientFunds`] -/// so the exact `available`/`required` duff amounts survive. The builder's own -/// `InsufficientFunds` figures cover only what the primary-account selector saw, -/// so we substitute the union-wide selectable total (`available`) and the -/// outputs-plus-fee-ish target — `required` is at least the outputs total; a -/// coin-selection error already carries the fee-inclusive figure, which we -/// prefer when present. +/// so the exact `available`/`required` duff amounts survive. +/// +/// `available` is the **selected account's** spendable total, deliberately — +/// never a wallet-wide figure. Reporting a wallet-wide "available" against a +/// single-account shortfall would invite the caller to retry with a larger +/// amount that can only succeed by crossing privacy domains, which this +/// primitive will not do (see [`crate::wallet::funding_privacy`]). `required` is +/// at least the outputs total; a coin-selection error already carries the +/// fee-inclusive figure, which we prefer when present. fn map_send_builder_error( error: BuilderError, - union_available: u64, + available_in_account: u64, outputs_total: u64, ) -> PlatformWalletError { match error { BuilderError::InsufficientFunds { required, .. } => { PlatformWalletError::PaymentInsufficientFunds { - available: union_available, + available: available_in_account, required: required.max(outputs_total), } } BuilderError::CoinSelection(SelectionError::InsufficientFunds { required, .. }) => { PlatformWalletError::PaymentInsufficientFunds { - available: union_available, + available: available_in_account, required: required.max(outputs_total), } } BuilderError::CoinSelection(SelectionError::NoUtxosAvailable) => { PlatformWalletError::PaymentInsufficientFunds { - available: union_available, + available: available_in_account, required: outputs_total, } } @@ -323,6 +487,7 @@ mod tests { use dashcore::{Address as DashAddress, Network, OutPoint, TxOut, Txid}; use key_wallet::account::account_type::StandardAccountType; use key_wallet::account::AccountType; + use key_wallet::bip32::DerivationPath; use key_wallet::managed_account::ManagedCoreFundsAccount; use key_wallet::Utxo; @@ -369,6 +534,40 @@ mod tests { } } + /// Snapshot the BIP44 and CoinJoin outpoints of a split fixture, plus the + /// CoinJoin account's account-level derivation path (the `funding_path` a + /// caller passes to spend previously-mixed coins deliberately). + async fn split_account_outpoints_and_coinjoin_path( + wm: &Arc>>, + wallet_id: &WalletId, + ) -> (HashSet, HashSet, DerivationPath) { + use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; + + let guard = wm.read().await; + let (_, info) = guard.get_wallet_and_info(wallet_id).expect("wallet present"); + let network = info.core_wallet.network(); + let bip44 = info + .core_wallet + .accounts + .standard_bip44_accounts + .get(&0) + .map(|a| a.utxos.keys().copied().collect()) + .unwrap_or_default(); + let coinjoin_acc = info + .core_wallet + .accounts + .coinjoin_accounts + .get(&0) + .expect("coinjoin account 0 present"); + let coinjoin = coinjoin_acc.utxos.keys().copied().collect(); + let path = coinjoin_acc + .managed_account_type() + .to_account_type() + .derivation_path(network) + .expect("coinjoin account-level path"); + (bip44, coinjoin, path) + } + /// A single-account BIP44 payment: the recipient output is present with the /// exact value, a fee is charged, and the change amount is exactly /// selected_input − output − fee (here the whole 0.1 DASH rides on one @@ -382,7 +581,7 @@ mod tests { let to = recipient(42); let amount = 1_000_000u64; let payment = core - .build_signed_payment(vec![(to.clone(), amount)], None, &signer) + .build_signed_payment(vec![(to.clone(), amount)], None, &signer, None) .await .expect("build should succeed with 0.1 DASH funded"); @@ -417,40 +616,102 @@ mod tests { assert_all_inputs_signed(&payment); } - /// Coin selection spans the UNION of signable funds accounts: a payment - /// that exceeds either the BIP44 slice or the CoinJoin slice alone pulls - /// inputs from BOTH, and every mixed-account input is signed. + /// **Replaces `payment_funds_from_bip44_and_coinjoin_union`**, which asserted + /// the blocked union behavior as correct (dashpay/platform#4247; see the + /// regression note in `build_signed_payment`). + /// + /// The DEFAULT funding path must never select CoinJoin (or any other + /// non-BIP44 domain) coins, even when BIP44 alone cannot cover the payment. + /// Failing is the correct outcome: a shortfall is reported as a typed error + /// rather than silently satisfied by crossing a privacy domain, because the + /// cross-domain link would be irreversible while the failure is merely + /// retryable with an explicit `funding_path`. #[tokio::test] - async fn payment_funds_from_bip44_and_coinjoin_union() { - // 0.09 DASH on BIP44, 0.09 on CoinJoin; ask 0.15 → needs both. + async fn default_funding_never_selects_other_domains() { + // 0.09 DASH on BIP44, 0.09 on CoinJoin; ask 0.15 → only a union covers it. let (wm, wallet_id, signer) = split_funded_wallet_manager(9_000_000, 9_000_000).await; + let core = core_wallet(wm, wallet_id, Arc::new(WalletBalance::new())); - // Snapshot each account's outpoints before building. - let (bip44_ops, coinjoin_ops): (HashSet, HashSet) = { - let guard = wm.read().await; - let (_, info) = guard.get_wallet_and_info(&wallet_id).expect("wallet present"); - let bip44 = info - .core_wallet - .accounts - .standard_bip44_accounts - .get(&0) - .map(|a| a.utxos.keys().copied().collect()) - .unwrap_or_default(); - let coinjoin = info - .core_wallet - .accounts - .coinjoin_accounts - .get(&0) - .map(|a| a.utxos.keys().copied().collect()) - .unwrap_or_default(); - (bip44, coinjoin) - }; + let result = core + .build_signed_payment(vec![(recipient(7), 15_000_000)], None, &signer, None) + .await; + + match result { + Err(PlatformWalletError::PaymentInsufficientFunds { + available, + required, + }) => { + assert_eq!( + available, 9_000_000, + "available must reflect ONLY the BIP44 account, never the \ + wallet-wide union" + ); + assert!( + required >= 15_000_000, + "required {required} should be at least the requested amount" + ); + } + other => panic!( + "the default path must not union BIP44 with CoinJoin — expected \ + PaymentInsufficientFunds, got {other:?}" + ), + } + } + + /// The default path funds happily from BIP44 when BIP44 alone suffices, and + /// still leaves the CoinJoin coins untouched. + #[tokio::test] + async fn default_funding_selects_strictly_within_bip44() { + // 0.2 DASH on BIP44, 0.09 on CoinJoin; ask 0.15 → BIP44 alone covers it. + let (wm, wallet_id, signer) = split_funded_wallet_manager(20_000_000, 9_000_000).await; + let (bip44_ops, coinjoin_ops, _) = + split_account_outpoints_and_coinjoin_path(&wm, &wallet_id).await; + + let core = core_wallet(wm, wallet_id, Arc::new(WalletBalance::new())); + let payment = core + .build_signed_payment(vec![(recipient(7), 15_000_000)], None, &signer, None) + .await + .expect("0.15 DASH is fundable from the 0.2 DASH BIP44 account"); + + let spent: HashSet = payment + .transaction + .input + .iter() + .map(|i| i.previous_output) + .collect(); + assert!( + spent.iter().all(|op| bip44_ops.contains(op)), + "every input must come from BIP44, spent {spent:?}" + ); + assert!( + !spent.iter().any(|op| coinjoin_ops.contains(op)), + "the default path must never reach CoinJoin coins, spent {spent:?}" + ); + assert_all_inputs_signed(&payment); + } + + /// An explicitly-passed CoinJoin path selects strictly from that account and + /// nothing else — the caller-consented, single-domain half of the #4184 + /// contract. Change still lands on BIP44 because key-wallet cannot derive a + /// change address on a non-Standard account; that is structural, not a + /// co-spend. + #[tokio::test] + async fn explicit_coinjoin_path_selects_only_coinjoin() { + // 0.09 DASH on BIP44 (short), 0.2 on CoinJoin; take 0.15 from CoinJoin. + let (wm, wallet_id, signer) = split_funded_wallet_manager(9_000_000, 20_000_000).await; + let (bip44_ops, coinjoin_ops, coinjoin_path) = + split_account_outpoints_and_coinjoin_path(&wm, &wallet_id).await; let core = core_wallet(wm, wallet_id, Arc::new(WalletBalance::new())); let payment = core - .build_signed_payment(vec![(recipient(7), 15_000_000)], None, &signer) + .build_signed_payment( + vec![(recipient(7), 15_000_000)], + None, + &signer, + Some(coinjoin_path), + ) .await - .expect("0.15 DASH must be fundable from the 0.18 DASH union"); + .expect("the named CoinJoin account covers 0.15 DASH"); let spent: HashSet = payment .transaction @@ -458,27 +719,41 @@ mod tests { .iter() .map(|i| i.previous_output) .collect(); + assert!(!spent.is_empty(), "the payment must have selected inputs"); assert!( - spent.iter().any(|op| bip44_ops.contains(op)), - "at least one BIP44 input should be selected" + spent.iter().all(|op| coinjoin_ops.contains(op)), + "every input must come from the named CoinJoin account, spent {spent:?}" ); assert!( - spent.iter().any(|op| coinjoin_ops.contains(op)), - "at least one CoinJoin input should be selected" + !spent.iter().any(|op| bip44_ops.contains(op)), + "an explicit CoinJoin path must not pull BIP44 inputs, spent {spent:?}" + ); + // Change is returned to the transparent BIP44 sink. + assert!( + payment.change_amount > 0, + "spending a 0.2 DASH UTXO for 0.15 DASH must leave change" ); assert_all_inputs_signed(&payment); } - /// A shortfall across the whole signable union surfaces as the typed + /// A shortfall inside the SELECTED account surfaces as the typed /// [`PlatformWalletError::PaymentInsufficientFunds`], with `available` - /// reflecting the union total (not just the primary BIP44 slice). + /// reflecting only that account — never a wallet-wide union total, which + /// would invite a retry that can only succeed by crossing domains. #[tokio::test] - async fn union_shortfall_is_typed() { + async fn selected_account_shortfall_is_typed() { let (wm, wallet_id, signer) = split_funded_wallet_manager(9_000_000, 9_000_000).await; + let (_, _, coinjoin_path) = + split_account_outpoints_and_coinjoin_path(&wm, &wallet_id).await; let core = core_wallet(wm, wallet_id, Arc::new(WalletBalance::new())); let result = core - .build_signed_payment(vec![(recipient(7), 100_000_000)], None, &signer) + .build_signed_payment( + vec![(recipient(7), 100_000_000)], + None, + &signer, + Some(coinjoin_path), + ) .await; match result { @@ -486,9 +761,9 @@ mod tests { available, required, }) => { - assert!( - (9_000_000..=18_000_000).contains(&available), - "available {available} should reflect the union (9M<..<=18M)" + assert_eq!( + available, 9_000_000, + "available must reflect only the named CoinJoin account" ); assert!( required >= 100_000_000, @@ -499,6 +774,32 @@ mod tests { } } + /// A `funding_path` that names no funds account is a hard error — never a + /// silent fallback to the default account, which would fund the payment + /// from coins the caller did not choose. + #[tokio::test] + async fn unknown_funding_path_is_rejected() { + use std::str::FromStr; + + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(wm, wallet_id, balance); + + let nowhere = DerivationPath::from_str("m/44'/5'/77'").expect("valid path"); + let result = core + .build_signed_payment( + vec![(recipient(7), 1_000_000)], + None, + &signer, + Some(nowhere), + ) + .await; + assert!( + matches!(result, Err(PlatformWalletError::TransactionBuild(_))), + "an unmatched funding path must fail, got {result:?}" + ); + } + /// A watch-only `DashpayExternalAccount` (a contact's addresses, which this /// wallet cannot sign) is EXCLUDED from coin selection: its UTXO is never /// spent, and its value is not counted toward the selectable total. @@ -568,10 +869,11 @@ mod tests { let core = core_wallet(wm, wallet_id, Arc::new(WalletBalance::new())); // Ask for 0.5 DASH: covered only if the 1.0-DASH watch-only UTXO were - // spendable. Since it is excluded, the build must fail — and the - // reported `available` must be just the 0.1-DASH BIP44 slice. + // spendable. It is on a different domain from the default BIP44 funding + // path, so the default send can never reach it — the build must fail + // with the 0.1-DASH BIP44 slice as `available`. let result = core - .build_signed_payment(vec![(recipient(7), 50_000_000)], None, &signer) + .build_signed_payment(vec![(recipient(7), 50_000_000)], None, &signer, None) .await; match result { Err(PlatformWalletError::PaymentInsufficientFunds { available, .. }) => { @@ -586,7 +888,7 @@ mod tests { // And a payment that the 0.1-DASH BIP44 slice CAN cover must never spend // the watch-only outpoint. let payment = core - .build_signed_payment(vec![(recipient(7), 1_000_000)], None, &signer) + .build_signed_payment(vec![(recipient(7), 1_000_000)], None, &signer, None) .await .expect("0.01 DASH is fundable from the BIP44 slice alone"); assert!( @@ -608,11 +910,11 @@ mod tests { funded_wallet_manager(StandardAccountType::BIP44Account).await; let core = core_wallet(wm, wallet_id, balance); - let empty = core.build_signed_payment(vec![], None, &signer).await; + let empty = core.build_signed_payment(vec![], None, &signer, None).await; assert!(matches!(empty, Err(PlatformWalletError::TransactionBuild(_)))); let zero = core - .build_signed_payment(vec![(recipient(7), 0)], None, &signer) + .build_signed_payment(vec![(recipient(7), 0)], None, &signer, None) .await; assert!(matches!(zero, Err(PlatformWalletError::TransactionBuild(_)))); } diff --git a/packages/rs-platform-wallet/src/wallet/funding_privacy.rs b/packages/rs-platform-wallet/src/wallet/funding_privacy.rs new file mode 100644 index 0000000000..a665410fb5 --- /dev/null +++ b/packages/rs-platform-wallet/src/wallet/funding_privacy.rs @@ -0,0 +1,359 @@ +//! **Funding-domain isolation** — the privacy invariant every L1 spend path in +//! this crate must satisfy, plus the automated guardrail that enforces it. +//! +//! # The invariant +//! +//! > A single L1 transaction must draw its funding inputs from **exactly one** +//! > funds account (one derivation domain). No spend path may union across +//! > funding accounts, and never implicitly. +//! +//! A wallet's funds accounts are separate *privacy domains*: ordinary BIP44 / +//! BIP32 coins, DIP-9 **CoinJoin** (previously-mixed) coins, and +//! **DashPay-receiving** coins each carry a different linkability story. Any +//! transaction that spends inputs from two of them publishes, irreversibly and +//! on chain, that the same entity controls both — and shielding those coins +//! afterwards cannot undo the link, because the link is already in the L1 +//! transaction graph. +//! +//! # History — why this module exists +//! +//! * **dashpay/platform#4073** — shielding failed on wallets whose balance sat +//! on the DIP-9 CoinJoin path, because asset-lock coin selection only ever +//! reached BIP44 account 0. +//! * The **first** fix unioned every funding account and ran `LargestFirst` +//! over the combined candidate set. Reviewer `shumkov` **blocked** it on +//! dashpay/platform#4184 (2026-07-21): largest-first over a union can combine +//! ordinary, CoinJoin, and DashPay-receiving coins into one transaction with +//! BIP44 change, irreversibly linking the domains. +//! * **dashpay/platform#4184** (commit `4d3e1322bc`, signed off 2026-07-23) is +//! the approved resolution: a single optional derivation-path parameter that +//! names the ONE funding account, **defaulting to the non-mixed BIP44 +//! account**. No union, and no consent gate — because the caller names +//! exactly one funding source, there is nothing to consent to. +//! +//! The regression this module guards against is real and already happened once: +//! `CoreWallet::build_signed_payment` (dashpay/platform#4247) was written +//! against the pre-re-scope design and shipped the blocked union behavior into +//! the general send path. +//! +//! # How to comply +//! +//! A spend path takes `funding_path: Option`, resolves `None` +//! to the unmixed BIP44 account's account-level path, locates the **one** +//! managed funds account whose account-level path equals it, and points the +//! key-wallet `TransactionBuilder`'s `set_funding` at that account alone. If +//! that account cannot cover the spend, the build **fails** with a typed +//! shortfall — it must never top up from a second account. See +//! [`CoreWallet::build_signed_payment`](crate::wallet::core::CoreWallet::build_signed_payment) +//! for the reference implementation on this branch. +//! +//! Change is the one structural exception, and it is not a co-spend: key-wallet +//! derives change addresses only for *Standard* (BIP44/BIP32) accounts, so a +//! transaction funded from a non-Standard account (CoinJoin / DashPay +//! receiving) must route its change to the BIP44 account. That is inherent to +//! spending those coins at all, happens only when the caller explicitly named +//! the non-default account, and is the behavior #4184 approved. +//! +//! # The guardrail +//! +//! `all_funding_accounts()` / `all_funding_accounts_mut()` live in the pinned +//! `key-wallet` fork, so the invariant cannot be documented at their +//! definition. Instead, [`guardrail::every_union_iteration_is_privacy_reviewed`] +//! scans this crate's own sources and fails if any use of those iterators is +//! not preceded by an explicit `PRIVACY-DOMAIN-OK:` review marker, and +//! [`guardrail::no_spend_entry_point_unions_by_default`] asserts behaviorally +//! that the send path does not cross domains. See the `guardrail` module docs +//! for why the pair is sufficient. + +use key_wallet::ManagedAccountType; + +/// Whether a funds account can be *signed for* by the local mnemonic, and may +/// therefore fund a spend at all. +/// +/// Only `DashpayExternalAccount`s are watch-only: they hold a **contact's** +/// receiving addresses (we keep the contact's xpub to build payments *to* them +/// and to watch that side), so no private key of ours derives their UTXOs and +/// signing them would fail. Every other funds account +/// (BIP44 / BIP32 / CoinJoin / DashPay-receiving) comes from our own seed. +/// +/// This is a **signability** filter, not a privacy filter — it is orthogonal to +/// the module-level funding-domain invariant, and passing it does NOT make an +/// account eligible to be unioned with another. A watch-only account must be +/// refused even when a caller names its derivation path explicitly. +pub(crate) fn is_signable_funding_account(managed_type: &ManagedAccountType) -> bool { + !matches!( + managed_type, + ManagedAccountType::DashpayExternalAccount { .. } + ) +} + +#[cfg(test)] +mod guardrail { + //! Automated enforcement of the funding-domain invariant. + //! + //! Two complementary tests, because either alone has a blind spot: + //! + //! * [`no_spend_entry_point_unions_by_default`] is **behavioral**. It + //! proves, over a wallet whose balance is split across two domains, that + //! the general send entry point cannot fund a transaction neither domain + //! covers alone. This catches a semantic regression in an *existing* + //! entry point even if it is written without ever naming + //! `all_funding_accounts` (e.g. by iterating the account maps directly). + //! Its blind spot: it only knows about the entry points listed in it, so + //! a *newly added* spend path is invisible to it. + //! + //! On this branch (the isolated `send-raw-tx` feature line) the only + //! funding-domain-sensitive spend entry point is + //! [`CoreWallet::build_signed_payment`]: the asset-lock builder here is + //! still the pre-#4184 per-`account_index` model and does not accept a + //! `funding_path`, so it is out of scope for this behavioral test. + //! + //! * [`every_union_iteration_is_privacy_reviewed`] is **static**, and + //! covers exactly that blind spot. `all_funding_accounts()` / + //! `all_funding_accounts_mut()` are the only wallet-wide funds-account + //! iterators key-wallet exposes, so a new unparameterized union spend + //! path essentially has to call one of them. This test fails unless each + //! such call is preceded by an explicit `PRIVACY-DOMAIN-OK:` marker + //! comment, which forces the author to state why the call does not + //! union — and makes the marker show up in the review diff, which is how + //! the #4247 regression should have been caught. + //! + //! Scope is this crate's `src/` on purpose: coin selection happens only + //! here. The FFI / JNI layers above merely forward a `funding_path` and + //! cannot select coins themselves. + + use std::collections::HashSet; + use std::path::{Path, PathBuf}; + use std::sync::Arc; + + use dashcore::{Address as DashAddress, Network, OutPoint}; + + use crate::test_support::{split_funded_wallet_manager, AlwaysRejectedBroadcaster}; + use crate::wallet::core::balance::WalletBalance; + use crate::wallet::core::CoreWallet; + use crate::PlatformWalletError; + + // -- static guard -------------------------------------------------------- + + /// The wallet-wide funds-account iterators. Any use of these in a spend + /// path is a potential cross-domain union. + const UNION_ITERATORS: [&str; 2] = ["all_funding_accounts(", "all_funding_accounts_mut("]; + + /// The marker a call site must carry to certify it was reviewed against the + /// funding-domain invariant. + const REVIEW_MARKER: &str = "PRIVACY-DOMAIN-OK"; + + /// How many lines above a call site the marker may sit. Wide enough for the + /// explanatory comment block a legitimate use needs, narrow enough that one + /// marker cannot silently cover an unrelated call added later. + const MARKER_LOOKBACK_LINES: usize = 15; + + /// Collect every `.rs` file under `dir`, recursively. + fn rust_sources(dir: &Path, out: &mut Vec) { + let entries = std::fs::read_dir(dir) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", dir.display())); + for entry in entries { + let path = entry.expect("readable dir entry").path(); + if path.is_dir() { + rust_sources(&path, out); + } else if path.extension().is_some_and(|e| e == "rs") { + out.push(path); + } + } + } + + /// Every use of key-wallet's wallet-wide funds-account iterators must be + /// explicitly privacy-reviewed. + /// + /// Fails with the offending `file:line` and the invariant restated, so an + /// author who reintroduces an unparameterized `all_funding_accounts()` + /// spend path (the dashpay/platform#4247 regression) is told exactly what + /// rule they tripped and where the contract lives. + /// + /// Comment lines are ignored (prose may name the iterators freely), as is + /// this file — it names them in the constants above. + #[test] + fn every_union_iteration_is_privacy_reviewed() { + let src = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut files = Vec::new(); + rust_sources(&src, &mut files); + assert!( + !files.is_empty(), + "found no Rust sources under {} — the guardrail would silently pass", + src.display() + ); + + let this_file = Path::new(file!()) + .file_name() + .expect("this file has a name") + .to_owned(); + + let mut unreviewed = Vec::new(); + for file in &files { + if file.file_name() == Some(this_file.as_os_str()) { + continue; + } + let text = std::fs::read_to_string(file) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", file.display())); + let lines: Vec<&str> = text.lines().collect(); + for (i, line) in lines.iter().enumerate() { + // Prose is free to discuss the iterators. + if line.trim_start().starts_with("//") { + continue; + } + if !UNION_ITERATORS.iter().any(|needle| line.contains(needle)) { + continue; + } + let from = i.saturating_sub(MARKER_LOOKBACK_LINES); + let reviewed = lines[from..=i].iter().any(|l| l.contains(REVIEW_MARKER)); + if !reviewed { + unreviewed.push(format!( + " {}:{}: {}", + file.strip_prefix(&src).unwrap_or(file).display(), + i + 1, + line.trim() + )); + } + } + } + + assert!( + unreviewed.is_empty(), + "FUNDING-DOMAIN INVARIANT: unreviewed use of key-wallet's wallet-wide \ + funds-account iterator(s):\n{}\n\n\ + A single L1 transaction must draw its inputs from EXACTLY ONE funds \ + account. Unioning ordinary BIP44/BIP32, CoinJoin, and DashPay-receiving \ + coins into one transaction irreversibly links those privacy domains on \ + chain (dashpay/platform#4073, blocked and re-scoped by #4184; regressed \ + once already in #4247).\n\n\ + If your call site does NOT select coins across accounts (e.g. it is \ + looking one account up by derivation path, or reading balances), add a \ + comment containing `{REVIEW_MARKER}:` within {MARKER_LOOKBACK_LINES} \ + lines above it saying why. If it DOES select across accounts, it is the \ + bug — take a `funding_path: Option` instead and fund \ + from the one named account, defaulting to unmixed BIP44. See \ + `wallet::funding_privacy`.", + unreviewed.join("\n"), + ); + } + + // -- behavioral guard ---------------------------------------------------- + + /// Duffs on BIP44 account 0 in the split fixture. + const BIP44_DUFFS: u64 = 9_000_000; + /// Duffs on DIP-9 CoinJoin account 0 in the split fixture. + const COINJOIN_DUFFS: u64 = 9_000_000; + /// More than either domain holds, less than their sum — fundable ONLY by a + /// cross-domain union. The send entry point must refuse it. + const CROSS_DOMAIN_ONLY: u64 = 15_000_000; + + /// The general L1 send entry point may not fund a transaction that requires + /// coins from more than one funding account. + /// + /// The fixture splits the balance evenly across two privacy domains (BIP44 + /// and DIP-9 CoinJoin) and asks the default (unmixed BIP44) send path for an + /// amount that neither domain covers alone but their union does. A + /// union-funding path succeeds here; a compliant single-domain path fails. + /// Failing is the CORRECT outcome: selection is confined to the named + /// account, and a shortfall surfaces as a typed insufficient-funds error + /// rather than silently reaching into a second domain. + #[tokio::test] + async fn no_spend_entry_point_unions_by_default() { + let (wm, wallet_id, signer) = + split_funded_wallet_manager(BIP44_DUFFS, COINJOIN_DUFFS).await; + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let core = CoreWallet::new( + sdk, + wm, + wallet_id, + Arc::new(AlwaysRejectedBroadcaster), + Arc::new(WalletBalance::new()), + ); + let payment = core + .build_signed_payment( + vec![(DashAddress::dummy(Network::Testnet, 7), CROSS_DOMAIN_ONLY)], + None, + &signer, + None, + ) + .await; + assert!( + matches!( + payment, + Err(PlatformWalletError::PaymentInsufficientFunds { .. }) + ), + "build_signed_payment must not union BIP44 with CoinJoin, got {payment:?}" + ); + } + + /// The flip side of the invariant: naming a domain explicitly confines + /// selection to it and to nothing else. Asks for an amount BIP44 alone + /// could not cover, from a CoinJoin account that can — and asserts no BIP44 + /// input rides along. + #[tokio::test] + async fn an_explicit_domain_selects_strictly_within_itself() { + use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; + + // 0.09 DASH BIP44, 0.2 DASH CoinJoin; take 0.15 DASH from CoinJoin. + let (wm, wallet_id, signer) = split_funded_wallet_manager(9_000_000, 20_000_000).await; + + let (bip44_ops, coinjoin_ops, coinjoin_path) = { + let guard = wm.read().await; + let (_, info) = guard.get_wallet_and_info(&wallet_id).expect("wallet present"); + let network = info.core_wallet.network(); + let bip44: HashSet = info + .core_wallet + .accounts + .standard_bip44_accounts + .get(&0) + .map(|a| a.utxos.keys().copied().collect()) + .unwrap_or_default(); + let coinjoin_acc = info + .core_wallet + .accounts + .coinjoin_accounts + .get(&0) + .expect("coinjoin account 0 present"); + let coinjoin: HashSet = coinjoin_acc.utxos.keys().copied().collect(); + let path = coinjoin_acc + .managed_account_type() + .to_account_type() + .derivation_path(network) + .expect("coinjoin account-level path"); + (bip44, coinjoin, path) + }; + + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let core = CoreWallet::new( + sdk, + wm, + wallet_id, + Arc::new(AlwaysRejectedBroadcaster), + Arc::new(WalletBalance::new()), + ); + let payment = core + .build_signed_payment( + vec![(DashAddress::dummy(Network::Testnet, 7), CROSS_DOMAIN_ONLY)], + None, + &signer, + Some(coinjoin_path), + ) + .await + .expect("the named CoinJoin account covers 0.15 DASH"); + + let spent: HashSet = payment + .transaction + .input + .iter() + .map(|i| i.previous_output) + .collect(); + assert!( + spent.iter().all(|op| coinjoin_ops.contains(op)), + "every input must come from the named CoinJoin account, spent {spent:?}" + ); + assert!( + !spent.iter().any(|op| bip44_ops.contains(op)), + "an explicitly-named domain must not pull BIP44 inputs, spent {spent:?}" + ); + } +} diff --git a/packages/rs-platform-wallet/src/wallet/mod.rs b/packages/rs-platform-wallet/src/wallet/mod.rs index 1963422be7..1c03472516 100644 --- a/packages/rs-platform-wallet/src/wallet/mod.rs +++ b/packages/rs-platform-wallet/src/wallet/mod.rs @@ -2,6 +2,7 @@ pub mod apply; pub mod asset_lock; pub mod core; pub mod core_address_key; +pub mod funding_privacy; pub mod identity; pub mod persister; pub mod platform_addresses; diff --git a/packages/rs-unified-sdk-jni/src/funding.rs b/packages/rs-unified-sdk-jni/src/funding.rs index f8dc82f050..dd8f2d5bfb 100644 --- a/packages/rs-unified-sdk-jni/src/funding.rs +++ b/packages/rs-unified-sdk-jni/src/funding.rs @@ -258,6 +258,40 @@ fn read_cstring_opt( } } +/// Strict variant of [`read_cstring_opt`] for parameters where a JNI read +/// failure must be surfaced rather than silently degraded. JVM null (or an +/// empty string) is still `Ok(None)` (the natural "unset" default), but a +/// genuine `get_string` error THROWS and returns `Err(())` instead of falling +/// back to `None`. Used for money-source parameters such as `fundingPath`, +/// where degrading to the default account would spend the wrong coins. +pub(crate) fn read_cstring_opt_strict( + env: &mut JNIEnv, + s: &JString, + field: &str, +) -> Result, ()> { + if s.is_null() { + return Ok(None); + } + let owned: String = match env.get_string(s) { + Ok(v) => v.into(), + Err(_) => { + let _ = env.exception_clear(); + throw_sdk_exception(env, 1, &format!("{field} string was invalid")); + return Err(()); + } + }; + if owned.is_empty() { + return Ok(None); + } + match std::ffi::CString::new(owned) { + Ok(c) => Ok(Some(c)), + Err(_) => { + throw_sdk_exception(env, 1, &format!("{field} contained an interior NUL")); + Err(()) + } + } +} + /// The default Orchard payment address for `account` on the wallet's bound /// shielded sub-wallet — bridges `platform_wallet_manager_shielded_default_address`. /// Returns the 43 raw bytes (11-byte diversifier + 32-byte pk_d) as a diff --git a/packages/rs-unified-sdk-jni/src/wallet_manager.rs b/packages/rs-unified-sdk-jni/src/wallet_manager.rs index f3731ef23c..93b9046ab2 100644 --- a/packages/rs-unified-sdk-jni/src/wallet_manager.rs +++ b/packages/rs-unified-sdk-jni/src/wallet_manager.rs @@ -1014,16 +1014,20 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_c } /// `core_wallet_build_signed_payment` — build + sign a standard L1 payment -/// funded from the UNION of the wallet's signable funds accounts (BIP44 + -/// BIP32 + CoinJoin + DashPay receiving; watch-only DashPay external accounts -/// excluded), and return the result WITHOUT broadcasting. +/// funded from ONE of the wallet's signable funds accounts, and return the +/// result WITHOUT broadcasting. /// /// `core_handle` is the transient core-wallet `Handle` from /// [platformWalletGetCore]. `outputs_blob` is the recipients, encoded /// big-endian as `u32 count` then per row `u32 addrLen, addr utf8, u64 amount` /// (`ManagedPlatformWallet.encodePaymentOutputs`). `fee_per_kb` is duffs/kB, or /// 0 for the default. `core_signer_handle` is the manager's -/// `MnemonicResolverHandle`. +/// `MnemonicResolverHandle`. `funding_path` is an optional UTF-8 BIP32 +/// derivation-path string (dashpay/platform#4184) naming the SINGLE funds +/// account whose UTXOs fund the payment: null (the default) funds from the +/// unmixed BIP44 account; an explicit account-level path (e.g. the DIP-9 +/// CoinJoin account path) funds strictly from that one account, with no union +/// across accounts and no consent gate. /// /// Returns a `byte[]` packed big-endian as `u64 fee, u64 change,` then the /// consensus-serialized signed transaction bytes (`fee` and `change` in duffs), @@ -1038,6 +1042,7 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_c outputs_blob: JByteArray, fee_per_kb: jlong, core_signer_handle: jlong, + funding_path: JString, ) -> jbyteArray { guard(&mut env, ptr::null_mut(), |env| { if core_handle == 0 { @@ -1060,6 +1065,21 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_c return ptr::null_mut(); } }; + // Optional BIP32 derivation-path string naming the single funds account + // (null = the unmixed BIP44 account). Passed to the FFI as UTF-8 bytes + // (without the trailing NUL) + length. Uses the STRICT reader: a genuine + // read error must throw, not silently degrade this money-source param to + // the default BIP44 account (which would spend the wrong coins). + let funding_path = + match crate::funding::read_cstring_opt_strict(env, &funding_path, "fundingPath") { + Ok(v) => v, + Err(()) => return ptr::null_mut(), + }; + let (funding_path_ptr, funding_path_len) = + funding_path.as_ref().map_or((ptr::null(), 0usize), |c| { + let b = c.as_bytes(); + (b.as_ptr(), b.len()) + }); let mut out_tx_bytes: *mut u8 = ptr::null_mut(); let mut out_tx_len: usize = 0; @@ -1072,6 +1092,8 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_c blob.len(), fee_per_kb as u64, core_signer_handle as *mut rs_sdk_ffi::MnemonicResolverHandle, + funding_path_ptr, + funding_path_len, &mut out_tx_bytes, &mut out_tx_len, &mut out_fee, From fe72da65170c185c7cae255e6d11a9774851e4d1 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Fri, 31 Jul 2026 01:04:52 -0400 Subject: [PATCH 4/7] feat(platform-wallet): expose account-level derivationPath in accountBalances MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds derivation_path (Option) to AccountBalanceRow, the FFI AccountBalanceEntryFFI (appended, ABI-additive), and the JNI JSON ("derivationPath": string|null). Computed from the same AccountType::derivation_path(network) the funding-path spend selector compares against, so the emitted string is byte-identical to what build_signed_payment expects — the app passes it verbatim to spend a DashPay receival account. Null for account types with no derivable path. Co-Authored-By: Claude Opus 4.8 --- .../src/core_wallet_types.rs | 13 +++++++++ packages/rs-platform-wallet-ffi/src/wallet.rs | 22 ++++++++++++++ .../src/manager/accessors.rs | 29 +++++++++++++++++-- packages/rs-unified-sdk-jni/src/dashpay.rs | 11 ++++++- 4 files changed, 72 insertions(+), 3 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet_types.rs b/packages/rs-platform-wallet-ffi/src/core_wallet_types.rs index 20e5e82ac2..c3edadfa51 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet_types.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet_types.rs @@ -491,6 +491,17 @@ fn account_index_of(at: &key_wallet::account::AccountType) -> u32 { /// account. The pool counts are meaningful for both funds and keys /// variants; the explorer surfaces them as the headline number on /// keys-only rows where balance reads zero by construction. +/// +/// `derivation_path` is a heap-owned, NUL-terminated UTF-8 C string +/// carrying the account-level derivation path (e.g. the DIP-15 +/// `m/9'/coinType'/15'/{index}'/0x/0x` for a +/// DashPay receiving-funds account). It is `null` for account types that +/// have no derivable account-level path. Freed by +/// [`platform_wallet_manager_free_account_balances`]. Appended as the +/// LAST field so the layout change is purely additive — existing readers +/// of the earlier fields are byte-compatible, but any consumer that +/// indexes the array by struct stride (iOS/Android bindings) must be +/// regenerated against this definition before use. #[repr(C)] pub struct AccountBalanceEntryFFI { pub type_tag: crate::wallet_restore_types::AccountTypeTagFFI, @@ -506,6 +517,8 @@ pub struct AccountBalanceEntryFFI { pub locked: u64, pub keys_used: u32, pub keys_total: u32, + /// Heap-owned NUL-terminated UTF-8 derivation-path string, or `null`. + pub derivation_path: *const c_char, } // --------------------------------------------------------------------------- diff --git a/packages/rs-platform-wallet-ffi/src/wallet.rs b/packages/rs-platform-wallet-ffi/src/wallet.rs index 8ffd78a896..9e8a6c6b1d 100644 --- a/packages/rs-platform-wallet-ffi/src/wallet.rs +++ b/packages/rs-platform-wallet-ffi/src/wallet.rs @@ -167,6 +167,16 @@ pub unsafe extern "C" fn platform_wallet_manager_get_account_balances( locked: row.balance.locked(), keys_used: row.keys_used, keys_total: row.keys_total, + // Account-level derivation-path string → heap-owned C + // string, or null when the account type has no path. + // `CString::new` only fails on an interior NUL, which a + // derivation path never contains; `.ok()` degrades that + // impossible case to null rather than panicking. + derivation_path: row + .derivation_path + .and_then(|s| std::ffi::CString::new(s).ok()) + .map(|c| c.into_raw() as *const std::os::raw::c_char) + .unwrap_or(std::ptr::null()), } }) .collect(); @@ -191,6 +201,18 @@ pub unsafe extern "C" fn platform_wallet_manager_free_account_balances( count: usize, ) { if !entries.is_null() && count > 0 { + // Reclaim each entry's heap-owned derivation-path C string before + // dropping the array backing store (mirrors the `into_raw` in the + // producer above). + let slice = std::slice::from_raw_parts_mut(entries, count); + for e in slice.iter_mut() { + if !e.derivation_path.is_null() { + let _ = std::ffi::CString::from_raw( + e.derivation_path as *mut std::os::raw::c_char, + ); + e.derivation_path = std::ptr::null(); + } + } let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(entries, count)); } } diff --git a/packages/rs-platform-wallet/src/manager/accessors.rs b/packages/rs-platform-wallet/src/manager/accessors.rs index 7dfc444c83..327082a76a 100644 --- a/packages/rs-platform-wallet/src/manager/accessors.rs +++ b/packages/rs-platform-wallet/src/manager/accessors.rs @@ -75,12 +75,23 @@ pub struct PlatformAddressSyncConfigSnapshot { /// rather than a positional tuple so adding the next field /// (`pool_count`, `last_used_height`, …) doesn't ripple through every /// destructuring site. -#[derive(Debug, Clone, Copy)] +/// +/// `derivation_path` is the account-level derivation path STRING (e.g. +/// the DIP-15 `m/9'/coinType'/15'/{index}'/0x/0x` for a DashPay receiving-funds account), produced by the SAME +/// `AccountType::derivation_path(network)` call that +/// [`CoreWallet::build_signed_payment`](crate::wallet::core::CoreWallet::build_signed_payment)'s +/// funds-account selector compares against — so a caller can round-trip +/// this exact string back in as `funding_path` to spend from this single +/// account with no divergence risk. `None` for account types that have +/// no derivable account-level path. +#[derive(Debug, Clone)] pub struct AccountBalanceRow { pub account_type: AccountType, pub balance: WalletCoreBalance, pub keys_used: u32, pub keys_total: u32, + pub derivation_path: Option, } /// Snapshot of [`IdentitySyncManager`] tunables / queue depth, returned @@ -383,6 +394,11 @@ impl PlatformWalletManager

{ let Some(info) = wm.get_wallet_info(wallet_id) else { return Vec::new(); }; + // Same `Network` the funds-account selector in + // `CoreWallet::build_signed_payment` derives paths under, so the + // emitted `derivation_path` string is byte-identical to what the + // selector compares against (see `AccountBalanceRow` docs). + let network = info.core_wallet.network(); info.core_wallet .accounts .all_accounts() @@ -405,11 +421,20 @@ impl PlatformWalletManager

{ let pool_total = pool.addresses.len() as u32; (used + pool_used, total + pool_total) }); + let account_type = account.managed_account_type().to_account_type(); + // The single source of truth: the identical call the + // spend-selector uses. `Ok(path)` for account types with + // a derivable account-level path (Standard/BIP44, + // CoinJoin, DashPay receiving-funds, …); `Err` (→ `None`) + // for the ones that have none. + let derivation_path = + account_type.derivation_path(network).ok().map(|p| p.to_string()); AccountBalanceRow { - account_type: account.managed_account_type().to_account_type(), + account_type, balance, keys_used, keys_total, + derivation_path, } }) .collect() diff --git a/packages/rs-unified-sdk-jni/src/dashpay.rs b/packages/rs-unified-sdk-jni/src/dashpay.rs index 725532e7ca..30922cf2cb 100644 --- a/packages/rs-unified-sdk-jni/src/dashpay.rs +++ b/packages/rs-unified-sdk-jni/src/dashpay.rs @@ -356,12 +356,20 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_DashpayNative_walletM if !entries.is_null() && count > 0 { let items = unsafe { std::slice::from_raw_parts(entries, count) }; for e in items { + // Account-level derivation-path string (the value a caller + // hands back to `build_signed_payment` as `funding_path` + // to spend this single account). Null C-string → JSON + // `null`; a present path is JSON-escaped. + let derivation_path_json = unsafe { opt_cstr(e.derivation_path) } + .map(|s| json_string(&s)) + .unwrap_or_else(|| "null".to_string()); rows.push(format!( "{{\"typeTag\":{},\"standardTag\":{},\"index\":{},\ \"registrationIndex\":{},\"keyClass\":{},\ \"userIdentityId\":{},\"friendIdentityId\":{},\ \"confirmed\":{},\"unconfirmed\":{},\"immature\":{},\ - \"locked\":{},\"keysUsed\":{},\"keysTotal\":{}}}", + \"locked\":{},\"keysUsed\":{},\"keysTotal\":{},\ + \"derivationPath\":{}}}", e.type_tag as u8, e.standard_tag as u8, e.index, @@ -375,6 +383,7 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_DashpayNative_walletM e.locked, e.keys_used, e.keys_total, + derivation_path_json, )); } } From 920b70683c84117115bfe62477321ad5708b5968 Mon Sep 17 00:00:00 2001 From: bfoss765 Date: Fri, 31 Jul 2026 18:59:06 -0400 Subject: [PATCH 5/7] fix(platform-wallet): dust + size bounds, typed build errors, and tests for the send hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review findings on dashpay/platform#4247. Dust outputs (BLOCKING). `TransactionBuilder::add_output` applies no relay policy, so a one-duff recipient produced fully signed bytes for a transaction every standard node rejects as nonstandard — from a primitive documented as building a *standard* payment for later broadcast. Each recipient amount is now checked against its OWN destination script's `dust_value()` (546 duffs for P2PKH), before the wallet lock, before any input is reserved and before the signer is called. Fee/size overflow (BLOCKING). The `MAX_FEE_PER_KB = MAX_MONEY / 100` bound assumed the transaction stayed under the 100 kB standard limit, which the method never enforced; key-wallet's `calculate_fee` then multiplies `sat_per_kb * size_bytes` unchecked and overflows at ~878 kB — reachable both by a ~25.8k-recipient list and by a funding account with a few thousand small denominations. Two-sided fix: * the recipient count is bounded at build time against `MAX_STANDARD_TX_SIZE` (derived from `dashcore::policy::MAX_STANDARD_TX_WEIGHT / 4`), with checked arithmetic mirroring key-wallet's own base-size formula; * `MAX_FEE_PER_KB` is re-derived as `u64::MAX / u32::MAX`, which makes the product unrepresentable-free for ANY size a `u32` can express and therefore does not depend on the input count. ~43 DASH/kB is still three orders of magnitude above any legitimate rate; * the signed transaction is re-measured and refused if it exceeds the standard limit. Typed build errors. `PlatformWalletError::TransactionBuild` had no FFI arm, so every `funding_path` failure — "no spendable funds account matches" and "names a watch-only account", the two failure modes the single-account design rests on — reached Kotlin as `Generic(99)` and could only be told apart by string-matching. Adds `ErrorTransactionBuild = 32` (27-31 are claimed by sibling v4.1 stack PRs, so this needs no renumbering whichever order they land) plus the Kotlin `PlatformWallet.TransactionBuild` type. Tests for the six hardening fixes, which shipped with no coverage. `core_wallet/send.rs` had no test module at all; it now covers the `count` bound, `try_reserve_exact`, checked cursor math at every field boundary, UTF-8, and wrong-network address rejection. Also covers MAX_MONEY aggregation, the fee-rate bound, `parse_optional_derivation_path`, dust rejection (including that a refused request reserves nothing), and the new size bound. Also: corrects the stale `PaymentInsufficientFunds` doc, which still described the pre-#4184 union semantics; corrects an inverted fee-direction comment; adds the missing non-empty assertion to a funding-privacy guardrail test; and runs `cargo fmt` over the five files that failed `--check`. platform-wallet 510 passed, platform-wallet-ffi 222 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 --- .../dashsdk/errors/DashSdkError.kt | 19 + .../dashsdk/errors/DashSdkErrorTest.kt | 25 ++ .../src/core_wallet/broadcast.rs | 17 +- .../src/core_wallet/send.rs | 228 +++++++++- packages/rs-platform-wallet-ffi/src/error.rs | 104 +++++ packages/rs-platform-wallet-ffi/src/utils.rs | 54 +++ packages/rs-platform-wallet-ffi/src/wallet.rs | 4 +- packages/rs-platform-wallet/src/error.rs | 19 +- .../src/manager/accessors.rs | 6 +- .../src/wallet/core/send.rs | 388 +++++++++++++++++- .../src/wallet/funding_privacy.rs | 9 +- 11 files changed, 836 insertions(+), 37 deletions(-) diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt index de41a05412..a3b062423f 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt @@ -89,6 +89,24 @@ sealed class DashSdkError( class CoreInsufficientFunds(message: String, cause: Throwable? = null) : PlatformWallet(message, cause) + /** + * `ErrorTransactionBuild` (native code 32). A Core transaction could + * not be assembled from the request. The REQUEST is at fault, so a + * verbatim retry fails identically — the caller must change it. + * + * It is what every non-shortfall `buildSignedPayment` rejection + * surfaces as: a `fundingPath` matching no spendable funds account or + * naming a watch-only one (the two failure modes the single-account + * send design rests on, dashpay/platform#4184), a request breaching a + * monetary bound (MAX_MONEY total, max fee rate, a below-dust + * recipient, an over-100 kB recipient list), or a malformed recipients + * blob. Before this code existed they all arrived as [Generic] with + * native code 99 and could only be told apart by string-matching the + * message. The specific cause is still in [message]. + */ + class TransactionBuild(message: String, cause: Throwable? = null) : + PlatformWallet(message, cause) + class AssetLockNotTracked(message: String, cause: Throwable? = null) : PlatformWallet(message, cause) @@ -245,6 +263,7 @@ sealed class DashSdkError( 23 -> PlatformWallet.AssetLockNotTracked(message, cause) // ErrorAssetLockNotTracked 24 -> PlatformWallet.AssetLockAlreadyConsumed(message, cause) // ErrorAssetLockAlreadyConsumed 25 -> PlatformWallet.AssetLockFundingMismatch(message, cause) // ErrorAssetLockFundingMismatch + 32 -> PlatformWallet.TransactionBuild(message, cause) // ErrorTransactionBuild else -> PlatformWallet.Generic(code, message, cause) } } diff --git a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt index f8e397cade..b2e3ec4b38 100644 --- a/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt +++ b/packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt @@ -105,6 +105,31 @@ class DashSdkErrorTest { assertTrue(broadcastUnconfirmed.message!!.contains("do NOT retry")) } + /** + * `ErrorTransactionBuild` (32) must reach callers as its own type. Every + * non-shortfall `buildSignedPayment` rejection maps to it — including the + * two failure modes the single-account send design rests on, an unmatched + * `fundingPath` and a watch-only one. They previously arrived as + * [DashSdkError.PlatformWallet.Generic] with code 99, distinguishable only + * by string-matching the message (dashpay/platform#4247 review). + */ + @Test + fun transactionBuildFailuresGetTheirOwnType() { + val offset = DashSdkError.PLATFORM_WALLET_CODE_OFFSET + val message = "no spendable funds account matches funding derivation path m/44'/5'/7'" + + val mapped = DashSdkError.fromNative(DashSDKException(offset + 32, message)) + assertTrue( + "code 32 must not fall through to Generic", + mapped is DashSdkError.PlatformWallet.TransactionBuild, + ) + assertEquals(message, mapped.message) + assertFalse( + "the request itself is at fault, so a verbatim retry cannot help", + mapped.isRetryable, + ) + } + @Test fun unmappedPlatformWalletCodesFallBackToGeneric() { val offset = DashSdkError.PLATFORM_WALLET_CODE_OFFSET 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 26fa825fa5..1ef26e5071 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs @@ -236,14 +236,27 @@ mod outcome_tests { ); } + /// An error raised BEFORE the transaction reached the network must not + /// report a txid — the caller would otherwise track a transaction that was + /// never broadcast. The code itself is whatever the blanket `From` impl + /// maps the variant to; `TransactionBuild` gained a dedicated + /// `ErrorTransactionBuild` code (dashpay/platform#4247 review) instead of + /// flattening to `ErrorUnknown`, and the txid contract is unchanged by + /// that. #[test] fn operational_error_does_not_carry_a_txid() { let outcome = classify_broadcast_result( Err(PlatformWalletError::TransactionBuild("invalid".to_string())), txid(4), ); - assert_eq!(outcome.0, None); - assert_eq!(outcome.1.code, PlatformWalletFFIResultCode::ErrorUnknown); + assert_eq!( + outcome.0, None, + "a pre-broadcast failure must report no txid" + ); + assert_eq!( + outcome.1.code, + PlatformWalletFFIResultCode::ErrorTransactionBuild + ); } } diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs index 57033e0376..103f9c0eb6 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs @@ -49,17 +49,23 @@ fn decode_payment_outputs( // (Android armeabi-v7a) can overflow and panic inside this `extern "C"` // frame, where the JNI guard cannot safely recover it. let read_u32 = |buf: &[u8], at: &mut usize| -> Result { - let end = at.checked_add(4).filter(|e| *e <= buf.len()).ok_or_else(|| { - PlatformWalletError::TransactionBuild("truncated recipients blob (u32)".to_string()) - })?; + let end = at + .checked_add(4) + .filter(|e| *e <= buf.len()) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild("truncated recipients blob (u32)".to_string()) + })?; let v = u32::from_be_bytes([buf[*at], buf[*at + 1], buf[*at + 2], buf[*at + 3]]); *at = end; Ok(v) }; let read_u64 = |buf: &[u8], at: &mut usize| -> Result { - let end = at.checked_add(8).filter(|e| *e <= buf.len()).ok_or_else(|| { - PlatformWalletError::TransactionBuild("truncated recipients blob (u64)".to_string()) - })?; + let end = at + .checked_add(8) + .filter(|e| *e <= buf.len()) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild("truncated recipients blob (u64)".to_string()) + })?; let mut b = [0u8; 8]; b.copy_from_slice(&buf[*at..end]); *at = end; @@ -95,9 +101,11 @@ fn decode_payment_outputs( let parsed = DashAddress::from_str(addr_str) .map_err(|e| err(format!("invalid recipient address {addr_str:?}: {e}")))?; - let address = parsed - .require_network(network) - .map_err(|e| err(format!("recipient address {addr_str:?} network mismatch: {e}")))?; + let address = parsed.require_network(network).map_err(|e| { + err(format!( + "recipient address {addr_str:?} network mismatch: {e}" + )) + })?; outputs.push((address, amount)); } Ok(outputs) @@ -206,3 +214,205 @@ pub unsafe extern "C" fn core_wallet_free_payment_bytes(bytes: *mut u8, len: usi let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut(bytes, len)); } } + +/// Decoder hardening tests. +/// +/// `decode_payment_outputs` parses a caller-controlled blob inside an +/// `extern "C"` frame, where a panic or an allocation abort cannot be recovered +/// by the JNI guard. Every bound below was a blocking review finding on +/// dashpay/platform#4247 and shipped without coverage; these pin them so a +/// later cleanup cannot quietly reintroduce `Vec::with_capacity(count)` or +/// unchecked cursor arithmetic. +#[cfg(test)] +mod tests { + use super::*; + use dashcore::Network; + + /// Encode one recipient row in the wire layout `decode_payment_outputs` + /// documents: `u32 addr_len`, the UTF-8 address, `u64 amount`. + fn row(address: &str, amount: u64) -> Vec { + let mut v = Vec::new(); + v.extend_from_slice(&(address.len() as u32).to_be_bytes()); + v.extend_from_slice(address.as_bytes()); + v.extend_from_slice(&amount.to_be_bytes()); + v + } + + fn blob(rows: &[(&str, u64)]) -> Vec { + let mut v = (rows.len() as u32).to_be_bytes().to_vec(); + for (a, amt) in rows { + v.extend_from_slice(&row(a, *amt)); + } + v + } + + fn testnet_address(id: usize) -> String { + DashAddress::dummy(Network::Testnet, id).to_string() + } + + #[test] + fn decodes_a_well_formed_blob() { + let (a, b) = (testnet_address(1), testnet_address(2)); + let decoded = decode_payment_outputs( + &blob(&[(a.as_str(), 1_000_000), (b.as_str(), 546)]), + Network::Testnet, + ) + .expect("a well-formed blob decodes"); + + assert_eq!(decoded.len(), 2); + assert_eq!(decoded[0].0.to_string(), a); + assert_eq!(decoded[0].1, 1_000_000); + assert_eq!(decoded[1].0.to_string(), b); + assert_eq!(decoded[1].1, 546); + } + + #[test] + fn zero_outputs_decode_to_an_empty_vec() { + let decoded = decode_payment_outputs(&0u32.to_be_bytes(), Network::Testnet) + .expect("an empty list is a decode success"); + assert!( + decoded.is_empty(), + "emptiness is rejected upstream, not here" + ); + } + + /// THE allocation blocker: a four-byte blob declaring `u32::MAX` outputs + /// must produce a decode error, never a ~64 GiB `Vec::with_capacity` that + /// takes Rust's process-aborting allocation-failure path inside + /// `extern "C"`. + #[test] + fn an_impossible_count_is_rejected_without_allocating() { + for count in [u32::MAX, u32::MAX / 2, 1_000_000, 1] { + let err = decode_payment_outputs(&count.to_be_bytes(), Network::Testnet) + .expect_err("a header-only blob cannot hold any output"); + let PlatformWalletError::TransactionBuild(m) = err else { + panic!("expected a decode error for count {count}"); + }; + assert!( + m.contains("holds at most 0"), + "count {count} must be bounded by the blob length, got {m:?}" + ); + } + } + + /// The bound is computed from the remaining bytes, so a count that merely + /// overstates a non-empty blob is refused too. + #[test] + fn a_count_exceeding_the_rows_present_is_rejected() { + let a = testnet_address(1); + let mut b = blob(&[(a.as_str(), 1_000)]); + b[0..4].copy_from_slice(&9u32.to_be_bytes()); + + let err = decode_payment_outputs(&b, Network::Testnet).expect_err("9 rows are not present"); + assert!( + matches!(err, PlatformWalletError::TransactionBuild(ref m) if m.contains("declares 9")), + "got {err:?}" + ); + } + + /// Checked cursor arithmetic: a truncated blob is a clean error at every + /// field boundary, never an out-of-bounds slice or a `cursor + len` + /// overflow panic (reachable on 32-bit Android targets). + #[test] + fn truncation_at_any_boundary_is_a_clean_error() { + let a = testnet_address(1); + let full = blob(&[(a.as_str(), 1_000)]); + + for cut in 1..full.len() { + let err = decode_payment_outputs(&full[..cut], Network::Testnet) + .expect_err("a truncated blob must not decode"); + assert!( + matches!(err, PlatformWalletError::TransactionBuild(_)), + "truncation at {cut} must be a decode error, got {err:?}" + ); + } + + // The full blob still decodes, so the loop above proved truncation is + // the cause rather than the fixture being malformed. + assert!(decode_payment_outputs(&full, Network::Testnet).is_ok()); + } + + /// A declared address length far beyond the blob must not panic on + /// `cursor + addr_len`. + #[test] + fn an_absurd_address_length_is_rejected() { + let mut b = 1u32.to_be_bytes().to_vec(); + b.extend_from_slice(&u32::MAX.to_be_bytes()); + b.extend_from_slice(&[0u8; 16]); + + let err = decode_payment_outputs(&b, Network::Testnet) + .expect_err("an address longer than the blob must not decode"); + assert!( + matches!(err, PlatformWalletError::TransactionBuild(_)), + "got {err:?}" + ); + } + + #[test] + fn a_non_utf8_address_is_rejected() { + let mut b = 1u32.to_be_bytes().to_vec(); + b.extend_from_slice(&4u32.to_be_bytes()); + b.extend_from_slice(&[0xff, 0xfe, 0xfd, 0xfc]); + b.extend_from_slice(&1_000u64.to_be_bytes()); + + let err = decode_payment_outputs(&b, Network::Testnet).expect_err("invalid UTF-8"); + assert!( + matches!(err, PlatformWalletError::TransactionBuild(ref m) if m.contains("UTF-8")), + "got {err:?}" + ); + } + + #[test] + fn an_unparseable_address_is_rejected() { + let err = decode_payment_outputs(&blob(&[("not-an-address", 1_000)]), Network::Testnet) + .expect_err("garbage is not an address"); + assert!( + matches!(err, PlatformWalletError::TransactionBuild(ref m) if m.contains("invalid recipient address")), + "got {err:?}" + ); + } + + /// Network confusion is a funds-loss shape: a mainnet address accepted on a + /// testnet wallet (or the reverse) sends real coins to an address the user + /// did not intend. + #[test] + fn a_wrong_network_address_is_rejected() { + let mainnet = DashAddress::dummy(Network::Mainnet, 1).to_string(); + let err = decode_payment_outputs(&blob(&[(mainnet.as_str(), 1_000)]), Network::Testnet) + .expect_err("a mainnet address must not decode for a testnet wallet"); + assert!( + matches!(err, PlatformWalletError::TransactionBuild(ref m) if m.contains("network mismatch")), + "got {err:?}" + ); + + // …and it decodes fine against its own network, proving the rejection + // is the network check rather than the address being malformed. + assert!( + decode_payment_outputs(&blob(&[(mainnet.as_str(), 1_000)]), Network::Mainnet).is_ok() + ); + } + + /// `core_wallet_free_payment_bytes` tolerates the null/zero pair its own + /// documented contract permits. + #[test] + fn freeing_null_payment_bytes_is_a_no_op() { + unsafe { + core_wallet_free_payment_bytes(std::ptr::null_mut(), 0); + core_wallet_free_payment_bytes(std::ptr::null_mut(), 32); + } + } + + /// Round-trip through the real allocation path: what + /// `core_wallet_build_signed_payment` hands out is what + /// `core_wallet_free_payment_bytes` takes back. + #[test] + fn payment_bytes_round_trip_through_the_free_function() { + let payload = vec![7u8; 128]; + let len = payload.len(); + let ptr = Box::into_raw(payload.into_boxed_slice()) as *mut u8; + unsafe { + assert_eq!(std::slice::from_raw_parts(ptr, len), [7u8; 128]); + core_wallet_free_payment_bytes(ptr, len); + } + } +} diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index edccbe300f..510fcd8779 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -172,6 +172,33 @@ pub enum PlatformWalletFFIResultCode { /// rejected the transaction, so its UTXO reservation was released and the /// host may safely retry after addressing the rejection reason. ErrorTransactionBroadcastRejected = 26, + /// Maps `PlatformWalletError::TransactionBuild`. A Core transaction could + /// not be assembled from the request — the request itself is at fault, and + /// the host must change it rather than retry it verbatim. It is the code + /// every `build_signed_payment` rejection lands on: + /// + /// * the named `funding_path` matches no spendable funds account, or names + /// a watch-only one whose coins the local mnemonic cannot sign — the two + /// failure modes the single-account design rests on + /// (dashpay/platform#4184); + /// * the request violates a monetary bound (`MAX_MONEY` output total, + /// `MAX_FEE_PER_KB` fee rate, a below-dust recipient output, or an + /// over-`MAX_STANDARD_TX_SIZE` recipient list); + /// * the recipients blob failed to decode. + /// + /// These previously flattened to `ErrorUnknown` (99), reaching Kotlin as + /// `DashSdkError.PlatformWallet.Generic` and leaving the host to + /// string-match the message to tell a bad funding path from a bad amount + /// (dashpay/platform#4247 review). The specific cause still travels in the + /// result `message` via the typed `Display`. + /// + /// Numbering: 27–31 are claimed by sibling v4.1 stack PRs + /// (`ErrorStaleReservationToken`/`ErrorReservationTokenConsumed` #4185, + /// `ErrorAssetLockInsufficientFunds` #4184, + /// `ErrorReservationWalletMismatch`, `ErrorSigningKeyUnavailable`), so this + /// takes the first slot free on every branch of that stack and needs no + /// renumbering whatever order they land in. + ErrorTransactionBuild = 32, NotFound = 98, // Used exclusively for all the Option that are retuned as errors ErrorUnknown = 99, @@ -337,6 +364,18 @@ impl From for PlatformWalletFFIResult { | PlatformWalletError::PaymentInsufficientFunds { .. } => { PlatformWalletFFIResultCode::ErrorCoreInsufficientFunds } + // Every `build_signed_payment` rejection that is not a shortfall + // arrives here: an unmatched or watch-only `funding_path`, a + // monetary-bound violation (MAX_MONEY / MAX_FEE_PER_KB / dust / + // MAX_STANDARD_TX_SIZE), or a recipients-blob decode failure. + // Without this arm they all flattened to `ErrorUnknown` (99), so + // the two failure modes the single-account design rests on — + // "that path names no spendable account" and "that path is + // watch-only" — were distinguishable only by string-matching the + // message (dashpay/platform#4247 review). + PlatformWalletError::TransactionBuild(..) => { + PlatformWalletFFIResultCode::ErrorTransactionBuild + } PlatformWalletError::AssetLockNotTracked(..) => { PlatformWalletFFIResultCode::ErrorAssetLockNotTracked } @@ -646,6 +685,71 @@ mod tests { } } + /// The one-shot payment primitive's shortfall shares code 22 with the + /// atomic builder's. Pinned separately from + /// `atomic_core_insufficient_funds_maps_to_dedicated_code`, which only ever + /// constructs `CoreInsufficientFunds`: if a cleanup dropped + /// `PaymentInsufficientFunds` from that arm it would silently fall through + /// to `ErrorUnknown` and no existing test would notice. + #[test] + fn payment_insufficient_funds_shares_the_core_shortfall_code() { + let result: PlatformWalletFFIResult = PlatformWalletError::PaymentInsufficientFunds { + available: 9_000_000, + required: 15_000_000, + } + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorCoreInsufficientFunds + ); + let msg = unsafe { std::ffi::CStr::from_ptr(result.message) }.to_string_lossy(); + assert!( + msg.contains("9000000") && msg.contains("15000000"), + "the single-account available/required duffs must survive in the \ + message: {msg}" + ); + } + + /// Every `build_signed_payment` rejection that is not a shortfall is a + /// `TransactionBuild`, and must reach the host as its own code rather than + /// `ErrorUnknown` (99) — otherwise "that funding path names no spendable + /// account" and "that funding path is watch-only", the two failure modes + /// the single-account design rests on, are distinguishable only by + /// string-matching (dashpay/platform#4247 review). + #[test] + fn transaction_build_failures_map_to_a_dedicated_code() { + for message in [ + "no spendable funds account matches funding derivation path m/44'/5'/7'", + "funding derivation path m/9'/5'/4'/0' names a watch-only account", + "output amounts overflow or exceed MAX_MONEY", + "fee rate 99999999999 duffs/kB exceeds the maximum", + "recipients blob declares 4294967295 outputs but holds at most 0", + ] { + let result: PlatformWalletFFIResult = + PlatformWalletError::TransactionBuild(message.to_string()).into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorTransactionBuild, + "{message:?} must not flatten to ErrorUnknown" + ); + let rendered = unsafe { std::ffi::CStr::from_ptr(result.message) }.to_string_lossy(); + assert!( + rendered.contains(message), + "the specific cause must survive in the message: {rendered}" + ); + } + } + + /// The new code must not silently collide with a sibling v4.1 stack PR's + /// (27–31 are claimed; see the variant's doc comment). + #[test] + fn transaction_build_code_is_thirty_two() { + assert_eq!( + PlatformWalletFFIResultCode::ErrorTransactionBuild as i32, + 32 + ); + } + #[test] fn asset_lock_recovery_failures_map_to_stable_codes() { use dashcore::OutPoint; diff --git a/packages/rs-platform-wallet-ffi/src/utils.rs b/packages/rs-platform-wallet-ffi/src/utils.rs index 9d62627bb4..9e8b0eae25 100644 --- a/packages/rs-platform-wallet-ffi/src/utils.rs +++ b/packages/rs-platform-wallet-ffi/src/utils.rs @@ -263,6 +263,60 @@ pub unsafe extern "C" fn platform_wallet_pubkey_hash_from_private_key( mod tests { use super::*; + /// `parse_optional_derivation_path` turns the caller's optional + /// `funding_path` bytes into the single-account selector the send primitive + /// funds from (dashpay/platform#4184). Null/empty means "the default + /// account", so the null case must stay distinguishable from a parse + /// failure — mapping a malformed path to `None` would silently fund from + /// BIP44 instead of the account the caller named. + #[test] + fn optional_derivation_path_treats_null_and_empty_as_default() { + for (ptr, len) in [ + (std::ptr::null::(), 0usize), + (std::ptr::null::(), 12usize), + (b"m/44'/5'/0'".as_ptr(), 0usize), + ] { + let parsed = unsafe { parse_optional_derivation_path(ptr, len) } + .expect("null/empty is not an error"); + assert!(parsed.is_none(), "null/empty must mean the default account"); + } + } + + #[test] + fn optional_derivation_path_parses_account_level_paths() { + use std::str::FromStr; + + for text in ["m/44'/5'/0'", "m/9'/5'/4'/0'", "m/44'/1'/7'"] { + let parsed = unsafe { parse_optional_derivation_path(text.as_ptr(), text.len()) } + .expect("a valid BIP32 path parses"); + assert_eq!( + parsed, + Some(key_wallet::bip32::DerivationPath::from_str(text).expect("valid")), + "{text} must round-trip verbatim" + ); + } + } + + /// A malformed path is an error, never a silent `None`. + #[test] + fn optional_derivation_path_rejects_garbage() { + for text in ["not a path", "m/44'/5'/zzz", "///"] { + let err = unsafe { parse_optional_derivation_path(text.as_ptr(), text.len()) } + .expect_err("garbage must not parse"); + assert_eq!(err.code, PlatformWalletFFIResultCode::ErrorInvalidParameter); + } + } + + #[test] + fn optional_derivation_path_rejects_non_utf8() { + let bytes = [0xffu8, 0xfe, 0xfd]; + let err = unsafe { parse_optional_derivation_path(bytes.as_ptr(), bytes.len()) } + .expect_err("invalid UTF-8 must not parse"); + assert_eq!(err.code, PlatformWalletFFIResultCode::ErrorInvalidParameter); + let msg = unsafe { std::ffi::CStr::from_ptr(err.message) }.to_string_lossy(); + assert!(msg.contains("UTF-8"), "got {msg}"); + } + #[test] fn test_hash160_matches_known_vector() { use dashcore::hashes::Hash; diff --git a/packages/rs-platform-wallet-ffi/src/wallet.rs b/packages/rs-platform-wallet-ffi/src/wallet.rs index 9e8a6c6b1d..746704ef33 100644 --- a/packages/rs-platform-wallet-ffi/src/wallet.rs +++ b/packages/rs-platform-wallet-ffi/src/wallet.rs @@ -207,9 +207,7 @@ pub unsafe extern "C" fn platform_wallet_manager_free_account_balances( let slice = std::slice::from_raw_parts_mut(entries, count); for e in slice.iter_mut() { if !e.derivation_path.is_null() { - let _ = std::ffi::CString::from_raw( - e.derivation_path as *mut std::os::raw::c_char, - ); + let _ = std::ffi::CString::from_raw(e.derivation_path as *mut std::os::raw::c_char); e.derivation_path = std::ptr::null(); } } diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 09e99cd0b1..edd626c6ae 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -72,12 +72,19 @@ pub enum PlatformWalletError { AssetLockTransaction(String), /// A general Core L1 payment build (`CoreWallet::build_signed_payment`) - /// could not cover the requested outputs plus fee from the union of the - /// wallet's *signable* funds accounts (BIP44 + BIP32 + CoinJoin + DashPay - /// receiving; watch-only DashPay external accounts are excluded). `available` - /// is the total selectable value across those accounts, `required` the - /// outputs-plus-fee target — carried as exact duff amounts (instead of being - /// flattened into a string) so callers can render a precise shortfall. + /// could not cover the requested outputs plus fee from the **one** funds + /// account named by `funding_path` (defaulting to the unmixed BIP44 + /// account). `available` is that single selected account's spendable total + /// and `required` its outputs-plus-fee target — carried as exact duff + /// amounts (instead of being flattened into a string) so callers can render + /// a precise shortfall. + /// + /// Both figures are deliberately SINGLE-ACCOUNT, never a wallet-wide union: + /// reporting a cross-account total against a single-account shortfall would + /// invite a retry that can only succeed by linking privacy domains, which + /// this primitive will not do. The actionable signal is "fund from a + /// different account", not "retry the same one with a smaller amount" + /// (dashpay/platform#4073 → #4184; see `crate::wallet::funding_privacy`). #[error( "payment coin selection is short: available {available} duffs, \ required {required} duffs" diff --git a/packages/rs-platform-wallet/src/manager/accessors.rs b/packages/rs-platform-wallet/src/manager/accessors.rs index 327082a76a..5ef4bc5a34 100644 --- a/packages/rs-platform-wallet/src/manager/accessors.rs +++ b/packages/rs-platform-wallet/src/manager/accessors.rs @@ -427,8 +427,10 @@ impl PlatformWalletManager

{ // a derivable account-level path (Standard/BIP44, // CoinJoin, DashPay receiving-funds, …); `Err` (→ `None`) // for the ones that have none. - let derivation_path = - account_type.derivation_path(network).ok().map(|p| p.to_string()); + let derivation_path = account_type + .derivation_path(network) + .ok() + .map(|p| p.to_string()); AccountBalanceRow { account_type, balance, diff --git a/packages/rs-platform-wallet/src/wallet/core/send.rs b/packages/rs-platform-wallet/src/wallet/core/send.rs index f4d4e3a554..ec589cc7e8 100644 --- a/packages/rs-platform-wallet/src/wallet/core/send.rs +++ b/packages/rs-platform-wallet/src/wallet/core/send.rs @@ -55,7 +55,9 @@ use key_wallet::managed_account::ManagedCoreFundsAccount; use key_wallet::signer::Signer; use key_wallet::wallet::managed_wallet_info::coin_selection::{SelectionError, SelectionStrategy}; use key_wallet::wallet::managed_wallet_info::fee::FeeRate; -use key_wallet::wallet::managed_wallet_info::transaction_builder::{BuilderError, TransactionBuilder}; +use key_wallet::wallet::managed_wallet_info::transaction_builder::{ + BuilderError, TransactionBuilder, +}; use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; use crate::broadcaster::TransactionBroadcaster; @@ -70,14 +72,50 @@ const DEFAULT_FEE_PER_KB: u64 = 1000; /// Consensus cap on any single amount this primitive will accept or aggregate. const MAX_MONEY: u64 = dashcore::blockdata::constants::MAX_MONEY; +/// Dash's standard-transaction size limit, in bytes. A transaction above this +/// is non-standard and will not relay, so building one is never useful. +/// +/// Derived from `dashcore::policy::MAX_STANDARD_TX_WEIGHT` (400_000 weight +/// units) rather than hard-coded: Dash has no segwit, so weight is exactly +/// 4× size and the byte limit is `MAX_STANDARD_TX_WEIGHT / 4` = 100_000. +const MAX_STANDARD_TX_SIZE: usize = (dashcore::policy::MAX_STANDARD_TX_WEIGHT / 4) as usize; + +/// Encoded size of one P2PKH output, matching key-wallet's `TX_OUTPUT_SIZE`. +const TX_OUTPUT_SIZE: usize = 34; + +/// Encoded size of one signed P2PKH input, matching the `148` key-wallet passes +/// to `select_coins_with_size`. +const TX_INPUT_SIZE: usize = 148; + +/// Largest a Bitcoin/Dash varint can encode to. Used instead of the exact +/// varint width so the size estimate never comes in under key-wallet's. +const MAX_VARINT_SIZE: usize = 9; + /// Upper bound on the caller-supplied fee rate, in duffs/kB. /// -/// Derived so that even a maximum-size standard transaction cannot produce a -/// fee above [`MAX_MONEY`]: Dash's standard-transaction limit is 100_000 bytes, -/// i.e. 100 kB, and `FeeRate::calculate_fee` computes -/// `sat_per_kb * size_bytes / 1000` — so `MAX_MONEY / 100` also keeps the -/// intermediate `sat_per_kb * size_bytes` product (≤ 2.1e18) inside `u64`. -const MAX_FEE_PER_KB: u64 = MAX_MONEY / 100; +/// `FeeRate::calculate_fee` computes `sat_per_kb * size_bytes` with **unchecked** +/// `u64` multiplication (key-wallet `managed_wallet_info/fee.rs`), and the public +/// Kotlin/FFI APIs accept any non-negative `Long` — so an unbounded rate panics +/// in an overflow-checking Android build, or wraps in release, silently turning +/// an astronomical requested rate into a tiny fee. +/// +/// The bound is derived so the product cannot overflow **for any transaction +/// size expressible in a `u32`** (~4.3 GB): with `sat_per_kb ≤ u64::MAX / +/// u32::MAX`, `sat_per_kb * size_bytes ≤ u64::MAX` whenever +/// `size_bytes ≤ u32::MAX`. That deliberately does NOT depend on the input +/// count. An earlier `MAX_MONEY / 100` bound assumed the transaction stayed +/// under [`MAX_STANDARD_TX_SIZE`], which this method never enforced — leaving +/// the product to overflow at ~878 kB, reachable both by an oversized recipient +/// list and by a CoinJoin account with a few thousand small denominations +/// (dashpay/platform#4247 and #4256 review). Since size is bounded by `u32` +/// long before it is bounded by policy, tying the bound to `u32::MAX` closes +/// the overflow unconditionally. +/// +/// ~4.29e9 duffs/kB is ~43 DASH/kB — three orders of magnitude above any +/// legitimate rate (the default is 1_000), so nothing real is rejected. The +/// maximum fee this permits on a standard-size transaction is +/// `MAX_FEE_PER_KB * 100` ≈ 4_295 DASH, still far below [`MAX_MONEY`]. +const MAX_FEE_PER_KB: u64 = u64::MAX / u32::MAX as u64; /// The unmixed BIP44 account this primitive is pinned to, in both of its roles: /// @@ -178,6 +216,64 @@ impl CoreWallet { )); } + // Bound the recipient count so the transaction stays relayable AND so + // key-wallet's unchecked `sat_per_kb * size_bytes` fee arithmetic cannot + // be driven to overflow from the output side. `outputs.len()` is the one + // caller-controlled size dimension (~25.8k recipients still fits in a + // practical JNI blob); the input count is wallet-owned and key-wallet + // caps it separately. + // + // Mirrors key-wallet's own base-size formula so the estimate is the one + // the builder will actually use: 8 bytes of version/type/locktime, a + // 1-byte input-count varint, the output-count varint (≤ 9, taken at its + // maximum so this never under-estimates), 34 bytes per P2PKH output, + // and 34 for the change output. Every step is checked — + // `outputs.len() * 34` is an unchecked `usize` multiply inside + // key-wallet. Room for at least one 148-byte input is required, since a + // transaction with no inputs cannot be funded. + let outputs_count = outputs.len(); + let base_size = outputs_count + .checked_mul(TX_OUTPUT_SIZE) + .and_then(|s| s.checked_add(8 + 1 + MAX_VARINT_SIZE + TX_OUTPUT_SIZE)) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "{outputs_count} recipients overflow the transaction size calculation" + )) + })?; + if base_size.saturating_add(TX_INPUT_SIZE) > MAX_STANDARD_TX_SIZE { + return Err(PlatformWalletError::TransactionBuild(format!( + "{outputs_count} recipients need {base_size} bytes of outputs, leaving no \ + room for inputs within the {MAX_STANDARD_TX_SIZE}-byte standard \ + transaction limit" + ))); + } + + // Reject below-dust recipients. `TransactionBuilder::add_output` applies + // no relay policy at all — it copies the requested amount straight into + // the `TxOut` — so without this a one-duff recipient produced a fully + // signed transaction that every standard node rejects as nonstandard, + // from a primitive documented as building a *standard* payment for + // later broadcast (dashpay/platform#4247 review). Checked per output + // against its OWN destination script, not a shared constant: the + // threshold is script-shaped (546 duffs for P2PKH, less for P2SH), + // which is also why key-wallet's hard-coded 546 change-dust literal is + // not reusable here. + // + // After the count bound so an absurd recipient list is rejected before + // this loop runs a script serialization per output, and before the + // wallet lock is taken, before any input is reserved, and before the + // signer is called — a request that can never relay must not tie up + // coins or prompt the user for a keystore signature. + for (address, amount) in &outputs { + let dust = address.script_pubkey().dust_value().to_sat(); + if *amount < dust { + return Err(PlatformWalletError::TransactionBuild(format!( + "output {amount} duffs to {address} is below the {dust}-duff dust \ + threshold for its script type; such a transaction cannot be relayed" + ))); + } + } + // Checked aggregation, bounded by MAX_MONEY. key-wallet sums the same // amounts with unchecked `u64` arithmetic while building, so an // unchecked total here would wrap in release builds (four outputs of @@ -420,8 +516,9 @@ impl CoreWallet { // a plain payment (no special payload) can carry is the single change // output back to the BIP44 sink, so `change = total_out − outputs`. // Any selected input we somehow can't price (impossible — every - // spendable UTXO was recorded above) counts as 0, so `fee` is over- - // rather than under-reported. + // spendable UTXO was recorded above) counts as 0, which LOWERS + // `selected_input_value` and therefore lowers the `saturating_sub` + // result: `fee` would be UNDER-reported, not over-. let selected_input_value: u64 = transaction .input .iter() @@ -431,6 +528,22 @@ impl CoreWallet { let fee = selected_input_value.saturating_sub(total_out); let change_amount = total_out.saturating_sub(outputs_total); + // Belt-and-braces: the pre-build check bounded only the output side, + // because the input count is not knowable until coin selection has run. + // Measure the transaction we actually built and refuse to hand back + // bytes that cannot relay. In practice this fires only for a request + // whose recipient list already passed the output-side bound but whose + // funding account then contributed enough small inputs to push the + // whole transaction over the limit. + let signed_size = transaction.size(); + if signed_size > MAX_STANDARD_TX_SIZE { + return Err(PlatformWalletError::TransactionBuild(format!( + "the signed transaction is {signed_size} bytes, over the \ + {MAX_STANDARD_TX_SIZE}-byte standard transaction limit; it would not relay. \ + Send a smaller amount (fewer inputs) or fewer recipients" + ))); + } + Ok(SignedCorePayment { transaction, fee, @@ -505,7 +618,13 @@ mod tests { /// so the broadcaster is irrelevant (and the balance handle is unused by /// build — a fresh one is fine for the split fixtures that don't return it). fn core_wallet( - wallet_manager: Arc>>, + wallet_manager: Arc< + tokio::sync::RwLock< + key_wallet_manager::WalletManager< + crate::wallet::platform_wallet::PlatformWalletInfo, + >, + >, + >, wallet_id: WalletId, balance: Arc, ) -> CoreWallet { @@ -538,13 +657,21 @@ mod tests { /// CoinJoin account's account-level derivation path (the `funding_path` a /// caller passes to spend previously-mixed coins deliberately). async fn split_account_outpoints_and_coinjoin_path( - wm: &Arc>>, + wm: &Arc< + tokio::sync::RwLock< + key_wallet_manager::WalletManager< + crate::wallet::platform_wallet::PlatformWalletInfo, + >, + >, + >, wallet_id: &WalletId, ) -> (HashSet, HashSet, DerivationPath) { use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; let guard = wm.read().await; - let (_, info) = guard.get_wallet_and_info(wallet_id).expect("wallet present"); + let (_, info) = guard + .get_wallet_and_info(wallet_id) + .expect("wallet present"); let network = info.core_wallet.network(); let bip44 = info .core_wallet @@ -911,11 +1038,244 @@ mod tests { let core = core_wallet(wm, wallet_id, balance); let empty = core.build_signed_payment(vec![], None, &signer, None).await; - assert!(matches!(empty, Err(PlatformWalletError::TransactionBuild(_)))); + assert!(matches!( + empty, + Err(PlatformWalletError::TransactionBuild(_)) + )); let zero = core .build_signed_payment(vec![(recipient(7), 0)], None, &signer, None) .await; - assert!(matches!(zero, Err(PlatformWalletError::TransactionBuild(_)))); + assert!(matches!( + zero, + Err(PlatformWalletError::TransactionBuild(_)) + )); + } + + /// A positive-but-below-dust recipient must be refused. `add_output` applies + /// no relay policy, so before this check the primitive happily returned + /// fully signed bytes for a transaction every standard node rejects as + /// nonstandard (dashpay/platform#4247 review). 546 duffs is the P2PKH + /// threshold `Script::dust_value()` computes. + #[tokio::test] + async fn below_dust_outputs_are_rejected() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(wm, wallet_id, balance); + + let to = recipient(42); + let dust = to.script_pubkey().dust_value().to_sat(); + assert_eq!(dust, 546, "P2PKH dust threshold"); + + for amount in [1u64, dust - 1] { + let result = core + .build_signed_payment(vec![(to.clone(), amount)], None, &signer, None) + .await; + match result { + Err(PlatformWalletError::TransactionBuild(m)) => assert!( + m.contains("dust"), + "the rejection must name dust as the cause, got {m:?}" + ), + other => panic!("{amount} duffs is below dust and must be refused, got {other:?}"), + } + } + + // A dust-sized output hidden among valid ones is caught too — the check + // is per output, not just on the first. + let mixed = core + .build_signed_payment( + vec![ + (recipient(1), 1_000_000), + (recipient(2), 5), + (recipient(3), 1_000_000), + ], + None, + &signer, + None, + ) + .await; + assert!( + matches!(mixed, Err(PlatformWalletError::TransactionBuild(ref m)) if m.contains("dust")), + "a below-dust output among valid ones must still be refused, got {mixed:?}" + ); + + // Exactly at the threshold is valid and still builds. + let at_threshold = core + .build_signed_payment(vec![(to, dust)], None, &signer, None) + .await + .expect("an output exactly at the dust threshold is standard"); + assert_all_inputs_signed(&at_threshold); + } + + /// Rejecting a below-dust request must not cost the caller anything: it + /// happens before the wallet lock, so no input is reserved and the very + /// next legitimate build still finds the account's coins selectable. + #[tokio::test] + async fn a_rejected_dust_request_reserves_nothing() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(wm, wallet_id, balance); + + for _ in 0..3 { + assert!(core + .build_signed_payment(vec![(recipient(9), 100)], None, &signer, None) + .await + .is_err()); + } + + let payment = core + .build_signed_payment(vec![(recipient(9), 1_000_000)], None, &signer, None) + .await + .expect("refused dust requests must not have reserved the account's UTXOs"); + assert_all_inputs_signed(&payment); + } + + /// The output total is aggregated with checked arithmetic and bounded by + /// `MAX_MONEY`. Four outputs of `1 << 62` sum to exactly 2^64: unchecked, + /// that wraps to zero in release builds and lets selection fund only the + /// fee while retaining four enormous outputs — a signed transaction + /// consensus rejects, with meaningless fee/change metadata. + #[tokio::test] + async fn output_total_overflow_and_max_money_are_rejected() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(wm, wallet_id, balance); + + let wrapping = vec![ + (recipient(1), 1u64 << 62), + (recipient(2), 1u64 << 62), + (recipient(3), 1u64 << 62), + (recipient(4), 1u64 << 62), + ]; + match core + .build_signed_payment(wrapping, None, &signer, None) + .await + { + Err(PlatformWalletError::TransactionBuild(m)) => assert!( + m.contains("MAX_MONEY"), + "a wrapping total must be refused as a monetary-bound breach, got {m:?}" + ), + other => panic!("4 × (1 << 62) wraps to zero and must be refused, got {other:?}"), + } + + // A single in-range-but-over-MAX_MONEY amount is refused as well. + let over = core + .build_signed_payment( + vec![(recipient(1), super::MAX_MONEY + 1)], + None, + &signer, + None, + ) + .await; + assert!( + matches!(over, Err(PlatformWalletError::TransactionBuild(ref m)) if m.contains("MAX_MONEY")), + "an amount over MAX_MONEY must be refused, got {over:?}" + ); + + // MAX_MONEY itself is within bounds, so it passes validation and fails + // later on funds — proving the bound is inclusive, not off by one. + let at_max = core + .build_signed_payment(vec![(recipient(1), super::MAX_MONEY)], None, &signer, None) + .await; + assert!( + matches!( + at_max, + Err(PlatformWalletError::PaymentInsufficientFunds { .. }) + ), + "MAX_MONEY exactly must pass the bound and fail on funds, got {at_max:?}" + ); + } + + /// The fee rate is bounded before it reaches key-wallet, whose + /// `calculate_fee` multiplies `sat_per_kb * size_bytes` unchecked — a rate + /// near `u64::MAX` (the Kotlin/FFI APIs accept any non-negative `Long`) + /// panics in an overflow-checking build or wraps in release. + #[tokio::test] + async fn excessive_fee_rates_are_rejected() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(wm, wallet_id, balance); + + for rate in [u64::MAX, u64::MAX / 2, super::MAX_FEE_PER_KB + 1] { + let result = core + .build_signed_payment(vec![(recipient(7), 1_000_000)], Some(rate), &signer, None) + .await; + match result { + Err(PlatformWalletError::TransactionBuild(m)) => assert!( + m.contains("fee rate"), + "the rejection must name the fee rate, got {m:?}" + ), + other => panic!("fee rate {rate} must be refused, got {other:?}"), + } + } + + // A sane rate still works, so the bound isn't rejecting real traffic. + let ok = core + .build_signed_payment(vec![(recipient(7), 1_000_000)], Some(5_000), &signer, None) + .await + .expect("5000 duffs/kB is an ordinary rate"); + assert!(ok.fee > 0); + } + + /// The fee-rate bound must make key-wallet's unchecked + /// `sat_per_kb * size_bytes` product unrepresentable-free for ANY + /// transaction size a `u32` can express — which is the point of deriving it + /// from `u32::MAX` rather than from the standard size limit. A cleanup that + /// loosened it back to `MAX_MONEY / 100` would overflow at ~878 kB, which a + /// funding account with a few thousand small denominations can reach. + #[test] + fn max_fee_rate_cannot_overflow_key_wallets_fee_product() { + for size in [super::MAX_STANDARD_TX_SIZE as u64, 878_434, u32::MAX as u64] { + assert!( + super::MAX_FEE_PER_KB.checked_mul(size).is_some(), + "MAX_FEE_PER_KB * {size} must not overflow u64" + ); + } + // And it stays permissive enough to be irrelevant in practice. + assert!( + super::MAX_FEE_PER_KB > 1_000_000, + "the bound must sit far above any legitimate duffs/kB rate" + ); + } + + /// An oversized recipient list is refused before any wallet work. ~25.8k + /// recipients fit in a practical JNI blob and would drive key-wallet's + /// estimated size past the point where the fee product overflows, as well + /// as producing a transaction far too large to relay. + #[tokio::test] + async fn oversized_recipient_lists_are_rejected() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(wm, wallet_id, balance); + + // Smallest count whose outputs alone leave no room for a single input + // within the 100 kB standard limit. + let over = (super::MAX_STANDARD_TX_SIZE - super::TX_INPUT_SIZE) / super::TX_OUTPUT_SIZE; + let outputs: Vec<_> = (0..over) + .map(|i| (recipient((i % 250) as u8), 1_000u64)) + .collect(); + let result = core + .build_signed_payment(outputs, None, &signer, None) + .await; + match result { + Err(PlatformWalletError::TransactionBuild(m)) => assert!( + m.contains("standard") && m.contains("recipients"), + "the rejection must cite the standard size limit, got {m:?}" + ), + other => panic!("{over} recipients must be refused, got {other:?}"), + } + + // The 25.8k figure from the review is refused by the same bound. + let huge: Vec<_> = (0..25_835) + .map(|i| (recipient((i % 250) as u8), 1_000u64)) + .collect(); + assert!( + matches!( + core.build_signed_payment(huge, Some(super::MAX_FEE_PER_KB), &signer, None) + .await, + Err(PlatformWalletError::TransactionBuild(_)) + ), + "the review's 25,835-recipient overflow case must be refused" + ); } } diff --git a/packages/rs-platform-wallet/src/wallet/funding_privacy.rs b/packages/rs-platform-wallet/src/wallet/funding_privacy.rs index a665410fb5..b9b2653928 100644 --- a/packages/rs-platform-wallet/src/wallet/funding_privacy.rs +++ b/packages/rs-platform-wallet/src/wallet/funding_privacy.rs @@ -299,7 +299,9 @@ mod guardrail { let (bip44_ops, coinjoin_ops, coinjoin_path) = { let guard = wm.read().await; - let (_, info) = guard.get_wallet_and_info(&wallet_id).expect("wallet present"); + let (_, info) = guard + .get_wallet_and_info(&wallet_id) + .expect("wallet present"); let network = info.core_wallet.network(); let bip44: HashSet = info .core_wallet @@ -347,6 +349,11 @@ mod guardrail { .iter() .map(|i| i.previous_output) .collect(); + // `all` is vacuously true on an empty set, so without this the + // guardrail would pass while proving nothing. The sibling + // `explicit_coinjoin_path_selects_only_coinjoin` in `wallet::core::send` + // already asserts it. + assert!(!spent.is_empty(), "the payment must have selected inputs"); assert!( spent.iter().all(|op| coinjoin_ops.contains(op)), "every input must come from the named CoinJoin account, spent {spent:?}" From 72c000dcfd2d0578b764d34ed0f9e0fbd7f8ec1a Mon Sep 17 00:00:00 2001 From: bfoss765 Date: Fri, 31 Jul 2026 19:20:00 -0400 Subject: [PATCH 6/7] fix(platform-wallet): fail closed when no wallet-level account matches the funding path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape #4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (#4247) and CodeRabbit (#4256). Co-Authored-By: Claude Opus 4.8 --- .../src/wallet/core/send.rs | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/core/send.rs b/packages/rs-platform-wallet/src/wallet/core/send.rs index ec589cc7e8..acc38d918b 100644 --- a/packages/rs-platform-wallet/src/wallet/core/send.rs +++ b/packages/rs-platform-wallet/src/wallet/core/send.rs @@ -408,8 +408,22 @@ impl CoreWallet { // `set_change_address` override, so `acc` must be the funding account — // passing the BIP44 xpub for an explicitly-selected BIP32 account would // record a change entry derived from the wrong xpub into that account's - // pool (dashpay/platform#4184 review). Falls back to `bip44_acc` when no - // wallet-level account matches, preserving the default behavior. + // pool (dashpay/platform#4184 review). + // + // FAILS CLOSED. This previously fell back to `bip44_acc` when no + // wallet-level account matched, which is the same silent-fallback shape + // #4184 removed from the selector: the managed-account lookup below can + // still resolve a CoinJoin or DashPay receival account, so the fallback + // would hand `set_funding` another account's xpub and record a change + // entry derived from it into the funding account's pool. Refusing is the + // only safe answer — the two lookups disagreeing is a wallet-state bug, + // not something to paper over with BIP44 (dashpay/platform#4247 and + // #4256 review). + // + // Verified not to narrow any real path: `all_accounts()` does enumerate + // CoinJoin and DashPay receiving-funds accounts, so every send test — + // including the explicit-CoinJoin one — passes with the fallback + // removed. It was dead code on every exercised path. let funding_wallet_acc = wallet .all_accounts() .into_iter() @@ -418,7 +432,11 @@ impl CoreWallet { .map(|p| p == funding_path) .unwrap_or(false) }) - .unwrap_or(&bip44_acc); + .ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "no wallet-level account matches funding derivation path {funding_path}; refusing to fund with another account's xpub" + )) + })?; // Locate the ONE managed funds account whose account-level path equals // `funding_path`, MUTABLY, so `set_funding` reserves the selected inputs From 0dcdc743e7400eb58a71cec49bea909d27b986a6 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Sat, 1 Aug 2026 08:47:12 -0400 Subject: [PATCH 7/7] feat(kotlin-sdk): expose release_payment_reservation for abandoned builds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `build_signed_payment` reserves its selected inputs and leaves them reserved on success, expecting a broadcast to follow. Nothing across Rust/FFI/JNI/Kotlin let a caller that declines to commit give those coins back, so an abandoned build stranded them for RESERVATION_TTL_BLOCKS (24, ~1h) — and indefinitely while the wallet has no processed height, since `ReservationSet::sweep` early-returns at height 0 and therefore never reclaims a pre-sync reservation. A single abandoned build on a freshly restored wallet could strand the whole balance for the life of the process. Adds a standalone release across all four layers: CoreWallet::release_payment_reservation(&Transaction, Option) core_wallet_release_payment_reservation (FFI) coreWalletReleasePaymentReservation (JNI) ManagedPlatformWallet.releasePaymentReservation (Kotlin) The transaction is the ownership signal: a reserved outpoint is skipped by every other build's coin selection, so no concurrent build can hold a reservation on any input of the transaction being released — releasing its inputs releases precisely this build's own reservation and can never free a competing build's coins. Same signal the internal `release_reservation_after_rejected_broadcast` cleanup already uses. The release consults no height, so it works pre-sync where the TTL backstop cannot. It is idempotent (per-outpoint map removal) and a silent no-op after a successful broadcast — it cannot resurrect a spent coin, since selection reads the UTXO set that sync already updated — so callers can wire it into an unconditional cleanup path. Also routes the build's default-funding-account resolution through the shared `bip44_account_path` helper, so a release and its build can never disagree about what `funding_path: None` means. Tests: release-then-reselect (with the second-build failure pinned as a precondition so it can't pass vacuously), release twice, release after a processed broadcast, release against the wrong account frees nothing, unknown funding path is refused, and the height-0 case — 30 build attempts prove the TTL never fires there, then the explicit release frees the inputs. Addresses review item 3 on dashpay/platform#4247. The reviewer's suggestion to unify this with #4256's reservation-token finalize is tracked separately rather than done here, to avoid reshaping an API the Android app already calls and hard-coupling this PR to #4185. Co-Authored-By: Claude Opus 4.8 --- .../dashsdk/ffi/WalletManagerNative.kt | 29 ++ .../dashsdk/wallet/ManagedCoreWallet.kt | 9 + .../dashsdk/wallet/ManagedPlatformWallet.kt | 49 ++ .../src/core_wallet/send.rs | 72 +++ .../src/wallet/core/send.rs | 492 +++++++++++++++++- .../rs-unified-sdk-jni/src/wallet_manager.rs | 76 +++ 6 files changed, 708 insertions(+), 19 deletions(-) diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt index 60f66658aa..7256f009e8 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt @@ -239,6 +239,35 @@ internal object WalletManagerNative { fundingPath: String?, ): ByteArray + /** + * `core_wallet_release_payment_reservation` — release the UTXO reservation + * a [coreWalletBuildSignedPayment] call took, for a build that will NOT be + * broadcast. + * + * `build_signed_payment` leaves its selected inputs reserved on success, + * expecting a broadcast to follow. A caller that abandons the build must + * call this or the coins stay unselectable until key-wallet's 24-block TTL + * backstop reclaims them — and that backstop never fires before the first + * sync completes (`ReservationSet::sweep` early-returns at height 0), so an + * abandoned build on a freshly restored wallet can otherwise strand the + * whole balance for the life of the process (dashpay/platform#4247 review). + * This call consults no height. + * + * [coreHandle] is a core-wallet handle from [platformWalletGetCore]. + * [txBytes] is the consensus-serialized signed transaction exactly as + * [coreWalletBuildSignedPayment] returned it — the transaction is the + * ownership signal, so only that build's own inputs are released. + * [fundingPath] must be the SAME optional path the build was given (null = + * the unmixed BIP44 account). + * + * Idempotent, and a silent no-op after a successful broadcast. + */ + external fun coreWalletReleasePaymentReservation( + coreHandle: Long, + txBytes: ByteArray, + fundingPath: String?, + ) + /** * `core_wallet_broadcast_transaction` — broadcast a transaction built by * [coreTxBuilderBuildSigned]. [accountType]/[accountIndex] identify the diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt index bb40200f49..2a749865d4 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt @@ -77,6 +77,15 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable { fundingPath, ) + /** + * Release the UTXO reservation a [buildSignedPayment] call took, for a + * build that will not be broadcast. See + * [ManagedPlatformWallet.releasePaymentReservation] for the full contract; + * drive this through it, not directly. + */ + internal fun releasePaymentReservation(txBytes: ByteArray, fundingPath: String?) = + WalletManagerNative.coreWalletReleasePaymentReservation(handle, txBytes, fundingPath) + override fun close() { cleanable.clean() } diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt index 05b0c409c5..4bd3e52f06 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt @@ -255,6 +255,55 @@ class ManagedPlatformWallet internal constructor( } } + /** + * Release the UTXO reservation a [buildSignedPayment] call took, for a + * build this caller has decided **not** to broadcast. + * + * [buildSignedPayment] deliberately leaves its selected inputs reserved on + * success, because the expected next step is a broadcast. If the build is + * abandoned instead — the user backed out of the confirmation screen, an + * upstream check failed, the send screen is being torn down — this must be + * called or those coins stay unselectable. + * + * **Why it matters (dashpay/platform#4247 review).** Without it the inputs + * are stranded until key-wallet's TTL backstop reclaims them 24 blocks + * (~1 hour) later. Worse, that backstop does not merely run late but does + * not run at all before the first sync completes: the sweep early-returns + * while the wallet has no processed height, so a reservation taken at + * height 0 is never reclaimed for the life of the process. A single + * abandoned build on a freshly restored wallet could otherwise strand the + * entire balance. This call consults no height, so it is the one release + * path that works pre-sync. + * + * **Releases only this build's own inputs.** The transaction is the + * ownership signal: a reserved outpoint is skipped by every other build's + * coin selection, so no concurrent build can hold a reservation on any + * input of [txBytes]. + * + * **Idempotent, and safe after a broadcast.** Calling it twice, or on a + * transaction that was in fact broadcast, is a silent no-op rather than an + * error, and it cannot resurrect a spent coin — coin selection reads the + * UTXO set, from which sync removes the spend independently of any + * reservation. That makes it safe in an unconditional `finally` without + * tracking whether the broadcast succeeded. + * + * @param txBytes [SignedCorePayment.txBytes] from the build being + * abandoned. + * @param fundingPath the **same** [fundingPath] the build was given, so the + * release lands on the account holding the reservation; `null` means the + * unmixed BIP44 account, exactly as it does for the build. A path naming + * a different account is harmless but frees nothing. + */ + suspend fun releasePaymentReservation( + txBytes: ByteArray, + fundingPath: String? = null, + ): Unit = gate.op { + require(txBytes.isNotEmpty()) { "txBytes must not be empty" } + mapNativeErrors { + coreWallet().use { core -> core.releasePaymentReservation(txBytes, fundingPath) } + } + } + /** * The wallet's Platform-payment addresses that currently hold credits, * each as a [FundingInput] whose `credits` is the full cached balance — diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs index 103f9c0eb6..f570ac3e9d 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/send.rs @@ -215,6 +215,78 @@ pub unsafe extern "C" fn core_wallet_free_payment_bytes(bytes: *mut u8, len: usi } } +/// Release the UTXO reservation a [`core_wallet_build_signed_payment`] call +/// took, for a build the caller has decided NOT to broadcast. +/// +/// `build_signed_payment` leaves its selected inputs reserved on success, +/// because the expected next step is a broadcast. A caller that abandons the +/// build instead — the user backed out, an upstream check failed, the app is +/// tearing down — must call this, or those coins stay unselectable until +/// key-wallet's 24-block TTL backstop reclaims them. Before the first sync +/// completes that backstop never fires at all (`ReservationSet::sweep` +/// early-returns at height 0), so without this call a single abandoned build +/// on a freshly restored wallet can strand the whole balance for the life of +/// the process (dashpay/platform#4247 review). This call consults no height. +/// +/// Releases ONLY this build's own inputs: the transaction is the ownership +/// signal, since a reserved outpoint is skipped by every other build's coin +/// selection, so no concurrent build can hold a reservation on any input of +/// `tx_bytes`. +/// +/// Idempotent, and a silent no-op when the transaction was in fact broadcast — +/// safe to wire into an unconditional cleanup path without tracking whether the +/// broadcast succeeded. +/// +/// * `handle` — a core-wallet handle (`platform_wallet_get_core`). +/// * `tx_bytes`/`tx_bytes_len` — the consensus-serialized signed transaction +/// exactly as `core_wallet_build_signed_payment` returned it. +/// * `funding_path_ptr`/`funding_path_len` — the SAME optional funding path the +/// build was given, so the release lands on the account holding the +/// reservation. `null` / `0` means the unmixed BIP44 account, as it does for +/// the build. A path naming a different account is harmless but frees +/// nothing. +/// +/// # Safety +/// `tx_bytes` must be readable for `tx_bytes_len` bytes; `funding_path_ptr`, +/// when non-null, must point to `funding_path_len` readable bytes for the +/// duration of the call. +#[no_mangle] +pub unsafe extern "C" fn core_wallet_release_payment_reservation( + handle: Handle, + tx_bytes: *const u8, + tx_bytes_len: usize, + funding_path_ptr: *const u8, + funding_path_len: usize, +) -> PlatformWalletFFIResult { + check_ptr!(tx_bytes); + + let funding_path = match parse_optional_derivation_path(funding_path_ptr, funding_path_len) { + Ok(p) => p, + Err(result) => return result, + }; + + let raw = std::slice::from_raw_parts(tx_bytes, tx_bytes_len); + let transaction: dashcore::Transaction = + match dashcore::consensus::deserialize(raw).map_err(|e| { + PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + format!("tx_bytes is not a consensus-serialized transaction: {e}"), + ) + }) { + Ok(tx) => tx, + Err(result) => return result, + }; + + let option = CORE_WALLET_STORAGE.with_item(handle, |wallet| { + runtime().block_on(wallet.release_payment_reservation(&transaction, funding_path.clone())) + }); + + let result = unwrap_option_or_return!(option); + unwrap_result_or_return!(result); + + PlatformWalletFFIResult::ok() +} + /// Decoder hardening tests. /// /// `decode_payment_outputs` parses a caller-controlled blob inside an diff --git a/packages/rs-platform-wallet/src/wallet/core/send.rs b/packages/rs-platform-wallet/src/wallet/core/send.rs index acc38d918b..18a5e2ca8e 100644 --- a/packages/rs-platform-wallet/src/wallet/core/send.rs +++ b/packages/rs-platform-wallet/src/wallet/core/send.rs @@ -39,10 +39,21 @@ //! That reservation is in-memory only (never //! serialized) and is released when the spend is later processed back into the //! wallet by sync, or by the reservation-TTL backstop, or explicitly via -//! [`ManagedCoreFundsAccount::release_reservation`] for an abandoned build. No +//! [`CoreWallet::release_payment_reservation`] for an abandoned build. No //! balance is debited until the transaction actually confirms — exactly what //! the transition flow needs, since dashj owns commit/broadcast. //! +//! ## Abandoning a build +//! +//! A caller that builds and then decides not to broadcast MUST call +//! [`CoreWallet::release_payment_reservation`] with the transaction it was +//! handed. Without it the selected inputs stay reserved until the TTL backstop +//! fires 24 blocks later — and, critically, **forever** while the wallet has no +//! processed height: key-wallet's `ReservationSet::sweep` early-returns at +//! height 0, so a build made before the first sync completes can strand the +//! whole balance for the life of the process (dashpay/platform#4247 review). +//! The explicit release is height-independent and closes that hole. +//! //! [`ManagedCoreFundsAccount::release_reservation`]: //! key_wallet::managed_account::ManagedCoreFundsAccount::release_reservation @@ -337,24 +348,7 @@ impl CoreWallet { // Resolve the account-level path of the unmixed BIP44 account: both the // default funding source and the change sink. - let bip44_path = info - .core_wallet - .accounts - .standard_bip44_accounts - .get(&BIP44_ACCOUNT_INDEX) - .ok_or_else(|| { - PlatformWalletError::TransactionBuild(format!( - "BIP44 account {BIP44_ACCOUNT_INDEX} not found for payment funding" - )) - })? - .managed_account_type() - .to_account_type() - .derivation_path(network) - .map_err(|e| { - PlatformWalletError::TransactionBuild(format!( - "failed to derive the unmixed BIP44 account-level path: {e}" - )) - })?; + let bip44_path = bip44_account_path(info, network)?; let funding_path = funding_path.unwrap_or_else(|| bip44_path.clone()); let funds_from_change_account = funding_path == bip44_path; @@ -568,6 +562,142 @@ impl CoreWallet { change_amount, }) } + + /// Release the UTXO reservation that a previous [`build_signed_payment`] + /// took, for a build the caller has decided **not** to broadcast. + /// + /// [`build_signed_payment`] deliberately leaves its selected inputs + /// reserved on success, because the expected next step is a broadcast. A + /// caller that abandons the build instead — the user backed out of the + /// confirmation screen, an upstream check failed, the app is tearing + /// down — must say so, or those coins stay unselectable. + /// + /// ## Why this is needed (dashpay/platform#4247 review) + /// + /// Without an explicit release the inputs are stranded until key-wallet's + /// TTL backstop reclaims them `RESERVATION_TTL_BLOCKS` (24) blocks later, + /// roughly an hour. Worse, that backstop is not merely slow but *absent* + /// before the first sync completes: `ReservationSet::sweep` early-returns + /// when the current height is 0, so a reservation taken at height 0 is + /// never reclaimed for the life of the process. A single abandoned build + /// on a freshly restored wallet could therefore strand the entire balance + /// indefinitely. This method takes no height and consults none, so it is + /// the one release path that works pre-sync. + /// + /// ## What is released — only this build's own inputs + /// + /// The transaction *is* the ownership signal. `build_signed_payment` + /// reserves exactly the outpoints it selected, and a reserved outpoint is + /// skipped by every subsequent coin selection — so no concurrent build can + /// hold a reservation on any input of `transaction`. Releasing precisely + /// `transaction`'s inputs therefore releases precisely this build's own + /// reservation and can never free a competing in-flight build's coins. + /// (The same signal already backs the internal + /// [`release_reservation_after_rejected_broadcast`] cleanup.) + /// + /// [`release_reservation_after_rejected_broadcast`]: + /// crate::wallet::reservations::release_reservation_after_rejected_broadcast + /// + /// ## Idempotent, and safe after a broadcast + /// + /// Releasing is a per-outpoint map removal, so calling this twice — or on + /// a transaction that was in fact broadcast — is a silent no-op rather + /// than an error. It cannot resurrect a spent coin: coin selection reads + /// the UTXO set, and a broadcast spend is removed from that set by sync + /// independently of any reservation. That makes the release safe to wire + /// into an unconditional cleanup path (a `finally`, a teardown hook) + /// without the caller having to track whether the broadcast succeeded. + /// + /// ## Parameters + /// + /// * `transaction` — the transaction [`build_signed_payment`] returned + /// (`SignedCorePayment::transaction`), or the same bytes deserialized. + /// * `funding_path` — the **same** `funding_path` the build was given, so + /// the release lands on the account that holds the reservation. `None` + /// means the unmixed BIP44 account, exactly as it does for the build. + /// Passing a path that names a different account is harmless: that + /// account's ledger holds none of these outpoints, so nothing is + /// released. + /// + /// [`build_signed_payment`]: CoreWallet::build_signed_payment + pub async fn release_payment_reservation( + &self, + transaction: &Transaction, + funding_path: Option, + ) -> Result<(), PlatformWalletError> { + // `release_reservation` takes `&self` and no manager entry is mutated, + // so a read lock suffices — abandoning a build must not serialize + // against concurrent sends (same reasoning as the rejected-broadcast + // cleanup in `crate::wallet::reservations`). + let wm = self.wallet_manager.read().await; + let (_, info) = wm + .get_wallet_and_info(&self.wallet_id) + .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; + + let network = info.core_wallet.network(); + let funding_path = match funding_path { + Some(path) => path, + None => bip44_account_path(info, network)?, + }; + + // PRIVACY-DOMAIN-OK: iterates funds accounts only to LOOK ONE UP by + // derivation path, exactly as the build does. Nothing is accumulated + // across accounts and only the named account's ledger is touched. + for account in info.core_wallet.accounts.all_funding_accounts() { + let account_path = account + .managed_account_type() + .to_account_type() + .derivation_path(network) + .map_err(|e| { + PlatformWalletError::TransactionBuild(format!( + "failed to derive account-level path for a funds account: {e}" + )) + })?; + if account_path == funding_path { + account.release_reservation(transaction); + return Ok(()); + } + } + + // An unresolvable path is a caller error worth reporting, and is NOT + // the idempotent case: repeat releases resolve the account fine and + // no-op inside it. Watch-only accounts are not filtered out here the + // way the build filters them — releasing is not a spend, and a + // watch-only account can never have been funded a build to abandon. + Err(PlatformWalletError::TransactionBuild(format!( + "no funds account matches funding derivation path {funding_path}; \ + the build to abandon must be released against the account that funded it" + ))) + } +} + +/// Account-level derivation path of the unmixed BIP44 account at +/// [`BIP44_ACCOUNT_INDEX`] — the default funding source and the change sink. +/// +/// Shared by the build and the release paths so both resolve `funding_path: +/// None` to the same account; a release that disagreed with its build would +/// silently fail to free anything. +fn bip44_account_path( + info: &crate::wallet::platform_wallet::PlatformWalletInfo, + network: dashcore::Network, +) -> Result { + info.core_wallet + .accounts + .standard_bip44_accounts + .get(&BIP44_ACCOUNT_INDEX) + .ok_or_else(|| { + PlatformWalletError::TransactionBuild(format!( + "BIP44 account {BIP44_ACCOUNT_INDEX} not found for payment funding" + )) + })? + .managed_account_type() + .to_account_type() + .derivation_path(network) + .map_err(|e| { + PlatformWalletError::TransactionBuild(format!( + "failed to derive the unmixed BIP44 account-level path: {e}" + )) + }) } /// Map a key-wallet [`BuilderError`] to a [`PlatformWalletError`], promoting the @@ -1296,4 +1426,328 @@ mod tests { "the review's 25,835-recipient overflow case must be refused" ); } + + // ------------------------------------------------------------------ + // Abandoning a build — `release_payment_reservation` + // + // `funded_wallet_manager` puts the WHOLE balance on a single UTXO, so + // "the reservation was released" and "the reservation was not released" + // are cleanly distinguishable: while that one input is reserved the next + // build has nothing to select and fails, and the moment it is released + // the next build succeeds. Every test below turns on that signal. + // ------------------------------------------------------------------ + + type TestWalletManager = Arc< + tokio::sync::RwLock< + key_wallet_manager::WalletManager, + >, + >; + + /// Force the wallet's `last_processed_height`. Lets a test reproduce the + /// pre-sync state (height 0) in which key-wallet's TTL sweep early-returns + /// and therefore never reclaims anything. + async fn set_last_processed_height(wm: &TestWalletManager, wallet_id: &WalletId, height: u32) { + use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; + + let mut guard = wm.write().await; + let (_, info) = guard + .get_wallet_and_info_mut(wallet_id) + .expect("wallet present"); + info.core_wallet.update_last_processed_height(height); + } + + /// The BIP44 account-0 outpoints currently in the wallet's UTXO set. + async fn bip44_outpoints(wm: &TestWalletManager, wallet_id: &WalletId) -> HashSet { + let guard = wm.read().await; + let (_, info) = guard + .get_wallet_and_info(wallet_id) + .expect("wallet present"); + info.core_wallet + .accounts + .standard_bip44_accounts + .get(&0) + .map(|a| a.utxos.keys().copied().collect()) + .unwrap_or_default() + } + + /// Process `tx` back into the wallet as a chain-locked spend — what sync + /// does after a real broadcast confirms, removing the spent input from the + /// UTXO set. + async fn process_spend( + wm: &TestWalletManager, + wallet_id: &WalletId, + tx: &dashcore::Transaction, + ) { + use dashcore::BlockHash; + use key_wallet::transaction_checking::{ + BlockInfo, TransactionContext, WalletTransactionChecker, + }; + + let mut guard = wm.write().await; + let (wallet, info) = guard + .get_wallet_mut_and_info_mut(wallet_id) + .expect("wallet present"); + info.core_wallet + .check_core_transaction( + tx, + TransactionContext::InChainLockedBlock(BlockInfo::new( + 2, + BlockHash::all_zeros(), + 1_700_000_100, + )), + wallet, + true, + true, + ) + .await; + } + + /// The core contract: a build reserves its inputs (proved by the second + /// build failing), and abandoning it makes exactly those inputs selectable + /// again immediately — no TTL wait. + #[tokio::test] + async fn abandoning_a_build_makes_its_inputs_selectable_again() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(Arc::clone(&wm), wallet_id, balance); + + let payment = core + .build_signed_payment(vec![(recipient(4), 1_000_000)], None, &signer, None) + .await + .expect("the funded account covers the payment"); + let reserved: HashSet = payment + .transaction + .input + .iter() + .map(|i| i.previous_output) + .collect(); + assert!(!reserved.is_empty(), "the build must have selected inputs"); + + // Precondition: the reservation is real and it is what blocks a + // second build. Without this the test could pass vacuously. + assert!( + matches!( + core.build_signed_payment(vec![(recipient(4), 1_000_000)], None, &signer, None) + .await, + Err(PlatformWalletError::PaymentInsufficientFunds { .. }) + ), + "the first build's reservation must block a second build" + ); + + core.release_payment_reservation(&payment.transaction, None) + .await + .expect("abandoning a build must succeed"); + + let after = core + .build_signed_payment(vec![(recipient(4), 1_000_000)], None, &signer, None) + .await + .expect("the abandoned build's inputs must be selectable again"); + let reselected: HashSet = after + .transaction + .input + .iter() + .map(|i| i.previous_output) + .collect(); + assert_eq!( + reselected, reserved, + "the rebuild must reselect exactly the released inputs" + ); + assert_all_inputs_signed(&after); + } + + /// Releasing twice is a no-op, not an error: the second call resolves the + /// funding account fine and removes outpoints that are already gone. This + /// is what lets a caller wire the release into an unconditional cleanup + /// path without tracking whether it already ran. + #[tokio::test] + async fn abandoning_twice_is_a_no_op() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(Arc::clone(&wm), wallet_id, balance); + + let payment = core + .build_signed_payment(vec![(recipient(5), 1_000_000)], None, &signer, None) + .await + .expect("the funded account covers the payment"); + + for attempt in 0..3 { + core.release_payment_reservation(&payment.transaction, None) + .await + .unwrap_or_else(|e| panic!("release attempt {attempt} must be a no-op, got {e:?}")); + } + + // Still exactly one release's worth of effect: the coins are free. + core.build_signed_payment(vec![(recipient(5), 1_000_000)], None, &signer, None) + .await + .expect("repeated releases must leave the inputs selectable"); + } + + /// Releasing after the transaction was actually broadcast and confirmed is + /// a no-op, and critically cannot resurrect the spent coin: coin selection + /// reads the UTXO set, from which sync has already removed the spend, so + /// the released reservation has nothing to expose. A caller that always + /// releases in a `finally` therefore cannot double-spend itself. + #[tokio::test] + async fn abandoning_after_broadcast_is_a_no_op() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(Arc::clone(&wm), wallet_id, balance); + + // Send the entire balance so the confirmed spend leaves no change to + // fund a follow-up build — any later success could only come from a + // resurrected input. + let payment = core + .build_signed_payment(vec![(recipient(6), 9_900_000)], None, &signer, None) + .await + .expect("the funded account covers the payment"); + let spent: HashSet = payment + .transaction + .input + .iter() + .map(|i| i.previous_output) + .collect(); + + // Stand in for the caller broadcasting and sync observing it. + process_spend(&wm, &wallet_id, &payment.transaction).await; + + core.release_payment_reservation(&payment.transaction, None) + .await + .expect("releasing after a broadcast must be a silent no-op, not an error"); + + let live = bip44_outpoints(&wm, &wallet_id).await; + assert!( + spent.iter().all(|op| !live.contains(op)), + "the release must not resurrect the spent inputs {spent:?} into the UTXO set {live:?}" + ); + } + + /// The height-0 case shumkov flagged: before the first sync completes the + /// wallet's processed height is 0, and key-wallet's `ReservationSet::sweep` + /// early-returns at height 0 — so the TTL backstop never fires and an + /// abandoned build strands the balance for the life of the process. This + /// pins both halves: the TTL genuinely cannot recover it, and the explicit + /// release can. + #[tokio::test] + async fn abandoning_releases_at_height_zero_where_the_ttl_never_fires() { + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + // Pre-sync: no processed height yet. The funding UTXO is non-coinbase, + // so it stays spendable at height 0 — only the sweep is disabled. + set_last_processed_height(&wm, &wallet_id, 0).await; + let core = core_wallet(Arc::clone(&wm), wallet_id, balance); + + let payment = core + .build_signed_payment(vec![(recipient(8), 1_000_000)], None, &signer, None) + .await + .expect("a pre-sync wallet can still build from a confirmed UTXO"); + assert!( + !payment.transaction.input.is_empty(), + "the build must have selected — and so reserved — inputs at height 0" + ); + + // The TTL backstop is inert here: even far beyond RESERVATION_TTL_BLOCKS + // worth of build attempts, the height-0 reservation is never swept, so + // the coins stay stranded. This is the bug, reproduced. + for _ in 0..30 { + assert!( + matches!( + core.build_signed_payment(vec![(recipient(8), 1_000_000)], None, &signer, None) + .await, + Err(PlatformWalletError::PaymentInsufficientFunds { .. }) + ), + "at height 0 the TTL sweep must never reclaim the reservation" + ); + } + + // The explicit release consults no height, so it works where the TTL + // cannot. + core.release_payment_reservation(&payment.transaction, None) + .await + .expect("the release must not depend on a processed height"); + + core.build_signed_payment(vec![(recipient(8), 1_000_000)], None, &signer, None) + .await + .expect("releasing at height 0 must free the stranded inputs"); + } + + /// A release aimed at the wrong account frees nothing — the reservation + /// lives in the funding account's own ledger. Guards the "releases ONLY + /// its own build's inputs" property against a path-confusion regression. + #[tokio::test] + async fn releasing_against_another_account_frees_nothing() { + let (wm, wallet_id, signer) = split_funded_wallet_manager(9_000_000, 20_000_000).await; + let (_, _, coinjoin_path) = + split_account_outpoints_and_coinjoin_path(&wm, &wallet_id).await; + let core = core_wallet(Arc::clone(&wm), wallet_id, Arc::new(WalletBalance::new())); + + // Fund from CoinJoin, then try to release against the BIP44 default. + let payment = core + .build_signed_payment( + vec![(recipient(7), 15_000_000)], + None, + &signer, + Some(coinjoin_path.clone()), + ) + .await + .expect("the named CoinJoin account covers 0.15 DASH"); + + core.release_payment_reservation(&payment.transaction, None) + .await + .expect("a mismatched release resolves the account and simply frees nothing"); + assert!( + matches!( + core.build_signed_payment( + vec![(recipient(7), 15_000_000)], + None, + &signer, + Some(coinjoin_path.clone()), + ) + .await, + Err(PlatformWalletError::PaymentInsufficientFunds { .. }) + ), + "releasing against BIP44 must not free the CoinJoin account's reservation" + ); + + // The correctly-aimed release does free it. + core.release_payment_reservation(&payment.transaction, Some(coinjoin_path.clone())) + .await + .expect("releasing against the funding account must succeed"); + core.build_signed_payment( + vec![(recipient(7), 15_000_000)], + None, + &signer, + Some(coinjoin_path), + ) + .await + .expect("the CoinJoin inputs must be selectable again"); + } + + /// An unresolvable funding path is reported rather than silently treated + /// as "nothing to release" — a caller passing a bad path would otherwise + /// believe it had cleaned up. + #[tokio::test] + async fn releasing_with_an_unknown_funding_path_is_rejected() { + use std::str::FromStr; + + let (wm, wallet_id, balance, signer) = + funded_wallet_manager(StandardAccountType::BIP44Account).await; + let core = core_wallet(Arc::clone(&wm), wallet_id, balance); + + let payment = core + .build_signed_payment(vec![(recipient(3), 1_000_000)], None, &signer, None) + .await + .expect("the funded account covers the payment"); + + let bogus = DerivationPath::from_str("m/44'/5'/77'").expect("valid path"); + match core + .release_payment_reservation(&payment.transaction, Some(bogus)) + .await + { + Err(PlatformWalletError::TransactionBuild(m)) => assert!( + m.contains("no funds account matches"), + "the rejection must name the unresolvable path, got {m:?}" + ), + other => panic!("an unknown funding path must be refused, got {other:?}"), + } + } } diff --git a/packages/rs-unified-sdk-jni/src/wallet_manager.rs b/packages/rs-unified-sdk-jni/src/wallet_manager.rs index 93b9046ab2..5cfef13a66 100644 --- a/packages/rs-unified-sdk-jni/src/wallet_manager.rs +++ b/packages/rs-unified-sdk-jni/src/wallet_manager.rs @@ -1126,6 +1126,82 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_c }) } +/// `core_wallet_release_payment_reservation` — release the UTXO reservation a +/// [coreWalletBuildSignedPayment] call took, for a build that will NOT be +/// broadcast. +/// +/// `build_signed_payment` leaves its selected inputs reserved on success, +/// expecting a broadcast to follow. A caller that abandons the build instead +/// must call this or the coins stay unselectable until key-wallet's 24-block +/// TTL backstop reclaims them — and that backstop never fires before the first +/// sync completes (`ReservationSet::sweep` early-returns at height 0), so an +/// abandoned build on a freshly restored wallet can otherwise strand the whole +/// balance for the life of the process (dashpay/platform#4247 review). This +/// call consults no height. +/// +/// `core_handle` is the transient core-wallet `Handle` from +/// [platformWalletGetCore]. `tx_bytes` is the consensus-serialized signed +/// transaction exactly as [coreWalletBuildSignedPayment] returned it — the +/// transaction is the ownership signal, so only this build's own inputs are +/// released. `funding_path` must be the SAME optional path the build was given +/// (null = the unmixed BIP44 account). +/// +/// Idempotent, and a silent no-op after a successful broadcast, so it is safe +/// in an unconditional cleanup path. Throws only on an invalid handle, +/// undecodable transaction bytes, or an unresolvable funding path. +#[no_mangle] +pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_WalletManagerNative_coreWalletReleasePaymentReservation( + mut env: JNIEnv, + _class: JClass, + core_handle: jlong, + tx_bytes: JByteArray, + funding_path: JString, +) { + guard(&mut env, (), |env| { + if core_handle == 0 { + throw_sdk_exception(env, 1, "core handle is 0"); + return; + } + let raw = match env.convert_byte_array(&tx_bytes) { + Ok(b) => b, + Err(_) => { + let _ = env.exception_clear(); + throw_sdk_exception(env, 1, "txBytes byte[] was invalid"); + return; + } + }; + if raw.is_empty() { + throw_sdk_exception(env, 1, "txBytes must not be empty"); + return; + } + // STRICT reader, matching the build: a genuine read error must throw + // rather than silently degrade to the default BIP44 account, which + // would release against the wrong ledger and leave the real + // reservation stranded — the exact failure this export exists to fix. + let funding_path = + match crate::funding::read_cstring_opt_strict(env, &funding_path, "fundingPath") { + Ok(v) => v, + Err(()) => return, + }; + let (funding_path_ptr, funding_path_len) = + funding_path.as_ref().map_or((ptr::null(), 0usize), |c| { + let b = c.as_bytes(); + (b.as_ptr(), b.len()) + }); + + let result = unsafe { + platform_wallet_ffi::core_wallet_release_payment_reservation( + core_handle as Handle, + raw.as_ptr(), + raw.len(), + funding_path_ptr, + funding_path_len, + ) + }; + take_pwffi_error(env, result); + }) +} + /// `platform_wallet_get_core` — resolve the transient core-wallet `Handle` /// (as `jlong`) from a `PlatformWallet` handle, for [coreWalletBroadcastTransaction]. /// Free with [coreWalletDestroy]. Returns 0 after throwing.