-
Notifications
You must be signed in to change notification settings - Fork 58
fix(platform-wallet): contain panics before the extern "C" abort shim #4424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<dashcore::Txid, PlatformWalletError>, | ||
| result: Result<dashcore::Txid, GuardedError<PlatformWalletError>>, | ||
| local_txid: dashcore::Txid, | ||
| ) -> (Option<dashcore::Txid>, 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() | ||
|
Comment on lines
+149
to
160
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Keep the reservation recoverable when abandon panics The transaction is removed from source: ['codex'] |
||
| } | ||
|
|
||
|
|
@@ -204,14 +218,21 @@ 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<dashcore::Txid, GuardedError<PlatformWalletError>> { | ||
| Err(GuardedError::Domain(error)) | ||
| } | ||
|
|
||
| #[test] | ||
| fn network_outcomes_all_carry_a_txid() { | ||
| let accepted = classify_broadcast_result(Ok(txid(1)), txid(9)); | ||
| assert_eq!(accepted.0, Some(txid(9))); | ||
| 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<platform_wallet::broadcaster::SpvBroadcaster>; | ||
|
|
||
| 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)); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Suggestion: Return the local txid for an unknown panic outcome
A caught panic can occur after
broadcast_finalized_transactionhas submitted the transaction but before it returns. This arm nevertheless discards the already-computed deterministiclocal_txid, after the finalized handle has been consumed, even though the panic contract tells the host to treat the result as an unknown outcome and reconcile against chain state. Supplying the txid does not claim network acceptance—the definitive rejection path already supplies it despite proving that the send failed. Preserve the txid for panic-marked boundary failures and update the Swift and JNI wrappers to expose a code-6, panic-marked result as an unknown outcome carrying that txid; JNI currently throws before readingout_txid, while Swift only constructs an outcome for codes 0, 20, and 26.source: ['codex']