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.

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-based finalize_transaction signature.

---

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 pinned
handle had no age guard, so a long-held handle could broadcast against
funding inputs that key-wallet's ReservationSet TTL sweep may already have
released 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:

• Shared bound. RESERVATION_MAX_AGE_BLOCKS (20; key-wallet TTL 24) and
  reservation_expired() are hoisted to wallet::reservations, so the
  registry and the finalized-handle path measure a reservation's age against
  the same number.
• Guarded op. broadcast_finalized_transaction refuses — before
  touching the broadcaster — once current_height − reservation_height >= the
  shared bound, using the reservation's own stamp height already carried on
  SignedCoreTransaction::reservation_height. The check runs after the
  existing 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 ownership
  transferred) frees the still-owned inputs for the instructed immediate
  rebuild.
• Error code. A new token-less PlatformWalletError::StaleReservation
  reuses the existing FFI ErrorStaleReservationToken (34); no new code is
  allocated. Reuse is documented on both sides.
• Abandon/free release owner-guarded at any age — only a token-less build
  (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 StaleReservation
and 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

  • Finalized transactions with stale funding reservations are rejected before broadcast, preventing unintended network submissions.
  • Clear stale-reservation errors explain when a payment must be rebuilt and confirm that no network call occurred.
  • Abandoning aged transactions no longer releases reservations that may belong to newer transactions.
  • Deferred payment tokens and finalized transaction handles now share consistent stale-reservation behavior.

• Documentation

  • Clarified reservation expiration, cleanup, broadcast rejection, and transaction rebuilding behavior.

• Tests

  • Added coverage for fresh, boundary, and expired reservations, including cleanup, rebuilding, and repeated release scenarios.

[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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding age guards to finalized-transaction handle broadcasts.
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.

✨ 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]

✅ Final review complete — no blockers (commit 520e5d9)

[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 86.16%. Comparing base (6495991) to head (520e5d9).
⚠️ Report is 18 commits behind head on v4.2-dev.

Additional details and impacted files

@@             Coverage Diff              @@
##           v4.2-dev    #4309      +/-   ##
============================================
+ Coverage     84.74%   86.16%   +1.42%     
============================================
  Files          2711     2731      +20     
  Lines        357138   352360    -4778     
============================================
+ Hits         302668   303624     +956     
+ Misses        54470    48736    -5734     

Components	Coverage Δ
dpp	88.96% <ø> (+3.63%)	⬆️
drive	84.21% <ø> (+0.41%)	⬆️
drive-abci	89.19% <ø> (+2.53%)	⬆️
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.92% <ø> (ø)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	47.14% <ø> (+8.18%)	⬆️

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

[bfoss765]

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]

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.

[thepastaclaw]

🔴 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']}

[bfoss765]

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 broadcast returns.
• pending-spend — installed on drop when the dispatch returned anything but BroadcastError::Rejected, bounded at dispatch_height + IN_BROADCAST_FENCE_BLOCKS where IN_BROADCAST_FENCE_BLOCKS = 24 — key-wallet's own RESERVATION_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 ReservationSet and RESERVATION_TTL_BLOCKS are private at rev 173ffac. 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 Rejected frees the inputs at dispatch return, per your "only a definitive pre-send rejection" note. MaybeSent stays 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's spent_utxos and 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_returns dispatches 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 the retain_pending_spend call 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_fence proves the rejection path still frees immediately.
• The existing in_broadcast_pin_blocks_reselection_until_dispatch_returns keeps 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.

[bfoss765]

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.

[bfoss765]

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.

[bfoss765]

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.

[bfoss765]

You're right, and I've fixed it in 7dde1c1.

I had the sample under the guard but not the install. The let height = { let wm = …read().await; … }; block ended the guard's scope, and anchor_pending_spend + drop(in_broadcast_pin) — the statements that actually write the bound into WalletGeneration and release the dispatching hold — ran after it. A manager writer queued behind that guard is woken the instant it drops, so on another worker it could advance last_processed_height before the resuming task reached the install, and the fence would land anchored on a height the wallet had already left, in the same breath as the dispatching hold going away.

The transition is now one critical section. CoreWallet::settle_dispatch_fence takes the read guard, samples last_processed_height, settles the pin, and only then drops the guard, so no height writer can interleave between the reading and the install.

On the deadlock risk you flagged: the pin's Drop does not take the manager lock — it calls WalletGeneration::unpin_in_broadcast, which takes only the in_broadcast std::sync::Mutex for a few hash operations and never awaits. That is the crate's existing order (manager lock, then in_broadcast, exactly as every in_broadcast_conflict call site takes them under the manager write guard), so settling under the read guard cannot invert it. I still restructured to a consuming InBroadcastPin::settle_pending_spend(Option<u32>) — plus settle_released for the definitive-rejection path, which needs no guard and no height — rather than leaving an end-of-scope drop, because pin was a &mut borrow and the drop could not sit inside the guard scope at all. Drop is unchanged and stays the unanchored fallback the cancellation design relies on.

Regression test settle_does_not_interleave_with_a_parked_height_writer parks a manager writer across the post-await section, has it advance a full IN_BROADCAST_FENCE_BLOCKS, and records the fence state at the instant it is granted the lock. Two interleavings are legal — granted after the handoff (must see the pin settled, with a bound still ahead of the height it holds) or granted before the dispatch sampled (the dispatch then reads the advanced height, so the bound must run from that advance) — and the torn case, pin still dispatching while the installed bound sits below the advance, fails. It repeats 16 times because the interleaving is scheduler-dependent.

One thing I'd rather state than overclaim. With the sample and the settle adjacent there is no suspension point between them, so the window is a few instructions wide and I could not make the test lose that race against the pre-fix code — it passes 10/10 there. And for this interleaving the end state is the same either way: pending_until is sample + FENCE and blocks() is evaluated at the querying build's height, so a writer that advances a full interval lapses the fence whether it does so before or after the install, and no build can run in between because it needs the write lock that writer holds. What the guard buys is that the anchor is provably last_processed_height at the install — the fence is never born dead — and that the handoff cannot be observed half-done. It also forecloses the version of this that is genuinely exploitable: anything that suspends between the sample and the settle makes the window schedulable, and the test fails 6/6 runs against a shape with a single yield_now().await there, while passing 5/5 with the guard held.

cargo test -p platform-wallet is green (695 lib + 9), as are cargo fmt --check and cargo clippy --all-targets -- -D warnings.

[bfoss765]

You're right, and this time the correction is to the shape of the fix rather than to where I sampled a height.

I had been treating this as an anchoring problem for three rounds — pre-await, post-await, post-await under a held guard — and each time I moved the sample and argued the bound was now measured from the right moment. Your point is that the bound itself is the defect: last_processed_height is not a clock during catch-up. The wallet can advance it by thousands of blocks in seconds, and those blocks were mined before the transaction was submitted, so they carry no information about whether it has been seen or dropped. installed_height + N is therefore not a weak bound that better sampling could rescue — it is a bound denominated in the wrong quantity, and an ordinary historical sync clears it no matter where the anchor came from.

Fixed in adb65b4, which is your option (a) with the (c) backstop.

Release on evidence. The pending-spend phase now ends when the wallet observes the outpoint spent — WalletGeneration::observe_spent. Two shapes release it, for the same reason: the dispatch's own transaction appearing (in the mempool or in a block), or a competing transaction spending the outpoint. After either, the outpoint has left this wallet's selectable set and there is no re-selection left that could race anything on the wire.

The observation comes from the hook I offered earlier and you preferred: a new SpendObservationHandler on the wallet-event fan-out, projecting TransactionDetected / BlockProcessed records through spent_outpoints — the same per-record input walk that produces CoreChangeSet::spent_utxos. Both consumers share one definition of "we spent this", so the fence and the persisted spent set cannot drift apart. It is registered next to BalanceUpdateHandler and takes the same wallets map for the same reason (the event fires inside SPV's block-processing write section, so the generation can't be resolved through the manager lock). A dropped observation only defers to the backstop — it can delay a release, never cause one.

Backstop on a monotonic clock. IN_BROADCAST_FENCE_BLOCKS (24 blocks) is replaced by IN_BROADCAST_FENCE_ORPHAN_TIMEOUT, one hour on std::time::Instant. I picked wall-clock over the alternative you'd also accept — counting only blocks processed after installation, filtered by timestamp — because Instant is the only clock in this crate with no chain input at all. Catch-up, a re-org, a peer feeding historical headers and a system clock adjustment are all incapable of moving it, so it satisfies (c) structurally rather than by a filtering rule that has to be got right. A post-install block counter would also need block timestamps threaded into the fence and would inherit their ±MTP slack.

To be explicit about what it is and isn't: this is not spend evidence and I'm not claiming it as such. All the safety comes from the observed-spend release. The backstop exists only so a transaction that is never observed at all (evicted for fee, conflicted away) can't hold its inputs for the life of the process. One hour is the real-time analogue of key-wallet's RESERVATION_TTL_BLOCKS (24 blocks ≈ 1 h at the 2.5-minute mainnet target), so the residual exposure for an unobserved transaction is the magnitude the reservation TTL already accepts — the clock changed, not the budget. It's also short enough to matter within a session: the fence is in-memory and never persisted, so a much longer bound would make process restart the real recovery path for stranded inputs.

What this deleted. Instant::now() needs no lock, no guard and no await, so it's readable from a synchronous Drop. That removed the machinery the height version needed:

• the anchored/unanchored split and InBroadcastFence::anchor — the cancellation path no longer needs a special case, because Drop stamps its own deadline exactly like the returning path;
• the current_height parameter on in_broadcast_conflict — the fence has no way to consult the chain clock now, which is the property you're asking for;
• settle_dispatch_fence and its post-await manager read guard, added last round. Its entire job was making the sample and the install atomic against height writers; with no height sampled there's nothing to interleave with, and the deadline is computed inside the same in_broadcast critical section that installs it. That also takes one manager lock acquisition off every dispatch.

Reproduction — this is the part I'd point at. fence_survives_a_full_historical_catch_up_advance is your scenario literally: AlwaysOkBroadcaster (accepted, so certainly on the wire), a manager with no mempool pipeline (the DAPI shape), then a 17,000-block catch-up applied between the dispatch returning and the next build.

I ported it back to each earlier revision to check it actually discriminates, rather than just asserting the new behaviour. It fails on all four:

revision	result
2b911bc (fence added)	FAILED
89586f7 (fence by default)	FAILED
58efacf (post-await anchor)	FAILED
7dde1c1 (guarded sample+install)	FAILED

Each failure is a fully signed competing transaction spending the same outpoint, with in_broadcast: Mutex { data: {} } in the dump — the fence map emptied by the catch-up. That's the double-spend, not a missing assertion, and it confirms the guarded handoff from last round fixed a real but much narrower race while leaving this one wide open.

Also covered: release on the dispatch's own observed spend and on a competing spend (both driven through the real event projection, in the post-TTL-sweep window where the fence is the only thing holding the input); the backstop freeing an unobserved fence; and the backstop being immune to a 17,000-block advance while still expiring on its own clock.

cargo test -p platform-wallet --features shielded is green (869 lib + 9), as are cargo fmt --check and cargo clippy --all-targets -- -D warnings.

[thepastaclaw]

Resolved in 2b911bc — Retain the input fence when dispatch outlives the reservation TTL 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.}

[thepastaclaw]

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.

[thepastaclaw]

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.

[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]

🟡 Suggestion: 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.

_{source: ['codex']}

[bfoss765]

Done in adb65b4. PlatformWalletError::InputMidBroadcast { outpoint: OutPoint } now carries the conflicting outpoint structurally, and all three selection choke points return it — finalize_transaction, the contact-payment build, and the asset-lock build, which previously split the same condition across TransactionBuild and AssetLockTransaction strings. The tests match the variant instead of message.contains("mid-broadcast"); expect_mid_broadcast returns the outpoint, so several of them now also assert which input was refused, which the string form couldn't check.

One deliberate limitation. The FFI code is unchanged: all three call sites already fell through to ErrorUnknown (neither TransactionBuild nor AssetLockTransaction is matched in the blanket impl), and minting a dedicated numeric code means claiming a value in the cross-PR registry and mirroring it into the Swift and Kotlin result enums — more surface than I wanted to add to this PR while the fence blocker was open. What I did add is an explicit, documented match arm for the new variant, so the mapping is a reviewable decision in one place rather than an accident of the catch-all, and becomes a one-line change when a code is claimed. Happy to do that here instead if you'd rather it land together.

[thepastaclaw]

Resolved in adb65b4 — Use a typed error for an in-broadcast input conflict 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.}

