fix(key-wallet): reserve selected UTXOs to prevent double-spend

[xdustinface]

A freshly built transaction is not reflected in the UTXO set until its broadcast is processed back into the wallet, so a second build in that window can select the same coins and produce a double-spend the network rejects.

• Add an ephemeral ReservationSet on ManagedCoreFundsAccount: set_funding skips reserved outpoints during coin selection and assemble_unsigned reserves the inputs it selects.
• Release the reservation when the spend is processed, and on a failed sign, so coin selection is unaffected by a build that never reaches the network.
• Reclaim stale reservations via a 100-block TTL backstop, skipping the sweep at height 0 where elapsed time is unknown.
• Share the set via Arc<Mutex<_>> so a reservation outlives the wallet lock, and never persist it: after a restart, chain and mempool sync are the source of truth.
• Cover the read, write, release, TTL, and height-0 paths with unit tests, plus a dashd integration test asserting two pre-broadcast builds select disjoint inputs.

Summary by CodeRabbit

Bug Fixes

• Fixed potential double-spending when creating multiple transactions concurrently by reserving chosen UTXOs during transaction creation so separate builds don’t select the same inputs.
• Reserved UTXOs are now released when a transaction is processed, and also when a transaction build/signing fails (or after the reservation TTL).

Tests

• Added regression coverage for concurrent transaction builds to ensure inputs don’t overlap, plus unit/integration tests verifying reservation non-persistence across restarts and proper reservation release on spend processing and failure.

[coderabbitai]

Warning

Review limit reached

@xdustinface, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 59 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bafd7c0b-bc2f-41dd-bdaf-2b640c667929

📥 Commits

Reviewing files that changed from the base of the PR and between e9ff904 and 97e8acd.

📒 Files selected for processing (6)

• dash-spv/tests/dashd_sync/tests_transaction.rs
• key-wallet/src/managed_account/managed_core_funds_account.rs
• key-wallet/src/managed_account/mod.rs
• key-wallet/src/managed_account/reservation.rs
• key-wallet/src/tests/spent_outpoints_tests.rs
• key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs

📝 Walkthrough

Walkthrough

Introduces a ReservationSet primitive that tracks ephemeral UTXO locks keyed by block height with a TTL backstop. This is wired into ManagedCoreFundsAccount (new field, serde-skipped, released on spend) and TransactionBuilder (filters reserved UTXOs during coin selection, reserves selected inputs after assembly, releases on signing or encoding failure). Unit, integration, and end-to-end tests validate the full lifecycle.

Changes

UTXO Reservation System

Layer / File(s)	Summary
ReservationSet: TTL-based UTXO lock primitive key-wallet/src/managed_account/mod.rs, key-wallet/src/managed_account/reservation.rs	Declares the reservation submodule and implements ReservationSet backed by Arc<Mutex<HashMap<OutPoint, u32>>> with reserve, reserved, and release operations, TTL-based sweep (disabled at height 0), poison-recovery on mutex lock, and comprehensive unit tests covering all edge cases.
ManagedCoreFundsAccount: reservation field and lifecycle key-wallet/src/managed_account/managed_core_funds_account.rs	Adds reservations: ReservationSet (serde-skipped, defaulted in new, wrap, and deserialization), a pub(crate) reservations() accessor, a pub release_reservation(tx) method, and a reservations.release(...) call in update_utxos when a spend is processed.
TransactionBuilder: reservation-aware coin selection and signing key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs	Adds reservations: Option<ReservationSet> field, filters already-reserved outpoints in set_funding, calls reserve after coin selection in assemble_unsigned, and releases reservations immediately on signing or encoded-size computation failure in build_unsigned and build_signed.
Tests: unit, integration, and end-to-end key-wallet/src/tests/spent_outpoints_tests.rs, key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs, dash-spv/tests/dashd_sync/tests_transaction.rs	Tests verify serde excludes reservations and deserialization clears state, spend processing releases reservations, set_funding skips reserved UTXOs, signing/encoding failures release reservations, build_unsigned marks inputs reserved, and an end-to-end SPV test asserts two concurrent builds produce non-overlapping inputs.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related issues

