From a4bf2cd071344875ff2de73e58440279b57839d8 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Mon, 18 May 2026 14:02:54 +0200 Subject: [PATCH 1/5] fix(rs-platform-wallet): reserve platform receive address on hand-out (Found-026) [backport] Semantic backport of the #3549-proven Found-026 reserve-on-hand-out fix. Region diverged on v3.1-dev so re-applied by hand, not cherry-picked. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/wallet/platform_addresses/wallet.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index aec6d5b4f9..1c9f485df5 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -259,10 +259,18 @@ impl PlatformAddressWallet { key_wallet::KeySource::Public(xpub) }; - let address = managed_account + // Reserve the address on hand-out (Found-026): platform-payment + // `used` only flips on a positive synced balance, so without + // marking it here a concurrent caller's `next_unused` would + // re-hand the same index before the sync pass. `mark_index_used` + // is idempotent — a later real sync hit on this index is a + // no-op, so gap-limit/`highest_used` accounting isn't doubled. + let info = managed_account .addresses - .next_unused(&key_source, true) + .next_unused_with_info(&key_source, true) .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?; + let address = info.address.clone(); + managed_account.addresses.mark_index_used(info.index); PlatformAddress::try_from(address).map_err(|e| { PlatformWalletError::AddressSync(format!("Failed to convert to PlatformAddress: {e}")) From 8d0f0fbfb6b15980a56cc1c4fc6c6866530fe645 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Mon, 25 May 2026 13:36:01 +0200 Subject: [PATCH 2/5] =?UTF-8?q?refactor(platform-wallet):=20apply=20#3658?= =?UTF-8?q?=20triage=20=E2=80=94=20nits=20+=20accept-risk=20note?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on PR #3658: - Rename inner `info` to `address_info` in next_unused_receive_address to disambiguate from the outer wallet-info binding. - Elide `info.address.clone()` by reordering mark_index_used to use the Copy `index` before moving `address` out. - Document the in-memory-only reservation design via an INTENTIONAL comment block (accept-risk decision on persistence-gap finding). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/wallet/platform_addresses/wallet.rs | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index 69d33cd91f..14583279e2 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -271,12 +271,27 @@ impl PlatformAddressWallet { // re-hand the same index before the sync pass. `mark_index_used` // is idempotent — a later real sync hit on this index is a // no-op, so gap-limit/`highest_used` accounting isn't doubled. - let info = managed_account + let address_info = managed_account .addresses .next_unused_with_info(&key_source, true) .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?; - let address = info.address.clone(); - managed_account.addresses.mark_index_used(info.index); + // INTENTIONAL(CMT-001 / #3658 review): The reservation is in-memory only + // by design — no `persister.store` on the hand-out path. Two properties + // justify this: + // 1. Chain sync re-marks actually-used addresses via the positive- + // balance flip, so a crash that loses the in-memory flag is + // self-healing once any payment arrives at the index. + // 2. Addresses requested but never paid to are freed for reuse on the + // next session, avoiding unbounded index-gap growth from + // speculative or abandoned hand-outs. + // The narrow surviving window — crash before next sync AND no payment + // received by restart — manifests as address-reuse (privacy/accounting), + // not fund loss. See also follow-up: persist on hand-out becomes + // required if/when this function is wired to FFI. + managed_account + .addresses + .mark_index_used(address_info.index); + let address = address_info.address; PlatformAddress::try_from(address).map_err(|e| { PlatformWalletError::AddressSync(format!("Failed to convert to PlatformAddress: {e}")) From 480375c8be10256d4528b84d42706faabda97c2f Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Mon, 25 May 2026 13:43:13 +0200 Subject: [PATCH 3/5] test(platform-wallet): port found-026 back-to-back regression guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #3658 reviewer (thepastaclaw) request for a regression test on the v3.1-dev backport. Ports the `found_026_back_to_back_handout_returns_distinct_addresses` unit test from the e2e campaign tree (PR #3549) along with the `wallet_with_platform_account` helper. Asserts two consecutive next_unused_receive_address calls (no intervening sync) return distinct addresses — the exact Found-026 race the fix in 8d0f0fb closes. The companion test (`found_026_repeated_handouts_advance_gap_limit_exactly_k`) is intentionally NOT ported — it reaches into platform_payment_managed_account pool internals, which is on the deprecation path per QuantumExplorer's review on #3648. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/wallet/platform_addresses/wallet.rs | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index 14583279e2..7f7d0dee12 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -350,3 +350,84 @@ impl std::fmt::Debug for PlatformAddressWallet { .finish() } } + +#[cfg(test)] +mod found_026_tests { + use super::*; + use crate::wallet::persister::{NoPlatformPersistence, WalletPersister}; + use key_wallet::account::account_collection::PlatformPaymentAccountKey; + use key_wallet::wallet::initialization::{ + PlatformPaymentAccountSpec, WalletAccountCreationOptions, + }; + use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; + use key_wallet::{Network, Wallet}; + use key_wallet_manager::WalletManager; + use std::collections::{BTreeMap, BTreeSet}; + + const ACCOUNT_KEY: PlatformPaymentAccountKey = PlatformPaymentAccountKey { + account: 0, + key_class: 0, + }; + + /// Build a network-free `PlatformAddressWallet` over one DIP-17 + /// platform-payment account (account 0, key_class 0). Mirrors the + /// `register_wallet` path: `ManagedWalletInfo::from_wallet` + + /// `insert_wallet`, no SPV / no funding. + fn wallet_with_platform_account() -> PlatformAddressWallet { + let mut pp = BTreeSet::new(); + pp.insert(PlatformPaymentAccountSpec { + account: 0, + key_class: 0, + }); + let opts = WalletAccountCreationOptions::AllAccounts( + BTreeSet::new(), + BTreeSet::new(), + BTreeSet::new(), + BTreeSet::new(), + pp, + ); + let wallet = Wallet::new_random(Network::Testnet, opts).expect("wallet"); + + let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk")); + let info = PlatformWalletInfo { + core_wallet: ManagedWalletInfo::from_wallet(&wallet, 0), + balance: Arc::new(crate::wallet::core::WalletBalance::new()), + identity_manager: crate::wallet::identity::IdentityManager::new(), + tracked_asset_locks: BTreeMap::new(), + }; + let wallet_manager = Arc::new(RwLock::new(WalletManager::new(Network::Testnet))); + let wallet_id = wallet_manager + .try_write() + .expect("uncontended") + .insert_wallet(wallet, info) + .expect("insert"); + let persister = WalletPersister::new(wallet_id, Arc::new(NoPlatformPersistence)); + PlatformAddressWallet::new(sdk, wallet_manager, wallet_id, persister) + } + + /// Found-026 durable guard: two `next_unused_receive_address` calls + /// with NO intervening sync/balance update must return DISTINCT + /// addresses. Pre-fix, `next_unused` re-hands index 0 (its `used` + /// flag only flips on a positive synced balance) → identical + /// addresses → this assertion fails. Post-fix the first call + /// reserves index 0 via `mark_index_used`, so the second yields + /// index 1. + #[tokio::test] + async fn found_026_back_to_back_handout_returns_distinct_addresses() { + let wallet = wallet_with_platform_account(); + + let a = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("first hand-out"); + let b = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("second hand-out"); + + assert_ne!( + a, b, + "back-to-back hand-out with no sync re-handed the same address (Found-026)" + ); + } +} From 1e9d98e9e714db51d65b42d0e94d584a7f4ab5e4 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 26 May 2026 09:20:22 +0200 Subject: [PATCH 4/5] refactor(rs-platform-wallet): generalize Reservations for outpoint and address races Generalizes the OutpointReservations RAII-guard mechanism into Reservations + ReservationGuard, exposing AddressReservations (K = (u32, u32)) as a type alias for the platform-address hand-out path. OutpointReservations is preserved as a domain wrapper composing Reservations + Reservations
so the change-address peek/commit logic stays bound to the broadcast caller. release_after_commit and leak_until_sync semantics from #3585 flow through to both aliases unchanged. Tests cover the address alias (distinct keys, leak_until_sync hold, snapshot reflection) alongside the existing outpoint suite. Also fixes pre-existing clippy::manual_dangling_ptr findings in rs-platform-wallet-ffi/src/core_wallet/broadcast.rs tests, surfaced once the workspace -D warnings gate ran on this branch. Addresses @QuantumExplorer review feedback on #3658: marking address index used at hand-out time advanced gap-limit / highest_used prematurely. Reservation guard holds the slot until the chain-sync positive-balance flip commits highest_used, mirroring leak_until_sync semantics from #3585. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/core_wallet/broadcast.rs | 6 +- .../src/wallet/core/reservations.rs | 423 +++++++++++++----- 2 files changed, 310 insertions(+), 119 deletions(-) diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs index f10d71848e..c96cfcf585 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs @@ -166,7 +166,7 @@ mod tests { // Use a non-null but fake signer pointer; the closure that // would dereference it is never entered because `NULL_HANDLE` // makes `with_item` return `None`. - let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let fake_signer = std::ptr::dangling_mut::(); let mut out_tx: *mut u8 = std::ptr::null_mut(); let mut out_len: usize = 0; @@ -193,7 +193,7 @@ mod tests { let dummy_amount: u64 = 0; let addrs: [*const c_char; 1] = [dummy_addr]; let amts: [u64; 1] = [dummy_amount]; - let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let fake_signer = std::ptr::dangling_mut::(); let mut out_tx: *mut u8 = std::ptr::null_mut(); let mut out_len: usize = 0; @@ -219,7 +219,7 @@ mod tests { fn send_to_addresses_null_element_is_rejected() { let addrs: [*const c_char; 2] = [std::ptr::null(), std::ptr::null()]; let amts: [u64; 2] = [1, 2]; - let fake_signer = 0x1 as *mut MnemonicResolverHandle; + let fake_signer = std::ptr::dangling_mut::(); let mut out_tx: *mut u8 = std::ptr::null_mut(); let mut out_len: usize = 0; diff --git a/packages/rs-platform-wallet/src/wallet/core/reservations.rs b/packages/rs-platform-wallet/src/wallet/core/reservations.rs index 2d869e1258..dabcf979a5 100644 --- a/packages/rs-platform-wallet/src/wallet/core/reservations.rs +++ b/packages/rs-platform-wallet/src/wallet/core/reservations.rs @@ -1,115 +1,116 @@ -//! Per-wallet outpoint reservation set for [`CoreWallet::send_to_addresses`](super::broadcast). +//! Generic key reservation set with an RAII guard, plus domain-specific +//! aliases / wrappers used across the platform-wallet race-protection paths: //! -//! Closes the same-UTXO concurrent-selection race: the first caller reserves its selected -//! outpoints under the write lock; subsequent callers filter them out and short-circuit with -//! [`PlatformWalletError::NoSpendableInputs`](crate::PlatformWalletError) before hitting the -//! network. Reservations are released by an RAII guard on success, error, or panic. +//! * [`OutpointReservations`] — closes the same-UTXO concurrent-selection +//! race in [`CoreWallet::send_to_addresses`](super::broadcast). Composes +//! a generic `Reservations` with a parallel +//! `Reservations
` tracking change addresses peeked but not yet +//! committed to a confirmed broadcast. +//! * [`AddressReservations`] — closes the back-to-back +//! `next_unused_receive_address` hand-out race (Found-026) without +//! eagerly advancing `highest_used` in the upstream `AddressPool`. Keys +//! are `(account_index, address_index)` pairs. +//! +//! Both paths share the [`Reservations`] core: a single `Mutex`-protected +//! `HashSet` with an RAII [`ReservationGuard`] that releases keys on +//! drop, with `release_after_commit` and `leak_until_sync` escape hatches +//! mirroring the broadcast-success / unrecoverable-leak distinction +//! introduced in PR #3585. use std::collections::HashSet; +use std::hash::Hash; use std::sync::{Arc, Mutex}; use dashcore::{Address, OutPoint}; -/// Inner state shared between an [`OutpointReservations`] handle and every -/// outstanding [`OutpointReservationGuard`]. Held behind a single `Mutex` -/// so reservation + change-address tracking commit atomically. -#[derive(Debug, Default)] -struct ReservationsInner { - outpoints: HashSet, - /// Change addresses already committed (`advance=true`) by an - /// in-flight `send_to_addresses` whose broadcast has not yet - /// completed. Concurrent senders that peek a change address still - /// present here advance past it under the same write lock so two - /// disjoint-UTXO sends do not both broadcast with the same change - /// address (privacy regression — CMT-006). Address-keyed rather than - /// `(account, index)` because the upstream pool API exposes addresses - /// but not indices. - pending_change: HashSet
, +/// Inner shared state for [`Reservations`]. Held behind a `Mutex` so +/// reserve / release commit atomically. +#[derive(Debug)] +struct ReservationsInner { + keys: HashSet, +} + +impl Default for ReservationsInner { + fn default() -> Self { + Self { + keys: HashSet::new(), + } + } } -/// Per-wallet set of outpoints that have been selected for an in-flight -/// broadcast but not yet marked spent in `ManagedWalletInfo`, plus any -/// change addresses peeked but not yet reconciled with a confirmed -/// broadcast. +/// Generic per-wallet set of keys reserved by an in-flight operation but +/// not yet committed to durable wallet state. Cheaply cloneable: holds +/// an `Arc>`. All clones share the same set. /// -/// Cheaply cloneable: holds an `Arc>` internally. All clones share -/// the same set. -#[derive(Debug, Default, Clone)] -pub(crate) struct OutpointReservations { - inner: Arc>, +/// `K` must be hashable and cloneable (the guard stores its own copy of +/// the reserved keys to release on drop) and `Debug` so the guard prints +/// usefully in `tracing` events. +#[derive(Debug)] +pub(crate) struct Reservations { + inner: Arc>>, } -impl OutpointReservations { - pub(crate) fn new() -> Self { - Self::default() +impl Default for Reservations { + fn default() -> Self { + Self { + inner: Arc::new(Mutex::new(ReservationsInner::default())), + } } +} - /// Test whether `outpoint` is currently reserved. - #[cfg(test)] - pub(crate) fn contains(&self, outpoint: &OutPoint) -> bool { - let guard = self - .inner - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.outpoints.contains(outpoint) +impl Clone for Reservations { + fn clone(&self) -> Self { + Self { + inner: Arc::clone(&self.inner), + } } +} - /// Test whether a change address is currently pending. - #[cfg(test)] - pub(crate) fn change_address_pending(&self, addr: &Address) -> bool { - let guard = self - .inner - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.pending_change.contains(addr) +impl Reservations { + pub(crate) fn new() -> Self { + Self::default() } - /// Clone the current outpoint reservation set under a single lock - /// acquisition. Callers filter spendable UTXOs against the returned - /// snapshot to avoid one mutex lock per candidate outpoint. - pub(crate) fn snapshot(&self) -> HashSet { + /// Test whether `key` is currently reserved. Only used by tests and + /// the [`OutpointReservations`] wrapper's `#[cfg(test)]` accessors — + /// production paths use [`snapshot`](Self::snapshot) to filter many + /// candidates under one lock acquisition. + #[cfg(test)] + pub(crate) fn contains(&self, key: &K) -> bool { let guard = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.outpoints.clone() + guard.keys.contains(key) } - /// Clone the current pending-change-address set so callers can skip - /// past in-flight peeks without holding the reservation mutex. - pub(crate) fn pending_change_snapshot(&self) -> HashSet
{ + /// Clone the current reservation set under a single lock acquisition. + /// Callers filter spendable candidates against the returned snapshot + /// to avoid one mutex lock per candidate. + pub(crate) fn snapshot(&self) -> HashSet { let guard = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - guard.pending_change.clone() + guard.keys.clone() } - /// Reserve `outpoints` and (optionally) a chosen change address in - /// the same lock acquisition, returning an RAII guard that releases - /// both on drop. The guard must be held until the broadcast outcome - /// is reconciled into wallet state. - pub(crate) fn reserve( - &self, - outpoints: Vec, - change_address: Option
, - ) -> OutpointReservationGuard { + /// Reserve `keys`, returning an RAII guard that releases them on drop. + /// The guard must be held until the operation outcome has been + /// reconciled into durable wallet state. + pub(crate) fn reserve(&self, keys: Vec) -> ReservationGuard { { let mut guard = self .inner .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - for op in &outpoints { - guard.outpoints.insert(*op); - } - if let Some(addr) = &change_address { - guard.pending_change.insert(addr.clone()); + for k in &keys { + guard.keys.insert(k.clone()); } } - OutpointReservationGuard { + ReservationGuard { reservations: Arc::clone(&self.inner), - outpoints, - pending_change: change_address, + keys, released: false, } } @@ -117,29 +118,27 @@ impl OutpointReservations { /// RAII guard releasing reservations on drop. /// -/// Drop is infallible and panic-safe — the underlying `Mutex` is recovered -/// from poisoning so a panicking caller still releases its outpoints -/// and pending change index (if any). +/// Drop is infallible and panic-safe — the underlying `Mutex` is +/// recovered from poisoning so a panicking caller still releases its +/// keys. #[must_use = "dropping the guard immediately releases the reservation"] -pub(crate) struct OutpointReservationGuard { - reservations: Arc>, - outpoints: Vec, - /// Pending change address reserved for this in-flight send (if any). - pending_change: Option
, +pub(crate) struct ReservationGuard { + reservations: Arc>>, + keys: Vec, /// Set after a successful `release_after_commit` so `Drop` is a no-op. released: bool, } -impl OutpointReservationGuard { - /// Release outpoints and any pending change index now, marking the - /// guard inert so its `Drop` is a no-op. Called by the broadcast - /// path after `check_core_transaction` has transitioned the inputs - /// from "reserved" to "spent" and the change index has been - /// committed via `next_change_address(..., true)`. The deliberate - /// release point exists so the same code path that *succeeded* the - /// broadcast also relinquishes the reservation — separating it from - /// the panic/drop path keeps post-broadcast-failure handling - /// (CMT-003) on the implicit `Drop` branch. +impl ReservationGuard { + /// Release the reserved keys now, marking the guard inert so its + /// `Drop` is a no-op. Called by the caller path after the wallet + /// state has transitioned the keys from "reserved" to "committed" + /// (e.g., an outpoint marked spent, an address marked used). + /// + /// The deliberate release point exists so the same code path that + /// *succeeded* the operation also relinquishes the reservation — + /// separating it from the panic/drop path keeps post-failure + /// handling on the implicit `Drop` branch. pub(crate) fn release_after_commit(mut self) { self.do_release(); self.released = true; @@ -147,28 +146,26 @@ impl OutpointReservationGuard { /// Keep the reservation held until the process exits. /// - /// Use this when the broadcast succeeded but wallet state could not be - /// reconciled (e.g., own-built tx not recognised by - /// `check_core_transaction`, or the wallet handle went stale - /// post-broadcast). Releasing the outpoints in that scenario would let a - /// concurrent caller select the same already-broadcast UTXO and produce - /// a double-spend the network would reject — keeping the reservation is - /// the safer of two bad outcomes. + /// Use this when the operation succeeded but wallet state could not + /// be reconciled, OR when the keys must remain reserved across an + /// asynchronous follow-up event (chain sync, balance flip) that + /// will eventually commit them out-of-band. /// - /// **No in-process reclaim path exists today.** The outpoints stay - /// pinned in [`OutpointReservations`] for the lifetime of the - /// `PlatformWalletManager`; only a wallet-process restart (which drops - /// the whole reservations set) releases them. + /// **No in-process reclaim path exists today.** Keys stay pinned in + /// [`Reservations`] for the lifetime of the holding manager; only + /// a process restart (which drops the whole reservations set) + /// releases them. /// /// TODO(@thepastaclaw, PR #3585): a periodic-sync-driven - /// `OutpointReservations::reclaim_confirmed(&[OutPoint])` API would let - /// the next confirmation pass reconcile leaked reservations without a - /// restart. Tracked on the PR review thread. + /// `Reservations::reclaim_committed(&[K])` API would let the next + /// confirmation / balance-sync pass reconcile leaked reservations + /// without a restart. Tracked on the PR review thread. pub(crate) fn leak_until_sync(self) { - // `mem::forget` skips `Drop`, leaving the reservation entries in - // the shared set. The guard's only owned heap allocation is the - // `Vec`, which is small and never freed for the lifetime - // of the process — acceptable given this is a rare error branch. + // `mem::forget` skips `Drop`, leaving the reservation entries + // in the shared set. The guard's only owned heap allocation is + // the `Vec`, which is small and never freed for the lifetime + // of the process — acceptable given this is a rare error / async + // commit branch. std::mem::forget(self); } @@ -177,16 +174,13 @@ impl OutpointReservationGuard { .reservations .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - for op in &self.outpoints { - inner.outpoints.remove(op); - } - if let Some(addr) = self.pending_change.take() { - inner.pending_change.remove(&addr); + for k in &self.keys { + inner.keys.remove(k); } } } -impl Drop for OutpointReservationGuard { +impl Drop for ReservationGuard { fn drop(&mut self) { if self.released { return; @@ -195,6 +189,126 @@ impl Drop for OutpointReservationGuard { } } +// ===================================================================== +// Domain aliases / wrappers +// ===================================================================== + +/// `(account_index, address_index)` key for [`AddressReservations`]. +pub(crate) type AddressReservationKey = (u32, u32); + +/// Per-wallet set of `(account_index, address_index)` slots handed out +/// by [`PlatformAddressWallet::next_unused_receive_address`](crate::wallet::platform_addresses::PlatformAddressWallet::next_unused_receive_address) +/// but not yet flipped to `used` by a positive-balance sync hit. +/// Closes the Found-026 back-to-back hand-out race without prematurely +/// advancing the upstream `AddressPool`'s `highest_used`. +pub(crate) type AddressReservations = Reservations; + +/// RAII guard for [`AddressReservations`]. The platform-payment +/// hand-out path leaks this via `leak_until_sync` after a successful +/// hand-out — exposed as a named alias so any future caller that wants +/// to thread the guard onto its own state (e.g., a transfer builder +/// dropping the guard on broadcast failure) can spell the type. +#[allow(dead_code)] // surfaced for API completeness; no in-tree caller yet +pub(crate) type AddressReservationGuard = ReservationGuard; + +/// Per-wallet set of outpoints reserved by an in-flight +/// `send_to_addresses` plus any change addresses peeked but not yet +/// reconciled with a confirmed broadcast. Cheaply cloneable. +/// +/// Composes a [`Reservations`] (outpoint slot) with a +/// [`Reservations
`] (pending change address). Two distinct +/// generic instances are used rather than one keyed by an enum because +/// the call-sites filter on each independently (`snapshot` returns just +/// the outpoints, `pending_change_snapshot` just the addresses) and the +/// two sets never overlap. Holding them under separate mutexes is safe +/// — `reserve` always takes them in `(outpoints, change)` order, the +/// only acquisition order in the module. +#[derive(Debug, Default, Clone)] +pub(crate) struct OutpointReservations { + outpoints: Reservations, + pending_change: Reservations
, +} + +impl OutpointReservations { + pub(crate) fn new() -> Self { + Self::default() + } + + /// Test whether `outpoint` is currently reserved. + #[cfg(test)] + pub(crate) fn contains(&self, outpoint: &OutPoint) -> bool { + self.outpoints.contains(outpoint) + } + + /// Test whether a change address is currently pending. + #[cfg(test)] + pub(crate) fn change_address_pending(&self, addr: &Address) -> bool { + self.pending_change.contains(addr) + } + + /// Clone the current outpoint reservation set under a single lock + /// acquisition. + pub(crate) fn snapshot(&self) -> HashSet { + self.outpoints.snapshot() + } + + /// Clone the current pending-change-address set so callers can skip + /// past in-flight peeks without holding the reservation mutex. + pub(crate) fn pending_change_snapshot(&self) -> HashSet
{ + self.pending_change.snapshot() + } + + /// Reserve `outpoints` and (optionally) a chosen change address, + /// returning an RAII guard that releases both on drop. The guard + /// must be held until the broadcast outcome is reconciled into + /// wallet state. + pub(crate) fn reserve( + &self, + outpoints: Vec, + change_address: Option
, + ) -> OutpointReservationGuard { + let outpoint_guard = self.outpoints.reserve(outpoints); + let change_guard = change_address.map(|addr| self.pending_change.reserve(vec![addr])); + OutpointReservationGuard { + outpoint_guard, + change_guard, + } + } +} + +/// RAII guard releasing outpoint + (optional) change-address +/// reservations together. Mirrors the `release_after_commit` / +/// `leak_until_sync` API of the generic guard but composes two inner +/// guards under one type so callers don't have to thread both. +#[must_use = "dropping the guard immediately releases the reservation"] +pub(crate) struct OutpointReservationGuard { + outpoint_guard: ReservationGuard, + change_guard: Option>, +} + +impl OutpointReservationGuard { + /// Release outpoints and any pending change index now, marking the + /// guard inert so its `Drop` is a no-op. Called by the broadcast + /// path after `check_core_transaction` has transitioned the inputs + /// from "reserved" to "spent" and the change index has been + /// committed via `next_change_address(..., true)`. + pub(crate) fn release_after_commit(self) { + self.outpoint_guard.release_after_commit(); + if let Some(g) = self.change_guard { + g.release_after_commit(); + } + } + + /// Keep the reservation held until the process exits. See + /// [`ReservationGuard::leak_until_sync`] for the rationale. + pub(crate) fn leak_until_sync(self) { + self.outpoint_guard.leak_until_sync(); + if let Some(g) = self.change_guard { + g.leak_until_sync(); + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -298,4 +412,81 @@ mod tests { "leak_until_sync must keep the outpoint reserved until process restart" ); } + + // ===================================================================== + // Generic `Reservations` tests — exercise the address-reservation + // alias the platform-payment hand-out path uses (Found-026). + // ===================================================================== + + #[test] + fn address_reservation_same_key_twice_is_redundant_release() { + // Reserving the same `(account, index)` from two distinct guards + // is *allowed* (HashSet idempotency) but dropping one releases + // the slot — the second guard would then mistakenly hold a key + // the set no longer contains. The platform-address hand-out + // path guards against this by **checking `contains` before + // reserving** and advancing past any reserved index, so the + // duplicate-reserve case never arises in practice. This test + // documents the invariant rather than the (would-be-broken) + // overlapping-guards behaviour. + let res: AddressReservations = AddressReservations::new(); + let k = (0u32, 5u32); + let g = res.reserve(vec![k]); + assert!(res.contains(&k)); + assert!( + res.contains(&k), + "second `contains` would also return true — callers must skip past it" + ); + drop(g); + assert!(!res.contains(&k)); + } + + #[test] + fn address_reservation_distinct_keys_independent() { + // Mirrors the outpoint `second_reservation_is_disjoint` test + // for the address-reservation alias: two distinct + // `(account, address_index)` slots can be reserved + // concurrently and both remain held until each guard drops. + let res: AddressReservations = AddressReservations::new(); + let k1 = (0u32, 0u32); + let k2 = (0u32, 1u32); + let g1 = res.reserve(vec![k1]); + let g2 = res.reserve(vec![k2]); + assert!(res.contains(&k1)); + assert!(res.contains(&k2)); + drop(g1); + assert!(!res.contains(&k1)); + assert!(res.contains(&k2)); + drop(g2); + assert!(!res.contains(&k2)); + } + + #[test] + fn address_reservation_leak_until_sync_holds_slot() { + // The hand-out path leaks the guard so the slot stays reserved + // until the next balance-positive sync flips `used` on the + // pool — at which point the in-memory reservation is harmless + // because `next_unused` will skip the index regardless. This + // test pins that semantic for the address alias. + let res: AddressReservations = AddressReservations::new(); + let k = (0u32, 7u32); + let g = res.reserve(vec![k]); + g.leak_until_sync(); + assert!( + res.contains(&k), + "leak_until_sync must keep the address slot reserved until process restart" + ); + } + + #[test] + fn address_reservation_snapshot_reflects_state() { + let res: AddressReservations = AddressReservations::new(); + let k1 = (0u32, 0u32); + let k2 = (1u32, 3u32); + let _g = res.reserve(vec![k1, k2]); + let snap = res.snapshot(); + assert!(snap.contains(&k1)); + assert!(snap.contains(&k2)); + assert_eq!(snap.len(), 2); + } } From e655aafdcb2c1f32b3ecf68a02252f69305abac8 Mon Sep 17 00:00:00 2001 From: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Date: Tue, 26 May 2026 09:20:34 +0200 Subject: [PATCH 5/5] fix(rs-platform-wallet): defer mark_index_used to chain-sync via AddressReservations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the eager `managed_account.addresses.mark_index_used(idx)` call on the platform-payment hand-out path with an in-memory AddressReservations slot. Hand-out picks the first `!used && !reserved` index in `0..=highest_generated`, falling back to minting at `highest_generated + 1` when the generated range is exhausted. The upstream pool's `highest_used` is left untouched until a positive-balance sync hit flips `used`, addressing @QuantumExplorer's review feedback on #3658. Reservation guards are leaked via `leak_until_sync` so concurrent / back-to-back hand-outs see the slot as taken; the in-memory set is cleared on process restart, matching the documented "addresses requested but never paid to are freed for reuse on the next session" contract. Regression tests added: - next_unused_receive_address_does_not_advance_highest_used - handout_skips_reserved_but_not_used_indices (three back-to-back hand-outs yield three distinct indices) The original Found-026 back-to-back distinct-address invariant still holds — guarded by the unchanged `found_026_back_to_back_handout_*` test. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/wallet/platform_addresses/wallet.rs | 173 +++++++++++++++--- 1 file changed, 145 insertions(+), 28 deletions(-) diff --git a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs index 7f7d0dee12..0e11a60795 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs @@ -7,6 +7,7 @@ use dpp::fee::Credits; use tokio::sync::RwLock; use crate::error::PlatformWalletError; +use crate::wallet::core::reservations::AddressReservations; use crate::wallet::platform_wallet::{PlatformWalletInfo, WalletId}; use key_wallet_manager::WalletManager; @@ -29,6 +30,14 @@ pub struct PlatformAddressWallet { pub(crate) provider: Arc>>, /// Per-wallet persistence handle for queuing changesets. pub(crate) persister: WalletPersister, + /// `(account_index, address_index)` slots handed out by + /// [`next_unused_receive_address`](Self::next_unused_receive_address) + /// but not yet flipped to `used` by a positive-balance sync hit. + /// Closes the Found-026 back-to-back hand-out race without + /// eagerly advancing the upstream `AddressPool`'s `highest_used` — + /// the upstream flip happens when a real payment lands on the + /// address. + pub(crate) reservations: AddressReservations, } impl PlatformAddressWallet { @@ -47,6 +56,7 @@ impl PlatformAddressWallet { wallet_id, provider: Arc::new(RwLock::new(None)), persister, + reservations: AddressReservations::new(), } } @@ -265,32 +275,79 @@ impl PlatformAddressWallet { key_wallet::KeySource::Public(xpub) }; - // Reserve the address on hand-out (Found-026): platform-payment - // `used` only flips on a positive synced balance, so without - // marking it here a concurrent caller's `next_unused` would - // re-hand the same index before the sync pass. `mark_index_used` - // is idempotent — a later real sync hit on this index is a - // no-op, so gap-limit/`highest_used` accounting isn't doubled. - let address_info = managed_account - .addresses - .next_unused_with_info(&key_source, true) - .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?; - // INTENTIONAL(CMT-001 / #3658 review): The reservation is in-memory only - // by design — no `persister.store` on the hand-out path. Two properties - // justify this: - // 1. Chain sync re-marks actually-used addresses via the positive- - // balance flip, so a crash that loses the in-memory flag is - // self-healing once any payment arrives at the index. - // 2. Addresses requested but never paid to are freed for reuse on the - // next session, avoiding unbounded index-gap growth from - // speculative or abandoned hand-outs. - // The narrow surviving window — crash before next sync AND no payment - // received by restart — manifests as address-reuse (privacy/accounting), - // not fund loss. See also follow-up: persist on hand-out becomes - // required if/when this function is wired to FFI. - managed_account - .addresses - .mark_index_used(address_info.index); + // Found-026 / #3658-review: reserve the slot in an in-memory + // `AddressReservations` set rather than flipping the upstream + // pool's `used` flag. Hand-out must not advance `highest_used` + // (gap-limit accounting) until the address actually receives a + // payment — chain sync flips `used` on the positive-balance + // hit, at which point the reservation becomes redundant. + // + // The reservation must filter the pool's `next_unused_with_info` + // result: that function only knows about the pool's own `used` + // flag, so we scan forward from index 0 picking the first slot + // that is neither `used` nor reserved. If every generated index + // is taken, mint a new one via `address_range`. + let reserved = self.reservations.snapshot(); + let account_index = account_key.account; + let pool = &mut managed_account.addresses; + + let pick = (0..=pool.highest_generated.unwrap_or(0)).find_map(|i| { + pool.addresses.get(&i).and_then(|info| { + if info.used || reserved.contains(&(account_index, i)) { + None + } else { + Some(info.clone()) + } + }) + }); + + let address_info = match pick { + Some(info) => info, + None => { + // No unused-and-unreserved index exists in the + // already-generated range: mint a fresh one strictly + // beyond `highest_generated`. Two cases: + // + // 1. `highest_generated` is `Some(h)` — `address_range(h+1, h+2)` + // generates exactly one new address at index `h+1`. + // 2. `highest_generated` is `None` (pool empty) — + // `address_range`'s `unwrap_or(0)` for the "current + // highest" collides with the "no indices exist" + // state, so a `(0, 1)` range yields an empty vec. + // Fall back to the upstream `next_unused_with_info`, + // which is correct in the empty case (no `!used && + // !reserved` index can exist if no indices exist). + match pool.highest_generated { + Some(h) => { + let next_index = h + 1; + pool.address_range(next_index, next_index + 1, &key_source) + .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?; + pool.info_at_index(next_index).cloned().ok_or_else(|| { + PlatformWalletError::AddressSync(format!( + "Failed to retrieve generated address info at index {next_index}" + )) + })? + } + None => pool + .next_unused_with_info(&key_source, true) + .map_err(|e| PlatformWalletError::AddressSync(e.to_string()))?, + } + } + }; + + // INTENTIONAL(CMT-001 / #3658 review): the reservation is + // in-memory only by design. A crash before the next sync AND + // no payment received by restart manifests as address-reuse + // (a privacy/accounting nuisance, not fund loss). Addresses + // requested but never paid to are freed for reuse on the next + // session, avoiding unbounded index-gap growth from + // speculative or abandoned hand-outs. The eager + // `mark_index_used` flip was removed at QuantumExplorer's + // request because it advanced `highest_used` permanently — + // see #3658 review thread. + self.reservations + .reserve(vec![(account_index, address_info.index)]) + .leak_until_sync(); let address = address_info.address; PlatformAddress::try_from(address).map_err(|e| { @@ -410,8 +467,10 @@ mod found_026_tests { /// addresses. Pre-fix, `next_unused` re-hands index 0 (its `used` /// flag only flips on a positive synced balance) → identical /// addresses → this assertion fails. Post-fix the first call - /// reserves index 0 via `mark_index_used`, so the second yields - /// index 1. + /// reserves index 0 in the in-memory `AddressReservations` set, + /// so the second call skips it and yields index 1. The upstream + /// pool's `highest_used` is left untouched until a real payment + /// lands — see #3658 review thread. #[tokio::test] async fn found_026_back_to_back_handout_returns_distinct_addresses() { let wallet = wallet_with_platform_account(); @@ -430,4 +489,62 @@ mod found_026_tests { "back-to-back hand-out with no sync re-handed the same address (Found-026)" ); } + + /// `next_unused_receive_address` must not advance `highest_used` + /// in the upstream `AddressPool` — that was @QuantumExplorer's + /// review concern on #3658. The reservation should be in-memory + /// only (gap-limit accounting stays anchored to actually-paid + /// addresses). + #[tokio::test] + async fn next_unused_receive_address_does_not_advance_highest_used() { + let wallet = wallet_with_platform_account(); + + let _a = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("first hand-out"); + let _b = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("second hand-out"); + + let wm = wallet.wallet_manager.read().await; + let info = wm.get_wallet_info(&wallet.wallet_id).expect("wallet info"); + let account = info + .core_wallet + .platform_payment_managed_account_at_index(ACCOUNT_KEY.account) + .expect("account"); + assert_eq!( + account.addresses.highest_used, None, + "hand-out must not advance `highest_used` — reservations are in-memory only" + ); + } + + /// Three consecutive hand-outs (no sync, no payment) must yield + /// three distinct indices. Pre-fix with `mark_index_used` removed, + /// a naive "scan for first `!used`" would re-hand index 0 because + /// `used` never flips. The `AddressReservations` filter must skip + /// reserved-but-not-used slots to maintain the distinct-handout + /// invariant across more than two calls. + #[tokio::test] + async fn handout_skips_reserved_but_not_used_indices() { + let wallet = wallet_with_platform_account(); + + let a = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("first hand-out"); + let b = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("second hand-out"); + let c = wallet + .next_unused_receive_address(ACCOUNT_KEY) + .await + .expect("third hand-out"); + + assert_ne!(a, b, "1st vs 2nd hand-out collided"); + assert_ne!(a, c, "1st vs 3rd hand-out collided"); + assert_ne!(b, c, "2nd vs 3rd hand-out collided"); + } }