fix(platform-wallet): age-guard the finalized-transaction handle broadcast

[bfoss765]

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.

What this does

Two related fixes to the same hazard: a funding reservation can be swept out from under a transaction that is already signed, or already on the wire, and re-selected by an unrelated build.

key-wallet's ReservationSet sweeps any reservation older than RESERVATION_TTL_BLOCKS (24) and returns the outpoints to the selectable pool with no ownership or generation check. Two windows were unguarded against that.

1. Age-guard the finalized-transaction handle

A core_wallet_tx_builder_finalize handle can be pinned by the host for an arbitrary time before broadcast_finalized_transaction. Held past the sweep, it would broadcast against inputs another build had since re-reserved.

• Shared bound. RESERVATION_MAX_AGE_BLOCKS (20) and reservation_expired() move to wallet::reservations, so the deferred registry and the finalized-handle path measure age against the same number. Kept strictly below key-wallet's TTL of 24 on the same last_processed_height clock, so the guard always trips before a sweep could have happened.
• Guarded op. The check runs inside dispatch_unexpired, atomically with dispatch under the wallet-manager read guard — not by the caller beforehand, where it would go stale before the send. It runs after the existing generation-identity check, matching the registry's order.
• Refusal reconciles. The FFI wrapper has already consumed the opaque handle by then, so no follow-up abandon is possible. The refusal releases owner-guarded (release_reservation_if_owner, safe at any age — a no-op once ownership transferred), freeing the still-owned inputs for the instructed immediate rebuild.
• Error code. A token-less PlatformWalletError::StaleReservation reuses the existing FFI ErrorStaleReservationToken (34); no new code is allocated. Reuse is documented on both sides.

2. Fence in-broadcast inputs until the spend is observed

The age check alone is not an ordering invariant. dispatch_unexpired must drop the manager guard before the broadcaster await — holding it starves the dash-spv mempool pipeline that the wait itself depends on — and the broadcaster can suspend before submission. Catch-up can advance the clock in that gap, the TTL can sweep, and a concurrent build can re-reserve the same inputs while the signed transaction is still in flight.

