diff --git a/packages/rs-platform-wallet-ffi/src/error.rs b/packages/rs-platform-wallet-ffi/src/error.rs index 444573c5dbc..d531d4c6259 100644 --- a/packages/rs-platform-wallet-ffi/src/error.rs +++ b/packages/rs-platform-wallet-ffi/src/error.rs @@ -586,6 +586,18 @@ impl From for PlatformWalletFFIResult { PlatformWalletError::PlatformShieldCapacityExceeded { .. } => { PlatformWalletFFIResultCode::ErrorShieldedInsufficientBalance } + // The per-input sibling of the account-capacity variant above: a + // live pre-broadcast per-input shortfall (a stale-snapshot race). + // It rides the SAME capacity code — the host's corrective action is + // identical (refresh preflight, retry) — but as its OWN wallet + // variant so the message names the offending address and the typed + // `available`/`required` are understood as that single input's live + // figures, not an account maximum. Minting a distinct FFI code was + // deliberately avoided to not collide with the in-flight code-space + // frontier; the disambiguation lives in the message. + PlatformWalletError::PlatformShieldInputShortfall { .. } => { + PlatformWalletFFIResultCode::ErrorShieldedInsufficientBalance + } // The core-transaction sibling of the shielded pair above: the // do-not-retry signal must survive the boundary as a typed code // so hosts can distinguish it from a definitive rejection. @@ -708,6 +720,14 @@ impl From for PlatformWalletFFIResult { // `MessageSigningKeyUnavailable` mapped above. By the time a // `MessageSigningFailed` reason exists, any marker in it sits // mid-string and is deliberately not matched. + // A panic recovered at the runtime-helper boundary + // (`runtime::block_on_worker` / `run_on_big_stack_thread`) instead + // of aborting the host. It is by definition an unexpected internal + // failure, so it reuses the generic `ErrorUnknown` code (per the + // "add or reuse an internal-panic code" contract); the panic text + // rides the message. An explicit arm — rather than the catch-all + // below — so the deliberate reuse is greppable. + PlatformWalletError::InternalPanic(..) => PlatformWalletFFIResultCode::ErrorUnknown, _ => PlatformWalletFFIResultCode::ErrorUnknown, }; PlatformWalletFFIResult::err(code, error.to_string()) diff --git a/packages/rs-platform-wallet-ffi/src/runtime.rs b/packages/rs-platform-wallet-ffi/src/runtime.rs index ee96010db00..108d4d903ca 100644 --- a/packages/rs-platform-wallet-ffi/src/runtime.rs +++ b/packages/rs-platform-wallet-ffi/src/runtime.rs @@ -45,19 +45,144 @@ pub(crate) fn runtime() -> &'static tokio::runtime::Runtime { &RT } +/// Convert a caught worker/thread panic into a value of the driven future's +/// own output type, so a panic surfaces as a typed error at the FFI boundary +/// instead of unwinding across `extern "C"` and aborting the host (workspace +/// policy — `Cargo.toml`: "a JNI library must never abort the app process"). +/// +/// Implemented for every output type actually driven through +/// [`block_on_worker`]: any `Result` whose error type recovers (the +/// overwhelmingly common shape — the panic becomes a typed `Err`), plus the +/// handful of non-`Result`, best-effort outputs whose panic degrades to a +/// logged, empty value. The [`block_on_worker`] bound is fail-closed: a new +/// call site whose output does not implement this will not compile until it +/// opts into an explicit recovery here. +pub(crate) trait RecoverWorkerPanic { + fn recover_from_worker_panic(reason: String) -> Self; +} + +impl RecoverWorkerPanic for Result { + fn recover_from_worker_panic(reason: String) -> Self { + Err(E::recover_from_worker_panic(reason)) + } +} + +impl RecoverWorkerPanic for platform_wallet::PlatformWalletError { + fn recover_from_worker_panic(reason: String) -> Self { + platform_wallet::PlatformWalletError::InternalPanic(reason) + } +} + +/// Fire-and-forget `..._sync_now` entry points returning `()`: the panic is +/// already logged by the runtime helper; the sync is a no-op for this pass and +/// the host's next periodic sync retries. +impl RecoverWorkerPanic for () { + fn recover_from_worker_panic(_reason: String) -> Self {} +} + +/// Contact-crypto counters: a recovered panic reports zero (logged), which the +/// host reconciles on its next sync — never an abort. +impl RecoverWorkerPanic for usize { + fn recover_from_worker_panic(_reason: String) -> Self { + 0 + } +} + +/// Best-effort sync summaries: a recovered panic yields an empty summary +/// (0 processed / 0 errors), logged; the host's next sync retries. Never an +/// abort. +impl RecoverWorkerPanic for platform_wallet::DashPaySyncSummary { + fn recover_from_worker_panic(_reason: String) -> Self { + Self::default() + } +} + +impl RecoverWorkerPanic for platform_wallet::manager::dpns_sync::DpnsSyncPassSummary { + fn recover_from_worker_panic(_reason: String) -> Self { + Self::default() + } +} + +impl RecoverWorkerPanic for platform_wallet::manager::shielded_sync::ShieldedSyncPassSummary { + fn recover_from_worker_panic(_reason: String) -> Self { + Self::default() + } +} + +/// Best-effort message from a caught panic payload (`Box`), which is a +/// `&'static str` or `String` for essentially every panic (`panic!`, `unwrap`, +/// `expect`, assertions). +fn panic_payload_message(payload: &(dyn std::any::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 { + "non-string panic payload".to_string() + } +} + /// Drive `future` to completion, moving the actual polling onto a /// worker thread so the caller's stack size doesn't bound the /// computation. /// /// The calling thread still blocks (that's what FFI wants); it just /// parks on a oneshot instead of driving the future itself. +/// +/// ## Panic safety +/// +/// A panic inside `future` must NOT cross the `extern "C"` FFI boundary: on the +/// `unwind` (Android/host) build that unwind aborts the process with `SIGABRT` +/// (Rust aborts when a panic escapes an `extern "C"` fn), and the JNI shim's +/// own `catch_unwind` (`rs-unified-sdk-jni`) sits ABOVE this callee, so it can +/// never intercept the abort — exactly what the workspace policy in `Cargo.toml` +/// forbids. Two panic surfaces are closed here: +/// +/// 1. `future` panics — tokio unwind-catches the spawned task into a +/// `JoinError`. The pre-fix `.expect("tokio worker panicked")` re-raised it +/// (that re-panic was the abort). We instead convert it into the output +/// type's typed error via [`RecoverWorkerPanic`]. +/// 2. Any stray panic while `block_on` drives the `JoinHandle` — caught by the +/// surrounding `catch_unwind` and recovered the same way. +/// +/// On the iOS `panic = "abort"` profiles (`dev-ios` / `release-ios`) this is +/// INERT by design: the process aborts at the panic site before any +/// `catch_unwind`/`JoinError` is observed. That matches the in-tree note that +/// `catch_unwind` cannot protect an abort-configured build; this hardens the +/// `unwind` builds without pretending to protect iOS. +/// +/// The success path is unchanged: a future that completes normally returns its +/// value directly. pub(crate) fn block_on_worker(future: F) -> F::Output where F: std::future::Future + Send + 'static, - F::Output: Send + 'static, + F::Output: Send + 'static + RecoverWorkerPanic, { let rt = runtime(); - rt.block_on(async move { rt.spawn(future).await.expect("tokio worker panicked") }) + let joined = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + rt.block_on(async move { rt.spawn(future).await }) + })); + match joined { + // Success path (unchanged): the task completed and returned its value. + Ok(Ok(output)) => output, + // The spawned task panicked (or was cancelled): tokio captured it as a + // `JoinError`. Recover into the output's typed error instead of + // re-raising it across the `extern "C"` caller. + Ok(Err(join_err)) => { + let reason = format!("tokio worker task did not complete: {join_err}"); + tracing::error!(target: "platform_wallet_ffi", "{reason}"); + F::Output::recover_from_worker_panic(reason) + } + // Belt and suspenders: a panic in the `block_on` driver itself. + Err(panic_payload) => { + let reason = format!( + "panic while driving tokio worker: {}", + panic_payload_message(panic_payload.as_ref()) + ); + tracing::error!(target: "platform_wallet_ffi", "{reason}"); + F::Output::recover_from_worker_panic(reason) + } + } } /// Run `f` to completion on a freshly spawned scoped OS thread with the @@ -76,16 +201,34 @@ 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. +/// ## Panic safety +/// +/// A panic inside `f` is CAUGHT (`std::thread::join` captures it) and mapped to +/// an `io::Error`, rather than re-raised: the pre-fix +/// `.expect("big-stack FFI thread panicked")` would have unwound across the +/// `extern "C"` caller and aborted the host on the `unwind` build (`Cargo.toml` +/// policy). The lone caller already threads the returned `io::Result` into its +/// `PlatformWalletFFIResult`. On the iOS `abort` profiles this is inert (the +/// process aborts at the panic site), as documented on [`block_on_worker`]. pub(crate) fn run_on_big_stack_thread(f: impl FnOnce() -> T + Send) -> std::io::Result { 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")) + match handle.join() { + Ok(value) => Ok(value), + Err(panic_payload) => { + let reason = panic_payload_message(panic_payload.as_ref()); + tracing::error!( + target: "platform_wallet_ffi", + "big-stack FFI thread panicked: {reason}" + ); + Err(std::io::Error::other(format!( + "big-stack FFI thread panicked: {reason}" + ))) + } + } }) } @@ -122,6 +265,48 @@ mod tests { let out = run_on_big_stack_thread(|| recurse(1_000)).expect("spawn should succeed"); assert!(out > 0); } + + // --- Panic safety (the abort-hazard fix) ------------------------------- + // + // Gated to the `unwind` config: on an `abort`-configured build the panic + // aborts the process at the panic site (documented, accepted iOS behavior), + // so there is no recoverable outcome to assert. Under the normal test + // profile (`unwind`) these prove a panicking future/closure returns the + // typed error instead of aborting the runner. + + #[cfg(panic = "unwind")] + #[test] + fn block_on_worker_recovers_panicking_future_as_typed_error() { + let out: Result = + block_on_worker(async { panic!("boom in worker") }); + match out { + Err(platform_wallet::PlatformWalletError::InternalPanic(msg)) => { + assert!( + msg.contains("did not complete") || msg.contains("boom in worker"), + "unexpected recovered message: {msg}" + ); + } + other => panic!("expected recovered InternalPanic, got {other:?}"), + } + } + + #[cfg(panic = "unwind")] + #[test] + fn block_on_worker_success_path_is_unchanged() { + let out: Result = + block_on_worker(async { Ok(7) }); + assert!(matches!(out, Ok(7))); + } + + #[cfg(panic = "unwind")] + #[test] + fn run_on_big_stack_thread_maps_panic_to_io_error() { + let result: std::io::Result<()> = + run_on_big_stack_thread(|| panic!("boom on big stack")); + let err = result.expect_err("a panicking closure must map to Err, not abort"); + assert_eq!(err.kind(), std::io::ErrorKind::Other); + assert!(err.to_string().contains("big-stack FFI thread panicked")); + } } #[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..7632f7f6614 100644 --- a/packages/rs-platform-wallet-ffi/src/shielded_send.rs +++ b/packages/rs-platform-wallet-ffi/src/shielded_send.rs @@ -597,15 +597,19 @@ fn map_spend_result( format!("{operation} failed: {e}"), ), // The cached Platform Payment-account set no longer covers the - // requested claim plus input-0's fee reserve. Keep this distinct from - // generic wallet-operation failures so hosts can refresh preflight and - // re-confirm a smaller amount instead of retrying unchanged. - Err(e @ PlatformWalletError::PlatformShieldCapacityExceeded { .. }) => { - PlatformWalletFFIResult::err( - PlatformWalletFFIResultCode::ErrorShieldedInsufficientBalance, - format!("{operation} failed: {e}"), - ) - } + // requested claim plus input-0's fee reserve (account-wide), or a live + // per-input hard balance check found one input short (a stale-snapshot + // race). Both share this code — the host's corrective action is the same + // (refresh preflight, retry) — and both stay distinct from generic + // wallet-operation failures so a host never retries the stale amount + // unchanged. The per-input variant's message names the short address. + Err( + e @ (PlatformWalletError::PlatformShieldCapacityExceeded { .. } + | PlatformWalletError::PlatformShieldInputShortfall { .. }), + ) => PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorShieldedInsufficientBalance, + format!("{operation} failed: {e}"), + ), Err(e) => PlatformWalletFFIResult::err( PlatformWalletFFIResultCode::ErrorWalletOperation, format!("{operation} failed: {e}"), @@ -1852,6 +1856,32 @@ mod tests { ); } + #[test] + fn map_spend_result_maps_per_input_shortfall_to_same_code_with_address() { + // The per-input shortfall (a live stale-snapshot race) must ride the + // same code as the account-capacity variant — not regress to the + // generic ErrorWalletOperation — and keep the offending address in the + // message so a host never misreads the single input's balance as the + // account maximum. + let result = map_spend_result( + Err(PlatformWalletError::PlatformShieldInputShortfall { + address: "yShieldInputAddrExample".to_string(), + available: 3_623_849_220, + required: 3_623_849_221, + }), + "shielded shield", + ); + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorShieldedInsufficientBalance + ); + let message = message_of(&result); + assert!(message.contains("yShieldInputAddrExample"), "message: {message}"); + assert!(message.contains("3623849220")); + assert!(message.contains("3623849221")); + } + #[test] fn map_asset_lock_funding_result_preserves_already_consumed_code_only() { let out_point = dashcore::OutPoint { diff --git a/packages/rs-platform-wallet/src/error.rs b/packages/rs-platform-wallet/src/error.rs index 8349eb1df21..1ee62abace7 100644 --- a/packages/rs-platform-wallet/src/error.rs +++ b/packages/rs-platform-wallet/src/error.rs @@ -494,6 +494,26 @@ pub enum PlatformWalletError { #[error("Platform shield capacity exceeded: available {available}, required {required}")] PlatformShieldCapacityExceeded { available: u64, required: u64 }, + /// A shield's pre-broadcast per-input hard balance check found ONE input + /// address short: its live on-chain balance dropped below what the cached + /// planner snapshot assumed (a stale-snapshot race), so the fetched claim + /// cannot be funded. STRICTLY per-input — `available`/`required` are that + /// single address's live figures, NOT an account-capacity total. Distinct + /// from [`PlatformShieldCapacityExceeded`](Self::PlatformShieldCapacityExceeded) + /// (an account-wide deterministic-selection limit) precisely so a host never + /// misreads this per-address `available` as the account maximum (it would + /// understate capacity by up to the versioned max input count). The + /// offending `address` (bech32m) is preserved in the message rather than + /// dropped. Nothing was built or broadcast; refresh preflight and retry. + #[error( + "Shield input address {address} is short: has {available}, requires at least {required}" + )] + PlatformShieldInputShortfall { + address: String, + available: u64, + required: u64, + }, + #[error("Shielded build error: {0}")] ShieldedBuildError(String), @@ -579,6 +599,17 @@ pub enum PlatformWalletError { #[error("Shielded sub-wallet not bound: call bind_shielded first")] ShieldedNotBound, + + /// An internal async task or big-stack worker panicked and was RECOVERED at + /// the FFI runtime boundary ([`block_on_worker`](crate) / + /// `run_on_big_stack_thread`) instead of being re-raised across the + /// `extern "C"` boundary, which would abort the host process on the + /// `unwind` (Android/host) build. Always an internal bug; the payload is the + /// panic message. Surfaced to the FFI as the generic `ErrorUnknown` code + /// (an unexpected internal failure) with the panic text in the message. + /// Not retryable as-is. + #[error("Internal task panicked (recovered): {0}")] + InternalPanic(String), } /// Check whether an SDK error indicates that an InstantSend lock proof was diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs index b6d29e67c40..b269d24b2c9 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs @@ -94,13 +94,17 @@ impl ShieldedShieldInputPlan { } if amount > self.preflight.max_shieldable_credits { - let available = if self.usable_candidates.is_empty() { - self.preflight.account_balance_credits - } else { - self.preflight.usable_balance_credits - }; + // `available` must be the usable-for-shield amount so a FAILED call + // never reports the self-contradictory `available > required`. When + // no candidate can retain the fee reserve (a fragmented account) + // `usable_balance_credits` is 0 — coherent with a nonzero + // `required = amount + fee_reserve`. The pre-fix fallback to + // `account_balance_credits` reported the FULL account balance here, + // which can exceed `required` and read as a contradictory shortfall; + // the total account balance stays available on the preflight + // snapshot (`account_balance_credits`) and its `reason` for display. return Err(PlatformWalletError::PlatformShieldCapacityExceeded { - available, + available: self.preflight.usable_balance_credits, required: amount.saturating_add(self.preflight.fee_reserve_credits), }); } @@ -1575,7 +1579,7 @@ impl PlatformWallet { }), account.address_balances.keys().copied(), ); - let candidates = candidate_addresses + let candidates: Vec<(PlatformAddress, Credits)> = candidate_addresses .into_iter() .filter_map(|p2pkh| { let balance = account.address_credit_balance(&p2pkh); @@ -1585,11 +1589,21 @@ impl PlatformWallet { let platform_version = self.sdk.version(); let state_transition_version = &platform_version.dpp.state_transitions; + let max_address_inputs = usize::from(state_transition_version.max_address_inputs); + // The reserve retained on input 0 must cover the per-input + // `SetBalanceToAddress` writes of every input a Max shield can admit + // from this account: the funded-candidate count, capped at the protocol + // maximum. That is an upper bound on the inputs any shield from this + // account uses, so the reserve is sufficient for every requested amount + // (`shielded_shield_from_account` re-plans through this same helper, so + // preflight and execution stay consistent) while staying tight — no + // always-16 over-reservation for accounts with few funded addresses. + let reserve_input_count = candidates.len().min(max_address_inputs); plan_shield_inputs( candidates, - shield_fee_reserve_credits(platform_version)?, + shield_fee_reserve_credits(platform_version, reserve_input_count)?, state_transition_version.address_funds.min_input_amount, - usize::from(state_transition_version.max_address_inputs), + max_address_inputs, ) } @@ -2055,8 +2069,12 @@ mod shield_input_selection_tests { use dpp::address_funds::PlatformAddress; use dpp::version::LATEST_PLATFORM_VERSION; + /// The zero-input base reserve (`2 × F`) — a stable unit for the selection + /// tests below, which exercise `plan_shield_inputs` / `select_inputs` with a + /// controlled reserve value. The input-count scaling of the reserve itself + /// is covered separately in `operations::reserve_shield_fee_tests`. fn reserve() -> Credits { - shield_fee_reserve_credits(LATEST_PLATFORM_VERSION) + shield_fee_reserve_credits(LATEST_PLATFORM_VERSION, 0) .expect("latest shield fee reserve must be computable") } @@ -2132,13 +2150,47 @@ mod shield_input_selection_tests { ); assert!(plan.preflight.reason.is_some()); let err = plan.select_inputs(1).unwrap_err(); + // `available` is the usable-for-shield amount (0 here — the sole address + // cannot retain the reserve), NOT the account balance, so it stays below + // `required` for this failed shield. assert!(matches!( err, PlatformWalletError::PlatformShieldCapacityExceeded { available, required } - if available == reserve() && required == 1 + reserve() + if available == 0 && required == 1 + reserve() )); } + #[test] + fn fragmented_account_reports_coherent_available_not_exceeding_required() { + // Several addresses each holding exactly the reserve: none is strictly + // `> reserve`, so `usable_candidates` is empty and `max_shieldable` is 0, + // yet the account holds a large total balance. A failed shield must + // report `available` as the usable-for-shield amount (0), NOT the full + // account balance — otherwise a fragmented account yields the + // self-contradictory `available > required`. + let candidates = vec![(addr(1), reserve()), (addr(2), reserve()), (addr(3), reserve())]; + let plan = plan(candidates).unwrap(); + assert!(plan.usable_candidates.is_empty()); + assert_eq!(plan.preflight.max_shieldable_credits, 0); + assert_eq!(plan.preflight.account_balance_credits, 3 * reserve()); + + let amount = 1; + match plan.select_inputs(amount).unwrap_err() { + PlatformWalletError::PlatformShieldCapacityExceeded { available, required } => { + assert_eq!( + available, 0, + "a fragmented account must report 0 usable, not the full balance" + ); + assert_eq!(required, amount + reserve()); + assert!( + available < required, + "a failed shield must never report available > required" + ); + } + other => panic!("expected PlatformShieldCapacityExceeded, got {other:?}"), + } + } + #[test] fn amount_equal_to_total_minus_reserve_claims_exactly_amount() { // Single address holding exactly amount + reserve: claim == diff --git a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs index 4783ce75f9a..ffccad33272 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs @@ -55,6 +55,7 @@ use dpp::shielded::builder::{ build_unshield_transition, OrchardProver, SpendableNote, }; use dpp::shielded::compute_minimum_shielded_fee; +use dpp::shielded::SHIELDED_UNSHIELD_ADDRESS_STORAGE_BYTES; use dpp::state_transition::proof_result::StateTransitionProofResult; use dpp::state_transition::public_key_in_creation::IdentityPublicKeyInCreation; use dpp::state_transition::StateTransition; @@ -76,39 +77,90 @@ use tracing::{debug, info, trace, warn}; /// count, so the wallet's fee reservation must use the same count. const SHIELD_NUM_ACTIONS: usize = 2; -/// Multiplier applied to the versioned minimum shield fee when sizing the -/// planner's input-0 reserve. -/// -/// Execution deducts the ACTUAL fee — the GroveDB-metered storage/processing -/// of the note/nullifier writes plus `compute_shielded_verification_fee` — -/// from input 0's post-reallocation residue, and rejects the shield when the -/// residue can't cover it. `compute_minimum_shielded_fee` estimates that -/// actual fee with a flat per-action storage term the client cannot meter -/// itself, so the reserve keeps one extra fee of headroom for metering -/// variance. The reserve is NOT what satisfies the structure gate -/// (`Σ claims ≥ amount + fee`) — `reserve_shield_fee_on_input_0` loads the -/// claimed fee for that — so it needs no allowance beyond metering variance. -const SHIELD_FEE_RESERVE_MULTIPLIER: u64 = 2; +/// Extra metering-variance headroom the shield planner keeps on input 0 BEYOND +/// the input-count-scaled modeled fee, expressed as a count of base +/// structure-gate fees `F`. `1` = one extra `F`, preserving the pre-input- +/// scaling "one extra fee of headroom" semantics on top of the now +/// input-count-scaled modeled fee. +const SHIELD_FEE_RESERVE_HEADROOM_FEES: u64 = 1; /// Versioned balance the shield planner keeps unclaimed on the -/// lexicographically first (fee-paying) input. +/// lexicographically first (fee-paying) input, SCALED by the number of address +/// inputs the transition will admit. /// /// The preflight and the execution path both derive capacity from this one /// value, so it directly sets three host-visible numbers: the viability /// threshold an address must exceed to serve as input 0, the account's /// `max_shieldable_credits`, and the residue a Max shield leaves transparent /// (`reserve − actual fee`). Deriving it from the versioned fee formula keeps -/// all three tracking fee-constant bumps instead of freezing a magic number -/// that overstates the fee and understates capacity. +/// all three tracking fee-constant bumps instead of freezing a magic number. +/// +/// The whole transition fee is deducted from input 0's post-reallocation +/// residue (`DeductFromInput(0)`), so input 0 must retain at least the actual +/// metered fee. For a transparent Shield that fee is: +/// +/// * the flat Orchard-bundle fee `F = compute_minimum_shielded_fee(2)` — the +/// ZK compute plus the two output-bundle actions' note storage, which is +/// also the amount the consensus structure check requires +/// (`Σ claims ≥ amount + F`); PLUS +/// * one `SetBalanceToAddress` address-balance write PER INPUT that drive +/// meters as storage. `compute_minimum_shielded_fee` prices NONE of these +/// per-input writes, so a FLAT reserve thins as inputs grow (blind to input +/// count): a large Max shield can land in the estimate-vs-actual band that +/// risks the `InternalError`/`TxAction::Removed` app-hash-divergence class +/// (the family of the mainnet shield-halt). Scaling the reserve by input +/// count keeps the headroom from thinning. +/// +/// The per-input write is priced off the SAME versioned per-byte storage rate +/// the executor reads (never a hardcoded credit figure), using the reviewed +/// [`SHIELDED_UNSHIELD_ADDRESS_STORAGE_BYTES`] address-write model. That +/// constant sizes a NEW-address `AddBalanceToAddress`; a shield's +/// `SetBalanceToAddress` reduces an EXISTING address balance (a value +/// replacement), which meters no more than a fresh subtree write — so pricing +/// every input at the new-address figure is a deliberate, conservative UPPER +/// BOUND on the true per-input cost. +/// +/// `reserve = F + input_count × per_input_write + headroom`, with +/// `headroom = SHIELD_FEE_RESERVE_HEADROOM_FEES × F`. At `input_count == 0` +/// this equals the pre-scaling flat `2 × F`. +/// +/// SAFETY / RESIDUAL FLAGGED FOR REVIEW: the per-input term is an upper bound +/// derived from an existing calibrated constant, NOT a re-derivation of drive's +/// exact `SetBalanceToAddress` metering. It stays conservative for the +/// protocol-max input count (16), but a reviewer should confirm drive never +/// charges MORE than a new-address write per input; if it can, raise the +/// per-input byte model or `SHIELD_FEE_RESERVE_HEADROOM_FEES`. pub fn shield_fee_reserve_credits( platform_version: &PlatformVersion, + input_count: usize, ) -> Result { - let fee = compute_minimum_shielded_fee(SHIELD_NUM_ACTIONS, platform_version) + let overflow = + || PlatformWalletError::ShieldedBuildError("shield fee reserve overflows u64".to_string()); + + let base_fee = compute_minimum_shielded_fee(SHIELD_NUM_ACTIONS, platform_version) .map_err(|e| PlatformWalletError::ShieldedBuildError(e.to_string()))?; - fee.checked_mul(SHIELD_FEE_RESERVE_MULTIPLIER) - .ok_or_else(|| { - PlatformWalletError::ShieldedBuildError("shield fee reserve overflows u64".to_string()) - }) + + // Per-input `SetBalanceToAddress` storage, priced off the versioned rate the + // executor reads (disk + processing credits/byte), so it tracks fee-constant + // bumps rather than freezing a magic number. + let storage = &platform_version.fee_version.storage; + let per_byte_rate = storage + .storage_disk_usage_credit_per_byte + .checked_add(storage.storage_processing_credit_per_byte) + .ok_or_else(overflow)?; + let per_input_write = SHIELDED_UNSHIELD_ADDRESS_STORAGE_BYTES + .checked_mul(per_byte_rate) + .ok_or_else(overflow)?; + let inputs_cost = (input_count as u64) + .checked_mul(per_input_write) + .ok_or_else(overflow)?; + + // modeled_fee = F + Σ per-input writes; reserve adds one extra F of headroom. + let modeled_fee = base_fee.checked_add(inputs_cost).ok_or_else(overflow)?; + let headroom = base_fee + .checked_mul(SHIELD_FEE_RESERVE_HEADROOM_FEES) + .ok_or_else(overflow)?; + modeled_fee.checked_add(headroom).ok_or_else(overflow) } /// Try to extract a structured `AddressesNotEnoughFundsError` from @@ -157,16 +209,27 @@ fn address_not_enough_funds( } } -/// Promote the shield pre-broadcast hard balance check into the typed capacity +/// Promote the shield pre-broadcast per-input hard balance check into the typed /// error the FFI and Swift layers recognize. /// /// The values are Platform's live per-input view, not the cached planner -/// snapshot. Their `Display` rendering therefore preserves the actionable -/// available/required diagnostic while the typed variant lets the host refresh -/// preflight instead of retrying the stale amount unchanged. -fn map_shield_input_fetch_error(e: &dash_sdk::Error) -> PlatformWalletError { +/// snapshot. `AddressNotEnoughFundsError` is STRICTLY per-address: `balance()` / +/// `required_balance()` describe the ONE short input, not the account. Mapping +/// them onto the account-wide `PlatformShieldCapacityExceeded` (as the pre-fix +/// code did) let a host misread that single address's `available` as the +/// account maximum — understating capacity by up to the versioned input-count +/// cap — and DROPPED the offending address entirely. We map instead to the +/// distinct [`PlatformShieldInputShortfall`](PlatformWalletError::PlatformShieldInputShortfall) +/// variant, which restores the bech32m address in the message and keeps the +/// per-input figures typed as per-input. Both variants ride the same FFI code +/// (the host's corrective action — refresh preflight, retry — is identical). +fn map_shield_input_fetch_error( + e: &dash_sdk::Error, + network: key_wallet::Network, +) -> PlatformWalletError { match address_not_enough_funds(e) { - Some(short) => PlatformWalletError::PlatformShieldCapacityExceeded { + Some(short) => PlatformWalletError::PlatformShieldInputShortfall { + address: short.address().to_bech32m_string(network), available: short.balance(), required: short.required_balance(), }, @@ -499,7 +562,7 @@ pub async fn shield, P: OrchardPr let fetched = fetch_inputs_with_nonce(sdk, &inputs) .await - .map_err(|error| map_shield_input_fetch_error(&error))?; + .map_err(|error| map_shield_input_fetch_error(&error, sdk.network))?; let mut inputs_with_nonce: BTreeMap = BTreeMap::new(); for (addr, (nonce, credits)) in fetched { @@ -2928,22 +2991,36 @@ mod shield_input_fetch_error_tests { use dpp::consensus::state::address_funds::AddressNotEnoughFundsError; #[test] - fn live_address_shortfall_maps_to_typed_shield_capacity_error() { + fn live_per_input_shortfall_maps_to_typed_input_shortfall_with_address() { + let network = key_wallet::Network::Mainnet; + let short_addr = PlatformAddress::P2pkh([7; 20]); let sdk_error = dash_sdk::Error::from(AddressNotEnoughFundsError::new( - PlatformAddress::P2pkh([7; 20]), + short_addr, 3_623_849_220, 3_623_849_221, )); - let mapped = map_shield_input_fetch_error(&sdk_error); - assert!(matches!( - &mapped, - PlatformWalletError::PlatformShieldCapacityExceeded { available, required } - if *available == 3_623_849_220 && *required == 3_623_849_221 - )); - assert_eq!( - mapped.to_string(), - "Platform shield capacity exceeded: available 3623849220, required 3623849221" + let mapped = map_shield_input_fetch_error(&sdk_error, network); + // A per-input shortfall must map to the DISTINCT per-input variant, not + // the account-capacity one — so a host never misreads this single + // input's balance as the account maximum. + let expected_address = short_addr.to_bech32m_string(network); + match &mapped { + PlatformWalletError::PlatformShieldInputShortfall { + address, + available, + required, + } => { + assert_eq!(address, &expected_address); + assert_eq!(*available, 3_623_849_220); + assert_eq!(*required, 3_623_849_221); + } + other => panic!("expected PlatformShieldInputShortfall, got {other:?}"), + } + // The offending address is restored in the message (dropped pre-fix). + assert!( + mapped.to_string().contains(&expected_address), + "message must name the short address: {mapped}" ); } } @@ -2985,10 +3062,14 @@ mod reserve_shield_fee_tests { .state_transitions .address_funds .min_input_amount; + let max_inputs = usize::from( + LATEST_PLATFORM_VERSION + .dpp + .state_transitions + .max_address_inputs, + ); let shield_fee = compute_minimum_shielded_fee(SHIELD_NUM_ACTIONS, LATEST_PLATFORM_VERSION) .expect("latest shield fee must be computable"); - let reserve = shield_fee_reserve_credits(LATEST_PLATFORM_VERSION) - .expect("latest shield fee reserve must be computable"); let smallest_fee_inclusive_claim = shield_fee .checked_add(1) .expect("latest shield fee plus one credit must fit"); @@ -2997,15 +3078,83 @@ mod reserve_shield_fee_tests { smallest_fee_inclusive_claim >= min_input_amount, "adding the fee must lift even input 0's smallest positive base claim above the protocol minimum" ); + + // `input_count == 0` reproduces the pre-scaling flat 2×F, so the change + // is a superset of the old behavior at the base. + let reserve_flat = shield_fee_reserve_credits(LATEST_PLATFORM_VERSION, 0) + .expect("zero-input reserve must be computable"); + assert_eq!( + reserve_flat, + shield_fee.saturating_mul(2), + "the zero-input reserve must equal the pre-scaling flat 2×F" + ); + + let reserve_max = shield_fee_reserve_credits(LATEST_PLATFORM_VERSION, max_inputs) + .expect("max-input reserve must be computable"); assert!( - reserve >= shield_fee, + reserve_max >= shield_fee, "the retained input-0 headroom must cover the versioned shield fee" ); assert!( - reserve <= shield_fee.saturating_mul(4), - "the reserve must stay a small multiple of the versioned fee — an oversized \ - reserve silently understates preflight capacity and strands the excess \ - below the input-0 viability threshold after a Max shield" + reserve_max > reserve_flat, + "a Max shield at the protocol-max input count must reserve strictly more than the flat base" + ); + // Even scaled to the protocol-max input count the reserve stays a small + // multiple of the versioned fee (~2.6×F at latest constants), so it does + // not silently understate preflight capacity: the old 4×F ceiling still + // holds and is deliberately kept. + assert!( + reserve_max <= shield_fee.saturating_mul(4), + "even at the max input count the reserve must stay within 4×F — an \ + oversized reserve silently understates preflight capacity and strands \ + the excess below the input-0 viability threshold after a Max shield" + ); + } + + #[test] + fn reserve_scales_with_admitted_input_count() { + let max_inputs = usize::from( + LATEST_PLATFORM_VERSION + .dpp + .state_transitions + .max_address_inputs, + ); + + // Strictly monotonic in input count: every extra input prices in one + // more metered `SetBalanceToAddress` write, so the headroom cannot thin + // as inputs grow. + let mut previous = + shield_fee_reserve_credits(LATEST_PLATFORM_VERSION, 0).expect("reserve at 0 inputs"); + for n in 1..=max_inputs { + let reserve = + shield_fee_reserve_credits(LATEST_PLATFORM_VERSION, n).expect("reserve computable"); + assert!( + reserve > previous, + "reserve must strictly increase with input count (n={n})" + ); + previous = reserve; + } + + // At the 16-input boundary the reserve must equal the modeled worst-case + // fee (F + 16 × per-input SetBalanceToAddress write) plus one extra F of + // metering-variance headroom, all derived from the versioned constants. + let shield_fee = compute_minimum_shielded_fee(SHIELD_NUM_ACTIONS, LATEST_PLATFORM_VERSION) + .expect("shield fee computable"); + let storage = &LATEST_PLATFORM_VERSION.fee_version.storage; + let per_byte_rate = storage.storage_disk_usage_credit_per_byte + + storage.storage_processing_credit_per_byte; + let per_input = SHIELDED_UNSHIELD_ADDRESS_STORAGE_BYTES * per_byte_rate; + let modeled_fee_16 = shield_fee + (max_inputs as u64) * per_input; + let reserve_16 = shield_fee_reserve_credits(LATEST_PLATFORM_VERSION, max_inputs) + .expect("reserve at 16 inputs"); + assert_eq!( + reserve_16, + modeled_fee_16 + shield_fee, + "reserve(16) must equal modeled_fee(16) + one F of headroom" + ); + assert!( + reserve_16 > modeled_fee_16, + "the reserve must exceed the modeled worst-case 16-input fee" ); }