fix(platform-wallet): age-guard the finalized-transaction handle broadcast - #4309
fix(platform-wallet): age-guard the finalized-transaction handle broadcast#4309bfoss765 wants to merge 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe wallet rejects finalized transaction handles whose reservations reach 20 blocks of age. Abandonment avoids unsafe aged outpoint release. FFI mappings, Kotlin documentation, and cleanup tests cover the stale-reservation behavior. ChangesFinalized transaction reservation expiry
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CoreWallet
participant reservation_expired
participant TransactionBroadcaster
participant abandon_transaction
CoreWallet->>reservation_expired: Check finalized transaction age
reservation_expired-->>CoreWallet: Return stale or usable status
CoreWallet->>TransactionBroadcaster: Broadcast usable finalized transaction
CoreWallet->>abandon_transaction: Abandon stale finalized transaction
abandon_transaction-->>CoreWallet: Apply age-aware reservation cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit f1250e0) |
2a540f5 to
224704f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 50-55: Update the broadcast method around the reservation
validation to acquire generation_payment_guard, verify is_current_generation,
and return the appropriate stale-generation error when the wallet is no longer
current. Hold the guard through the broadcaster call so teardown cannot occur
between validation and network submission, while preserving the existing
reservation_expired check.
In `@packages/rs-platform-wallet/src/wallet/reservations.rs`:
- Around line 57-68: Correct the aged-cleanup documentation to distinguish
token-less reservations from owner-guarded reservations: in
packages/rs-platform-wallet/src/wallet/reservations.rs lines 57-68, state that
only token-less cleanup skips unguarded release while abandon_transaction can
release with an owner token; update the corresponding stale-broadcast and
release descriptions in
packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs lines 163-168,
packages/rs-platform-wallet/src/error.rs lines 103-108,
packages/rs-platform-wallet/src/test_support.rs lines 364-366,
packages/rs-platform-wallet/src/wallet/core/broadcast.rs lines 403-405,
packages/rs-platform-wallet-ffi/src/error.rs lines 276-281,
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
lines 65-71, and packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
lines 389-395 so normal aged finalized handles are documented as owner-guarded
releases and only the token-less branch skips release.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e486da5f-6817-4ca9-a83f-f928619636b5
📒 Files selected for processing (10)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/reservations.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4309 +/- ##
============================================
+ Coverage 84.74% 86.68% +1.94%
============================================
Files 2711 2712 +1
Lines 357138 349627 -7511
============================================
+ Hits 302668 303090 +422
+ Misses 54470 46537 -7933
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The age check correctly prevents stale V2 transactions from reaching the broadcaster, and the new owner-guarded abandon/free behavior safely releases still-owned reservations at any age. However, the terminal FFI stale-broadcast path consumes the only transaction handle without invoking that cleanup, so an immediate rebuild can remain blocked until the reservation TTL expires. Several public comments also still describe the superseded age-based cleanup policy or omit the stale terminal outcome.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:50-55: Use the reservation owner token when stale handles are consumed
The stale branch returns without reconciling the reservation. At the FFI boundary, `core_wallet_broadcast_signed_transaction_v2` has already removed the opaque handle, while Swift and Kotlin also clear their local handles before entering the ABI, so the caller cannot abandon it afterward. Between the 20-block guard and key-wallet's 24-block TTL, the reservation is normally still owned by this finalized build; consequently, the instructed immediate rebuild can fail because the only available input remains reserved. `abandon_transaction` now uses `release_reservation_if_owner` whenever the finalized transaction carries its owner token, safely releasing a still-owned reservation and doing nothing if a sweep or re-reservation transferred ownership. Invoke that cleanup before returning `StaleReservation`. The existing Rust test does not cover the terminal FFI behavior because it explicitly calls `abandon_transaction` after receiving the stale error.
In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:101-108: StaleReservation docs describe the old abandon behavior
These comments say aged abandon/free always skips reservation release, but `CoreWallet::abandon_transaction` now skips only for token-less transactions. A normal funded finalized handle carries an owner token and attempts `release_reservation_if_owner` at every age, releasing inputs only while this build still owns them and safely doing nothing after ownership transfers. The same obsolete policy appears in `wallet/reservations.rs:57-68`, `wallet/signed_payment_registry.rs:163-168`, `test_support.rs:364-366`, `wallet/core/broadcast.rs:403-406`, `rs-platform-wallet-ffi/src/error.rs:269-281`, the FFI test comment at `rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:389-396`, and Kotlin's `ManagedCoreWallet.kt:64-71`. Update these mirrors to distinguish owner-guarded cleanup from the token-less by-outpoint fallback.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:25-34: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction_v2` can return `ErrorStaleReservationToken` code 34 after permanently consuming the opaque handle. This outcome does not touch the broadcaster, does not allocate an output txid string, and cannot be recovered by subsequently calling abandon/free with the consumed handle. The exported C-boundary documentation currently describes success, ambiguous submission, definitive rejection, and removed-wallet failure only. Document code 34 and its handle, network, txid, rebuild, and owner-guarded reservation-cleanup contract consistently with the stale-consumption fix.
…dcast Rebased down to the age-guard onto current v4.2-dev: the #4185/#4308 stack it was riding merged, and #4323/#4325 renamed the finalized- transaction surface (the v2 suffix is gone), so the guard now lands on core_wallet_broadcast_signed_transaction and the slice-based finalize_transaction signature. Mirrors the deferred registry-token age policy on the finalized-handle path: RESERVATION_MAX_AGE_BLOCKS (20; key-wallet TTL 24) and reservation_expired() live in wallet::reservations, shared by both surfaces. broadcast_finalized_transaction refuses with StaleReservation (FFI ErrorStaleReservationToken, 34) before touching the broadcaster once the reservation's stamp height has aged past the bound — and the refusal reconciles the reservation on the way out, exactly like the registry's stale-token branch: the FFI wrapper has already consumed the opaque handle, so no follow-up abandon is possible, and the owner- guarded release (safe at any age; a no-op once ownership transferred) frees the still-owned inputs for the instructed immediate rebuild. Abandon/free likewise release owner-guarded at any age, with the by-outpoint skip retained only for token-less builds. Boundary tests cover both account types on the platform and FFI layers, including the terminal FFI stale-broadcast path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
224704f to
61f871e
Compare
bfoss765
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/rs-platform-wallet/src/error.rs`:
- Around line 121-147: Fix the rustdoc link in
PlatformWalletError::StaleReservation so it does not reference the private
crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS item. Replace that link
with a publicly reachable target, while retaining the existing public
SignedCoreTransaction::reservation_height link and the documented behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 133f9314-a352-40da-9641-f276b3b3b5e3
📒 Files selected for processing (10)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/reservations.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/rs-platform-wallet/src/wallet/reservations.rs
- packages/rs-platform-wallet-ffi/src/error.rs
- packages/rs-platform-wallet/src/wallet/core/transaction.rs
- packages/rs-platform-wallet/src/test_support.rs
- packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
- packages/rs-platform-wallet/src/wallet/core/broadcast.rs
…a public doc Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The stale-handle path now performs owner-guarded cleanup and has strong terminal-path coverage, but the freshness check can still race a multi-block height advance and reservation reassignment before network dispatch. The exported C documentation omits the stale terminal outcome, and Kotlin promises a typed stale error without translating the JNI exception on its public direct broadcast method.
Source: reviewers gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:55-62: Keep the reservation valid until broadcast dispatch
`last_processed_height()` releases the wallet-manager read lock before the broadcaster reaches network dispatch. The FFI lifecycle guard excludes wallet teardown, but it does not exclude sync updates or concurrent finalization because payment guards are shared. A call can therefore sample the reservation at age 19, yield in the broadcaster while catch-up advances the wallet to age 24, and then race a new finalization that triggers key-wallet's TTL sweep and reserves the same input under a new token. The old signed transaction can subsequently be submitted against that reassigned UTXO. The four-block margin reduces ordinary likelihood but does not establish an ordering invariant because catch-up can advance multiple blocks. Atomically validate ownership and pin or mark the reservation as in-broadcast under the same synchronization used by height advancement and coin selection, keeping that state until dispatch has definitively begun. The registry-token broadcast uses the same check-then-dispatch pattern and should use the same primitive.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but its exported contract still lists only ordinary broadcast and removed-wallet outcomes. On the stale branch, Rust has already consumed the opaque handle, leaves `out_txid` null, never invokes the broadcaster, and performs owner-guarded reservation cleanup so the caller can rebuild immediately. Native callers need these terminal ownership and recovery semantics explicitly documented; retrying, abandoning, or freeing the consumed handle is not valid.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt:45-49: Translate the stale JNI error promised by the Kotlin API
The public method documents that stale broadcast throws `DashSdkError.PlatformWallet.StaleReservationToken`, but it invokes the external JNI method directly. JNI turns native code 34 into the internal `DashSDKException`; without `mapNativeErrors`, direct callers of `coreWallet().broadcastTransaction(...)` receive that internal exception rather than the documented public type. `sendToAddresses` happens to wrap this call from outside, but `coreWallet()` and `broadcastTransaction` are themselves public, so that outer wrapper is not an API-wide invariant.
…roadcastTransaction The method documents DashSdkError.PlatformWallet.StaleReservationToken but called the JNI native directly, so direct callers received the internal DashSDKException instead of the documented public type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pre-checked age is not an ordering invariant: between the check and the broadcaster await, sync catch-up can advance last_processed_height past the bound and a concurrent finalization can trigger key-wallet's TTL sweep, re-reserving the same inputs under a new token — the old signed transaction then hits the wire against reassigned UTXOs. New shared primitive dispatch_unexpired performs the age check and reaches the broadcaster under ONE wallet-manager READ guard. Both writers this orders against — the ReservationSet TTL sweep (inside coin selection) and height advancement — mutate under the manager WRITE lock, so 'the reservation is unexpired' and 'dispatch has begun' become a single atomic observation. Ownership needs no separate probe: the key-wallet TTL exceeds RESERVATION_MAX_AGE_BLOCKS on the same clock, so an unexpired reservation cannot already have been swept. Both check-then-dispatch sites now route through it: the finalized- handle broadcast and the registry-token broadcast (whose composite gains the reservation height and returns the stale verdict for the registry's existing owner-guarded reconciliation). Reconciliation runs OUTSIDE the guard — those paths retake manager locks. Deliberate cost: writers queue behind the network await, bounded by the broadcaster's own timeout — the price of the invariant without a key-wallet-side in-broadcast pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 47-60: The dispatch_unexpired method currently holds the
wallet_manager read guard across the asynchronous broadcast, risking blocked
writes and re-entrant deadlocks. Add the required key-wallet in-broadcast pin
while the manager guard is held, then release the guard before awaiting
broadcaster.broadcast; also configure an explicit timeout for the
DapiBroadcaster request instead of relying on RequestSettings::default().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e758b3e3-a12c-46d9-93b3-027dcab04e8a
📒 Files selected for processing (2)
packages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The previous freshness race is closed, but the replacement holds the shared wallet-manager read lock while the production SPV broadcaster waits for acceptance. Dash-SPV must acquire the same manager's write lock before its serialized mempool task can process the acceptance signals, so fresh transactions can reach peers yet consistently time out as MaybeSent; the exported FFI documentation also still omits the terminal stale outcome.
Source: reviewers gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:52-59: Release the manager lock before awaiting SPV acceptance
`dispatch_unexpired` retains the shared wallet-manager read guard throughout `TransactionBroadcaster::broadcast`. The production `SpvBroadcaster` does not return when initial dispatch begins: it calls dash-spv's `broadcast_transaction_and_wait` and waits up to 30 seconds for a peer echo, InstantSend lock, or confirmation. `SpvRuntime` was constructed with this same wallet manager. Dash-SPV's local transaction handler first sends the transaction to selected peers and then calls `wallet.write().await` before `process_mempool_transaction`; that write cannot proceed while this read guard is held. Because the mempool manager handles its local transaction, peer messages, and sync events serially, it also cannot process the later echo, InstantSend, or confirmation that would resolve the waiting broadcast. A fresh transaction can therefore reach peers but time out as `MaybeSent`, retaining its reservation and reporting an ambiguous failure instead of success. The same guard also delays all manager writers during DAPI or SPV network I/O. Preserve freshness and ownership with a reservation-level in-broadcast pin installed under the manager lock, or split initial dispatch from acceptance waiting, then release the manager guard as soon as network dispatch has definitively begun.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but its exported contract documents only ordinary broadcast and removed-wallet outcomes. The function removes the opaque transaction handle before validation. On the stale branch, the broadcaster is never invoked, `out_txid` remains null, and owner-guarded cleanup releases any reservation still owned by this transaction so the caller can rebuild immediately. Raw native callers need to know that this outcome is terminal: retrying, abandoning, or freeing the consumed handle is invalid.
Held across the broadcaster await, the read guard starved the very pipeline the await depends on: the production SpvBroadcaster waits on dash-spv's mempool manager, whose local-transaction handler takes wallet.write() on this same manager lock before it can process the echo/IS-lock/confirmation events that complete the wait. Every dispatch therefore rode the full 30s acceptance timeout to an ambiguous MaybeSent — reservation kept while the transaction was actually on-chain, rebuild selection left with no spendable UTXOs — and tokio's write-preferring queue stalled the whole manager for the window. The mock broadcasters in the test suite never touch the wallet lock, which is why no test caught it. The age check stays at dispatch time under the read guard; the guard now drops before the await (the same lock-free shape as broadcast_releasing_on_rejection). The residual check-to-wire gap is covered by key-wallet's TTL margin — the same margin that already covers the propagation phase, which the guard never spanned — and releasing early is strictly stronger afterwards: the mempool pipeline marks the inputs spent in the wallet's own view within milliseconds instead of after the timeout. All atomicity claims in docs, comments, and the test narrative are rewritten to the actual contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The prior manager-lock deadlock is fixed, but releasing that lock without installing a reservation-level dispatch pin leaves a check-to-send race that can broadcast an old transaction after its inputs have been swept and reassigned. The exported C contract still omits the terminal stale-reservation outcome, and Kotlin's documentation misstates how a second operation on the consumed handle fails.
Source: reviewers gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:56-67: Pin the reservation until initial network dispatch
The manager read guard establishes freshness only at line 61 and is dropped before the broadcaster has dispatched anything. Both production broadcasters can suspend before submission; the SPV path awaits configuration, event subscription, and the network lock before `dispatch_local`. During that gap, catch-up can acquire the manager write lock and advance `last_processed_height` from reservation age 19 to at least 24, after which a concurrent finalization causes key-wallet's `ReservationSet` to sweep the old reservation and reserve the same input under a new owner token. The original future can then resume and submit its already-signed transaction against an input now assigned to another payment. The four-block difference between the age guard and key-wallet's TTL is not an ordering guarantee because catch-up can process multiple blocks and async scheduling places no bound on the pre-dispatch interval. Install an owner-checked, non-expiring in-broadcast pin while the manager guard is held, and retain it until initial dispatch is definitively established; holding the global manager guard through the later acceptance wait is not safe because the SPV mempool path needs its write side.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but its exported contract documents only ordinary broadcast and removed-wallet outcomes. The function removes the opaque transaction handle before checking reservation freshness. On the stale branch, the broadcaster is never invoked, `out_txid` remains null, and owner-guarded cleanup releases any reservation still owned by this transaction so the caller can rebuild immediately. Raw native callers need to know that this outcome is terminal: retrying, abandoning, or freeing the consumed handle is invalid.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`:
- [NITPICK] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt:37-43: Document the local error after Kotlin consumes the handle
`broadcastTransaction` calls `tx.takeForBroadcast()`, which atomically clears the Kotlin handle before JNI runs. A subsequent `abandonTransaction(tx)` therefore does not produce a native invalid-handle error: `takeForAbandon()` delegates to `takeForBroadcast()`, whose `check` throws `IllegalStateException("FinalizedCoreTransaction has already been consumed")` locally. Document the actual exception so callers do not expect a native or typed SDK error from the repeated operation.
The guarded dispatch proves reservation freshness under the manager read guard but must drop that guard before the broadcaster await (holding it starves the SPV mempool pipeline). Both production broadcasters can suspend before submission, and in that unbounded gap sync catch-up can advance last_processed_height past key-wallet's reservation TTL, letting a concurrent build's selection sweep the dispatched build's reservation and re-reserve the same inputs — the already-signed transaction would then hit the wire against inputs reassigned to another payment. Close the window with a non-expiring in-broadcast pin on WalletGeneration, installed atomically with the freshness check while the read guard is still held (freshness below the TTL on the same clock IS the ownership proof — sweeps and height advances run under the write lock) and released by RAII only after the broadcaster returns, cancelled dispatches included. Pins are counted per outpoint so a duplicate dispatch of the same transaction keeps the fence until its last send returns. Every coin-selection choke point — finalize_transaction, the contact-payment build, the asset-lock build — now refuses a build whose selection picked a pinned input, releasing its fresh reservation exactly under the still-held write guard. The registry-token broadcast shares dispatch_unexpired and therefore the same primitive. Also document the Kotlin-side consume semantics: after broadcastTransaction consumes the handle, a follow-up abandonTransaction fails locally with IllegalStateException before any native code runs — not with a native invalid-handle error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The new counted RAII pin closes the reservation check-to-dispatch race while allowing the wallet-manager guard to be released before the broadcaster await, and the Kotlin consumed-handle contract now matches the implementation. Two public broadcast surfaces still omit the new terminal stale-reservation outcome and its required rebuild semantics; these are documentation suggestions, not blocking defects.
Source: Codex reviewers gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but the exported C contract documents only ordinary broadcast and removed-wallet outcomes. The function removes the opaque transaction handle before freshness validation. On the stale branch, `out_txid` remains null, the broadcaster is never invoked, and `broadcast_finalized_transaction` performs owner-guarded cleanup so the caller can rebuild immediately. Raw native callers need to know that this outcome is terminal: retrying, abandoning, or freeing the consumed handle is invalid.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:243-244: Document the stale terminal error on the Swift broadcast API
`broadcastTransactionWithOutcome` calls `takeForBroadcast()` before entering the FFI, but its public documentation describes only accepted, rejected, and unknown network outcomes. The new age guard can instead throw `.staleReservationToken` (34) before touching the network. The native and Swift handles have nevertheless been consumed and the still-owned reservation has been released, so neither retry nor `abandonTransaction` is available; the caller must rebuild.
Carries #4380 (dashpay profile payment addresses) + #4381 (rust-dashcore pin bump). Conflict + pooled-funding reconciliation, mirroring the resolutions already proven on the integration branch: - identity/network/payments.rs: keep the in-broadcast refusal block, return the 5-tuple with offered_accounts, and sweep the release across every offered account (pooled selection) instead of the single managed_account. - asset_lock/build.rs: the refusal-path release now sweeps result.funding_accounts owner-guarded (the pre-merge single funding_account param no longer exists under pooled funding). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Status for a resolution pass — head `831150ca8`. Both threads addressed in `80c54b4420` (broadcast.rs in-broadcast pin; ManagedCoreWallet.kt consume-semantics KDoc). Merge conflict with v4.2-dev resolved by mirroring the integration-branch pooled-funding release (payments.rs 5-tuple refusal sweeps every offered account; asset_lock/build.rs sweeps result.funding_accounts owner-guarded). platform-wallet 846 tests passing. Ready to resolve — thanks. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The age check and in-broadcast pin close the pre-dispatch reassignment race, but the pin is released when the broadcaster returns even if synchronization advanced the reservation beyond its TTL during the await. On the DAPI path, no local mempool processing immediately marks the inputs spent, so a subsequent build can reselect them and create a conflicting payment; the native and Swift broadcast contracts also still omit the terminal stale-reservation outcome.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:75-91: Retain the input fence when dispatch outlives the reservation TTL
`_in_broadcast_pin` is dropped as soon as `TransactionBroadcaster::broadcast` returns. If catch-up advances `last_processed_height` beyond key-wallet's reservation TTL during that await, the underlying reservation can be swept as soon as another selection runs; after the pin drops, the same input is selectable again. This is not reliably reconciled by local transaction processing: `DapiBroadcaster` only awaits `sdk.execute` and does not inject the transaction into this wallet's mempool state, so both an accepted response and a `MaybeSent` response can return while the local UTXO remains selectable. The test at lines 764-773 confirms that a build succeeds immediately after the pin lifts when no local mempool pipeline updates the wallet, which matches the DAPI path. Preserve a pending-broadcast fence until the spend is observed, or atomically renew the reservation at dispatch so it remains protected after the await; only a definitive pre-send rejection should immediately remove the fence and release the reservation.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but the exported contract documents only ordinary broadcast and removed-wallet outcomes. The function consumes the opaque transaction handle before freshness validation. On the stale branch, `out_txid` remains null, the broadcaster is never invoked, and `broadcast_finalized_transaction` performs owner-guarded cleanup so the caller can rebuild immediately. Native callers need to know this outcome is terminal: retrying, abandoning, or freeing the consumed handle is invalid.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:243-244: Document the stale terminal error on the Swift broadcast API
`broadcastTransactionWithOutcome` calls `takeForBroadcast()` before entering the FFI, but its public documentation describes only accepted, rejected, and unknown network outcomes. The age guard can instead throw `.staleReservationToken` (34) before touching the network. The native and Swift handles have nevertheless been consumed and the still-owned reservation has been released, so neither retry nor `abandonTransaction` is available; the caller must rebuild.
| let _in_broadcast_pin = { | ||
| let wm = self.wallet_manager.read().await; | ||
| let info = wm.get_wallet_info(&self.wallet_id); | ||
| let height = info.map(|info| info.core_wallet.last_processed_height()); | ||
| if reservation_expired(reservation_height, height) { | ||
| return GuardedDispatch::Stale; | ||
| } | ||
| // Pin BEFORE the guard drops: check-and-pin is one atomic step, | ||
| // and freshness under this guard proves the reservation is still | ||
| // ours to pin (see the method docs). The pin outlives the guard | ||
| // and is dropped only after the broadcaster returns below. | ||
| info.map(|info| info.generation.pin_in_broadcast(transaction)) | ||
| // Guard dropped here — holding it across the await starves the | ||
| // SPV pipeline that must complete the wait; the pin, not the | ||
| // guard, covers check-to-wire. | ||
| }; | ||
| GuardedDispatch::Sent(self.broadcaster.broadcast(transaction).await) |
There was a problem hiding this comment.
🔴 Blocking: Retain the input fence when dispatch outlives the reservation TTL
_in_broadcast_pin is dropped as soon as TransactionBroadcaster::broadcast returns. If catch-up advances last_processed_height beyond key-wallet's reservation TTL during that await, the underlying reservation can be swept as soon as another selection runs; after the pin drops, the same input is selectable again. This is not reliably reconciled by local transaction processing: DapiBroadcaster only awaits sdk.execute and does not inject the transaction into this wallet's mempool state, so both an accepted response and a MaybeSent response can return while the local UTXO remains selectable. The test at lines 764-773 confirms that a build succeeds immediately after the pin lifts when no local mempool pipeline updates the wallet, which matches the DAPI path. Preserve a pending-broadcast fence until the spend is observed, or atomically renew the reservation at dispatch so it remains protected after the await; only a definitive pre-send rejection should immediately remove the fence and release the reservation.
source: ['codex']
There was a problem hiding this comment.
Fixed in 2b911bc — the in-broadcast pin is now a two-phase fence whose second phase (pending-spend) survives the broadcaster return, because ReservationSet exposes no renew primitive at the pinned revision, so the fix re-anchors an equivalent TTL at dispatch instead of re-stamping the reservation.
Concretely, on WalletGeneration:
- dispatching — unchanged: counted, non-expiring, from check-and-pin under the manager read guard until
broadcastreturns. - pending-spend — installed on drop when the dispatch returned anything but
BroadcastError::Rejected, bounded atdispatch_height + IN_BROADCAST_FENCE_BLOCKSwhereIN_BROADCAST_FENCE_BLOCKS = 24— key-wallet's ownRESERVATION_TTL_BLOCKS, measured from dispatch rather than from the build.
Why this rather than renewal or an unbounded fence:
- Renewal was your first option and is the right shape, but key-wallet's
ReservationSetandRESERVATION_TTL_BLOCKSare private at rev173ffac. A dispatch-anchored fence of exactly that TTL is the same guarantee in the layer that can express it: the inputs are continuously protected — by the reservation until its build-anchored TTL, then by the fence — for a full TTL past the moment they actually reached the network. - Only
Rejectedfrees the inputs at dispatch return, per your "only a definitive pre-send rejection" note.MaybeSentstays fenced. - The fence lapses rather than persisting until an explicit spend observation. An outpoint the wallet has already observed as spent never reaches selection, so the bound is not consulted in the normal case — the fence goes inert on its own. The bound exists only for a transaction that is never observed (dropped for fee or conflict): its reservation is gone at TTL, and a non-expiring fence would strand those funds with nothing able to clear it. Lapsing at the same TTL leaves the residual exposure identical to the one key-wallet's reservation TTL already accepts, and no larger. If you want a hard "until observed" release instead, the hook would have to come from
changeset/core_bridge'sspent_utxosand would couple the generation to the sync adapter — happy to do that if you prefer it, but I did not think the extra coupling was worth it given the above.
SPV starvation (9b033cb) is untouched: neither phase takes a manager lock, and the read guard is still dropped before the broadcaster await. Lapsed entries are reaped by in_broadcast_conflict — the only reader — so the map stays bounded without a background task.
Tests, in the barrier-gated shape:
dispatched_input_stays_fenced_after_the_broadcaster_returnsdispatches at the oldest height the age guard admits (stamped + RESERVATION_MAX_AGE_BLOCKS - 1), which is what separates the two clocks, then probes at a height where key-wallet's TTL has provably swept the reservation and only the fence stands — the "unreserved AND unfenced" window you identified — and then past the bound to prove it lapses. Reverting just theretain_pending_spendcall makes the competing build succeed (Ok(SignedCoreTransaction { … })), i.e. it reproduces the reported race rather than merely asserting the new behaviour.definitively_rejected_dispatch_installs_no_fenceproves the rejection path still frees immediately.- The existing
in_broadcast_pin_blocks_reselection_until_dispatch_returnskeeps its mid-dispatch assertion; its tail assertion is now annotated, because that test's 48-block catch-up already outruns the new bound, so what it proves there is the dispatching phase lifting, not the fence. - Generation-level:
dispatching_pin_never_expires,retained_pin_fences_past_dispatch_until_the_bound,lapsed_fences_are_reaped_on_read,the_longer_pending_fence_wins,rejected_dispatch_frees_the_input_immediately.
854 tests green; cargo fmt --check --all and cargo clippy --all-targets --all-features --locked -- --no-deps -D warnings both clean.
There was a problem hiding this comment.
Fixed in 89586f7248, together with the two related findings — they are one defect with three symptoms.
InBroadcastPin::drop decided whether to fence from a flag set only AFTER broadcaster.broadcast(...).await returned, so the guard's DEFAULT was "nothing reached the network". Every scenario the three findings describe stops inside that await — a suspension before submission, a TTL advance during it, or a cancelled dispatch — and none of them says anything about whether the transaction was sent.
The default is now inverted: the pin fences on drop, and release_pending_spend() (replacing retain_pending_spend()) is called ONLY on BroadcastError::Rejected, the one outcome that proves nothing was sent. So suspension, TTL expiry, unwind and cancellation all keep the fence; the only path that frees inputs has positive evidence of a pre-send rejection. The bound is unchanged: IN_BROADCAST_FENCE_BLOCKS past the dispatch height.
This reverses a documented intent — the old test doc asserted a cancelled dispatch "unpins outright, exactly as a definitive pre-send rejection does". That equivalence was the bug; the doc is rewritten rather than left contradicting the code.
dropping_an_unreleased_pin_keeps_the_fence reproduces the cancellation case and pins the bound with a negative control (the fence still lapses AT the bound, so this does not turn into a permanent hold). 689 platform-wallet tests pass.
There was a problem hiding this comment.
Addressed at head (89586f724). The pin is no longer simply dropped when the broadcaster returns — on drop it converts to a pending-spend fence:
impl Drop for InBroadcastPin {
fn drop(&mut self) {
let pending_until = self.retain_pending_spend.then(|| {
self.dispatch_height.saturating_add(IN_BROADCAST_FENCE_BLOCKS)
});
self.generation.unpin_in_broadcast(&self.outpoints, pending_until);
}
}IN_BROADCAST_FENCE_BLOCKS is 24, anchored on dispatch_height — the same last_processed_height sample the freshness check consumed, not a fresh read, so the check and the fence can never disagree about the clock. That is the reservation TTL the inputs would have had, re-anchored at dispatch, which is the "atomically renew the reservation at dispatch" option you named.
Your DapiBroadcaster point is the reason this shape was chosen rather than relying on local processing: sdk.execute injects nothing into this wallet's mempool state, so on that path the fence is the only thing holding the inputs. On the SPV path the wallet observes the spend within milliseconds and the outpoint stops reaching selection at all, so the fence goes inert well before the bound — the 24 blocks are only the backstop for a transaction never observed.
Only BroadcastError::Rejected clears it; MaybeSent stays fenced.
There was a problem hiding this comment.
You're right, and my previous reply defended a design that didn't answer your scenario. I'd argued the fence was anchored on "the SAME last_processed_height the freshness check consumed, not a fresh read, so the check and the fence can never disagree about the clock." That consistency argument is true and irrelevant: the two agreeing doesn't help when the reading they agree on is stale by the time the fence is installed.
The case I missed is the one you describe — catch-up advancing during the await. broadcast can suspend for minutes mid-catch-up (routine on mobile). If the wallet advances a full IN_BROADCAST_FENCE_BLOCKS in that gap, dispatch_height + IN_BROADCAST_FENCE_BLOCKS is already in the past when Drop runs, so InBroadcastFence::blocks() returns false immediately, the next in_broadcast_conflict reaps the entry, and the input is reselectable while the transaction may be on the network. An expired fence is indistinguishable from no fence — so inverting the default in 89586f7 retained a bound that had already run out.
Fixed in 58efacf. Every non-rejection outcome now samples last_processed_height after the broadcaster returns and anchors a full interval on that reading:
let outcome = self.broadcaster.broadcast(transaction).await;
if matches!(outcome, Err(BroadcastError::Rejected { .. })) {
if let Some(pin) = in_broadcast_pin.as_mut() { pin.release_pending_spend(); }
} else if let Some(pin) = in_broadcast_pin.as_mut() {
let height = { /* manager read guard */ };
if let Some(height) = height { pin.anchor_pending_spend(height); }
}The dispatching phase is still held across that sampling, so the outpoints are never selectable between the broadcaster's return and the stamp. And the clock-consistency property is kept rather than traded away — the post-await sample is now the single anchor for the installed fence, read from the same last_processed_height under the same manager guard the freshness check used, which is the clock in_broadcast_conflict and key-wallet's TTL sweep both run on. Nothing is stamped against a height no guarded section saw.
pin_in_broadcast no longer takes a height parameter at all, so the pre-await anchor is unrepresentable rather than merely corrected.
Reproduction: fence_anchors_after_the_await_so_catch_up_cannot_pre_expire_it parks a barrier-gated broadcaster, runs catch-up a full fence interval past the pre-await sample (also past key-wallet's TTL, so the reservation is provably swept and the fence is the only protection), returns Ok, then builds. Against the pre-await anchor it fails with a fully signed competing transaction spending the same outpoint — the double-spend, not just a missing assertion.
One thing worth surfacing: the tail of in_broadcast_pin_blocks_reselection_until_dispatch_returns had encoded this bug. It advanced 48 blocks during the await and then asserted the input was selectable again, annotated as "the fence is already lapsed here". That test failed against the fix and is rewritten — the input stays fenced, and lapses a full interval past the post-await anchor.
…turn `dispatch_unexpired` dropped its in-broadcast pin the moment `TransactionBroadcaster::broadcast` returned. That is safe only for `SpvBroadcaster`, which injects the transaction into dash-spv's local mempool pipeline so the inputs leave this wallet's selectable set within milliseconds. `DapiBroadcaster::broadcast` only awaits `sdk.execute` and injects nothing, so on that path both an accepted response and an ambiguous `MaybeSent` returned with the input still selectable while the transaction was in flight — and if catch-up had advanced `last_processed_height` past key-wallet's 24-block reservation TTL during the await, the reservation was already swept too. The input was then neither reserved nor fenced: exactly the sweep + re-select race the pin was added to close (#4309). The pin becomes a two-phase fence on `WalletGeneration`: * dispatching — the existing counted, non-expiring pin, from check-and-pin until the broadcaster returns. * pending-spend — installed when the broadcaster returns anything but a definitive pre-send rejection, lasting `IN_BROADCAST_FENCE_BLOCKS` (24, key-wallet's own `RESERVATION_TTL_BLOCKS`) past the height the dispatch was authorized at. The second phase is key-wallet's reservation renewal implemented one layer up: `ReservationSet` exposes no renew primitive at the pinned revision, so instead of re-stamping the reservation we re-anchor an equivalent TTL at dispatch — which is the moment the transaction actually reached the network, and the point the TTL should always have been measured from. Only `BroadcastError::Rejected` frees the inputs at dispatch return; that outcome proves nothing is on the wire, and the caller releases the reservation in the same breath. The fence lapses rather than persisting: an outpoint the wallet has already observed as spent never reaches selection at all, so in the normal case the bound is never consulted. It exists only so a never-observed transaction cannot strand its inputs forever, and it leaves the residual exposure identical to the one key-wallet's reservation TTL already accepts. Nothing here touches the wallet-manager lock, so the SPV-starvation fix from 9b033cb is preserved verbatim: the manager read guard is still dropped before the broadcaster await. Lapsed entries are reaped by the conflict check itself — the only place the fence is read — so the map stays bounded with no background task. Tests: the barrier-gated race test's tail assertion is corrected (its 48-block catch-up already outruns the new bound, so it still proves the *dispatching* phase and says so); a new `dispatched_input_stays_fenced_after_the_broadcaster_returns` dispatches at the oldest height the age guard admits, then probes a height where the reservation is provably swept and only the fence stands. Reverting the `retain_pending_spend` call makes that test's competing build succeed, i.e. it reproduces the reported race. Five generation-level tests cover the non-expiring dispatching phase, the bound, on-read reaping, the longer-fence-wins merge, and the rejection path installing no fence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…height `dispatch_unexpired` sampled `last_processed_height` twice under the same manager read guard — once for the freshness check, once for the pin's anchor. Both reads are identical in practice, but the pin is now taken via `info.zip(height)` so the fence is anchored on the very value the check consumed and the two cannot drift apart. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The age check and dispatching pin close the original check-to-send race, but the post-dispatch protection still has two unsafe gaps: its deadline can already be expired when a long broadcast returns, and cancellation drops the pin without preserving a fence for a possibly submitted transaction. The native and Swift public contracts also omit the terminal stale-reservation outcome.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:124-137: Preserve the fence when a dispatched broadcast future is cancelled
`retain_pending_spend()` is reached only after `broadcast(...).await` returns. If a caller cancels this public async operation during that await, `InBroadcastPin::drop` sees `retain_pending_spend == false` and removes the dispatch fence. Cancellation does not establish a pre-send failure: DAPI may have delivered the request while waiting for its response, and SPV may have dispatched to peers while waiting for an echo, InstantSend lock, or confirmation. Once the underlying reservation is swept, a caller using `timeout` or `select!` can therefore reselect inputs belonging to a possibly submitted transaction. Make cancellation conservative like `MaybeSent`, or keep the actual broadcast operation alive independently of caller cancellation; disable pending retention only after a returned definitive rejection.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:114-119: Retain the input fence when dispatch outlives the reservation TTL
(existing thread: https://github.com/dashpay/platform/pull/4309#discussion_r3771352689)
The pending-spend deadline is derived from `height`, which is sampled before `TransactionBroadcaster::broadcast` is awaited. If catch-up advances `last_processed_height` by `IN_BROADCAST_FENCE_BLOCKS` or more during that await, `retain_pending_spend()` installs a deadline that has already elapsed. Dropping the dispatching pin then makes the input immediately selectable even though the transaction was accepted or may have been sent. The test `in_broadcast_pin_blocks_reselection_until_dispatch_returns` explicitly advances 48 blocks during the await and permits a new build immediately after return; that is unsafe for `DapiBroadcaster`, which does not inject the transaction into the local wallet. Keep the dispatching pin active while sampling the current height after every non-rejected return, then anchor a full pending-spend interval to that post-await height. Only a definitive `BroadcastError::Rejected` should remove the pin without retaining a fence.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but the exported contract documents only ordinary broadcast and removed-wallet outcomes. The function consumes the opaque transaction handle before freshness validation. On the stale branch, `out_txid` remains null, the broadcaster is never invoked, and `broadcast_finalized_transaction` performs owner-guarded cleanup so the caller can rebuild immediately. Native callers need to know this outcome is terminal: retrying, abandoning, or freeing the consumed handle is invalid.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:243-244: Document the stale terminal error on the Swift broadcast API
`broadcastTransactionWithOutcome` calls `takeForBroadcast()` before entering the FFI, but its public documentation describes only accepted, rejected, and unknown network outcomes. The age guard can instead throw `.staleReservationToken` (34) before touching the network. The native and Swift handles have nevertheless been consumed and the still-owned reservation has been released, so neither retry nor `abandonTransaction` is available; the caller must rebuild.
| let outcome = self.broadcaster.broadcast(transaction).await; | ||
| // Retain the fence for everything except a definitive pre-send | ||
| // rejection: only `Rejected` proves the transaction is not on the | ||
| // network, so only `Rejected` may free the inputs at dispatch return. | ||
| // An ambiguous `MaybeSent` is precisely the case that must stay fenced. | ||
| if !matches!( | ||
| outcome, | ||
| Err(crate::broadcaster::BroadcastError::Rejected { .. }) | ||
| ) { | ||
| if let Some(pin) = in_broadcast_pin.as_mut() { | ||
| pin.retain_pending_spend(); | ||
| } | ||
| } | ||
| drop(in_broadcast_pin); |
There was a problem hiding this comment.
🔴 Blocking: Preserve the fence when a dispatched broadcast future is cancelled
retain_pending_spend() is reached only after broadcast(...).await returns. If a caller cancels this public async operation during that await, InBroadcastPin::drop sees retain_pending_spend == false and removes the dispatch fence. Cancellation does not establish a pre-send failure: DAPI may have delivered the request while waiting for its response, and SPV may have dispatched to peers while waiting for an echo, InstantSend lock, or confirmation. Once the underlying reservation is swept, a caller using timeout or select! can therefore reselect inputs belonging to a possibly submitted transaction. Make cancellation conservative like MaybeSent, or keep the actual broadcast operation alive independently of caller cancellation; disable pending retention only after a returned definitive rejection.
source: ['codex']
There was a problem hiding this comment.
Fixed in 89586f7248, together with the two related findings — they are one defect with three symptoms.
InBroadcastPin::drop decided whether to fence from a flag set only AFTER broadcaster.broadcast(...).await returned, so the guard's DEFAULT was "nothing reached the network". Every scenario the three findings describe stops inside that await — a suspension before submission, a TTL advance during it, or a cancelled dispatch — and none of them says anything about whether the transaction was sent.
The default is now inverted: the pin fences on drop, and release_pending_spend() (replacing retain_pending_spend()) is called ONLY on BroadcastError::Rejected, the one outcome that proves nothing was sent. So suspension, TTL expiry, unwind and cancellation all keep the fence; the only path that frees inputs has positive evidence of a pre-send rejection. The bound is unchanged: IN_BROADCAST_FENCE_BLOCKS past the dispatch height.
This reverses a documented intent — the old test doc asserted a cancelled dispatch "unpins outright, exactly as a definitive pre-send rejection does". That equivalence was the bug; the doc is rewritten rather than left contradicting the code.
dropping_an_unreleased_pin_keeps_the_fence reproduces the cancellation case and pins the bound with a negative control (the fence still lapses AT the bound, so this does not turn into a permanent hold). 689 platform-wallet tests pass.
There was a problem hiding this comment.
Addressed at head (89586f724) — this is what that commit inverted. The pin now fences by default and only a definitive pre-send rejection releases it:
InBroadcastPin {
...
// Fenced by default; only a definitive pre-send rejection releases it.
retain_pending_spend: true,
}retain_pending_spend() is gone; the call site is now the opposite polarity:
let outcome = self.broadcaster.broadcast(transaction).await;
if matches!(outcome, Err(BroadcastError::Rejected { .. })) {
if let Some(pin) = in_broadcast_pin.as_mut() { pin.release_pending_spend(); }
}So a caller cancelling under timeout/select!, or an unwind mid-broadcast, drops the pin with retention still set and the fence is installed — the code never has to observe the cancellation. This is the "make cancellation conservative like MaybeSent" option, made the default so that every unobserved exit is conservative rather than only the ones we enumerated.
Covered by a unit test that models exactly this — it drops the pin with no release_pending_spend() call and asserts the outpoints stay fenced through DISPATCH_HEIGHT + IN_BROADCAST_FENCE_BLOCKS - 1 and clear at the bound.
There was a problem hiding this comment.
Also right, and my previous reply only solved half of it. Fencing-by-default does mean a cancelled dispatch keeps its fence — but the fence it kept carried the pre-await anchor, and the same catch-up that made the caller's timeout fire had typically already expired it. So the retained fence was installed dead and the next selection reaped it. Conservative default, useless bound.
Fixed in 58efacf, taking your first option (make cancellation conservative) with the anchor problem closed. A pin that never reaches the post-await height sample — cancelled or unwound inside broadcast, or the wallet gone from the manager — settles unanchored: it blocks unconditionally, and the first coin selection to consult it stamps a full IN_BROADCAST_FENCE_BLOCKS from its own last_processed_height, read under the manager write guard.
I went this way rather than reading the clock in Drop because Drop is synchronous and can't await the manager lock, and there's no lock-free last_processed_height mirror to read (the balance mirror carries no height). Deferring to the first consult is equivalent in effect and strictly safer: in_broadcast_conflict is the only reader of the fence, so between the pin's drop and that call there is no path by which a fenced outpoint could be selected, and the height it reads is ≥ the drop-time height. Anchoring happens once, before the reap and the lookup, so repeated builds can't extend it.
It still lapses, which was the constraint from the sibling thread — anchoring sweeps every entry rather than only the querying transaction's inputs, so a cancelled dispatch of a coin nobody selects again is still bounded and reaped instead of sitting in the map forever (unanchored_fences_are_bounded_and_reaped_by_unrelated_reads).
cancelled_dispatch_fence_survives_catch_up_during_the_await drives the real path: barrier-gated broadcaster, catch-up past both the TTL and a full fence interval, then dispatcher.abort() and await the handle so InBroadcastPin::drop has provably run. Against the pre-await anchor it fails with a signed competing transaction spending the input.
694 platform-wallet tests pass; cargo fmt --check --all and cargo clippy --all-targets --all-features --locked -- --no-deps -D warnings clean.
There was a problem hiding this comment.
Resolved in 89586f7 — Preserve the fence when a dispatched broadcast future is cancelled no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
… on rejection #4309 review: three blocking findings — "pin the reservation until initial network dispatch", "retain the input fence when dispatch outlives the reservation TTL", and "preserve the fence when a dispatched broadcast future is cancelled" — are one defect. `InBroadcastPin::drop` decided whether to fence from a flag set only AFTER `broadcaster.broadcast(...).await` returned, so the guard's default was "nothing reached the network". But every path the reviewer names stops INSIDE that await: the dispatching future cancelled, an unwind, or the broadcaster suspending before submission. None of them carries any information about whether the transaction was sent, and the old default freed the inputs there — letting an immediate reselection double-spend a transaction already on the wire. Invert it. The pin now fences by default and `release_pending_spend()` (replacing `retain_pending_spend()`) is called ONLY on `BroadcastError::Rejected`, the one outcome that proves nothing was sent. Absence of evidence that a send happened is not evidence that it did not, so the fence survives every exit except the proving one. The bound is unchanged: IN_BROADCAST_FENCE_BLOCKS past dispatch height. This reverses a documented intent — the old test doc asserted a cancelled dispatch "unpins outright, exactly as a definitive pre-send rejection does". That equivalence is the bug; doc rewritten to match. Tests: `dropping_an_unreleased_pin_keeps_the_fence` reproduces the cancellation case and pins the bound with a negative control (the fence lapses at the bound, it is not permanent). Existing rejection/counting tests now release explicitly. 689 platform-wallet lib tests pass.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest change makes cancellation conservative by retaining a pending-spend fence by default, but that fence is still anchored to the height sampled before the broadcaster await. A sufficiently large height advance during either a completed or cancelled broadcast therefore installs an already-expired fence, and the native and Swift public contracts still omit the terminal stale-reservation outcome.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but this exported contract documents only ordinary broadcast outcomes and removed-wallet failure. The function removes the opaque transaction handle before freshness validation. On the stale branch, `out_txid` remains null, the broadcaster is never invoked, and owner-guarded cleanup releases any reservation still owned by the transaction. Document that this result is terminal: the consumed handle cannot be retried, abandoned, or freed, and the caller must rebuild.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/ManagedCoreWallet.swift:243-244: Document the stale terminal error on the Swift broadcast API
`broadcastTransactionWithOutcome` calls `takeForBroadcast()` before entering the FFI, but its public documentation describes only accepted, rejected, and unknown network outcomes. The age guard can instead throw `.staleReservationToken` (34) before touching the network. Even in that case, both the Swift and native handles have been consumed and owner-guarded cleanup has released any still-owned reservation, so retry and `abandonTransaction` are unavailable; callers must rebuild the transaction.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:113-124: Retain the input fence when dispatch outlives the reservation TTL
(existing thread: https://github.com/dashpay/platform/pull/4309#discussion_r3771352689)
The pending-spend deadline is still derived from `height`, which is sampled before `self.broadcaster.broadcast(transaction).await`. `InBroadcastPin::drop` calculates `pending_until` as that old height plus `IN_BROADCAST_FENCE_BLOCKS`, so if catch-up advances by at least that interval during the await, the fence is already expired when an accepted or `MaybeSent` broadcast returns. The next selection prunes it and can reselect an input belonging to a transaction that reached or may have reached the network. The test `in_broadcast_pin_blocks_reselection_until_dispatch_returns` explicitly advances 48 blocks while the broadcaster is parked and permits a build after return for this reason; that remains unsafe for `DapiBroadcaster`, which does not inject the transaction into the local wallet. For every outcome other than definitive `Rejected`, keep the dispatching pin active while sampling the post-await wallet height and anchor a full pending-spend interval to that current height.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:123-130: Preserve the fence when a dispatched broadcast future is cancelled
(existing thread: https://github.com/dashpay/platform/pull/4309#discussion_r3777819130)
Default pending retention prevents cancellation from immediately removing the fence, but `InBroadcastPin::drop` still derives its deadline from the height sampled before entering the broadcaster. If the wallet catches up by `IN_BROADCAST_FENCE_BLOCKS` or more while the future is suspended, cancelling the operation installs an already-expired fence; `in_broadcast_conflict` then prunes it on the next build. Cancellation does not prove that nothing was sent: DAPI may have delivered the request while awaiting its response, and SPV may have dispatched to peers while awaiting acceptance. This path needs a deadline anchored to the height at cancellation/drop, an unbounded fence cleared by spend observation, or a broadcast task whose completion and post-await height sampling are independent of caller cancellation.
… before
The pending-spend fence was bounded at `dispatch_height +
IN_BROADCAST_FENCE_BLOCKS`, where `dispatch_height` was the
`last_processed_height` sampled in the guarded section BEFORE
`broadcaster.broadcast(...).await`.
A broadcast await can suspend for minutes in the middle of chain catch-up —
the ordinary mobile case. If the wallet advances a full fence interval inside
that await, the fence installed when the send returns is ALREADY LAPSED: the
next coin selection reaps it and can reselect an input of a transaction that
reached the network. An expired fence is indistinguishable from no fence, so
inverting the fence-by-default polarity did not close this — it made the guard
retain a bound that had already run out.
The same defect hits the cancellation path, where `Drop` is synchronous and
cannot await the manager lock to read a fresh clock at all.
Fix, in two halves:
* Non-rejection outcomes (accepted and ambiguous `MaybeSent`) sample
`last_processed_height` AFTER the broadcaster returns, while the dispatching
phase of the pin is still held, and anchor a full interval on that reading.
The dispatching hold covers the sampling, so the outpoints are never
selectable between the return and the stamp. Check-vs-fence clock
consistency is preserved by making this the single anchor: both readings
come from the same `last_processed_height` under the manager guard, the
clock `in_broadcast_conflict` and key-wallet's TTL sweep also run on.
* A dispatch that never reaches that sample — future cancelled or unwound
inside `broadcast`, or the wallet gone from the manager — settles UNANCHORED
and blocks unconditionally until the first coin selection stamps it from its
own height, read under the manager write guard. That is the drop-time clock,
deferred to the first moment it is both readable and relevant: nothing can
reach a fenced outpoint in between. Anchoring runs over every entry, so an
unanchored fence is still bounded and reaped even when its own outpoint is
never re-selected.
`pin_in_broadcast` no longer accepts a height at all, which makes the stale
anchor unrepresentable rather than merely corrected.
Tests: `fence_anchors_after_the_await_so_catch_up_cannot_pre_expire_it` and
`cancelled_dispatch_fence_survives_catch_up_during_the_await` drive the real
`dispatch_unexpired` through a barrier-gated broadcaster with catch-up running
inside the await; both fail against the pre-await anchor by returning a fully
signed competing transaction spending the same outpoint. The tail of
`in_broadcast_pin_blocks_reselection_until_dispatch_returns` asserted the buggy
behaviour ("the pin lifted, so the input is selectable") and is rewritten.
Generation-level: `cancelled_dispatch_fence_anchors_at_the_first_selection_not_at_dispatch`,
`an_unanchored_fence_outlives_an_anchored_one`,
`unanchored_fences_are_bounded_and_reaped_by_unrelated_reads`.
694 platform-wallet tests pass; fmt and clippy clean.
Refs: #4309
…n broadcast `ErrorStaleReservationToken` (34) from `core_wallet_broadcast_signed_transaction` is terminal, and neither the Rust ABI doc nor the Swift wrapper said so. A caller reading either could reasonably expect the refusal to be retryable, or to need an abandon to clean up — both are wrong and the second is unreachable. State it on both surfaces: the handle is already consumed when the age guard runs (a retry returns `NotFound` (98) rather than resending, and abandon has nothing left to free), the refusal path reconciles the reservation itself owner-guarded, and the only recovery is to REBUILD the transaction — the released inputs are immediately reselectable with no cleanup call in between. Behaviour is unchanged; this documents what `aged_broadcast_refuses_and_releases_for_rebuild` already asserts. The deprecated Swift `broadcastTransaction(_:)` delegates to the documented `broadcastTransactionWithOutcome(_:)`, so it is covered by the same note. Refs: #4309
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The stale-handle guard, cancellation handling, and terminal FFI/Swift documentation are substantially improved, but one blocking phase-transition race remains: the post-await height read is no longer protected when that height is installed as the pending-spend fence. A concurrent height writer can therefore make the newly installed fence immediately expired, reopening input reselection after an accepted or possibly-sent broadcast.
Source: reviewers backend model gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:195-204: Retain the input fence when dispatch outlives the reservation TTL
(existing thread: https://github.com/dashpay/platform/pull/4309#discussion_r3771352689)
The fence now uses a post-await height, but the wallet-manager read guard is released at line 199 before `anchor_pending_spend` and `drop(in_broadcast_pin)` install that height into `WalletGeneration`. A waiting manager writer can run in this interval and advance `last_processed_height` by `IN_BROADCAST_FENCE_BLOCKS` or more. The still-live dispatching pin blocks selection during that advance, but when this task resumes it installs a deadline based on the older sample while simultaneously removing the dispatching hold. The next build observes the deadline as already expired, reaps it, and can reselect an input belonging to an accepted or possibly-sent transaction. Keep the manager read guard held while anchoring and dropping the pin so height advancement cannot interleave with the dispatching-to-pending transition.
2 participants
Continues #4196 — moved from a fork branch to an in-repo branch so maintainers can push changes directly, per review request. Full review history on #4196.
Rebased down to just the age-guard onto current
v4.2-dev(2026-08-10): the #4185/#4308 stack this PR was riding has merged, so every stacked commit was dropped and the single age-guard commit was adapted to the renamed finalized-transaction surface (#4323/#4325 removed the_v2/V2 suffixes) and the slice-basedfinalize_transactionsignature.Follow-up to #4185 (requested by @shumkov): the finalized-transaction
handle surface (
core_wallet_tx_builder_finalize→broadcast_finalized_transaction) retained a stale-release hazard — a pinnedhandle had no age guard, so a long-held handle could broadcast against
funding inputs that key-wallet's
ReservationSetTTL sweep may already havereleased and re-selected for an unrelated build. This goes live the moment iOS
starts issuing deferred sends.
This mirrors the deferred registry-token age policy on the finalized-handle path:
RESERVATION_MAX_AGE_BLOCKS(20; key-wallet TTL 24) andreservation_expired()are hoisted towallet::reservations, so theregistry and the finalized-handle path measure a reservation's age against
the same number.
broadcast_finalized_transactionrefuses — beforetouching the broadcaster — once
current_height − reservation_height >=theshared bound, using the reservation's own stamp height already carried on
SignedCoreTransaction::reservation_height. The check runs after theexisting generation-identity check, matching the registry order. The refusal
reconciles the reservation on the way out, exactly like the registry's
stale-token branch: the FFI wrapper has already consumed the opaque handle,
so no follow-up abandon is possible, and the owner-guarded release
(
release_reservation_if_owner, safe at any age — a no-op once ownershiptransferred) frees the still-owned inputs for the instructed immediate
rebuild.
PlatformWalletError::StaleReservationreuses the existing FFI
ErrorStaleReservationToken(34); no new code isallocated. Reuse is documented on both sides.
(never reached on the funded finalize path) honours the bound and skips its
unguarded by-outpoint release, leaving the aged reservation for key-wallet's
TTL to reclaim.
Tests: fresh handle broadcasts; aged handle refuses with
StaleReservationand the refusal itself releases for an immediate rebuild (a late abandon of the
consumed handle is an owner-guarded no-op that cannot free the rebuild's
reservation); exact threshold boundary (BIP44/BIP32); FFI mapping to the shared
code; terminal FFI stale-broadcast, aged free, and aged failure-path abandon.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests