fix(platform-wallet): prevent duplicate HD address hand-out with an ephemeral reservation bridge#3770
Closed
Claudius-Maginificent wants to merge 13 commits into
Conversation
…ss hand-out race next_unused_receive_address called the upstream two-state pool (AddressPool::next_unused, Unused -> Used). used only flips on a positive-balance sync, so two concurrent callers both saw the same index as unused and were handed the SAME address. Add a platform-local Reserved layer as a thin, self-contained bridge behind ONE function, next_unused_and_reserve, whose signature mirrors the intended upstream AddressPool::next_unused_and_reserve so the future swap is a one-liner with no call-site churn. - Reserved indices live in a process-global Mutex-guarded side table, keyed by (wallet_id, account). Ephemeral: never persisted, rebuilt empty on restart, absent from every serde/bincode form. - Pick-and-reserve is one critical section under the table Mutex (and the wallet write lock at the call site) -- no TOCTOU gap. - The chosen index is materialized via the pool's public generator, so reserved-but-unused indices count toward the gap-limit scan window (they become pending addresses the BLAST sync covers); the ceiling is computed upstream from the materialized pool, so no local max(highest_used, highest_reserved) patch is needed. - on_address_found releases the reservation once an address is proven used, completing Unused -> Reserved -> Used. - TTL reclaim: sweep_expired + PlatformAddressWallet::sweep_expired_reservations (default 1h) free stale reservations in long-lived processes. Tests (Found-026 adapted + mandatory concurrency stress): - back-to-back hand-outs distinct; reserved index skipped while pool still reports unused; on-use clears reservation; hand-out advances highest_reserved while highest_used does NOT advance until a sync hit. - 1000-task always-on concurrency test + 10_000-task #[ignore] variant: every task gets a pairwise-distinct address (verified 10k -> 10k). - persisted-form invariant: first hand-out picks the same index as upstream next_unused, so the serialized pool is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rust-dashcore#791); set reservation TTL to 5m The process-global reserved table is a deliberate bridge until key-wallet gains a native Reserved tri-state in its AddressPool. Document that intent inline and at fn table(), referencing the upstream feature request dashpay/rust-dashcore#791 so the future swap to a one-line pool.next_unused_and_reserve(..) delegation is obvious. Tighten RESERVATION_TTL from 1h to 5m so abandoned receive-address hand-outs reclaim their gap-limit slot faster. The set stays in-memory only (rebuilt empty on restart, never serialized), so this only bounds leakage within a single long-lived process. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ation (CW-001) Core BIP44 address hand-out (`next_receive_address_for_account`, `next_change_address_for_account`, their blocking twins, and FFI `core_wallet_next_receive_address` / `core_wallet_next_change_address`) called `next_receive_address(.., true)` / `next_change_address(.., true)`, which delegate to `AddressPool::next_unused`. That only flips `used` on a positive-balance sync, so two concurrent callers were handed the SAME index -- the same two-state race the platform-address bridge already fixes. Route both pools through the ephemeral Reserved bridge, keyed by a new `PoolKind` discriminant so the BIP44 external and internal pools each keep a disjoint index space from the DIP-17 platform-payment pool and from each other. Reservations are in-memory only and released by the 5-minute TTL sweep; once an index is actually paid the pool's own `used` flag keeps it out of future hand-out, so a lingering reservation is harmless until swept. - Generalize `address_reserve` to key by `(wallet_id, PoolKind, account)`; `PlatformReceive` preserves the existing platform-address behavior. - Add `CoreWallet::reserve_bip44_address` reaching the external/internal pool via `managed_account_type_mut().address_pools_mut()[0|1]`. - BRIDGE/TODO(upstream) -> rust-dashcore#791; ephemeral, no serialized form changed (no #3625 schema impact). Tests: pool-kind disjointness, plus the existing distinct/reserved-skip/ release/TTL/concurrency suite now parameterized over `PoolKind` (13 ok). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…adability (no behavior change) The change-address peek/retry/commit loop was duplicated verbatim in `core/broadcast.rs::send_to_addresses` and `identity/network/payments.rs::send_payment`. Extract it into one `core::change_address::pick_and_reserve_change_address` helper both call sites use. Behavior is identical: same `pending_change` snapshot consultation, same peek(advance=false) / commit(advance=true) ordering, same retry-on-pending, same `TransactionBuild` error mapping, same wallet-write-lock scope (the caller still holds it and still records the returned address into `pending_change` via the outpoint reservation). The advance commits exactly once per iteration -- the prior code did the same in both its break and burn branches, just written twice. Proof: existing `concurrent_callers_get_no_spendable_inputs`, `send_to_addresses_reserves_change_address`, `send_to_addresses_burns_change_index_when_reserved`, and the change-reservation tests pass unchanged (283 passed, 0 failed -- same count as before the refactor). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the `pub(crate) mod change_address;` declaration that the readability refactor's extracted helper needs. Folds into the preceding refactor commit logically; kept as a follow-up because history rewriting is disabled in this environment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `next_change_address_skips_pending_change_reservation`: peek the change address an in-flight `send_to_addresses` would choose, mark it pending in `OutpointReservations.pending_change`, then assert a standalone `next_change_address_for_account` hand-out returns a DIFFERENT address. Locks in the CW-002 avoid-loop behaviour. The CW-002 production change (the `pending_change` avoid-loop in `reserve_bip44_address`) shipped folded into the CW-001 commit because history rewrite is disabled in this environment; this commit adds the dedicated regression test for it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…TL sweep, observe leak_until_sync, cross-ref reservation impls SEC-001: the ephemeral address-reservation TTL sweep was implemented and tested but nothing drove it at runtime, so abandoned receive/change/CW reservations pinned gap-limit slots until process restart. Drive `sweep_expired(RESERVATION_TTL)` from the address-sync `sync_finished` seam — a natural per-completed-sync periodic point that already runs in the provider — reclaiming stale hand-outs across ALL pool kinds. Addresses proven used are still released eagerly in `on_address_found`; this only frees never-paid hand-outs. `RESERVATION_TTL` moves to `address_reserve` (next to `sweep_expired`) so both the provider driver and the existing host-facing `sweep_expired_reservations` share one constant. SEC-002: the success-but-unreconciled broadcast branch leaks the outpoint /change reservation via `leak_until_sync` (no in-process reclaim). Both leak `tracing::warn!` events already explained why; add a `leaked_reservations` count (new `reserved_count()` on the guards) so the leak is observable — how many slots are pinned until restart. PROJ-001: cross-reference the two reservation models — `core/reservations` (RAII drop-release) and `platform_addresses/address_reserve` (global TTL-sweep) — in each module header so a future change to one reclaim model isn't forgotten in the other. No behavior change beyond the now-driven sweep; TTL value (300s) and all reservation semantics unchanged. 219 tests pass; clippy/fmt clean; platform-wallet-ffi + dash-sdk check rc=0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…L const, reserved_count, sweep driver) The preceding review-feedback commit shipped partial — several edits did not land, leaving the tree non-compiling (unresolved RESERVATION_TTL import, missing ReservationGuard::reserved_count). Complete the wiring: - SEC-001: add `pub(crate) const RESERVATION_TTL` to `address_reserve` (the shared home `wallet.rs` already imports) and drive `sweep_expired(RESERVATION_TTL)` from `PlatformPaymentAddressProvider::sync_finished` — the natural per-completed-sync periodic seam — reclaiming stale hand-outs across all pool kinds. - SEC-002: add `ReservationGuard::reserved_count` so the broadcast leak warn! can report `leaked_reservations` (how many slots are pinned until restart). - PROJ-001: add the `address_reserve` side of the two-models cross-ref. Restores green: platform-wallet 219 passed/0 failed/3 ignored; clippy --all-features --tests 0 warnings; fmt clean; platform-wallet-ffi + dash-sdk check rc=0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claudius-Maginificent
changed the title
6 tasks
# Conflicts: # packages/rs-platform-wallet/src/wallet/core/broadcast.rs
…committed follow-up Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ning reserve After merging the reserve-exclusivity change, OutpointReservations::reserve returns Option. Unwrap it at the change-handout test's pending-mark site so the live guard is held for the duration of the assertion, matching the other reserve call sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claudius-Maginificent
changed the title
Contributor
|
We have decided to implement this on rust-dashcore. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
key-wallet'sAddressPooltracks onlyunused → used, and an address flips tousedonly once a payment to it is observed on-chain. Concurrent — or rapid back-to-back — "next unused address" requests therefore hand out the same derivation index, causing address reuse (a privacy leak) and receive/change collisions.N→ funds land on a reused address; or a send's change index collides with a concurrently handed-out receive address.OutpointReservationsUTXO guard — the BIP-44 change pool skips any index whose outpoints are reserved by an in-flight (unconfirmed) spend. Review and merge fix(platform-wallet): prevent concurrent UTXO double-spend via outpoint reservations #3585 first.What was done?
An ephemeral reservation layer that makes concurrent HD address hand-out mutually exclusive:
wallet/platform_addresses/address_reserve.rs): adds an intermediate state (unused → reserved → used) in a process-global, mutex-guarded table reclaimed by a 5-minute TTL sweep. Concurrent hand-out skips reserved indices; reservations that are never consumed expire and are swept. This is a deliberate stopgap untilkey-walletgains a nativeReservedstate upstream (tracked atdashpay/rust-dashcore#791).platform_addresses/provider.rs,platform_addresses/wallet.rs).OutpointReservations.wallet/core/change_address.rs) used by the core broadcast path (wallet/core/wallet.rs,wallet/core/broadcast.rs).How Has This Been Tested?
cargo build -p dash-sdk -p platform-wallet -p platform-wallet-ffi— clean.address_reserve.rs(14 tests), receive-pool reservation inprovider.rs(5 tests), and the change-pool skip of indices with in-flight reserved outpoints.Breaking Changes
None. The reservation layer is an additive in-memory mechanism; no public call signatures change.
Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent