Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -266,13 +266,23 @@ sealed class DashSdkError(
PlatformWallet(message, cause)

/**
* `ErrorStaleReservationToken` (native code 34). A deferred
* (BIP70/BIP270) [broadcastSigned][org.dashfoundation.dashsdk.wallet.ManagedPlatformWallet.broadcastSigned]
* token has outlived its funding reservation's lifetime: key-wallet's
* TTL may already have swept and re-selected the inputs, so acting on it
* could touch a newer, unrelated reservation. The call did NOT touch the
* network. NOT retryable in place — rebuild the payment with
* [buildSignedPayment][org.dashfoundation.dashsdk.wallet.ManagedPlatformWallet.buildSignedPayment].
* `ErrorStaleReservationToken` (native code 34). A payment's funding
* reservation has outlived its lifetime: key-wallet's TTL may already
* have swept and re-selected the inputs, so sending it could spend
* against a newer, unrelated reservation. The call did NOT touch the
* network, and it released the still-owned reservation on the way out
* (owner-guarded — a no-op if ownership had already transferred). NOT
* retryable in place — rebuild the payment, which can reselect the
* freed inputs immediately.
*
* The code is shared by BOTH deferred-payment surfaces (the messages
* distinguish them): a deferred (BIP70/BIP270)
* [broadcastSigned][org.dashfoundation.dashsdk.wallet.ManagedPlatformWallet.broadcastSigned]
* token, rebuilt with
* [buildSignedPayment][org.dashfoundation.dashsdk.wallet.ManagedPlatformWallet.buildSignedPayment];
* and a finalized handle whose
* [broadcastTransaction][org.dashfoundation.dashsdk.wallet.ManagedCoreWallet.broadcastTransaction]
* aged past the same reservation bound (abandon still works at any age).
*
* Sibling of the other two deferred-token failures this code used to
* conflate: [ReservationTokenConsumed] (unknown / already broadcast /
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,39 @@ class ManagedCoreWallet internal constructor(handle: Long) : AutoCloseable {
check(it != 0L) { "ManagedCoreWallet has been closed" }
}

/** Consume and broadcast a finalized transaction. */
fun broadcastTransaction(tx: FinalizedCoreTransaction): String =
/**
* Consume and broadcast a finalized transaction. A handle held past the
* reservation age bound throws the typed
* [StaleReservationToken][org.dashfoundation.dashsdk.errors.DashSdkError.PlatformWallet.StaleReservationToken]
* (native code 34, shared with the deferred-token surface) instead of
* broadcasting against inputs key-wallet's TTL may have re-selected.
*
* On that refusal the handle has **already been consumed** by this call and
* its funding reservation released owner-guarded (freed only while this
* build still owned it; a no-op once a TTL sweep or re-reservation
* transferred ownership), so a follow-up [abandonTransaction] is an
* invalid-handle error, not a recovery path — there is nothing left to
* release. Recover by rebuilding the transaction, which can reselect the
* freed inputs immediately.
Comment on lines +37 to +46

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.

💬 Nitpick: Document the local error after Kotlin consumes the handle

broadcastTransaction calls tx.takeForBroadcast(), which atomically clears the Kotlin handle before JNI runs. A subsequent abandonTransaction(tx) therefore does not produce a native invalid-handle error: takeForAbandon() delegates to takeForBroadcast(), whose check throws IllegalStateException("FinalizedCoreTransaction has already been consumed") locally. Document the actual exception so callers do not expect a native or typed SDK error from the repeated operation.

Suggested change
* On that refusal the handle has **already been consumed** by this call and
* its funding reservation released owner-guarded (freed only while this
* build still owned it; a no-op once a TTL sweep or re-reservation
* transferred ownership), so a follow-up [abandonTransaction] is an
* invalid-handle error, not a recovery path — there is nothing left to
* release. Recover by rebuilding the transaction, which can reselect the
* freed inputs immediately.
* transferred ownership), so a follow-up [abandonTransaction] fails locally
* with [IllegalStateException] because [FinalizedCoreTransaction] has
* already been consumed; it never re-enters native code and is not a
* recovery path. Recover by rebuilding the transaction, which can reselect
* the freed inputs immediately.

source: ['codex']

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 80c54b4 — broadcastTransaction KDoc now states the handle is consumed up front on every outcome and a follow-up abandonTransaction fails locally with IllegalStateException (never re-enters native code); matching note on abandonTransaction.

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.

Resolved in 80c54b4Document the local error after Kotlin consumes the handle no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.

*/
fun broadcastTransaction(tx: FinalizedCoreTransaction): String = mapNativeErrors {
WalletManagerNative.coreWalletBroadcastSignedTransaction(
handle,
tx.takeForBroadcast(),
)
}

/** Consume without sending and release the selected inputs immediately. */
/**
* Consume a finalized transaction without sending. With the build's owner
* token present (the normal funded-finalize case) the release is
* owner-guarded and safe at any age: it frees the selected inputs while
* this build still owns them — so a rebuild can reselect them immediately —
* and no-ops once key-wallet's TTL sweep or a re-reservation transferred
* ownership. Only a token-less handle honours the reservation age bound and
* skips its unguarded by-outpoint release past it (releasing by outpoint
* could free a newer build's reservation), leaving the aged reservation for
* the TTL to reclaim. The handle is torn down either way.
*/
fun abandonTransaction(tx: FinalizedCoreTransaction) {
WalletManagerNative.coreWalletAbandonSignedTransaction(
handle,
Expand Down
112 changes: 112 additions & 0 deletions packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,26 @@ mod tests {
runtime().block_on(core.abandon_transaction(&retry));
}

/// Prove the funding reservation was released owner-guarded: a fresh
/// finalize of the same size reselects the single fixture UTXO. An aged
/// abandon/free with the build's owner token present releases via
/// `release_reservation_if_owner` (safe at any age — no-op once ownership
/// transferred), so the input must be immediately reselectable.
fn assert_released_for_rebuild(core: &TestCore, signer: &WalletSigner, tag: u8) {
let rebuild = runtime().block_on(core.finalize_transaction(
TransactionBuilder::new().add_output(
&Address::dummy(Network::Testnet, usize::from(tag)),
1_000_000,
),
&[AccountTypePreference::BIP44],
0,
signer,
));
let rebuilt = rebuild
.expect("aged abandon/free must release the still-owned reservation for a rebuild");
runtime().block_on(core.abandon_transaction(&rebuilt));
}

#[test]
fn double_free_is_safe_and_releases_reservation() {
let (core, signer) =
Expand Down Expand Up @@ -327,6 +347,98 @@ mod tests {
CORE_WALLET_STORAGE.remove(other_handle);
}

/// The deinit/GC backstop (`core_wallet_signed_transaction_free`) is the
/// exact path shumkov flagged: a `FinalizedCoreTransaction` never broadcast
/// or abandoned, freed by the host GC long after finalize. The funded
/// finalize stamped an owner token, so the aged free still releases —
/// owner-guarded via `release_reservation_if_owner`, which is safe at any
/// age (it no-ops once key-wallet's TTL swept and an unrelated build
/// re-reserved the outpoint) — freeing the still-owned input for a rebuild.
/// The handle is torn down (the storage entry is removed) so a re-free is a
/// safe no-op.
#[test]
fn aged_free_releases_owner_guarded() {
let (core, signer) =
runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account));
let transaction_handle = insert(&core, finalize(&core, &signer, 48));

// Age the pinned handle past the guard bound (still below the TTL, so the
// reservation is provably still held — only the software guard trips).
runtime().block_on(platform_wallet::test_support::age_core_past_reservation_guard(&core));

core_wallet_signed_transaction_free(transaction_handle);

// The aged free released owner-guarded: the input is reselectable.
assert_released_for_rebuild(&core, &signer, 49);
// Handle is gone regardless — a re-free is a harmless no-op.
core_wallet_signed_transaction_free(transaction_handle);
}

/// The FFI broadcast/abandon *failure* paths (invalid or wrong-generation
/// wallet handle) route their cleanup through `abandon_transaction`, so they
/// inherit the same policy: an aged handle with the build's owner token
/// still releases owner-guarded (safe at any age), so the failure-path
/// cleanup frees the still-owned input instead of stranding it.
#[test]
fn aged_failure_path_abandon_releases_owner_guarded() {
let (origin, signer) =
runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account));
let transaction_handle = insert(&origin, finalize(&origin, &signer, 50));

runtime().block_on(platform_wallet::test_support::age_core_past_reservation_guard(&origin));

// Invalid wallet handle → routes through abandon_transaction, then returns
// ErrorInvalidHandle. The embedded aged reservation is released
// owner-guarded on the way out.
let invalid =
unsafe { core_wallet_abandon_signed_transaction(u64::MAX, transaction_handle) };
assert_eq!(
invalid.code,
PlatformWalletFFIResultCode::ErrorInvalidHandle
);
assert_released_for_rebuild(&origin, &signer, 51);
}

/// The terminal FFI stale-broadcast behavior: by the time the age guard
/// runs, `core_wallet_broadcast_signed_transaction` has already consumed
/// the opaque handle (and the host bindings cleared theirs before entering
/// the ABI), so no follow-up abandon is possible. The refusal must
/// therefore reconcile the reservation itself — owner-guarded, freeing the
/// still-owned input so the instructed immediate rebuild can reselect it —
/// and surface the shared `ErrorStaleReservationToken` (34) code with no
/// txid. A retry of the consumed handle is `NotFound`, not a resend.
#[test]
fn aged_broadcast_refuses_and_releases_for_rebuild() {
let (core, signer) =
runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account));
let core_handle = CORE_WALLET_STORAGE.insert(core.clone());
let transaction_handle = insert(&core, finalize(&core, &signer, 52));

runtime().block_on(platform_wallet::test_support::age_core_past_reservation_guard(&core));

let mut txid = ptr::null_mut();
let stale = unsafe {
core_wallet_broadcast_signed_transaction(core_handle, transaction_handle, &mut txid)
};
assert_eq!(
stale.code,
PlatformWalletFFIResultCode::ErrorStaleReservationToken
);
assert!(txid.is_null());

// The refusal released owner-guarded: the input is reselectable with no
// further cleanup call.
assert_released_for_rebuild(&core, &signer, 53);

// The handle was consumed by the refused broadcast — a retry cannot
// reconsume it.
let retry = unsafe {
core_wallet_broadcast_signed_transaction(core_handle, transaction_handle, &mut txid)
};
assert_eq!(retry.code, PlatformWalletFFIResultCode::NotFound);
CORE_WALLET_STORAGE.remove(core_handle);
}

#[test]
fn abandon_then_free_or_broadcast_cannot_reconsume_handle() {
let (core, signer) =
Expand Down
53 changes: 53 additions & 0 deletions packages/rs-platform-wallet-ffi/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,23 @@ pub enum PlatformWalletFFIResultCode {
/// [`Self::ErrorReservationWalletMismatch`] (36, minted against a different
/// wallet generation). All three are non-retryable-in-place and none touched
/// the network; they are distinct codes so a host can message each precisely.
///
/// Also maps `PlatformWalletError::StaleReservation` from the atomic
/// finalized-transaction handle path
/// (`core_wallet_broadcast_signed_transaction`): a pinned handle whose
/// funding reservation aged past the SAME `RESERVATION_MAX_AGE_BLOCKS` bound
/// carries the identical "may already have been swept — rebuild" meaning, so
/// the two surfaces intentionally share this one code. The handle carries
/// no numeric reservation token, hence a distinct (token-less) wallet-error
/// variant behind the same FFI code. The refusal reconciles the reservation
/// on the way out: a funded finalize always stamps an owner token, so the
/// release is owner-guarded (safe at any age — a no-op once ownership
/// transferred) and the still-owned inputs are freed for the instructed
/// rebuild. Abandon/free of a handle never surfaces this — abandon returns
/// no result code and likewise releases owner-guarded at any age; only a
/// token-less build skips its unguarded by-outpoint release past the bound
/// (leaving the aged outpoint to key-wallet's TTL, since releasing it
/// unguarded could free an unrelated newer build's reservation).
ErrorStaleReservationToken = 34,

/// Maps `SignedPaymentError::StaleToken`. The deferred reservation token is
Expand Down Expand Up @@ -579,6 +596,14 @@ impl From<PlatformWalletError> for PlatformWalletFFIResult {
PlatformWalletError::TransactionBroadcast(..) => {
PlatformWalletFFIResultCode::ErrorTransactionBroadcastRejected
}
// The finalized-transaction handle path's age guard. Shares the
// `ErrorStaleReservationToken` code with the deferred registry-token
// sibling (`SignedPaymentError::StaleReservationToken`): both mean
// "the funding reservation may already have been swept — rebuild",
// and neither touched the network. See the code's doc note.
PlatformWalletError::StaleReservation => {
PlatformWalletFFIResultCode::ErrorStaleReservationToken
}
// A definitively-failed address-nonce race (reaches the blanket impl
// via identity `top_up_from_addresses` → `?`/`.into()`). Exposing
// provided/expected nonce as structured out-fields is INTENTIONALLY
Expand Down Expand Up @@ -1195,6 +1220,34 @@ mod tests {
assert_eq!(msg, rendered, "Display payload must survive verbatim");
}

/// The finalized-transaction handle age guard
/// (`core_wallet_broadcast_signed_transaction` → `broadcast_finalized_transaction`)
/// surfaces `PlatformWalletError::StaleReservation` through the blanket
/// `From` impl, which must reuse the deferred registry-token path's
/// `ErrorStaleReservationToken` (34) code rather than flattening to
/// `ErrorUnknown` — the two surfaces share the "reservation may have been
/// swept; rebuild" meaning and this one code. The typed Display rendering
/// survives across the boundary as the message.
#[test]
fn stale_reservation_maps_to_shared_stale_reservation_code() {
let err = PlatformWalletError::StaleReservation;
let rendered = err.to_string();
let result: PlatformWalletFFIResult = err.into();
assert_eq!(
result.code,
PlatformWalletFFIResultCode::ErrorStaleReservationToken,
"StaleReservation must reuse the registry-token stale code (rendered: {rendered})"
);
assert!(!result.message.is_null());
let msg = unsafe { std::ffi::CStr::from_ptr(result.message) }
.to_string_lossy()
.into_owned();
assert_eq!(
msg, rendered,
"Display payload must survive the FFI boundary verbatim"
);
}

/// `AddressNonceMismatch` maps to the dedicated `ErrorAddressNonceMismatch`
/// FFI code through the blanket `From` impl (the path identity
/// `top_up_from_addresses` takes via `?`/`.into()`) rather than flattening
Expand Down
27 changes: 27 additions & 0 deletions packages/rs-platform-wallet/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,33 @@ pub enum PlatformWalletError {
)]
TransactionBroadcastUnconfirmed(String),

/// A finalized transaction handle
/// (`core_wallet_tx_builder_finalize` → `broadcast_finalized_transaction`)
/// was held long enough that its funding reservation may already have been
/// swept and re-selected by key-wallet's TTL: the wallet's
/// `last_processed_height` advanced at least
/// `RESERVATION_MAX_AGE_BLOCKS`
/// blocks past the height the reservation was stamped at
/// ([`SignedCoreTransaction::reservation_height`](crate::SignedCoreTransaction::reservation_height)).
/// Broadcasting it could spend against a newer, unrelated reservation, so it
/// is refused **before** touching the network — NOT retryable in place, the
/// caller must rebuild the payment. The refusal reconciles the reservation
/// on the way out: a funded finalize always stamps an owner token, so the
/// release is owner-guarded (`release_reservation_if_owner`, safe at any
/// age — it no-ops once ownership transferred) and the still-owned inputs
/// are freed for the instructed rebuild. Abandoning/freeing the handle
/// likewise releases owner-guarded at any age; only a token-less build
/// skips its unguarded by-outpoint release past the bound and leaves the
/// aged outpoint for key-wallet's TTL to reclaim.
///
/// This is the handle-path sibling of the deferred registry-token
/// [`SignedPaymentError::StaleReservationToken`](crate::SignedPaymentError::StaleReservationToken);
/// both share the same age bound and the FFI `ErrorStaleReservationToken`
/// code. Carries no token — the handle path is keyed by an opaque handle,
/// not a numeric reservation token.
#[error("finalized transaction reservation has outlived its lifetime; rebuild the payment")]
StaleReservation,

Comment thread
coderabbitai[bot] marked this conversation as resolved.
#[error("Transaction building failed: {0}")]
TransactionBuild(String),

Expand Down
32 changes: 32 additions & 0 deletions packages/rs-platform-wallet/src/test_support.rs
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,38 @@ pub async fn funded_spv_core_wallet(
)
}

/// Advance `core`'s `last_processed_height` to just past the reservation age
/// guard bound ([`RESERVATION_MAX_AGE_BLOCKS`](crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS))
/// but below key-wallet's `ReservationSet` TTL, so a handle finalized at the
/// current height ages enough to trip the software guard while its underlying
/// reservation is provably still held (no key-wallet sweep yet). Returns the new
/// height.
///
/// FFI lifecycle tests use this to exercise aged owner-guarded cleanup — the
/// deinit/GC backstop and the broadcast/abandon failure paths that route their
/// cleanup through `abandon_transaction`, which releases owner-guarded at any
/// age (only a token-less build skips its by-outpoint release).
pub async fn age_core_past_reservation_guard<B>(core: &crate::CoreWallet<B>) -> u32
where
B: crate::broadcaster::TransactionBroadcaster + ?Sized,
{
use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;

let stamped = core
.last_processed_height()
.await
.expect("wallet present in manager");
let target = stamped + crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS + 2;
{
let mut wm = core.wallet_manager.write().await;
let (_, info) = wm
.get_wallet_and_info_mut(&core.wallet_id())
.expect("wallet present in manager");
info.core_wallet.update_last_processed_height(target);
}
target
}

/// No-op persister satisfying [`PlatformWalletManager`] construction for tests
/// that need a full [`PlatformWallet`] but no real persistence pipeline.
pub struct NoopTestPersister;
Expand Down
Loading
Loading