feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission

[bfoss765]

Splits transaction build from broadcast for BIP70-style deferred submission: a signed-payment registry with reservation release, a deferred-payment token bounded to the reservation's lifetime, native code 26 for stale reservation tokens, token sweeping only when the final wallet write wins, and routing of deferred builds through the atomic finalize-and-register path — across rs-platform-wallet, platform-wallet-ffi, rs-unified-sdk-jni, and the Kotlin SDK surface.

Re-opens #4090 which was auto-closed when the #3999 base branch was deleted; rebased onto v4.1-dev. All seven original commits replayed cleanly — no hunks needed to be dropped as already-absorbed.

Verified: cargo test -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni all pass (504 / 229 / 10); :sdk:assembleRelease + sdk unit tests pass.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added support for deferred signed payments, allowing transactions to be built and signed, then broadcast or released later.
  • Added payment details including transaction ID, raw transaction data, fees, and reservation tokens.
  • Added idempotent reservation release for abandoned payments.
• Bug Fixes
  • Added clear, non-retryable handling for expired, invalid, already-used, or wallet-mismatched reservation tokens.
  • Improved cleanup when wallets are closed while deferred payments remain active.

---

Why the token registry instead of the V2 handle surface

The deferred BIP70/BIP270 flow uses the reservation-token registry rather than the V2 finalized-transaction handle for two concrete reasons. First, ownership and cleanup: the token is wrapped in an owning, AutoCloseable Kotlin object with a GC/Cleaner backstop, so a payment that is signed but then abandoned — the merchant server never acks, the user backs out, or the coroutine is cancelled after the native registration returned — always releases its funding reservation, for free, without the caller having to remember to abandon a handle. Second, the token path carries a lifetime bound the V2 handle does not: it stamps each token with the reservation's own pre-signing height and refuses to act once that reservation could have aged into key-wallet's TTL sweep, so a slow external signer can never let a stale token spend against an outpoint the wallet already swept and re-selected. A pinned V2 CoreWallet handle has no such age guard and would keep the old wallet actionable indefinitely. Both paths now share one wallet-generation identity and one teardown policy, so the V2 surface stays correct for the immediate send it was built for while the deferred flow gets the GC-safe, age-bounded ownership it needs. A follow-up adds the age guard to the V2 handle path itself (it becomes live the moment iOS does deferred sends).

Review-response summary (2026-07-21)

All five lifetime findings addressed as merge blockers, one commit each, with regression tests:

1. Destroy vs teardown: final-alias platform_wallet_destroy releases the generation's reservations against the still-live wallet; actual generation removal drops tokens and V2 handles — token cleanup is now tied to wallet-generation removal.
2. Height carry: the pre-signing reservation height travels on SignedCoreTransaction and register uses it — no post-signing resample; boundary test pins the TTL margin.
3. One generation identity: CoreWallet::is_same_generation (per-generation identity) is checked by BOTH the V2-handle and registry-token paths, with one teardown policy.
4. Cancellation-safe ownership: SignedCoreTransaction is an owning AutoCloseable with a NativeCleaner backstop; round-2 adds object-owning broadcastSigned/releaseReservation overloads that hold the object reachable across the native call and disarm the backstop on consumption (the bare-token forms remain but document the reachability requirement).
5. Validate-under-lock: broadcast peeks and consumes atomically under one lock hold (network I/O outside the lock); a wrong-wallet caller leaves the owner's token untouched, pinned by test.

