feat(key-wallet): owner-tagged reservations to close the broadcast-release TOCTOU (platform#4185)#916
feat(key-wallet): owner-tagged reservations to close the broadcast-release TOCTOU (platform#4185)#916bfoss765 wants to merge 2 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>
|
Warning Review limit reached
Next review available in: 51 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 Plus Run ID: 📒 Files selected for processing (1)
📝 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 Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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}
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.53% -0.04%
==========================================
Files 328 328
Lines 75711 75798 +87
==========================================
+ Hits 56451 56493 +42
- Misses 19260 19305 +45
|
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>
1 participant
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