From d6610262b00ad8012c9bc1f693b439e130adb886 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 5 Aug 2026 12:52:06 -0400 Subject: [PATCH 1/2] feat(shielded): multi-output transfers + output-aware fee predictor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a multi-output ShieldedTransfer so one transition can fund an address with several notes, and fixes the fee predictor that made such a transfer impossible to construct. ## The fee predictor (blocking bug) `build_shielded_transfer_transition` sized its fee from `spends.len().max(2)`, ignoring the output count. An Orchard action is a joined spend/output slot, so the on-wire action count is `max(num_spends, num_outputs)` padded to `MIN_ACTIONS = 2`. A ShieldedTransfer's `value_balance` IS its fee and consensus pins it to `compute_minimum_shielded_fee(actions.len())` EXACTLY (`validate_minimum_shielded_fee` rejects under- AND over-payment), so any transfer publishing three or more outputs would carve `min_fee(2)` while consensus demanded `min_fee(3)` and be rejected on chain. The spends-only form happened to be correct while `num_outputs <= 2`, because `max(n, 1).max(2) == max(n, 2).max(2)` — which is why the single-output builder never hit it. Both builders now size the fee through a shared `shielded_bundle_action_count`, which delegates to Orchard's own `BundleType::num_actions` so the predictor cannot drift from the builder that lays out the bundle. ## Why several outputs Orchard pads any bundle to two actions, and a padding action's dummy nullifier is randomly generated. An identity id derived from published nullifiers is therefore only reproducible offline when at least two REAL notes are spent — with one real note a retry builds a different dummy and a different id. Funding an address with two sub-target notes instead of one full-target note structurally forces a later spend to select BOTH: greedy largest-first selection cannot stop on a note that does not cover the target. That keeps the padding action, and its random nullifier, out of the bundle. `shielded_identity_id_is_reproducible` states that rule as one predicate next to the id derivation it guards, so callers that must recognise an identity their earlier attempt created gate on the note count — no chain lookup, decided before any proving work. ## Shape The multi-output builder ALWAYS emits a change output and requires the spent value to strictly exceed `sum(amounts) + fee`. That makes the output count — and hence the action count and the fee — a pure function of the inputs (`max(spends, recipients + 1, 2)`), with no circular dependency between "is there change?" and "what is the fee?". Note selection reserves against the same `recipients + 1` floor, so the reserved and carved fees cannot diverge. Repeating the same address across outputs is allowed and is the point: Orchard derives independent notes regardless. ## Layers - rs-dpp: `shielded_bundle_action_count`, `ShieldedTransferOutput`, `build_shielded_transfer_transition_multi`, `shielded_identity_id_is_reproducible` - rs-platform-wallet: `operations::transfer_multi`, `PlatformWallet::shielded_transfer_multi_to` - rs-platform-wallet-ffi: `platform_wallet_manager_shielded_transfer_multi` - rs-unified-sdk-jni + kotlin-sdk: `shieldedTransferMulti` ## Tests - `multi_output_transfer_fee_matches_on_wire_action_count` builds a REAL 2-spend/3-output bundle and pins `value_balance == fee == min_fee(actions.len()) == min_fee(3)`, asserting it is NOT `min_fee(2)`. - `single_output_transfer_fee_matches_on_wire_action_count` pins the single-output builder against a real bundle so the shared helper cannot regress it. - `shielded_bundle_action_count_*` pin the predictor as `max(spends, outputs)` padded to 2, and against a real bundle's on-wire count. - `test_two_sub_denomination_notes_are_both_selected` / `test_single_full_denomination_note_selects_alone` pin the selector behaviour the two-note layout depends on. - The existing padding tests now also assert `shielded_identity_id_is_reproducible`. Swift parity for the new entry point is a follow-up; the cbindgen header is generated at build time and nothing in the Swift SDK references the new symbol, so the Swift build is unaffected. Co-Authored-By: Claude Opus 4.8 --- .../dashsdk/ffi/FundingNative.kt | 20 + .../dashsdk/wallet/PlatformWalletManager.kt | 55 ++ .../identity_create_from_shielded_pool.rs | 14 + packages/rs-dpp/src/shielded/builder/mod.rs | 88 +++- .../src/shielded/builder/shielded_transfer.rs | 468 +++++++++++++++++- .../mod.rs | 29 ++ .../src/shielded_send.rs | 147 ++++++ .../src/wallet/platform_wallet.rs | 52 ++ .../src/wallet/shielded/note_selection.rs | 80 +++ .../src/wallet/shielded/operations.rs | 197 +++++++- packages/rs-unified-sdk-jni/src/funding.rs | 114 ++++- 11 files changed, 1256 insertions(+), 8 deletions(-) diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.kt index d85f538d31d..bc540b9fd24 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.kt @@ -165,6 +165,26 @@ internal object FundingNative { memoText: String?, ) + /** + * Multi-output shielded → shielded transfer, Type 16 (bridges + * `platform_wallet_manager_shielded_transfer_multi`). + * + * [recipientsRaw43] holds `amounts.size` raw 43-byte Orchard addresses + * laid out back to back (length must be `43 * amounts.size`), and + * [amounts] the matching credit values. Each pair becomes its own note; + * repeating the same address funds it with several independent notes. + * [memoText] is attached to every recipient note. + */ + external fun shieldedTransferMulti( + managerHandle: Long, + walletId: ByteArray, + resolverHandle: Long, + account: Int, + recipientsRaw43: ByteArray, + amounts: LongArray, + memoText: String?, + ) + /** * Shielded → Platform unshield, Type 17 (bridges * `platform_wallet_manager_shielded_unshield`). [toPlatformAddress] is a diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt index 940a9b79639..67ed81b6b4c 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt @@ -1644,6 +1644,61 @@ class PlatformWalletManager( } } + /** + * Multi-output shielded → shielded transfer (Type 16). Spends notes from + * [account] on [walletId] and creates ONE note per entry of [outputs] in + * a single atomic transition. + * + * Repeating the same address across entries is allowed and is the point + * of this call: it funds one address with several independent notes, so + * a later spend of that address spends several REAL notes rather than + * one real note plus an Orchard padding dummy (whose nullifier is + * randomly generated and therefore not reproducible offline). + * + * The transition always emits a change note, so the spendable balance + * must strictly exceed the summed amounts plus the fee. The fee grows + * with the output count: the bundle publishes + * `max(spentNotes, outputs.size + 1, 2)` Orchard actions. + * + * @param walletId the 32-byte wallet id. + * @param outputs (raw 43-byte Orchard address, credits) pairs; must be + * non-empty and every amount must be positive. + * @param account the ZIP-32 shielded account to spend from (usually 0). + * @param memo optional UTF-8 memo attached to EVERY recipient note + * (null / empty = no memo; at most 32 UTF-8 bytes). + */ + suspend fun shieldedTransferMulti( + walletId: ByteArray, + outputs: List>, + account: Int = 0, + memo: String? = null, + ): Unit = teardownGate.op { + require(outputs.isNotEmpty()) { "outputs must not be empty" } + require(account >= 0) { "account must be non-negative, got $account" } + outputs.forEachIndexed { index, (recipientRaw43, amount) -> + require(recipientRaw43.size == 43) { + "outputs[$index] address must be exactly 43 bytes, got ${recipientRaw43.size}" + } + require(amount > 0) { "outputs[$index] amount must be positive, got $amount" } + } + val recipientsRaw43 = ByteArray(outputs.size * 43) + outputs.forEachIndexed { index, (recipientRaw43, _) -> + recipientRaw43.copyInto(recipientsRaw43, index * 43) + } + val amounts = LongArray(outputs.size) { outputs[it].second } + mapNativeErrors { + FundingNative.shieldedTransferMulti( + managerHandle, + walletId, + mnemonicResolver.nativeHandle, + account, + recipientsRaw43, + amounts, + memo?.takeIf { it.isNotEmpty() }, + ) + } + } + /** * Shielded → Platform unshield (Type 17) — port of Swift's * `PlatformWalletManager.shieldedUnshield(walletId:account:toPlatformAddress:amount:)` diff --git a/packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs b/packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs index 6d2823008ac..fe6a720f836 100644 --- a/packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs +++ b/packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs @@ -414,6 +414,13 @@ mod tests { identity_id_from_nullifiers(&[real_nullifier]), "the padding action's dummy nullifier must participate in the id derivation" ); + // …which is precisely what `shielded_identity_id_is_reproducible` reports: with one real + // spend the published set contains fresh randomness, so the id cannot be re-derived + // offline (a retry would build a different dummy and thus a different id). + assert!( + !crate::state_transition::identity_create_from_shielded_pool_transition::shielded_identity_id_is_reproducible(1), + "a single-spend bundle is padded, so its id must be reported as NOT reproducible" + ); assert!( result.predicted_fee < DENOMINATION, "predicted fee must leave the new identity a positive balance" @@ -497,5 +504,12 @@ mod tests { identity_id_from_nullifiers(&[nf_a, nf_b]), "with no padding, the published set is exactly the real spends' nullifiers" ); + // …which is precisely what `shielded_identity_id_is_reproducible` reports: with two real + // spends no padding is added, so the id is a pure function of the spent notes and a retry + // re-derives the SAME id. This is the property two-note funding buys. + assert!( + crate::state_transition::identity_create_from_shielded_pool_transition::shielded_identity_id_is_reproducible(2), + "a two-spend bundle needs no padding, so its id must be reported as reproducible" + ); } } diff --git a/packages/rs-dpp/src/shielded/builder/mod.rs b/packages/rs-dpp/src/shielded/builder/mod.rs index ae7b9047752..42151a50189 100644 --- a/packages/rs-dpp/src/shielded/builder/mod.rs +++ b/packages/rs-dpp/src/shielded/builder/mod.rs @@ -43,7 +43,10 @@ pub use identity_create_from_shielded_pool::{ pub use shield_from_asset_lock::build_shield_from_asset_lock_transition; #[cfg(feature = "core_key_wallet")] pub use shield_from_asset_lock::build_shield_from_asset_lock_transition_with_signer; -pub use shielded_transfer::build_shielded_transfer_transition; +pub use shielded_transfer::{ + build_shielded_transfer_transition, build_shielded_transfer_transition_multi, + ShieldedTransferOutput, +}; pub use shielded_withdrawal::build_shielded_withdrawal_transition; pub use unshield::build_unshield_transition; @@ -103,6 +106,36 @@ impl From<&OrchardAddress> for PaymentAddress { } } +/// The number of Orchard actions a `BundleType::DEFAULT` bundle built from `num_spends` spends +/// and `num_outputs` outputs will publish **on the wire**. +/// +/// Every shielded fee predictor MUST size its fee with this function, because consensus prices +/// the fee off the on-wire `actions.len()` (see +/// `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee`, which reads +/// `v0.actions.len()`), and an Orchard action is a *joined* spend/output slot: the action count +/// is `max(num_spends, num_outputs)`, then padded up to Orchard's `MIN_ACTIONS = 2`. +/// +/// The output side matters. A predictor that looks only at the spend count is correct **only** +/// while `num_outputs <= 2`, because `max(n, 1).max(2) == max(n, 2).max(2)`. As soon as a +/// transition publishes three or more outputs (a multi-recipient transfer plus change), a +/// spends-only predictor under-counts and carves a fee below the one consensus computes — fatal +/// for `ShieldedTransfer`, whose `value_balance` must equal the minimum fee **exactly**. +/// +/// This delegates to Orchard's own [`BundleType::num_actions`] rather than re-deriving the rule, +/// so the predictor cannot drift from the builder that actually lays out the bundle. +pub fn shielded_bundle_action_count( + num_spends: usize, + num_outputs: usize, +) -> Result { + BundleType::DEFAULT + .num_actions(num_spends, num_outputs) + .map_err(|e| { + ProtocolError::ShieldedBuildError(format!( + "invalid Orchard bundle shape ({num_spends} spends, {num_outputs} outputs): {e}" + )) + }) +} + /// Serializes an authorized Orchard bundle into the raw fields used by /// state transition constructors. pub fn serialize_authorized_bundle(bundle: &Bundle) -> SerializedBundle { @@ -781,4 +814,57 @@ mod mod_tests { other => panic!("expected the closure's error to propagate, got {:?}", other), } } + + // ------------------------------------------------------------------ + // `shielded_bundle_action_count` — the shared fee-sizing predictor. + // ------------------------------------------------------------------ + + /// The predictor must be `max(num_spends, num_outputs)` padded to Orchard's 2-action + /// minimum — for the OUTPUT side as well as the spend side. The `num_outputs >= 3` rows are + /// the ones a spends-only predictor gets wrong. + #[test] + fn shielded_bundle_action_count_is_max_spends_outputs_padded_to_two() { + for (spends, outputs, expected) in [ + (0usize, 1usize, 2usize), + (1, 1, 2), + (1, 2, 2), + (2, 2, 2), + // Output-dominated shapes: the spend count no longer determines the fee. + (1, 3, 3), + (2, 3, 3), + (1, 4, 4), + (5, 3, 5), + (3, 7, 7), + ] { + let actual = shielded_bundle_action_count(spends, outputs) + .expect("DEFAULT bundles accept any spend/output mix"); + assert_eq!( + actual, expected, + "action count for {spends} spends / {outputs} outputs" + ); + } + } + + /// A real bundle's on-wire `actions.len()` — the number consensus prices the fee off — must + /// equal what the predictor said. Exercised through the output-only builder because it is + /// the cheapest real bundle to construct at several output counts. + #[test] + fn shielded_bundle_action_count_matches_a_real_bundle() { + let recipient = test_orchard_address(); + // (dummy_outputs, total outputs = 1 real + dummies) + for dummies in [0usize, 1, 4] { + let num_outputs = 1 + dummies; + let bundle = + build_output_only_bundle(&recipient, 10_000, [0u8; 36], None, dummies, &TestProver) + .expect("bundle should build"); + let predicted = + shielded_bundle_action_count(0, num_outputs).expect("valid bundle shape"); + assert_eq!( + bundle.actions().len(), + predicted, + "predicted action count must match the real bundle's on-wire count for \ + {num_outputs} outputs" + ); + } + } } diff --git a/packages/rs-dpp/src/shielded/builder/shielded_transfer.rs b/packages/rs-dpp/src/shielded/builder/shielded_transfer.rs index 3c870f37c3c..c2d0d6c6202 100644 --- a/packages/rs-dpp/src/shielded/builder/shielded_transfer.rs +++ b/packages/rs-dpp/src/shielded/builder/shielded_transfer.rs @@ -12,7 +12,10 @@ use crate::state_transition::StateTransition; use crate::ProtocolError; use platform_version::version::PlatformVersion; -use super::{prove_and_sign_bundle, serialize_authorized_bundle, OrchardProver, SpendableNote}; +use super::{ + prove_and_sign_bundle, serialize_authorized_bundle, shielded_bundle_action_count, + OrchardProver, SpendableNote, +}; /// Builds a ShieldedTransfer state transition (shielded pool -> shielded pool). /// @@ -54,9 +57,14 @@ pub fn build_shielded_transfer_transition( ) -> Result<(StateTransition, Credits), ProtocolError> { let total_spent: u64 = spends.iter().map(|s| s.note.value().inner()).sum(); - // Conservative action count: at least (spends, 2) since we always have - // a recipient output and likely a change output. - let num_actions = spends.len().max(2); + // Action count = max(spends, outputs), padded to Orchard's 2-action minimum. This bundle + // publishes at most two outputs (recipient + change), and the no-change case collapses to the + // same number because of that padding: `max(n, 1).max(2) == max(n, 2).max(2)`. So sizing the + // fee for the with-change shape is exact in BOTH branches — see the + // `single_output_transfer_fee_matches_on_wire_action_count` test, which pins the carved fee + // against the bundle's real `actions.len()`. + const MAX_OUTPUTS: usize = 2; // recipient + change + let num_actions = shielded_bundle_action_count(spends.len(), MAX_OUTPUTS)?; // The fee is fixed at the minimum: a transfer's `value_balance` IS the fee and consensus // pins it to exactly this amount (overpayment buys nothing and would leak a distinguishing // fee fingerprint that breaks shielded uniformity). @@ -134,12 +142,464 @@ pub fn build_shielded_transfer_transition( Ok((state_transition, fee)) } +/// One recipient output of a multi-output [`build_shielded_transfer_transition_multi`]. +/// +/// Each entry becomes its own Orchard output — its own note, with its own randomness and so its +/// own (deterministic) nullifier when later spent. Two entries may name the SAME `recipient` +/// address: Orchard derives independent notes regardless, which is exactly how a single transfer +/// funds one address with several notes. +#[derive(Clone, Copy, Debug)] +pub struct ShieldedTransferOutput { + /// Orchard address receiving this note. + pub recipient: OrchardAddress, + /// Value of this note, in credits. + pub amount: u64, + /// 36-byte structured memo (4-byte type tag + 32-byte payload) for this note. + pub memo: [u8; 36], +} + +/// Builds a ShieldedTransfer state transition with **several** recipient outputs in one atomic +/// bundle (shielded pool -> shielded pool). +/// +/// This is the multi-output sibling of [`build_shielded_transfer_transition`]. It exists because +/// some flows must land more than one note in a single transition — most importantly, funding an +/// address with two sub-target notes so that a later spend of that address is forced to spend +/// BOTH of them. +/// +/// # Why more than one output changes the fee +/// +/// An Orchard action is a joined spend/output slot, so the on-wire action count is +/// `max(num_spends, num_outputs)` padded to `MIN_ACTIONS = 2`. A `ShieldedTransfer`'s +/// `value_balance` IS its fee and consensus pins it to `compute_minimum_shielded_fee(actions.len())` +/// **exactly**. With three or more outputs the output side sets the action count, so the fee MUST +/// be sized from it — see [`shielded_bundle_action_count`]. +/// +/// # Deterministic shape +/// +/// This builder ALWAYS emits a change output and therefore requires the spent value to STRICTLY +/// exceed `sum(amounts) + fee`. That makes the output count — and hence the action count and the +/// fee — a pure function of the inputs (`max(spends, recipients + 1, 2)`), with no circular +/// dependency between "is there change?" and "what is the fee?". A caller that spends *exactly* +/// `sum(amounts) + fee` is rejected rather than silently re-shaped into a different action count; +/// note selection always reserves against the same `recipients + 1` floor, so the reserved fee and +/// the carved fee cannot diverge. +/// +/// All recipient outputs and the change output are encrypted with the sender's External-scope OVK, +/// so the sender can recover its own send history from chain data (see +/// [`build_shielded_transfer_transition`]). +/// +/// # Parameters +/// - `spends` - Notes to spend with their Merkle paths +/// - `outputs` - Recipient outputs; must be non-empty +/// - `change_address` - Orchard address for the (always present) change output +/// - `fvk` / `ask` - Full viewing key and spend authorizing key +/// - `anchor` - Sinsemilla root of the note commitment tree +/// - `prover` - Orchard prover (holds the Halo 2 proving key) +/// - `platform_version` - Protocol version +/// +/// Returns the built transition together with the fee (in credits) that was applied. +#[allow(clippy::too_many_arguments)] +pub fn build_shielded_transfer_transition_multi( + spends: Vec, + outputs: &[ShieldedTransferOutput], + change_address: &OrchardAddress, + fvk: &FullViewingKey, + ask: &SpendAuthorizingKey, + anchor: Anchor, + prover: &P, + platform_version: &PlatformVersion, +) -> Result<(StateTransition, Credits), ProtocolError> { + if outputs.is_empty() { + return Err(ProtocolError::ShieldedBuildError( + "a multi-output shielded transfer needs at least one recipient output".to_string(), + )); + } + + // Checked: a crafted output set could otherwise wrap u64 in release builds. + let transfer_total = outputs + .iter() + .try_fold(0u64, |acc, o| acc.checked_add(o.amount)) + .ok_or_else(|| { + ProtocolError::ShieldedBuildError( + "multi-output shielded transfer amounts overflow u64".to_string(), + ) + })?; + let total_spent = spends + .iter() + .try_fold(0u64, |acc, s| acc.checked_add(s.note.value().inner())) + .ok_or_else(|| { + ProtocolError::ShieldedBuildError( + "multi-output shielded transfer total spent value overflows u64".to_string(), + ) + })?; + + // A change output is always emitted (see the doc comment), so the output count — and with it + // the action count and the fee — is fixed before any value arithmetic. + let num_outputs = outputs.len().checked_add(1).ok_or_else(|| { + ProtocolError::ShieldedBuildError("output count overflows usize".to_string()) + })?; + let num_actions = shielded_bundle_action_count(spends.len(), num_outputs)?; + let fee = compute_minimum_shielded_fee(num_actions, platform_version)?; + + let required = transfer_total.checked_add(fee).ok_or_else(|| { + ProtocolError::ShieldedBuildError("fee + transfer amounts overflow u64".to_string()) + })?; + // STRICTLY greater: the change output is unconditional, so it must carry a positive value. + if required >= total_spent { + return Err(ProtocolError::ShieldedBuildError(format!( + "transfer amounts {} + fee {} = {} must be strictly less than the total spendable \ + value {} (a multi-output transfer always emits a change output)", + transfer_total, fee, required, total_spent + ))); + } + let change_amount = total_spent - required; + + let sender_ovk = fvk.to_ovk(Scope::External); + let mut builder = Builder::::new(BundleType::DEFAULT, anchor); + + for spend in spends { + builder + .add_spend(fvk.clone(), spend.note, spend.merkle_path) + .map_err(|e| { + ProtocolError::ShieldedBuildError(format!("failed to add spend: {:?}", e)) + })?; + } + + for output in outputs { + builder + .add_output( + Some(sender_ovk.clone()), + PaymentAddress::from(&output.recipient), + NoteValue::from_raw(output.amount), + output.memo, + ) + .map_err(|e| { + ProtocolError::ShieldedBuildError(format!("failed to add output: {:?}", e)) + })?; + } + + builder + .add_output( + Some(sender_ovk), + PaymentAddress::from(change_address), + NoteValue::from_raw(change_amount), + [0u8; 36], + ) + .map_err(|e| { + ProtocolError::ShieldedBuildError(format!("failed to add change output: {:?}", e)) + })?; + + // ShieldedTransfer has no extra_data in sighash + let bundle = prove_and_sign_bundle(builder, prover, std::slice::from_ref(ask), &[])?; + let sb = serialize_authorized_bundle(&bundle); + + // The fee was predicted before the bundle existed; consensus recomputes it from the ON-WIRE + // action count and demands exact equality. Catch any divergence here (cheap) instead of as an + // opaque rejection after the ~30 s proof. + if sb.actions.len() != num_actions { + return Err(ProtocolError::ShieldedBuildError(format!( + "predicted {} actions but the bundle published {}; the carved fee would not match \ + the consensus minimum", + num_actions, + sb.actions.len() + ))); + } + + let state_transition = ShieldedTransferTransition::try_from_bundle( + sb.actions, + sb.value_balance as u64, + sb.anchor, + sb.proof, + sb.binding_signature, + platform_version, + )?; + Ok((state_transition, fee)) +} + #[cfg(test)] mod tests { use super::*; use crate::shielded::builder::test_helpers::{ test_orchard_address, test_spendable_note, TestProver, }; + use grovedb_commitment_tree::{ + ExtractedNoteCommitment, Hashable, MerkleHashOrchard, MerklePath, SpendingKey, + NOTE_COMMITMENT_TREE_DEPTH, + }; + + /// Two distinct notes witnessed in one two-leaf commitment tree, plus the shared anchor. + /// + /// Each path's level-0 sibling is the other leaf and the upper siblings are shared, so both + /// witnesses compute the SAME root — a consistent anchor the Orchard circuit accepts. (Same + /// construction the identity-create builder's two-spend test uses.) + fn two_spends_in_one_tree( + value_a: u64, + value_b: u64, + fvk: &FullViewingKey, + ) -> (Vec, Anchor, [[u8; 32]; 2]) { + let note_a = test_spendable_note(value_a).note; + let note_b = test_spendable_note(value_b).note; + let cmx_a = ExtractedNoteCommitment::from(note_a.commitment()); + let cmx_b = ExtractedNoteCommitment::from(note_b.commitment()); + + let mut auth_path_a = [MerkleHashOrchard::empty_leaf(); NOTE_COMMITMENT_TREE_DEPTH]; + auth_path_a[0] = MerkleHashOrchard::from_cmx(&cmx_b); + let mut auth_path_b = [MerkleHashOrchard::empty_leaf(); NOTE_COMMITMENT_TREE_DEPTH]; + auth_path_b[0] = MerkleHashOrchard::from_cmx(&cmx_a); + let path_a = MerklePath::from_parts(0, auth_path_a); + let path_b = MerklePath::from_parts(1, auth_path_b); + + let anchor = path_a.root(cmx_a); + assert_eq!( + anchor.to_bytes(), + path_b.root(cmx_b).to_bytes(), + "both witnesses must compute the same anchor" + ); + + let nullifiers = [ + note_a.nullifier(fvk).to_bytes(), + note_b.nullifier(fvk).to_bytes(), + ]; + ( + vec![ + SpendableNote { + note: note_a, + merkle_path: path_a, + }, + SpendableNote { + note: note_b, + merkle_path: path_b, + }, + ], + anchor, + nullifiers, + ) + } + + /// Destructure a built `ShieldedTransfer` into `(actions.len(), value_balance)` — the two + /// fields consensus reads when it recomputes and pins the fee. + fn on_wire_actions_and_value_balance(st: &StateTransition) -> (usize, u64) { + match st { + StateTransition::ShieldedTransfer( + crate::state_transition::shielded_transfer_transition::ShieldedTransferTransition::V0(v0), + ) => (v0.actions.len(), v0.value_balance), + other => panic!("expected a ShieldedTransfer transition, got {other:?}"), + } + } + + /// THE regression pin for the multi-output fee predictor. + /// + /// A `ShieldedTransfer`'s `value_balance` IS its fee, and consensus pins it to + /// `compute_minimum_shielded_fee(actions.len())` EXACTLY (see + /// `validate_minimum_shielded_fee`: `amount_is_pure_fee` rejects both under- and + /// over-payment). With two recipient outputs plus change the bundle publishes THREE actions, + /// so a spends-only predictor (`spends.len().max(2)`) would carve `min_fee(2)` and be + /// rejected on chain. This asserts the carved fee equals `min_fee(on-wire actions.len())` + /// and, explicitly, that it is NOT the 2-action fee. + /// + /// It also pins the two-notes-to-one-address shape: both outputs name the SAME recipient and + /// must still become two DISTINCT notes (distinct commitments), which is what makes a later + /// spend of that address spend two real notes rather than one real note plus a random dummy. + #[test] + fn multi_output_transfer_fee_matches_on_wire_action_count() { + let platform_version = PlatformVersion::latest(); + let sk = SpendingKey::from_bytes([42u8; 32]).expect("valid spending key"); + let fvk = FullViewingKey::from(&sk); + let ask = SpendAuthorizingKey::from(&sk); + let change_address = test_orchard_address(); + let recipient = test_orchard_address(); + + let (spends, anchor, _) = two_spends_in_one_tree(6_000_000_000, 7_000_000_000, &fvk); + + // The two-note invite funding shape: D split into floor(D/2) + ceil(D/2), both to the + // SAME one-time address, each strictly below D. + const D: u64 = 3_000_000_000; // 0.03 DASH in credits + let outputs = vec![ + ShieldedTransferOutput { + recipient, + amount: D / 2, + memo: [0u8; 36], + }, + ShieldedTransferOutput { + recipient, + amount: D - D / 2, + memo: [0u8; 36], + }, + ]; + + let (st, fee) = build_shielded_transfer_transition_multi( + spends, + &outputs, + &change_address, + &fvk, + &ask, + anchor, + &TestProver, + platform_version, + ) + .expect("a two-spend, three-output transfer must build"); + + let (num_actions, value_balance) = on_wire_actions_and_value_balance(&st); + assert_eq!( + num_actions, 3, + "2 spends + 3 outputs (2 recipients + change) must publish max(2,3) = 3 actions" + ); + + let expected_fee = compute_minimum_shielded_fee(num_actions, platform_version) + .expect("fee computation should not overflow"); + assert_eq!( + fee, expected_fee, + "the carved fee must equal compute_minimum_shielded_fee(on-wire actions.len())" + ); + assert_eq!( + value_balance, expected_fee, + "value_balance IS the fee and consensus pins it to the minimum for the on-wire \ + action count exactly" + ); + + // The bug this fixes: the old spends-only predictor would have carved the 2-action fee. + let two_action_fee = compute_minimum_shielded_fee(2, platform_version) + .expect("fee computation should not overflow"); + assert!( + expected_fee > two_action_fee, + "a 3-action bundle must cost strictly more than a 2-action one, otherwise this test \ + cannot detect the under-count" + ); + assert_ne!( + fee, two_action_fee, + "a spends-only fee predictor would carve the 2-action fee and be rejected on chain" + ); + + // Two outputs to the SAME address are still two distinct notes. + let commitments: Vec<[u8; 32]> = match &st { + StateTransition::ShieldedTransfer( + crate::state_transition::shielded_transfer_transition::ShieldedTransferTransition::V0(v0), + ) => v0.actions.iter().map(|a| a.cmx).collect(), + _ => unreachable!(), + }; + let unique: std::collections::BTreeSet<[u8; 32]> = commitments.iter().copied().collect(); + assert_eq!( + unique.len(), + commitments.len(), + "every published note commitment must be distinct, including the two notes paid to \ + the same address" + ); + } + + /// The single-output builder's fee must ALSO equal `min_fee(on-wire actions.len())`. Its + /// output count (recipient + change = 2) can never exceed Orchard's 2-action minimum, so + /// routing it through `shielded_bundle_action_count` is numerically a no-op — this pins that + /// claim against a real bundle so the shared helper cannot regress it. + #[test] + fn single_output_transfer_fee_matches_on_wire_action_count() { + let platform_version = PlatformVersion::latest(); + let sk = SpendingKey::from_bytes([42u8; 32]).expect("valid spending key"); + let fvk = FullViewingKey::from(&sk); + let ask = SpendAuthorizingKey::from(&sk); + let change_address = test_orchard_address(); + let recipient = test_orchard_address(); + + let (spends, anchor, _) = two_spends_in_one_tree(6_000_000_000, 7_000_000_000, &fvk); + + let (st, fee) = build_shielded_transfer_transition( + spends, + &recipient, + 3_000_000_000, + &change_address, + &fvk, + &ask, + anchor, + &TestProver, + [0u8; 36], + platform_version, + ) + .expect("a two-spend, two-output transfer must build"); + + let (num_actions, value_balance) = on_wire_actions_and_value_balance(&st); + assert_eq!( + num_actions, 2, + "2 spends + 2 outputs must publish 2 actions" + ); + let expected_fee = compute_minimum_shielded_fee(num_actions, platform_version) + .expect("fee computation should not overflow"); + assert_eq!(fee, expected_fee); + assert_eq!( + value_balance, expected_fee, + "value_balance must equal the minimum fee for the on-wire action count exactly" + ); + } + + #[test] + fn multi_output_transfer_rejects_empty_output_set() { + let platform_version = PlatformVersion::latest(); + let sk = SpendingKey::from_bytes([42u8; 32]).expect("valid sk"); + let fvk = FullViewingKey::from(&sk); + let ask = SpendAuthorizingKey::from(&sk); + let change_address = test_orchard_address(); + + let err = build_shielded_transfer_transition_multi( + vec![test_spendable_note(u64::MAX / 2)], + &[], + &change_address, + &fvk, + &ask, + Anchor::empty_tree(), + &TestProver, + platform_version, + ) + .expect_err("an empty output set must be rejected"); + assert!( + err.to_string().contains("at least one recipient output"), + "unexpected error: {err}" + ); + } + + /// The multi-output builder always emits a change output, so it requires the spent value to + /// STRICTLY exceed `sum(amounts) + fee`. Spending exactly that much is rejected rather than + /// silently re-shaped into a different (and differently priced) action count. + #[test] + fn multi_output_transfer_requires_strictly_positive_change() { + let platform_version = PlatformVersion::latest(); + let sk = SpendingKey::from_bytes([42u8; 32]).expect("valid sk"); + let fvk = FullViewingKey::from(&sk); + let ask = SpendAuthorizingKey::from(&sk); + let change_address = test_orchard_address(); + let recipient = test_orchard_address(); + + // One spend + 3 outputs (2 recipients + change) → max(1, 3) = 3 actions. + let fee = compute_minimum_shielded_fee(3, platform_version).expect("fee"); + let amount = 1_000_000u64; + // Exactly `sum + fee` — the boundary that must be rejected. + let note = test_spendable_note(2 * amount + fee); + let outputs = vec![ + ShieldedTransferOutput { + recipient, + amount, + memo: [0u8; 36], + }, + ShieldedTransferOutput { + recipient, + amount, + memo: [0u8; 36], + }, + ]; + + let err = build_shielded_transfer_transition_multi( + vec![note], + &outputs, + &change_address, + &fvk, + &ask, + Anchor::empty_tree(), + &TestProver, + platform_version, + ) + .expect_err("spending exactly sum + fee must be rejected"); + assert!( + err.to_string().contains("strictly less than"), + "unexpected error: {err}" + ); + } #[test] fn test_shielded_transfer_insufficient_funds() { diff --git a/packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/mod.rs b/packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/mod.rs index 0e11c927edb..079a66c698d 100644 --- a/packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/mod.rs +++ b/packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/mod.rs @@ -78,6 +78,35 @@ pub fn identity_id_from_nullifiers(nullifiers: &[[u8; 32]]) -> Identifier { Identifier::new(hash_double(buf)) } +/// Whether the identity id an `IdentityCreateFromShieldedPool` will publish can be reproduced +/// OFFLINE, before (or after) the bundle is built, from the spent-note set alone. +/// +/// The id is derived over the bundle's PUBLISHED nullifiers — every action's nullifier, padding +/// included. Orchard's `BundleType::DEFAULT` pads any bundle to `MIN_ACTIONS = 2`, and a padding +/// action carries a **randomly generated** dummy nullifier. So: +/// +/// - `num_real_spends >= 2` — no padding is added, every published nullifier is the deterministic +/// nullifier of a real note, and the id is a pure function of the spent notes. It can be +/// predicted before building and RE-derived identically on a later retry. +/// - `num_real_spends < 2` — the bundle is padded and at least one published nullifier is fresh +/// randomness. The id is unpredictable beforehand and, critically, **not reproducible**: a retry +/// builds a different dummy and therefore a different identity id. +/// +/// Any flow that must recognise "this identity is the one my earlier attempt created" — idempotent +/// claim recovery being the motivating case — MUST gate on this. When it returns `false` the +/// caller cannot derive an expected id and has to treat recovery as unreliable rather than +/// computing an id that will not match. Guarding on the *note count* is the cheapest correct check: +/// it needs no chain lookup and is decided before any proving work. +/// +/// The corollary drives note layout: funding an address with two sub-target notes (instead of one +/// note covering the whole target) forces a later spend of that address to select BOTH — greedy +/// largest-first selection cannot stop after one note that does not cover the target — which keeps +/// the padding action, and its random nullifier, out of the bundle entirely. +pub fn shielded_identity_id_is_reproducible(num_real_spends: usize) -> bool { + // Mirrors Orchard's `MIN_ACTIONS = 2`: at or above it, no padding action is appended. + num_real_spends >= 2 +} + /// Convenience wrapper around [`identity_id_from_nullifiers`] that extracts the nullifiers from a /// slice of serialized Orchard actions. Shared by the SDK builder and the consensus re-derivation /// check so both compute the id identically. diff --git a/packages/rs-platform-wallet-ffi/src/shielded_send.rs b/packages/rs-platform-wallet-ffi/src/shielded_send.rs index 789995526a7..d2b283f7926 100644 --- a/packages/rs-platform-wallet-ffi/src/shielded_send.rs +++ b/packages/rs-platform-wallet-ffi/src/shielded_send.rs @@ -350,6 +350,153 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_transfer( map_spend_result(result, "shielded transfer") } +/// Defensive upper bound on the recipient count of a multi-output shielded transfer. +/// +/// This is an FFI sanity bound, not the protocol limit: it stops an absurd or corrupt +/// `num_recipients` from driving a huge allocation before anything else can reject it. The real +/// ceiling is the 20 KiB state-transition size limit, which admits roughly six Orchard actions — +/// so a legitimate caller stays far below this. +const MAX_SHIELDED_TRANSFER_RECIPIENTS: usize = 16; + +/// Send a shielded → shielded transfer with SEVERAL outputs in one +/// atomic transition. +/// +/// Multi-output sibling of +/// [`platform_wallet_manager_shielded_transfer`]. `recipients_raw_43` +/// is `num_recipients` raw 43-byte Orchard payment addresses laid out +/// back to back, and `amounts` is the matching array of +/// `num_recipients` credit amounts. Each pair becomes its own note. +/// +/// Repeating the same address is allowed and is the primary use: it +/// funds one address with several independent notes, so a later spend +/// of that address spends several REAL notes rather than one real note +/// plus an Orchard padding dummy (whose nullifier is randomly +/// generated and so cannot be reproduced offline). +/// +/// `memo_text` is attached to EVERY recipient note (same encoding and +/// 32-byte UTF-8 limit as the single-output call). The change note +/// always carries the empty memo. +/// +/// A multi-output transfer always emits a change output, so the spent +/// value must strictly exceed `sum(amounts) + fee`. +/// +/// `mnemonic_resolver_handle` supplies the per-operation Orchard spend +/// authority (see `platform_wallet_manager_shielded_transfer`). +/// +/// # Safety +/// - `wallet_id_bytes` must point to 32 readable bytes. +/// - `mnemonic_resolver_handle` must come from +/// `dash_sdk_mnemonic_resolver_create` and outlive this call; the +/// caller retains ownership. +/// - `recipients_raw_43` must point to `num_recipients * 43` readable +/// bytes and `amounts` to `num_recipients` readable `u64`s. +/// - `memo_text`, when non-null, must be a valid NUL-terminated UTF-8 +/// C string for the duration of the call. +#[no_mangle] +pub unsafe extern "C" fn platform_wallet_manager_shielded_transfer_multi( + handle: Handle, + wallet_id_bytes: *const u8, + mnemonic_resolver_handle: *mut MnemonicResolverHandle, + account: u32, + recipients_raw_43: *const u8, + amounts: *const u64, + num_recipients: usize, + memo_text: *const c_char, +) -> PlatformWalletFFIResult { + check_ptr!(wallet_id_bytes); + check_ptr!(mnemonic_resolver_handle); + check_ptr!(recipients_raw_43); + check_ptr!(amounts); + + if num_recipients == 0 { + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + "num_recipients must be at least 1".to_string(), + ); + } + if num_recipients > MAX_SHIELDED_TRANSFER_RECIPIENTS { + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + format!( + "num_recipients {num_recipients} exceeds the maximum of \ + {MAX_SHIELDED_TRANSFER_RECIPIENTS}" + ), + ); + } + + let mut wallet_id = [0u8; 32]; + std::ptr::copy_nonoverlapping(wallet_id_bytes, wallet_id.as_mut_ptr(), 32); + + let amount_slice = std::slice::from_raw_parts(amounts, num_recipients); + let mut outputs: Vec<([u8; 43], u64)> = Vec::with_capacity(num_recipients); + for (index, &amount) in amount_slice.iter().enumerate() { + if amount == 0 { + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + format!("amount at index {index} must be positive"), + ); + } + let mut recipient = [0u8; 43]; + std::ptr::copy_nonoverlapping( + recipients_raw_43.add(index * 43), + recipient.as_mut_ptr(), + 43, + ); + outputs.push((recipient, amount)); + } + + // Decode the optional memo before touching wallet state so a malformed memo fails fast. + let memo_str = if memo_text.is_null() { + None + } else { + match CStr::from_ptr(memo_text).to_str() { + Ok(s) => Some(s), + Err(e) => { + return PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorUtf8Conversion, + format!("memo_text is not valid UTF-8: {e}"), + ); + } + } + }; + let memo = match encode_memo_text(memo_str) { + Ok(m) => m, + Err(result) => return result, + }; + + let (wallet, coordinator) = match resolve_wallet_and_coordinator(handle, &wallet_id) { + Ok(p) => p, + Err(result) => return result, + }; + + let seed = match crate::identity_keys_from_mnemonic::resolve_seed_from_resolver( + mnemonic_resolver_handle, + &wallet_id, + ) { + Ok(seed) => seed, + Err(result) => return result, + }; + + // Prove on a worker thread with an 8 MB stack (see + // `platform_wallet_manager_shielded_transfer`). + let result = block_on_worker(async move { + let prover = CachedOrchardProver::new(); + let r = wallet + .shielded_transfer_multi_to( + &coordinator, + seed.as_ref(), + account, + &outputs, + memo, + &prover, + ) + .await; + poke_sync_on_unconfirmed(&r, handle); + r + }); + map_spend_result(result, "shielded multi-output transfer") +} + /// Unshield: spend shielded notes and send `amount` credits to a /// platform address. /// diff --git a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs index cfb310ea597..75ebc0f5356 100644 --- a/packages/rs-platform-wallet/src/wallet/platform_wallet.rs +++ b/packages/rs-platform-wallet/src/wallet/platform_wallet.rs @@ -1138,6 +1138,58 @@ impl PlatformWallet { .await } + /// Multi-output sibling of [`shielded_transfer_to`](Self::shielded_transfer_to): spend + /// `account`'s notes and create SEVERAL notes in one atomic Type-16 transition. + /// + /// `outputs` pairs each recipient (43 raw Orchard address bytes) with its amount in credits. + /// Repeating the same address is allowed and is the point of this call: it funds one address + /// with several independent notes, so a later spend of that address spends several REAL + /// notes instead of one real note plus an Orchard padding dummy (whose nullifier is random + /// and therefore not reproducible offline). + /// + /// `memo` is attached to every recipient note. `seed` supplies the transient spend authority + /// (see [`shielded_transfer_to`](Self::shielded_transfer_to)). + #[cfg(feature = "shielded")] + #[allow(clippy::too_many_arguments)] + pub async fn shielded_transfer_multi_to( + &self, + coordinator: &Arc, + seed: &[u8], + account: u32, + outputs: &[([u8; 43], u64)], + memo: [u8; 36], + prover: P, + ) -> Result<(), PlatformWalletError> { + let keyset = self.derive_spend_keyset(seed, account).await?; + let parsed: Vec<(grovedb_commitment_tree::PaymentAddress, u64)> = outputs + .iter() + .map(|(raw, amount)| { + Option::::from( + grovedb_commitment_tree::PaymentAddress::from_raw_address_bytes(raw), + ) + .map(|addr| (addr, *amount)) + .ok_or_else(|| { + PlatformWalletError::ShieldedBuildError( + "invalid Orchard payment address bytes".to_string(), + ) + }) + }) + .collect::>()?; + + super::shielded::operations::transfer_multi( + &self.sdk, + coordinator.store(), + Some(&self.persister), + self.wallet_id, + &keyset, + account, + &parsed, + memo, + &prover, + ) + .await + } + /// Unshield from `account`'s notes to a transparent platform /// address (`"dash1…"` / `"tdash1…"`). Parsed via /// `PlatformAddress::from_bech32m_string`; the recipient's HRP is diff --git a/packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs b/packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs index b4e0b7ea81f..6f27f628526 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs @@ -383,6 +383,86 @@ mod tests { assert_eq!(exact_fee, min_fee_2); } + /// A multi-output transfer reserves against `recipients + 1` outputs, because the bundle + /// publishes `max(spends, recipients + 1, 2)` actions and a ShieldedTransfer's + /// `value_balance` must equal `compute_minimum_shielded_fee(actions.len())` EXACTLY. If the + /// reservation used the 2-action floor instead, it would under-reserve and the builder's + /// carved fee would not match what was reserved. + #[test] + fn test_select_notes_with_fee_reserves_multi_output_action_floor() { + let platform_version = PlatformVersion::latest(); + // Two recipient notes + change = 3 outputs → a 3-action floor. + let min_actions = 3; + let min_fee_3 = compute_minimum_shielded_fee(3, platform_version).expect("fee"); + let min_fee_2 = compute_minimum_shielded_fee(2, platform_version).expect("fee"); + assert!( + min_fee_3 > min_fee_2, + "a 3-action bundle must cost more than a 2-action one" + ); + + let amount = 3_000_000_000u64; // the invite denomination, split across two notes + // A single note covering amount + the 3-action fee (plus change). + let notes = vec![test_note(amount + min_fee_3 + 1, 0)]; + + let (selected, _total, exact_fee) = select_notes_with_fee( + ¬es, + amount, + min_actions, + ShieldedFeeKind::Base, + platform_version, + ) + .expect("selection ok"); + + assert_eq!(selected.len(), 1); + assert_eq!( + exact_fee, min_fee_3, + "one spend but three outputs must reserve the 3-action fee, not the 2-action floor" + ); + } + + /// Two sub-denomination notes on one key are STRUCTURALLY forced to both be selected when + /// the spend targets the full denomination: the greedy selector takes the largest note first + /// and only stops once the accumulated value covers the target, and neither note alone can. + /// This is what removes Orchard's padding action (and its random, unreproducible dummy + /// nullifier) from the claim bundle. + #[test] + fn test_two_sub_denomination_notes_are_both_selected() { + // The two shipped invite denominations, each split floor(D/2) + ceil(D/2). + for denomination in [3_000_000_000u64, 25_000_000_000u64] { + let lo = denomination / 2; + let hi = denomination - lo; + assert!( + lo < denomination && hi < denomination, + "each half must be strictly below the denomination" + ); + + let notes = vec![test_note(hi, 0), test_note(lo, 1)]; + // The claim targets the denomination exactly (fee metered from it, not added). + let selected = select_notes(¬es, denomination, 0).expect("selection ok"); + assert_eq!( + selected.len(), + 2, + "both sub-denomination notes must be selected for denomination {denomination}" + ); + let total: u64 = selected.iter().map(|n| n.value).sum(); + assert_eq!(total, denomination); + } + } + + /// Contrast: a SINGLE note worth the whole denomination stops the greedy selector after one + /// note — the one-note invite shape that leaves Orchard to pad the bundle with a dummy. + #[test] + fn test_single_full_denomination_note_selects_alone() { + let denomination = 3_000_000_000u64; + let notes = vec![test_note(denomination, 0)]; + let selected = select_notes(¬es, denomination, 0).expect("selection ok"); + assert_eq!( + selected.len(), + 1, + "a single full-denomination note covers the target alone, so the bundle needs padding" + ); + } + #[test] fn test_select_notes_with_fee_uses_actual_action_count() { let platform_version = PlatformVersion::latest(); diff --git a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs index a79ed4e2d16..d3c3acff301 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs @@ -51,8 +51,9 @@ use dpp::identity::{Identity, IdentityPublicKey}; use dpp::prelude::Identifier; use dpp::shielded::builder::{ build_identity_create_from_shielded_pool_transition, build_shield_transition, - build_shielded_transfer_transition, build_shielded_withdrawal_transition, - build_unshield_transition, OrchardProver, SpendableNote, + build_shielded_transfer_transition, build_shielded_transfer_transition_multi, + build_shielded_withdrawal_transition, build_unshield_transition, OrchardProver, + ShieldedTransferOutput, SpendableNote, }; use dpp::shielded::compute_minimum_shielded_fee; use dpp::state_transition::proof_result::StateTransitionProofResult; @@ -989,6 +990,198 @@ pub async fn transfer( } } +/// Transfer funds privately from `account`'s shielded notes to +/// SEVERAL Orchard outputs in one atomic transition (Type 16). +/// +/// Multi-output sibling of [`transfer`]. Each `(address, amount)` pair becomes its own note — +/// including when several pairs name the SAME address, which is how one transition funds an +/// address with more than one note. +/// +/// `memo` is attached to every recipient note (the change note always carries the empty memo). +/// +/// # Fee sizing +/// +/// The bundle publishes `max(spends, recipients + 1, 2)` actions and a `ShieldedTransfer`'s +/// `value_balance` must equal `compute_minimum_shielded_fee(actions.len())` EXACTLY. Note +/// selection therefore reserves against `recipients.len() + 1` outputs — the same floor the +/// builder sizes its fee from — so the reserved and carved fees cannot diverge. +#[allow(clippy::too_many_arguments)] +pub async fn transfer_multi( + sdk: &Arc, + store: &Arc>, + persister: Option<&WalletPersister>, + wallet_id: WalletId, + keys: &OrchardKeySet, + account: u32, + outputs: &[(PaymentAddress, u64)], + memo: [u8; 36], + prover: &P, +) -> Result<(), PlatformWalletError> { + if outputs.is_empty() { + return Err(PlatformWalletError::ShieldedBuildError( + "a multi-output shielded transfer needs at least one recipient output".to_string(), + )); + } + + let builder_outputs: Vec = outputs + .iter() + .map(|(addr, amount)| { + Ok(ShieldedTransferOutput { + recipient: payment_address_to_orchard(addr)?, + amount: *amount, + memo, + }) + }) + .collect::>()?; + + let total_amount = outputs + .iter() + .try_fold(0u64, |acc, (_, amount)| acc.checked_add(*amount)) + .ok_or_else(|| { + PlatformWalletError::ShieldedBuildError( + "multi-output shielded transfer amounts overflow u64".to_string(), + ) + })?; + + let views = keys.viewing_keys(); + let change_addr = default_orchard_address(&views)?; + let id = SubwalletId::new(wallet_id, account); + + // Reserve against the SAME output count the builder sizes its fee from: every recipient + // output plus the unconditional change output. + let num_outputs = builder_outputs.len() + 1; + let (selected_notes, total_input, exact_fee) = reserve_unspent_notes( + sdk, + store, + id, + total_amount, + num_outputs, + ShieldedFeeKind::Base, + ) + .await?; + + info!( + account, + credits = total_amount, + note_outputs = builder_outputs.len(), + fee = exact_fee, + inputs = selected_notes.len(), + total_input, + "Shielded multi-output transfer" + ); + + let mut pending_entry = None; + let result = async { + let (spends, anchor) = extract_spends_and_anchor(sdk, store, &selected_notes).await?; + let anchor_bytes = anchor.to_bytes(); + + let (state_transition, fee_used) = build_shielded_transfer_transition_multi( + spends, + &builder_outputs, + &change_addr, + &keys.full_viewing_key, + &keys.spend_auth_key, + anchor, + prover, + sdk.version(), + ) + .map_err(|e| PlatformWalletError::ShieldedBuildError(e.to_string()))?; + debug_assert_eq!( + fee_used, exact_fee, + "builder fee must match the reserved minimum fee" + ); + + // One activity row for the whole transition. The counterparty is only meaningful when + // every output lands on the same address (the fund-an-address-with-N-notes shape); a + // genuine multi-recipient send has no single counterparty to record. + let counterparty = outputs + .first() + .filter(|(first, _)| outputs.iter().all(|(a, _)| a == first)) + .map(|(addr, _)| addr.to_raw_address_bytes().to_vec()); + + pending_entry = record_pending_activity( + store, + persister, + wallet_id, + id, + &views, + LiveEntryParams { + kind: ShieldedActivityKind::Sent, + direction: ShieldedDirection::Out, + amount: total_amount, + fee: Some(fee_used), + counterparty, + memo: non_zero_memo(&memo), + actions: shielded_actions(&state_transition), + spent_notes: &selected_notes, + }, + ) + .await; + arm_pending_release(store, id, anchor_bytes, &pending_entry, &selected_notes).await; + + trace!("Shielded multi-output transfer: state transition built, broadcasting..."); + broadcast_shielded_spend_with_redrive( + sdk, + store, + id, + &pending_entry, + anchor_bytes, + &selected_notes, + &state_transition, + "transfer_multi", + ) + .await + } + .await; + + match result { + Ok(()) => { + record_activity_status( + store, + persister, + wallet_id, + id, + &pending_entry, + ShieldedActivityStatus::Confirmed, + None, + ) + .await; + if let Err(e) = finalize_pending(store, persister, wallet_id, id, &selected_notes).await + { + warn!( + account, + error = %e, + "Shielded multi-output transfer broadcast succeeded but local spent-state \ + update failed; will heal on next sync" + ); + } + info!( + account, + credits = total_amount, + "Shielded multi-output transfer broadcast succeeded" + ); + Ok(()) + } + // Ambiguous post-broadcast confirmation failure: leave the reservation (and the Pending + // activity row) in place — a later scan flips it to Confirmed (see `unshield`). + Err(e @ PlatformWalletError::ShieldedSpendUnconfirmed { .. }) => Err(e), + Err(e) => { + record_activity_status( + store, + persister, + wallet_id, + id, + &pending_entry, + ShieldedActivityStatus::Failed, + None, + ) + .await; + cancel_pending(store, id, &selected_notes).await; + Err(e) + } + } +} + // ------------------------------------------------------------------------- // Withdraw: shielded pool -> Core L1 address (Type 19) // ------------------------------------------------------------------------- diff --git a/packages/rs-unified-sdk-jni/src/funding.rs b/packages/rs-unified-sdk-jni/src/funding.rs index f8dc82f050a..a2035b4f33a 100644 --- a/packages/rs-unified-sdk-jni/src/funding.rs +++ b/packages/rs-unified-sdk-jni/src/funding.rs @@ -41,7 +41,7 @@ use crate::pubkey_rows::decode_registration_pubkeys_blob; use crate::support::{guard, take_pwffi_error, throw_sdk_exception, JVM}; -use jni::objects::{GlobalRef, JByteArray, JClass, JObject, JString}; +use jni::objects::{GlobalRef, JByteArray, JClass, JLongArray, JObject, JString}; use jni::sys::{jboolean, jint, jlong, JNI_FALSE, JNI_TRUE}; use jni::JNIEnv; use platform_wallet_ffi::handle::Handle; @@ -845,6 +845,118 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_FundingNative_shielde }) } +/// Multi-output shielded → shielded transfer (Type 16) — bridges +/// `platform_wallet_manager_shielded_transfer_multi`. +/// +/// `recipientsRaw43` is `amounts.length` raw 43-byte Orchard addresses laid +/// out back to back (so its length must be `43 * amounts.length`), and +/// `amounts` holds the matching credit amounts. Each pair becomes its own +/// note; repeating the same address funds that address with several +/// independent notes. `memoText` is attached to every recipient note. +#[no_mangle] +pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_FundingNative_shieldedTransferMulti( + mut env: JNIEnv, + _class: JClass, + manager_handle: jlong, + wallet_id: JByteArray, + resolver_handle: jlong, + account: jint, + recipients_raw43: JByteArray, + amounts: JLongArray, + memo_text: JString, +) { + guard(&mut env, (), |env| { + if account < 0 { + throw_sdk_exception(env, 1, "account must be non-negative"); + return; + } + let Some(wid) = read_id32(env, &wallet_id, "walletId") else { + return; + }; + if recipients_raw43.is_null() { + throw_sdk_exception(env, 1, "recipientsRaw43 byte[] was null"); + return; + } + if amounts.is_null() { + throw_sdk_exception(env, 1, "amounts long[] was null"); + return; + } + let recipients = match env.convert_byte_array(&recipients_raw43) { + Ok(b) => b, + Err(_) => { + let _ = env.exception_clear(); + throw_sdk_exception(env, 1, "recipientsRaw43 byte[] was invalid"); + return; + } + }; + let amount_len = match env.get_array_length(&amounts) { + Ok(n) if n >= 0 => n as usize, + _ => { + let _ = env.exception_clear(); + throw_sdk_exception(env, 1, "amounts long[] was invalid"); + return; + } + }; + if amount_len == 0 { + throw_sdk_exception(env, 1, "amounts must contain at least one entry"); + return; + } + if recipients.len() != amount_len * 43 { + throw_sdk_exception( + env, + 1, + &format!( + "recipientsRaw43 must be 43 bytes per amount ({} expected), got {}", + amount_len * 43, + recipients.len() + ), + ); + return; + } + let mut amount_buf = vec![0i64; amount_len]; + if env + .get_long_array_region(&amounts, 0, &mut amount_buf) + .is_err() + { + let _ = env.exception_clear(); + throw_sdk_exception(env, 1, "amounts long[] could not be read"); + return; + } + // Reject sign errors at the boundary — negatives would otherwise bit-cast to huge + // unsigned values (never clamp). + for (index, &amount) in amount_buf.iter().enumerate() { + if amount <= 0 { + throw_sdk_exception( + env, + 1, + &format!("amounts[{index}] must be positive, got {amount}"), + ); + return; + } + } + let amounts_u64: Vec = amount_buf.iter().map(|&a| a as u64).collect(); + + let memo = match read_cstring_opt(env, &memo_text, "memoText") { + Ok(m) => m, + Err(()) => return, + }; + let memo_ptr = memo.as_ref().map_or(ptr::null(), |c| c.as_ptr()); + let result = unsafe { + platform_wallet_ffi::platform_wallet_manager_shielded_transfer_multi( + manager_handle as Handle, + wid.as_ptr(), + resolver_handle as *mut MnemonicResolverHandle, + account as u32, + recipients.as_ptr(), + amounts_u64.as_ptr(), + amount_len, + memo_ptr, + ) + }; + let _ = take_pwffi_error(env, result); + }) +} + /// Shielded → Platform unshield (Type 17) — bridges /// `platform_wallet_manager_shielded_unshield`. /// From 9f11d66c752c73c24bc5d393a961b74a1acdd050 Mon Sep 17 00:00:00 2001 From: bfoss765 <38437574+bfoss765@users.noreply.github.com> Date: Wed, 5 Aug 2026 18:20:03 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(shielded):=20review-gate=20round=20?= =?UTF-8?q?=E2=80=94=20action-limit=20gate,=20strict-change=20note=20selec?= =?UTF-8?q?tion,=20FFI=20panic=20guard,=20JNI=20allocation=20bound?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the four findings on dashpay/platform#4301 (2 blocking, 2 suggestions). ## BLOCKING — reject bundles over the consensus action limit before proving `shielded_bundle_action_count` computed the on-wire action count but never compared it with `platform_version.system_limits.max_shielded_transition_actions` (16). `ShieldedTransferTransitionV0::validate_structure` rejects anything above that limit, while `try_from_bundle` performs no structural validation — so the FFI's 16 recipients (17 outputs once the unconditional change output is added, therefore >= 17 actions), or a fragmented wallet's spend count, would build and prove a bundle (~30 s of Halo 2) that consensus is guaranteed to reject. The helper now takes `platform_version` and validates the computed count. Because the count is `max(spends, outputs)` padded to 2, the single comparison bounds BOTH sides. Both transfer builders route through it, so the rejection happens before any spend is added to the Orchard builder. ## BLOCKING — reserve enough input to guarantee positive change `select_notes_with_fee` accepted `total_input == amount + exact_fee`, but `build_shielded_transfer_transition_multi` emits an unconditional change output and rejects equality. With notes `[amount + fee, 1]`, largest-first selection reserved the exact-coverage note alone and the build then failed even though taking the remaining credit would have satisfied the builder. Note selection now carries a `ChangeRequirement`. `StrictlyPositive` (the multi-output transfer) folds one credit into the selection target and into the sufficiency test on every convergence iteration, so the strict postcondition holds against the RE-COMPUTED fee after an added note changes the action count. The other three spends keep `Optional` — their builders accept zero change. The returned fee stays the pure consensus fee the builder carves. ## SUGGESTION — catch panics before crossing the C ABI A panic cannot unwind through `extern "C"`: it aborts the process before the JNI layer's `support::guard` can turn it into a Java exception. `block_on_worker` makes this reachable — it `.expect`s on the tokio `JoinError`, so a panicking proving task re-panics inside the export. The multi-output transfer export's body moved into a plain Rust function invoked under `catch_unwind`. A caught panic maps to `ErrorShieldedSpendUnconfirmed`, whose contract is exactly the conservative one required: the spend may have been broadcast, the reservation stays, and the host must not auto-retry. ## SUGGESTION — enforce the recipient bound before allocating The JNI adapter copied the whole Java recipient array and both amount buffers before the native ceiling could reject the call. It now reads both array LENGTHS first (header reads, no allocation), rejects counts above `MAX_SHIELDED_TRANSFER_RECIPIENTS` (now public so the bridges share the constant instead of duplicating the literal), and only then converts — so every allocation is bounded by the ceiling, not by the caller. `PlatformWalletManager.shieldedTransferMulti` mirrors the check before it flattens its own buffers. Tests: action-count boundary passes / one over fails fast from both the output and spend sides (helper + builder level); the `[amount + fee, 1]` exact-fit case now selects both notes, one credit short reports the extra credit in `required`, and the strict floor survives fee re-convergence; the FFI panic guard maps a panic to the unconfirmed contract and is transparent otherwise. --- .../dashsdk/wallet/PlatformWalletManager.kt | 18 +- packages/rs-dpp/src/shielded/builder/mod.rs | 84 ++++++- .../src/shielded/builder/shielded_transfer.rs | 143 ++++++++++- .../src/shielded_send.rs | 129 +++++++++- .../src/wallet/shielded/note_selection.rs | 227 +++++++++++++++++- .../src/wallet/shielded/operations.rs | 53 +++- packages/rs-unified-sdk-jni/src/funding.rs | 53 +++- 7 files changed, 667 insertions(+), 40 deletions(-) diff --git a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt index 67ed81b6b4c..362e5dc4bae 100644 --- a/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt +++ b/packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt @@ -1662,7 +1662,8 @@ class PlatformWalletManager( * * @param walletId the 32-byte wallet id. * @param outputs (raw 43-byte Orchard address, credits) pairs; must be - * non-empty and every amount must be positive. + * non-empty, hold at most 16 entries (the native ceiling), and every + * amount must be positive. * @param account the ZIP-32 shielded account to spend from (usually 0). * @param memo optional UTF-8 memo attached to EVERY recipient note * (null / empty = no memo; at most 32 UTF-8 bytes). @@ -1674,6 +1675,12 @@ class PlatformWalletManager( memo: String? = null, ): Unit = teardownGate.op { require(outputs.isNotEmpty()) { "outputs must not be empty" } + // Mirror the native ceiling BEFORE flattening: the arrays built below are sized by + // `outputs.size`, and the native layer would reject an oversized call anyway — after + // this side had already allocated for it. + require(outputs.size <= MAX_SHIELDED_TRANSFER_RECIPIENTS) { + "outputs must hold at most $MAX_SHIELDED_TRANSFER_RECIPIENTS entries, got ${outputs.size}" + } require(account >= 0) { "account must be non-negative, got $account" } outputs.forEachIndexed { index, (recipientRaw43, amount) -> require(recipientRaw43.size == 43) { @@ -2277,6 +2284,15 @@ class PlatformWalletManager( /** SPV progress poll cadence — matches Swift's 1 Hz `startProgressPolling`. */ const val POLL_INTERVAL_MS = 1_000L + /** + * Recipient ceiling of [shieldedTransferMulti] — mirrors + * `MAX_SHIELDED_TRANSFER_RECIPIENTS` in + * `packages/rs-platform-wallet-ffi/src/shielded_send.rs`, which the JNI adapter enforces + * from the array lengths before allocating. Checked here too so an oversized call is + * refused before this side flattens caller-sized buffers. + */ + const val MAX_SHIELDED_TRANSFER_RECIPIENTS = 16 + /** De-offset `PlatformWalletFFIResultCode::ErrorInvalidParameter`. */ const val PWFFI_INVALID_PARAMETER = 2 } diff --git a/packages/rs-dpp/src/shielded/builder/mod.rs b/packages/rs-dpp/src/shielded/builder/mod.rs index 42151a50189..9ab58623507 100644 --- a/packages/rs-dpp/src/shielded/builder/mod.rs +++ b/packages/rs-dpp/src/shielded/builder/mod.rs @@ -55,6 +55,7 @@ use grovedb_commitment_tree::{ FullViewingKey, MerklePath, Note, NoteValue, OutgoingViewingKey, PaymentAddress, ProvingKey, Scope, SpendAuthorizingKey, SpendingKey, }; +use platform_version::version::PlatformVersion; use rand::rngs::OsRng; use rand::RngCore; @@ -107,7 +108,8 @@ impl From<&OrchardAddress> for PaymentAddress { } /// The number of Orchard actions a `BundleType::DEFAULT` bundle built from `num_spends` spends -/// and `num_outputs` outputs will publish **on the wire**. +/// and `num_outputs` outputs will publish **on the wire**, validated against the consensus +/// action ceiling. /// /// Every shielded fee predictor MUST size its fee with this function, because consensus prices /// the fee off the on-wire `actions.len()` (see @@ -123,17 +125,41 @@ impl From<&OrchardAddress> for PaymentAddress { /// /// This delegates to Orchard's own [`BundleType::num_actions`] rather than re-deriving the rule, /// so the predictor cannot drift from the builder that actually lays out the bundle. +/// +/// # The consensus ceiling +/// +/// Every shielded transition's `validate_structure` rejects a bundle whose `actions.len()` +/// exceeds `platform_version.system_limits.max_shielded_transition_actions` (via +/// `validate_actions_count`), but the `try_from_bundle` constructors do NOT run structural +/// validation — so without this gate an over-sized bundle is built, proved (~30 s of Halo 2), +/// and only then rejected on chain. Because the action count is `max(spends, outputs)`, bounding +/// it here bounds BOTH sides: a fragmented wallet spending too many notes and a caller asking +/// for too many outputs are rejected by the same comparison, before any proving work starts. pub fn shielded_bundle_action_count( num_spends: usize, num_outputs: usize, + platform_version: &PlatformVersion, ) -> Result { - BundleType::DEFAULT + let num_actions = BundleType::DEFAULT .num_actions(num_spends, num_outputs) .map_err(|e| { ProtocolError::ShieldedBuildError(format!( "invalid Orchard bundle shape ({num_spends} spends, {num_outputs} outputs): {e}" )) - }) + })?; + + let max_actions = platform_version + .system_limits + .max_shielded_transition_actions as usize; + if num_actions > max_actions { + return Err(ProtocolError::ShieldedBuildError(format!( + "a bundle of {num_spends} spends and {num_outputs} outputs publishes {num_actions} \ + Orchard actions, exceeding the consensus limit of {max_actions} \ + (max_shielded_transition_actions); consensus would reject the proved transition" + ))); + } + + Ok(num_actions) } /// Serializes an authorized Orchard bundle into the raw fields used by @@ -824,6 +850,7 @@ mod mod_tests { /// the ones a spends-only predictor gets wrong. #[test] fn shielded_bundle_action_count_is_max_spends_outputs_padded_to_two() { + let platform_version = PlatformVersion::latest(); for (spends, outputs, expected) in [ (0usize, 1usize, 2usize), (1, 1, 2), @@ -836,7 +863,7 @@ mod mod_tests { (5, 3, 5), (3, 7, 7), ] { - let actual = shielded_bundle_action_count(spends, outputs) + let actual = shielded_bundle_action_count(spends, outputs, platform_version) .expect("DEFAULT bundles accept any spend/output mix"); assert_eq!( actual, expected, @@ -850,6 +877,7 @@ mod mod_tests { /// the cheapest real bundle to construct at several output counts. #[test] fn shielded_bundle_action_count_matches_a_real_bundle() { + let platform_version = PlatformVersion::latest(); let recipient = test_orchard_address(); // (dummy_outputs, total outputs = 1 real + dummies) for dummies in [0usize, 1, 4] { @@ -857,8 +885,8 @@ mod mod_tests { let bundle = build_output_only_bundle(&recipient, 10_000, [0u8; 36], None, dummies, &TestProver) .expect("bundle should build"); - let predicted = - shielded_bundle_action_count(0, num_outputs).expect("valid bundle shape"); + let predicted = shielded_bundle_action_count(0, num_outputs, platform_version) + .expect("valid bundle shape"); assert_eq!( bundle.actions().len(), predicted, @@ -867,4 +895,48 @@ mod mod_tests { ); } } + + /// The predictor is also the CONSENSUS gate: `validate_actions_count` rejects + /// `actions.len() > max_shielded_transition_actions`, but `try_from_bundle` runs no + /// structural validation — so a bundle over the ceiling would be proved (~30 s of Halo 2) + /// and only then rejected on chain. The boundary itself must still pass. + #[test] + fn shielded_bundle_action_count_accepts_the_consensus_boundary() { + let platform_version = PlatformVersion::latest(); + let max = platform_version + .system_limits + .max_shielded_transition_actions as usize; + + // Exactly at the ceiling, from each side. + assert_eq!( + shielded_bundle_action_count(1, max, platform_version) + .expect("the output-side boundary must be accepted"), + max + ); + assert_eq!( + shielded_bundle_action_count(max, 1, platform_version) + .expect("the spend-side boundary must be accepted"), + max + ); + } + + /// One action over the ceiling must fail fast — from the OUTPUT side (the 16-recipient FFI + /// call, which becomes 17 outputs once the unconditional change output is added) and from + /// the SPEND side (a fragmented wallet selecting too many notes). + #[test] + fn shielded_bundle_action_count_rejects_over_the_consensus_limit() { + let platform_version = PlatformVersion::latest(); + let max = platform_version + .system_limits + .max_shielded_transition_actions as usize; + + for (spends, outputs) in [(1usize, max + 1), (max + 1, 1), (max + 1, max + 1)] { + let err = shielded_bundle_action_count(spends, outputs, platform_version) + .expect_err("a bundle over the consensus action limit must be rejected"); + assert!( + err.to_string().contains("exceeding the consensus limit"), + "unexpected error for {spends} spends / {outputs} outputs: {err}" + ); + } + } } diff --git a/packages/rs-dpp/src/shielded/builder/shielded_transfer.rs b/packages/rs-dpp/src/shielded/builder/shielded_transfer.rs index c2d0d6c6202..365e1f085e8 100644 --- a/packages/rs-dpp/src/shielded/builder/shielded_transfer.rs +++ b/packages/rs-dpp/src/shielded/builder/shielded_transfer.rs @@ -63,8 +63,12 @@ pub fn build_shielded_transfer_transition( // fee for the with-change shape is exact in BOTH branches — see the // `single_output_transfer_fee_matches_on_wire_action_count` test, which pins the carved fee // against the bundle's real `actions.len()`. + // + // The helper also enforces the consensus action ceiling + // (`max_shielded_transition_actions`), so a wallet fragmented enough to need more spends + // than consensus allows fails here rather than after the ~30 s proof. const MAX_OUTPUTS: usize = 2; // recipient + change - let num_actions = shielded_bundle_action_count(spends.len(), MAX_OUTPUTS)?; + let num_actions = shielded_bundle_action_count(spends.len(), MAX_OUTPUTS, platform_version)?; // The fee is fixed at the minimum: a transfer's `value_balance` IS the fee and consensus // pins it to exactly this amount (overpayment buys nothing and would leak a distinguishing // fee fingerprint that breaks shielded uniformity). @@ -238,7 +242,11 @@ pub fn build_shielded_transfer_transition_multi( let num_outputs = outputs.len().checked_add(1).ok_or_else(|| { ProtocolError::ShieldedBuildError("output count overflows usize".to_string()) })?; - let num_actions = shielded_bundle_action_count(spends.len(), num_outputs)?; + // Also the consensus action-ceiling gate: `max(spends, recipients + 1)` must stay within + // `max_shielded_transition_actions`, or the transition is doomed at `validate_structure`. + // `try_from_bundle` performs no structural validation, so without this the caller would burn + // the ~30 s Halo 2 proof on a bundle consensus is guaranteed to reject. + let num_actions = shielded_bundle_action_count(spends.len(), num_outputs, platform_version)?; let fee = compute_minimum_shielded_fee(num_actions, platform_version)?; let required = transfer_total.checked_add(fee).ok_or_else(|| { @@ -601,6 +609,137 @@ mod tests { ); } + // -------------------------------------------------------------- + // Consensus action ceiling (`max_shielded_transition_actions`) + // -------------------------------------------------------------- + + /// Build `count` recipient outputs of `amount` each, all to the same test address. + fn n_outputs(count: usize, amount: u64) -> Vec { + let recipient = test_orchard_address(); + (0..count) + .map(|_| ShieldedTransferOutput { + recipient, + amount, + memo: [0u8; 36], + }) + .collect() + } + + /// `max_shielded_transition_actions` recipient outputs plus the unconditional change output + /// publish one action too many. `try_from_bundle` runs no structural validation, so without + /// an up-front gate this bundle would be laid out, proved (~30 s of Halo 2) and only then + /// rejected by consensus at `validate_structure`. It must fail BEFORE proving — this test + /// completing in milliseconds is itself part of the assertion. + #[test] + fn multi_output_transfer_rejects_output_count_over_the_action_limit() { + let platform_version = PlatformVersion::latest(); + let max = platform_version + .system_limits + .max_shielded_transition_actions as usize; + let sk = SpendingKey::from_bytes([42u8; 32]).expect("valid sk"); + let fvk = FullViewingKey::from(&sk); + let ask = SpendAuthorizingKey::from(&sk); + let change_address = test_orchard_address(); + + // `max` recipients → `max + 1` outputs once change is added. This is exactly what the + // FFI's 16-recipient ceiling admits today. + let outputs = n_outputs(max, 1_000_000); + + let err = build_shielded_transfer_transition_multi( + vec![test_spendable_note(u64::MAX / 2)], + &outputs, + &change_address, + &fvk, + &ask, + Anchor::empty_tree(), + &TestProver, + platform_version, + ) + .expect_err("a bundle over the consensus action limit must be rejected"); + assert!( + err.to_string().contains("exceeding the consensus limit"), + "unexpected error: {err}" + ); + } + + /// The boundary itself must still build: `max - 1` recipients plus change is exactly + /// `max_shielded_transition_actions` actions. The gate must not reject it — the build gets + /// past the fee/limit arithmetic and only stops at the (unrelated) `add_spend` anchor + /// mismatch of the test note, which is how the other builder tests pin "proceeded past the + /// value checks" without paying for a real proof. + #[test] + fn multi_output_transfer_accepts_the_action_limit_boundary() { + let platform_version = PlatformVersion::latest(); + let max = platform_version + .system_limits + .max_shielded_transition_actions as usize; + let sk = SpendingKey::from_bytes([42u8; 32]).expect("valid sk"); + let fvk = FullViewingKey::from(&sk); + let ask = SpendAuthorizingKey::from(&sk); + let change_address = test_orchard_address(); + + let outputs = n_outputs(max - 1, 1_000_000); + + let err = build_shielded_transfer_transition_multi( + vec![test_spendable_note(u64::MAX / 2)], + &outputs, + &change_address, + &fvk, + &ask, + Anchor::empty_tree(), + &TestProver, + platform_version, + ) + .expect_err("the test note's all-zero path mismatches the empty-tree anchor"); + let err = err.to_string(); + assert!( + !err.contains("exceeding the consensus limit"), + "exactly {max} actions is AT the limit and must not be rejected by it, got: {err}" + ); + assert!( + err.contains("failed to add spend") || err.contains("nchor"), + "expected the downstream add_spend error, got: {err}" + ); + } + + /// The spend side can breach the ceiling too: a fragmented wallet selecting more notes than + /// `max_shielded_transition_actions` publishes one action per spend. The single-output + /// builder shares the same gate, so it must fail fast as well. + #[test] + fn transfer_rejects_spend_count_over_the_action_limit() { + let platform_version = PlatformVersion::latest(); + let max = platform_version + .system_limits + .max_shielded_transition_actions as usize; + let sk = SpendingKey::from_bytes([42u8; 32]).expect("valid sk"); + let fvk = FullViewingKey::from(&sk); + let ask = SpendAuthorizingKey::from(&sk); + let change_address = test_orchard_address(); + let recipient = test_orchard_address(); + + let spends: Vec = (0..max + 1) + .map(|_| test_spendable_note(1_000_000_000)) + .collect(); + + let err = build_shielded_transfer_transition( + spends, + &recipient, + 1_000, + &change_address, + &fvk, + &ask, + Anchor::empty_tree(), + &TestProver, + [0u8; 36], + platform_version, + ) + .expect_err("more spends than the consensus action limit must be rejected"); + assert!( + err.to_string().contains("exceeding the consensus limit"), + "unexpected error: {err}" + ); + } + #[test] fn test_shielded_transfer_insufficient_funds() { let platform_version = PlatformVersion::latest(); diff --git a/packages/rs-platform-wallet-ffi/src/shielded_send.rs b/packages/rs-platform-wallet-ffi/src/shielded_send.rs index d2b283f7926..ac036848e5b 100644 --- a/packages/rs-platform-wallet-ffi/src/shielded_send.rs +++ b/packages/rs-platform-wallet-ffi/src/shielded_send.rs @@ -356,7 +356,53 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_transfer( /// `num_recipients` from driving a huge allocation before anything else can reject it. The real /// ceiling is the 20 KiB state-transition size limit, which admits roughly six Orchard actions — /// so a legitimate caller stays far below this. -const MAX_SHIELDED_TRANSFER_RECIPIENTS: usize = 16; +/// +/// Public so language bridges (the JNI adapter, Swift) can enforce the SAME bound before they +/// allocate their own caller-sized buffers, rather than duplicating the literal. +pub const MAX_SHIELDED_TRANSFER_RECIPIENTS: usize = 16; + +/// Render a caught panic payload as a human-readable string. +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 { + "".to_string() + } +} + +/// Run a shielded-spend export body under [`std::panic::catch_unwind`], converting a panic into a +/// typed FFI error instead of letting it reach the `extern "C"` frame. +/// +/// A Rust panic cannot unwind through a C ABI boundary: it aborts the process. The JNI layer +/// wraps its calls in `support::guard` (which catches panics and raises a Java exception), but +/// that guard sits on the FAR side of this `extern "C"` export, so it never sees the unwind — the +/// process is already gone. `block_on_worker` makes this reachable rather than theoretical: it +/// `.expect`s on the tokio `JoinError`, so any panic inside the proving future (Halo 2 synthesis, +/// note bookkeeping, the SDK) re-panics right here inside the export. +/// +/// The panic is mapped to [`PlatformWalletFFIResultCode::ErrorShieldedSpendUnconfirmed`], NOT to +/// a definitive failure code: a panic can strike after the notes were reserved and even after the +/// transition was broadcast, so the outcome is genuinely ambiguous. That code's contract is +/// exactly the conservative one this needs — the host must not auto-retry, the reservation stays +/// in place, and the next nullifier sync (or an app restart) reconciles whether the spend landed. +fn catch_spend_panic( + operation: &str, + body: impl FnOnce() -> PlatformWalletFFIResult, +) -> PlatformWalletFFIResult { + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(body)) { + Ok(result) => result, + Err(payload) => PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorShieldedSpendUnconfirmed, + format!( + "{operation} panicked: {}. The spend may or may not have been broadcast — do \ + NOT retry; the next shielded sync reconciles the outcome.", + panic_payload_message(payload.as_ref()) + ), + ), + } +} /// Send a shielded → shielded transfer with SEVERAL outputs in one /// atomic transition. @@ -402,6 +448,39 @@ pub unsafe extern "C" fn platform_wallet_manager_shielded_transfer_multi( amounts: *const u64, num_recipients: usize, memo_text: *const c_char, +) -> PlatformWalletFFIResult { + // The whole body runs under `catch_unwind`: a panic (most concretely `block_on_worker`'s + // `.expect` on a panicking proving task) must NOT reach this `extern "C"` frame, where it + // would abort the process instead of surfacing to the host as a typed error. + catch_spend_panic("shielded multi-output transfer", || { + shielded_transfer_multi_inner( + handle, + wallet_id_bytes, + mnemonic_resolver_handle, + account, + recipients_raw_43, + amounts, + num_recipients, + memo_text, + ) + }) +} + +/// Body of [`platform_wallet_manager_shielded_transfer_multi`], as an ordinary Rust function so a +/// panic unwinds into [`catch_spend_panic`] instead of across the C ABI. +/// +/// # Safety +/// Identical contract to the export that calls it. +#[allow(clippy::too_many_arguments)] +unsafe fn shielded_transfer_multi_inner( + handle: Handle, + wallet_id_bytes: *const u8, + mnemonic_resolver_handle: *mut MnemonicResolverHandle, + account: u32, + recipients_raw_43: *const u8, + amounts: *const u64, + num_recipients: usize, + memo_text: *const c_char, ) -> PlatformWalletFFIResult { check_ptr!(wallet_id_bytes); check_ptr!(mnemonic_resolver_handle); @@ -1614,6 +1693,54 @@ mod tests { .into_owned() } + /// A non-panicking body passes its result straight through — the guard must be invisible on + /// the happy path. + #[test] + fn catch_spend_panic_passes_results_through() { + let ok = catch_spend_panic("test", PlatformWalletFFIResult::ok); + assert_eq!(ok.code, PlatformWalletFFIResultCode::Success); + + let err = catch_spend_panic("test", || { + PlatformWalletFFIResult::err( + PlatformWalletFFIResultCode::ErrorInvalidParameter, + "bad input", + ) + }); + assert_eq!(err.code, PlatformWalletFFIResultCode::ErrorInvalidParameter); + assert_eq!(message_of(&err), "bad input"); + } + + /// A panic inside a shielded-spend export must NOT unwind into the `extern "C"` frame (that + /// aborts the process). It becomes `ErrorShieldedSpendUnconfirmed` — the conservative + /// "may have been broadcast, do NOT retry" contract, because a panic can strike after the + /// notes are reserved and after the transition is submitted. + #[test] + fn catch_spend_panic_maps_a_panic_to_the_unconfirmed_contract() { + let previous = std::panic::take_hook(); + // Silence the default hook's backtrace spew for this deliberate panic. + std::panic::set_hook(Box::new(|_| {})); + let result = catch_spend_panic("shielded multi-output transfer", || { + panic!("tokio worker panicked"); + }); + std::panic::set_hook(previous); + + assert_eq!( + result.code, + PlatformWalletFFIResultCode::ErrorShieldedSpendUnconfirmed, + "a panic must map to the ambiguous, do-not-retry code" + ); + let message = message_of(&result); + assert!( + message.contains("shielded multi-output transfer panicked") + && message.contains("tokio worker panicked"), + "the panic payload must survive into the FFI message: {message}" + ); + assert!( + message.contains("do NOT retry"), + "the message must carry the do-not-retry guidance: {message}" + ); + } + /// `map_spend_result` pins the retry-relevant code split the three spend /// entry points depend on: /// - `ShieldedSpendUnconfirmed` → `ErrorShieldedSpendUnconfirmed` (host diff --git a/packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs b/packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs index 6f27f628526..18d0a2db811 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs @@ -70,6 +70,35 @@ impl ShieldedFeeKind { } } +/// Whether the spend's builder tolerates a zero-valued change output. +/// +/// The two shapes differ by exactly one credit at the boundary, and note selection MUST reserve +/// against the shape its builder actually enforces — otherwise a selection is reserved, the +/// builder rejects it, and a wallet with sufficient balance reports a failed spend (the +/// reservation is released, so nothing is stranded, but the spend is refused). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ChangeRequirement { + /// The builder tolerates zero change: `build_shielded_transfer_transition` simply omits its + /// change output, and `build_unshield_transition` / + /// `build_shielded_withdrawal_transition` emit a zero-valued one. All three reject only + /// `required > total_spent`, so exact coverage is a fundable selection. + Optional, + /// The builder ALWAYS emits a change output, which must carry a positive value, so it + /// rejects `total_spent == amount + fee` (`build_shielded_transfer_transition_multi` tests + /// `required >= total_spent`). Selection must therefore cover `amount + fee + 1`. + StrictlyPositive, +} + +impl ChangeRequirement { + /// Credits the selection must cover ON TOP of `amount + fee`. + fn min_change_credits(self) -> u64 { + match self { + ChangeRequirement::Optional => 0, + ChangeRequirement::StrictlyPositive => 1, + } + } +} + /// Select unspent notes to cover `amount + fee` using a greedy algorithm. /// /// Notes are sorted by value descending and accumulated until the target is met. @@ -153,27 +182,50 @@ pub fn select_notes( /// ShieldedTransfer. This MUST match the fee the builder/consensus will charge, otherwise the spend /// is under-funded. /// -/// Returns the selected notes, total input value, and the exact fee. +/// `change` states whether the builder can omit its change output. Pass +/// [`ChangeRequirement::StrictlyPositive`] for the multi-output transfer builder, whose change +/// output is unconditional and must carry a positive value: an exact-coverage selection +/// (`total_input == amount + fee`) satisfies this function's `>=` test but is then REJECTED by +/// that builder, so a wallet that could fund the spend by selecting one more note would report a +/// failure instead. The extra credit is folded into the selection target on every iteration, so +/// the fee re-computation that follows an added note (and the action count that added note +/// implies) is applied to the strict target too. +/// +/// Returns the selected notes, total input value, and the exact fee. The returned fee is the pure +/// consensus fee — the change-requirement credit is a selection-side floor only and is NOT part +/// of what the builder carves. pub fn select_notes_with_fee<'a>( unspent: &'a [ShieldedNote], amount: u64, min_actions: usize, fee_kind: ShieldedFeeKind, + change: ChangeRequirement, platform_version: &PlatformVersion, ) -> Result<(Vec<&'a ShieldedNote>, u64, u64), PlatformWalletError> { + let min_change = change.min_change_credits(); let mut fee_estimate = fee_kind .compute(min_actions, platform_version) .map_err(|e| PlatformWalletError::ShieldedBuildError(e.to_string()))?; + // Target for `select_notes`, which adds it to `amount`: the fee plus the minimum change the + // builder demands. + let selection_target = |fee: u64| -> Result { + fee.checked_add(min_change).ok_or_else(|| { + PlatformWalletError::ShieldedBuildError( + "fee + minimum change overflows u64".to_string(), + ) + }) + }; + for _ in 0..5 { - let selected = select_notes(unspent, amount, fee_estimate)?; + let selected = select_notes(unspent, amount, selection_target(fee_estimate)?)?; let total: u64 = selected.iter().map(|n| n.value).sum(); let num_actions = selected.len().max(min_actions); let exact_fee = fee_kind .compute(num_actions, platform_version) .map_err(|e| PlatformWalletError::ShieldedBuildError(e.to_string()))?; - if total >= amount.saturating_add(exact_fee) { + if total >= amount.saturating_add(exact_fee).saturating_add(min_change) { return Ok((selected, total, exact_fee)); } @@ -181,17 +233,18 @@ pub fn select_notes_with_fee<'a>( } // Final attempt with last computed fee - let selected = select_notes(unspent, amount, fee_estimate)?; + let selected = select_notes(unspent, amount, selection_target(fee_estimate)?)?; let total: u64 = selected.iter().map(|n| n.value).sum(); let num_actions = selected.len().max(min_actions); let exact_fee = fee_kind .compute(num_actions, platform_version) .map_err(|e| PlatformWalletError::ShieldedBuildError(e.to_string()))?; - if total < amount.saturating_add(exact_fee) { + let required = amount.saturating_add(exact_fee).saturating_add(min_change); + if total < required { return Err(PlatformWalletError::ShieldedInsufficientBalance { available: total, - required: amount.saturating_add(exact_fee), + required, }); } @@ -372,9 +425,15 @@ mod tests { // A single note covering amount + the 2-action fee. let notes = vec![test_note(amount + min_fee_2 + 5, 0)]; - let (selected, total, exact_fee) = - select_notes_with_fee(¬es, amount, 2, ShieldedFeeKind::Base, platform_version) - .expect("selection ok"); + let (selected, total, exact_fee) = select_notes_with_fee( + ¬es, + amount, + 2, + ShieldedFeeKind::Base, + ChangeRequirement::Optional, + platform_version, + ) + .expect("selection ok"); assert_eq!(selected.len(), 1); assert_eq!(total, amount + min_fee_2 + 5); @@ -409,6 +468,7 @@ mod tests { amount, min_actions, ShieldedFeeKind::Base, + ChangeRequirement::Optional, platform_version, ) .expect("selection ok"); @@ -420,6 +480,141 @@ mod tests { ); } + /// THE regression pin for the exact-fit selection bug. + /// + /// `build_shielded_transfer_transition_multi` always emits a change output and so requires + /// the spent value to STRICTLY exceed `sum(amounts) + fee`. With notes valued + /// `[amount + fee, 1]`, largest-first selection used to stop on the exact-coverage note + /// alone: the reservation succeeded, the builder then rejected the spend, and a wallet that + /// could have funded it by taking the remaining credit reported a failure. + /// [`ChangeRequirement::StrictlyPositive`] makes the selector demand that extra credit. + #[test] + fn test_select_notes_with_fee_strict_change_takes_one_more_credit() { + let platform_version = PlatformVersion::latest(); + // Two recipient notes + change = 3 outputs → the 3-action floor the multi-output + // transfer reserves against. + let min_actions = 3; + let fee = compute_minimum_shielded_fee(min_actions, platform_version).expect("fee"); + let amount = 3_000_000_000u64; + + // The reviewer's shape: one note covering `amount + fee` exactly, plus a single credit. + let notes = vec![test_note(amount + fee, 0), test_note(1, 1)]; + + let (selected, total, exact_fee) = select_notes_with_fee( + ¬es, + amount, + min_actions, + ShieldedFeeKind::Base, + ChangeRequirement::StrictlyPositive, + platform_version, + ) + .expect("a wallet holding amount + fee + 1 must be able to fund a multi-output transfer"); + + assert_eq!( + selected.len(), + 2, + "the exact-coverage note alone leaves zero change; the extra credit must be selected" + ); + assert_eq!( + exact_fee, fee, + "two spends still sit under the 3-action floor" + ); + assert!( + total > amount.saturating_add(exact_fee), + "the selection must leave STRICTLY positive change ({total} > {amount} + {exact_fee})" + ); + + // Same wallet under the permissive contract stops on the exact-coverage note — the state + // the builder rejects. This is what the strict variant exists to prevent. + let (permissive, permissive_total, permissive_fee) = select_notes_with_fee( + ¬es, + amount, + min_actions, + ShieldedFeeKind::Base, + ChangeRequirement::Optional, + platform_version, + ) + .expect("selection ok"); + assert_eq!(permissive.len(), 1); + assert_eq!( + permissive_total, + amount + permissive_fee, + "the permissive contract accepts exact coverage — zero change" + ); + } + + /// One credit short of the strict requirement is a genuine insufficient balance, and the + /// reported `required` must include the change credit so the caller sees the real shortfall. + #[test] + fn test_select_notes_with_fee_strict_change_reports_the_extra_credit_as_required() { + let platform_version = PlatformVersion::latest(); + let min_actions = 3; + let fee = compute_minimum_shielded_fee(min_actions, platform_version).expect("fee"); + let amount = 3_000_000_000u64; + // Exactly `amount + fee` and not a credit more. + let notes = vec![test_note(amount + fee, 0)]; + + let err = select_notes_with_fee( + ¬es, + amount, + min_actions, + ShieldedFeeKind::Base, + ChangeRequirement::StrictlyPositive, + platform_version, + ) + .expect_err("exact coverage cannot fund a builder that demands positive change"); + + match err { + PlatformWalletError::ShieldedInsufficientBalance { + available, + required, + } => { + assert_eq!(available, amount + fee); + assert_eq!( + required, + amount + fee + 1, + "the required figure must include the change credit" + ); + } + other => panic!("unexpected error: {other:?}"), + } + } + + /// The strict floor must survive fee convergence: when the selector adds notes, the action + /// count (and therefore the fee) is recomputed, and the strict `total > amount + fee` + /// postcondition must hold against the RECOMPUTED fee, not the initial estimate. + #[test] + fn test_select_notes_with_fee_strict_change_holds_after_fee_reconvergence() { + let platform_version = PlatformVersion::latest(); + let min_actions = 3; + let amount = 1_000_000u64; + // Many equal mid-size notes, so several must be selected and the action count — and with + // it the fee — climbs past the 3-action floor during convergence. + let notes: Vec = (0..20).map(|i| test_note(60_000_000, i)).collect(); + + let (selected, total, exact_fee) = select_notes_with_fee( + ¬es, + amount, + min_actions, + ShieldedFeeKind::Base, + ChangeRequirement::StrictlyPositive, + platform_version, + ) + .expect("selection ok"); + + let expected_fee = + compute_minimum_shielded_fee(selected.len().max(min_actions), platform_version) + .expect("fee"); + assert_eq!( + exact_fee, expected_fee, + "the returned fee must match the selected action count" + ); + assert!( + total > amount.saturating_add(exact_fee), + "strict change must hold against the recomputed fee" + ); + } + /// Two sub-denomination notes on one key are STRUCTURALLY forced to both be selected when /// the spend targets the full denomination: the greedy selector takes the largest note first /// and only stops once the accumulated value covers the target, and neither note alone can. @@ -472,9 +667,15 @@ mod tests { let note_val = 60_000_000u64; let notes: Vec = (0..20).map(|i| test_note(note_val, i)).collect(); - let (selected, total, exact_fee) = - select_notes_with_fee(¬es, amount, 2, ShieldedFeeKind::Base, platform_version) - .expect("selection ok"); + let (selected, total, exact_fee) = select_notes_with_fee( + ¬es, + amount, + 2, + ShieldedFeeKind::Base, + ChangeRequirement::Optional, + platform_version, + ) + .expect("selection ok"); let expected_fee = compute_minimum_shielded_fee(selected.len().max(2), platform_version).unwrap(); @@ -513,6 +714,7 @@ mod tests { amount, 2, ShieldedFeeKind::Withdrawal, + ChangeRequirement::Optional, platform_version, ) .expect("selection ok"); @@ -550,6 +752,7 @@ mod tests { amount, 2, ShieldedFeeKind::Unshield, + ChangeRequirement::Optional, platform_version, ) .expect("selection ok"); diff --git a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs index d3c3acff301..4f04ecf125c 100644 --- a/packages/rs-platform-wallet/src/wallet/shielded/operations.rs +++ b/packages/rs-platform-wallet/src/wallet/shielded/operations.rs @@ -25,7 +25,7 @@ use super::activity_recorder::{ }; use super::keys::{AccountViewingKeys, OrchardKeySet}; use super::note_selection::{ - select_notes_for_denomination, select_notes_with_fee, ShieldedFeeKind, + select_notes_for_denomination, select_notes_with_fee, ChangeRequirement, ShieldedFeeKind, }; use super::store::{PendingRedrive, ShieldedNote, ShieldedStore, SubwalletId}; use crate::changeset::{PlatformWalletChangeSet, ShieldedChangeSet}; @@ -673,8 +673,18 @@ pub async fn unshield( // reserve against `ShieldedFeeKind::Unshield` — reserving the base fee here would under-fund the // address-write cost and the builder would reject the spend (and the `fee_used == exact_fee` // debug assert below would fire). - let (selected_notes, total_input, exact_fee) = - reserve_unspent_notes(sdk, store, id, amount, 2, ShieldedFeeKind::Unshield).await?; + let (selected_notes, total_input, exact_fee) = reserve_unspent_notes( + sdk, + store, + id, + amount, + 2, + ShieldedFeeKind::Unshield, + // `build_unshield_transition` accepts a zero-valued change output (it rejects only + // `required > total_spent`), so exact coverage is a fundable selection. + ChangeRequirement::Optional, + ) + .await?; info!( account, @@ -860,8 +870,18 @@ pub async fn transfer( // ShieldedTransfer is carved with the base `compute_minimum_shielded_fee`, so reserve // against `ShieldedFeeKind::Base`. - let (selected_notes, total_input, exact_fee) = - reserve_unspent_notes(sdk, store, id, amount, 2, ShieldedFeeKind::Base).await?; + let (selected_notes, total_input, exact_fee) = reserve_unspent_notes( + sdk, + store, + id, + amount, + 2, + ShieldedFeeKind::Base, + // `build_shielded_transfer_transition` emits change only when there is some, so an + // exact-coverage selection is fundable. + ChangeRequirement::Optional, + ) + .await?; info!( account, @@ -1049,6 +1069,11 @@ pub async fn transfer_multi( // Reserve against the SAME output count the builder sizes its fee from: every recipient // output plus the unconditional change output. + // + // That change output is unconditional and must carry a positive value, so the builder + // rejects `total_input == total_amount + fee`. Selection must therefore demand STRICTLY + // more, or a wallet holding e.g. `[total_amount + fee, 1]` would have the exact-coverage + // note reserved on its own and the build would fail despite the balance being sufficient. let num_outputs = builder_outputs.len() + 1; let (selected_notes, total_input, exact_fee) = reserve_unspent_notes( sdk, @@ -1057,6 +1082,7 @@ pub async fn transfer_multi( total_amount, num_outputs, ShieldedFeeKind::Base, + ChangeRequirement::StrictlyPositive, ) .await?; @@ -1213,8 +1239,17 @@ pub async fn withdraw( // `ShieldedFeeKind::Withdrawal` — reserving the base fee here would under-fund the document // cost and the builder would reject the spend (and the `fee_used == exact_fee` debug assert // below would fire). - let (selected_notes, total_input, exact_fee) = - reserve_unspent_notes(sdk, store, id, amount, 2, ShieldedFeeKind::Withdrawal).await?; + let (selected_notes, total_input, exact_fee) = reserve_unspent_notes( + sdk, + store, + id, + amount, + 2, + ShieldedFeeKind::Withdrawal, + // `build_shielded_withdrawal_transition` likewise accepts zero change. + ChangeRequirement::Optional, + ) + .await?; info!( account, @@ -2069,13 +2104,15 @@ async fn reserve_unspent_notes( amount: u64, outputs: usize, fee_kind: ShieldedFeeKind, + change: ChangeRequirement, ) -> Result<(Vec, u64, u64), PlatformWalletError> { let mut store = store.write().await; let unspent = store .get_unspent_notes(id) .map_err(|e| PlatformWalletError::ShieldedStoreError(e.to_string()))?; let (selected, total_input, exact_fee) = - select_notes_with_fee(&unspent, amount, outputs, fee_kind, sdk.version())?.into_owned(); + select_notes_with_fee(&unspent, amount, outputs, fee_kind, change, sdk.version())? + .into_owned(); for note in &selected { store .mark_pending(id, ¬e.nullifier) diff --git a/packages/rs-unified-sdk-jni/src/funding.rs b/packages/rs-unified-sdk-jni/src/funding.rs index a2035b4f33a..a7d1d6cc1ac 100644 --- a/packages/rs-unified-sdk-jni/src/funding.rs +++ b/packages/rs-unified-sdk-jni/src/funding.rs @@ -853,6 +853,11 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_FundingNative_shielde /// `amounts` holds the matching credit amounts. Each pair becomes its own /// note; repeating the same address funds that address with several /// independent notes. `memoText` is attached to every recipient note. +/// +/// At most `platform_wallet_ffi::MAX_SHIELDED_TRANSFER_RECIPIENTS` recipients +/// are accepted, and that ceiling is enforced from the Java array LENGTHS +/// before either array is copied into a native buffer — so no allocation is +/// ever sized by an unvalidated caller-supplied count. #[no_mangle] pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_FundingNative_shieldedTransferMulti( mut env: JNIEnv, @@ -881,14 +886,12 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_FundingNative_shielde throw_sdk_exception(env, 1, "amounts long[] was null"); return; } - let recipients = match env.convert_byte_array(&recipients_raw43) { - Ok(b) => b, - Err(_) => { - let _ = env.exception_clear(); - throw_sdk_exception(env, 1, "recipientsRaw43 byte[] was invalid"); - return; - } - }; + // Establish the recipient count and bound it BEFORE any caller-sized allocation. Both + // `convert_byte_array` (43 bytes per recipient) and the amount buffers below are sized + // from Java-supplied lengths, so an accidental or hostile oversized call would otherwise + // drive several large allocations — and possibly an allocator OOM — on its way to the + // native layer's clean `ErrorInvalidParameter`. `get_array_length` reads a header field + // and allocates nothing. let amount_len = match env.get_array_length(&amounts) { Ok(n) if n >= 0 => n as usize, _ => { @@ -901,18 +904,48 @@ pub extern "system" fn Java_org_dashfoundation_dashsdk_ffi_FundingNative_shielde throw_sdk_exception(env, 1, "amounts must contain at least one entry"); return; } - if recipients.len() != amount_len * 43 { + if amount_len > platform_wallet_ffi::MAX_SHIELDED_TRANSFER_RECIPIENTS { + throw_sdk_exception( + env, + 1, + &format!( + "amounts must hold at most {} entries, got {amount_len}", + platform_wallet_ffi::MAX_SHIELDED_TRANSFER_RECIPIENTS + ), + ); + return; + } + // Same rule for the address blob: verify its LENGTH (a header read) against the bounded + // recipient count before copying it into a native buffer, so the copy that follows is + // bounded by `MAX_SHIELDED_TRANSFER_RECIPIENTS * 43` rather than by the caller. + let recipients_len = match env.get_array_length(&recipients_raw43) { + Ok(n) if n >= 0 => n as usize, + _ => { + let _ = env.exception_clear(); + throw_sdk_exception(env, 1, "recipientsRaw43 byte[] was invalid"); + return; + } + }; + if recipients_len != amount_len * 43 { throw_sdk_exception( env, 1, &format!( "recipientsRaw43 must be 43 bytes per amount ({} expected), got {}", amount_len * 43, - recipients.len() + recipients_len ), ); return; } + let recipients = match env.convert_byte_array(&recipients_raw43) { + Ok(b) => b, + Err(_) => { + let _ = env.exception_clear(); + throw_sdk_exception(env, 1, "recipientsRaw43 byte[] was invalid"); + return; + } + }; let mut amount_buf = vec![0i64; amount_len]; if env .get_long_array_region(&amounts, 0, &mut amount_buf)