diff --git a/packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs b/packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs index 2e5e9068292..34fc6b6adf8 100644 --- a/packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs +++ b/packages/rs-platform-wallet-ffi/src/asset_lock/manager.rs @@ -3,7 +3,7 @@ use crate::error::*; use crate::handle::*; use crate::runtime::runtime; -use crate::{check_ptr, unwrap_option_or_return}; +use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; /// C-compatible tracked asset lock entry. #[repr(C)] @@ -54,7 +54,7 @@ pub unsafe extern "C" fn asset_lock_manager_list_tracked_locks( let option = ASSET_LOCK_MANAGER_STORAGE.with_item(handle, |manager| { use platform_wallet::AssetLockStatus; - let locks = runtime().block_on(manager.list_tracked_locks()); + let locks = runtime().try_block_on(manager.list_tracked_locks())?; let entries: Vec = locks .iter() .map(|lock| { @@ -86,9 +86,9 @@ pub unsafe extern "C" fn asset_lock_manager_list_tracked_locks( } }) .collect(); - entries + Ok::<_, crate::panic_guard::FfiBoundaryError>(entries) }); - let entries = unwrap_option_or_return!(option); + let entries = unwrap_result_or_return!(unwrap_option_or_return!(option)); *out_count = entries.len(); if entries.is_empty() { diff --git a/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs b/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs index 5b840f9d039..a9e730ae975 100644 --- a/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs +++ b/packages/rs-platform-wallet-ffi/src/asset_lock/sync.rs @@ -132,6 +132,9 @@ pub unsafe extern "C" fn asset_lock_manager_catch_up_blocking( ); } }; + // Peel the FFI-local outer failure off first: the arm below adds context + // around the error, which would push a caught panic's marker off position 0. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); match result { Ok(_) => { tracing::info!( diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs index 8eacdf4f355..bd4a6a3ed4c 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs @@ -1,23 +1,31 @@ //! FFI bindings for CoreWallet transaction broadcasting. +use crate::core_wallet::lifecycle::lifecycle_gate_or_release; use crate::error::*; use crate::handle::*; +use crate::panic_guard::GuardedError; use crate::runtime::runtime; use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; use platform_wallet::PlatformWalletError; use std::os::raw::c_char; fn classify_broadcast_result( - result: Result, + result: Result>, local_txid: dashcore::Txid, ) -> (Option, PlatformWalletFFIResult) { match result { Ok(_) => (Some(local_txid), PlatformWalletFFIResult::ok()), - Err(error @ PlatformWalletError::TransactionBroadcast(_)) - | Err(error @ PlatformWalletError::TransactionBroadcastUnconfirmed(_)) => { - (Some(local_txid), error.into()) - } - Err(error) => (None, error.into()), + // A boundary failure (caught panic / no runtime) carries NO outcome + // guarantee, so it gets the same shape as any other unclassified + // failure: the generic code, its own message verbatim, and no txid — + // it must not borrow the `Some(local_txid)` treatment that says + // "this reached the network, reconcile by this id". + Err(GuardedError::Boundary(error)) => (None, error.into()), + Err(GuardedError::Domain(error @ PlatformWalletError::TransactionBroadcast(_))) + | Err(GuardedError::Domain( + error @ PlatformWalletError::TransactionBroadcastUnconfirmed(_), + )) => (Some(local_txid), error.into()), + Err(GuardedError::Domain(error)) => (None, error.into()), } } @@ -76,11 +84,11 @@ pub unsafe extern "C" fn core_wallet_broadcast_signed_transaction( // exclusive side, so it cannot interleave between the check and the send. // Scoped per generation, so this send — up to the broadcaster's timeout — // blocks only THIS wallet's teardown, never an unrelated wallet's. - let (_lifecycle, wallet_is_live) = runtime().block_on(async { - let gate = wallet.generation_payment_guard().await; - let live = wallet.is_current_generation().await; - (gate, live) - }); + let (_lifecycle, wallet_is_live) = unwrap_result_or_return!(lifecycle_gate_or_release( + &wallet, + &finalized.wallet, + &finalized.transaction + )); if !wallet_is_live { runtime().block_on(finalized.wallet.abandon_transaction(&finalized.transaction)); return PlatformWalletFFIResult::err( @@ -138,11 +146,17 @@ pub unsafe extern "C" fn core_wallet_abandon_signed_transaction( "transaction was finalized by a different wallet generation".to_string(), ); } - runtime().block_on( + // `try_block_on`: releasing the reservation IS this entry point's job, and + // the handle was consumed on entry so there is no retry — a panic must not + // fall through to a success the host records as "reservation released" + // (same contract as `core_wallet_signed_payment_release`). The two abandon + // calls in the error arms above stay on the swallowing `block_on`: they are + // best-effort cleanup on paths that already report an error of their own. + unwrap_result_or_return!(runtime().try_block_on( transaction .wallet .abandon_transaction(&transaction.transaction), - ); + )); PlatformWalletFFIResult::ok() } @@ -204,6 +218,13 @@ mod outcome_tests { dashcore::Txid::from_byte_array([byte; 32]) } + /// A domain error in the guarded shape the classifier now takes. + fn domain( + error: PlatformWalletError, + ) -> Result> { + Err(GuardedError::Domain(error)) + } + #[test] fn network_outcomes_all_carry_a_txid() { let accepted = classify_broadcast_result(Ok(txid(1)), txid(9)); @@ -211,7 +232,7 @@ mod outcome_tests { assert_eq!(accepted.1.code, PlatformWalletFFIResultCode::Success); let rejected = classify_broadcast_result( - Err(PlatformWalletError::TransactionBroadcast( + domain(PlatformWalletError::TransactionBroadcast( "rejected".to_string(), )), txid(2), @@ -223,7 +244,7 @@ mod outcome_tests { ); let unknown = classify_broadcast_result( - Err(PlatformWalletError::TransactionBroadcastUnconfirmed( + domain(PlatformWalletError::TransactionBroadcastUnconfirmed( "timeout".to_string(), )), txid(3), @@ -238,7 +259,7 @@ mod outcome_tests { #[test] fn operational_error_does_not_carry_a_txid() { let outcome = classify_broadcast_result( - Err(PlatformWalletError::TransactionBuild("invalid".to_string())), + domain(PlatformWalletError::TransactionBuild("invalid".to_string())), txid(4), ); assert_eq!(outcome.0, None); @@ -258,12 +279,14 @@ mod tests { use platform_wallet::{CoreWallet, SignedCoreTransaction}; use super::*; + use crate::core_wallet::lifecycle::arm_lifecycle_gate_panic; use crate::core_wallet::FFICoreSignedTransaction; type TestCore = CoreWallet; fn finalize(core: &TestCore, signer: &WalletSigner, tag: u8) -> SignedCoreTransaction { runtime() + .raw() .block_on(core.finalize_transaction( TransactionBuilder::new().add_output( &Address::dummy(Network::Testnet, usize::from(tag)), @@ -285,13 +308,14 @@ mod tests { fn assert_released(core: &TestCore, signer: &WalletSigner, tag: u8) { let retry = finalize(core, signer, tag); - runtime().block_on(core.abandon_transaction(&retry)); + runtime().raw().block_on(core.abandon_transaction(&retry)); } #[test] fn double_free_is_safe_and_releases_reservation() { - let (core, signer) = - runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); + let (core, signer) = runtime() + .raw() + .block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); let transaction_handle = insert(&core, finalize(&core, &signer, 40)); core_wallet_signed_transaction_free(transaction_handle); @@ -302,8 +326,9 @@ mod tests { #[test] fn invalid_or_wrong_wallet_consumes_and_releases() { - let (origin, origin_signer) = - runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); + let (origin, origin_signer) = runtime() + .raw() + .block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); let invalid_transaction = insert(&origin, finalize(&origin, &origin_signer, 42)); let invalid = unsafe { core_wallet_abandon_signed_transaction(u64::MAX, invalid_transaction) }; @@ -313,8 +338,9 @@ mod tests { ); assert_released(&origin, &origin_signer, 43); - let (other, _) = - runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); + let (other, _) = runtime() + .raw() + .block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); let other_handle = CORE_WALLET_STORAGE.insert(other); let wrong_transaction = insert(&origin, finalize(&origin, &origin_signer, 44)); let wrong = @@ -329,8 +355,9 @@ mod tests { #[test] fn abandon_then_free_or_broadcast_cannot_reconsume_handle() { - let (core, signer) = - runtime().block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); + let (core, signer) = runtime() + .raw() + .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, 46)); @@ -348,4 +375,144 @@ mod tests { assert_released(&core, &signer, 47); CORE_WALLET_STORAGE.remove(core_handle); } + + /// The main-path abandon is this entry point's whole job, and the handle + /// is consumed on entry — so a panic during the release must surface as + /// the typed panic error, never fall through to `ok()` for the host to + /// record as "reservation released". + /// + /// Driving the call from inside a runtime context makes the guarded + /// `block_on` panic in the guarded region of the genuine entry point + /// ("Cannot start a runtime from within a runtime") before the abandon + /// future is ever polled — a real panic, on the real path. Before the + /// fix, `block_on`'s `()` recovery swallowed exactly this panic and the + /// function reported `Success`. + #[test] + fn abandon_reports_a_panic_as_an_error_not_success() { + let (core, signer) = runtime() + .raw() + .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, 48)); + + let result = tokio::runtime::Builder::new_current_thread() + .build() + .expect("build helper runtime") + .block_on(async { + unsafe { core_wallet_abandon_signed_transaction(core_handle, transaction_handle) } + }); + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation, + "a panicked abandon must not report success — the host would \ + record the reservation as released" + ); + let message = unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_str() + .expect("message is UTF-8"); + assert!( + message.starts_with(crate::panic_guard::FFI_PANIC_PREFIX), + "message must carry the panic marker: {message}" + ); + + // The handle was consumed on entry regardless of the panic — a retry + // is a clean not-found error, not a second release attempt. + let retry = + unsafe { core_wallet_abandon_signed_transaction(core_handle, transaction_handle) }; + assert_eq!(retry.code, PlatformWalletFFIResultCode::NotFound); + CORE_WALLET_STORAGE.remove(core_handle); + } + + /// A lifecycle-gate failure must not strand the build's UTXO reservation. + /// + /// The gate is taken *after* `finalize_transaction` has reserved the + /// inputs, and on this path the transaction handle has already been + /// consumed on entry — so if the guarded acquisition fails and the entry + /// point just returns, the host is left holding neither handle nor token, + /// and nothing in the process can ever release those inputs again. The + /// wallet would silently lose that much spendable balance until restart + /// (`dashpay/platform#4424` review). + /// + /// `assert_released` is the proof: it finalizes a *second* transaction + /// against the same account, which can only fund if the first build's + /// inputs came back. + #[test] + fn a_lifecycle_gate_panic_releases_the_reservation_and_reports_the_panic() { + let (core, signer) = runtime() + .raw() + .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, 50)); + let mut out_txid: *mut c_char = std::ptr::null_mut(); + + arm_lifecycle_gate_panic(); + let result = unsafe { + core_wallet_broadcast_signed_transaction(core_handle, transaction_handle, &mut out_txid) + }; + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation, + "a caught panic must arrive as the generic code" + ); + let message = unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_str() + .expect("message is UTF-8"); + assert_eq!( + message.find(crate::panic_guard::FFI_PANIC_PREFIX), + Some(0), + "the marker must be at position 0: {message}" + ); + assert!( + message.contains("injected lifecycle-gate panic"), + "the payload must survive: {message}" + ); + assert!( + out_txid.is_null(), + "nothing was broadcast, so no txid may be published" + ); + + // The reservation came back: a fresh build on the same account funds. + assert_released(&core, &signer, 51); + + // The handle was consumed on entry, panic or not. + let retry = unsafe { + core_wallet_broadcast_signed_transaction(core_handle, transaction_handle, &mut out_txid) + }; + assert_eq!(retry.code, PlatformWalletFFIResultCode::NotFound); + CORE_WALLET_STORAGE.remove(core_handle); + } + + /// The same guarantee stated at the seam the other two entry points share + /// (`core_wallet_tx_builder_finalize` and + /// `core_wallet_signed_payment_finalize` reach it with an unpublished + /// handle / unregistered token respectively). Those two need a live + /// `MnemonicResolverHandle` vtable to drive end-to-end, so the shared + /// helper is pinned directly instead. + #[test] + fn lifecycle_gate_helper_releases_before_returning_the_failure() { + let (core, signer) = runtime() + .raw() + .block_on(funded_spv_core_wallet(StandardAccountType::BIP44Account)); + let finalized = finalize(&core, &signer, 52); + + arm_lifecycle_gate_panic(); + let error = lifecycle_gate_or_release(&core, &core, &finalized) + .expect_err("the armed panic must surface as a boundary failure"); + assert_eq!( + error.to_string().find(crate::panic_guard::FFI_PANIC_PREFIX), + Some(0) + ); + + assert_released(&core, &signer, 53); + + // The happy path still hands back a held gate and a live generation. + let (_gate, live) = lifecycle_gate_or_release(&core, &core, &finalized) + .expect("acquisition must succeed when nothing is armed"); + assert!(live, "a freshly built wallet is its own current generation"); + runtime() + .raw() + .block_on(core.abandon_transaction(&finalized)); + } } diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/lifecycle.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/lifecycle.rs new file mode 100644 index 00000000000..3b937dadf69 --- /dev/null +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/lifecycle.rs @@ -0,0 +1,105 @@ +//! The generation lifecycle gate, taken on behalf of an already-finalized +//! transaction. +//! +//! # Why this is one function and not three inline blocks +//! +//! Three entry points — `core_wallet_tx_builder_finalize`, +//! `core_wallet_signed_payment_finalize` and +//! `core_wallet_broadcast_signed_transaction` — reach a point where a +//! `SignedCoreTransaction` exists and **holds a UTXO reservation**, but no +//! handle or token has been published to the host yet (or, on the broadcast +//! path, the handle has already been consumed). Between that point and the +//! publish/send they take this wallet generation's lifecycle gate and check +//! that the generation is still live. +//! +//! Acquiring the gate is a guarded call, so it can now fail on its own account +//! (a caught panic, or an async runtime that would not build). When it does, +//! the entry point returns an error — and the reservation it is holding has +//! **nothing left that could ever release it**: the host got no token and no +//! handle, so the inputs stay reserved for the life of the process and the +//! wallet quietly loses spendable balance (`dashpay/platform#4424` review, +//! CodeRabbit + Codex, same finding). +//! +//! So the release lives here, next to the acquisition that can fail, rather +//! than being re-derived correctly at three call sites. +//! +//! # The release is deliberately best-effort +//! +//! `abandon_transaction` runs on the *swallowing* `block_on` (its `()` output +//! absorbs a boundary failure into the guard's `ERROR` log). That is the right +//! shape here and only here: this path is already returning an error of its +//! own, so a second failure has nothing to add and must not mask the first. +//! The release is also generation-bound, so on a genuine teardown it is a +//! logged no-op and on a re-create it correctly declines to touch the new +//! generation's inputs. + +use platform_wallet::broadcaster::SpvBroadcaster; +use platform_wallet::{CoreWallet, SignedCoreTransaction}; +use tokio::sync::RwLockReadGuard; + +use crate::panic_guard::FfiBoundaryError; +use crate::runtime::runtime; + +// Fault injection for the release-on-failure tests. +// +// The failure this guards against is a panic *inside* the guarded region, +// which is not otherwise reachable from a test: driving a real entry point +// from inside a runtime context would make the compensating `abandon` panic +// too, and so could never show that the reservation came back. Arming this +// makes only the gate acquisition panic, on the real code path, which is +// exactly the scenario. +// +// `#[cfg(test)]`, so it does not exist in the cdylib. +#[cfg(test)] +thread_local! { + static PANIC_IN_LIFECYCLE_GATE: std::cell::Cell = const { std::cell::Cell::new(false) }; +} + +/// Arm a one-shot panic in the next [`lifecycle_gate_or_release`] on this +/// thread. +#[cfg(test)] +pub(crate) fn arm_lifecycle_gate_panic() { + PANIC_IN_LIFECYCLE_GATE.with(|armed| armed.set(true)); +} + +/// Take `gate_on`'s generation lifecycle gate and read its liveness, releasing +/// `finalized`'s reservation through `release_on` if the *acquisition itself* +/// fails. +/// +/// `gate_on` and `release_on` are separate because the broadcast path gates on +/// the caller-supplied wallet handle but acts through the transaction's own +/// originating wallet; the two finalize paths pass the same wallet twice. +/// +/// Returns the held gate plus whether the generation is still registered. The +/// caller keeps the gate alive across its own publish/send so a teardown cannot +/// interleave between the check and the act. +/// +/// A `false` liveness result is NOT a failure here — the caller has its own +/// (already correct) reconciliation and error message for that case, so this +/// function leaves it alone. +pub(crate) fn lifecycle_gate_or_release<'a>( + gate_on: &'a CoreWallet, + release_on: &CoreWallet, + finalized: &SignedCoreTransaction, +) -> Result<(RwLockReadGuard<'a, ()>, bool), FfiBoundaryError> { + let acquired = runtime().try_block_on(async { + #[cfg(test)] + if PANIC_IN_LIFECYCLE_GATE.with(std::cell::Cell::take) { + panic!("injected lifecycle-gate panic"); + } + let gate = gate_on.generation_payment_guard().await; + let live = gate_on.is_current_generation().await; + (gate, live) + }); + + match acquired { + Ok(acquired) => Ok(acquired), + Err(error) => { + // Nothing was published and nothing was registered, so no token or + // handle exists that could ever release this build's reservation. + // Reconcile it here or it is held until the process exits. + runtime().block_on(release_on.abandon_transaction(finalized)); + Err(error) + } + } +} diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs index fe8e737d051..0357521e3b6 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/mod.rs @@ -4,6 +4,7 @@ mod addresses; mod broadcast; +pub(crate) mod lifecycle; mod sign_message; pub(crate) mod signed_payment; mod transaction_builder; diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs index bb516ccb2ca..d746cc43686 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/signed_payment.rs @@ -18,7 +18,7 @@ use crate::error::*; use crate::handle::{Handle, CORE_WALLET_STORAGE}; use crate::runtime::runtime; -use crate::{check_ptr, unwrap_option_or_return}; +use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; use once_cell::sync::Lazy; use platform_wallet::broadcaster::SpvBroadcaster; use platform_wallet::{ReservationToken, SignedPaymentError, SignedPaymentRegistry}; @@ -86,8 +86,17 @@ pub unsafe extern "C" fn core_wallet_signed_payment_broadcast( let core = unwrap_option_or_return!(CORE_WALLET_STORAGE.with_item(core_handle, |w| w.clone())); - let result = - runtime().block_on(SIGNED_PAYMENT_REGISTRY.broadcast(ReservationToken::from(token), &core)); + // `try_block_on`, deliberately NOT a `FromCaughtPanicError` impl on + // `SignedPaymentError`: its only generic-ish variant is `Broadcast(..)`, + // whose payload carries the typed retry semantics of a REAL broadcast + // outcome. A panic must not be dressed up as one — it reaches the host as + // the generic ErrorWalletOperation with the panic text instead. + let result = match runtime() + .try_block_on(SIGNED_PAYMENT_REGISTRY.broadcast(ReservationToken::from(token), &core)) + { + Ok(result) => result, + Err(error) => return error.into(), + }; match result { Ok(txid) => { @@ -144,6 +153,10 @@ pub unsafe extern "C" fn core_wallet_signed_payment_broadcast( /// Always safe to call; `token` is a plain value. #[no_mangle] pub unsafe extern "C" fn core_wallet_signed_payment_release(token: u64) -> PlatformWalletFFIResult { - runtime().block_on(SIGNED_PAYMENT_REGISTRY.release(ReservationToken::from(token))); + // `try_block_on`: releasing IS this entry point's job, so a panic must + // not come back as a success the host records as "reservation released". + unwrap_result_or_return!( + runtime().try_block_on(SIGNED_PAYMENT_REGISTRY.release(ReservationToken::from(token))) + ); PlatformWalletFFIResult::ok() } diff --git a/packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs b/packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs index 2cefb0888a0..d2797b4fd75 100644 --- a/packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs +++ b/packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs @@ -1,3 +1,4 @@ +use crate::core_wallet::lifecycle::lifecycle_gate_or_release; use crate::core_wallet_types::OutPointFFI; use crate::error::*; use crate::handle::{Handle, CORE_SIGNED_TRANSACTION_STORAGE, PLATFORM_WALLET_STORAGE}; @@ -165,11 +166,11 @@ pub unsafe extern "C" fn core_wallet_tx_builder_finalize( // the signer await, never around it: holding it across an open signing prompt // would stall this wallet's teardown for as long as the user takes, and the // check makes that unnecessary. - 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) - }); + let (_lifecycle, wallet_is_live) = unwrap_result_or_return!(lifecycle_gate_or_release( + wallet.core(), + wallet.core(), + &finalized + )); if !wallet_is_live { // No handle was published, so nothing would ever release this build's // reservation. Reconcile it here: the release is generation-bound, so on @@ -299,11 +300,11 @@ pub unsafe extern "C" fn core_wallet_signed_payment_finalize( // Deliberately acquired AFTER the signer await rather than around it: holding // it across an open signing prompt would stall this wallet's teardown for as // long as the user takes, and the check below makes that unnecessary. - 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) - }); + let (_lifecycle, wallet_is_live) = unwrap_result_or_return!(lifecycle_gate_or_release( + wallet.core(), + wallet.core(), + &finalized + )); if !wallet_is_live { // Nothing was registered, so no token would ever release this build's // reservation. Reconcile it here: the release is generation-bound, so on @@ -777,6 +778,11 @@ pub unsafe extern "C" fn core_wallet_tx_builder_add_inputs_from_outpoints( Ok::<_, String>(()) }); + // Peel the FFI-local outer failure off first. This is one of the three + // call sites the review named: `add_inputs_from_outpoints failed: ` would + // otherwise be prepended to a caught panic, pushing its marker off + // position 0 (dashpay/platform#4424 review). + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); match result { Ok(()) => PlatformWalletFFIResult::ok(), Err(e) => PlatformWalletFFIResult::err( diff --git a/packages/rs-platform-wallet-ffi/src/dashpay.rs b/packages/rs-platform-wallet-ffi/src/dashpay.rs index f87050caec6..9fcac6ae63c 100644 --- a/packages/rs-platform-wallet-ffi/src/dashpay.rs +++ b/packages/rs-platform-wallet-ffi/src/dashpay.rs @@ -42,7 +42,7 @@ use crate::contact_request::CONTACT_REQUEST_STORAGE; use crate::error::*; use crate::established_contact::ESTABLISHED_CONTACT_STORAGE; use crate::handle::*; -use crate::runtime::block_on_worker; +use crate::runtime::{block_on_worker, try_block_on_worker}; use crate::types::*; use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; @@ -878,7 +878,7 @@ pub unsafe extern "C" fn platform_wallet_drain_pending_contact_crypto( network, ) }; - block_on_worker(async move { + try_block_on_worker(async move { let drained = identity .dashpay() .drain_pending_contact_crypto(&provider) @@ -897,7 +897,7 @@ pub unsafe extern "C" fn platform_wallet_drain_pending_contact_crypto( drained + accepted }) }); - let total = unwrap_option_or_return!(option); + let total = unwrap_result_or_return!(unwrap_option_or_return!(option)); unsafe { *out_drained = total as u32; } @@ -927,9 +927,9 @@ pub unsafe extern "C" fn platform_wallet_pending_contact_crypto_count( let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity = wallet.identity().clone(); - block_on_worker(async move { identity.dashpay().pending_contact_crypto_count().await }) + try_block_on_worker(async move { identity.dashpay().pending_contact_crypto_count().await }) }); - let count = unwrap_option_or_return!(option); + let count = unwrap_result_or_return!(unwrap_option_or_return!(option)); unsafe { *out_count = count as u32; } @@ -1049,6 +1049,9 @@ pub unsafe extern "C" fn platform_wallet_verify_seed_binds_to_wallet( block_on_worker(async move { wallet.verify_seed_binds(&provider).await }) }); let result = unwrap_option_or_return!(option); + // Peel the FFI-local outer failure off first: the domain arms below add + // operation context, which a caught panic must never be dressed in. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); match result { Ok(()) => PlatformWalletFFIResult::ok(), Err(e @ platform_wallet::PlatformWalletError::SeedMismatch { .. }) => { @@ -1148,6 +1151,9 @@ pub unsafe extern "C" fn platform_wallet_verify_seed_binds_to_wallet_cached( }) }); let result = unwrap_option_or_return!(option); + // Peel the FFI-local outer failure off first: the domain arms below add + // operation context, which a caught panic must never be dressed in. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); match result { Ok((platform_wallet::SeedBindingVerification::MarkerMatched, _)) => { PlatformWalletFFIResult::ok() @@ -1201,9 +1207,11 @@ pub unsafe extern "C" fn platform_wallet_drainable_contact_crypto_count( let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity = wallet.identity().clone(); - block_on_worker(async move { identity.dashpay().drainable_contact_crypto_count().await }) + try_block_on_worker( + async move { identity.dashpay().drainable_contact_crypto_count().await }, + ) }); - let count = unwrap_option_or_return!(option); + let count = unwrap_result_or_return!(unwrap_option_or_return!(option)); unsafe { *out_count = count as u32; } diff --git a/packages/rs-platform-wallet-ffi/src/dashpay_sync.rs b/packages/rs-platform-wallet-ffi/src/dashpay_sync.rs index 7bcdc34fe76..fec3f57cd4a 100644 --- a/packages/rs-platform-wallet-ffi/src/dashpay_sync.rs +++ b/packages/rs-platform-wallet-ffi/src/dashpay_sync.rs @@ -31,8 +31,8 @@ use std::time::Duration; use crate::error::*; use crate::handle::*; -use crate::runtime::{block_on_worker, runtime}; -use crate::{check_ptr, unwrap_option_or_return}; +use crate::runtime::{runtime, try_block_on_worker}; +use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; /// Start the recurring DashPay sync loop in the background. Idempotent /// — calling while already running is a no-op. @@ -41,10 +41,15 @@ pub unsafe extern "C" fn platform_wallet_manager_dashpay_sync_start( handle: Handle, ) -> PlatformWalletFFIResult { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - let _entered = runtime().enter(); + // The loop's `tokio::spawn` needs a runtime in scope, so acquisition + // is fallible here: with no runtime there is nothing to start the + // DashPay loop on, and that has to be reported rather than + // silently skipped. + let _entered = runtime().checked()?.enter(); manager.dashpay_sync_arc().start(); + Ok::<(), crate::panic_guard::FfiBoundaryError>(()) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -170,9 +175,9 @@ pub unsafe extern "C" fn platform_wallet_manager_dashpay_sync_sync_now( // the ~512 KB stack of the iOS calling thread (SIGBUS observed // on-device 2026-06-12). The worker dispatch moves the compute // onto the runtime's 8 MB-stack threads (see runtime.rs). - block_on_worker(async move { mgr.sync_now().await }) + try_block_on_worker(async move { mgr.sync_now().await }) }); - let summary = unwrap_option_or_return!(option); + let summary = unwrap_result_or_return!(unwrap_option_or_return!(option)); if !out_success_count.is_null() { *out_success_count = summary.success_count(); diff --git a/packages/rs-platform-wallet-ffi/src/document.rs b/packages/rs-platform-wallet-ffi/src/document.rs index 32509bc3356..ca9007784e9 100644 --- a/packages/rs-platform-wallet-ffi/src/document.rs +++ b/packages/rs-platform-wallet-ffi/src/document.rs @@ -14,6 +14,7 @@ use rs_sdk_ffi::{SignerHandle, VTableSigner}; use crate::check_ptr; use crate::error::*; use crate::handle::*; +use crate::panic_guard::GuardedError; use crate::runtime::block_on_worker; use crate::types::read_identifier; use crate::{unwrap_option_or_return, unwrap_result_or_return}; @@ -89,7 +90,7 @@ pub unsafe extern "C" fn platform_wallet_create_document_with_signer( let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity_wallet = wallet.identity().clone(); - let result: Result<(Identifier, String), PlatformWalletError> = + let result: Result<(Identifier, String), GuardedError> = block_on_worker(async move { let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); let confirmed: Document = identity_wallet @@ -205,7 +206,7 @@ pub unsafe extern "C" fn platform_wallet_document_replace( let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity_wallet = wallet.identity().clone(); - let result: Result<(Identifier, String), PlatformWalletError> = + let result: Result<(Identifier, String), GuardedError> = block_on_worker(async move { let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); let confirmed: Document = identity_wallet @@ -270,20 +271,21 @@ pub unsafe extern "C" fn platform_wallet_document_delete( let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity_wallet = wallet.identity().clone(); - let result: Result = block_on_worker(async move { - let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); - let deleted_id: Identifier = identity_wallet - .delete_document_with_signer( - &owner_id, - &contract_id_value, - &document_type_str, - &document_id_value, - signing_key_id, - signer, - ) - .await?; - Ok::<_, PlatformWalletError>(deleted_id) - }); + let result: Result> = + block_on_worker(async move { + let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); + let deleted_id: Identifier = identity_wallet + .delete_document_with_signer( + &owner_id, + &contract_id_value, + &document_type_str, + &document_id_value, + signing_key_id, + signer, + ) + .await?; + Ok::<_, PlatformWalletError>(deleted_id) + }); result }); let result = unwrap_option_or_return!(option); @@ -337,7 +339,7 @@ pub unsafe extern "C" fn platform_wallet_document_transfer( let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity_wallet = wallet.identity().clone(); - let result: Result<(Identifier, String), PlatformWalletError> = + let result: Result<(Identifier, String), GuardedError> = block_on_worker(async move { let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); let confirmed: Document = identity_wallet @@ -408,7 +410,7 @@ pub unsafe extern "C" fn platform_wallet_document_set_price( let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity_wallet = wallet.identity().clone(); - let result: Result<(Identifier, String), PlatformWalletError> = + let result: Result<(Identifier, String), GuardedError> = block_on_worker(async move { let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); let confirmed: Document = identity_wallet @@ -481,7 +483,7 @@ pub unsafe extern "C" fn platform_wallet_document_purchase( let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { let identity_wallet = wallet.identity().clone(); - let result: Result<(Identifier, String), PlatformWalletError> = + let result: Result<(Identifier, String), GuardedError> = block_on_worker(async move { let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); let confirmed: Document = identity_wallet diff --git a/packages/rs-platform-wallet-ffi/src/dpns_sync.rs b/packages/rs-platform-wallet-ffi/src/dpns_sync.rs index 2b31cf6aabb..98a699d6c0b 100644 --- a/packages/rs-platform-wallet-ffi/src/dpns_sync.rs +++ b/packages/rs-platform-wallet-ffi/src/dpns_sync.rs @@ -27,8 +27,8 @@ use std::time::Duration; use crate::error::*; use crate::handle::*; -use crate::runtime::{block_on_worker, runtime}; -use crate::{check_ptr, unwrap_option_or_return}; +use crate::runtime::{runtime, try_block_on_worker}; +use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; /// Start the recurring DPNS marketplace sync loop in the background. /// Idempotent — calling while already running is a no-op. @@ -37,10 +37,15 @@ pub unsafe extern "C" fn platform_wallet_manager_dpns_sync_start( handle: Handle, ) -> PlatformWalletFFIResult { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - let _entered = runtime().enter(); + // The loop's `tokio::spawn` needs a runtime in scope, so acquisition + // is fallible here: with no runtime there is nothing to start the + // DPNS marketplace loop on, and that has to be reported rather than + // silently skipped. + let _entered = runtime().checked()?.enter(); manager.dpns_sync_arc().start(); + Ok::<(), crate::panic_guard::FfiBoundaryError>(()) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -164,9 +169,9 @@ pub unsafe extern "C" fn platform_wallet_manager_dpns_sync_sync_now( // GroveDB document-query proofs whose recursion blows the ~512 KB // stack of the iOS calling thread. The worker dispatch moves the // compute onto the runtime's 8 MB-stack threads (see runtime.rs). - block_on_worker(async move { mgr.sync_now().await }) + try_block_on_worker(async move { mgr.sync_now().await }) }); - let summary = unwrap_option_or_return!(option); + let summary = unwrap_result_or_return!(unwrap_option_or_return!(option)); if !out_success_count.is_null() { *out_success_count = summary.success_count(); diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 444573c5dbc..1534255c158 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -690,6 +690,19 @@ impl From for PlatformWalletFFIResult { // rides `NotFound` rather than spending a fifth marketplace // code hosts would handle identically. PlatformWalletError::DpnsNameNotFound { .. } => PlatformWalletFFIResultCode::NotFound, + // NOTE: a panic caught below an entry point is deliberately NOT a + // variant of this enum, so it never reaches this match at all. It + // rides `crate::panic_guard::FfiBoundaryError` — an FFI-LOCAL OUTER + // result — which every entry point intercepts before reaching here, + // and which converts straight to `ErrorWalletOperation` with its + // `FFI_PANIC_PREFIX` still at position 0. Folding it into a domain + // variant instead would (a) hand this table a value with no + // retry/outcome meaning to classify, (b) let the arms below re-code + // or re-prefix a message hosts are told to key on, and (c) add a + // variant to a public, non-`#[non_exhaustive]` enum in the + // lower-level `platform-wallet` crate — a source break for every + // downstream exhaustive match (dashpay/platform#4424 review). + // // NOTE: `MessageSigningFailed` is deliberately NOT matched, so it // falls to the `ErrorUnknown` catch-all below. Its causes are // internal invariant breaks (a public key that does not own the diff --git a/packages/rs-platform-wallet-ffi/src/identity_sync.rs b/packages/rs-platform-wallet-ffi/src/identity_sync.rs index a05a543185c..848d73c74bf 100644 --- a/packages/rs-platform-wallet-ffi/src/identity_sync.rs +++ b/packages/rs-platform-wallet-ffi/src/identity_sync.rs @@ -18,8 +18,8 @@ use platform_wallet::{IdentityTokenSyncInfo, IdentityTokenSyncState}; use crate::error::*; use crate::handle::*; -use crate::runtime::{block_on_worker, runtime}; -use crate::{check_ptr, unwrap_option_or_return}; +use crate::runtime::{runtime, try_block_on_worker}; +use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; /// Flattened per-(identity, token) row mirroring /// [`IdentityTokenSyncInfo`]. @@ -66,10 +66,15 @@ pub unsafe extern "C" fn platform_wallet_manager_identity_sync_start( handle: Handle, ) -> PlatformWalletFFIResult { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - let _entered = runtime().enter(); + // The loop's `tokio::spawn` needs a runtime in scope, so acquisition + // is fallible here: with no runtime there is nothing to start the + // identity-token loop on, and that has to be reported rather than + // silently skipped. + let _entered = runtime().checked()?.enter(); manager.identity_sync_arc().start(); + Ok::<(), crate::panic_guard::FfiBoundaryError>(()) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -136,9 +141,9 @@ pub unsafe extern "C" fn platform_wallet_manager_identity_sync_last_sync_unix_se let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { let mgr = manager.identity_sync_arc(); - runtime().block_on(async move { mgr.last_sync_unix_for_identity(&identity_id).await }) + runtime().try_block_on(async move { mgr.last_sync_unix_for_identity(&identity_id).await }) }); - let value = unwrap_option_or_return!(option); + let value = unwrap_result_or_return!(unwrap_option_or_return!(option)); *out_last_sync_unix = value.unwrap_or(0); PlatformWalletFFIResult::ok() } @@ -175,9 +180,9 @@ pub unsafe extern "C" fn platform_wallet_manager_identity_sync_sync_now( // stack of the host's dispatch/concurrency calling thread // (same SIGBUS as the shielded/dashpay Sync Now buttons). let mgr = manager.identity_sync_arc(); - block_on_worker(async move { mgr.sync_now().await }); + try_block_on_worker(async move { mgr.sync_now().await }) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -214,9 +219,9 @@ pub unsafe extern "C" fn platform_wallet_manager_identity_sync_state_for_identit let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { let mgr = manager.identity_sync_arc(); - runtime().block_on(async move { mgr.state_for_identity(&identity_id).await }) + runtime().try_block_on(async move { mgr.state_for_identity(&identity_id).await }) }); - let row = unwrap_option_or_return!(option); + let row = unwrap_result_or_return!(unwrap_option_or_return!(option)); match row { Some(state) => { let rows: Vec = state @@ -263,9 +268,9 @@ pub unsafe extern "C" fn platform_wallet_manager_identity_sync_state_all( let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { let mgr = manager.identity_sync_arc(); - runtime().block_on(async move { mgr.all_state().await }) + runtime().try_block_on(async move { mgr.all_state().await }) }); - let snapshot = unwrap_option_or_return!(option); + let snapshot = unwrap_result_or_return!(unwrap_option_or_return!(option)); let mut rows: Vec = Vec::new(); for state in snapshot.values() { for info in &state.tokens { @@ -347,9 +352,9 @@ pub unsafe extern "C" fn platform_wallet_manager_identity_sync_register_identity let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { let mgr = manager.identity_sync_arc(); - runtime().block_on(async move { mgr.register_identity(identity_id, token_ids).await }); + runtime().try_block_on(async move { mgr.register_identity(identity_id, token_ids).await }) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -369,9 +374,9 @@ pub unsafe extern "C" fn platform_wallet_manager_identity_sync_unregister_identi let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { let mgr = manager.identity_sync_arc(); - runtime().block_on(async move { mgr.unregister_identity(&identity_id).await }); + runtime().try_block_on(async move { mgr.unregister_identity(&identity_id).await }) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -405,8 +410,9 @@ pub unsafe extern "C" fn platform_wallet_manager_identity_sync_update_watched_to let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { let mgr = manager.identity_sync_arc(); - runtime().block_on(async move { mgr.update_watched_tokens(identity_id, token_ids).await }); + runtime() + .try_block_on(async move { mgr.update_watched_tokens(identity_id, token_ids).await }) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } diff --git a/packages/rs-platform-wallet-ffi/src/identity_withdrawal.rs b/packages/rs-platform-wallet-ffi/src/identity_withdrawal.rs index 8b330d62aae..a3226e034b5 100644 --- a/packages/rs-platform-wallet-ffi/src/identity_withdrawal.rs +++ b/packages/rs-platform-wallet-ffi/src/identity_withdrawal.rs @@ -35,33 +35,32 @@ pub unsafe extern "C" fn platform_wallet_withdraw_credits_with_signer( let signer_addr = signer_handle as usize; - let option = - PLATFORM_WALLET_STORAGE.with_item( - wallet_handle, - |wallet| -> Result< - Result<(), platform_wallet::PlatformWalletError>, - PlatformWalletFFIResult, - > { - let wallet_network = wallet.platform().network(); - let to_address_parsed = to_address_unchecked - .clone() - .require_network(wallet_network) - .map_err(PlatformWalletFFIResult::from)?; - let identity_wallet = wallet.identity().clone(); - Ok(block_on_worker(async move { - let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); - identity_wallet - .withdraw_credits_with_external_signer( - &id, - amount, - &to_address_parsed, - signer, - None, - ) - .await - })) - }, - ); + let option = PLATFORM_WALLET_STORAGE.with_item( + wallet_handle, + |wallet| -> Result< + Result<(), crate::panic_guard::GuardedError>, + PlatformWalletFFIResult, + > { + let wallet_network = wallet.platform().network(); + let to_address_parsed = to_address_unchecked + .clone() + .require_network(wallet_network) + .map_err(PlatformWalletFFIResult::from)?; + let identity_wallet = wallet.identity().clone(); + Ok(block_on_worker(async move { + let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); + identity_wallet + .withdraw_credits_with_external_signer( + &id, + amount, + &to_address_parsed, + signer, + None, + ) + .await + })) + }, + ); let inner = unwrap_option_or_return!(option); let result = unwrap_result_or_return!(inner); unwrap_result_or_return!(result); diff --git a/packages/rs-platform-wallet-ffi/src/lib.rs b/packages/rs-platform-wallet-ffi/src/lib.rs index 19aa69700c2..3676dd1b5ee 100644 --- a/packages/rs-platform-wallet-ffi/src/lib.rs +++ b/packages/rs-platform-wallet-ffi/src/lib.rs @@ -59,6 +59,7 @@ pub mod manager; pub mod manager_diagnostics; pub mod memory_explorer; pub mod mnemonic_words; +mod panic_guard; pub mod persistence; pub mod platform_address_sync; pub mod platform_address_types; diff --git a/packages/rs-platform-wallet-ffi/src/manager.rs b/packages/rs-platform-wallet-ffi/src/manager.rs index 3cfcebb4957..8c785918853 100644 --- a/packages/rs-platform-wallet-ffi/src/manager.rs +++ b/packages/rs-platform-wallet-ffi/src/manager.rs @@ -278,7 +278,7 @@ unsafe fn platform_wallet_manager_create_impl( // attached. Enter the FFI's shared runtime for the duration of // the constructor so the spawn lands on it; the guard drops on // return and leaves the spawned task running on that runtime. - let _runtime_guard = runtime().enter(); + let _runtime_guard = unwrap_result_or_return!(runtime().checked()).enter(); let manager = PlatformWalletManager::new(sdk, persister, handler); let handle = PLATFORM_WALLET_MANAGER_STORAGE.insert(manager); @@ -596,9 +596,9 @@ pub unsafe extern "C" fn platform_wallet_manager_get_wallet( let wallet_id_value = *wallet_id; let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(manager_handle, |manager| { - runtime().block_on(manager.get_wallet(&wallet_id_value)) + runtime().try_block_on(manager.get_wallet(&wallet_id_value)) }); - let inner = unwrap_option_or_return!(option); + let inner = unwrap_result_or_return!(unwrap_option_or_return!(option)); match inner { Some(wallet) => { let handle = PLATFORM_WALLET_STORAGE.insert(wallet); @@ -638,18 +638,32 @@ pub unsafe extern "C" fn platform_wallet_manager_destroy( // platform-address sync. `shutdown()` is idempotent, so this is // safe even if the host already stopped some sync managers // before calling destroy. - let report = runtime().block_on(manager.shutdown()); - if !report.all_clean() { - // A worker panicked, exceeded its join budget, or stayed - // detached. Its persister/event-handler Arcs keep the host - // callback contexts alive until it actually exits, so this - // is diagnostic, not a UAF hazard. - tracing::warn!( - ?report, - "platform wallet manager shutdown did not join every worker \ - cleanly; stragglers keep their callback contexts alive and \ - release them on exit" - ); + // A panic inside `shutdown()` is logged and swallowed rather than + // returned: destroy has no failure mode in its published contract, and + // it must still drop the manager below so the host's callback contexts + // are released. `try_block_on` keeps the panic from unwinding into this + // function's `extern "C"` abort shim. + match runtime().try_block_on(manager.shutdown()) { + Ok(report) if !report.all_clean() => { + // A worker panicked, exceeded its join budget, or stayed + // detached. Its persister/event-handler Arcs keep the host + // callback contexts alive until it actually exits, so this + // is diagnostic, not a UAF hazard. + tracing::warn!( + ?report, + "platform wallet manager shutdown did not join every worker \ + cleanly; stragglers keep their callback contexts alive and \ + release them on exit" + ); + } + Ok(_) => {} + Err(error) => { + tracing::error!( + %error, + "platform wallet manager shutdown panicked; dropping the manager \ + anyway so its callback contexts are released" + ); + } } // Dropping the manager here releases its persister/event-handler // references; the host contexts are released (via `release_fn`) @@ -730,6 +744,9 @@ pub unsafe extern "C" fn platform_wallet_manager_remove_wallet( )) }); let result = unwrap_option_or_return!(option); + // Peel the FFI-local outer failure off first: the idempotency arm below + // answers `Ok()`, which a caught panic must never reach. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); match result { Ok(()) => PlatformWalletFFIResult::ok(), // Idempotency: a wallet that's already gone is the success @@ -1190,7 +1207,7 @@ mod remove_wallet_lifecycle_tests { // which asserts on `outstanding()` counts — serialize against it. let _registry = crate::core_wallet::signed_payment::registry_test_guard(); - runtime().block_on(async { + runtime().raw().block_on(async { let (manager, wallet_id) = test_platform_wallet_manager().await; let wallet = manager .get_wallet(&wallet_id) @@ -1335,7 +1352,7 @@ mod remove_wallet_lifecycle_tests { // which asserts on `outstanding()` counts — serialize against it. let _registry = crate::core_wallet::signed_payment::registry_test_guard(); - runtime().block_on(async { + runtime().raw().block_on(async { let (manager, wallet_id) = test_platform_wallet_manager().await; let wallet = manager .get_wallet(&wallet_id) @@ -1435,7 +1452,7 @@ mod remove_wallet_lifecycle_tests { // The extern "C" entry points call `runtime().block_on` themselves, so // they must be invoked from OUTSIDE a runtime context — do the async // setup first, then call across the boundary. - let core = runtime().block_on(async { + let core = runtime().raw().block_on(async { let (manager, wallet_id) = test_platform_wallet_manager().await; let wallet = manager .get_wallet(&wallet_id) @@ -1500,7 +1517,7 @@ mod remove_wallet_lifecycle_tests { fn teardown_waits_for_an_in_flight_finalized_operation_and_then_sweeps_its_handle() { let _registry = crate::core_wallet::signed_payment::registry_test_guard(); - let (core, transaction_handle) = runtime().block_on(async { + let (core, transaction_handle) = runtime().raw().block_on(async { let (manager, wallet_id) = test_platform_wallet_manager().await; let wallet = manager .get_wallet(&wallet_id) @@ -1572,7 +1589,7 @@ mod remove_wallet_lifecycle_tests { fn public_remove_wallet_waits_for_an_in_flight_payment_on_that_generation() { let _registry = crate::core_wallet::signed_payment::registry_test_guard(); - runtime().block_on(async { + runtime().raw().block_on(async { let (manager, wallet_id) = test_platform_wallet_manager().await; let wallet = manager .get_wallet(&wallet_id) @@ -1624,7 +1641,7 @@ mod remove_wallet_lifecycle_tests { fn an_in_flight_payment_does_not_block_an_unrelated_wallets_teardown() { let _registry = crate::core_wallet::signed_payment::registry_test_guard(); - runtime().block_on(async { + runtime().raw().block_on(async { let (manager_a, wallet_id_a) = test_platform_wallet_manager().await; let (manager_b, wallet_id_b) = test_platform_wallet_manager().await; diff --git a/packages/rs-platform-wallet-ffi/src/panic_guard.rs b/packages/rs-platform-wallet-ffi/src/panic_guard.rs new file mode 100644 index 00000000000..91d3d8b0a9a --- /dev/null +++ b/packages/rs-platform-wallet-ffi/src/panic_guard.rs @@ -0,0 +1,528 @@ +//! Panic containment for this crate's `extern "C"` boundary. +//! +//! # Why this exists +//! +//! Every entry point here is a `pub unsafe extern "C" fn`, and `extern "C"` +//! is a **non-unwind** ABI. rustc plants the abort shim in the *callee*, so a +//! panic that reaches the boundary calls `abort()` (SIGABRT) from inside this +//! library — it never becomes an unwind that the caller could observe. That +//! is why `rs-unified-sdk-jni`'s `support::guard` (a `catch_unwind` on the +//! Rust side of each JNI export) cannot intercept it: the process is already +//! gone before control would return to the shim. +//! +//! The workspace states the requirement explicitly for the Android profiles +//! (`Cargo.toml`, `[profile.release-android]`): *"a JNI library must never +//! abort the app process — panics are caught at the shim boundary and +//! rethrown as Java exceptions."* That only holds if a panic can actually +//! *reach* the shim, so this crate has to stop it one frame earlier, on its +//! own side of the boundary. +//! +//! # The contract +//! +//! A caught panic is converted into the crate's generic error result — +//! [`PlatformWalletFFIResultCode::ErrorWalletOperation`] (code 6) — with a +//! message that starts with [`FFI_PANIC_PREFIX`], carries the panic payload, +//! and names the guarded call site. The panic is also logged at `ERROR` on +//! the `platform_wallet_ffi::panic` target before it is converted, so a +//! swallowed panic still leaves a trace in logcat / the host log. +//! +//! **A panic is never a domain outcome.** It says nothing about whether a +//! transition reached the network, which is why it maps to the generic code +//! rather than to any of the typed broadcast codes: hosts must treat it as +//! "unknown outcome" and reconcile against chain state, exactly as they would +//! for any other `ErrorWalletOperation`. +//! +//! ## The panic rides OUTSIDE the operation's own error type +//! +//! Keeping code 6 and the position-zero marker is only worth something if the +//! message survives to the host *unchanged*. An earlier shape folded the +//! panic into whatever error type the guarded future already returned, and +//! that lost the contract at three different call sites: `From>` re-codes to `ErrorUnknown` (99) and prefixes `unclassified error: +//! `; `core_wallet::transaction_builder` prefixes `add_inputs_from_outpoints +//! failed: `; the catch-all `PlatformWalletError` handlers in `shielded_send` +//! prepend operation context (dashpay/platform#4424 review). +//! +//! So the carrier is an **FFI-local outer result** — [`FfiOutcome`], whose +//! error half is [`FfiBoundaryError`] — that sits *around* the domain result +//! rather than inside it. `From for PlatformWalletFFIResult` +//! is the only conversion it has, it is defined in this crate, and it copies +//! the already-composed message verbatim. Every entry point therefore +//! intercepts the boundary failure **before** any legacy `From` +//! mapping can re-code or re-prefix it. +//! +//! That is also why nothing here reaches into `platform-wallet`'s public +//! `PlatformWalletError`: adding a variant to a non-`#[non_exhaustive]` public +//! enum for a boundary-only failure is a source-breaking change to a +//! lower-layer domain API, and this containment work is not supposed to break +//! anything (same review). +//! +//! # iOS +//! +//! `[profile.release-ios]` and `[profile.dev-ios]` deliberately set +//! `panic = "abort"`. Under those profiles [`std::panic::catch_unwind`] still +//! compiles — it simply never observes an `Err`, because the panic aborts +//! before unwinding starts. Nothing in this module is `cfg`-gated on the +//! panic strategy: the same source builds under both, and the iOS carve-out +//! keeps its documented behavior. + +use std::any::Any; +use std::fmt; +use std::panic::{catch_unwind, AssertUnwindSafe, Location}; + +use crate::error::{PlatformWalletFFIResult, PlatformWalletFFIResultCode}; + +/// Machine-recognizable marker at position 0 of every FFI message this +/// module synthesizes from a caught panic. +/// +/// Hosts (and log greps) can use it to tell an internal panic apart from an +/// ordinary generic wallet-operation failure, both of which arrive as code 6. +pub(crate) const FFI_PANIC_PREFIX: &str = "Internal panic caught at the FFI boundary: "; + +/// Machine-recognizable marker at position 0 of the *other* boundary failure: +/// the guarded work never ran because this crate could not obtain somewhere to +/// run it — a tokio runtime whose driver init returned an `io::Error`, or an +/// OS that refused the 8 MB worker thread. +/// +/// Distinct from [`FFI_PANIC_PREFIX`] because the two mean different things to +/// a host: a panic is an internal invariant break with an unknown outcome, a +/// missing execution context means the work provably never started. Both are +/// code 6 — neither is a domain outcome. +pub(crate) const FFI_RUNTIME_UNAVAILABLE_PREFIX: &str = "FFI execution context unavailable: "; + +/// Render a `catch_unwind` / `JoinError` payload as a human-readable string. +/// +/// `panic!("literal")` payloads are `&'static str`; formatted payloads are +/// `String`. Anything else (a custom `panic_any`) has no textual form, so it +/// is reported by type rather than dropped silently. +pub(crate) fn panic_payload_message(payload: &(dyn Any + Send)) -> String { + if let Some(s) = payload.downcast_ref::<&'static str>() { + (*s).to_string() + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + "".to_string() + } +} + +/// Make `detail` safe to carry through a `CString`. +/// +/// A Rust panic payload may contain interior NUL bytes (`panic!("{}", s)` over +/// attacker- or network-supplied text, a `Debug` rendering of raw bytes). +/// `CString::new` rejects those, and [`PlatformWalletFFIResult::err`]'s +/// fallback then replaces the **whole** message with `` — +/// discarding the marker, the payload and the call site, i.e. exactly the +/// contract this module exists to publish (dashpay/platform#4424 review). +/// Escaping keeps the message printable and keeps the marker at position 0. +fn escape_nuls(detail: &str) -> String { + detail.replace('\0', "\\0") +} + +/// Compose the FFI-visible message for a caught panic: the marker prefix, the +/// guarded call site, and the panic payload. +pub(crate) fn ffi_panic_message(location: &Location<'_>, detail: &str) -> String { + let detail = escape_nuls(detail); + format!( + "{FFI_PANIC_PREFIX}{detail} (guarded call site: {}:{})", + location.file(), + location.line() + ) +} + +/// The error half of [`FfiOutcome`]: the guarded work produced no value +/// because **the boundary itself** failed, not the operation. +/// +/// Holds the fully composed, FFI-visible message with its marker already at +/// position 0 ([`FFI_PANIC_PREFIX`] or [`FFI_RUNTIME_UNAVAILABLE_PREFIX`]). +/// Deliberately crate-private and deliberately convertible to exactly one +/// thing — [`PlatformWalletFFIResult`], verbatim, at code 6. It has no +/// conversion into any domain error type, which is what structurally prevents +/// the message from being re-coded or re-prefixed on its way out. +#[derive(Debug, Clone)] +pub(crate) struct FfiBoundaryError(String); + +impl FfiBoundaryError { + /// Wrap an already-composed panic message (from [`ffi_panic_message`] via + /// [`report_panic`]), whose marker is already at position 0. + pub(crate) fn caught_panic(message: String) -> Self { + Self(message) + } + + /// The guarded work never started: no runtime, or no thread to run it on. + pub(crate) fn runtime_unavailable(detail: &str) -> Self { + Self(format!( + "{FFI_RUNTIME_UNAVAILABLE_PREFIX}{}", + escape_nuls(detail) + )) + } +} + +impl fmt::Display for FfiBoundaryError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +impl std::error::Error for FfiBoundaryError {} + +impl From for PlatformWalletFFIResult { + /// The one conversion, and it is direct: code 6 with the message copied + /// verbatim, so [`FFI_PANIC_PREFIX`] / [`FFI_RUNTIME_UNAVAILABLE_PREFIX`] + /// stays at position 0 for the host. + fn from(error: FfiBoundaryError) -> Self { + PlatformWalletFFIResult::err(PlatformWalletFFIResultCode::ErrorWalletOperation, error.0) + } +} + +/// The FFI-local **outer** result: "the guarded work produced `T`" versus +/// "the boundary failed and there is no `T`". +/// +/// Wrapping rather than folding is the whole point — see the module docs. A +/// `T` that is itself a `Result<_, DomainError>` keeps its own error channel +/// untouched, and the panic travels beside it where no domain-error mapping +/// can reach it. +#[derive(Debug)] +#[must_use] +pub(crate) enum FfiOutcome { + /// The guarded work ran to completion and produced `T`. + Ok(T), + /// The guarded work panicked. The `String` is the composed FFI-visible + /// message, marker at position 0. + Panicked(String), +} + +impl FfiOutcome { + /// Project into the `Result` the guarded entry helpers hand out. + pub(crate) fn into_result(self) -> Result { + match self { + Self::Ok(value) => Ok(value), + Self::Panicked(message) => Err(FfiBoundaryError::caught_panic(message)), + } + } +} + +/// The error channel of a guarded call whose work already had one of its own. +/// +/// This is [`FfiOutcome`] flattened into the operation's own `Result`, and it +/// is the shape that makes the contract hard to lose: the boundary failure and +/// the domain failure are **separate variants**, so no call site can format, +/// re-code or re-prefix one while believing it is handling the other. The +/// compiler forces every hand-written error arm to say which it means. +/// +/// The [`From`] impl below is the interception point the review asked for: it +/// answers [`Self::Boundary`] itself, verbatim, and only delegates to the +/// legacy `From` mapping for [`Self::Domain`]. +/// +/// **Deliberately not [`Display`](std::fmt::Display).** Interpolating an error +/// into a context string (`format!("{operation} failed: {e}")`) is precisely +/// how the position-zero marker got lost, so the type refuses to be +/// interpolated at all: a site that wants to add context must first say which +/// failure it is talking about, via [`peel_boundary`] or an explicit match. +/// That turns the whole class of regression the review found into a compile +/// error rather than something a future reader has to notice. +#[derive(Debug)] +pub(crate) enum GuardedError { + /// The boundary failed: a caught panic, or no execution context. Says + /// **nothing** about whether the operation reached the network. + Boundary(FfiBoundaryError), + /// The operation's own error, untouched and still typed. + Domain(E), +} + +impl From> for PlatformWalletFFIResult +where + PlatformWalletFFIResult: From, +{ + fn from(error: GuardedError) -> Self { + match error { + // Intercepted FIRST, and answered here: code 6 with the message + // verbatim, so the marker stays at position 0. The domain mapping + // below never sees it and therefore cannot re-code it (as + // `From>` did, to `ErrorUnknown` behind an + // `unclassified error: ` prefix) — dashpay/platform#4424 review. + GuardedError::Boundary(error) => error.into(), + GuardedError::Domain(error) => error.into(), + } + } +} + +/// Peel the boundary failure off a guarded result, so a hand-written match can +/// classify domain errors without ever seeing one. +/// +/// `Ok` carries the operation's own `Result` untouched, for the caller's domain +/// arms. `Err` is the **finished** FFI result for a boundary failure — code 6, +/// message verbatim, marker at position 0 — which the caller returns +/// immediately, conventionally via `unwrap_result_or_return!`: +/// +/// ```ignore +/// let result = unwrap_result_or_return!(peel_boundary(result)); +/// match result { +/// Err(PlatformWalletError::ShieldedSpendUnconfirmed { .. }) => /* typed arm */, +/// Err(e) => /* free to add operation context: `e` cannot be a panic */, +/// } +/// ``` +/// +/// This is what lets arms like `format!("{operation} failed: {e}")` keep +/// existing: by the time they run, the panic has already left through its own +/// door with its marker intact (dashpay/platform#4424 review). +pub(crate) fn peel_boundary( + result: Result>, +) -> Result, PlatformWalletFFIResult> { + match result { + Ok(value) => Ok(Ok(value)), + Err(GuardedError::Domain(error)) => Ok(Err(error)), + Err(GuardedError::Boundary(error)) => Err(error.into()), + } +} + +/// How a guarded helper reshapes its future's output so a boundary failure has +/// somewhere honest to go. +/// +/// Three shapes, three answers: +/// +/// * `Result` gains the [`GuardedError`] wrapper described above — the +/// domain error keeps its own channel, the boundary failure gets its own. +/// * [`PlatformWalletFFIResult`] absorbs it directly: it is already the FFI +/// error channel, and `err(ErrorWalletOperation, message)` is lossless. +/// * `()` swallows it, with the guard's `ERROR` log as the whole record. +/// +/// Deliberately NOT implemented for bare value types (`u64`, `Vec<_>`, a +/// balance struct, …): fabricating a plausible-looking zero balance or empty +/// peer list out of a panic would convert a crash into silent data corruption, +/// which is worse than the abort this module exists to prevent. Such a call +/// site fails to compile against [`crate::runtime::RuntimeHandle::block_on`] / +/// [`crate::runtime::block_on_worker`], which is the intended signal to move it +/// to the `try_` sibling and handle the outer `Err`. +pub(crate) trait GuardedOutput { + /// What the guarded helper returns in place of `Self`. + type Guarded; + + /// The work completed; hand its value back in the guarded shape. + fn into_guarded(self) -> Self::Guarded; + + /// The work did not complete because the boundary failed. `error` is + /// already composed and already logged. + fn from_boundary_error(error: FfiBoundaryError) -> Self::Guarded; +} + +impl GuardedOutput for () { + type Guarded = (); + + fn into_guarded(self) -> Self::Guarded {} + + /// The last-resort fallback, for fire-and-forget work whose future + /// genuinely yields nothing (`abandon_transaction`, `release`, a stop + /// signal). There is no value to carry a failure in, so the guard's + /// `ERROR` log — payload plus guarded call site — is the whole record, + /// and the entry point goes on to report whatever it was going to report. + /// + /// That makes a swallowed panic look like success to the host, so it is + /// **only** acceptable where the call is already best-effort cleanup on a + /// path that returns an error for its own reasons. Anything whose success + /// the host acts on must use [`crate::runtime::RuntimeHandle::try_block_on`] + /// (or [`crate::runtime::try_block_on_worker`]) and report the `Err`. + fn from_boundary_error(_error: FfiBoundaryError) -> Self::Guarded {} +} + +impl GuardedOutput for PlatformWalletFFIResult { + type Guarded = Self; + + fn into_guarded(self) -> Self::Guarded { + self + } + + fn from_boundary_error(error: FfiBoundaryError) -> Self::Guarded { + error.into() + } +} + +impl GuardedOutput for Result { + type Guarded = Result>; + + fn into_guarded(self) -> Self::Guarded { + self.map_err(GuardedError::Domain) + } + + fn from_boundary_error(error: FfiBoundaryError) -> Self::Guarded { + Err(GuardedError::Boundary(error)) + } +} + +/// Run `f`, capturing a panic as a value instead of letting it unwind into the +/// `extern "C"` abort shim. +/// +/// `#[track_caller]` so the reported location is the *guarded call site* +/// (the entry point's `block_on`, or the `guard_ffi` wrapping an entry-point +/// body) rather than a line inside this module. +/// +/// `AssertUnwindSafe` is required and is sound here for the same reason it is +/// in the JNI shim: the guarded work owns FFI-local state, and every value +/// that outlives the guard is either dropped during the unwind or is a handle +/// whose interior locks (`parking_lot`) do not poison. A panic mid-mutation +/// can still leave wallet state stale, so the guard reports the failure and +/// leaves recovery to the host rather than pretending the work succeeded. +#[track_caller] +pub(crate) fn guard_ffi(f: impl FnOnce() -> T) -> FfiOutcome { + let location = Location::caller(); + match catch_unwind(AssertUnwindSafe(f)) { + Ok(value) => FfiOutcome::Ok(value), + Err(payload) => FfiOutcome::Panicked(report_panic( + location, + &panic_payload_message(payload.as_ref()), + )), + } +} + +/// Log a caught panic and compose its FFI-visible message. +/// +/// Shared by [`guard_ffi`] and by [`crate::runtime::block_on_worker`]'s +/// `JoinError` arm (where tokio caught the panic for us and hands back the +/// payload rather than an unwind). +#[must_use] +pub(crate) fn report_panic(location: &Location<'_>, detail: &str) -> String { + let message = ffi_panic_message(location, detail); + tracing::error!( + target: "platform_wallet_ffi::panic", + panic = %detail, + call_site = %format_args!("{}:{}", location.file(), location.line()), + "caught panic below an FFI entry point; returning an error result instead of aborting" + ); + message +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Read an FFI result's message back as a Rust `String`. + fn message_of(result: &PlatformWalletFFIResult) -> String { + assert!( + !result.message.is_null(), + "error result must carry a message" + ); + unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_str() + .expect("message is UTF-8") + .to_string() + } + + #[test] + fn payload_message_reads_str_and_string_panics() { + let str_payload = catch_unwind(|| panic!("literal boom")).unwrap_err(); + assert_eq!(panic_payload_message(str_payload.as_ref()), "literal boom"); + + let value = 7; + let string_payload = catch_unwind(|| panic!("formatted boom {value}")).unwrap_err(); + assert_eq!( + panic_payload_message(string_payload.as_ref()), + "formatted boom 7" + ); + + let other_payload = catch_unwind(|| std::panic::panic_any(7u8)).unwrap_err(); + assert_eq!( + panic_payload_message(other_payload.as_ref()), + "" + ); + } + + #[test] + fn guard_returns_generic_error_result_with_payload() { + let outcome: FfiOutcome<()> = guard_ffi(|| panic!("guarded boom")); + let result: PlatformWalletFFIResult = outcome + .into_result() + .expect_err("the guarded work panicked") + .into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation + ); + let message = message_of(&result); + assert!( + message.starts_with(FFI_PANIC_PREFIX), + "message must be recognizable as a panic: {message}" + ); + assert!( + message.contains("guarded boom"), + "message must carry the panic payload: {message}" + ); + assert!( + message.contains("panic_guard.rs"), + "message must name the guarded call site: {message}" + ); + } + + #[test] + fn guard_passes_values_through_untouched() { + let ok: Result = guard_ffi(|| 42).into_result(); + assert_eq!(ok.expect("no panic"), 42); + } + + /// A NUL byte in the payload must not cost the host the whole message. + /// + /// `CString::new` rejects interior NULs, and `PlatformWalletFFIResult::err` + /// answers that by substituting `` for the *entire* string — + /// so without escaping, a panic whose text happened to contain a NUL would + /// arrive with no marker, no payload and no call site. + #[test] + fn nul_bytes_in_a_payload_keep_the_marker_and_the_payload() { + let outcome: FfiOutcome<()> = guard_ffi(|| panic!("boom\0with\0nuls")); + let result: PlatformWalletFFIResult = outcome + .into_result() + .expect_err("the guarded work panicked") + .into(); + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation + ); + let message = message_of(&result); + assert!( + message.starts_with(FFI_PANIC_PREFIX), + "the marker must survive a NUL-bearing payload: {message}" + ); + assert!( + message.contains("boom\\0with\\0nuls"), + "the payload must survive, with its NULs escaped: {message}" + ); + assert!( + message.contains("panic_guard.rs"), + "the call site must survive: {message}" + ); + assert!( + !message.contains(""), + "the message must not have collapsed to the CString fallback: {message}" + ); + } + + /// The same escaping on the other marker's constructor. + #[test] + fn runtime_unavailable_escapes_nuls_and_keeps_its_marker() { + let result: PlatformWalletFFIResult = + FfiBoundaryError::runtime_unavailable("driver\0failed").into(); + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation + ); + let message = message_of(&result); + assert!( + message.starts_with(FFI_RUNTIME_UNAVAILABLE_PREFIX), + "{message}" + ); + assert!(message.contains("driver\\0failed"), "{message}"); + } + + /// The boundary error has exactly one conversion, and it is verbatim: the + /// marker stays at position 0 and the code stays generic. + #[test] + fn boundary_error_converts_verbatim_at_the_generic_code() { + let error = FfiBoundaryError::caught_panic(format!("{FFI_PANIC_PREFIX}payload")); + let result: PlatformWalletFFIResult = error.into(); + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation + ); + assert_eq!(message_of(&result), format!("{FFI_PANIC_PREFIX}payload")); + } +} diff --git a/packages/rs-platform-wallet-ffi/src/platform_address_sync.rs b/packages/rs-platform-wallet-ffi/src/platform_address_sync.rs index dc9893a8869..338ef75a7b3 100644 --- a/packages/rs-platform-wallet-ffi/src/platform_address_sync.rs +++ b/packages/rs-platform-wallet-ffi/src/platform_address_sync.rs @@ -9,7 +9,7 @@ use crate::error::*; use crate::handle::*; use crate::platform_address_types::AddressSyncConfigFFI; use crate::runtime::{run_on_big_stack_thread, runtime}; -use crate::{check_ptr, unwrap_option_or_return}; +use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; /// Flattened sync metrics for one wallet result in a platform-address sync pass. #[repr(C)] @@ -82,10 +82,15 @@ pub unsafe extern "C" fn platform_wallet_manager_platform_address_sync_start( handle: Handle, ) -> PlatformWalletFFIResult { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - let _entered = runtime().enter(); + // The loop's `tokio::spawn` needs a runtime in scope, so acquisition + // is fallible here: with no runtime there is nothing to start the + // platform-address loop on, and that has to be reported rather than + // silently skipped. + let _entered = runtime().checked()?.enter(); manager.platform_address_sync_arc().start(); + Ok::<(), crate::panic_guard::FfiBoundaryError>(()) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -199,16 +204,23 @@ pub unsafe extern "C" fn platform_wallet_manager_platform_address_sync_sync_now( // limitation (rust-lang/rust issue #100013) against // `block_on_worker`'s `Send + 'static` bounds. run_on_big_stack_thread(|| { - runtime().block_on(manager.platform_address_sync().sync_now()); + // Guarded on the big-stack thread's own side: a panic in this pass + // is converted by `run_on_big_stack_thread`'s join, and a runtime + // that will not build is reported by the inner `block_on`. + // `try_block_on`, not `block_on`: `PlatformWalletFFIResult` owns a + // raw `*mut c_char` and so is not `Send`, and this value has to + // cross back off the big-stack thread. `FfiBoundaryError` is. + runtime().try_block_on(manager.platform_address_sync().sync_now()) }) }); - let spawn_result = unwrap_option_or_return!(option); - if let Err(e) = spawn_result { - return PlatformWalletFFIResult::err( - PlatformWalletFFIResultCode::ErrorWalletOperation, - format!("failed to spawn big-stack thread for address sync: {e}"), - ); - } + let pass_result = unwrap_option_or_return!(option); + // Two boundary-only layers: the outer is the thread (refused spawn / caught + // panic), the inner is runtime acquisition. Both are returned verbatim — + // their messages already carry the matching marker at position 0. The old + // `format!("failed to spawn big-stack thread for address sync: {e}")` here + // pushed that marker off position 0 on the panic path, which is exactly the + // classification hosts are told to key on (dashpay/platform#4424 review). + unwrap_result_or_return!(unwrap_result_or_return!(pass_result)); PlatformWalletFFIResult::ok() } @@ -229,6 +241,10 @@ pub unsafe extern "C" fn platform_wallet_manager_platform_address_sync_reset( runtime().block_on(manager.reset_platform_address_sync_state()) }); let result = unwrap_option_or_return!(option); + // Peel the FFI-local outer failure off first: the generic arm below adds + // `reset_platform_address_sync_state failed: ` context, which would push a + // caught panic's marker off position 0. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); if let Err(e) = result { // Mirrors `platform_wallet_manager_shielded_clear`: an incomplete // drain is surfaced with its own code so the host can distinguish diff --git a/packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs b/packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs index d42a6dfb6a6..f116010cf4f 100644 --- a/packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs +++ b/packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rs @@ -48,13 +48,13 @@ pub unsafe extern "C" fn platform_address_wallet_restore_sync_state( last_known_recent_block: u64, ) -> PlatformWalletFFIResult { let option = PLATFORM_ADDRESS_WALLET_STORAGE.with_item(handle, |wallet| { - runtime().block_on(wallet.restore_sync_state( + runtime().try_block_on(wallet.restore_sync_state( sync_height, sync_timestamp, last_known_recent_block, - )); + )) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -70,9 +70,10 @@ pub unsafe extern "C" fn platform_address_wallet_total_credits( ) -> PlatformWalletFFIResult { check_ptr!(out_credits); - let option = PLATFORM_ADDRESS_WALLET_STORAGE - .with_item(handle, |wallet| runtime().block_on(wallet.total_credits())); - *out_credits = unwrap_option_or_return!(option); + let option = PLATFORM_ADDRESS_WALLET_STORAGE.with_item(handle, |wallet| { + runtime().try_block_on(wallet.total_credits()) + }); + *out_credits = unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -136,20 +137,22 @@ pub unsafe extern "C" fn platform_address_wallet_addresses_with_balances( check_ptr!(out_count); let option = PLATFORM_ADDRESS_WALLET_STORAGE.with_item(handle, |wallet| { - let balances = runtime().block_on(wallet.addresses_with_balances()); - balances - .into_iter() - .map(|(address, balance)| AddressBalanceEntryFFI { - address: address.into(), - balance, - nonce: 0, - account_index: 0, - address_index: 0, - as_of_height: 0, - }) - .collect::>() + let balances = runtime().try_block_on(wallet.addresses_with_balances())?; + Ok::<_, crate::panic_guard::FfiBoundaryError>( + balances + .into_iter() + .map(|(address, balance)| AddressBalanceEntryFFI { + address: address.into(), + balance, + nonce: 0, + account_index: 0, + address_index: 0, + as_of_height: 0, + }) + .collect::>(), + ) }); - let entries = unwrap_option_or_return!(option); + let entries = unwrap_result_or_return!(unwrap_option_or_return!(option)); *out_count = entries.len(); if entries.is_empty() { *out_entries = std::ptr::null_mut(); diff --git a/packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs b/packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs index cd9fe15f2a2..49ae37ebc3a 100644 --- a/packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs +++ b/packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs @@ -330,6 +330,10 @@ pub unsafe extern "C" fn platform_address_wallet_preflight_withdrawal( let option = PLATFORM_ADDRESS_WALLET_STORAGE.with_item(handle, |wallet| wallet.clone()); let wallet = unwrap_option_or_return!(option); let result = block_on_worker(async move { wallet.preflight_withdrawal(account_index).await }); + // Peel the FFI-local outer failure off first: `classify_preflight_error` + // reasons about typed domain errors and would otherwise be handed a panic + // to classify as retryable / not-fundable. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); match result { Ok(plan) => { diff --git a/packages/rs-platform-wallet-ffi/src/runtime.rs b/packages/rs-platform-wallet-ffi/src/runtime.rs index ee96010db00..6fd7abea036 100644 --- a/packages/rs-platform-wallet-ffi/src/runtime.rs +++ b/packages/rs-platform-wallet-ffi/src/runtime.rs @@ -17,32 +17,216 @@ //! (small-stacked) calling thread. `block_on` itself still runs //! on the caller, but it parks almost immediately — all the //! compute happens on the tokio worker. +//! +//! ## Panic containment +//! +//! This module is also the crate's execution choke point, and therefore +//! where panic containment is cheapest: [`block_on_worker`], +//! [`RuntimeHandle::block_on`] and [`run_on_big_stack_thread`] between them run +//! the async body of nearly every `extern "C"` entry point. A panic that +//! escapes any of them reaches a non-unwind ABI boundary and aborts the host +//! process — see [`crate::panic_guard`] for the mechanics and for why the +//! JNI shim's own `catch_unwind` cannot save it. All three convert a panic +//! into an error value instead. +//! +//! ## Acquiring the runtime is itself fallible +//! +//! Building the runtime can fail *before* any of those guards exist. Tokio's +//! `Builder::build` reports driver-init failure as an `io::Error`, and its +//! multi-thread worker launch can panic outright when the OS refuses a thread. +//! While `runtime()` handed out a `Lazy` that `expect`ed the build, the very +//! first async FFI call on a constrained device evaluated that `Lazy` *outside* +//! any guard, and the resulting panic went straight into the caller's +//! `extern "C"` abort shim (dashpay/platform#4424 review). +//! +//! So acquisition is fallible now, and the one-time construction happens +//! *inside* [`crate::panic_guard::guard_ffi`]: [`runtime_checked`] returns +//! `Result<&'static FfiRuntime, FfiBoundaryError>`, and the ergonomic +//! [`runtime()`] handle is zero-sized — it holds no runtime, so merely calling +//! it can neither build nor panic. Every method on it acquires through +//! `runtime_checked` and folds a failure into the entry point's own error +//! channel, which is what makes "all entry paths route through the fallible +//! acquisition" true by construction rather than by review. + +use std::panic::Location; + +use crate::panic_guard::{ + guard_ffi, panic_payload_message, report_panic, FfiBoundaryError, FfiOutcome, GuardedOutput, +}; /// Worker thread stack size for the shared runtime. 8 MB gives proof /// verification + GroveDB comfortable headroom without meaningfully /// affecting memory footprint (we spin up a small number of workers). const WORKER_STACK_BYTES: usize = 8 * 1024 * 1024; -/// Get the shared tokio runtime. +/// The shared runtime, once it exists. /// -/// All async FFI functions use this runtime. Prefer -/// [`block_on_worker`] over `runtime().block_on(...)` so the heavy -/// work runs on a worker thread with the larger stack configured -/// here, rather than the (small) calling thread. -pub(crate) fn runtime() -> &'static tokio::runtime::Runtime { - static RT: once_cell::sync::Lazy = once_cell::sync::Lazy::new(|| { - let rt = tokio::runtime::Builder::new_multi_thread() - .enable_all() - .thread_stack_size(WORKER_STACK_BYTES) - .build() - .expect("Failed to create tokio runtime for platform-wallet-ffi"); +/// Reachable only through [`runtime_checked`] (or the [`RuntimeHandle`] methods +/// built on it), so there is no way to touch it without having handled the +/// possibility that it could not be built. +pub(crate) struct FfiRuntime(tokio::runtime::Runtime); + +impl std::ops::Deref for FfiRuntime { + type Target = tokio::runtime::Runtime; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl FfiRuntime { + /// The unwrapped tokio runtime, for call sites that compose their own + /// guarding ([`block_on_worker`], anything already inside + /// [`run_on_big_stack_thread`]) or that need to pass a `&Runtime` on. + pub(crate) fn raw(&self) -> &tokio::runtime::Runtime { + &self.0 + } +} - #[cfg(feature = "tokio-metrics")] - metrics::spawn_sampler(&rt); +/// Construct the runtime. Fallible **by value**: `Builder::build` surfaces a +/// failed driver init as an `io::Error` rather than panicking, so this function +/// contributes no panic of its own. (Tokio's worker launch still can panic; +/// that is what the `guard_ffi` around the call below is for.) +fn build_runtime() -> std::io::Result { + let rt = tokio::runtime::Builder::new_multi_thread() + .enable_all() + .thread_stack_size(WORKER_STACK_BYTES) + .build()?; - rt + #[cfg(feature = "tokio-metrics")] + metrics::spawn_sampler(&rt); + + Ok(FfiRuntime(rt)) +} + +/// One-time construction, performed inside the panic guard. +/// +/// Both failure shapes become a *value* here, so nothing about first-call +/// initialization can unwind into an `extern "C"` frame: +/// +/// * the driver's `io::Error`, which the old `.expect(...)` turned into a +/// panic, and +/// * a panic out of tokio's worker-thread launch, which no `expect` of ours +/// could have caught at all. +/// +/// The `Lazy` closure itself is therefore infallible in the panic sense, which +/// also means it can never poison and re-panic on a later access. +static RT: once_cell::sync::Lazy> = + once_cell::sync::Lazy::new(|| match guard_ffi(build_runtime) { + FfiOutcome::Ok(Ok(runtime)) => Ok(runtime), + FfiOutcome::Ok(Err(error)) => Err(FfiBoundaryError::runtime_unavailable(&format!( + "failed to create the tokio runtime for platform-wallet-ffi: {error}" + ))), + // Already carries the panic marker at position 0 — a panic is a panic + // wherever it happened, and re-labelling it would cost the host the + // one thing the marker is for. + FfiOutcome::Panicked(message) => Err(FfiBoundaryError::caught_panic(message)), }); - &RT + +/// Fallible acquisition of the shared runtime — the only way to reach it. +/// +/// Cheap after the first call (a `Lazy` deref and a clone of nothing on the +/// success path). The `Err` is cloned rather than borrowed so callers can put +/// it in their own `Result` without borrowing from the static. +pub(crate) fn runtime_checked() -> Result<&'static FfiRuntime, FfiBoundaryError> { + RT.as_ref().map_err(Clone::clone) +} + +/// Test-only unguarded accessor, for test bodies that need to drive async +/// setup directly (and for which a failure to build the runtime is simply a +/// failed test, not something to report across an ABI). +#[cfg(test)] +pub(crate) fn test_runtime() -> &'static tokio::runtime::Runtime { + runtime_checked() + .expect("the FFI runtime must build in tests") + .raw() +} + +/// A zero-sized, deferred handle to the shared runtime. +/// +/// Constructing it does not build, touch, or even initialize anything — which +/// is the point: `runtime()` is evaluated at the top of entry points, *outside* +/// any guard, so it must not be able to fail. Each method below acquires the +/// real runtime through [`runtime_checked`] and deals with a failure inside its +/// own error channel. +/// +/// [`Self::block_on`] shadows what used to be a `Deref` to +/// `tokio::runtime::Runtime::block_on`, and keeping that name is deliberate: a +/// future entry point that reaches for `runtime().block_on(...)` gets +/// containment without having to know this module exists, which is the only way +/// the invariant survives across 478 entry points. +#[derive(Clone, Copy, Debug)] +pub(crate) struct RuntimeHandle; + +/// Get a handle to the shared tokio runtime. +/// +/// All async FFI functions use this runtime. Prefer [`block_on_worker`] over +/// `runtime().block_on(...)` so the heavy work runs on a worker thread with the +/// larger stack configured here, rather than the (small) calling thread. +pub(crate) fn runtime() -> RuntimeHandle { + RuntimeHandle +} + +impl RuntimeHandle { + /// Drive `future` to completion on the calling thread, converting a + /// boundary failure into `F::Output`'s lossless error representation + /// instead of letting it unwind into the caller's `extern "C"` abort shim. + /// + /// The output is reshaped by [`GuardedOutput`]: a future that already + /// returns `Result` comes back as `Result>`, so + /// the boundary failure travels in its own variant instead of being folded + /// into (and re-coded by) the domain error. Outputs that cannot carry a + /// failure at all do not implement the trait and must use + /// [`Self::try_block_on`]. + #[track_caller] + pub(crate) fn block_on(self, future: F) -> ::Guarded + where + F: std::future::Future, + F::Output: GuardedOutput, + { + match self.try_block_on(future) { + Ok(value) => value.into_guarded(), + Err(error) => F::Output::from_boundary_error(error), + } + } + + /// [`Self::block_on`] with the boundary failure returned in an **outer** + /// `Result` instead of folded into the output. + /// + /// This is the shape every call site needs whose future already returns a + /// domain `Result`, or whose output cannot represent failure at all — a + /// balance `u64`, a `Vec` of peers, a sync summary, a lock guard. + /// Fabricating a zero balance or an empty peer list out of a panic would + /// turn a crash into silent, plausible-looking wrong data; folding it into + /// a domain error would cost it its code and marker. The outer `Err` does + /// neither. + #[track_caller] + pub(crate) fn try_block_on(self, future: F) -> Result + where + F: std::future::Future, + { + let rt = runtime_checked()?; + guard_ffi(|| rt.raw().block_on(future)).into_result() + } + + /// Fallible access to the runtime itself, for the handful of call sites + /// that need `enter()`, `spawn()` or `spawn_blocking()` rather than a + /// blocking drive. + pub(crate) fn checked(self) -> Result<&'static FfiRuntime, FfiBoundaryError> { + runtime_checked() + } + + /// Test-only shorthand for [`test_runtime`], so test bodies that drive + /// async setup directly keep reading as `runtime().raw().block_on(...)`. + /// + /// Deliberately `#[cfg(test)]`: in a test, a runtime that will not build is + /// a failed test, but in production it is something an entry point has to + /// *report* — which is why the production sibling ([`Self::checked`]) is + /// fallible and this one is not. + #[cfg(test)] + pub(crate) fn raw(self) -> &'static tokio::runtime::Runtime { + test_runtime() + } } /// Drive `future` to completion, moving the actual polling onto a @@ -51,13 +235,97 @@ pub(crate) fn runtime() -> &'static tokio::runtime::Runtime { /// /// The calling thread still blocks (that's what FFI wants); it just /// parks on a oneshot instead of driving the future itself. -pub(crate) fn block_on_worker(future: F) -> F::Output +/// +/// ## Panics are returned, not propagated +/// +/// This is the crate's highest-traffic execution site, so it is also where +/// panic containment pays off most. Two distinct failure shapes are handled: +/// +/// * **The spawned task panicked.** tokio polls the task inside its own +/// `catch_unwind`, so the panic never unwinds through our frames — it +/// arrives as [`tokio::task::JoinError`]. The previous +/// `.expect("tokio worker panicked")` turned that value back into a *live +/// panic on the calling thread*, which then unwound into the entry point's +/// `extern "C"` abort shim and SIGABRTed the process. It is now converted +/// to `F::Output`'s error representation instead. +/// * **The task was cancelled** (runtime shutdown, `abort()`), which the same +/// `.expect` also treated as a panic. It becomes an error result too: the +/// work definitively did not finish, but that is not a reason to kill the +/// host. +/// +/// The outer [`guard_ffi`] additionally covers a panic raised by `block_on` +/// itself (e.g. driving the runtime from inside another runtime), and a runtime +/// that could not be built is reported rather than `expect`ed. +/// +/// `F::Output` is reshaped by [`GuardedOutput`] exactly as in +/// [`RuntimeHandle::block_on`]; see that trait for why bare value types are +/// deliberately excluded. +#[track_caller] +pub(crate) fn block_on_worker(future: F) -> ::Guarded +where + F: std::future::Future + Send + 'static, + F::Output: Send + 'static + GuardedOutput, +{ + match try_block_on_worker(future) { + Ok(value) => value.into_guarded(), + Err(error) => F::Output::from_boundary_error(error), + } +} + +/// [`block_on_worker`] with the boundary failure in an **outer** `Result`. +/// +/// Same rationale as [`RuntimeHandle::try_block_on`]: rather than inventing a +/// value for a `usize` count or a sync summary — or hiding the panic inside a +/// domain error whose FFI mapping would re-code it — the failure is returned as +/// the `Err` half so the entry point turns it into a real error result. +#[track_caller] +pub(crate) fn try_block_on_worker(future: F) -> Result where F: std::future::Future + Send + 'static, F::Output: Send + 'static, { - let rt = runtime(); - rt.block_on(async move { rt.spawn(future).await.expect("tokio worker panicked") }) + let location = Location::caller(); + let rt = runtime_checked()?; + guard_ffi(|| { + // `raw()`: the surrounding `guard_ffi` already covers this frame, and + // the join below reports the worker's panic with better context than + // a second, nested guard could. + rt.raw().block_on(async move { + match rt.raw().spawn(future).await { + Ok(value) => Ok(value), + Err(join_error) => Err(from_join_error(location, join_error)), + } + }) + }) + .into_result() + // Flatten "the guard caught a panic" and "the join reported one" into the + // single outer channel the callers handle. + .and_then(std::convert::identity) +} + +/// Convert a [`tokio::task::JoinError`] into the outer boundary error: the +/// replacement for `.expect("tokio worker panicked")`. +/// +/// Split out of [`try_block_on_worker`] so both `JoinError` shapes — panicked +/// and cancelled — can be exercised directly by tests; producing a *cancelled* +/// join through the full path would need the worker's `JoinHandle`, which that +/// function owns. +fn from_join_error( + location: &'static Location<'static>, + join_error: tokio::task::JoinError, +) -> FfiBoundaryError { + let detail = if join_error.is_panic() { + format!( + "tokio worker task panicked: {}", + panic_payload_message(join_error.into_panic().as_ref()) + ) + } else { + // Cancellation (runtime shutdown, an explicit `abort()`). The work + // definitively did not finish — but that is an error to report, not a + // reason to take the host process down with it. + format!("tokio worker task did not complete: {join_error}") + }; + FfiBoundaryError::caught_panic(report_panic(location, &detail)) } /// Run `f` to completion on a freshly spawned scoped OS thread with the @@ -76,22 +344,279 @@ where /// compiles: it reuses pooled runtime workers instead of paying a /// thread spawn per call. /// -/// A panic inside `f` is propagated as a panic here, matching -/// [`block_on_worker`]'s "tokio worker panicked" convention — a panic -/// in the pass is a bug, not a recoverable condition. -pub(crate) fn run_on_big_stack_thread(f: impl FnOnce() -> T + Send) -> std::io::Result { +/// A panic inside `f` is reported through the SAME channel as a failed spawn, +/// rather than being re-raised on the calling thread. Joining a panicked scoped +/// thread hands back the payload as a value (the unwind was already contained +/// by the thread boundary); re-raising it — which +/// `.expect("big-stack FFI thread panicked")` used to do — turned a contained +/// panic back into a live one on a thread that unwinds straight into an +/// `extern "C"` abort shim. +/// +/// Both shapes come back as [`FfiBoundaryError`], which converts to +/// `ErrorWalletOperation` with its message verbatim. That is what keeps the +/// marker at position 0: while this returned `io::Result`, a call site was free +/// to re-wrap the text (`format!("failed to spawn …: {e}")`) and did, which cost +/// the host the marker on exactly the panic path this exists to report. +#[track_caller] +pub(crate) fn run_on_big_stack_thread( + f: impl FnOnce() -> T + Send, +) -> Result { + let location = Location::caller(); std::thread::scope(|scope| { let handle = std::thread::Builder::new() .name("pw-ffi-bigstack".into()) .stack_size(WORKER_STACK_BYTES) - .spawn_scoped(scope, f)?; - Ok(handle.join().expect("big-stack FFI thread panicked")) + .spawn_scoped(scope, f) + .map_err(|error| { + FfiBoundaryError::runtime_unavailable(&format!( + "failed to spawn the 8 MB FFI worker thread: {error}" + )) + })?; + handle.join().map_err(|payload| { + FfiBoundaryError::caught_panic(report_panic( + location, + &format!( + "big-stack FFI thread panicked: {}", + panic_payload_message(payload.as_ref()) + ), + )) + }) }) } #[cfg(test)] mod tests { use super::*; + use crate::error::{PlatformWalletFFIResult, PlatformWalletFFIResultCode}; + use crate::panic_guard::{FFI_PANIC_PREFIX, FFI_RUNTIME_UNAVAILABLE_PREFIX}; + + /// Read an FFI result's message back as a Rust `String`. + fn message_of(result: &PlatformWalletFFIResult) -> String { + assert!( + !result.message.is_null(), + "error result must carry a message" + ); + unsafe { std::ffi::CStr::from_ptr(result.message) } + .to_str() + .expect("message is UTF-8") + .to_string() + } + + /// A stand-in for the crate's ~478 real entry points, with the shape they + /// all share: `#[no_mangle] pub unsafe extern "C" fn -> PlatformWalletFFIResult` + /// whose body drives an async body through `block_on_worker`. + /// + /// It has to be a genuine `extern "C"` fn for the test to mean anything: + /// rustc plants the non-unwind ABI's abort shim in the **callee**, so this + /// function aborts on an escaping panic no matter who calls it — a plain + /// Rust fn would not reproduce the bug at all. + /// + /// `#[cfg(test)]`, so it never reaches the cdylib's exported surface or + /// the cbindgen header. + #[no_mangle] + unsafe extern "C" fn platform_wallet_ffi_test_panicking_entry_point() -> PlatformWalletFFIResult + { + let outcome: Result<(), FfiBoundaryError> = try_block_on_worker(async move { + // A bounds check on network-supplied data — the panic class the + // audit calls out, and one that fires in every profile (unlike an + // overflow check, which only trips where `overflow-checks` is on). + let payload = [0u8; 4]; + let offset_from_the_wire = std::hint::black_box(7usize); + let _ = payload[offset_from_the_wire]; + }); + + match outcome { + Ok(()) => PlatformWalletFFIResult::ok(), + Err(error) => PlatformWalletFFIResult::from(error), + } + } + + /// A second stand-in, for the entry-point shape whose async body returns a + /// **domain** `Result` that the FFI boundary would ordinarily re-map. + /// + /// `platform_wallet::PlatformWalletError::TransactionBuild` is a stand-in + /// for any of the domain errors whose `From` impl re-codes or re-prefixes; + /// the point of the test below is that a *panic* on this path never reaches + /// that mapping at all. + #[no_mangle] + unsafe extern "C" fn platform_wallet_ffi_test_panicking_domain_entry_point( + ) -> PlatformWalletFFIResult { + // The idiom every real call site of this shape uses: intercept the + // OUTER boundary failure first, then map the domain error. + let domain: Result = + match try_block_on_worker(async move { + let payload = [0u8; 4]; + let offset_from_the_wire = std::hint::black_box(7usize); + Ok(u64::from(payload[offset_from_the_wire])) + }) { + Ok(value) => value, + Err(error) => return error.into(), + }; + + match domain { + Ok(_) => PlatformWalletFFIResult::ok(), + Err(error) => PlatformWalletFFIResult::from(error), + } + } + + /// The headline regression test: a panic raised inside an entry point's + /// async body comes back as a clean error result. + /// + /// **The test process surviving IS half the assertion.** Before this + /// change the panic was re-raised on the calling thread by + /// `.expect("tokio worker panicked")` and unwound into the `extern "C"` + /// shim above, which aborts — the test binary would die with SIGABRT and + /// no assertion below would ever run. + #[test] + fn panicking_entry_point_returns_an_error_result_instead_of_aborting() { + let result = unsafe { platform_wallet_ffi_test_panicking_entry_point() }; + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation, + "a caught panic must arrive as the generic wallet-operation code, \ + never as a code that carries retry/outcome semantics" + ); + let message = message_of(&result); + assert!( + message.starts_with(FFI_PANIC_PREFIX), + "message must be recognizable as an internal panic: {message}" + ); + assert!( + message.contains("tokio worker task panicked"), + "message must say the worker task panicked: {message}" + ); + assert!( + message.contains("index out of bounds"), + "message must carry the panic payload: {message}" + ); + } + + /// The contract the review asked to be pinned on an **exported path**: a + /// panic under an entry point whose async body returns a domain `Result` + /// still arrives as code 6 with the marker at *exactly* position 0. + /// + /// This is the regression that the outer-result carrier exists for. With + /// the panic folded into the domain error instead, this same entry point + /// would answer with whatever that error's `From` impl decided — for + /// `Box` that is `ErrorUnknown` (99) behind an `unclassified + /// error: ` prefix, which is neither the code nor the position hosts are + /// told to key on. + #[test] + fn exported_domain_result_entry_point_pins_code_six_and_prefix_at_position_zero() { + let result = unsafe { platform_wallet_ffi_test_panicking_domain_entry_point() }; + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation, + "code 6 is the documented panic code" + ); + let message = message_of(&result); + assert_eq!( + message.find(FFI_PANIC_PREFIX), + Some(0), + "the marker must be at position 0, not merely present: {message}" + ); + assert!( + message.contains("index out of bounds"), + "message must carry the panic payload: {message}" + ); + } + + /// `try_block_on_worker` covers the outputs that cannot represent failure + /// themselves (a bare `u64` here), so the panic has to ride the `Err` half. + #[test] + fn try_block_on_worker_surfaces_a_panic_as_err() { + let result = try_block_on_worker(async move { panic!("counting boom") }); + + let error = result.expect_err("a panicking worker must not report success"); + assert!(error.to_string().starts_with(FFI_PANIC_PREFIX)); + assert!(error.to_string().contains("counting boom")); + // The success path still yields the bare value untouched. + assert_eq!( + try_block_on_worker(async move { 7u64 }).expect("no panic"), + 7 + ); + } + + /// `RuntimeHandle::block_on` keeps the name tokio's had, so the many + /// `runtime().block_on(...)` entry points are guarded without being + /// rewritten. + #[test] + fn runtime_block_on_is_guarded_and_passes_values_through() { + let ok = runtime().block_on(async { PlatformWalletFFIResult::ok() }); + assert_eq!(ok.code, PlatformWalletFFIResultCode::Success); + + let result = runtime().block_on(async { + panic!("local boom"); + #[allow(unreachable_code)] + PlatformWalletFFIResult::ok() + }); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation + ); + assert!(message_of(&result).contains("local boom")); + + let value: Result = runtime().try_block_on(async { 9u64 }); + assert_eq!(value.expect("no panic"), 9); + } + + /// Both `JoinError` shapes, against errors produced by tokio itself. + /// + /// The cancelled shape is why this mapping exists as its own function: the + /// old `.expect("tokio worker panicked")` re-panicked on a *cancelled* + /// worker too — mislabelling it, and aborting the host over work that + /// merely did not finish. + #[test] + fn join_error_shapes_both_become_error_values() { + let location = Location::caller(); + + let panicked: tokio::task::JoinError = test_runtime().block_on(async { + tokio::spawn(async { panic!("joined boom") }) + .await + .expect_err("the task panicked") + }); + assert!(panicked.is_panic()); + let result: PlatformWalletFFIResult = from_join_error(location, panicked).into(); + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation + ); + let message = message_of(&result); + assert!(message.starts_with(FFI_PANIC_PREFIX)); + assert!(message.contains("joined boom"), "{message}"); + + let cancelled: tokio::task::JoinError = test_runtime().block_on(async { + let handle = tokio::spawn(std::future::pending::<()>()); + handle.abort(); + handle.await.expect_err("the task was aborted") + }); + assert!(cancelled.is_cancelled()); + let error = from_join_error(location, cancelled); + assert!( + error.to_string().contains("did not complete"), + "a cancelled worker must not be reported as a panic: {error}" + ); + } + + /// A panic on the big-stack path is reported through the boundary error the + /// call sites already handle, rather than re-raised into the abort shim. + #[test] + fn run_on_big_stack_thread_reports_a_panic_as_a_boundary_error() { + let result = run_on_big_stack_thread(|| panic!("big-stack boom")); + + let error = result.expect_err("a panicking pass must not report success"); + let rendered = error.to_string(); + assert!(rendered.starts_with(FFI_PANIC_PREFIX), "{rendered}"); + assert!(rendered.contains("big-stack boom"), "{rendered}"); + + // …and it keeps code 6 with the marker still at position 0 once it + // reaches the host, because the only conversion it has is verbatim. + let ffi: PlatformWalletFFIResult = error.into(); + assert_eq!(ffi.code, PlatformWalletFFIResultCode::ErrorWalletOperation); + assert_eq!(message_of(&ffi).find(FFI_PANIC_PREFIX), Some(0)); + } #[test] fn run_on_big_stack_thread_round_trips_return_value() { @@ -122,6 +647,41 @@ mod tests { let out = run_on_big_stack_thread(|| recurse(1_000)).expect("spawn should succeed"); assert!(out > 0); } + + /// The runtime builds, and acquisition is the fallible call every entry + /// path now goes through. + #[test] + fn runtime_acquisition_is_fallible_and_succeeds_here() { + let runtime = runtime_checked().expect("the runtime must build on a test host"); + assert!(runtime.raw().metrics().num_workers() > 0); + } + + /// A runtime that could not be built reaches the host as the generic code + /// with its own marker at position 0 — the same shape as a caught panic, + /// and never an abort. + /// + /// The failure itself cannot be provoked on a healthy test host (that is + /// the point of the `Lazy`), so this pins the conversion the `Err` arm of + /// [`runtime_checked`] feeds. + #[test] + fn an_unavailable_runtime_is_a_generic_error_result_not_an_abort() { + let error = FfiBoundaryError::runtime_unavailable( + "failed to create the tokio runtime for platform-wallet-ffi: too many open files", + ); + let result: PlatformWalletFFIResult = error.into(); + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation + ); + let message = message_of(&result); + assert_eq!( + message.find(FFI_RUNTIME_UNAVAILABLE_PREFIX), + Some(0), + "hosts key on the marker at position 0: {message}" + ); + assert!(message.contains("too many open files"), "{message}"); + } } #[cfg(feature = "tokio-metrics")] diff --git a/packages/rs-platform-wallet-ffi/src/shielded_send.rs b/packages/rs-platform-wallet-ffi/src/shielded_send.rs index 21d98fac4db..f13e0b6c46d 100644 --- a/packages/rs-platform-wallet-ffi/src/shielded_send.rs +++ b/packages/rs-platform-wallet-ffi/src/shielded_send.rs @@ -61,6 +61,7 @@ use crate::handle::*; use crate::identity_registration_with_signer::{decode_identity_pubkeys, IdentityPubkeyFFI}; use crate::runtime::{block_on_worker, runtime}; use crate::shielded_types::ShieldedShieldPreflightFFI; +use crate::unwrap_result_or_return; /// A serialized `PlatformAddress` is exactly 21 bytes (1-byte variant tag + 20-byte hash). const PLATFORM_ADDRESS_LEN: usize = 21; @@ -142,7 +143,15 @@ unsafe fn parse_required_platform_address( /// `OnceLock`. #[no_mangle] pub unsafe extern "C" fn platform_wallet_shielded_warm_up_prover() { - runtime().spawn_blocking(|| CachedOrchardProver::new().warm_up()); + // Fire-and-forget, and the entry point returns nothing, so a runtime + // that will not build is logged rather than reported: the prover simply + // stays cold and `platform_wallet_shielded_prover_is_ready` says so. + match runtime().checked() { + Ok(rt) => { + rt.spawn_blocking(|| CachedOrchardProver::new().warm_up()); + } + Err(error) => tracing::error!(%error, "shielded prover warm-up not scheduled"), + } } /// Whether the Halo 2 proving key has already been built. @@ -536,7 +545,12 @@ fn poke_sync_on_unconfirmed(result: &Result, handle: else { return; }; - runtime().spawn(async move { + let Ok(rt) = runtime().checked() else { + // Best-effort re-drive; with no runtime there is nothing to poke and + // the next pass owns it anyway. + return; + }; + rt.spawn(async move { let summary = sync_manager.sync_now(true).await; if summary.sync_unix_seconds == 0 { tracing::debug!( @@ -556,9 +570,16 @@ fn poke_sync_on_unconfirmed(result: &Result, handle: /// code split so hosts can tell "definitively failed, safe to retry" from /// "may have executed, do NOT retry". fn map_spend_result( - result: Result<(), PlatformWalletError>, + result: Result<(), crate::panic_guard::GuardedError>, operation: &str, ) -> PlatformWalletFFIResult { + let result = match crate::panic_guard::peel_boundary(result) { + Ok(result) => result, + // Verbatim: every arm below adds operation context, and a caught panic + // must keep its marker at position 0 (and must never borrow one of the + // typed retry/outcome codes). + Err(boundary) => return boundary, + }; match result { Ok(()) => PlatformWalletFFIResult::ok(), // Ambiguous: the broadcast was accepted but its execution result @@ -618,9 +639,15 @@ fn map_spend_result( /// error path. The wallet retains nonterminal consumption-unknown state; the /// host must not interpret this code as authenticated completion. fn map_asset_lock_funding_result( - result: Result<(), PlatformWalletError>, + result: Result<(), crate::panic_guard::GuardedError>, operation: &str, ) -> PlatformWalletFFIResult { + let result = match crate::panic_guard::peel_boundary(result) { + Ok(result) => result, + // Verbatim: the generic arm below would otherwise prefix a caught + // panic with `{operation} failed: `. + Err(boundary) => return boundary, + }; match result { Ok(()) => PlatformWalletFFIResult::ok(), Err(e @ PlatformWalletError::AssetLockAlreadyConsumed(_)) => e.into(), @@ -796,6 +823,11 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_identity_create_from_p r }); + // Peel the FFI-local outer failure off first: the arms below write + // `out_identity_id` and reach for typed shielded codes, neither of which a + // caught panic may claim. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); + match result { Ok(identity_id) => { *out_identity_id = identity_id.to_buffer(); @@ -1507,6 +1539,9 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_seed_pool_notes( .await }); + // Peel the FFI-local outer failure off first: the arm below adds context + // around the error, which would push a caught panic's marker off position 0. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); match result { Ok(_outcome) => PlatformWalletFFIResult::ok(), Err(e) => PlatformWalletFFIResult::err( @@ -1526,11 +1561,12 @@ fn resolve_wallet( wallet_id: &[u8; 32], ) -> Result, PlatformWalletFFIResult> { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(manager.get_wallet(wallet_id)) + runtime().try_block_on(manager.get_wallet(wallet_id)) }); match option { - Some(Some(wallet)) => Ok(wallet), - Some(None) => Err(PlatformWalletFFIResult::err( + Some(Ok(Some(wallet))) => Ok(wallet), + Some(Err(error)) => Err(PlatformWalletFFIResult::from(error)), + Some(Ok(None)) => Err(PlatformWalletFFIResult::err( PlatformWalletFFIResultCode::ErrorWalletOperation, format!("wallet not found: {}", hex::encode(wallet_id)), )), @@ -1556,14 +1592,15 @@ fn resolve_wallet_and_coordinator( PlatformWalletFFIResult, > { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(async { + runtime().try_block_on(async { let wallet = manager.get_wallet(wallet_id).await; let coordinator = manager.shielded_coordinator().await; (wallet, coordinator) }) }); let (wallet_opt, coord_opt) = match option { - Some(v) => v, + Some(Ok(v)) => v, + Some(Err(error)) => return Err(PlatformWalletFFIResult::from(error)), None => { return Err(PlatformWalletFFIResult::err( PlatformWalletFFIResultCode::ErrorInvalidHandle, @@ -1589,8 +1626,56 @@ fn resolve_wallet_and_coordinator( #[cfg(test)] mod tests { use super::*; + use crate::panic_guard::{FfiBoundaryError, GuardedError, FFI_PANIC_PREFIX}; use dpp::shielded::MEMO_PAYLOAD_SIZE; + /// A domain error in the guarded shape the mappers now take. + fn domain(error: PlatformWalletError) -> Result<(), GuardedError> { + Err(GuardedError::Domain(error)) + } + + /// A caught panic in that same shape. + fn caught_panic(message: &str) -> Result<(), GuardedError> { + Err(GuardedError::Boundary(FfiBoundaryError::caught_panic( + format!("{FFI_PANIC_PREFIX}{message}"), + ))) + } + + /// A panic must leave these mappers by their own door. + /// + /// Every domain arm in `map_spend_result` / `map_asset_lock_funding_result` + /// either reaches for a typed shielded code or wraps the payload in + /// `"{operation} failed: "`. Both would be wrong for a panic: the typed + /// codes carry retry/outcome contracts a panic cannot honour, and the + /// prefix pushes the machine-readable marker off position 0, which is + /// exactly what hosts key on (dashpay/platform#4424 review). + #[test] + fn mappers_pass_a_caught_panic_through_at_code_six_with_the_marker_first() { + for result in [ + map_spend_result(caught_panic("spend boom"), "shielded transfer"), + map_asset_lock_funding_result( + caught_panic("funding boom"), + "shielded fund-from-asset-lock", + ), + ] { + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorWalletOperation, + "a panic must ride the generic code, never a typed shielded one" + ); + let message = message_of(&result); + assert_eq!( + message.find(FFI_PANIC_PREFIX), + Some(0), + "the marker must stay at position 0, un-prefixed: {message}" + ); + assert!( + !message.contains("failed: "), + "the operation context must not be prepended to a panic: {message}" + ); + } + } + #[test] fn encode_memo_text_none_is_empty() { let bytes = encode_memo_text(None).expect("None must encode"); @@ -1731,11 +1816,10 @@ mod tests { /// every error arm so callers keep diagnostics across the boundary. #[test] fn map_spend_result_pins_retry_relevant_codes() { - let unconfirmed: Result<(), PlatformWalletError> = - Err(PlatformWalletError::ShieldedSpendUnconfirmed { - operation: "unshield", - reason: "transient proof fetch failed".to_string(), - }); + let unconfirmed = domain(PlatformWalletError::ShieldedSpendUnconfirmed { + operation: "unshield", + reason: "transient proof fetch failed".to_string(), + }); let result = map_spend_result(unconfirmed, "shielded unshield"); assert_eq!( result.code, @@ -1746,9 +1830,9 @@ mod tests { "unconfirmed message must carry the wallet Display payload" ); - let failed: Result<(), PlatformWalletError> = Err( - PlatformWalletError::ShieldedBroadcastFailed("relay rejected".to_string()), - ); + let failed = domain(PlatformWalletError::ShieldedBroadcastFailed( + "relay rejected".to_string(), + )); let result = map_spend_result(failed, "shielded transfer"); assert_eq!( result.code, @@ -1761,9 +1845,9 @@ mod tests { // No Platform-recorded anchor yet → its own retryable code, distinct // from the "was broadcast, do NOT retry" unconfirmed code above. - let no_anchor: Result<(), PlatformWalletError> = Err( - PlatformWalletError::ShieldedNoRecordedAnchor("mid-block".to_string()), - ); + let no_anchor = domain(PlatformWalletError::ShieldedNoRecordedAnchor( + "mid-block".to_string(), + )); let result = map_spend_result(no_anchor, "shielded withdraw"); assert_eq!( result.code, @@ -1774,8 +1858,7 @@ mod tests { "no-recorded-anchor message must be the retryable guidance" ); - let other: Result<(), PlatformWalletError> = - Err(PlatformWalletError::ShieldedNoUnspentNotes); + let other = domain(PlatformWalletError::ShieldedNoUnspentNotes); let result = map_spend_result(other, "shielded withdraw"); assert_eq!( result.code, @@ -1794,12 +1877,11 @@ mod tests { /// The submitted/expected nonce values must survive in the message. #[test] fn map_spend_result_maps_address_nonce_mismatch_to_dedicated_code() { - let mismatch: Result<(), PlatformWalletError> = - Err(PlatformWalletError::AddressNonceMismatch { - address: PlatformAddress::P2pkh([7u8; 20]), - provided_nonce: 1, - expected_nonce: 2, - }); + let mismatch = domain(PlatformWalletError::AddressNonceMismatch { + address: PlatformAddress::P2pkh([7u8; 20]), + provided_nonce: 1, + expected_nonce: 2, + }); let result = map_spend_result(mismatch, "shielded shield"); assert_eq!( result.code, @@ -1822,7 +1904,7 @@ mod tests { #[test] fn map_spend_result_maps_shield_capacity_race_to_dedicated_code() { let shield_result = map_spend_result( - Err(PlatformWalletError::PlatformShieldCapacityExceeded { + domain(PlatformWalletError::PlatformShieldCapacityExceeded { available: 3_623_849_220, required: 3_623_849_221, }), @@ -1838,7 +1920,7 @@ mod tests { assert!(message.contains("required 3623849221")); let transfer_result = map_spend_result( - Err(PlatformWalletError::ShieldedInsufficientBalance { + domain(PlatformWalletError::ShieldedInsufficientBalance { available: 3_623_849_220, required: 3_623_849_221, }), @@ -1859,7 +1941,7 @@ mod tests { vout: 7, }; let result = map_asset_lock_funding_result( - Err(PlatformWalletError::AssetLockAlreadyConsumed(out_point)), + domain(PlatformWalletError::AssetLockAlreadyConsumed(out_point)), "shielded fund-from-asset-lock", ); assert_eq!( @@ -1869,7 +1951,7 @@ mod tests { assert!(message_of(&result).contains("Platform completion is unconfirmed")); let unrelated = map_asset_lock_funding_result( - Err(PlatformWalletError::ShieldedNoUnspentNotes), + domain(PlatformWalletError::ShieldedNoUnspentNotes), "shielded fund-from-asset-lock", ); assert_eq!( diff --git a/packages/rs-platform-wallet-ffi/src/shielded_sync.rs b/packages/rs-platform-wallet-ffi/src/shielded_sync.rs index 8b41f702b8e..3eca567a60c 100644 --- a/packages/rs-platform-wallet-ffi/src/shielded_sync.rs +++ b/packages/rs-platform-wallet-ffi/src/shielded_sync.rs @@ -17,9 +17,9 @@ use platform_wallet::wallet::shielded::ShieldedSyncSummary; use crate::error::*; use crate::handle::*; -use crate::runtime::{block_on_worker, runtime}; +use crate::runtime::{block_on_worker, runtime, try_block_on_worker}; use crate::shielded_types::ShieldedSyncWalletResultFFI; -use crate::{check_ptr, unwrap_option_or_return}; +use crate::{check_ptr, unwrap_option_or_return, unwrap_result_or_return}; use rs_sdk_ffi::MnemonicResolverHandle; impl ShieldedSyncWalletResultFFI { @@ -53,10 +53,15 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_sync_start( handle: Handle, ) -> PlatformWalletFFIResult { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - let _entered = runtime().enter(); + // The loop's `tokio::spawn` needs a runtime in scope, so acquisition + // is fallible here: with no runtime there is nothing to start the + // shielded loop on, and that has to be reported rather than + // silently skipped. + let _entered = runtime().checked()?.enter(); manager.shielded_sync_arc().start(); + Ok::<(), crate::panic_guard::FfiBoundaryError>(()) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -89,9 +94,9 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_sync_stop( handle: Handle, ) -> PlatformWalletFFIResult { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(manager.shielded_sync().quiesce()) + runtime().try_block_on(manager.shielded_sync().quiesce()) }); - let drained = unwrap_option_or_return!(option); + let drained = unwrap_result_or_return!(unwrap_option_or_return!(option)); if !drained { // The in-flight pass did not drain within the quiesce budget — // it may still fire persistence / completion callbacks. Surface @@ -191,9 +196,9 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_sync_sync_now( // and on-sim 2026-07-07 from the Sync Now button). The worker // dispatch moves the compute onto the runtime's 8 MB-stack // threads (see runtime.rs) — same fix as dashpay_sync. - block_on_worker(async move { mgr.sync_now(true).await }); + try_block_on_worker(async move { mgr.sync_now(true).await }) }); - unwrap_option_or_return!(option); + unwrap_result_or_return!(unwrap_option_or_return!(option)); PlatformWalletFFIResult::ok() } @@ -271,13 +276,13 @@ pub unsafe extern "C" fn platform_wallet_manager_bind_shielded( // self-registers its viewing keys for the coordinator-driven // sync loop. let lookup = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(async { + runtime().try_block_on(async { let wallet = manager.get_wallet(&wallet_id).await; let coordinator = manager.shielded_coordinator().await; (wallet, coordinator) }) }); - let (wallet_arc, coordinator) = unwrap_option_or_return!(lookup); + let (wallet_arc, coordinator) = unwrap_result_or_return!(unwrap_option_or_return!(lookup)); let wallet_arc = match wallet_arc { Some(w) => w, None => { @@ -301,9 +306,10 @@ pub unsafe extern "C" fn platform_wallet_manager_bind_shielded( // prior seed-backed bind. `Ok(false)` means at least one // requested account has no persisted row — only then is the // mnemonic resolved. - match runtime() - .block_on(wallet_arc.bind_shielded_from_persisted(accounts.as_slice(), &coordinator)) - { + match unwrap_result_or_return!(crate::panic_guard::peel_boundary( + runtime() + .block_on(wallet_arc.bind_shielded_from_persisted(accounts.as_slice(), &coordinator)) + )) { Ok(true) => return PlatformWalletFFIResult::ok(), Ok(false) => {} Err(e) => { @@ -325,11 +331,11 @@ pub unsafe extern "C" fn platform_wallet_manager_bind_shielded( Err(result) => return result, }; - if let Err(e) = runtime().block_on(wallet_arc.bind_shielded( - seed.as_ref(), - accounts.as_slice(), - &coordinator, - )) { + if let Err(e) = + unwrap_result_or_return!(crate::panic_guard::peel_boundary(runtime().block_on( + wallet_arc.bind_shielded(seed.as_ref(), accounts.as_slice(), &coordinator) + ))) + { return PlatformWalletFFIResult::err( PlatformWalletFFIResultCode::ErrorWalletOperation, format!("bind_shielded failed: {e}"), @@ -377,7 +383,9 @@ pub unsafe extern "C" fn platform_wallet_manager_configure_shielded( let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { runtime().block_on(manager.configure_shielded(&db_path)) }); - let result = unwrap_option_or_return!(option); + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary( + unwrap_option_or_return!(option) + )); if let Err(e) = result { return PlatformWalletFFIResult::err( PlatformWalletFFIResultCode::ErrorWalletOperation, @@ -435,6 +443,10 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_clear( runtime().block_on(manager.clear_shielded()) }); let result = unwrap_option_or_return!(option); + // Peel the FFI-local outer failure off first: the generic arm below adds + // `clear_shielded failed: ` context, which would push a caught panic's + // marker off position 0. + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary(result)); if let Err(e) = result { // A drain that did not complete is NOT an ordinary store failure: // it means callback-capable work may still be running, which the @@ -497,7 +509,7 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_default_address( } let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(async { + runtime().try_block_on(async { match manager.get_wallet(&wallet_id).await { None => Outcome::WalletMissing, Some(w) => match w.shielded_default_address(account).await { @@ -507,7 +519,7 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_default_address( } }) }); - let outcome = unwrap_option_or_return!(option); + let outcome = unwrap_result_or_return!(unwrap_option_or_return!(option)); match outcome { Outcome::WalletMissing => PlatformWalletFFIResult::err( @@ -559,7 +571,9 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_sync_wallet( let mgr = manager.shielded_sync_arc(); block_on_worker(async move { mgr.sync_wallet(&wallet_id, true).await }) }); - let result = unwrap_option_or_return!(option); + let result = unwrap_result_or_return!(crate::panic_guard::peel_boundary( + unwrap_option_or_return!(option) + )); match result { Ok(_) => PlatformWalletFFIResult::ok(), Err(e) => PlatformWalletFFIResult::err( diff --git a/packages/rs-platform-wallet-ffi/src/spv.rs b/packages/rs-platform-wallet-ffi/src/spv.rs index ef66c16d9c4..c2b7b67246f 100644 --- a/packages/rs-platform-wallet-ffi/src/spv.rs +++ b/packages/rs-platform-wallet-ffi/src/spv.rs @@ -144,9 +144,9 @@ pub unsafe extern "C" fn platform_wallet_manager_sync_progress( check_ptr!(out_progress); let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(manager.spv().sync_progress()) + runtime().try_block_on(manager.spv().sync_progress()) }); - let progress = unwrap_option_or_return!(option); + let progress = unwrap_result_or_return!(unwrap_option_or_return!(option)); *out_progress = match progress { Some(p) => progress_to_ffi(&p), None => FFISpvSyncProgress::default(), @@ -296,9 +296,9 @@ pub unsafe extern "C" fn platform_wallet_manager_spv_connected_peers( *out_count = 0; let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(manager.spv().connected_peers()) + runtime().try_block_on(manager.spv().connected_peers()) }); - let peers = unwrap_option_or_return!(option); + let peers = unwrap_result_or_return!(unwrap_option_or_return!(option)); if peers.is_empty() { return PlatformWalletFFIResult::ok(); } @@ -371,9 +371,9 @@ pub unsafe extern "C" fn platform_wallet_manager_spv_tip_unix_seconds( ) -> PlatformWalletFFIResult { check_ptr!(out_unix_seconds); let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { - runtime().block_on(manager.spv().tip_block_time()) + runtime().try_block_on(manager.spv().tip_block_time()) }); - let tip = unwrap_option_or_return!(option); + let tip = unwrap_result_or_return!(unwrap_option_or_return!(option)); *out_unix_seconds = tip.map(|t| t as u64).unwrap_or(0); PlatformWalletFFIResult::ok() } @@ -553,8 +553,19 @@ pub unsafe extern "C" fn platform_wallet_manager_spv_start( }; if start_result.is_ok() { - let _guard = runtime().enter(); - spv.spawn_run_loop(); + // `start_result` being `Ok` means `block_on_worker` already + // acquired the runtime, so this cannot fail; match rather than + // unwrap so a future refactor that breaks that coupling reports + // instead of aborting. + match runtime().checked() { + Ok(rt) => { + let _guard = rt.enter(); + spv.spawn_run_loop(); + } + Err(error) => { + tracing::error!(%error, "SPV started but its run loop could not be spawned"); + } + } } start_result @@ -572,6 +583,9 @@ pub unsafe extern "C" fn platform_wallet_manager_spv_stop( handle: Handle, ) -> PlatformWalletFFIResult { let option = PLATFORM_WALLET_MANAGER_STORAGE.with_item(handle, |manager| { + // Deliberately lossy (pre-existing contract): a `stop()` error — and, + // via the guarded `()` recovery, a panic — is logged but not reported; + // stop is best-effort teardown and hosts do not act on its outcome. runtime().block_on(async { let _ = manager.spv().stop().await; }); diff --git a/packages/rs-platform-wallet-ffi/src/wallet.rs b/packages/rs-platform-wallet-ffi/src/wallet.rs index bf53c57171f..fd85d8f2ca0 100644 --- a/packages/rs-platform-wallet-ffi/src/wallet.rs +++ b/packages/rs-platform-wallet-ffi/src/wallet.rs @@ -451,7 +451,7 @@ mod destroy_tests { // NativeCleaner threads do (never from inside a tokio runtime). Calling // them from within an outer `block_on` would nest runtimes and abort, so // they run on the plain test thread below. - let (manager, handle_a, handle_b, token, baseline) = runtime().block_on(async { + let (manager, handle_a, handle_b, token, baseline) = runtime().raw().block_on(async { let (manager, wallet_id) = test_platform_wallet_manager().await; // Two independent handles for the SAME logical wallet, exactly as two @@ -510,7 +510,9 @@ mod destroy_tests { // The token is still fully live: its owner can release it even after both // wrappers are gone (the registry entry pinned its own `CoreWallet`). - runtime().block_on(SIGNED_PAYMENT_REGISTRY.release(token)); + runtime() + .raw() + .block_on(SIGNED_PAYMENT_REGISTRY.release(token)); assert_eq!( SIGNED_PAYMENT_REGISTRY.outstanding(), baseline, diff --git a/packages/rs-platform-wallet-ffi/src/wallet_startup.rs b/packages/rs-platform-wallet-ffi/src/wallet_startup.rs index 583dfc11bb4..52427d7b0db 100644 --- a/packages/rs-platform-wallet-ffi/src/wallet_startup.rs +++ b/packages/rs-platform-wallet-ffi/src/wallet_startup.rs @@ -233,12 +233,13 @@ pub unsafe extern "C" fn platform_wallet_manager_start_wallet_subsystems( "Manager handle invalid".to_string(), ) } - Err(e) => { - return PlatformWalletFFIResult::err( - PlatformWalletFFIResultCode::ErrorWalletOperation, - format!("failed to spawn the startup thread: {e}"), - ) - } + // Verbatim, NOT re-wrapped: `run_on_big_stack_thread` reports a + // refused thread AND a caught panic through this one channel, and its + // message already carries the matching marker at position 0. The old + // `format!("failed to spawn the startup thread: {e}")` both pushed that + // marker off position 0 and mislabelled a panic as a spawn failure + // (dashpay/platform#4424 review). + Err(e) => return e.into(), }; *out_outcome = WalletStartupOutcomeFFI::from(outcome);