Also per review: the dead core_wallet_signed_payment_register four-layer chain is deleted; error code 26 is split into typed siblings 26 StaleReservationToken / 27 ReservationTokenConsumed / 28 ReservationWalletMismatch (Kotlin-only host impact — Swift's enum never mapped 26 and falls through to unknown safely; codes 29/30 are used by the asset-lock PR #4184, coordinated to avoid collision); the stale buildSignedPayment KDoc is fixed.

Local test evidence (fork PRs skip the Rust CI suite): platform-wallet --lib 508 passed, platform-wallet-ffi --lib 197 passed, clippy/fmt clean, Kotlin :sdk:testDebugUnitTest green.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds deferred Core signed-payment flows across the Rust wallet, FFI, JNI, and Kotlin SDK layers. Payments can be built and reserved, then broadcast or released using opaque reservation tokens, with lifecycle validation and stale-token error mapping.

Changes

Deferred payment registry and wallet lifecycle

Layer / File(s)	Summary
Reservation registry and wallet lifecycle packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs, packages/rs-platform-wallet/src/wallet/core/*, packages/rs-platform-wallet/src/test_support.rs	Adds token-backed signed-payment registration, wallet identity checks, reservation age handling, broadcast/release operations, wallet cleanup, and registry tests.

Native signed-payment FFI

Layer / File(s)	Summary
Native registration and finalization packages/rs-platform-wallet-ffi/src/core_wallet/*, packages/rs-platform-wallet-ffi/src/error.rs	Adds native registration, atomic fund/reserve/sign/finalize, broadcast, release, and stale-token error handling.
Wallet alias cleanup packages/rs-platform-wallet-ffi/src/wallet.rs, packages/rs-platform-wallet-ffi/src/handle.rs	Sweeps deferred-payment entries only when the final wallet alias is destroyed and tests alias behavior.

JNI and Kotlin integration

Layer / File(s)	Summary
JNI bridge operations packages/rs-unified-sdk-jni/src/wallet_manager.rs, packages/kotlin-sdk/sdk/src/main/kotlin/.../ffi/WalletManagerNative.kt	Connects native operations to Kotlin and marshals signed-payment blobs, transaction bytes, fees, tokens, and txids.
Kotlin deferred payment API packages/kotlin-sdk/sdk/src/main/kotlin/.../wallet/*, packages/kotlin-sdk/sdk/src/main/kotlin/.../errors/DashSdkError.kt, packages/kotlin-sdk/sdk/src/test/kotlin/.../DashSdkErrorTest.kt	Adds signed transaction decoding, deferred build/broadcast/release methods, builder ownership fencing, and stale-token error mapping coverage.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

• dashpay/platform 4089: Covers the split build/broadcast API and reservation lifecycle implemented by this change.
• dashpay/platform 4079: Relates to exposing signed Core transaction details, including raw bytes and fees, through ManagedPlatformWallet.

Possibly related PRs

• dashpay/platform#4181: Directly overlaps the platform-wallet error-code 26 changes in the FFI error mapping.

Suggested reviewers: shumkov, lklimek, llbartekll, zocolini, quantumexplorer

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: splitting build and broadcast for deferred BIP70-style submission with reservation release support.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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]

⛔ Blockers found — Sonnet deferred (commit 440897c)
Canonical validated blockers: 2

[shumkov]

The core design is right (atomic finalize-and-register closes the double-selection race; reservation lifecycle is leak-free and the test matrix is strong). But two structural asks before merge:

• core_wallet_signed_payment_register → coreWalletRegisterSignedPayment → WalletManagerNative → ManagedCoreWallet.registerSignedPayment is a dead four-layer chain with zero callers after the finalize routing — and it's the unsafe variant (its age guard baselines at registration time, so the TTL protection is structurally defeated). Please delete it (or state explicitly why it stays), especially given refactor(sdk): dedup shared wallet code + remove dead FFI/JNI chains #4106 just removed this class of dead chains.
• The FFI now has two parallel deferred-tx lifecycles: the V2 handle surface Swift uses (core_wallet_tx_builder_finalize/broadcast/abandon_v2) and this token registry. There are real reasons to prefer the token here (V2's GC-backstop free releases the reservation; V2 has no age guard) — but they're stated nowhere, and V2 retains exactly the stale-release hazard this PR defends against. Please add the why-not-V2 rationale to the PR body and file a follow-up for the V2 age guard (it becomes live the moment iOS does deferred sends).

Minor: error code 26 conflates already-consumed (possibly paid!) / wallet-mismatch / aged-out — a payment UX can't tell "maybe paid" from "definitely not"; at minimum fix the doc, ideally split. Stale KDoc on buildSignedPayment still describes the pre-finalize shape. No Swift bindings for the new surface — fine, but track it.

[bfoss765]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt (1)

58-75: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Deprecate or remove the legacy registerSignedPayment bridge. It has no Kotlin callers in this repo, so keeping it unmarked only leaves a dead ABI surface in place. If it must remain for compatibility, add @Deprecated and point docs to the atomic finalizeSignedPayment flow.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`
around lines 58 - 75, Deprecate the internal registerSignedPayment bridge
because it has no Kotlin callers and exposes a legacy ABI surface; if
compatibility requires retaining it, add `@Deprecated` and update its KDoc to
direct callers to the atomic finalizeSignedPayment flow, otherwise remove the
method.

🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- Around line 249-257: Update the KDoc paragraph for the method containing
finalizeSignedPayment to describe the atomic finalizeSignedPayment flow instead
of the deprecated new/addOutput*/setFunding/buildSigned sequence. State that
finalizeSignedPayment atomically selects, reserves, signs, and registers the
inputs, while preserving the existing explanation that broadcastSigned and
releaseReservation use the resulting token.

In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 171-179: Update the documentation for ErrorStaleReservationToken
to explicitly include SignedPaymentError::StaleReservationToken alongside
StaleToken and WalletMismatch, and distinguish the unknown/consumed-token,
wrong-wallet-instance, and aged-out reservation cases with their respective host
semantics. Review the broadcast handler’s mapping and error details so hosts can
determine whether the reservation expired versus was consumed or belongs to
another wallet, without changing the shared error code.

---

Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`:
- Around line 58-75: Deprecate the internal registerSignedPayment bridge because
it has no Kotlin callers and exposes a legacy ABI surface; if compatibility
requires retaining it, add `@Deprecated` and update its KDoc to direct callers to
the atomic finalizeSignedPayment flow, otherwise remove the method.

🪄 Autofix (Beta)

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

Run ID: 038310fd-6fae-4081-961e-4fe849c78f63

📥 Commits

Reviewing files that changed from the base of the PR and between 8b466ab and 32cd702.

📒 Files selected for processing (19)

• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt
• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt
• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt
• packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt
• packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs
• packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs
• packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
• packages/rs-platform-wallet-ffi/src/error.rs
• packages/rs-platform-wallet-ffi/src/handle.rs
• packages/rs-platform-wallet-ffi/src/wallet.rs
• packages/rs-platform-wallet/src/lib.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/wallet.rs
• packages/rs-platform-wallet/src/wallet/mod.rs
• packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
• packages/rs-unified-sdk-jni/src/wallet_manager.rs

[shumkov]

Additional/strengthened lifetime findings after checking the existing threads:

1. Final-alias destruction consumes registry tokens without releasing live reservations. platform_wallet_destroy calls remove_entries_for_wallet, which only drops entries. Destroying the last wrapper alias does not necessarily remove the logical wallet from its manager, so the same wallet can be handed out again while those inputs remain reserved until TTL. Token cleanup should be tied to actual wallet-generation removal, or release while the original generation is still live.
2. Reservation age and token age start on opposite sides of external signing. The reservation height is captured before sign_tx(...).await, while register samples a fresh height afterward. A slow external signer can let the reservation be swept/reselected while the newly minted token still appears fresh. Carry the original reservation height in SignedCoreTransaction and register with it.
3. The V2 handle and registry-token paths have incompatible wallet-generation rules. V2 storage pins an old CoreWallet and validates only wallet ID, while registry tokens use manager identity plus wallet ID. After wallet recreation, an old V2 handle can act through the old manager while the new manager selects the same inputs. Both paths need one generation identity and one teardown policy.
4. Kotlin cancellation can orphan a token. buildSignedPayment returns a plain value through cancellable withContext(IO). If cancellation is observed after the blocking JNI registration returns, the token is discarded without a Cleaner/release path. Return an owning closeable object or make publication/release cancellation-safe.
5. Wrong-wallet broadcast consumes the token before checking its binding. SignedPaymentRegistry::broadcast removes first and validates second, so a mismatched caller destroys the original wallet's token and leaves its reservation until TTL. Validate under the lock, then atomically consume only a matching entry.

The existing age and dual-lifecycle comments point in the right direction; I would treat them as merge blockers rather than follow-ups because they can produce conflicting spends or stranded reservations.

[bfoss765]

All five lifetime findings are fixed as merge blockers and pushed, one commit each with regression tests: destroy releases the generation's reservations while teardown drops tokens and V2 handles; the pre-signing reservation height travels on SignedCoreTransaction into register; one generation identity across both the V2-handle and token paths; cancellation-safe ownership (an owning AutoCloseable with a Cleaner backstop, plus object-owning broadcastSigned/releaseReservation overloads that keep the payment reachable across the native call); and validate-then-consume under a single lock hold with network I/O outside it. The dead register chain is deleted, code 26 is split into typed 26/27/28 siblings (Kotlin-only host impact; Swift falls through safely), and the why-token-not-V2 rationale is in the PR body.

The V2 age guard you asked to file as a follow-up is implemented as a stacked PR: the V2 broadcast refuses at the same shared threshold off the same pre-signing height stamp (abandon works at any age), with exact-boundary tests on both account types.

[shumkov]

Consolidated re-verification (two independent passes). All five lifetime findings are genuinely fixed with discriminating regression tests, the dead register chain is fully deleted, the why-not-V2 rationale landed, and the error split into 26/27/28 (with txid carried on 27) is right. Two issues remain from the deeper pass:

1. Generation validation and reservation mutation are not atomic. The registry validates is_same_generation and then mutates after releasing its lock (core/wallet.rs:70-98,321-338, core/broadcast.rs:111-124, core/transaction.rs:257-268) — a same-ID wallet recreation between validation and cleanup can make old cleanup release the new generation's reservation. Bind the cleanup to a generation-local handle, or validate-and-mutate under a single manager lock, and cover same-ID recreation in the tests.

2. Deferred CoinJoin finalization can leak reservations until TTL. The FFI finalize path (transaction_builder.rs:170-293) reserves inputs, but registry rejection/abandon/free releases only when an account_type handle is present — CoinJoin-funded deferred payments left without one keep their funds reserved until the 24-block TTL. Retain a releasable account handle in the registry entry.

Minor: signed_payment.rs:40-45 still documents the pre-split error semantics (repeated-broadcast / re-created-wallet now yield 27/28, not 26), and the PR body should cite #4196 by number as the V2-side follow-up.

[shumkov]

Addendum: the missing Swift mappings for the new codes belong to this PR too, not only #4196 — PlatformWalletResult.swift:68-70 jumps from code 25 straight to 98, so 26/27/28 (introduced here) all surface as .errorUnknown on iOS. Fine to fix in either PR, but one of the two must carry it before the pair lands.

[bfoss765]

Both remaining blockers fixed (510 platform-wallet tests):

1. Atomic validate-and-mutate — reservation cleanup now re-validates the wallet generation (Arc::ptr_eq on the per-generation balance Arc) AND mutates the ReservationSet under a single manager read-lock hold. A same-id recreation needs the write lock and so can't interleave — provably atomic, covered by a new regression test that recreates between register and release and asserts the input stays reserved.
2. CoinJoin releasable handle — the registry entry now retains the full AccountTypePreference (incl. CoinJoin), so a rejected/abandoned CoinJoin-funded deferred payment releases immediately instead of waiting out the 24-block TTL (tested).

Swift now maps 26/27/28 to typed StaleReservationToken/ReservationTokenConsumed/ReservationWalletMismatch (exhaustive init(result:), message parity with Kotlin); the stale-broadcast doc is corrected. The V2 handle path is the follow-up in #4196.

One conscious scoping note: the immediate-send reject-release path (reservations.rs) shares the same theoretical generation window but a far narrower one — synchronous build→broadcast, no persisted token surviving a restart, CoinJoin not used for immediate sends — so this PR scopes the guard to the deferred registry + V2 handle paths your finding named. Happy to extend it there too if you'd prefer.

[shumkov]

Round-3 verification (two independent passes, reconciled): both previous blockers are genuinely fixed — the generation-safe release validates and mutates under one manager read-lock hold (and recreation requires the write lock, so no interleave), and CoinJoin entries now retain a releasable AccountTypePreference handle with a production-path regression test.

One new P1 (shared with #4196) — freshness and release are still two separate decisions:

• A reservation stamped at height 100 passes the age guard at 119; the broadcast await can span more than the 4-block margin; sync reaches 124, key-wallet's TTL sweeps the reservation and another build re-reserves the same outpoint; the rejected-broadcast cleanup then releases the new reservation by outpoint.
• Fix: read the current height, validate generation, and mutate as one guarded operation under the release lock; re-check freshness after the broadcast await before the Rejected-release; cover the signing-failure release path too. The existing tests prove endpoint states — add a barrier-controlled interleaving test that forces check → sweep → re-reserve → release.

Also before merge: rebase (branch is CONFLICTING with v4.1-dev), and please cite #4196 by number in the body as the V2-side sibling. Nit: new comments carry fix-round narration — provenance belongs in the PR description.

[thepastaclaw]

Preliminary review — Codex only

The exact reservation height, generation-aware cleanup, typed host errors, and Kotlin token-owner cleanup are now implemented correctly. Three blocking lifecycle defects remain: registry registration can mint duplicate capabilities for one reservation, destroying the final wallet alias invalidates independently owned tokens, and wallet removal is not linearized with in-flight finalization or broadcast. The reservation token also remains an untyped Rust u64 capability.

Validated blockers were found in the Codex precheck. Sonnet 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
• Sonnet: not run (deferred by blocker gate)

🔴 3 blocking | 💬 1 nitpick(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/signed_payment_registry.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:233-251: Registration still does not preserve unique reservation ownership
  `register` accepts a freely clonable `Transaction` together with independently supplied wallet, account, and height metadata instead of consuming the non-`Clone` `SignedCoreTransaction` that owns those facts. The test at lines 1080-1115 registers sixteen clones of one reserved transaction, proving that multiple live tokens can name the same reservation. After one token releases the original outpoint and another payment reserves it, a second old token remains generation- and age-valid and can unconditionally release the newer reservation by outpoint. The current FFI finalizer calls this method once, but the registry is publicly re-exported, so the ownership invariant cannot depend on that caller's discipline. Make registration consume the finalized ownership object exactly once and derive its transaction, account, and mandatory reservation height internally.

In `packages/rs-platform-wallet-ffi/src/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/wallet.rs:420-427: Destroying the final platform-wallet alias invalidates independently owned payment tokens
  `platform_wallet_destroy` releases every token for the generation when no other `PlatformWallet` wrapper is currently stored. Those wrappers do not own the logical wallet: the manager still owns it and can return another alias, `platform_wallet_get_core` creates independently owned core handles, each registry entry retains its own `CoreWallet`, and Kotlin documents `SignedCoreTransaction` as the token owner. Closing or collecting the last wrapper therefore consumes and releases a still-live payment token even though its owning object was neither closed nor used, so a later merchant acknowledgement cannot be broadcast through a retained core handle or reacquired alias. Token cleanup must follow the payment owner or actual wallet-generation removal, not transient wrapper-alias count.

In `packages/rs-platform-wallet-ffi/src/manager.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/manager.rs:452-471: Wallet removal is not linearized with finalization or broadcast
  Logical wallet removal and registry cleanup are separate operations. An existing token can be consumed after `manager.remove_wallet` has deleted its generation but before `remove_entries_for_wallet` runs: the retained old core handle still matches the entry, `last_processed_height()` returns `None`, `reservation_expired` treats that as valid, and the transaction reaches the broadcaster. Conversely, `finalize_transaction` releases the manager lock before awaiting the external signer; removal can delete the generation and complete its token sweep during that await, after which the finalizer registers a new token without revalidating that the generation still exists. Coordinate token registration and consumption with generation removal under a shared lifecycle lock or gate, reject an absent current generation, and ensure teardown waits for or sweeps in-flight finalizers.

[thepastaclaw]

🔴 Blocking: Registration still does not preserve unique reservation ownership

register accepts a freely clonable Transaction together with independently supplied wallet, account, and height metadata instead of consuming the non-Clone SignedCoreTransaction that owns those facts. The test at lines 1080-1115 registers sixteen clones of one reserved transaction, proving that multiple live tokens can name the same reservation. After one token releases the original outpoint and another payment reserves it, a second old token remains generation- and age-valid and can unconditionally release the newer reservation by outpoint. The current FFI finalizer calls this method once, but the registry is publicly re-exported, so the ownership invariant cannot depend on that caller's discipline. Make registration consume the finalized ownership object exactly once and derive its transaction, account, and mandatory reservation height internally.

_{source: ['codex']}

[thepastaclaw]

Resolved in 440897c — Registration still does not preserve unique reservation ownership 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]

🔴 Blocking: Destroying the final platform-wallet alias invalidates independently owned payment tokens

platform_wallet_destroy releases every token for the generation when no other PlatformWallet wrapper is currently stored. Those wrappers do not own the logical wallet: the manager still owns it and can return another alias, platform_wallet_get_core creates independently owned core handles, each registry entry retains its own CoreWallet, and Kotlin documents SignedCoreTransaction as the token owner. Closing or collecting the last wrapper therefore consumes and releases a still-live payment token even though its owning object was neither closed nor used, so a later merchant acknowledgement cannot be broadcast through a retained core handle or reacquired alias. Token cleanup must follow the payment owner or actual wallet-generation removal, not transient wrapper-alias count.

_{source: ['codex']}

[thepastaclaw]

Resolved in 440897c — Destroying the final platform-wallet alias invalidates independently owned payment tokens 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]

🔴 Blocking: Wallet removal is not linearized with finalization or broadcast

Logical wallet removal and registry cleanup are separate operations. An existing token can be consumed after manager.remove_wallet has deleted its generation but before remove_entries_for_wallet runs: the retained old core handle still matches the entry, last_processed_height() returns None, reservation_expired treats that as valid, and the transaction reaches the broadcaster. Conversely, finalize_transaction releases the manager lock before awaiting the external signer; removal can delete the generation and complete its token sweep during that await, after which the finalizer registers a new token without revalidating that the generation still exists. Coordinate token registration and consumption with generation removal under a shared lifecycle lock or gate, reject an absent current generation, and ensure teardown waits for or sweeps in-flight finalizers.

_{source: ['codex']}

[thepastaclaw]

Resolved in this update — Wallet removal is not linearized with finalization or broadcast 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]

💬 Nitpick: The reservation capability remains an untyped u64 alias

ReservationToken has single-use capability and provenance semantics distinct from ordinary numeric identifiers, but its type alias provides no Rust-side domain separation. Registry and boundary code can accidentally interchange unrelated u64 handles with destructive reservation operations without a type error. A #[repr(transparent)] newtype would preserve straightforward C/JNI conversion while enforcing the distinction within Rust.

_{source: ['codex']}

[thepastaclaw]

Resolved in 440897c — The reservation capability remains an untyped u64 alias 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.}

[bfoss765]

⚠️ Merge-time error-code coordination with #4184

After the rebase onto v4.1-dev, this PR's deferred-reservation errors are 27 = ErrorStaleReservationToken, 28 = ErrorReservationTokenConsumed, 29 = ErrorReservationWalletMismatch.

#4184 independently allocates 29 = ErrorAssetLockInsufficientFunds, so 29 collides.

Whichever of #4184 / #4185 merges SECOND must shift its 29. Recommended resolution (and what the 11.10.x QA integration build uses): keep #4184's 29 = ErrorAssetLockInsufficientFunds and move THIS PR's ErrorReservationWalletMismatch → 30. Propagate through the Rust enum + FFI From mapping, Kotlin DashSdkError (+ DashSdkErrorTest), and Swift PlatformWalletResult. 26 = ErrorTransactionBroadcastRejected (v4.1-dev) and 31 = ErrorSigningKeyUnavailable (#4183) are unaffected.

[shumkov]

Round-4 verification (two independent passes, reconciled): the rebase preserved the atomicity work — atomic selection+reservation and owner-token-guarded cleanup verified (registry 20/20, core transaction 3/3), and the new owner-guard on the broadcast-reject release answers the post-await recheck ask. Three items:

1. Move ErrorReservationWalletMismatch 29 → 30 now, deterministically — both review passes concur with keeping fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073) #4184's established 29, but not with "whichever merges second shifts": that's a known public-ABI collision, so resolve it here across Rust, Swift, Kotlin, JNI docs, tests, and the PR body (30 is free after fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073) #4184's re-scope dropped the consent code).
2. New P1 — delayed-signer publication race. finalize_transaction drops the manager lock before awaiting the external signer; remove_wallet can complete its one-time registry/V2 sweep in that window, and the late signer return then publishes a token/handle for the already-removed generation (core/transaction.rs:250). Order generation-validation + registry/V2 publication atomically against wallet removal, and add a barrier-controlled delayed-signer remove/recreate test.
3. P2: final-alias scanning/cleanup can race same-generation alias insertion at rs-platform-wallet-ffi/src/wallet.rs:421. Minors: stale 26/27/28 references in JNI docs; a few PR-owned rustfmt diffs remain.

[thepastaclaw]

Preliminary review — Codex only

At exact head 3f719d4, the newest owner-token delta correctly carries key-wallet's reservation token through finalization and uses owner-guarded release; its regression test models a sweep, re-reservation, and rejected broadcast and verifies that the newer reservation survives. Three carried-forward blocking lifecycle issues remain: public registration does not enforce unique ownership, final wallet-alias destruction consumes independently owned payments, and wallet removal is not linearized with finalization or broadcast. The carried-forward untyped-token nitpick also remains, and the latest error-code renumbering left one JNI Rustdoc block on the old values.

Validated blockers were found in the Codex precheck. Sonnet 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
• Sonnet: not run (deferred by blocker gate)

🔴 3 blocking | 💬 1 nitpick(s)

4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

[thepastaclaw]

💬 Nitpick: JNI Rustdoc still lists the pre-renumbered deferred error codes

The native deferred-payment errors and Kotlin declarations now use codes 27, 28, and 29, but this exported JNI function still documents them as 26, 27, and 28. Rust-side consumers using this documentation to diagnose or mirror exceptions will associate each condition with the wrong native code.

Suggested change

	/// throws one of three sibling codes: `ErrorStaleReservationToken` (26, aged
	/// out), `ErrorReservationTokenConsumed` (27, unknown / already broadcast /
	/// already released), or `ErrorReservationWalletMismatch` (28, different wallet
	/// generation). `coreHandle` must resolve to the wallet the token was minted
	/// throws one of three sibling codes: `ErrorStaleReservationToken` (27, aged
	/// out), `ErrorReservationTokenConsumed` (28, unknown / already broadcast /
	/// already released), or `ErrorReservationWalletMismatch` (29, different wallet
	/// generation). `coreHandle` must resolve to the wallet the token was minted

_{source: ['codex']}

[thepastaclaw]

Resolved in 440897c — JNI Rustdoc still lists the pre-renumbered deferred error codes 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.}

[bfoss765]

@thepastaclaw pushed fixes for 2 of the 3 blockers + both smaller items in 440897c9 (signed_payment 18 tests / destroy 5 tests pass):

• Unique ownership — SignedPaymentRegistry::register now consumes the non-Clone SignedCoreTransaction (via into_registered_parts) and derives tx/account/mandatory height/token internally, instead of taking them as separate args. A single finalize can no longer mint two live tokens for one reservation; the concurrent_registers_yield_distinct_tokens test (which modelled the now-impossible clone pattern) is removed, distinctness guaranteed by the AtomicU64 allocator.
• Wrapper-destroy no longer consumes payments — platform_wallet_destroy just drops the handle; it no longer scans for a "final alias" or releases the generation's tokens. Cleanup happens only via the payment owner (broadcast/release) or actual generation teardown (remove_wallet → remove_entries_for_wallet). The destroy test now asserts a token survives destroying every alias and stays releasable.
• Nit — ReservationToken is now a #[repr(transparent)] struct(u64) (ABI unchanged, still u64 on the wire).
• JNI Rustdoc numbering — updated wallet_manager.rs:1375 to the current codes. Heads-up: the branch actually has ReservationWalletMismatch = 29 consistently (error.rs / Swift / Kotlin all 29); there is no 30 in the tree, so I matched the code (27 StaleReservationToken / 28 ReservationTokenConsumed / 29 ReservationWalletMismatch). If a renumber to 30 is planned to deconflict with fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073) #4184's 29 = AssetLockInsufficientFunds, it hasn't landed on this head — that collision still needs resolving at merge time between fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073) #4184 and feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4185.

Blocker 3 (wallet removal not linearized with finalize/broadcast) — holding for a design decision, not a minimal patch. Both windows you describe are real: (a) remove_wallet + remove_entries_for_wallet are separate steps, so a broadcast in between still passes is_same_generation/reservation_expired; (b) finalize_transaction drops the manager write lock before awaiting the external signer, so removal can sweep during that await. A partial "reject absent current generation" check only closes the fully-ordered case and leaves the interleaving window open while looking fixed — unsafe to claim on a money path. The correct fix is the per-wallet teardown gate the Kotlin side already references, at the Rust layer: a per-generation async RwLock where finalize+register and broadcast+release take a shared guard across their whole section, remove_wallet takes the exclusive guard (drains in-flight ops, then removes+sweeps as one unit), and register/broadcast revalidate generation liveness under the gate. That spans manager/registry/CoreWallet + new race tests — I'd rather scope it deliberately than land something partial. Happy to take it as a focused follow-up.

[thepastaclaw]

Preliminary review — Codex only

Carried forward, prior-1 remains partially valid: consuming SignedCoreTransaction prevents duplicate registration, but the independently supplied CoreWallet can still bind the reservation to the wrong wallet generation; prior-3 remains real but was explicitly deferred to a lifecycle-gate follow-up. The latest delta fixes prior-2, prior-4, and prior-5, but introduces another blocking ownership hole because the now-consuming registration API is still async and can lose the reservation owner before its first poll. The full PR also needs defensive Kotlin cleanup if token-owner construction fails, while the obsolete public alias-scan helper is a minor cleanup.

Validated blockers were found in the Codex precheck. Sonnet 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
• Sonnet: not run (deferred by blocker gate)
  Source: reviewers gpt-5.6-sol; verifier gpt-5.6-sol.

🔴 2 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(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/signed_payment_registry.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:283-300: Registration can bind a reservation to the wrong wallet generation
  `register` consumes the finalized transaction and derives its transaction, account, height, and funding token, but it still accepts an independent `CoreWallet`. `SignedCoreTransaction` carries no originating wallet-generation marker, so safe public Rust code can finalize through wallet A and call `register(core_b, signed_from_a)`. The registry then treats B as the owner: broadcasting through B passes the generation check and submits A's transaction through B's broadcaster, while release or definitive-rejection cleanup runs against B and leaves A's actual reservation held until its TTL. This violates the documented guarantee that a token is bound to the generation whose `ReservationSet` owns the inputs. Store an unforgeable originating-generation marker in `SignedCoreTransaction` and derive or validate the registry's core from it instead of trusting a separate argument.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs:283-302: Async registration can drop the reservation owner before insertion
  `register` now consumes the non-`Clone` `SignedCoreTransaction`, but it remains an `async fn` even though its body contains no await. Calling an async function only moves its arguments into the returned future; the body does not execute until that future is polled. If the future is dropped or cancelled before its first poll, the consumed `SignedCoreTransaction` is dropped without being inserted, no payment token is returned, and the type has no `Drop` implementation that releases its funding reservation. The reservation is therefore unreachable to explicit cleanup until key-wallet's TTL. Make registration synchronous and update callers to invoke it directly.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt:184-191: Release the native token if Kotlin owner construction fails
  Native finalization has already inserted the payment and committed its reservation when this byte array returns. The token does not gain its owning `NativeCleaner` until `SignedCoreTransaction.fromRegisterBlob` finishes, so an allocation failure, malformed-data exception from an ABI mismatch, or Cleaner-registration failure leaves the native token registered without any JVM owner capable of releasing it. Parse the token first and release it defensively if construction of the ownership object throws.

[thepastaclaw]

🔴 Blocking: Registration can bind a reservation to the wrong wallet generation

register consumes the finalized transaction and derives its transaction, account, height, and funding token, but it still accepts an independent CoreWallet. SignedCoreTransaction carries no originating wallet-generation marker, so safe public Rust code can finalize through wallet A and call register(core_b, signed_from_a). The registry then treats B as the owner: broadcasting through B passes the generation check and submits A's transaction through B's broadcaster, while release or definitive-rejection cleanup runs against B and leaves A's actual reservation held until its TTL. This violates the documented guarantee that a token is bound to the generation whose ReservationSet owns the inputs. Store an unforgeable originating-generation marker in SignedCoreTransaction and derive or validate the registry's core from it instead of trusting a separate argument.

_{source: ['codex']}

[thepastaclaw]

🔴 Blocking: Async registration can drop the reservation owner before insertion

register now consumes the non-Clone SignedCoreTransaction, but it remains an async fn even though its body contains no await. Calling an async function only moves its arguments into the returned future; the body does not execute until that future is polled. If the future is dropped or cancelled before its first poll, the consumed SignedCoreTransaction is dropped without being inserted, no payment token is returned, and the type has no Drop implementation that releases its funding reservation. The reservation is therefore unreachable to explicit cleanup until key-wallet's TTL. Make registration synchronous and update callers to invoke it directly.

_{source: ['codex']}

[thepastaclaw]

🟡 Suggestion: Release the native token if Kotlin owner construction fails

Native finalization has already inserted the payment and committed its reservation when this byte array returns. The token does not gain its owning NativeCleaner until SignedCoreTransaction.fromRegisterBlob finishes, so an allocation failure, malformed-data exception from an ABI mismatch, or Cleaner-registration failure leaves the native token registered without any JVM owner capable of releasing it. Parse the token first and release it defensively if construction of the ownership object throws.

Suggested change

	val blob = WalletManagerNative.coreWalletFinalizeSignedPayment(
	builderPtr,
	walletHandle,
	accountType.ffiValue,
	accountIndex,
	coreSignerHandle,
	)
	return ManagedPlatformWallet.SignedCoreTransaction.fromRegisterBlob(blob)
	val blob = WalletManagerNative.coreWalletFinalizeSignedPayment(
	builderPtr,
	walletHandle,
	accountType.ffiValue,
	accountIndex,
	coreSignerHandle,
	)
	var token: Long? = null
	return try {
	token = java.nio.ByteBuffer.wrap(blob).long
	ManagedPlatformWallet.SignedCoreTransaction.fromRegisterBlob(blob)
	} catch (error: Throwable) {
	token?.let { value ->
	runCatching { WalletManagerNative.coreWalletReleaseSignedPayment(value) }
	}
	throw error
	}

_{source: ['codex']}

[thepastaclaw]

💬 Nitpick: The removed final-alias policy left a stale public handle API

The latest delta removed the final-alias scan from platform_wallet_destroy, leaving HandleStorage::any with no call sites. Its Rustdoc still says it exists for that removed policy, and handle is a public module, so the helper unnecessarily remains part of the crate's public Rust surface. Remove the method and obsolete explanation.

_{source: ['codex']}