[thepastaclaw]

🟡 Suggestion: 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.

_{source: ['codex']}

[bfoss765]

Agreed, and you were right that it could stay green on pre-fix code — I'd said as much about the narrow window in my last reply and should have drawn the conclusion you did. Replaced in adb65b4.

Two things changed. First, the invariant is smaller: with the fence no longer carrying a height, there is no manager-guarded sample to protect, so the property under test is now just that lifting the dispatching hold and opening the pending-spend phase are not separately observable.

Second, the test no longer races the scheduler for it. WalletGeneration::on_next_settle_boundary is a test-only one-shot hook fired inside unpin_in_broadcast with the in_broadcast lock held, at exactly the midpoint. The observer thread is woken by the hook and the settling thread blocks until the observer publishes what it saw, so the observation is guaranteed to be taken mid-transition rather than merely likely to be.

The observer probes with try_lock (try_probe_in_broadcast) rather than reading normally, because a blocking read can't state the property: correct code holds the lock across the whole transition, so a blocking observer always sees the finished state and can never distinguish "held throughout" from "granted afterwards". TransitionInProgress is the legal outcome; Free is the failure, and Free is exactly what a split transition exposes. The test then asserts the end state is a live fence, so it can't pass by the transition simply vanishing.

Verified both ways rather than assumed: against an implementation that splits unpin_in_broadcast into two critical sections it fails 10/10, on the first attempt each time and with no repetition loop; against the committed implementation it passes 10/10. The 16-repetition loop and the try_write spin are gone.

[thepastaclaw]

Resolved in adb65b4 — Make the manager-guard handoff regression deterministic 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.}

[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]

🟡 Suggestion: 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.

_{source: ['codex']}

[bfoss765]

Applied in 520e5d9. fire_settle_boundary_hook() now runs between the dispatching decrement and the pending install — inside the torn state — instead of on lock acquisition, and the on_next_settle_boundary / unpin_in_broadcast docs now state that firing-point contract explicitly.

Verified the redesign restores discriminating power by mutation: splitting unpin_in_broadcast into two critical sections (a decrement section, then a pending-install section) with the hook honored at the semantic boundary between them now fails the_settle_handoff_is_never_observable_half_done with an observed Free — exactly the exposure you described — whereas that same split passed against the previous firing point. All 15 wallet::core::generation tests green (50/50 across wallet::core::).

[thepastaclaw]

Resolved in 520e5d9 — Make the manager-guard handoff regression deterministic 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.}

[thepastaclaw]

🟡 Suggestion: 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.

_{source: ['codex']}

[bfoss765]

Declining the in-PR code allocation — this is the deliberate deferral the arm's comment documents, and it stays that way for three reasons:

1. The numeric space is a cross-PR registry, and minting here re-opens the churn it exists to end. Codes are coordinated through ERROR_CODE_REGISTRY.md (docs(platform-wallet): error-code registry for the FFI result space #4261): this branch's claim table already tracks sibling PRs holding 29/31/32/33 and 37–41, and the mirrors are already ahead of this branch (Kotlin's fromPlatformWalletNative maps 41 -> PlatformShieldCapacityExceeded, a code this branch's Rust never emits). A unilateral claim from this PR is exactly how the 26–28 → 27/28/30 → 34–36 renumbering happened. The InputMidBroadcast allocation is registered as a follow-up against the docs(platform-wallet): error-code registry for the FFI result space #4318 registry; when it lands, this arm is the intended one-line change plus the two mirror entries.

2. The discriminator is retry-UX, not fund safety, so deferring it is sound. InputMidBroadcast is raised by the build-time choke points before signing or dispatch — nothing touched the network and no state was consumed. A host that classifies it as Generic/unknown and falls back to its generic failure path (rebuild) is safe: the rebuild either reselects around the fence or is re-refused while the fence lives. Contrast ErrorStaleReservationToken (34), which did warrant typed carriage in this PR because misclassifying it invites an unsafe act on a possibly-swept reservation.

3. The no-new-code alternative was considered and is worse. A stable machine marker at position 0 of the message (the DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX pattern) would let Kotlin/Swift classify without a numeric claim — but this very surface is actively deprecating marker sniffs in favor of typed codes (fix(kotlin-sdk): android host-app integration fixes (key security policy, unmanaged-identity reads, typed signing error) #4060 finding 7; the Kotlin catch-all's marker sniff is @Deprecated, scheduled for removal). Shipping a new marker contract now would either become permanent legacy or force hosts through a second migration when the registry code lands.

Meanwhile the structured OutPoint remains available to every in-process caller through the typed Rust variant; only the FFI message renders it as prose, which the dedicated code will fix once allocated.

[thepastaclaw]

Resolved in this update — Preserve the in-broadcast conflict discriminator across native bindings 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.}

[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)