feat(key-wallet): owner-tagged reservations to close the broadcast-release TOCTOU (platform#4185)#916
Open
bfoss765 wants to merge 3 commits into
Open
feat(key-wallet): owner-tagged reservations to close the broadcast-release TOCTOU (platform#4185)#916bfoss765 wants to merge 3 commits into
bfoss765 wants to merge 3 commits into
Conversation
…se the broadcast-release TOCTOU (platform#4185) ReservationSet previously mapped OutPoint -> reserved-at-height with no per-reservation owner, and released unconditionally by outpoint. The platform broadcast path (dashpay/platform#4185) reserves an asset-lock/deferred send's inputs, awaits the broadcast, and on a Rejected result releases them by outpoint. If key-wallet's TTL sweep reclaims that reservation mid-await and a different concurrent build re-reserves the same outpoint (same wallet generation, so any Arc::ptr_eq generation guard still passes), the rejected build's cleanup frees the OTHER build's reservation -> a double-spend window. The platform layer cannot detect this because the sweep is invisible inside key-wallet; the "release only if still mine" check must be atomic under the ReservationSet mutex. Stamp every reservation with a ReservationToken (a Copy newtype over a monotonic per-set counter, unique even across same-height reserves). reserve() returns the token it stamped; release_if_owner(outpoints, token) removes only outpoints still owned by that token, atomically under the mutex, and is a no-op for any swept-and-re-reserved outpoint. The unconditional release() is kept for the processed-spend path, where the coin is genuinely spent and must leave the set regardless of owner. Wiring: - assemble_unsigned returns the stamped token; build_signed's sign-failure path now releases owner-guarded (it too awaits, so it shares the hazard). - Additive token-returning builds: build_unsigned_reserved / build_signed_reserved. build_unsigned / build_signed keep their signatures. - Public seam for platform: ManagedCoreFundsAccount::release_reservation_if_owner and AssetLockResult::reservation_token; ReservationToken re-exported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReservation ownership tokens are introduced for UTXO reservations, propagated through reserved transaction and asset-lock builders, and exposed through managed account and crate APIs. Signing-failure cleanup now releases only reservations still owned by the originating build. ChangesReservation ownership flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedWalletInfo
participant TransactionBuilder
participant ReservationSet
participant Signer
ManagedWalletInfo->>TransactionBuilder: build_signed_reserved
TransactionBuilder->>ReservationSet: reserve selected outpoints
ReservationSet-->>TransactionBuilder: ReservationToken
TransactionBuilder->>Signer: sign transaction
Signer-->>TransactionBuilder: signed transaction or failure
TransactionBuilder->>ReservationSet: release_if_owner on failure
TransactionBuilder-->>ManagedWalletInfo: transaction, fee, token
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (1)
219-252: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReservation tokens are captured but never used to guard cleanup on downstream (non-signing) failures. Both asset-lock builders take a reservation via
build_signed_reservedand get backreservation_token, but a failure in the code that runs after the successful signed build (credit-key derivation, or the signer round-trip/bookkeeping) returnsErrwithout releasing it — stranding the already-signed transaction's reserved inputs until the 24-block TTL sweep, since the caller never sees the token to release it itself.
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs#L219-L252: inbuild_asset_lock, clone the funding account'sReservationSetbefore the credit-key derivation loop and callrelease_if_owneron the transaction's outpoints whenresolve_funding_account/next_private_keyfails, before returningErr.key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs#L293-L357: inbuild_asset_lock_with_signer, apply the same guarded-release pattern around the Phase 1–3 bookkeeping loop (resolve_funding_account,peek_next_path,signer.public_key(&path).await,mark_first_pool_index_used).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs` around lines 219 - 252, Add guarded reservation cleanup in key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs:219-252 within build_asset_lock by cloning the funding account’s ReservationSet before credit-key derivation and calling release_if_owner on the signed transaction’s outpoints whenever resolve_funding_account or next_private_key fails. Apply the same pattern in key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs:293-357 within build_asset_lock_with_signer for failures during the Phase 1–3 bookkeeping operations, including resolve_funding_account, peek_next_path, signer.public_key, and mark_first_pool_index_used; preserve successful returns and only release reservations owned by this build.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 219-252: Add guarded reservation cleanup in
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs:219-252 within
build_asset_lock by cloning the funding account’s ReservationSet before
credit-key derivation and calling release_if_owner on the signed transaction’s
outpoints whenever resolve_funding_account or next_private_key fails. Apply the
same pattern in
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs:293-357 within
build_asset_lock_with_signer for failures during the Phase 1–3 bookkeeping
operations, including resolve_funding_account, peek_next_path,
signer.public_key, and mark_first_pool_index_used; preserve successful returns
and only release reservations owned by this build.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 392d56f0-13b2-454e-aea8-f0de66b4c248
📒 Files selected for processing (6)
key-wallet/src/lib.rskey-wallet/src/managed_account/managed_core_funds_account.rskey-wallet/src/managed_account/mod.rskey-wallet/src/managed_account/reservation.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #916 +/- ##
==========================================
- Coverage 74.56% 74.44% -0.12%
==========================================
Files 328 328
Lines 75711 75822 +111
==========================================
- Hits 56451 56444 -7
- Misses 19260 19378 +118
|
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 23, 2026
Point all rust-dashcore crates at bfoss765/rust-dashcore e99959ced0062159d629930f488374e29f63c42b (PR dashpay/rust-dashcore#916), which is v4.1-dev's rust-dashcore tip 70d4bf8 plus the additive owner-tagged reservation API: key_wallet::ReservationToken, ReservationSet::reserve/release_if_owner, TransactionBuilder::build_{unsigned,signed}_reserved, ManagedCoreFundsAccount::release_reservation_if_owner, and AssetLockResult.reservation_token. Additive over 70d4bf8, so it stays compatible with the rest of the v4.1-dev workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tream failures Addresses the CodeRabbit Major (PR dashpay#916, asset_lock_builder.rs :219-252 / :293-357). Both asset-lock builders took a reservation via build_signed_reserved and captured its reservation_token, but a failure in the code that runs AFTER the successful signed build — credit-key derivation in build_asset_lock, and the Phase 1-3 signer/bookkeeping loop in build_asset_lock_with_signer — returned Err without releasing the reservation. The already-signed transaction's inputs then stranded until the 24-block TTL sweep, since the caller never received the token to release them itself. Clone the funding account's ReservationSet (a shared Arc view) before each loop re-borrows self.accounts, and on any failure release only THIS build's reservation with the owner-guarded release_if_owner(&reserved, token) — never the unconditional release, which could free a concurrent build's inputs that the TTL sweep + re-reserve handed over during this build (the very TOCTOU of dashpay/platform#4185 this PR closes). The success path is unchanged: a successful build consumes the reservation as designed, and the signer-failure release inside build_signed_reserved returns via `?` before this new code runs, so there is no double-release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
Why this PR exists
This adds an owner-tagged reservation capability to
key-walletto close a real time-of-check/time-of-use double-spend window that the platform layer (dashpay/platform#4185) cannot fix on its own.The bug it fixes:
ReservationSetcurrently mapsOutPoint → reserved-at-heightwith no per-reservation owner, andreleaseremoves by outpoint unconditionally. Platform's deferred-broadcast / asset-lock path reserves a transaction's inputs,.awaits the network broadcast, and on aRejectedresult releases those inputs by outpoint. If key-wallet's TTL sweep reclaims the reservation during that await and a different concurrent build re-reserves the same outpoint (same wallet generation, so anyArc::ptr_eqgeneration guard still passes), the rejected build's cleanup frees the other build's reservation — opening a double-spend window. Platform cannot detect this, because the sweep happens invisibly inside key-wallet; the "release only if it's still mine" check must be atomic under theReservationSetmutex. That's what this PR provides.What changed
New public type
ReservationToken(re-exported from crate root +managed_account): opaqueCopynewtype over a privateu64, with no public constructor — a token can only be obtained fromreserve(), so it is forge-proof and cheap to hold across an await. Height cannot serve as the identity (it collides across same-height reserves).ReservationSet(stillpub(crate)):reserve(&[OutPoint], height) -> ReservationToken— now returns a fresh per-call token stamping all outpoints; re-reserving transfers ownership to the new token.release_if_owner(&[OutPoint], ReservationToken)(new) — atomically removes only outpoints whose stored owner == the token; a no-op for swept / re-reserved / unreserved outpoints.release(...)(kept, unconditional) — doc now scopes it to the known-spent path only.HashMap<OutPoint, Reservation { reserved_at_height, owner }>+ anext_tokencounter; TTL sweep preserved.Public seam so consumers can adopt the fix (all additive):
TransactionBuilder::build_unsigned_reserved()/build_signed_reserved()→ return the extraOption<ReservationToken>.ManagedCoreFundsAccount::release_reservation_if_owner(&Transaction, ReservationToken)— the owner-guarded release a Rejected path should call instead of the unconditionalrelease_reservation.AssetLockResult.reservation_token: Option<ReservationToken>— carries the token out of the asset-lock build.The sign-failure path in
build_signed(which also awaits) was switched torelease_if_owner. The processed-spend release stays unconditional (the coin is genuinely spent).Back-compat
build_unsigned/build_signedandrelease_reservationkeep their existing signatures (they thin-delegate); the ~20 in-repo callers are untouched.ReservationSet::reserve's changed return ispub(crate), zero external impact; all in-repo sites updated.key-wallet-manager,dash-spv,key-wallet-ffidon't reference reservations; they still compile.Validation
cargo build -p key-wallet -p key-wallet-manager -p dash-spv -p key-wallet-fficlean;cargo test -p key-wallet --lib= 552 passed, 0 failed (13/13 reservation tests incl. the TOCTOU regression: reserve X→token A, simulate sweep + re-reserve X→token B, assertrelease_if_owner([X], A)does NOT free X); clippy + doc clean.Scope note
The asset-lock flow (the one platform#4185 cites) is wired end-to-end. The general manager send wrappers (
build_and_sign_transaction*) still return(Transaction, u64)and drop the token — platform's #4185 adoption can callbuild_signed_reserveddirectly; happy to thread the token through those wrappers too if preferred.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes