fix(platform-wallet): age-guard the V2 finalized-transaction handle broadcast (stacked on #4185)#4196
Open
bfoss765 wants to merge 24 commits into
Open
fix(platform-wallet): age-guard the V2 finalized-transaction handle broadcast (stacked on #4185)#4196bfoss765 wants to merge 24 commits into
bfoss765 wants to merge 24 commits into
Conversation
…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>
…dk-and-example-app Rebasing the split build/broadcast work onto the current base surfaced three semantic collisions the textual merge could not catch: - error code 22 was reassigned on base (ErrorCoreInsufficientFunds and the asset-lock family 22-25); moved ErrorStaleReservationToken to the next free code 26 in platform-wallet-ffi and DashSdkError's native-code mapping. - base added its own CoreWallet::release_transaction_reservation (taking AccountTypePreference, superset incl. CoinJoin) for the finalized-transaction abandon path, colliding with this PR's identically-named StandardAccountType method. Renamed this PR's deferred-payment release to release_payment_reservation (sole caller: SignedPaymentRegistry::release). - base removed the per-wallet coreSendMutex and now serializes/gates core sends through the TeardownGate (gate.op), moving send concurrency safety into the Rust reservation layer. buildSignedPayment now opens with gate.op like its sibling sendToAddresses instead of the removed mutex, which also satisfies the GateCoverageLintTest handle-borrowing fence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n mapping The rebase onto feat/kotlin-sdk-and-example-app reassigned native code 22 to ErrorCoreInsufficientFunds and moved ErrorStaleReservationToken to code 26 (on both the Rust enum and DashSdkError's mapping), but DashSdkErrorTest still constructed code 22 and asserted StaleReservationToken. That deterministically resolved to CoreInsufficientFunds, so platformWalletCodesMapToPlatformWalletSubtree failed and :sdk:testDebugUnitTest — the "Kotlin SDK build + tests (x86_64 emulator)" CI job — went red without actually verifying the code-26 mapping. Point the assertion at code 26 so it exercises the real production mapping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s own height clock The SignedPaymentRegistry age guard stamped registered_height with CoreWallet::synced_height() and compared it against a later synced_height(), while the funding reservation it is meant to stay under is stamped with last_processed_height() (the height finalize_transaction / build_signed pass to set_current_height, and the clock key-wallet's ReservationSet TTL sweeps against). synced_height can regress during a rescan while last_processed_height is monotonic, so measuring the reservation's age against synced_height could let a token outlive its reservation and act on an outpoint key-wallet had already swept and re-selected for an unrelated build. Read last_processed_height() for both the registration stamp and the current comparison so the guard measures the same clock the reservation is stamped with, trips strictly before the underlying TTL, and never regresses. Add CoreWallet::last_processed_height(); drop the now-unused synced_height(). The registry's expiry tests now stamp and advance last_processed_height to match production, and outstanding() is exposed under test-utils for downstream FFI tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…llet alias is destroyed platform_wallet_destroy unconditionally called remove_entries_for_wallet, which matches every registry entry sharing the destroyed handle's WalletManager pointer + wallet_id. But platform_wallet_manager_get_wallet hands out an independent handle per alias of the same logical wallet (the loadPersistedWallets path can publish a new wrapper while callers still hold an older one). Destroying one alias therefore consumed a sibling alias's still-live deferred-payment token: the sibling's later broadcast failed as stale while the sweep left the UTXO reserved until its TTL. Gate the sweep on final-alias liveness: after removing this handle, scan the remaining PlatformWallet handles for one that shares the same (WalletManager pointer + wallet_id) — exactly the key remove_entries_for_wallet matches. While a sibling is live the destructor only drops this handle; the sweep runs (releasing the registry's WalletManager pin) only once the last alias goes. Adds HandleStorage::any for the scan and a test_support helper that builds real PlatformWallet aliases; a new FFI test proves a sibling alias's token survives one alias's destruction and is swept when the final alias is destroyed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d-register path buildSignedPayment funded, signed, and registered a deferred payment as three separate native round-trips (setFunding + buildSigned + registerSignedPayment). Once the base branch removed the per-wallet coreSendMutex in favour of the TeardownGate — which only counts active ops for safe teardown and does not serialize sends — that split lost its atomic select-and-reserve boundary: two concurrent deferred builds, or a deferred build racing an immediate send, could select the same UTXO before either reserved it and return two signed transactions spending the same input. Restore atomicity in the Rust reservation layer, the correct home now that the Kotlin mutex is gone: add core_wallet_signed_payment_finalize, which runs the same finalize_transaction the immediate V2 path uses — selection and ReservationSet insertion commit as one unit under the wallet-manager lock, signing only after the lock drops — and then registers the built, reserved tx in the same call. buildSignedPayment now issues that single native operation (CoreTransactionBuilder.finalizeSignedPayment + coreWalletFinalizeSignedPayment), so the select+reserve window can no longer interleave. The existing concurrent_same_account_finalizers_cannot_reserve_the_same_input test already covers the atomic boundary the deferred path now shares. The deprecated split wrappers remain but are no longer on the deferred path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
After finalize routing landed, the four-layer deferred-register chain core_wallet_signed_payment_register (FFI) → coreWalletRegisterSignedPayment (JNI) → WalletManagerNative.coreWalletRegisterSignedPayment (Kotlin) → ManagedCoreWallet.registerSignedPayment had zero callers. It is the unsafe variant whose age guard baselines registered_height at registration time (after external signing) rather than at the reservation's own height, so removing it also removes that mis-baselined path. The atomic core_wallet_signed_payment_finalize path is the only remaining register site. Delete all four layers; repoint the surviving broadcast/finalize doc comments at the finalize entry point; drop the now-unused FFICoreTransaction::fee accessor (keep the ABI field, silence the lint). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eservation height finalize_transaction captures last_processed_height inside the funding critical section and stamps the selected inputs' reservation with it, then signs after dropping the wallet-manager lock. The registry, however, sampled a FRESH last_processed_height in register() — run AFTER the (possibly slow, external) signer returned. A slow signer could let the wallet advance so the token's baseline was higher than the reservation's true stamp height, making the age guard measure from the wrong side of signing: the token looked young while its reservation had already aged toward key-wallet's TTL sweep, risking a release/broadcast against an outpoint key-wallet had swept and re-selected. Carry the stamp height on SignedCoreTransaction (reservation_height, captured in the funding section before signing) and have register() take the height as an explicit parameter instead of sampling. The atomic finalize FFI passes finalized.reservation_height(); the age guard now baselines on the same clock the reservation was stamped with. Adds a regression test that registers after the wallet advanced (modelling a slow signer) and proves the guard trips MAX_AGE past the reservation height, not past a post-signing sample. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion identity The V2 finalized-transaction handle validated only wallet_id, while the registry-token path validated the shared WalletManager Arc plus wallet_id. Neither can tell one wallet generation from another: after a wallet is removed and re-created under the same id, both the manager Arc and wallet_id are equal, so an old V2 handle could act through the old generation while the new generation selects the same inputs. Add CoreWallet::is_same_generation — the single generation identity both paths now share. Aliases of one generation share the per-generation Arc<WalletBalance> (created fresh in the wallet-lifecycle create/load paths); a re-created wallet gets a new one, so Arc::ptr_eq on it distinguishes generations that wallet_id + the manager Arc cannot. Holding either handle pins the balance Arc, so its address can't be reused for a different generation — the same soundness argument the registry already uses for the manager Arc. Apply it to both V2 broadcast and abandon (replacing the wallet_id-only check). The registry broadcast path adopts the same identity in the follow-up validate-under-lock change. Adds a unit test proving alias-vs-recreation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…only a match SignedPaymentRegistry::broadcast removed the entry first and validated the wallet binding second, so a mismatched caller (wrong wallet, or a re-created generation) destroyed the ORIGINAL wallet's token and left its reservation stranded until the TTL backstop — a wrong-wallet broadcast could grief the rightful owner's in-flight payment. Peek under the registry lock, reject a non-matching caller with WalletMismatch WITHOUT removing the entry, and only remove (consume) an entry whose generation matches. The check-then-remove is one lock hold, so it stays atomic against a concurrent broadcast — the double-broadcast guard is unchanged (the second consumer finds nothing → StaleToken). The binding check now uses the shared CoreWallet::is_same_generation identity, so the registry-token and V2 handle paths agree on when a caller owns a token. Updates the two existing mismatch tests (which asserted the old drop-on-mismatch behaviour) and adds a regression proving a wrong-wallet broadcast preserves the owner's token and the owner can still broadcast it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…; drop them at generation teardown platform_wallet_destroy called remove_entries_for_wallet, which only DROPPED the registry entries. But destroying the last wrapper alias does not remove the logical wallet from its manager — the accounts' ReservationSets stay live and the same wallet can be handed out again — so the dropped tokens' inputs stayed reserved until key-wallet's TTL. Tokens were consumed without releasing live reservations. Split the two teardown moments under one generation identity: - Final-alias destroy (wallet still live): release_entries_for_wallet RELEASES each of the generation's reservations against the still-live wallet (honouring the age guard), so a wallet handed out again can respend the inputs. The final-alias check and the match are both by CoreWallet::is_same_generation. - Actual generation teardown (platform_wallet_manager_remove_wallet): the wallet and its ReservationSets are gone, so remove_entries_for_wallet DROPS the generation's registry tokens (nothing to reconcile) and remove_matching drops its finalized-tx V2 handles. This makes any stale handle to the removed generation inert, which is what makes the destroy-time release provably race-free: a torn-down generation has already had its tokens swept here, so destroy/release can never release-by-outpoint against a re-created generation's inputs. platform_wallet_destroy now block_on's the release (as it already runs off the tokio runtime on the JNI / NativeCleaner threads). Adds HandleStorage::remove_matching, registry release_entries_for_wallet, a registry regression proving destroy-time release frees the reservation while teardown drop does not, and reworks the FFI destroy test to invoke destroy off-runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hree siblings Native code 26 (ErrorStaleReservationToken) mapped all three SignedPaymentError variants — StaleToken (unknown/already-broadcast/released), WalletMismatch (different wallet generation), and StaleReservationToken (aged out) — so a host could not tell "you already broadcast this", "wrong wallet", and "the reservation aged out; rebuild" apart, even though the remedy and messaging differ. Split at the FFI (additive sibling codes, no renumbering): - 26 ErrorStaleReservationToken -> StaleReservationToken (aged out) - 27 ErrorReservationTokenConsumed -> StaleToken (unknown/already broadcast/released) - 28 ErrorReservationWalletMismatch -> WalletMismatch (different generation) core_wallet_signed_payment_broadcast now maps each variant to its own code. All three remain non-retryable-in-place and none touch the network. Host impact (Kotlin SDK only — the Swift host does not map these codes): adds DashSdkError.PlatformWallet.ReservationTokenConsumed / ReservationWalletMismatch, maps 27/28, narrows the code-26 doc, updates the JNI/Kotlin broadcast KDocs, and extends DashSdkErrorTest to assert all three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-register shape The KDoc still described the pre-finalize build (`new → addOutput* → setFunding → buildSigned`) and credited buildSigned with reserving the inputs. The deferred path now issues a single atomic finalizeSignedPayment (select + reserve + sign + register under the wallet-manager lock). Update the described step sequence and the atomicity claim to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ble with a Cleaner backstop buildSignedPayment returned a plain SignedCoreTransaction through a cancellable coroutine. The blocking JNI registration mints the reservation token before the Kotlin object exists, so if cancellation was observed after that native call returned — or the caller simply dropped the value — the token (and its funding reservation) was orphaned until key-wallet's TTL, with no release path. Make SignedCoreTransaction an AutoCloseable that registers a NativeCleaner backstop at construction: close(), or GC if the caller never calls it, releases the token exactly once. Native release is idempotent and tokens are process-unique, so releasing a token already consumed by broadcastSigned / releaseReservation (or closing twice) is a harmless no-op. This closes the cancellation window — the object is Cleaner-backed the instant it exists (no suspension point between the native return and construction), so a discarded object always releases its token. Adds a pure-JVM test pinning the ownership contract (owning AutoCloseable) and the Cleaner run-once guarantee it relies on, and documents the ownership on buildSignedPayment. :sdk:testDebugUnitTest passes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…etion Normalize two pre-existing long lines in coreWalletFinalizeSignedPayment that `cargo fmt --check` flags, so the JNI crate is formatting-clean after the register-chain removal touched this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…CoreTransaction Review round 2: the bare-Long token API couples the reservation's lifetime to the SignedCoreTransaction's GC-reachability — extracting the token and dropping the object lets the Cleaner backstop release the reservation out from under a pending broadcast. The object overloads keep the payment reachable across the native call (reachabilityFence) and disarm the backstop once the token is consumed; the bare-token docs now warn about the reachability requirement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
July 21, 2026 20:57
Contributor
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
🕓 Ready for review — 2 ahead in queue (commit ea4f783) |
Collaborator
|
Reviewed (two independent passes, both reaching the same conclusion). The broadcast guard itself is well built — the reservation-height baseline is sound by construction (sampled inside the funding critical section), the constant/predicate are genuinely shared with the registry rather than copied, and no handle leaks. But this closes only half the hazard it was opened for:
Nit: |
…ed deferred payments The deferred-payment registry stored only an `Option<StandardAccountType>`, so a CoinJoin funding — which has no `StandardAccountType` — reconciled nothing on rejection/abandon/free and kept its inputs reserved until key-wallet's 24-block TTL, even though `finalize` reserves the selected inputs for every account variant. Carry the full `AccountTypePreference` (BIP44/BIP32/CoinJoin) as the entry's releasable account handle. The registry now broadcasts through the new `broadcast_payment_releasing_reservation` and releases through `release_transaction_reservation` (both `AccountTypePreference`-typed and CoinJoin-capable), so a rejected or abandoned CoinJoin deferred payment frees its reservation immediately. The FFI finalize passes `account_type.into()` instead of the `StandardAccountType` subset; the now-unused `release_payment_reservation` (registry-only) is removed. Test: `coinjoin_funded_release_frees_the_reservation_immediately` funds CoinJoin account 0, finalizes a sweep, registers the token, and proves release makes the input immediately spendable again. Adds a `#[cfg(test)]` `funded_coinjoin_wallet_manager` fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… wallet generation The deferred registry validated a token's generation at the registry lock, then released its reservation later, off that lock. `ReservationSet::release` removes an outpoint unconditionally and is reached via `wallet_id` — an identity a same-id remove-then-recreate preserves — so a wallet re-created in that window could have the NEW generation's reservation freed by the old token's cleanup. Bind the cleanup to the token's own generation: `release_transaction_reservation` now re-validates the generation and mutates the `ReservationSet` under a single manager read-lock hold, acting only when the wallet still registered under the id carries the same per-generation balance `Arc` the handle captured. A recreation needs the manager write lock, so it cannot interleave between the check and the release — validate-and-mutate is atomic. This protects both the registry (release/abandon and broadcast-on-rejection) and the V2 finalized-transaction handle path, which share this primitive. Adds `CoreWallet::generation()`. Test: `recreation_between_validation_and_cleanup_cannot_release_new_generation` recreates the wallet under the same id between registration and release and asserts the input stays reserved (the reservation the new generation owns is untouched). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`PlatformWalletResultCode` jumped from 25 straight to 98, so the three deferred build->broadcast/release codes this PR owns (26 StaleReservationToken, 27 ReservationTokenConsumed, 28 ReservationWalletMismatch) fell through to `.errorUnknown` on iOS, erasing their distinct retry semantics. Add the three raw codes to `PlatformWalletResultCode`, matching cases to `PlatformWalletError`, and map them in both `init(ffi:)` and `init(result:)`. The `init(result:)` switch (no default) stays exhaustive — the same non-exhaustive-switch class shumkov flagged on dashpay#4184. Messages pass the Rust `Display` string straight through, matching the Kotlin SDK's mapping verbatim. Verified with `swiftc -parse` (the DashSDKFFI xcframework — cbindgen header + cdylib — is built separately by build_ios.sh and is not present in this checkout, so a full `swift build` type-check isn't possible here). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…error codes `core_wallet_signed_payment_broadcast` still documented the pre-split semantics: a repeated broadcast and a re-created wallet both as `ErrorStaleReservationToken` (26). Since the three-way split, a repeated/concurrent broadcast yields `ErrorReservationTokenConsumed` (27) and a re-created wallet generation yields `ErrorReservationWalletMismatch` (28); 26 is reserved for the aged-out case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…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>
…lin docs note shared code 26 Review round 2: the exact-boundary test now loops BIP44 and BIP32 (the commit previously claimed both but tested one), and the Kotlin docs for StaleReservationToken and ManagedCoreWallet.broadcastTransaction now say the V2 handle surface shares native code 26 with the deferred-token path, distinguishable by message. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
force-pushed
the
followup/v4.1/v2-handle-age-guard
branch
from
July 22, 2026 14:27
0d6cdb1 to
ea4f783
Compare
Contributor
Author
|
Fixed: Added aged-skip / below-bound-release tests for the core path and the FFI GC-backstop / failure paths, and corrected the misleading "abandon works at any age" KDoc (after a stale-refused broadcast the handle is already consumed, so abandon is invalid-handle). Rebased onto the updated #4185; the age-guard and #4185's generation-validated release primitive compose (both early-return skips). |
Collaborator
|
Round-3 verification (two independent passes, reconciled): verified complete within this PR — the age guard now sits at the single choke point ( Remaining gate is inherited from the stacked base: the freshness/release atomicity finding raised on #4185 (make height-read + generation-validate + release one guarded mutation for abandon/free, and re-check freshness after the broadcast await before a Rejected-release) must land there — this PR's paths route through the same function and will inherit that fix. Two smaller items: the Rust unit jobs were path-filter-SKIPPED at this head despite rs-platform-wallet/-ffi changes, so the new aged-skip tests have not executed in CI — confirm the filter or force a run before merge; and |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #4185 (split build/broadcast) — merge after it. The branch includes #4185's commits until it lands.
Follow-up to #4185 (requested by @shumkov): the V2 finalized-transaction
handle surface (
core_wallet_tx_builder_finalize→broadcast_finalized_transaction) retained a stale-release hazard — a pinnedV2 handle had no age guard, so a long-held handle could broadcast against
funding inputs that key-wallet's
ReservationSetTTL sweep may already havereleased and re-selected for an unrelated build. This goes live the moment iOS
starts issuing deferred sends.
This mirrors the deferred registry-token age policy on the V2 handle path:
RESERVATION_MAX_AGE_BLOCKS(20; key-wallet TTL 24) andreservation_expired()are hoisted towallet::reservations, so theregistry and the V2 handle path measure a reservation's age against the same
number.
broadcast_finalized_transactionrefuses — beforetouching the broadcaster — once
current_height − reservation_height >=theshared bound, using the reservation's own stamp height already carried on
SignedCoreTransaction::reservation_height. The stale reservation is leftfor key-wallet's TTL to reclaim (never released by outpoint). The check runs
after the existing generation-identity check, matching the registry order.
PlatformWalletError::StaleReservationreuses the existing FFI
ErrorStaleReservationToken(26); no new code is allocated(27/28 untouched; 29/30 remain reserved for the asset-lock PR fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073) #4184's branch)
(asset-lock PR) touched. Reuse is documented on both sides.
safe.
Tests: fresh handle broadcasts; aged handle refuses with
StaleReservationyet abandons cleanly and frees its inputs; exact threshold boundary
(BIP44/BIP32); FFI mapping to the shared code.
Stacked on
port/v4.1/split-build-broadcast(#4185) — review/merge thatfirst.