fix(platform-wallet): atomic multi-pool address reservations (receive + core BIP44 receive/change) + change-loop refactor#3770
Draft
Claudius-Maginificent wants to merge 8 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>
6 tasks
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
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
HD address hand-out in
platform-walletsuffers a two-state race. The upstreamkey-walletAddressPoolmodels only unused → used, andusedflips only when a positive-balance sync proves the address received funds. Sonext_unused-style hand-out returns the same derivation index to two concurrent callers until a payment lands — address reuse (a privacy + accounting hazard).This PR closes that race across every affected pool, generalizing the receive-address "Reserved" bridge to a multi-pool model:
CW-001.CW-002, which additionally collides with the in-flightsend_to_addresseschange-address peeks tracked inpending_change.It also extracts the duplicated change-address selection loop (Phase 3) and folds in validated review feedback.
Note
Stacked on #3585 (base branch
fix/dpns-case-and-utxo-race-v3.1-dev). Review/merge after #3585. The receive-address + multi-pool reservation work that earlier drafts of #3585 carried was moved here.PA-001 (asset-lock funding) — verified PHANTOM, no change
The suspected asset-lock funding-address reuse race does not exist.
build_asset_lock_transactionholdswallet_manager.write().awaitacross the entire build — the funding-address peek (peek_next_funding_address→next_address(.., false)) and the builder's used-flip (next_private_key→mark_index_used) run under the same write guard. Two concurrent builds are fully serialized: the second peeks only after the first has marked its index used. No funding-pool reservation is required;asset_lock/build.rsis untouched.What was done?
Ephemeral "Reserved" bridge, generalized to multiple pools (
platform_addresses/address_reserve.rs)Mutex-guarded reservation table supplies the missing reserved layer between unused and used, consulted/updated atomically while the caller holds the wallet write lock.(wallet_id, PoolKind, account), wherePoolKind ∈ {PlatformReceive, CoreReceive, CoreChange}keeps each pool's index space disjoint.CW-001 — core BIP-44 receive + change reservation (
core/wallet.rs,platform_addresses/{mod,provider,wallet}.rs)next_receive_address_for_account/next_change_address_for_account(+ blocking twins, behind FFIcore_wallet_next_receive_address/core_wallet_next_change_address) route throughCoreWallet::reserve_bip44_address, reaching the external/internal pool viamanaged_account_type_mut().address_pools_mut()[0|1]and picking through the bridge.CW-002 — change consults in-flight send reservations (
core/wallet.rs)OutpointReservations.pending_changebefore a send confirms. The standalone change hand-out now consults that snapshot and retries; each rejected index stays bridge-reserved, so it converges to the first address that is neither bridge-reserved nor pending. The receive path uses an empty avoid-set.Phase 3 — readability refactor (
core/change_address.rsnew,core/broadcast.rs,identity/network/payments.rs)send_to_addressesandsend_paymentis extracted into onecore::change_address::pick_and_reserve_change_addresshelper. Behavior is identical (samepending_changeconsultation, peek/commit ordering, retry, error mapping, lock scope).Review-feedback fixes
sweep_expired(RESERVATION_TTL)is now called fromPlatformPaymentAddressProvider::sync_finished(the natural per-completed-sync seam), reclaiming abandoned hand-outs across all pool kinds.RESERVATION_TTL(300s) shared fromaddress_reserve.tracing; it now reportsleaked_reservations(count of slots pinned until restart) via newreserved_count()guard accessors.core/reservationsRAII drop-release ⇄platform_addresses/address_reserveglobal TTL-sweep) in each module header.Upstream migration
This whole bridge is a deliberate stopgap. The proper home for the tri-state unused → reserved → used lifecycle is inside
key-wallet'sAddressPool— tracked at dashpay/rust-dashcore#791. Thenext_unused_and_reservesignature is intentionally shaped so the eventual swap is a one-liner with no call-site changes.How Has This Been Tested?
cargo test -p platform-wallet --all-features: 219 lib tests pass, 0 failed, 3 ignored (the 3 are pre-existing#[ignore]heavy variants).concurrent_reserve_no_duplicates(+ a#[ignore]d 10k variant) proves distinct hand-outs under parallel load;concurrent_same_utxo_sends_resolve_via_reservation_setproves the loser short-circuits withNoSpendableInputs.distinct_pool_kinds_do_not_collide(pool-kind index-space disjointness),next_change_address_skips_pending_change_reservation(CW-002), plus the existing distinct/reserved-skip/release/TTL suite re-parameterized overPoolKind.send_to_addresses_*andbroadcast_and_reconcile_*pass unchanged.cargo check -p platform-wallet-ffiandcargo check -p dash-sdk: rc=0.cargo clippy -p platform-wallet --all-features --tests: 0 warnings.cargo fmt --check: clean.pa_005/pa_008b/id_002/id_005/al_001) live on test(platform-wallet): e2e framework + full test suite — triage pins, Found-*/PA-* guards, fail-closed persist, Stage-2 merge #3549 / test(platform-wallet): shielded (Orchard) e2e suite — spec + Wave H harness #3727.Breaking Changes
None. No public API or serialized/persisted form changed; reservations are ephemeral (in-memory, rebuilt empty on restart).
Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent