Skip to content

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

Open
bfoss765 wants to merge 34 commits into
dashpay:v4.2-devfrom
bfoss765:port/v4.1/split-build-broadcast
Open

feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission#4185
bfoss765 wants to merge 34 commits into
dashpay:v4.2-devfrom
bfoss765:port/v4.1/split-build-broadcast

Conversation

@bfoss765

@bfoss765 bfoss765 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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-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, enabling transactions to be built and signed before later broadcast or release.
    • Added payment details including transaction ID, raw transaction data, fees, and reservation tokens.
    • Added idempotent reservation release for abandoned payments.
    • Added typed reservation and wallet-mismatch errors across supported SDKs.
  • Bug Fixes
    • Improved cleanup when wallets close while deferred payments remain active.
    • Prevented reservations from being used with the wrong wallet, after expiration, or more than once.

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; the single stale-token code is split into typed siblings 27 ErrorStaleReservationToken / 28 ErrorReservationTokenConsumed / 29 ErrorReservationWalletMismatch (code 26 is not used by this PR — upstream now owns it as ErrorTransactionBroadcastRejected; both the Kotlin and Swift enums on this branch map 27/28/29 explicitly); the stale buildSignedPayment KDoc is fixed.

Error-code allocation note. These 27 / 28 / 29 allocations are being reconciled repo-wide in the error-code registry PR #4261. That registry records that ErrorReservationWalletMismatch = 29 on this branch currently collides with ErrorAssetLockInsufficientFunds = 29 on the asset-lock PR #4184, and that code 30 is free after #4184's re-scope (the variant previously reserved at 30 is not defined anywhere). The resolution of record is that #4184 keeps 29 and this PR moves its mismatch code to 30; that renumber has not yet landed on this head.

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

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@bfoss765, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0f2f7701-3eb1-4b33-a7d3-37e7b2f345da

📥 Commits

Reviewing files that changed from the base of the PR and between b5023dc and 8813e98.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (12)
  • Cargo.toml
  • 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/ManagedPlatformWallet.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt
  • packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs
  • packages/rs-platform-wallet-ffi/src/error.rs
  • packages/rs-platform-wallet-ffi/src/manager.rs
  • packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs
  • packages/rs-platform-wallet/src/wallet/platform_wallet.rs
  • packages/rs-unified-sdk-jni/src/wallet_manager.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
📝 Walkthrough

Walkthrough

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

Changes

Deferred signed payment lifecycle

Layer / File(s) Summary
Wallet generation and reservation registry
packages/rs-platform-wallet/src/wallet/core/*, packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs, packages/rs-platform-wallet/src/manager/*
Adds generation-bound reservations, atomic registration, broadcast, release, expiry handling, teardown synchronization, and wallet recreation checks.
Native FFI lifecycle
packages/rs-platform-wallet-ffi/src/core_wallet/*, packages/rs-platform-wallet-ffi/src/manager.rs, packages/rs-platform-wallet-ffi/src/error.rs, packages/rs-platform-wallet-ffi/src/wallet.rs
Adds deferred finalization, token broadcast and release, generation validation, error codes, handle cleanup, and lifecycle tests.
JNI and Kotlin API
packages/rs-unified-sdk-jni/src/wallet_manager.rs, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/*, packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/*
Adds signed-payment construction, ownership cleanup, broadcast and release operations, native bindings, typed errors, and JVM tests.
Swift and workspace support
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift, packages/rs-platform-wallet/src/test_support.rs, Cargo.toml
Maps reservation errors in Swift, adds wallet test fixtures, and updates Rust dependency sources.

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
Loading

Possibly related issues

Possibly related PRs

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

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main Kotlin SDK change: separating payment construction and broadcasting with reservation release for deferred submission.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch port/v4.1/split-build-broadcast
🧪 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

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

@github-actions github-actions Bot added this to the v4.1.0 milestone Jul 21, 2026
@thepastaclaw

thepastaclaw commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

🕓 Ready for review — 12 ahead in queue (commit 8813e98)
Queue position: 13/17 · 2 reviews active
ETA: start ~01:26 UTC · complete ~01:43 UTC (median 17m across 30 recent reviews; 2 slots)
Queued 5m ago · Last checked: 2026-08-03 23:30 UTC

@shumkov

shumkov commented Jul 21, 2026

Copy link
Copy Markdown
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:

  • core_wallet_signed_payment_registercoreWalletRegisterSignedPaymentWalletManagerNativeManagedCoreWallet.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

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread packages/rs-platform-wallet-ffi/src/error.rs Outdated
@shumkov

shumkov commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

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 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>
@bfoss765

Copy link
Copy Markdown
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 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.

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>
@shumkov

shumkov commented Jul 22, 2026

Copy link
Copy Markdown
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:

  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

shumkov commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Addendum: the missing Swift mappings for the new codes belong to this PR too, not only #4196PlatformWalletResult.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 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>
@bfoss765

Copy link
Copy Markdown
Contributor Author

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.

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>
@shumkov

shumkov commented Jul 23, 2026

Copy link
Copy Markdown
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 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.

bfoss765 and others added 2 commits July 23, 2026 11:36
…BIP70 deferred submission

BIP70/BIP270 (CTX/DashSpend) sends must sign, POST the raw bytes to a
merchant server, and broadcast only on ack — structurally impossible on the
one-shot `sendToAddresses`. Expose the existing internal build/broadcast split
with an explicit reservation lifecycle, keeping `CoreTransactionBuilder`
internal so the manager stays the sole driver of the setFunding/buildSigned
race.

Rust core (rs-platform-wallet):
- New `SignedPaymentRegistry`: a generic, in-memory registry that owns a
  built+signed tx and its held UTXO reservation between build and submission,
  keyed by an opaque `ReservationToken`. `broadcast` removes the entry before
  sending (no double-broadcast — a repeat/concurrent call gets `StaleToken`),
  binds each token to its originating wallet instance (`Arc::ptr_eq` on the
  shared `WalletManager`, so a re-created wallet is rejected), and reconciles
  the reservation on failure via the existing release-on-rejection path.
  `release` is idempotent. Reservations are memory-only, so a crash between
  build and broadcast drops both the entry and the reservation on restart —
  the same property dashj has.
- `CoreWallet::release_transaction_reservation` — the explicit "abandoned /
  nacked" release arm.

FFI (platform-wallet-ffi) — additive C ABI:
- `core_wallet_transaction_get_bytes`, `core_wallet_signed_payment_register`
  (token + fee + txid), `core_wallet_signed_payment_broadcast`,
  `core_wallet_signed_payment_release`, backed by one process-global registry
  pinned to `SpvBroadcaster`.
- New `ErrorStaleReservationToken` (22) result code.

JNI (rs-unified-sdk-jni) — additive: `coreTransactionGetBytes`,
`coreWalletRegisterSignedPayment` (BLOB), `coreWalletBroadcastSignedPayment`,
`coreWalletReleaseSignedPayment`.

Kotlin — additive: `ManagedPlatformWallet.SignedCoreTransaction`,
`buildSignedPayment` (build under coreSendMutex), `broadcastSigned(token)`,
`releaseReservation(token)`; `DashSdkError.PlatformWallet.StaleReservationToken`.
No existing signatures change.

Refs dashpay#4089, dashpay/dash-wallet#1507 Phase 5c GAP-4.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er/release

Address review of the SignedPaymentRegistry deferred build→broadcast/release
flow.

BLOCKING: registry tokens never expired even though the key-wallet UTXO
reservation they depend on is swept after RESERVATION_TTL_BLOCKS (24) and
released by raw outpoint with no ownership check, so a long-outstanding
token's broadcast/release could free or spend against an unrelated newer
reservation. Bound the token lifetime: capture the wallet's synced height at
register and refuse broadcast/release once the wallet has synced
RESERVATION_MAX_AGE_BLOCKS (20, < TTL) past it, returning the typed
StaleReservationToken WITHOUT releasing (which could free a newer build's
reservation). The pinned key-wallet exposes no per-outpoint generation check,
so this client-side bound is the primary guard.

Also:
- WalletMismatch now compares wallet_id in addition to Arc::ptr_eq on the
  shared WalletManager, so two wallets in one multi-wallet manager are told
  apart.
- register() returns the raw tx bytes in the same native call and the JNI
  folds them into the register BLOB; the now-unused core_wallet_transaction_get_bytes
  / coreTransactionGetBytes is removed (one native round trip per kotlin-sdk rule).
- register() does its fallible/pure marshalling before the reservation-holding
  insert, and the JNI releases the token if it can't hand the BLOB back to
  Kotlin — no orphaned reservation on a marshalling failure.
- PlatformWallet teardown sweeps the registry of that wallet's tokens so a
  destroyed wallet's WalletManager is no longer pinned alive by a captured
  CoreWallet clone (hooked at platform_wallet_destroy, not the transient
  core-handle destroy the deferred flow cycles through).
- Registry mutex recovers from poisoning instead of panicking, matching
  key-wallet's ReservationSet.

Adds tests for token expiry (broadcast + release), same-manager different
wallet_id mismatch, and the teardown sweep.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@bfoss765

bfoss765 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Verified all three findings at 6c37e8679e before changing anything — all three hold. Fixed in d1bed4dc78.

The V2 path was ungated on both ends: core_wallet_tx_builder_finalize inserted into CORE_SIGNED_TRANSACTION_V2_STORAGE with no gate and no liveness re-check after the signer await, and core_wallet_broadcast_signed_transaction_v2 reached broadcast_finalized_transaction with neither — its is_same_generation check compares two handles, and a removed generation matches itself. Public removal took no exclusion at all; the gate lived only in the FFI wrapper, so a direct Rust embedder bypassed the linearization entirely. Contention was as described: one Lazy static, the shared side held across the SPV send, write-preferring lock — so one wallet's send blocked every other wallet's teardown, and their payments behind the queued writer.

On the nitpick — agreed, and the comments were the thing that was wrong. Finalizers deliberately take the gate after the signature returns, because holding it across an open signing prompt would stall teardown for every wallet. Corrected in manager.rs and in the gate's Rustdoc.

Design. The gate now lives in a new WalletGeneration that is the generation-identity marker — it owns the lock-free WalletBalance and that generation's RwLock, replacing Arc<WalletBalance> wherever identity was compared. One Arc rather than a parallel gate Arc is deliberate: threaded separately through ~15 construction sites, a future site could clone the identity but mint a fresh gate, and two handles would then compare as the same generation while excluding each other through different locks — a silent loss of exclusion with nothing to fail on. Deref<Target = WalletBalance> keeps every existing balance read unchanged.

PlatformWalletManager::remove_wallet_with_teardown holds the generation's exclusive gate across both the removal and a caller-supplied synchronous teardown hook, and remove_wallet routes through it, so the gate cannot be skipped by any caller; the FFI passes its registry + V2-handle sweep as that hook. Lock order stays gate -> manager: the lookup drops wallets before awaiting the gate, then re-validates under it and retries if the generation changed.

Paths gated (swept rather than taken from the finding's list): registry broadcast and release — the latter has no wallet argument, so it peeks the entry's generation under the map mutex, drops it, takes that gate, then consumes — plus core_wallet_signed_payment_finalize, and newly core_wallet_tx_builder_finalize and core_wallet_broadcast_signed_transaction_v2. Both new ones report a dead generation as the existing NotFound (98) after reconciling the reservation, so nothing is stranded.

Also swept: the third CORE_SIGNED_TRANSACTION_V2_STORAGE.insert is inside #[cfg(test)]; V2 abandon/free stay ungated since their release is already generation-bound under a single manager-lock hold. One judgment call flagged rather than skipped silently — the immediate-send path (core_wallet_broadcast_transaction) retains no handle across teardown, so it sits outside the deferred-lifecycle contract and I did not gate it. Say if you'd rather it were covered.

Tests: four new deterministic ones. With the three guards removed, broadcasting_a_v2_handle_for_a_removed_wallet_is_refused_before_the_network, teardown_waits_for_an_in_flight_v2_operation_and_then_sweeps_its_handle and public_remove_wallet_waits_for_an_in_flight_payment_on_that_generation all fail; with the gate re-pointed at a single process-global lock, an_in_flight_payment_does_not_block_an_unrelated_wallets_teardown fails with Elapsed.

Worth flagging: I first wrote two of these in the completion-order style of the existing a_broadcast_never_reaches_the_broadcaster_after_teardown_completed, and they passed against the broken code on some runs. That style is timing-dependent and isn't a reliable gate; they were replaced with the deterministic forms above.

ErrorReservationWalletMismatch stays 30; HandleStorage::any stays removed with remove_matching retained at its live call site. cargo fmt --check clean; cargo test -p platform-wallet-ffi -p platform-wallet green — 513 + 212 lib tests, 0 failures across all 11 result lines. Dependent crates all still compile (the PlatformWalletInfo.balance -> .generation rename is source-breaking).

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review — Codex only

Both carried-forward lifecycle findings are fixed at d1bed4d: lifecycle exclusion is now shared per wallet generation across direct removal, registry-token operations, and V2 operations, and the finalizer comments match the post-signing gate acquisition. The verifier ran the lifecycle and registry suites successfully (7/7 and 19/19), but the current head still has two blockers: same-ID registration can race the manager's two-map removal sequence, and all rust-dashcore dependencies remain pinned to an unmerged contributor fork. Three additional in-scope suggestions cover generation-state mutability, deterministic Kotlin cancellation cleanup, and typed Kotlin handling for definitive broadcast rejection.

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 | 🟡 3 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs:655-687: Remove the same wallet generation from both maps atomically
  The retry loop acquires generation G1's teardown gate and verifies that `self.wallets` still names G1, but removal then happens in two independently locked stages. After G1 is removed from the inner `wallet_manager`, that lock is released before `self.wallets` is acquired. A concurrent same-ID registration can now insert generation G2 into the inner manager and publish G2 into `self.wallets`; if that publication wins on a multithreaded executor, this remover takes G2 from the outer map and invokes `tear_down` for G2 while holding only G1's gate. G2 remains registered in the inner manager but disappears from the public map, and its registry tokens or V2 handles can be swept without excluding G2's payment operations. Give registration and removal a single per-ID ordering, or retain the verified G1 `Arc<PlatformWallet>` and remove the outer entry only when it still pointer-matches G1. Add a deterministic remove-versus-recreate regression test.

In `Cargo.toml`:
- [BLOCKING] Cargo.toml:55-62: Do not merge workspace-wide dependencies from a personal fork
  All eight rust-dashcore workspace dependencies are redirected from `dashpay/rust-dashcore` to `bfoss765/rust-dashcore` at e99959ce. That revision is only the first commit of dashpay/rust-dashcore#916, which is currently OPEN with `mergeStateStatus=BLOCKED` and `reviewDecision=REVIEW_REQUIRED`; the upstream PR has since added further fixes, including owner-guarded cleanup for asset-lock downstream failures. Merging this head would therefore make the entire workspace depend on a contributor-controlled repository and a stale, unmerged implementation. Land the reservation API and its follow-up fixes in the Dash-owned repository, then repin these entries and `Cargo.lock` to the corresponding Dash-owned revision.

In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:52: Keep the generation identity immutable outside the crate
  `PlatformWalletInfo` is exposed mutably through `PlatformWallet::state_mut()` and `state_mut_blocking()`, so downstream safe Rust can replace this `Arc<WalletGeneration>`. `PlatformWallet` and `CoreWallet` retain the original Arc, causing the inner manager's generation identity and lifecycle gate to diverge from every existing handle. `is_current_generation()` then rejects the otherwise live wallet, generation-bound reservation cleanup becomes a no-op, and teardown can exclude through a different gate than payment operations. This synchronization identity must not be publicly assignable.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt:316: Release the registered token when cancellation discards the result
  Native finalization registers the payment and transfers reservation ownership to a token before the blocking JNI call returns. `gate.op` uses `withContext(Dispatchers.IO)`, whose prompt-cancellation handoff can discard the newly constructed `SignedCoreTransaction` when the caller is cancelled while JNI is running. The only remaining cleanup is then the GC Cleaner, so the token and funding reservation can remain held until an unpredictable GC cycle or the reservation TTL. Use the existing `opWithCleanupOnCancellation` helper to close a completed result that cancellation prevents from reaching the caller.

In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:296-309: Preserve definitive broadcast rejection as a typed Kotlin error
  A definitive deferred-payment broadcast rejection is preserved as native code 26 through the C ABI and JNI, but this switch falls through to `PlatformWallet.Generic`. Callers can inspect `Generic.nativeCode` manually, but the public typed hierarchy otherwise distinguishes ambiguous broadcast outcomes and all deferred-token failures. The new `broadcastSigned` API should likewise expose definitive rejection as a dedicated subtype so callers can distinguish a consumed-and-released rejected payment, which must be rebuilt or corrected, from unrelated generic wallet failures. Add the subtype, map code 26, and pin its non-retry-in-place/rebuild semantics in `DashSdkErrorTest`.

Comment thread Cargo.toml
Comment on lines +55 to +62
dashcore = { git = "https://github.com/bfoss765/rust-dashcore", rev = "e99959ced0062159d629930f488374e29f63c42b" }
dash-network-seeds = { git = "https://github.com/bfoss765/rust-dashcore", rev = "e99959ced0062159d629930f488374e29f63c42b" }
dash-spv = { git = "https://github.com/bfoss765/rust-dashcore", rev = "e99959ced0062159d629930f488374e29f63c42b" }
key-wallet = { git = "https://github.com/bfoss765/rust-dashcore", rev = "e99959ced0062159d629930f488374e29f63c42b" }
key-wallet-ffi = { git = "https://github.com/bfoss765/rust-dashcore", rev = "e99959ced0062159d629930f488374e29f63c42b" }
key-wallet-manager = { git = "https://github.com/bfoss765/rust-dashcore", rev = "e99959ced0062159d629930f488374e29f63c42b" }
dash-network = { git = "https://github.com/bfoss765/rust-dashcore", rev = "e99959ced0062159d629930f488374e29f63c42b" }
dashcore-rpc = { git = "https://github.com/bfoss765/rust-dashcore", rev = "e99959ced0062159d629930f488374e29f63c42b" }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Do not merge workspace-wide dependencies from a personal fork

All eight rust-dashcore workspace dependencies are redirected from dashpay/rust-dashcore to bfoss765/rust-dashcore at e99959ce. That revision is only the first commit of dashpay/rust-dashcore#916, which is currently OPEN with mergeStateStatus=BLOCKED and reviewDecision=REVIEW_REQUIRED; the upstream PR has since added further fixes, including owner-guarded cleanup for asset-lock downstream failures. Merging this head would therefore make the entire workspace depend on a contributor-controlled repository and a stale, unmerged implementation. Land the reservation API and its follow-up fixes in the Dash-owned repository, then repin these entries and Cargo.lock to the corresponding Dash-owned revision.

source: ['codex']

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — this is the one finding that cannot be resolved inside this PR. The pin exists because this PR's reservation lifecycle requires the key-wallet reservation API introduced in dashpay/rust-dashcore#916, which is not yet in a Dash-owned revision. The resolution path is fixed: once #916 merges, the eight Cargo.toml entries and Cargo.lock get repinned to the corresponding dashpay/rust-dashcore revision before this PR lands.

Current #916 status (verified 2026-08-02): OPEN, CI green (35 checks passing), CodeRabbit-approved, blocked only on its required human CODEOWNER review. The note that the upstream PR has since added follow-up fixes beyond e99959ce is also part of the repin — the final pin will be to the merged head, not the current commit.

Keeping this thread open until the repin lands is correct; nothing else in this PR depends on which revision the pin points at.

/// This wallet generation's shared state: the lock-free balance for UI reads
/// (updated from `ManagedWalletInfo` after each SPV block/mempool processing
/// and RPC refresh) and the generation's lifecycle gate.
pub generation: Arc<WalletGeneration>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Keep the generation identity immutable outside the crate

PlatformWalletInfo is exposed mutably through PlatformWallet::state_mut() and state_mut_blocking(), so downstream safe Rust can replace this Arc<WalletGeneration>. PlatformWallet and CoreWallet retain the original Arc, causing the inner manager's generation identity and lifecycle gate to diverge from every existing handle. is_current_generation() then rejects the otherwise live wallet, generation-bound reservation cleanup becomes a no-op, and teardown can exclude through a different gate than payment operations. This synchronization identity must not be publicly assignable.

Suggested change
pub generation: Arc<WalletGeneration>,
pub(crate) generation: Arc<WalletGeneration>,

source: ['codex']

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied as suggested — generation is now pub(crate) (3dec774929).

Confirmed the reachability first: state_mut() and state_mut_blocking() are both pub and hand out a WalletStateWriteGuard that derefs mutably to PlatformWalletInfo, so with a pub field downstream safe code really could assign a fresh Arc here while PlatformWallet and CoreWallet keep the original — and the consequences are as described: is_current_generation() rejects the still-live wallet, generation-bound reservation cleanup no-ops, and teardown ends up excluding through a different gate than the payment operations it has to fence.

No downstream break: every construction site (manager/wallet_lifecycle.rs, manager/load.rs, wallet/apply.rs, test_support.rs, and the test helpers in contact_requests.rs / asset_lock/sync/recovery.rs) and the one reassignment (signed_payment_registry.rs, a test) are inside rs-platform-wallet. Nothing outside the crate touches the field — the public read path is the existing PlatformWallet::generation() accessor, which is unchanged.

Added a doc comment on the field recording why it is not pub, so it does not get widened back by someone who needs read access and reaches for the field instead of the accessor.

coreSignerHandle: Long,
accountType: AccountType = AccountType.BIP44,
accountIndex: Int = 0,
): SignedCoreTransaction = gate.op {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Release the registered token when cancellation discards the result

Native finalization registers the payment and transfers reservation ownership to a token before the blocking JNI call returns. gate.op uses withContext(Dispatchers.IO), whose prompt-cancellation handoff can discard the newly constructed SignedCoreTransaction when the caller is cancelled while JNI is running. The only remaining cleanup is then the GC Cleaner, so the token and funding reservation can remain held until an unpredictable GC cycle or the reservation TTL. Use the existing opWithCleanupOnCancellation helper to close a completed result that cancellation prevents from reaching the caller.

Suggested change
): SignedCoreTransaction = gate.op {
): SignedCoreTransaction = gate.opWithCleanupOnCancellation(
cleanup = { payment: SignedCoreTransaction -> payment.close() },
) {

source: ['codex']

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied as suggested (3dec774929) — buildSignedPayment now goes through gate.opWithCleanupOnCancellation(cleanup = { payment -> payment.close() }).

The window is real: coreWalletFinalizeSignedPayment mints the token and transfers reservation ownership natively before the blocking JNI call returns, and the SignedCoreTransaction only becomes the JVM owner after it is constructed on the way back out. op is withContext(Dispatchers.IO), whose prompt-cancellation handoff can discard that completed value, leaving the NativeCleaner as the only path to a release — i.e. an unpredictable GC cycle or the reservation TTL, with the coins held in the meantime.

opWithCleanupOnCancellation is the right existing primitive: it captures the completed value inside the op block and clears it only after the successful handoff, with no suspension point in between, so cleanup runs exactly on the discard path. Consuming the token later via broadcastSigned / releaseReservation makes the eventual close() a native no-op, so the extra ownership does not double-release.

Also fixed the KDoc on buildSignedPayment, which claimed the GC backstop was what covered a cancellation discarding the result — it now describes the deterministic close and leaves the Cleaner as the backstop it actually is.

Comment on lines 296 to 309
@@ -245,6 +301,11 @@ sealed class DashSdkError(
23 -> PlatformWallet.AssetLockNotTracked(message, cause) // ErrorAssetLockNotTracked
24 -> PlatformWallet.AssetLockAlreadyConsumed(message, cause) // ErrorAssetLockAlreadyConsumed
25 -> PlatformWallet.AssetLockFundingMismatch(message, cause) // ErrorAssetLockFundingMismatch
27 -> PlatformWallet.StaleReservationToken(message, cause) // ErrorStaleReservationToken
28 -> PlatformWallet.ReservationTokenConsumed(message, cause) // ErrorReservationTokenConsumed
// 29 is ErrorAssetLockInsufficientFunds (dashpay/platform#4184); this
// code is 30. See packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md.
30 -> PlatformWallet.ReservationWalletMismatch(message, cause) // ErrorReservationWalletMismatch
else -> PlatformWallet.Generic(code, message, cause)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Preserve definitive broadcast rejection as a typed Kotlin error

A definitive deferred-payment broadcast rejection is preserved as native code 26 through the C ABI and JNI, but this switch falls through to PlatformWallet.Generic. Callers can inspect Generic.nativeCode manually, but the public typed hierarchy otherwise distinguishes ambiguous broadcast outcomes and all deferred-token failures. The new broadcastSigned API should likewise expose definitive rejection as a dedicated subtype so callers can distinguish a consumed-and-released rejected payment, which must be rebuilt or corrected, from unrelated generic wallet failures. Add the subtype, map code 26, and pin its non-retry-in-place/rebuild semantics in DashSdkErrorTest.

source: ['codex']

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied (3dec774929) — added DashSdkError.PlatformWallet.TransactionBroadcastRejected and mapped native 26 to it, so it no longer falls through to Generic.

Confirmed the gap first: fromPlatformWalletNative had arms for 25 and then 27, with nothing for 26, so a definitive rejection arrived as Generic(26, …) even though the C ABI and JNI preserve it faithfully all the way up. That was the odd one out — 20 (TransactionBroadcastUnconfirmed) and all three deferred-token failures already had dedicated types.

Semantics as you framed them: isRetryable stays false. Core definitively rejected the transaction, and on the deferred path the reservation was released and the token consumed at the same time, so there is nothing left to retry against — the payment has to be rebuilt after addressing the rejection reason, which still travels in the message. The KDoc states that explicitly and contrasts it with TransactionBroadcastUnconfirmed (20), whose ambiguity is precisely why that one keeps its inputs reserved.

Pinned in DashSdkErrorTest with the type assertion carrying the message "code 26 must not fall through to Generic", plus the non-retryable and message-passthrough assertions.

One thing to note when reading the diff: this landed alongside the renumber for #4268, so the arms around it moved — the deferred-token trio is now 34/35/36 and 26 sits just above 25. Code 26 itself is merged ABI and did not move.

…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>
@bfoss765

bfoss765 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Blocker 1 — wallet_lifecycle.rs:655-687, same-ID registration racing the two-map removal: confirmed and fixed in 6be67488.

The finding is correct, and the mechanism is worth naming precisely: registration is deliberately ungated — register_wallet mints its own WalletGeneration (:186) and never takes a teardown gate, so a create never queues behind an unrelated wallet's removal. The consequence is that from the moment wm.remove_wallet (:672) frees the id, nothing orders a concurrent same-id register_wallet against this remover's own self.wallets acquisition (:683) — the two are separated only by a guard drop, with no lock and no await between them.

A remover descheduled there (OS preemption on a multi-threaded executor, or tokio's cooperative budget making RwLock::write() reschedule even on a free permit) resumes into a map that already names G2, removes it by key, returns it, and hands it to tear_down — which sweeps G2's registry tokens and V2 handles while holding only G1's gate. G2 then survives in wallet_manager but is gone from self.wallets: get_wallet returns None and a re-issued remove_wallet returns WalletNotFound, yet is_current_generation() reads the inner manager and still reports live, so G2's retained handles stay broadcastable. load_from_persistor is a second ungated publisher with the same wm-then-wallets shape.

I took the second option from the finding: the retry loop now carries the validated Arc<PlatformWallet> out, and the public-map entry is removed only while it still Arc::ptr_eq-matches that generation (:696-719, :762-780), with a warn! on the mismatch branch. The removed handle, the returned handle and the tear_down argument are now all the one generation this call validated. This reuses the existing generation-identity primitive rather than adding a parallel mechanism, adds no lock, and leaves lock order (gate -> manager) and the "gate not held across a signer await" property untouched.

I did not take the "single per-ID ordering" option by holding self.wallets.write() across the wallet_manager.write() acquisition. It would work, but wallet_manager.write() is SPV-contended and tokio's RwLock is write-preferring, so a remover queued on wm while holding wallets would stall BalanceUpdateHandler and the sync managers on the SPV event path.

I also did not add an identity check to the inner-manager removal. While this call holds G1's gate and G1 occupies the id, G1 can only leave wallet_manager through this same method or through a registration/load rollback for an insert that could not have happened while G1 was present — I checked all five wm.remove_wallet call sites and both insert_wallet sites. That branch would be unreachable and untestable. The reasoning is recorded in the method doc.

One thing left as-is and now documented: the shielded coordinator detach is still id-keyed and has no generation concept. A just-registered generation has not run bind_shielded and so holds no coordinator entry to detach — but that is an argument from timing, not from structure, and generation-scoping the coordinator is a separate change.

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.

On why a rendezvous rather than a lock-timing test: the window is bounded by two different locks, and the only lock a test could hold to park the remover inside it is self.wallets — the same lock the registration needs in order to publish. Parking the remover therefore also blocks the registration, and tokio's FIFO writer queue then puts the remover first. No sleep, barrier, or completion-order ticket can pin this ordering. With the rendezvous there is no concurrency in the test at all — one task, one fixed interleaving. That matters here: two earlier tests on this PR were written in a completion-order style and passed against provably broken code on some runs.

Against the previous code it fails deterministically (10/10) on all three load-bearing assertions: the returned generation is G2, the tear_down argument is G2, and the re-registered wallet is gone from the public map. Neither the static nor its call site exists in a non-test build.

cargo fmt --check clean; cargo test -p platform-wallet-ffi -p platform-wallet green — 514 + 212 unit, 9 + 26 + 6 integration, 0 failed. All 7 remove_wallet_lifecycle_tests pass, ErrorReservationWalletMismatch = 30 unchanged, HandleStorage::any stays removed with remove_matching retained.

Blocker 2 — Cargo.toml:55-62 fork pin: acknowledged, not addressed here. It clears when dashpay/rust-dashcore#916 merges and the entries plus Cargo.lock are repinned to the Dash-owned revision. #916 is approved, 35/36 green and mergeable, waiting only on a human CODEOWNER.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs (1)

154-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared generation-gate block into one helper.

Lines 154-173 and lines 276-295 are the same gate: acquire generation_payment_guard, read is_current_generation, abandon on a dead generation, return NotFound. Only the message text differs. Both blocks must stay identical for the teardown invariant to hold on both the V2-handle path and the deferred-token path. A single helper removes the risk that a future change lands on one path only.

♻️ Suggested helper
/// Acquire this generation's payment gate and verify the generation is still
/// current. Returns `Err(result)` after reconciling the build's reservation
/// when the wallet was removed or re-created during signing.
fn enter_live_generation<'a>(
    wallet: &'a platform_wallet::PlatformWallet,
    finalized: &platform_wallet::SignedCoreTransaction,
    not_found_message: &str,
) -> Result<tokio::sync::RwLockReadGuard<'a, ()>, PlatformWalletFFIResult> {
    let (lifecycle, live) = runtime().block_on(async {
        let gate = wallet.core().generation_payment_guard().await;
        let live = wallet.core().is_current_generation().await;
        (gate, live)
    });
    if live {
        return Ok(lifecycle);
    }
    drop(lifecycle);
    runtime().block_on(wallet.core().abandon_transaction(finalized));
    Err(PlatformWalletFFIResult::err(
        PlatformWalletFFIResultCode::NotFound,
        not_found_message.to_string(),
    ))
}

Then each call site becomes:

-    let (_lifecycle, wallet_is_live) = runtime().block_on(async {
-        let gate = wallet.core().generation_payment_guard().await;
-        let live = wallet.core().is_current_generation().await;
-        (gate, live)
-    });
-    if !wallet_is_live {
-        runtime().block_on(wallet.core().abandon_transaction(&finalized));
-        return PlatformWalletFFIResult::err(
-            PlatformWalletFFIResultCode::NotFound,
-            "wallet is no longer registered in the manager (removed or re-created while the \
-             transaction was being signed); no transaction handle was published and its \
-             reservation was reconciled"
-                .to_string(),
-        );
-    }
+    let _lifecycle = match enter_live_generation(
+        &wallet,
+        &finalized,
+        "wallet is no longer registered in the manager (removed or re-created while the \
+         transaction was being signed); no transaction handle was published and its \
+         reservation was reconciled",
+    ) {
+        Ok(guard) => guard,
+        Err(result) => return result,
+    };
🤖 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/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`
around lines 154 - 173, Extract the duplicated generation gate and
dead-generation reconciliation from the V2-handle and deferred-token paths into
a shared enter_live_generation helper. Have it acquire and return the payment
guard for live generations, otherwise release the guard, abandon the finalized
transaction, and return NotFound using the caller-provided message. Replace both
existing blocks with this helper while preserving each path’s distinct error
message and guard lifetime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 205-209: Record the reserved error-code assignments in a durable
registry by creating or updating ERROR_CODE_REGISTRY.md to map
ErrorAssetLockInsufficientFunds to 29 and ErrorReservationWalletMismatch to 30,
or remove the stale registry reference from the ErrorReservationWalletMismatch
documentation.

---

Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- Around line 154-173: Extract the duplicated generation gate and
dead-generation reconciliation from the V2-handle and deferred-token paths into
a shared enter_live_generation helper. Have it acquire and return the payment
guard for live generations, otherwise release the guard, abandon the finalized
transaction, and return NotFound using the caller-provided message. Replace both
existing blocks with this helper while preserving each path’s distinct error
message and guard lifetime.
🪄 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 Plus

Run ID: fcb68778-c02c-41fa-9170-df3e834cd62f

📥 Commits

Reviewing files that changed from the base of the PR and between 0b0d5c7 and 6be6748.

📒 Files selected for processing (25)
  • 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/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/SignedCoreTransactionTest.kt
  • packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.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/manager.rs
  • packages/rs-platform-wallet/src/manager/load.rs
  • packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs
  • packages/rs-platform-wallet/src/test_support.rs
  • packages/rs-platform-wallet/src/wallet/apply.rs
  • packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs
  • packages/rs-platform-wallet/src/wallet/core/generation.rs
  • packages/rs-platform-wallet/src/wallet/core/mod.rs
  • packages/rs-platform-wallet/src/wallet/core/transaction.rs
  • packages/rs-platform-wallet/src/wallet/core/wallet.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/contact_requests.rs
  • packages/rs-platform-wallet/src/wallet/platform_wallet.rs
  • packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs
  • packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
  • packages/rs-unified-sdk-jni/src/wallet_manager.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
💤 Files with no reviewable changes (1)
  • packages/rs-platform-wallet-ffi/src/handle.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/wallet/SignedCoreTransactionTest.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.kt

Comment thread packages/rs-platform-wallet-ffi/src/error.rs Outdated
…ence (dashpay#4185 review)

The doc comment on ErrorReservationWalletMismatch pointed at
packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md, which does not
exist in the tree (nor in dashpay#4184, which owns code 29). The inline note
that 29 is reserved by ErrorAssetLockInsufficientFunds (dashpay#4184) already
records the split, so remove the stale link rather than minting a new
registry file. Flagged by CodeRabbit.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bfoss765

bfoss765 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@thepastaclaw The 03:57 preliminary review is answered — requesting revalidation at the current head b5023dcbce.

  • Blocker 1 (same-ID registration racing the two-map removal) is fixed in 6be67488 — the remover now carries the validated Arc<PlatformWallet> out of the retry loop and removes the public-map entry only while it still Arc::ptr_eq-matches that generation, with a deterministic rendezvous regression test (removal_leaves_a_generation_registered_during_it_intact) that fails 10/10 against the previous code. Full mechanism and design rationale in the comment above (2026-08-02 15:08).
  • Blocker 2 (Cargo.toml fork pin) is externally gated: it clears when feat(key-wallet): owner-tagged reservations to close the broadcast-release TOCTOU (platform#4185) rust-dashcore#916 merges and the eight entries plus Cargo.lock are repinned to the Dash-owned revision. test(dapi-client): fix broken SimplifiedMasternodeListDAPIAddressProvider test #916 is CI-green (35 checks passing) and CodeRabbit-approved, waiting only on its required human CODEOWNER review — nothing on this PR can advance it.
  • Since then, b5023dcbce (doc-only) removed a dangling ERROR_CODE_REGISTRY.md reference flagged by CodeRabbit; no code change.

cargo fmt --check clean; cargo test -p platform-wallet-ffi -p platform-wallet green at the head — 514 + 212 unit tests, 0 failed. Please re-run the gate (including the deferred Sonnet pass) at b5023dcbce.

…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 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
…pay#4268 claimed

dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev
FFI ABI, colliding with the `ErrorStaleReservationToken = 27` this branch
carries alongside dashpay#4185. Renumber the deferred build/broadcast trio to the
contiguous block 34-36, matching dashpay#4185:

  ErrorStaleReservationToken      27 -> 34
  ErrorReservationTokenConsumed   28 -> 35
  ErrorReservationWalletMismatch  30 -> 36

34-36 sits above every code claimed by a merged commit or an open PR (27
dashpay#4268 merged, 29 dashpay#4184, 31 dashpay#4183/dashpay#4259, 32 dashpay#4247/dashpay#4256, 33 dashpay#4256), so it ends
the renumbering churn. 28 and 30 are vacated and return to the free pool.

This branch's own `ErrorTransactionBuild` (32) and `ErrorTransactionSigning`
(33) are unaffected; their numbering-rationale rustdoc is updated to name
dashpay#4268 as the owner of 27 and to record where the trio went.

Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc +
tests, and the Swift mirror (no compile-time cross-ABI check — verified by
grep).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev
ABI on 2026-08-02, taking the number dashpay#4185 had held. dashpay#4185 and dashpay#4256 moved the
deferred-token trio to the contiguous block 34-36 in response.

Registry changes:

* 27 enters the merged table, owned by dashpay#4268.
* The proposed table moves the trio to 34/35/36 and marks 28 and 30 free but
  deliberately not reissued. Next free integer is now 37.
* New "Collision history" section records all three numberings of the trio
  (26/27/28 -> 27/28/30 -> 34/35/36) and, more usefully, corrects this file's
  own reasoning: on 2026-08-01 it recorded dashpay#3954's `ErrorShutdownIncomplete =
  27` as a non-conforming claim that had to be withdrawn because dashpay#4185's claim
  was older. Seniority among open PRs does not decide an ABI number — merging
  does. dashpay#3954 was closed, its work landed as dashpay#4268, and 27 is now merged ABI.
  The trio therefore moved above every claimed number rather than into the
  next free gap, so nothing currently in flight can hit it again.
* dashpay#3968's 27 is re-characterised: it was a proposed-vs-proposed collision, and
  is now a contradiction of merged ABI. Its frontier is 37+.
* dashpay#4196 is now two moves behind at 26/27/28; the doc reference it owns has to
  chase 34, not 27.
* Records a mirror gap found while grepping for this move: dashpay#4256 declares
  `ErrorTransactionBuild` (32) and `ErrorTransactionSigning` (33) in Rust and
  maps both in Kotlin, but declares neither in Swift, so both reach Swift hosts
  as `.errorUnknown`. Rule 5's Swift clause; left for that PR's author.
* Provenance re-verified against v4.2-dev `5d68612a45`, including the check
  that 32 and 33 were already taken — which is why the trio went to 34-36 and
  not 32-34.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@bfoss765

bfoss765 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Renumbered the deferred-token trio: 27/28/30 → 34/35/36

#4268 merged into v4.2-dev and took 27 for ErrorShutdownIncomplete, which is the number this PR's ErrorStaleReservationToken was on. That number is merged ABI now, so this branch had to move.

I moved all three rather than just the colliding one, and moved them above every number currently claimed by anything:

owner
27 ErrorShutdownIncomplete — merged, #4268
29 ErrorAssetLockInsufficientFunds#4184
31 ErrorSigningKeyUnavailable#4183/#4259
32 ErrorTransactionBuild#4247/#4256
33 ErrorTransactionSigning#4256

So 32/33/34 was not available; the trio is now the contiguous block 34–36. 28 and 30 are vacated and left free rather than back-filled, so a reviewer who saw the old numbering can't find a familiar number attached to an unfamiliar meaning. #4256 got the identical move; registry updated in #4261.

Applied at 32 sites (every line that carried one of the old numbers) — Rust enum + rustdoc cross-refs, signed_payment.rs, the JNI rustdoc, Kotlin mapping/KDoc/tests, and the Swift mirror. The Swift and Kotlin mirrors have no compile-time cross-ABI check, so those were verified by grep rather than by the compiler.

Also addressed the three open suggestions

platform_wallet.rs:52 — generation Arc publicly assignable. Valid, applied. PlatformWalletInfo.generation is now pub(crate). It was reachable mutably from outside the crate via state_mut() / state_mut_blocking(), so downstream safe code could swap the Arc while PlatformWallet and CoreWallet kept the original — splitting the identity Arc::ptr_eq compares. is_current_generation() would then reject a live wallet, generation-bound reservation cleanup would no-op, and teardown would exclude through a different gate than the payments it must fence. Every construction and mutation site is already inside the crate, so this is not a downstream break.

ManagedPlatformWallet.kt:316 — cancellation discards the token. Valid, applied as suggested. buildSignedPayment now runs under opWithCleanupOnCancellation with cleanup = { it.close() }. Native finalization mints the token before the blocking JNI call returns, so withContext's prompt-cancellation handoff could drop the completed SignedCoreTransaction and leave the reservation to the GC Cleaner or the TTL. Updated the KDoc, which previously described the GC backstop as the only cleanup on that path.

DashSdkError.kt — code 26 falls through to Generic. Valid, applied. Added PlatformWallet.TransactionBroadcastRejected and mapped 26 to it, so a definitively-rejected, consumed-and-released payment is distinguishable from an unrelated generic wallet failure. isRetryable stays false — the reservation and token are already gone, so it is not retryable in place; it has to be rebuilt. Pinned in DashSdkErrorTest, including an explicit "must not fall through to Generic" assertion.

Verification

  • cargo fmt --check clean.
  • cargo test -p platform-wallet-ffi -p platform-wallet: 767 passed, 0 failed (514 + 212 + 26 + 9 + 6).
  • ./gradlew :sdk:test: 190 tests, 0 failures, 0 errors on both debug and release variants.
  • Swift is unverified by compilation — no Swift toolchain run here. Its four numeric sites were checked by grep against the Rust enum.

One note for whoever picks it up: this branch is not rebased onto current v4.2-dev, so it does not yet contain ErrorShutdownIncomplete itself. The renumber is what makes that merge clean — upstream's 27 now lands in an empty slot instead of colliding.

@bfoss765

bfoss765 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@thepastaclaw re-requesting at the ACTUAL current head 3dec7749 — the earlier request cited b5023dcb, which was superseded 20 minutes later by the error-code renumber (27/28/30 → 34/35/36, forced by #4268 merging ErrorShutdownIncomplete = 27 into v4.2-dev ABI; see #4261 for the registry record). Both prior blockers are addressed at this head: the same-ID registration race is fixed in 6be67488 (regression test fails 10/10 against the old code), and the Cargo.toml fork pin remains externally gated on dashpay/rust-dashcore#916 (green, CodeRabbit-approved, awaiting human merge). Please revalidate including the deferred Sonnet pass.

bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
…-> 37 and mirror it (dashpay#4204)

32 is allocated to `ErrorTransactionBuild` (dashpay#4247, also
carried by dashpay#4256) in ERROR_CODE_REGISTRY.md (dashpay#4261). This variant took 32
without a registry row, so the two collide as a hard `E0081: discriminant
value 32 assigned more than once` the moment both land — reproduced on a
real integration merge, not hypothetical. 27-36 are all claimed (27
ErrorShutdownIncomplete via the merged dashpay#4268; 29 dashpay#4184; 31 dashpay#4183; 32/33
dashpay#4247/dashpay#4256; 34-36 the dashpay#4185 trio) and 28/30 are vacated-but-RESERVED, so
37 is the allocation frontier.

The code was also unmirrored on BOTH hosts, which is the more dangerous
half: Swift is exhaustive, so it surfaced as .errorUnknown and lost its
identity; Kotlin fell through to Generic(32), and in any tree carrying
dashpay#4185's ErrorReservationWalletMismatch = 32 it actively MISCLASSIFIED
"shielded invite already claimed" as "reservation wallet mismatch". That
matters on the claim-recovery path specifically — the error is raised from
four sites in shielded/operations.rs, three inside the recovery function.

Adds the typed Kotlin PlatformWallet.ShieldedInviteAlreadyClaimed (terminal,
inherited isRetryable = false), the Swift enum case + init(ffi:) arm, a
DashSdkErrorTest assertion pinning 37, and refreshes the stale Swift
reservation comment the registry asked the next toucher to drop.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
…e 32 collision

dashpay#4204 allocated 32 without a row here, colliding with dashpay#4247's
ErrorTransactionBuild. Caught for real during the v41int13 integration as an
E0081, not on paper — the first collision this file has actually stopped.
dashpay#4204 moves to 37; frontier advances to 38. Also records that the code was
unmirrored on both hosts, which had Kotlin misclassifying it as
ReservationWalletMismatch wherever dashpay#4185's 32 was present.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
…keystore-qa4

Brings the ErrorShieldedInviteAlreadyClaimed 32 -> 37 renumber + host mirrors,
clearing the E0081 this integration hit against qa3's ErrorReservationWalletMismatch = 32.

Conflicts (4) all unions — kept BOTH sides, since this subset does not carry
dashpay#4185/dashpay#4256: the deferred-token trio stays at qa3's 27/28/32 and 37 is added
alongside. Swift's inherited comment was rewritten to describe THIS tree's
numbering rather than the 34-36 layout those branches introduce.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
…, the code this integration actually uses

DashSdkErrorTest asserted offset+29 -> ReservationWalletMismatch, but 29 became
ErrorAssetLockInsufficientFunds when dashpay#4184 took it and the mismatch code moved
to 32 — so the assertion had silently stopped testing its own mapping on the
qa3 integration line. dashpay#4185/dashpay#4256 fix this by moving the trio to 34-36 and
updating the assertion; both are out of scope for v41int13, and no feature
branch carries ReservationWalletMismatch = 32, so this is corrected here.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 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 3, 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 3, 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 3, 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>
…plit-build-broadcast

Brings the branch onto current v4.2-dev so it is mergeable again.

dashpay#4268 ("registry-owned coordinator lifecycle with Rust-owned FFI callback
contexts") landed upstream on 2026-08-02 and touches the neighbouring
lifecycle surface: `platform-wallet-ffi`'s manager / event_handler /
persistence, the manager's sync loops, and the Kotlin/Swift manager
mirrors. It does NOT touch this PR's `wallet/core/generation.rs`,
`manager/wallet_lifecycle.rs` or `wallet/signed_payment_registry.rs`, and
git merged every one of those cleanly.

The two designs are complementary, not competing: dashpay#4268 owns the
*coordinator* (async task registry + callback-context ownership across the
FFI boundary), this PR owns the *wallet generation* (the per-generation
balance Arc plus its payment/teardown gate). The only overlap is the error
space, resolved below.

Sole conflict: `PlatformWalletResult.swift`, five hunks, all additive on
both sides — dashpay#4268 adds `errorShutdownIncomplete = 27`, this PR adds the
deferred-token trio at 34-36. Both kept. The trio already sat at 34-36
precisely because dashpay#4268 had claimed 27, so no renumbering was needed; the
in-file registry comment already named dashpay#4268 as 27's owner.
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
Keeps this branch on top of dashpay#4185, which has just taken current v4.2-dev
(post-dashpay#4268). Sole conflict: `platform-wallet-ffi`'s result enum, where
dashpay#4268 adds `ErrorShutdownIncomplete = 27` and this branch adds
`ErrorTransactionBuild = 32`. Both kept, declared in numeric order; no
renumbering was needed because 32 was chosen from the registry frontier
with 27 already reserved to dashpay#4268.
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
Keeps this branch on top of dashpay#4247, which now carries current v4.2-dev
(post-dashpay#4268). Merged cleanly — dashpay#4268's coordinator-lifecycle rework and
this branch's funding-path finalize touch disjoint code, and the error
space was already reconciled to the registry (dashpay#4261): 27
ErrorShutdownIncomplete (dashpay#4268), 32 ErrorTransactionBuild (dashpay#4247), 33
ErrorTransactionSigning (this PR), 34-36 the dashpay#4185 deferred-token trio.
@bfoss765

bfoss765 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Restacked: this PR is now the base of a 3-PR stack

Stack order (bottom → top): #4185#4247#4256

These three PRs previously branched independently from the same point and each rewrote the deferred-payment lifecycle, so no two could be merged without hundreds of conflicted lines in signed_payment_registry.rs / core/send.rs. They are now a clean linear stack: a change to any one rebases mechanically instead of re-triggering that reconciliation.

This PR is unchanged in content. Its commits are byte-identical to before; the only new commit is a merge of current v4.2-dev.

What changed

Relationship to #4268 (merged upstream 2026-08-02)

#4268 owns the coordinator — the async task registry and Rust-owned FFI callback contexts (platform-wallet-ffi's manager.rs / event_handler.rs / persistence.rs, the manager sync loops). This PR owns the wallet generation — the per-generation balance Arc plus its payment/teardown gate (wallet/core/generation.rs, manager/wallet_lifecycle.rs, wallet/signed_payment_registry.rs). The two file sets are disjoint and every one of this PR's files merged cleanly. The only overlap was the error-code space, resolved above.

Error-code registry (#4261) after the stack

code name owner
27 ErrorShutdownIncomplete #4268 (merged)
28, 30 vacated but RESERVED
29 ErrorAssetLockInsufficientFunds #4184
31 ErrorSigningKeyUnavailable #4183/#4259
32 ErrorTransactionBuild #4247
33 ErrorTransactionSigning #4256
34/35/36 deferred-token trio #4185
37 ErrorShieldedInviteAlreadyClaimed #4204

Identical in Rust, Kotlin and Swift; verified for duplicates in all three.

Still open on this PR

The blocking finding 205eb14c72e3 (workspace deps pinned to bfoss765/rust-dashcore @ e99959ce) is not fixed and cannot be fixed here: the owner-tagged reservation API this PR depends on exists only on that fork, and its upstream PR dashpay/rust-dashcore#916 is still OPEN / BLOCKED / REVIEW_REQUIRED. Repointing the eight workspace entries at the Dash-owned repo today would fail to build. This PR cannot merge until #916 lands, after which the pins and Cargo.lock must move to the corresponding Dash-owned revision.

bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 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>
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 2026
 review blocker 2)

Reviewer (thepastaclaw) blocker 2 — build.rs:423-424:
build_asset_lock_tx_from_selected_account passed the BIP44 change
account (&bip44_acc) as set_funding's `acc`, but set_funding calls
funds_acc.next_change_address(Some(&acc.account_xpub)) on the SELECTED
funds account before the set_change_address override. For an explicitly
selected Standard BIP32 account with no pre-generated unused internal
address, that derived [1, index] from the wrong (BIP44) xpub and recorded
it under the BIP32 account's own path, poisoning that pool so a later
normal BIP32 send could use a change entry whose signer key does not match
the address. Now resolve the wallet-level Account whose account-level
derivation path equals funding_path and pass ITS xpub to set_funding,
while keeping the separate BIP44 set_change_address override. Default
BIP44 funding resolves to bip44_acc (unchanged); non-Standard
(CoinJoin/DashPay) accounts fail change derivation regardless, so the
xpub is immaterial and the bip44_acc fallback preserves prior behavior.

Also (thepastaclaw nitpick, test_support.rs): move the DashPay fixture
rustdoc so it attaches to split_funded_wallet_manager_dashpay rather than
foreign_contact_account_xpub.

Blocker 1 (build.rs:427-473, owner-guarded reservation rollback on the
pre-broadcast abandonment path) is NOT addressed here: the pinned
key-wallet rev 70d4bf8 exposes no owner-guarded release primitive
(ReservationSet has only reserve/reserved/release keyed by outpoint;
release_reservation is unconditional) and no reservation token, so the
required release_if_owner(token) mechanism (rust-dashcore#916 / dashpay#4185)
cannot be implemented against this pin without an upstream dependency
change — the very atomic-reservation fix this PR is held for. Deferred
pending that adoption rather than substituting an unconditional release
on the money path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765 added a commit to bfoss765/platform that referenced this pull request Aug 3, 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants