feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission - #4185
Open
bfoss765 wants to merge 32 commits into
Open
feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission#4185bfoss765 wants to merge 32 commits into
bfoss765 wants to merge 32 commits into
Conversation
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
July 21, 2026 12:41
Contributor
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (32)
📝 WalkthroughWalkthroughAdds deferred Core signed-payment flows across the Rust wallet, FFI, JNI, Kotlin SDK, and Swift SDK. Payments can be built, reserved, broadcast, or released with generation-bound reservation tokens and typed errors. ChangesDeferred signed payment lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant KotlinSDK
participant JNI
participant NativeFFI
participant SignedPaymentRegistry
participant CoreWallet
KotlinSDK->>JNI: finalize signed payment
JNI->>NativeFFI: fund, reserve, sign, and register
NativeFFI->>SignedPaymentRegistry: store payment and token
KotlinSDK->>JNI: broadcast or release token
JNI->>NativeFFI: execute token operation
NativeFFI->>SignedPaymentRegistry: broadcast or release reservation
SignedPaymentRegistry->>CoreWallet: update reservation state
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
⛔ Blockers found — Sonnet deferred (commit 326cd3e) |
Collaborator
|
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:
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 |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 valueDeprecate or remove the legacy
registerSignedPaymentbridge. 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@Deprecatedand point docs to the atomicfinalizeSignedPaymentflow.🤖 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
📒 Files selected for processing (19)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/rs-platform-wallet-ffi/src/core_wallet/mod.rspackages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rspackages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/handle.rspackages/rs-platform-wallet-ffi/src/wallet.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/wallet.rspackages/rs-platform-wallet/src/wallet/mod.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rspackages/rs-unified-sdk-jni/src/wallet_manager.rs
Collaborator
|
Additional/strengthened lifetime findings after checking the existing threads:
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
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 21, 2026
Cross-PR collision: the split-build-broadcast branch (dashpay#4185) already allocates 26-28 for the reservation-token errors on the same base, and both PRs would merge without textual conflict, silently misclassifying errors on whichever lands second. Codes 26-28 are now documented as reserved for dashpay#4185. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
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 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. |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 21, 2026
…the signer wire Replace end-to-end message sniffing for the signer's "missing key" failure with a typed discriminator (dashpay#4060 finding 7): - rs-sdk-ffi: DashSDKSignerErrorCode { Generic = 0, SigningKeyUnavailable = 1, AuthenticationFailed = 2 (reserved) }; SignCompletionCallback and dash_sdk_sign_async_completion gain error_code: i32 (before error_message). SignResult stays Result<Vec<u8>, ProtocolError> (a new rs-dpp ProtocolError variant would carry serialization blast radius), so code 1 rides the single Rust-owned machine prefix DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX through ProtocolError::Generic — typed at both ABI edges, one constant bridging the string segment. This is an internal coordinated ABI change: every piece versions together in this monorepo. - rs-platform-wallet-ffi: PlatformWalletFFIResultCode:: ErrorSigningKeyUnavailable = 31 (codes 26-28 are reserved for dashpay#4185's reservation-token errors and 29/30 for dashpay#4184's asset-lock errors on sibling branches — documented in the enum as dashpay#4184 does). The From<dpp::ProtocolError> conversion restores the typed code from the prefix FIRST (before the loose keyword sniffs), and the From<PlatformWalletError> blanket impl restores it on the catch-all only (dedicated retry-semantics codes are never overridden) — covering the Sdk(dash_sdk::Error::Protocol(..)) wrapping path. - JNI/Kotlin: SignerNative.completeSign(token, signature, errorCode, errorMessage); KeystoreSigner passes SIGNER_ERROR_CODE_KEY_UNAVAILABLE on the null-key branch (keeping the MESSAGE_MARKER text for the transition window) and Generic everywhere else. DashSdkError maps 31 → PlatformWallet.SigningKeyUnavailable; the dashpay#4191 marker sniff on the catch-all codes remains as a deprecated old-native fallback with a removal note tied to the next minor release. - Swift: KeychainSigner trampolines forward the code (missing-row / missing-scalar outcomes classify as 1); PlatformWalletResultCode gains errorSigningKeyUnavailable = 31 → PlatformWalletError .signingKeyUnavailable (Kotlin parity). - Tests: rs-sdk-ffi completion-code tests (prefix present for code 1, absent for generic), platform-wallet-ffi prefix→31 tests on both conversion points, Kotlin code-31 + fallback-marker tests, Swift mapping and trampoline-classifier tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
|
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:
Minor: |
Collaborator
|
Addendum: the missing Swift mappings for the new codes belong to this PR too, not only #4196 — |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 22, 2026
…roadcast A pinned V2 finalized-transaction handle (core_wallet_tx_builder_finalize → broadcast_finalized_transaction) had no reservation 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 — the same stale-release hazard the deferred registry-token path already defends against. This becomes live the moment iOS starts issuing deferred sends (follow-up requested on PR dashpay#4185). Mirror the registry-token age policy on the V2 handle path: - Hoist RESERVATION_MAX_AGE_BLOCKS (20) and reservation_expired() from signed_payment_registry into wallet::reservations so both the registry and the V2 handle path bound a reservation's lifetime against key-wallet's TTL with one shared number. - broadcast_finalized_transaction now refuses, before touching the broadcaster, once current last_processed_height - the reservation's stamp height (already carried on SignedCoreTransaction::reservation_height) >= the shared bound, returning the new token-less PlatformWalletError::StaleReservation. The stale reservation is left for key-wallet's TTL to reclaim (never released by outpoint, which could free a newer build's reservation). The check runs after the FFI layer's generation-identity check, matching the registry ordering. - The FFI reuses the existing ErrorStaleReservationToken (26) code for this variant (documented as shared between the registry-token and V2-handle surfaces); no new codes allocated. - Abandon/free (abandon_transaction) remain allowed at any age — releasing an old reservation is always safe. Tests: fresh handle broadcasts; aged handle refuses with StaleReservation yet still abandons cleanly and frees its inputs; exact boundary at the threshold (BIP44/BIP32); FFI mapping of StaleReservation to the shared code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 22, 2026
shumkov (PR dashpay#4185 follow-up) found the age guard covered only broadcast: `abandon_transaction` — and therefore the `_v2_free` deinit/GC backstop and the FFI broadcast/abandon failure paths that route their cleanup through it — still released the funding reservation by outpoint unconditionally at any age. A FinalizedCoreTransaction GC'd after ~1h whose outpoint was TTL-swept (24 blocks) and re-reserved would free the newer build's reservation, letting its inputs be re-selected into a third build (conflicting spends). Honor `reservation_expired` in `abandon_transaction`, mirroring the registry's `reconcile_removed_entry`: once aged past the shared `RESERVATION_MAX_AGE_BLOCKS` bound, skip the by-outpoint release (leave the outpoint for key-wallet's TTL to reclaim) while still tearing down the handle; below the bound, release as before. This covers every consumer of `abandon_transaction`, including the `_v2_free` GC-backstop and the FFI failure paths, off the same predicate/clock the broadcast guard uses. Also correct the reservation-policy docs that claimed releasing was always safe (`reservations.rs`, `broadcast_finalized_transaction`), and the misleading ManagedCoreWallet KDoc: after a stale-refused broadcast the handle is already consumed, so `abandonTransaction` is an invalid-handle error, not a recovery — the reservation waits out the TTL. Tests: platform-wallet gains aged-skips-release / below-bound-releases pairs (BIP44+BIP32); platform-wallet-ffi gains aged `_v2_free` and aged failure-path skip-release tests via a new `age_core_past_reservation_guard` test helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Both remaining blockers fixed (510 platform-wallet tests):
Swift now maps 26/27/28 to typed One conscious scoping note: the immediate-send reject-release path ( |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 23, 2026
…the signer wire Replace end-to-end message sniffing for the signer's "missing key" failure with a typed discriminator (dashpay#4060 finding 7): - rs-sdk-ffi: DashSDKSignerErrorCode { Generic = 0, SigningKeyUnavailable = 1, AuthenticationFailed = 2 (reserved) }; SignCompletionCallback and dash_sdk_sign_async_completion gain error_code: i32 (before error_message). SignResult stays Result<Vec<u8>, ProtocolError> (a new rs-dpp ProtocolError variant would carry serialization blast radius), so code 1 rides the single Rust-owned machine prefix DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX through ProtocolError::Generic — typed at both ABI edges, one constant bridging the string segment. This is an internal coordinated ABI change: every piece versions together in this monorepo. - rs-platform-wallet-ffi: PlatformWalletFFIResultCode:: ErrorSigningKeyUnavailable = 31 (codes 26-28 are reserved for dashpay#4185's reservation-token errors and 29/30 for dashpay#4184's asset-lock errors on sibling branches — documented in the enum as dashpay#4184 does). The From<dpp::ProtocolError> conversion restores the typed code from the prefix FIRST (before the loose keyword sniffs), and the From<PlatformWalletError> blanket impl restores it on the catch-all only (dedicated retry-semantics codes are never overridden) — covering the Sdk(dash_sdk::Error::Protocol(..)) wrapping path. - JNI/Kotlin: SignerNative.completeSign(token, signature, errorCode, errorMessage); KeystoreSigner passes SIGNER_ERROR_CODE_KEY_UNAVAILABLE on the null-key branch (keeping the MESSAGE_MARKER text for the transition window) and Generic everywhere else. DashSdkError maps 31 → PlatformWallet.SigningKeyUnavailable; the dashpay#4191 marker sniff on the catch-all codes remains as a deprecated old-native fallback with a removal note tied to the next minor release. - Swift: KeychainSigner trampolines forward the code (missing-row / missing-scalar outcomes classify as 1); PlatformWalletResultCode gains errorSigningKeyUnavailable = 31 → PlatformWalletError .signingKeyUnavailable (Kotlin parity). - Tests: rs-sdk-ffi completion-code tests (prefix present for code 1, absent for generic), platform-wallet-ffi prefix→31 tests on both conversion points, Kotlin code-31 + fallback-marker tests, Swift mapping and trampoline-classifier tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 23, 2026
Cross-PR collision: the split-build-broadcast branch (dashpay#4185) already allocates 26-28 for the reservation-token errors on the same base, and both PRs would merge without textual conflict, silently misclassifying errors on whichever lands second. Codes 26-28 are now documented as reserved for dashpay#4185. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
|
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 One new P1 (shared with #4196) — freshness and release are still two separate decisions:
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
suggested changes
Jul 23, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
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.
Comment on lines
+233
to
+251
| pub async fn register( | ||
| &self, | ||
| core: CoreWallet<B>, | ||
| tx: Transaction, | ||
| account_type: AccountTypePreference, | ||
| account_index: u32, | ||
| registered_height: Option<u32>, | ||
| ) -> ReservationToken { | ||
| let token = self.next_token.fetch_add(1, Ordering::SeqCst); | ||
| self.lock().insert( | ||
| token, | ||
| RegisteredPayment { | ||
| core, | ||
| tx, | ||
| account_type, | ||
| account_index, | ||
| registered_height, | ||
| }, | ||
| ); |
Collaborator
There was a problem hiding this comment.
🔴 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']
Collaborator
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
Verified still correct at 6c37e8679e. SignedCoreTransaction is deliberately not Clone, and into_registered_parts(self) (transaction.rs:155) consumes it, so one finalize yields exactly one ownership object and register can be handed it once. The forging constructor new_for_test is test-utils-gated, and that feature is enabled only under [dev-dependencies] (rs-platform-wallet-ffi/Cargo.toml:65), so the invariant holds for library consumers too. Fixed in 440897c9cb.
Comment on lines
+420
to
+427
| let core = wallet.core(); | ||
| let sibling_alias_alive = | ||
| PLATFORM_WALLET_STORAGE.any(|other| other.core().is_same_generation(core)); | ||
| if !sibling_alias_alive { | ||
| runtime().block_on( | ||
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY | ||
| .release_entries_for_wallet(core), | ||
| ); |
Collaborator
There was a problem hiding this comment.
🔴 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']
Collaborator
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
Verified at 6c37e8679e. platform_wallet_destroy no longer scans aliases at all — wallet.rs:392-414 is now just PLATFORM_WALLET_STORAGE.remove(handle). Token cleanup follows the payment owner or a real generation removal (remove_wallet) instead. Regression test wallet::destroy_tests::destroying_wrapper_aliases_never_sweeps_tokens asserts that destroying even the final alias leaves the token live.
Comment on lines
+454
to
+471
| }); | ||
| let result = unwrap_option_or_return!(option); | ||
| match result { | ||
| Ok(_) => PlatformWalletFFIResult::ok(), | ||
| Ok(removed) => { | ||
| // Generation teardown: the wallet and its accounts' `ReservationSet`s | ||
| // are now gone from the manager, so the deferred-payment reservations | ||
| // cease to exist — there is nothing to reconcile. DROP (do not | ||
| // release) this generation's registry tokens and its finalized-tx V2 | ||
| // handles. This is the teardown half of the single generation policy | ||
| // both deferred paths share: it makes any stale handle to the removed | ||
| // generation inert, so a later destroy/release of a lingering handle | ||
| // can never release-by-outpoint against a re-created generation's | ||
| // inputs. | ||
| let core = removed.core(); | ||
| crate::core_wallet::signed_payment::SIGNED_PAYMENT_REGISTRY | ||
| .remove_entries_for_wallet(core); | ||
| crate::handle::CORE_SIGNED_TRANSACTION_V2_STORAGE | ||
| .remove_matching(|tx| tx.wallet.is_same_generation(core)); |
Collaborator
There was a problem hiding this comment.
🔴 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']
Collaborator
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
You were right, and the automated "resolved" mark was wrong — it was a non-recurrence heuristic, not an engagement with the mechanism. Both halves reproduce. Fixed in 0b0d5c76.
For the record on why this took a week: the 07-24 push fixed the wrapper-destroy path, not the manager remove_wallet path this finding is actually about. Two different teardown paths in the same neighborhood, and the commit never touched manager.rs at all. The tests we added covered destroy and passed, which made it look done.
Remove-then-sweep. platform_wallet_manager_remove_wallet swept the registry only after manager.remove_wallet returned, and that removal awaits shielded-coordinator and identity-sync unregistration — so the gap contains real yield points, not a few instructions. A broadcast landing in it passed everything: is_same_generation compares two handles, so a removed generation matches itself; last_processed_height is None once the wallet is gone and reservation_expired maps None -> false; and broadcast_payment_releasing_reservation goes straight to the broadcaster with no existence gate. The comment claiming "the wallet-mismatch / account-lookup paths already reject those cases" was not true of the broadcast path — there is no account lookup before the send.
In-flight finalizer. finalize_transaction drops the manager write lock before awaiting the signer, and register only checks the payment against its finalizing generation, never that the generation still exists — so a removal during signing swept the registry and the finalizer then inserted a token no later sweep would catch.
Fix. SignedPaymentRegistry now owns a lifecycle gate (tokio::RwLock). Teardown holds the exclusive side across both the manager removal and the sweep, making them one linearization point; broadcast and release hold the shared side for their whole duration. The entries mutex couldn't serve — it's a std mutex dropped before every await by design. Lock order is always gate -> manager.
Broadcast now rejects an absent current generation (CoreWallet::is_current_generation) rather than silently proceeding. core_wallet_signed_payment_finalize holds the shared gate across its liveness check and the synchronous register, abandoning and reconciling the reservation if the wallet went away mid-signature. I took that gate after the signer await rather than around it, so an open signing prompt can't stall teardown for every wallet.
No new error code: the removed-wallet case maps to the existing NotFound (98), which both hosts already map, avoiding the 29/30 renumbering contested in #4261. Rust/Swift/Kotlin docs record the added meaning and how it differs from ErrorReservationWalletMismatch (29) — there, a different live generation answers to the id; here there is none, so it isn't retryable.
Three regression tests in the FFI crate, driving a remove_wallet_and_tear_down_generation helper that runs the exact production sequence. All three fail against the previous code; the race test reports "a payment reached the broadcaster even though wallet teardown had already completed", which is your finding verbatim.
Comment on lines
+77
to
+82
| /// Opaque handle to a registered, signed-but-unsent payment. Minted by | ||
| /// [`SignedPaymentRegistry::register`]; consumed by | ||
| /// [`SignedPaymentRegistry::broadcast`] or | ||
| /// [`SignedPaymentRegistry::release`]. Values are unique for the process | ||
| /// lifetime and never reused, so a stale token can always be recognised. | ||
| pub type ReservationToken = u64; |
Collaborator
There was a problem hiding this comment.
💬 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']
Collaborator
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
Adopted. ReservationToken is now #[repr(transparent)] pub struct ReservationToken(u64) (signed_payment_registry.rs:95-97), with conversions confined to the FFI edge (transaction_builder.rs:315). key-wallet's own token is aliased to FundingReservationToken so the funding token and the payment handle cannot be blurred.
bfoss765
force-pushed
the
port/v4.1/split-build-broadcast
branch
from
July 23, 2026 16:17
911c8f7 to
3f719d4
Compare
…; drop them at generation teardown platform_wallet_destroy called remove_entries_for_wallet, which only DROPPED the registry entries. But destroying the last wrapper alias does not remove the logical wallet from its manager — the accounts' ReservationSets stay live and the same wallet can be handed out again — so the dropped tokens' inputs stayed reserved until key-wallet's TTL. Tokens were consumed without releasing live reservations. Split the two teardown moments under one generation identity: - Final-alias destroy (wallet still live): release_entries_for_wallet RELEASES each of the generation's reservations against the still-live wallet (honouring the age guard), so a wallet handed out again can respend the inputs. The final-alias check and the match are both by CoreWallet::is_same_generation. - Actual generation teardown (platform_wallet_manager_remove_wallet): the wallet and its ReservationSets are gone, so remove_entries_for_wallet DROPS the generation's registry tokens (nothing to reconcile) and remove_matching drops its finalized-tx V2 handles. This makes any stale handle to the removed generation inert, which is what makes the destroy-time release provably race-free: a torn-down generation has already had its tokens swept here, so destroy/release can never release-by-outpoint against a re-created generation's inputs. platform_wallet_destroy now block_on's the release (as it already runs off the tokio runtime on the JNI / NativeCleaner threads). Adds HandleStorage::remove_matching, registry release_entries_for_wallet, a registry regression proving destroy-time release frees the reservation while teardown drop does not, and reworks the FFI destroy test to invoke destroy off-runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hree siblings Native code 26 (ErrorStaleReservationToken) mapped all three SignedPaymentError variants — StaleToken (unknown/already-broadcast/released), WalletMismatch (different wallet generation), and StaleReservationToken (aged out) — so a host could not tell "you already broadcast this", "wrong wallet", and "the reservation aged out; rebuild" apart, even though the remedy and messaging differ. Split at the FFI (additive sibling codes, no renumbering): - 26 ErrorStaleReservationToken -> StaleReservationToken (aged out) - 27 ErrorReservationTokenConsumed -> StaleToken (unknown/already broadcast/released) - 28 ErrorReservationWalletMismatch -> WalletMismatch (different generation) core_wallet_signed_payment_broadcast now maps each variant to its own code. All three remain non-retryable-in-place and none touch the network. Host impact (Kotlin SDK only — the Swift host does not map these codes): adds DashSdkError.PlatformWallet.ReservationTokenConsumed / ReservationWalletMismatch, maps 27/28, narrows the code-26 doc, updates the JNI/Kotlin broadcast KDocs, and extends DashSdkErrorTest to assert all three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-register shape The KDoc still described the pre-finalize build (`new → addOutput* → setFunding → buildSigned`) and credited buildSigned with reserving the inputs. The deferred path now issues a single atomic finalizeSignedPayment (select + reserve + sign + register under the wallet-manager lock). Update the described step sequence and the atomicity claim to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ble with a Cleaner backstop buildSignedPayment returned a plain SignedCoreTransaction through a cancellable coroutine. The blocking JNI registration mints the reservation token before the Kotlin object exists, so if cancellation was observed after that native call returned — or the caller simply dropped the value — the token (and its funding reservation) was orphaned until key-wallet's TTL, with no release path. Make SignedCoreTransaction an AutoCloseable that registers a NativeCleaner backstop at construction: close(), or GC if the caller never calls it, releases the token exactly once. Native release is idempotent and tokens are process-unique, so releasing a token already consumed by broadcastSigned / releaseReservation (or closing twice) is a harmless no-op. This closes the cancellation window — the object is Cleaner-backed the instant it exists (no suspension point between the native return and construction), so a discarded object always releases its token. Adds a pure-JVM test pinning the ownership contract (owning AutoCloseable) and the Cleaner run-once guarantee it relies on, and documents the ownership on buildSignedPayment. :sdk:testDebugUnitTest passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…etion Normalize two pre-existing long lines in coreWalletFinalizeSignedPayment that `cargo fmt --check` flags, so the JNI crate is formatting-clean after the register-chain removal touched this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…CoreTransaction Review round 2: the bare-Long token API couples the reservation's lifetime to the SignedCoreTransaction's GC-reachability — extracting the token and dropping the object lets the Cleaner backstop release the reservation out from under a pending broadcast. The object overloads keep the payment reachable across the native call (reachabilityFence) and disarm the backstop once the token is consumed; the bare-token docs now warn about the reachability requirement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed deferred payments The deferred-payment registry stored only an `Option<StandardAccountType>`, so a CoinJoin funding — which has no `StandardAccountType` — reconciled nothing on rejection/abandon/free and kept its inputs reserved until key-wallet's 24-block TTL, even though `finalize` reserves the selected inputs for every account variant. Carry the full `AccountTypePreference` (BIP44/BIP32/CoinJoin) as the entry's releasable account handle. The registry now broadcasts through the new `broadcast_payment_releasing_reservation` and releases through `release_transaction_reservation` (both `AccountTypePreference`-typed and CoinJoin-capable), so a rejected or abandoned CoinJoin deferred payment frees its reservation immediately. The FFI finalize passes `account_type.into()` instead of the `StandardAccountType` subset; the now-unused `release_payment_reservation` (registry-only) is removed. Test: `coinjoin_funded_release_frees_the_reservation_immediately` funds CoinJoin account 0, finalizes a sweep, registers the token, and proves release makes the input immediately spendable again. Adds a `#[cfg(test)]` `funded_coinjoin_wallet_manager` fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… wallet generation The deferred registry validated a token's generation at the registry lock, then released its reservation later, off that lock. `ReservationSet::release` removes an outpoint unconditionally and is reached via `wallet_id` — an identity a same-id remove-then-recreate preserves — so a wallet re-created in that window could have the NEW generation's reservation freed by the old token's cleanup. Bind the cleanup to the token's own generation: `release_transaction_reservation` now re-validates the generation and mutates the `ReservationSet` under a single manager read-lock hold, acting only when the wallet still registered under the id carries the same per-generation balance `Arc` the handle captured. A recreation needs the manager write lock, so it cannot interleave between the check and the release — validate-and-mutate is atomic. This protects both the registry (release/abandon and broadcast-on-rejection) and the V2 finalized-transaction handle path, which share this primitive. Adds `CoreWallet::generation()`. Test: `recreation_between_validation_and_cleanup_cannot_release_new_generation` recreates the wallet under the same id between registration and release and asserts the input stays reserved (the reservation the new generation owns is untouched). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`PlatformWalletResultCode` jumped from 25 straight to 98, so the three deferred build->broadcast/release codes this PR owns (26 StaleReservationToken, 27 ReservationTokenConsumed, 28 ReservationWalletMismatch) fell through to `.errorUnknown` on iOS, erasing their distinct retry semantics. Add the three raw codes to `PlatformWalletResultCode`, matching cases to `PlatformWalletError`, and map them in both `init(ffi:)` and `init(result:)`. The `init(result:)` switch (no default) stays exhaustive — the same non-exhaustive-switch class shumkov flagged on dashpay#4184. Messages pass the Rust `Display` string straight through, matching the Kotlin SDK's mapping verbatim. Verified with `swiftc -parse` (the DashSDKFFI xcframework — cbindgen header + cdylib — is built separately by build_ios.sh and is not present in this checkout, so a full `swift build` type-check isn't possible here). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…error codes `core_wallet_signed_payment_broadcast` still documented the pre-split semantics: a repeated broadcast and a re-created wallet both as `ErrorStaleReservationToken` (26). Since the three-way split, a repeated/concurrent broadcast yields `ErrorReservationTokenConsumed` (27) and a re-created wallet generation yields `ErrorReservationWalletMismatch` (28); 26 is reserved for the aged-out case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Point all rust-dashcore crates at bfoss765/rust-dashcore e99959ced0062159d629930f488374e29f63c42b (PR dashpay/rust-dashcore#916), which is v4.1-dev's rust-dashcore tip 70d4bf8 plus the additive owner-tagged reservation API: key_wallet::ReservationToken, ReservationSet::reserve/release_if_owner, TransactionBuilder::build_{unsigned,signed}_reserved, ManagedCoreFundsAccount::release_reservation_if_owner, and AssetLockResult.reservation_token. Additive over 70d4bf8, so it stays compatible with the rest of the v4.1-dev workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
v4.1-dev added ErrorTransactionBroadcastRejected = 26, colliding with this PR's three deferred-reservation siblings that also claimed 26/27/28. Keep v4.1-dev's 26 and shift this PR's codes up by one: 27 = ErrorStaleReservationToken (was 26) 28 = ErrorReservationTokenConsumed (was 27) 29 = ErrorReservationWalletMismatch (was 28) 29 is free on v4.1-dev (dashpay#4184's AssetLockInsufficientFunds is not yet merged there). The Rust FFI enum and Swift bindings were renumbered in the rebase conflict resolution; this finishes the propagation through the Kotlin runtime mapping and KDoc (DashSdkError.kt, WalletManagerNative.kt), the Kotlin error-code test, and the signed_payment FFI doc comments (also recast from fix-round narration to an as-built description). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lease A deferred send reserves its funding inputs at build, awaits the broadcast, and on a definitive rejection releases the reservation for an immediate rebuild. That release was unconditional (release-by-outpoint): during the broadcast await, key-wallet's TTL sweep can reclaim the reservation and a concurrent build can re-reserve the same outpoint under a new token, so the by-outpoint release would free that other build's inputs — the dashpay#4185 release/re-reserve double-spend window. Capture the key_wallet::ReservationToken build_unsigned_reserved stamps onto the selected inputs, carry it on SignedCoreTransaction alongside reservation_height, thread it through the deferred registry (RegisteredPayment / register / broadcast / reconcile) and broadcast_payment_releasing_reservation, and release via ManagedCoreFundsAccount::release_reservation_if_owner so a rejected or abandoned send frees only inputs its own build still owns. The finalize sign-failure path (a platform-side await between reserve and release) is owner-guarded the same way. None (no reservation taken) keeps the old by-outpoint fallback, never reached on the funded finalize path. Adds a regression test: a rejected deferred broadcast whose outpoint was swept and re-reserved under a new token leaves that new reservation intact. Docs name the shared generation identity's sibling V2 handle path (dashpay#4196). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…per-destroy from consuming payments, type the token Addresses two of the three carried-forward lifecycle blockers on dashpay#4185 plus the two smaller review items. The wallet-removal/finalize linearization blocker is intentionally NOT included here (see PR discussion) — it needs a shared lifecycle gate that is a design change on a money path. Blocker 1 — unique reservation ownership: `SignedPaymentRegistry::register` now CONSUMES the non-`Clone` `SignedCoreTransaction` and derives the transaction, funding account, mandatory reservation height, and owner-guard token from it (new `SignedCoreTransaction::into_registered_parts`). Because the ownership object is moved exactly once, a single finalize can no longer mint two live tokens naming the same held reservation. The FFI finalizer passes the finalized object straight in; the former duplicate-registration test (16 clones of one reserved tx) is removed as it modelled the now-impossible pattern. Blocker 2 — final wallet-alias destroy no longer consumes independently-owned payments: `platform_wallet_destroy` no longer releases the generation's tokens when the last wrapper alias is dropped. A wrapper handle does not own the logical wallet or the registered payment (the manager still owns the wallet; each registry entry pins its own `CoreWallet`). Token cleanup now follows the payment owner (broadcast/release) or actual generation teardown (`remove_wallet` → `remove_entries_for_wallet`), never a transient alias count. The unused `release_entries_for_wallet` method and its test are removed; the destroy test now asserts tokens survive destroying every alias. Nit — typed token: `ReservationToken` is now a `#[repr(transparent)]` newtype instead of a bare `u64` alias, converted to/from `u64` only at the FFI boundary, so a payment handle can't be silently confused with another numeric id. Docs — JNI Rustdoc: the `coreWalletBroadcastSignedPayment` block referenced the pre-renumber codes (26/27/28); updated to the current enum values (27 StaleReservationToken / 28 ReservationTokenConsumed / 29 ReservationWalletMismatch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…+ retain reservation owner through insertion (dashpay#4185 review) Addresses the two new thepastaclaw blockers plus the Kotlin owner-construction suggestion on PR dashpay#4185: 1. Registration could bind a reservation to the wrong wallet generation. SignedCoreTransaction now carries the unforgeable per-generation balance Arc (origin_generation) captured from the finalizing CoreWallet. SignedPaymentRegistry::register validates the supplied core against it and refuses a mismatch with the new typed RegisterWrongGeneration error, handing the rejected SignedCoreTransaction back so its reservation is not stranded. 2. Async registration could drop the reservation owner before insertion. register is now synchronous (its body has no await), so the consumed SignedCoreTransaction cannot be lost to a future dropped before its first poll. The FFI finalizer and all callers invoke it directly. 3. Kotlin: CoreTransactionBuilder.finalizeSignedPayment parses the native token first and releases it (owner-guarded) if SignedCoreTransaction construction throws, so an ABI/allocation/Cleaner failure never leaves the native token without a JVM owner. Adds a register_rejects_a_different_wallet_generation regression test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t + finalize (dashpay#4185 review) Wallet removal was not linearized with the deferred-payment registry, so a retained handle could push a removed wallet's payment onto the network. Two independent windows: 1. remove-then-sweep. `platform_wallet_manager_remove_wallet` called `manager.remove_wallet` and only afterwards swept the registry, with no shared lock spanning the two — and the removal's own awaits (shielded coordinator + identity-sync unregistration) sat in the gap. A concurrent `core_wallet_signed_payment_broadcast` in that window passed every guard: `is_same_generation` compares two handles, so a removed generation matches itself; `last_processed_height` is `None` once the wallet is gone and `reservation_expired` maps `None` to "not expired"; and `broadcast_payment_releasing_reservation` has no wallet-existence gate. 2. in-flight finalizer. `finalize_transaction` drops the manager write lock before awaiting the signer, and `register` only validates the payment against its finalizing generation — never that the generation still exists. A removal during the signer await swept the registry, then the finalizer inserted a fresh token no later sweep would catch, contradicting the documented teardown invariant that dropping tokens makes stale handles inert. Remedies: * `SignedPaymentRegistry` gains a lifecycle gate (`tokio::RwLock`). Teardown takes the exclusive side across BOTH the manager removal and the sweep, making them one linearization point; broadcast and release take the shared side for their whole duration. The existing `entries` mutex cannot do this — it is dropped before every await by design. Lock order is always gate then manager. * Broadcast rejects an absent current generation via the new `CoreWallet::is_current_generation`, returning `SignedPaymentError:: WalletRemoved` instead of silently proceeding to the broadcaster. * `core_wallet_signed_payment_finalize` holds the shared gate across its liveness check and the synchronous `register`, abandoning the payment (reconciling its reservation) if the wallet went away during signing. The gate is taken after the signer await, not around it, so an open signing prompt cannot stall teardown. No new FFI error code: the wallet-removed case is reported as the existing `NotFound` (98), which both hosts already map. Deliberately avoids the 29/30 renumbering contested in dashpay#4261. Swift/Kotlin/Rust docs updated to record that 98 now also carries this case, and how it differs from `ErrorReservationWalletMismatch` (29). Adds three FFI regression tests. All three fail against the pre-fix code — the race test reports a payment reaching the broadcaster after teardown completed. Also serializes the registry-count-asserting tests, which the new tests would otherwise race in the shared process-global registry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…30 (dashpay#4185 review) Code 29 collided with `ErrorAssetLockInsufficientFunds` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and this PR moves to 30. Verified 30 was genuinely free by reading `rs-platform-wallet-ffi/src/error.rs` at the head of all 62 open PRs: no PR defines a code 30. The `ErrorAssetLockCrossDomainConsentRequired` that in-tree comments name as 30's holder does not exist anywhere after dashpay#4184's re-scope. The discriminant is public ABI, so every mirror moves together: - Rust enum + its three rustdoc cross-references (error.rs) - two doc references in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value + doc - Kotlin fromPlatformWalletNative branch, class KDoc, code-98 comment, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also disarms the NativeCleaner backstop in SignedCoreTransactionTest by closing the SignedCoreTransaction, so the armed native release cannot fire from the cleaner thread in a pure-JVM test. Note: dashpay#4256 is stacked downstream and still carries the pre-renumber 29; it must adopt 30 on rebase.
…the final-alias policy removal (dashpay#4185 review) Blocker 2 removed the final-alias sweep from `platform_wallet_destroy` (wallet.rs:392-414 is now just a storage `remove`), but the helper that policy was introduced for survived it. `HandleStorage::any` was added by ade3999 for that sweep and has had zero call sites since the policy was dropped. Because `handle` is a `pub mod` and the method is `pub`, no dead-code lint fires and it stayed in the crate's public Rust surface, with Rustdoc still pointing at "the final-alias check in `platform_wallet_destroy`" — a policy that no longer exists. It is not on the base branch, so removing it restores the base surface rather than breaking an existing consumer. `HandleStorage::remove_matching` is retained: it still backs the generation sweep at manager.rs:494. Also corrects a doc cross-reference to the same removed policy in `test_support::test_platform_wallet_manager`, which described the helper as backing "final-alias registry-sweep gating" when its only FFI consumer now asserts the opposite (destroying wrapper aliases must NOT sweep). No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…on (dashpay#4185 review) The gate added in 0b0d5c7 lived on the FFI's process-global `SIGNED_PAYMENT_REGISTRY`, so it excluded only registry-token operations and did so across every wallet at once. Two consequences, both real: 1. Under-coverage. The V2 finalized-transaction-handle path bypassed it entirely. `core_wallet_tx_builder_finalize` awaited the external signer and then inserted into `CORE_SIGNED_TRANSACTION_V2_STORAGE` with no gate and no liveness re-check, so a teardown could sweep while signing was pending and the late finalizer published a handle no sweep would ever catch. `core_wallet_broadcast_signed_transaction_v2` then consumed such a handle and reached the broadcaster with no gate either — its `is_same_generation` check compares two HANDLES, and a removed generation matches itself. The same hole existed on the public Rust surface: `PlatformWalletManager::remove_wallet` never took the write side at all, so a direct embedder (the manager is public and `SignedPaymentRegistry` is re-exported) removed wallets with no exclusion. 2. Cross-wallet contention. A deferred broadcast holds the shared side across an SPV send; on one process-global write-preferring lock that send blocked teardown — and every payment operation queued behind the waiting writer — for every unrelated wallet in the process. Remedy: move the gate into shared per-generation state. * New `WalletGeneration` owns the lock-free `WalletBalance` AND that generation's `RwLock` lifecycle gate, and replaces `Arc<WalletBalance>` as the generation-identity marker. Folding them into one `Arc` is deliberate: the identity and the gate cannot diverge, so two handles can never compare as the same generation while excluding each other through different locks. `Deref` keeps every existing balance read unchanged. * `PlatformWalletManager::remove_wallet_with_teardown` takes that generation's exclusive gate across BOTH the removal and a caller-supplied teardown hook, and `remove_wallet` routes through it. The gate is no longer optional for any caller, FFI or not. The FFI passes its registry + V2-handle sweep as the hook. Lock order stays gate-then-manager: the lookup that resolves the gate drops `wallets` before awaiting it, then re-validates under the gate. * Every publication/network path now takes the generation's shared gate across its liveness check and the action: the registry `broadcast`/`release`, the token `core_wallet_signed_payment_finalize`, and — newly — both `core_wallet_tx_builder_finalize` and `core_wallet_broadcast_signed_transaction_v2`, which report a dead generation as the existing `NotFound` (98) after reconciling the build's reservation. V2 abandon/free stay ungated: their release is already generation-bound. The gate is still NOT held across an external signer await — finalizers acquire it only after the signature returns, so an open signing prompt cannot stall teardown, and a late finalizer instead fails its liveness check and abandons. The lifecycle comments that claimed the opposite were wrong about the code and are corrected. Adds four deterministic FFI regression tests alongside the existing three: a V2 broadcast-after-removal refusal, a teardown that waits for an in-flight V2 operation and then sweeps its handle, a public `PlatformWalletManager::remove_wallet` that waits for an in-flight payment, and a cross-wallet isolation test pinning the per-generation scoping. With the three guards removed the first three fail; with the gate re-pointed at a single process-global lock the fourth fails. No error-code changes: `ErrorReservationWalletMismatch` stays 30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…y key `remove_wallet_with_teardown` validated generation G1 under G1's lifecycle gate, then removed it from the two manager maps in two independently locked stages. Registration takes no gate at all — `register_wallet` mints its own `WalletGeneration` — so from the moment the inner-manager removal frees the id, a concurrent same-id registration can publish a different generation G2 into `wallet_manager` and then into `self.wallets`, with no happens-before edge to the remover's own `self.wallets` acquisition. A remover descheduled in that gap resumed into a map naming G2 and removed the entry BY KEY: it evicted a live wallet (still registered in the inner manager, so invisible and unremovable through the public map), returned it to the caller, and handed it to `tear_down` — which sweeps that generation's registry tokens and V2 finalized-transaction handles while holding only G1's gate, i.e. with G2's payment operations not excluded. That exclusion is the one property the gate exists to provide. Retain the `Arc<PlatformWallet>` validated under the gate and remove the public-map entry only while it still pointer-matches that generation, so the removed handle, the returned handle and the `tear_down` argument are all the one generation this call validated. The inner-manager removal needs no such check: G1 can only leave `wallet_manager` through this method (which requires G1's gate) or through a rollback for an insert that could not have happened while G1 occupied the id. Regression test `removal_leaves_a_generation_registered_during_it_intact` drives the real `create_wallet_from_seed_bytes` -> `register_wallet` path from a `cfg(test)` rendezvous fired in the exact window, so the interleaving is pinned with no sleep and no completion-order race. Against the previous code it fails on all three load-bearing assertions: the returned generation, the `tear_down` argument, and the survival of the re-registered wallet in the public map. Refs dashpay#4185 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with this PR's `ErrorStaleReservationToken = 27`. Renumber the deferred build/broadcast trio to the contiguous block 34-36, which sits above every code currently claimed by a merged commit or an open PR: 27 ErrorShutdownIncomplete MERGED, dashpay#4268 29 ErrorAssetLockInsufficientFunds dashpay#4184 31 ErrorSigningKeyUnavailable dashpay#4183, dashpay#4259 32 ErrorTransactionBuild dashpay#4247, dashpay#4256 33 ErrorTransactionSigning dashpay#4256 28 and 30 are vacated and return to the free pool. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (which has no compile-time cross-ABI check, so it was verified by grep). Also addresses three review suggestions: * `PlatformWalletInfo::generation` is now `pub(crate)`. It was publicly assignable through `state_mut()` / `state_mut_blocking()`, so downstream safe code could swap the `Arc` while `PlatformWallet` and `CoreWallet` kept the original — splitting the generation identity `Arc::ptr_eq` compares, which would make `is_current_generation()` reject a live wallet, turn generation-bound reservation cleanup into a no-op, and let teardown exclude through a different lifecycle gate than the payments it must fence. All construction and mutation sites are already inside the crate. * `buildSignedPayment` now runs under `opWithCleanupOnCancellation`. Native finalization mints the token before the blocking JNI call returns, so `withContext`'s prompt-cancellation handoff could discard the completed `SignedCoreTransaction` and leave the reservation to the GC Cleaner or the TTL. The discarded result is now closed deterministically. * Native code 26 (`ErrorTransactionBroadcastRejected`) no longer falls through to `PlatformWallet.Generic`. It maps to a dedicated `TransactionBroadcastRejected` subtype so callers can tell a definitively rejected, consumed-and-released payment (rebuild it) from an unrelated generic wallet failure, with its non-retry-in-place semantics pinned in `DashSdkErrorTest`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
force-pushed
the
port/v4.1/split-build-broadcast
branch
from
August 4, 2026 05:45
8813e98 to
326cd3e
Compare
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 4, 2026
…ilds `build_signed_payment` reserves its selected inputs and leaves them reserved on success, expecting a broadcast to follow. Nothing across Rust/FFI/JNI/Kotlin let a caller that declines to commit give those coins back, so an abandoned build stranded them for RESERVATION_TTL_BLOCKS (24, ~1h) — and indefinitely while the wallet has no processed height, since `ReservationSet::sweep` early-returns at height 0 and therefore never reclaims a pre-sync reservation. A single abandoned build on a freshly restored wallet could strand the whole balance for the life of the process. Adds a standalone release across all four layers: CoreWallet::release_payment_reservation(&Transaction, Option<DerivationPath>) core_wallet_release_payment_reservation (FFI) coreWalletReleasePaymentReservation (JNI) ManagedPlatformWallet.releasePaymentReservation (Kotlin) The transaction is the ownership signal: a reserved outpoint is skipped by every other build's coin selection, so no concurrent build can hold a reservation on any input of the transaction being released — releasing its inputs releases precisely this build's own reservation and can never free a competing build's coins. Same signal the internal `release_reservation_after_rejected_broadcast` cleanup already uses. The release consults no height, so it works pre-sync where the TTL backstop cannot. It is idempotent (per-outpoint map removal) and a silent no-op after a successful broadcast — it cannot resurrect a spent coin, since selection reads the UTXO set that sync already updated — so callers can wire it into an unconditional cleanup path. Also routes the build's default-funding-account resolution through the shared `bip44_account_path` helper, so a release and its build can never disagree about what `funding_path: None` means. Tests: release-then-reselect (with the second-build failure pinned as a precondition so it can't pass vacuously), release twice, release after a processed broadcast, release against the wrong account frees nothing, unknown funding path is refused, and the height-0 case — 30 build attempts prove the TTL never fires there, then the explicit release frees the inputs. Addresses review item 3 on dashpay#4247. The reviewer's suggestion to unify this with dashpay#4256's reservation-token finalize is tracked separately rather than done here, to avoid reshaping an API the Android app already calls and hard-coupling this PR to dashpay#4185. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 4, 2026
…nd-path test fixtures Restack adaptation only — no production-code change. This branch now sits on top of dashpay#4185 (port/v4.1/split-build-broadcast) rather than beside it. dashpay#4185 replaced the bare `Arc<WalletBalance>` generation marker with `Arc<WalletGeneration>` (balance + that generation's lifecycle gate in one Arc, so "same generation" and "same gate" cannot diverge), and renamed `PlatformWalletInfo::balance` to `::generation`. The send-path test fixtures added here still built wallets the old way, so they no longer compiled against the new base. Point them at the shared `WalletGeneration` the rest of the crate already uses: * `core/send.rs` — the local `core_wallet` fixture takes `Arc<WalletGeneration>`; `funded_wallet_manager` already hands one back. * `wallet/funding_privacy.rs` — same, for its two fixtures. * `test_support.rs` — the DashPay split fixture populates `PlatformWalletInfo::generation`. cargo test -p platform-wallet -p platform-wallet-ffi: 537 + 230 pass.
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 4, 2026
…#4185's registry contract Restack adaptation. This branch now sits on top of dashpay#4247 (which sits on dashpay#4185) instead of beside them, so the funding-path registration meets the contract dashpay#4185 established for the deferred-payment registry: * `RegisteredPayment` keeps ONE funding handle — dashpay#4256's `funding: FundingAccountRef` — and dashpay#4185's MANDATORY `registered_height: u32`. The two evolutions are orthogonal (which account vs. which clock), so the union is a strict superset: the Path arm still reaches a DashPay receiving-funds account, and the age guard can no longer be silently disabled by a `None` height. * `register_funded_by` therefore takes `registered_height: u32`. `FinalizedCorePayment::reservation_height` was already a non-optional `u32` sampled inside the funding critical section, so every caller simply drops its `Some(..)` wrapper — no behaviour change, one less way to disable the guard. * `ReservationToken` is dashpay#4185's newtype; the FFI converts with `as_u64` at the C ABI boundary. * The over-limit refusal binds the payment first and discharges its reservation through `abandon_payment` before returning, rather than stranding the account's coins until the TTL backstop. * Test fixtures build wallets with `WalletGeneration` (balance + the generation's lifecycle gate in one Arc) and read `PlatformWalletInfo::generation`. `register_funded_by` still cannot prove its `core` is the generation that produced the payment the way `register` can — `FinalizedCorePayment` carries no `origin_generation` marker, so the FFI's single `generation_payment_guard` hold is what upholds the binding. That gap is pre-existing on this branch and is documented on the method as follow-up. cargo test -p platform-wallet -p platform-wallet-ffi: 540 + 230 pass.
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 4, 2026
…ode (33) `map_send_builder_error` folded `BuilderError::SigningFailed` into `TransactionBuild` → native code 32, whose documented contract is "the request itself is at fault; a verbatim retry fails identically". That is false for the production `MnemonicResolverCoreSigner`: a locked or missing Keychain mnemonic surfaces as `SigningFailed`, and key-wallet `release_if_owner`-releases the build's owner-stamped input reservation before returning, so the identical recipients/amount/fee/funding path succeed once the signer is usable. Hosts were being told to make the user edit a payment that was never wrong. Adds `PlatformWalletError::TransactionSigning` → `ErrorTransactionSigning = 33` → `DashSdkError.PlatformWallet.TransactionSigning` (isRetryable = true, matching the ShieldedNoRecordedAnchor convention for "nothing committed, reservations released, retry once the precondition is met"). Code choice — 33, not the reviewer's suggested 31. 27-32 are all claimed across the sibling v4.1 stack (27/28 dashpay#4185, 29 dashpay#4184, 30 reserved for free on every branch. 31 IS reserved, but for a different contract: asserting the signer holds no usable private key for a requested public key, restored from the typed `DashSDKSignerErrorCode::SigningKeyUnavailable`. This is a Core L1 input signing failure with no such provenance — `BuilderError::SigningFailed` also covers an unresolved input derivation path, a sighash computation failure and a malformed signature encoding — so reusing 31 would assert "the key is unavailable" for failures that are nothing of the kind. Kept separate so neither contract is weakened; the rationale is recorded on the variant for maintainers reconciling the range. Tests: an end-to-end locked-signer build asserting TransactionSigning (not TransactionBuild), a retry-after-recovery test proving the inputs really were released, FFI code/mapping tests, and the Kotlin decode + retry-contract test. platform-wallet 539 passed, platform-wallet-ffi 225 passed, kotlin-sdk 190 passed; fmt clean, no new clippy warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 4, 2026
…roadcast A pinned V2 finalized-transaction handle (core_wallet_tx_builder_finalize → broadcast_finalized_transaction) had no reservation 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 — the same stale-release hazard the deferred registry-token path already defends against. This becomes live the moment iOS starts issuing deferred sends (follow-up requested on PR dashpay#4185). Mirror the registry-token age policy on the V2 handle path: - Hoist RESERVATION_MAX_AGE_BLOCKS (20) and reservation_expired() from signed_payment_registry into wallet::reservations so both the registry and the V2 handle path bound a reservation's lifetime against key-wallet's TTL with one shared number. - broadcast_finalized_transaction now refuses, before touching the broadcaster, once current last_processed_height - the reservation's stamp height (already carried on SignedCoreTransaction::reservation_height) >= the shared bound, returning the new token-less PlatformWalletError::StaleReservation. The stale reservation is left for key-wallet's TTL to reclaim (never released by outpoint, which could free a newer build's reservation). The check runs after the FFI layer's generation-identity check, matching the registry ordering. - The FFI reuses the existing ErrorStaleReservationToken (26) code for this variant (documented as shared between the registry-token and V2-handle surfaces); no new codes allocated. - Abandon/free (abandon_transaction) remain allowed at any age — releasing an old reservation is always safe. Tests: fresh handle broadcasts; aged handle refuses with StaleReservation yet still abandons cleanly and frees its inputs; exact boundary at the threshold (BIP44/BIP32); FFI mapping of StaleReservation to the shared code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 4, 2026
shumkov (PR dashpay#4185 follow-up) found the age guard covered only broadcast: `abandon_transaction` — and therefore the `_v2_free` deinit/GC backstop and the FFI broadcast/abandon failure paths that route their cleanup through it — still released the funding reservation by outpoint unconditionally at any age. A FinalizedCoreTransaction GC'd after ~1h whose outpoint was TTL-swept (24 blocks) and re-reserved would free the newer build's reservation, letting its inputs be re-selected into a third build (conflicting spends). Honor `reservation_expired` in `abandon_transaction`, mirroring the registry's `reconcile_removed_entry`: once aged past the shared `RESERVATION_MAX_AGE_BLOCKS` bound, skip the by-outpoint release (leave the outpoint for key-wallet's TTL to reclaim) while still tearing down the handle; below the bound, release as before. This covers every consumer of `abandon_transaction`, including the `_v2_free` GC-backstop and the FFI failure paths, off the same predicate/clock the broadcast guard uses. Also correct the reservation-policy docs that claimed releasing was always safe (`reservations.rs`, `broadcast_finalized_transaction`), and the misleading ManagedCoreWallet KDoc: after a stale-refused broadcast the handle is already consumed, so `abandonTransaction` is an invalid-handle error, not a recovery — the reservation waits out the TTL. Tests: platform-wallet gains aged-skips-release / below-bound-releases pairs (BIP44+BIP32); platform-wallet-ffi gains aged `_v2_free` and aged failure-path skip-release tests via a new `age_core_past_reservation_guard` test helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Aug 4, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
At the exact head, four carried-forward findings are fixed: same-generation removal, crate-private generation identity, cancellation cleanup, and typed Kotlin broadcast rejection; the personal-fork dependency remains blocking. A newly identified full-PR blocker remains in finalize_transaction: a retained stale generation can reserve the recreated generation's UTXOs, after which generation-bound cleanup cannot release them. The PR body's 27/28/29 mapping and planned 29-to-30 allocation note are obsolete because Rust, Kotlin, JNI documentation, and Swift now consistently use 34/35/36.
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)
🔴 2 blocking
1 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/transaction.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/transaction.rs:258-260: Reject a stale wallet generation before reserving current inputs
`finalize_transaction` resolves mutable wallet state only by `wallet_id`; it does not verify that `info.generation` pointer-matches this `CoreWallet` before funding and reserving inputs. After generation G1 is removed and the same wallet ID is recreated as G2, a retained G1 handle therefore selects and reserves G2's live UTXOs, but the returned ownership object is stamped with G1's generation. The FFI post-signing liveness check then rejects the stale G1 operation and calls `G1.abandon_transaction`, whose generation check correctly refuses to mutate G2. The reservation created in G2 is consequently stranded until key-wallet's TTL, and repeated calls through the stale handle can temporarily immobilize the current wallet's UTXOs. Compare the generation under the existing manager write lock before funding, and add a remove/recreate-then-finalize regression test.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 27 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-jniall pass (504 / 229 / 10);:sdk:assembleRelease+ sdk unit tests pass.🤖 Generated with Claude Code
Summary by CodeRabbit
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,
AutoCloseableKotlin object with a GC/Cleanerbackstop, 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 V2CoreWallethandle 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:
platform_wallet_destroyreleases 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.SignedCoreTransactionandregisteruses it — no post-signing resample; boundary test pins the TTL margin.CoreWallet::is_same_generation(per-generation identity) is checked by BOTH the V2-handle and registry-token paths, with one teardown policy.SignedCoreTransactionis an owningAutoCloseablewith aNativeCleanerbackstop; round-2 adds object-owningbroadcastSigned/releaseReservationoverloads 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).broadcastpeeks 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_registerfour-layer chain is deleted; the single stale-token code is split into typed siblings 27ErrorStaleReservationToken/ 28ErrorReservationTokenConsumed/ 29ErrorReservationWalletMismatch(code 26 is not used by this PR — upstream now owns it asErrorTransactionBroadcastRejected; both the Kotlin and Swift enums on this branch map 27/28/29 explicitly); the stalebuildSignedPaymentKDoc is fixed.Local test evidence (fork PRs skip the Rust CI suite):
platform-wallet --lib508 passed,platform-wallet-ffi --lib197 passed, clippy/fmt clean, Kotlin:sdk:testDebugUnitTestgreen.