• The pin. WalletGeneration::pin_in_broadcast records the dispatched transaction's outpoints on the manager-registered generation, installed before the guard drops so check-and-pin is one atomic step. It has no TTL while the dispatch is in flight and is released by Drop, so cancellation and unwind are covered without a special case.
• The check. Every coin-selection choke point — CoreWallet::finalize_transaction, the DashPay contact-payment build, the asset-lock build — calls in_broadcast_conflict immediately after it reserves its selection, still under the manager write guard the sweep runs under, and refuses with PlatformWalletError::InputMidBroadcast after releasing its own fresh reservation.
• The release is evidence, not elapsed chain. New SpendObservationHandler (wired into the manager's event fan-out) releases a fence when the wallet actually observes the outpoints spent, off WalletEvent::TransactionDetected and BlockProcessed. It is built on spent_outpoints, the same per-record input walk that produces CoreChangeSet::spent_utxos, so the fence and the persisted spent set cannot diverge. Either spend shape releases — the dispatch's own transaction or a competing one — because after either the outpoint is out of the selectable set.
• Orphan backstop only. IN_BROADCAST_FENCE_ORPHAN_TIMEOUT (1 h, a monotonic Instant) stops a transaction that is never observed from stranding its inputs for the life of the process. It is a liveness valve, not the safety argument.
• Settle policy. A definitive pre-send rejection (BroadcastError::Rejected, contractually pre-send) frees the outpoints outright. Every other outcome — accepted, ambiguous MaybeSent, cancellation, unwind — opens the pending-spend phase.

Why the fence grew through review

Rounds 2–4 bounded the pending-spend phase at last_processed_height + N and disagreed only about where the height was sampled. All three are unsound for the same reason: during catch-up the wallet advances that height by thousands of blocks in seconds, over blocks mined before the transaction was submitted, so elapsed height is evidence about the chain's past and never about this transaction. A routine historical sync could retire a fence protecting a transaction that had just gone to the network. Hence the observation-based release and the monotonic backstop — and pin_in_broadcast deliberately accepts no height at either end, so the mis-anchoring is unrepresentable rather than merely corrected.

Error surfaces

Rust	FFI code	Note
PlatformWalletError::StaleReservation	ErrorStaleReservationToken (34)	Reuses the deferred-token code; both mean "rebuild", neither touched the network
PlatformWalletError::InputMidBroadcast { outpoint }	ErrorUnknown (99)	Deliberate, and an explicit arm rather than the catch-all. The numeric space is a cross-PR registry and a new value must be mirrored into the Swift and Kotlin enums; claiming one is a separate coordinated change and a one-line edit here

Host bindings (Kotlin DashSdkError/ManagedCoreWallet, Swift ManagedCoreWallet) document that code 34 now covers both deferred-payment surfaces, and that broadcastTransaction consumes the handle on every outcome including the stale refusal.

Tests

Age guard: fresh handle broadcasts; aged handle refuses with StaleReservation and the refusal itself releases for an immediate rebuild; the age is re-checked at dispatch rather than by the caller; exact threshold boundary (BIP44/BIP32); FFI mapping to the shared code; terminal FFI stale-broadcast, aged free, and aged failure-path abandon.

Fence: a pin blocks re-selection until dispatch returns; the fence survives a full historical catch-up advance; an observed spend of the dispatched transaction releases it; a competing spend also releases it; observation reaches only the matching registered generation; the orphan backstop is the only timeout and catch-up cannot move it; a cancelled dispatch keeps its fence across catch-up; a definitive rejection installs no fence; the dispatching→pending handoff is never observable half-done (driven deterministically by a cfg(test) settle-boundary hook, not by scheduler luck); the manager constructor really wires SpendObservationHandler into the event fan-out.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The 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.

Changes

Finalized transaction reservation expiry

Layer / File(s)	Summary
Shared reservation age policy packages/rs-platform-wallet/src/wallet/reservations.rs, packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs	Defines the shared 20-block expiration rule and applies it to reservation lifecycle checks.
Core wallet stale-handle behavior packages/rs-platform-wallet/src/error.rs, packages/rs-platform-wallet/src/test_support.rs, packages/rs-platform-wallet/src/wallet/core/broadcast.rs, packages/rs-platform-wallet/src/wallet/core/transaction.rs	Rejects aged finalized transactions before broadcasting. Applies age-aware abandonment rules. Tests fresh, stale, boundary, and rebuild cases.
Error mapping and SDK contracts packages/rs-platform-wallet-ffi/src/error.rs, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt	Maps PlatformWalletError::StaleReservation to ErrorStaleReservationToken. Documents stale-handle recovery and cleanup behavior.
FFI cleanup validation packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs	Tests aged-handle release, invalid-wallet abandonment, rebuilding, stale broadcast rejection, and repeated freeing.

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

Loading

Possibly related PRs

• dashpay/platform#4185: Extends reservation-token handling to finalized-transaction handles.
• dashpay/platform#4308: Shares stale-reservation handling across wallet, FFI, and broadcast paths.
• dashpay/platform#4310: Adds related stale-reservation handling across broadcast and cleanup paths.

Suggested reviewers: lklimek, quantumexplorer

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: age-guarding finalized-transaction handle broadcasts in the platform wallet.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch followup/v4.1/v2-handle-age-guard

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

🕓 Ready for review — 4 ahead in queue (commit 1e0f594)
Queue position: 5/6 · 2 reviews active
ETA: start ~04:17 UTC · complete ~04:32 UTC (median 15m across 30 recent reviews; 2 slots)
Queued 9m ago · Last checked: 2026-08-27 03:30 UTC

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 438153d and 224704f.

📒 Files selected for processing (10)

• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
• packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
• packages/rs-platform-wallet-ffi/src/error.rs
• packages/rs-platform-wallet/src/error.rs
• packages/rs-platform-wallet/src/test_support.rs
• packages/rs-platform-wallet/src/wallet/core/broadcast.rs
• packages/rs-platform-wallet/src/wallet/core/transaction.rs
• packages/rs-platform-wallet/src/wallet/reservations.rs
• packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.69%. Comparing base (ecdfda7) to head (1e0f594).
⚠️ Report is 1 commits behind head on v4.2-dev.

Additional details and impacted files

@@             Coverage Diff              @@
##           v4.2-dev    #4309      +/-   ##
============================================
- Coverage     87.62%   83.69%   -3.94%     
============================================
  Files          2757     2760       +3     
  Lines        352614   368523   +15909     
============================================
- Hits         308967   308423     -544     
- Misses        43647    60100   +16453     

Components	Coverage Δ
dpp	85.04% <ø> (-3.79%)	⬇️
drive	82.06% <ø> (-4.56%)	⬇️
drive-abci	86.24% <ø> (-3.67%)	⬇️
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.92% <ø> (ø)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	48.41% <ø> (-0.24%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[thepastaclaw]

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.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 224704f and 61f871e.

📒 Files selected for processing (10)

• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
• packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
• packages/rs-platform-wallet-ffi/src/error.rs
• packages/rs-platform-wallet/src/error.rs
• packages/rs-platform-wallet/src/test_support.rs
• packages/rs-platform-wallet/src/wallet/core/broadcast.rs
• packages/rs-platform-wallet/src/wallet/core/transaction.rs
• packages/rs-platform-wallet/src/wallet/reservations.rs
• packages/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

[thepastaclaw]

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.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between ffc05fc and e4e6784.

📒 Files selected for processing (2)

• packages/rs-platform-wallet/src/wallet/core/broadcast.rs
• packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs

[thepastaclaw]

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.

[thepastaclaw]

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.

[thepastaclaw]

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.

[thepastaclaw]

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.

[thepastaclaw]

Preliminary review — Codex only

The guarded sample-and-install now closes the narrow torn-handoff race, but the bounded fence can still be consumed immediately by historical catch-up after the guard is released, allowing a DAPI-submitted transaction's input to be reselected. Two non-blocking quality issues also remain in the stringly typed conflict error and the probabilistic handoff regression test.
Source: Codex reviewer backend model gpt-5.6-sol (general, security-auditor, rust-quality, and 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 | 🟡 2 suggestion(s)

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/transaction.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/transaction.rs:434-442: Use a typed error for an in-broadcast input conflict
  An in-broadcast conflict is an expected transient domain condition carrying structured data—the conflicting `OutPoint`—but all three selection choke points reduce it to human-readable error strings. Tests consequently identify the condition with `message.contains("mid-broadcast")`, and Rust callers cannot distinguish a safe retry from unrelated transaction-building failures without parsing unstable prose. Add a dedicated `PlatformWalletError` variant carrying the outpoint and return it consistently from finalized-transaction, contact-payment, and asset-lock selection paths; presentation and FFI mapping can then be centralized.

In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:1168-1178: Make the manager-guard handoff regression deterministic
  This regression catches the former sample-to-settle gap only if the writer happens to observe a read guard held during a handful of synchronous instructions. Both execution before the sample and execution after the settle are accepted, while the `try_write` spin and 16 repetitions do not guarantee observation of the prohibited midpoint. The test can therefore remain green with the pre-fix implementation and does not reliably protect the invariant introduced by the latest commit. Add a test-only synchronization hook at the sample/settle boundary, or factor the guarded transition so the test can deterministically prove that a writer cannot acquire the manager lock before `settle_pending_spend` completes.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:234-242: Retain the input fence when dispatch outlives the reservation TTL
  (existing thread: https://github.com/dashpay/platform/pull/4309#discussion_r3771352689)
  `settle_dispatch_fence` now samples height H and installs `pending_until = H + IN_BROADCAST_FENCE_BLOCKS` while holding the manager read guard, which fixes the previous sample-to-install gap. It still does not preserve a full post-dispatch interval: after line 242 releases the guard, a synchronization writer queued during the short critical section—or ordinary catch-up completing before the next build—can immediately advance `last_processed_height` by the whole interval. The next build then calls `in_broadcast_conflict`, observes `current_height >= pending_until`, reaps the new fence, and reselects the input. Those elapsed heights may be historical blocks mined before the transaction was submitted, so they provide no evidence that the submitted transaction has been observed or dropped. This is unsafe for `DapiBroadcaster`, which returns after `sdk.execute` without injecting the transaction into local wallet state. The test at lines 1290-1308 explicitly permits the writer's advance to consume the entire bound, confirming that the current design protects only the instant of installation rather than the intended period after dispatch. Keep the fence until the spend is observed, inject accepted DAPI transactions into local pending state, or use an expiry clock that historical catch-up cannot fast-forward.

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The broadcast fence now survives historical catch-up, remains cancellation-safe, and is released when a spend is observed, while the new Rust error preserves the conflicting outpoint structurally. Two non-blocking issues remain: the handoff regression hook fires before the semantic handoff, and the native boundary flattens the new retryable conflict into an unknown error. Source: reviewers gpt-5.6-sol (general, security-auditor, and 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 — 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)

🤖 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/generation.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/generation.rs:547-562: Make the manager-guard handoff regression deterministic
  The replacement test synchronizes while `in_broadcast` is locked, but `fire_settle_boundary_hook()` runs before `dispatching` is decremented. The observer therefore reports `TransitionInProgress` solely because the lock was acquired, not because it observed the dispatching-to-pending boundary. An implementation that later performs the decrement and pending installation in separate critical sections can retain the hook in the first section and still pass: the observation completes before the unsafe free interval opens, and the final assertion sees the subsequently installed fence. Fire the hook at the semantic boundary after the dispatching hold is lifted and before the pending phase is opened, while the correct implementation still retains the same mutex across both operations.

In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:623-640: Preserve the in-broadcast conflict discriminator across native bindings
  `InputMidBroadcast` is a newly typed, expected transient condition for which retrying after the fence clears is safe, but this conversion deliberately maps it to `ErrorUnknown` (99). The JNI bridge consequently exposes code 1099 and Kotlin classifies it as `DashSdkError.PlatformWallet.Generic`, while Swift turns code 99 into `PlatformWalletError.unknown`. Native callers therefore cannot distinguish the safe wait-and-retry condition from an internal wallet failure without parsing the Rust display string, and the structured `OutPoint` is reduced to prose. Assign a dedicated stable FFI result code and mirror its retry semantics in the Swift and Kotlin result/error mappings.

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The exact-head implementation now performs the freshness check and installs the generation-level dispatch pin under the manager guard, releases that guard before network I/O, and preserves the input fence through cancellation, historical catch-up, and ambiguous submission until spend observation or the monotonic orphan timeout. No in-scope blocking issues or actionable suggestions remain; the dedicated native discriminator for InputMidBroadcast is intentionally deferred to the coordinated cross-language error-code allocation.
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 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)

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The age guard, owner-guarded cleanup, generation-level dispatch fence, and host-language error mappings are coherent, with no blocking defect found. One in-scope test-coverage gap remains: the release tests bypass the new SpendObservationHandler routing and wallet-map lookup, although the five targeted spend-observer projection tests pass. Source: reviewers gpt-5.6-sol; final verifier claude-opus-4-6. 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 — ffi-engineer (completed), gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed), gpt-5.6-sol — security-auditor (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)

🟡 1 suggestion(s)

🤖 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`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:835-851: Exercise the actual spend-observation handler
  `observe_via_event_handler` is described as driving the production `SpendObservationHandler` seam, but it calls `observed_spends` and then `WalletGeneration::observe_spent` directly. This bypasses `SpendObservationHandler::on_wallet_event`, `observing_wallet`, the `wallets.try_read()` branch, wallet-ID lookup, and selection of the registered generation. Consequently, the two headline release tests still pass if the handler routes an event to the wrong generation or fails its map lookup. Add a handler-level test backed by a real wallets map that invokes `on_wallet_event` and proves only the matching registered generation is released; a manager-level integration assertion can separately cover accidental omission from the event-manager handler list.

[bfoss765]

@HashEngineering requesting your review on this one — it's part of the Android-migration estate and is bot-clean/ready for human review. (GitHub won't accept a formal review request yet: your collaborator access on dashpay/platform hasn't been provisioned — flagged to be fixed alongside the #4449 team setup.)

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

At exact head efe7c73, the age guard, owner-guarded cleanup, dispatch fence, spend-observation release path, and native error mappings are coherent; the four targeted spend-observation and manager-wiring regression tests passed. The prior test-coverage finding is fixed, and no in-scope blocking issues or actionable suggestions remain.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model claude-opus-4-6. 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)

[thepastaclaw]

Final validation — Codex/Sol only (Phase 2 disabled)

The exact-head implementation atomically checks freshness and installs the generation-scoped input fence, releases the manager guard before network I/O, and preserves protection through cancellation and ambiguous submission until spend observation or the monotonic orphan timeout. No in-scope blocking issue or actionable suggestion remains; the supplied CodeRabbit comments describe fixed historical conditions or are acknowledgement/status replies rather than current findings.

Source: reviewer backend model gpt-5.6-sol; final verifier backend model claude-opus-4-6; 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)

[HashEngineering]

tACK, this has been tested several times for BIP70 payment. it works well.

[thepastaclaw]

Preliminary review — Codex only

The finalized/deferred dispatch path correctly performs its freshness check and installs a generation-scoped input fence before releasing the manager guard. Two blocking gaps remain: immediate contact-payment and asset-lock broadcasts do not fence their own selected inputs, and the one-hour orphan timeout releases possibly submitted transactions without evidence that they can no longer be relayed. Source: reviewers gpt-5.6-sol; final verifier claude-opus-4-6; 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)

🔴 2 blocking

🤖 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/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:1325-1341: Fence newly selected immediate sends before releasing the manager lock
  This new check prevents the contact-payment build from consuming an input fenced by another dispatch, but the newly selected transaction never installs a fence of its own. After the manager write guard is released at the end of this block, persistence and `self.broadcaster.broadcast(&tx).await` run without a generation pin. If catch-up advances by key-wallet's 24-block reservation TTL while the broadcaster is suspended, another build can sweep and reserve the same input; because the original transaction has no fence, this check passes for the competing build, after which the original future can resume and submit its already-signed transaction. The asset-lock flow has the same asymmetry after its conflict check at `wallet/asset_lock/build.rs:236` and before its direct broadcast at line 987. Install the transaction's pin atomically with its fresh reservation while the manager guard is still held, carry it through pre-send work and the broadcaster await, release it only on a definitive pre-send failure, and retain it for accepted, ambiguous, cancelled, or unwound dispatches until a safe settlement condition is established.

In `packages/rs-platform-wallet/src/wallet/core/generation.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/generation.rs:243-248: Elapsed time can retire a fence while the signed transaction remains valid
  `open_pending` gives every accepted, ambiguous, cancelled, or unwound dispatch a one-hour deadline, and `in_broadcast_conflict` later removes the fence solely because that deadline elapsed. Elapsed time does not invalidate the signed transaction or prove that no peer retained it. A malicious or isolated DAPI endpoint can receive the transaction while withholding it from the wallet and network, or a mobile wallet can remain backgrounded for more than an hour. Once catch-up also causes key-wallet's reservation to be swept, the next build prunes this fence and signs a conflicting transaction; the retained original can still be broadcast afterward, allowing either user intent to win the double-spend race. Non-rejected transactions must remain fenced until spend or replacement evidence, or an explicit abandon/replacement protocol, establishes that reselection is safe. A liveness path can persist and query/rebroadcast the pending transaction, but a timeout alone cannot make its inputs safe to reuse.

[thepastaclaw]

Preliminary review — Codex only

The two prior blockers are fixed: immediate contact-payment and asset-lock sends now pin their own selections, and elapsed time no longer retires pending-spend fences. Three new blocking lifecycle defects remain: cancellation during asset-lock pre-send work strands inputs permanently, generation recreation discards protection for possibly live transactions, and contact-payment rejection lowers the fence before an unconditional cleanup that can erase a newer reservation. Several comments and the public ambiguous-broadcast error still describe the deleted timeout behavior.
Source: reviewers gpt-5.6-sol and claude-opus-4-6; final verifier claude-opus-4-6. 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 — rust-quality (completed), gpt-5.6-sol — ffi-engineer (completed), gpt-5.6-sol — security-auditor (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 3 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/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:1017-1022: Release the asset-lock pin when cancellation happens before broadcast
  `build_asset_lock_transaction_fenced` creates `in_broadcast_pin` in its conservative pending-on-drop state, but this function subsequently awaits `persist_asset_lock_account_pools()` here and `track_asset_lock()` at lines 1060-1071 before reaching the broadcaster at line 1096. Cancelling the future during either pre-send await drops the pin as `Pending`, even though no transaction was submitted. Because no spend can ever be observed for that transaction, the evidence-only fence never clears; after key-wallet sweeps the reservation, every later build selecting the input is refused with `InputMidBroadcast` for the remainder of the process. Keep the dispatching hold active during pre-send work, but make pre-broadcast drop release it; arm pending-on-drop immediately before entering `broadcast`, where cancellation must remain conservative.

In `packages/rs-platform-wallet/src/wallet/core/generation.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/generation.rs:161-163: Preserve pending-spend protection across wallet recreation
  The evidence-only fence is explicitly process- and generation-local. `load_from_persistor` constructs a fresh `WalletGeneration` with an empty fence map, and `pins_do_not_cross_generations` confirms that replacing the generation immediately permits the same outpoint. After an accepted or `MaybeSent` DAPI broadcast that has not been observed locally, a restart or remove-and-recreate operation therefore restores the persisted UTXO without either the fence or key-wallet's memory-only reservation. The original signed transaction can still be retained and later relayed by a DAPI endpoint or peer, while the restored wallet can sign a conflicting intent. Synchronization alone does not close this when the original transaction is withheld until after the new selection. Persist pending transactions or their outpoints and rehydrate the fences before spending is enabled, or resolve each pending transaction through query, rebroadcast, replacement, or explicit abandonment before allowing restored inputs to be selected.

In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:1421-1434: Keep the contact-send fence up through reservation cleanup
  The definitive-rejection arm removes the fence before awaiting reservation cleanup. This path passes no owner token, so `release_reservation_after_rejected_broadcast` performs an unconditional `release_reservation`. If catch-up swept the first send's reservation while its broadcaster was suspended, a finalized-transaction build already queued on the manager write lock can run after `settle_released()`, reserve the same input, pass the now-absent conflict check, and release the manager lock while its external signer is pending. The cleanup then acquires the manager read lock and deletes that newer reservation. Because finalized builds do not install a pin until broadcast, another finalization can reserve and sign the same input, producing two fresh conflicting handles. Perform the unconditional cleanup while the original fence remains active, then release the pin; a queued build that runs first will still encounter the fence and roll back its own selection.

In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:105-118: Remove obsolete timeout guarantees from fence documentation
  `TransactionBroadcastUnconfirmed` still promises reconciliation through the reservation TTL, but sweeping the key-wallet reservation no longer releases the generation-level pending-spend fence. The same obsolete contract appears in `wallet/core/broadcast.rs:148-152`, which says the fence answers to a monotonic clock; `wallet/core/generation.rs:775-783`, which describes installing an `Instant::now` orphan deadline; and `wallet/core/spend_observer.rs:77-80`, which says a dropped observation eventually reaches an orphan backstop. Commit `6c4ea7736d` removed every elapsed-time release. Update these sites to state the actual contract: a non-rejected send remains fenced until observed-spend evidence, an explicit definitive pre-send release, or generation teardown.

[thepastaclaw]

🔴 Blocking: Preserve pending-spend protection across wallet recreation

The evidence-only fence is explicitly process- and generation-local. load_from_persistor constructs a fresh WalletGeneration with an empty fence map, and pins_do_not_cross_generations confirms that replacing the generation immediately permits the same outpoint. After an accepted or MaybeSent DAPI broadcast that has not been observed locally, a restart or remove-and-recreate operation therefore restores the persisted UTXO without either the fence or key-wallet's memory-only reservation. The original signed transaction can still be retained and later relayed by a DAPI endpoint or peer, while the restored wallet can sign a conflicting intent. Synchronization alone does not close this when the original transaction is withheld until after the new selection. Persist pending transactions or their outpoints and rehydrate the fences before spending is enabled, or resolve each pending transaction through query, rebroadcast, replacement, or explicit abandonment before allowing restored inputs to be selected.

_{source: ['codex']}

[bfoss765]

Fixed the in-process half at 9e19d23; the restart half is NOT closed and I am leaving this thread open for it.

WHAT CHANGED. The fence map moved out of WalletGeneration into InBroadcastFences, an Arc the manager keys by wallet_id and hands to every generation registered under that id (in_broadcast_fences_for, used by both register_wallet and load_from_persistor). WalletGeneration::with_fences is now the production constructor; new() keeps its meaning of "a wallet with no predecessor".

Rationale, since this splits two things that used to be one object: the balance and the lifecycle gate genuinely describe one instance and must not cross a recreation. A fence does not — it describes a transaction that may be live on the network, and that does not become invalid because the wallet object holding its record was replaced. The registry is deliberately never pruned on removal, because a removal is exactly when the protection has to survive.

Everything else is unchanged: released only by observe_spent, never by anything that elapses, and an observation on the new generation clears what the old one installed because both name the same map. Inheritance is strictly the conservative direction.

RED/GREEN.

• manager::wallet_lifecycle::register_wallet_duplicate_tests::a_recreated_wallet_inherits_the_pending_fences_of_the_generation_it_replaces — create, settle a dispatch into the pending-spend phase, remove_wallet, re-create from the same seed, assert the replacement still refuses the outpoint. RED before: left: None (no conflict at all). GREEN after.
• …::fences_do_not_leak_between_different_wallets — isolation half, green throughout.
• generation::tests::pins_cross_generations_of_the_same_wallet replaces pins_do_not_cross_generations. You cited that test as evidence of the bug and you were right — the assertion itself was the defect. The replacement also pins that an observation on the new generation clears the inherited fence. pins_do_not_cross_between_wallets keeps the property the old test was actually worth having.

WHAT IS STILL OPEN. The registry is process-lifetime, so a restart still loads the persisted UTXO with no fence on it. I did not implement that half, deliberately, because both viable shapes change host-visible state and neither is a review-round-sized change:

1. Record the dispatched transaction locally at the point the pending phase opens, which is what the SPV path already gets for free from dash-spv's mempool injection. PlatformWalletInfo already implements WalletTransactionChecker, so check_core_transaction(tx, Mempool, …) under the manager write lock yields the records, and CoreChangeSet::records / spent_utxos are already persisted by every host — so this needs NO new persistence surface, and it unifies the DAPI path with the SPV one rather than adding a parallel mechanism. It does change balance semantics (an unconfirmed send would leave the selectable set immediately, which is arguably the bug behind this one).
2. A dedicated pending-spend table, rehydrated before spending is enabled. Smaller in semantics, larger in surface: new FFI callbacks plus Room/CoreData schema work in each host.

I would rather propose (1) as its own change than land a half-verified balance-semantics shift inside a review round. The requirement and both options are recorded in the InBroadcastFences docs so the gap cannot be lost, and the standing invariant is untouched: nothing that merely ELAPSES may retire a fence.

[HashEngineering]

tACK