feat(key-wallet): atomic multi-account UTXO reservations (set_funding_multi)#912
feat(key-wallet): atomic multi-account UTXO reservations (set_funding_multi)#912bfoss765 wants to merge 3 commits into
Conversation
Add `TransactionBuilder::set_funding_multi`, a funding entry that draws a union of UTXOs from several accounts and reserves each selected input in the ledger of the account that *owns* it — not in one shared set. This fixes the reservation defect behind the platform asset-lock union builder, where extra-account inputs were reserved in the primary account's ledger, letting a build issued through the owning account reselect the same coin before broadcast reconciliation. Internals: the builder's `reservations` field becomes a private `Reservations` enum (`None` / `Single` / `Multi`). The single-account `set_funding` path is byte-for-byte unchanged (`Single`: every selected input reserved in the one set). `Multi` groups the selected inputs by owning account and commits them all in one synchronous, infallible step at assembly time, only after coin selection has fully succeeded. Because `ReservationSet::reserve` cannot fail and the commit yields to no other task, the union either reserves entirely or (on an earlier selection error) not at all — no window in which a subset is reserved and a concurrent selector grabs the rest. A signing failure after the commit releases every group, so no owning account is left with a stranded reservation. Per-account release and TTL sweep need no new machinery: each input lives in its owning account's existing set, so `update_utxos` / `release_reservation` / the TTL backstop already reclaim it in the right ledger. Deviates from the design note's `ReservationTxn`/`stage`/`ReservationConflict` sketch: since `reserve` is infallible and cross-account atomicity is already provided by the wallet-manager lock the single-account path documents and relies on, that staging/rollback surface is unnecessary. This keeps the new public API to a single method. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add tests for `set_funding_multi`: - each selected input is reserved in its owning account's ledger, across a three-account union drain; - an account whose coin is not selected reserves nothing (group filter); - an under-funded union reserves nothing anywhere (fail before commit); - a signing failure rolls back every account's group; - per-account `release_reservation` and the TTL backstop each act only on the owning ledger, needing no shared machinery; - two builds racing over the same union under a shared lock never double-select (Arc<Mutex> + thread::spawn, matching the crate's concurrency-test idiom). Expose `RESERVATION_TTL_BLOCKS` as `pub(crate)` so the TTL test can assert the exact reclaim boundary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…uard duplicate primary Round-2 review follow-ups on set_funding_multi: - extras are only read (utxos/reservations, both &self); the only mutation is primary.next_change_address. Change `extra` from `IntoIterator<Item = &mut ManagedCoreFundsAccount>` to `Item = &ManagedCoreFundsAccount`. The platform call site clones/borrows extras under an immutable all_funding_accounts() borrow and takes &mut only on the primary; shared refs make adoption a clean single-call swap instead of forcing N simultaneous exclusive borrows that also exclude the primary. - guard the documented "primary must not appear in extra" precondition: with shared-ref extras, compare identity via std::ptr::eq and skip the duplicate so it cannot be pushed and reserved twice; debug_assert!(false, ...) plus a tracing::warn! matches the crate's existing "shouldn't happen" idiom (the builder is fluent/returns Self, so a Result variant would break the chain). Add set_funding_multi_rejects_primary_listed_in_extra (should_panic). Update the doc comment and every test call site. Full suites green: key-wallet lib 555, key-wallet-ffi 233, key-wallet-manager 48+7+5; clippy+fmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
|
I don't think we would ever want this, as it goes against how SPV wallet accounts should work. Generally you want a barrier against accounts being linked together. |
|
Closing in favor of the single-account approach. #912 added atomic multi-account UTXO reservations ( No consumer depends on |
2 participants
What
Adds
TransactionBuilder::set_funding_multi, a funding entry that draws a unionof UTXOs from several accounts and reserves each selected input in the ledger
of the account that owns it, rather than pooling every reservation into one
account's set.
Why
This is the upstream fix for the Dash Platform #4184 reservation blocker.
The platform asset-lock "fund from all accounts" builder seeds a primary BIP44
account via
set_fundingand appends other accounts' UTXOs viaadd_inputs.Today the builder snapshots a single account's
ReservationSetand reservesall selected inputs there — so a CoinJoin/DashPay input gets reserved in the
primary account's ledger, and a later build issued through the owning
account can reselect that same coin before broadcast reconciliation releases it.
ReservationSetispub(crate), so the platform crate cannot fix this itself;the safe fix belongs here. Platform will adopt
set_funding_multi(replacingits
set_funding(primary) + add_inputs(extra)composition) at the nextkey-wallet pin bump.
How
reservationsfield becomes a privateReservationsenum(
None/Single/Multi).Singleis the existingset_fundingpath,byte-for-byte unchanged.
Multicarries one ledger per funding account ({ ReservationSet, owned outpoints }). At assembly the selected inputs are grouped by owner andcommitted in one synchronous, infallible step — only after coin selection has
fully succeeded.
primaryis&mut(its change address is derived here — the solemutation);
extrais anIntoIteratorof shared&ManagedCoreFundsAccount,since extras are only read. Shared refs keep adoption a clean single call: the
platform site holds the primary by
&mutand passes the rest by&straightout of its
all_funding_accounts()borrow, rather than partitioning thatcollection into N exclusive borrows that each also exclude the primary. The
documented "
primarymust not appear inextra" precondition is enforced byan identity check (
std::ptr::eq) that skips a duplicate so it can't bereserved twice, with a
debug_assertto catch the caller bug in debug builds(a
Resultreturn was avoided so the fluent builder chain is preserved).ReservationSet::reservecannot fail and the commit yields to noother task, so the union reserves entirely or (on an earlier selection error)
not at all. A signing failure releases every group, so no owning account is
left with a stranded reservation.
input lives in its owning account's existing set, so the release in
update_utxos(adjacent to the spent-outpoint tracking from fix(key-wallet): out-of-order UTXO spend recorded nowhere — phantom unspent balance (#649) #909) and theTTL sweep already reclaim it in the right ledger.
Deliberately not ported from the internal design note: a public
ReservationTxn/stage/commit/ReservationConflictstaging API. Sincereserveis infallible and cross-account atomicity is already provided by thewallet-manager lock the single-account path documents, that surface is
unnecessary — this keeps the new public API to a single method.
Known limitation (parity with the single-account path)
If the caller panics during signing (rather than returning an
Err), thealready-committed reservations are not released and the coins stay reserved
until the 24-block (
RESERVATION_TTL_BLOCKS) TTL backstop reclaims them. Thisis identical to the existing single-account
set_fundingpath — noDrop-basedpanic guard is added here, so behaviour stays on par with it.
Tests
New: atomic commit across 2–3 accounts, group filter, insufficient-funds
reserves-nothing, signing-failure rollback across all ledgers, per-account
release + TTL, a threaded no-double-select race, and a duplicate-primary guard
check. Existing single-account builder tests unchanged and green.
cargo test/clippy/fmtclean forkey-wallet,key-wallet-ffi,key-wallet-manager.