• Found-022: TransactionBuilder::set_funding mutates funds account before build_signed can fail — phantom monitor_revision bumps on failed builds #764: This PR implements the UTXO input reservation infrastructure referenced by the issue to prevent concurrent transaction builds from double-spending the same UTXOs.

Suggested reviewers

• ZocoLini

Poem

🐇 Hop, hop — no UTXO shared twice,
The reservation set keeps things nice.
Two builds race, but the second one sees
The first one's coins are locked with ease.
TTL sweeps the stale away,
Double-spend foiled — hooray, hooray! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'fix(key-wallet): reserve selected UTXOs to prevent double-spend' directly and concisely summarizes the main change—introducing a reservation mechanism to prevent double-spends from concurrent transaction builds.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/concurrent-utxo-reservation

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 94.81865% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.25%. Comparing base (03096dd) to head (97e8acd).

Files with missing lines	Patch %	Lines
.../wallet/managed_wallet_info/transaction_builder.rs	90.09%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #812      +/-   ##
==========================================
+ Coverage   73.16%   73.25%   +0.08%     
==========================================
  Files         323      324       +1     
  Lines       72438    72622     +184     
==========================================
+ Hits        52999    53197     +198     
+ Misses      19439    19425      -14     

Flag	Coverage Δ
core	76.94% <ø> (ø)
ffi	47.71% <ø> (+0.01%)	⬆️
rpc	20.00% <ø> (ø)
spv	90.43% <ø> (+0.10%)	⬆️
wallet	72.01% <94.81%> (+0.23%)	⬆️

Files with missing lines	Coverage Δ
.../src/managed_account/managed_core_funds_account.rs	77.30% <100.00%> (+0.58%)	⬆️
key-wallet/src/managed_account/reservation.rs	100.00% <100.00%> (ø)
.../wallet/managed_wallet_info/transaction_builder.rs	85.17% <90.09%> (+1.69%)	⬆️

... and 5 files with indirect coverage changes

[ZocoLini]

I find this a little confusing and overkill, why don't we use a static HashMap in the transaction_builder module and we only update that, also prevents a user from creating twice the same wallet and falling in the same issue when building transactions.

At the same time, I don't see how this would avoid reserving utxo used in a tx that the user decided not to broadcast, maybe I am missing it :/

[ZocoLini]

I find this a little confusing and overkill, why don't we use a static HashMap in the transaction_builder module and we only update that, also prevents a user from creating twice the same wallet and falling in the same issue when building transactions.

At the same time, I don't see how this would avoid reserving utxo used in a tx that the user decided not to broadcast, maybe I am missing it :/

[xdustinface]

I find this a little confusing and overkill, why don't we use a static HashMap in the transaction_builder module and we only update that, also prevents a user from creating twice the same wallet and falling in the same issue when building transactions.

At the same time, I don't see how this would avoid reserving utxo used in a tax that the user decided not to broadcast, maybe I am missing it :/

I added a function the that consumers can release it manually. About the static HashMap. Im not sure if the hazard of global state with mixed networks is worth it to fight an edge case of using two of the same wallets in one process?

[ZocoLini]

I don't like static stuff either, but this feels hard to use, what about a empty Rc field in an UTXO that only increases when a Transaction holds it, something like that, Maybe a wrappers around UTXO that tracks if they are being hold in a transaction with a Rc field, something that drops atomically when the Transaction is freed

[xdustinface]

I don't like static stuff either, but this feels hard to use, what about a empty Rc field in an UTXO that only increases when a Transaction holds it, something like that, Maybe a wrappers around UTXO that tracks if they are being hold in a transaction with a Rc field, something that drops atomically when the Transaction is freed

Im not sure what issue you are trying to solve here and what is hard to use. And im also not quite sure how you envision this to be implemented to be honest. Feel free to come up with another PR for this if you have strong feelings about this.

[github-actions]

This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them.