From dce62eba21e733dd660803c12498547103fc8ea8 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Wed, 4 Dec 2024 14:30:07 -0700 Subject: [PATCH 01/12] zcash_client_backend: Split `create_proposed_transaction` into two parts. This refactors `create_proposed_transaction` to allow the internal builder state and per-output metadata to be exposed after all inputs and outputs for a proposal step have been added, in order to make it possible to generate a PCZT from this data. --- zcash_client_backend/src/data_api/wallet.rs | 220 ++++++++++++++------ zcash_primitives/src/transaction/builder.rs | 2 + 2 files changed, 153 insertions(+), 69 deletions(-) diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 0d4384411..acffa9ec9 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -452,20 +452,43 @@ where Ok(NonEmpty::from_vec(txids).expect("proposal.steps is NonEmpty")) } +#[allow(clippy::type_complexity)] +struct BuildState<'a, P, AccountId> { + #[cfg(feature = "transparent-inputs")] + step_index: usize, + builder: Builder<'a, P, ()>, + #[cfg(feature = "orchard")] + orchard_output_meta: Vec<( + Recipient, + NonNegativeAmount, + Option, + )>, + sapling_output_meta: Vec<( + Recipient, + NonNegativeAmount, + Option, + )>, + transparent_output_meta: Vec<( + Recipient, + TransparentAddress, + NonNegativeAmount, + StepOutputIndex, + )>, + #[cfg(feature = "transparent-inputs")] + utxos_spent: Vec, +} + // `unused_transparent_outputs` maps `StepOutput`s for transparent outputs // that have not been consumed so far, to the corresponding pair of // `TransparentAddress` and `Outpoint`. #[allow(clippy::too_many_arguments)] #[allow(clippy::type_complexity)] -fn create_proposed_transaction( +fn build_proposed_transaction( wallet_db: &mut DbT, params: &ParamsT, - spend_prover: &impl SpendProver, - output_prover: &impl OutputProver, usk: &UnifiedSpendingKey, account_id: ::AccountId, ovk_policy: OvkPolicy, - fee_rule: &FeeRuleT, min_target_height: BlockHeight, prior_step_results: &[(&Step, StepResult<::AccountId>)], proposal_step: &Step, @@ -474,7 +497,7 @@ fn create_proposed_transaction, ) -> Result< - StepResult<::AccountId>, + BuildState<'static, ParamsT, DbT::AccountId>, CreateErrT, > where @@ -1001,76 +1024,133 @@ where } } + Ok(BuildState { + #[cfg(feature = "transparent-inputs")] + step_index, + builder, + #[cfg(feature = "orchard")] + orchard_output_meta, + sapling_output_meta, + transparent_output_meta, + #[cfg(feature = "transparent-inputs")] + utxos_spent, + }) +} + +// `unused_transparent_outputs` maps `StepOutput`s for transparent outputs +// that have not been consumed so far, to the corresponding pair of +// `TransparentAddress` and `Outpoint`. +#[allow(clippy::too_many_arguments)] +#[allow(clippy::type_complexity)] +fn create_proposed_transaction( + wallet_db: &mut DbT, + params: &ParamsT, + spend_prover: &impl SpendProver, + output_prover: &impl OutputProver, + usk: &UnifiedSpendingKey, + account_id: ::AccountId, + ovk_policy: OvkPolicy, + fee_rule: &FeeRuleT, + min_target_height: BlockHeight, + prior_step_results: &[(&Step, StepResult<::AccountId>)], + proposal_step: &Step, + #[cfg(feature = "transparent-inputs")] unused_transparent_outputs: &mut HashMap< + StepOutput, + (TransparentAddress, OutPoint), + >, +) -> Result< + StepResult<::AccountId>, + CreateErrT, +> +where + DbT: WalletWrite + WalletCommitmentTrees, + ParamsT: consensus::Parameters + Clone, + FeeRuleT: FeeRule, +{ + let build_state = build_proposed_transaction::<_, _, _, FeeRuleT, _, _>( + wallet_db, + params, + usk, + account_id, + ovk_policy, + min_target_height, + prior_step_results, + proposal_step, + #[cfg(feature = "transparent-inputs")] + unused_transparent_outputs, + )?; + // Build the transaction with the specified fee rule - let build_result = builder.build(OsRng, spend_prover, output_prover, fee_rule)?; + let build_result = build_state + .builder + .build(OsRng, spend_prover, output_prover, fee_rule)?; + #[cfg(feature = "orchard")] + let orchard_fvk: orchard::keys::FullViewingKey = usk.orchard().into(); #[cfg(feature = "orchard")] let orchard_internal_ivk = orchard_fvk.to_ivk(orchard::keys::Scope::Internal); #[cfg(feature = "orchard")] - let orchard_outputs = - orchard_output_meta - .into_iter() - .enumerate() - .map(|(i, (recipient, value, memo))| { - let output_index = build_result - .orchard_meta() - .output_action_index(i) - .expect("An action should exist in the transaction for each Orchard output."); - - let recipient = recipient - .map_internal_account_note(|pool| { - assert!(pool == PoolType::ORCHARD); - build_result - .transaction() - .orchard_bundle() - .and_then(|bundle| { - bundle - .decrypt_output_with_key(output_index, &orchard_internal_ivk) - .map(|(note, _, _)| Note::Orchard(note)) - }) - }) - .internal_account_note_transpose_option() - .expect("Wallet-internal outputs must be decryptable with the wallet's IVK"); - - SentTransactionOutput::from_parts(output_index, recipient, value, memo) - }); + let orchard_outputs = build_state.orchard_output_meta.into_iter().enumerate().map( + |(i, (recipient, value, memo))| { + let output_index = build_result + .orchard_meta() + .output_action_index(i) + .expect("An action should exist in the transaction for each Orchard output."); + + let recipient = recipient + .map_internal_account_note(|pool| { + assert!(pool == PoolType::ORCHARD); + build_result + .transaction() + .orchard_bundle() + .and_then(|bundle| { + bundle + .decrypt_output_with_key(output_index, &orchard_internal_ivk) + .map(|(note, _, _)| Note::Orchard(note)) + }) + }) + .internal_account_note_transpose_option() + .expect("Wallet-internal outputs must be decryptable with the wallet's IVK"); + + SentTransactionOutput::from_parts(output_index, recipient, value, memo) + }, + ); + let sapling_dfvk = usk.sapling().to_diversifiable_full_viewing_key(); let sapling_internal_ivk = PreparedIncomingViewingKey::new(&sapling_dfvk.to_ivk(Scope::Internal)); - let sapling_outputs = - sapling_output_meta - .into_iter() - .enumerate() - .map(|(i, (recipient, value, memo))| { - let output_index = build_result - .sapling_meta() - .output_index(i) - .expect("An output should exist in the transaction for each Sapling payment."); - - let recipient = recipient - .map_internal_account_note(|pool| { - assert!(pool == PoolType::SAPLING); - build_result - .transaction() - .sapling_bundle() - .and_then(|bundle| { - try_sapling_note_decryption( - &sapling_internal_ivk, - &bundle.shielded_outputs()[output_index], - zip212_enforcement(params, min_target_height), - ) - .map(|(note, _, _)| Note::Sapling(note)) - }) - }) - .internal_account_note_transpose_option() - .expect("Wallet-internal outputs must be decryptable with the wallet's IVK"); + let sapling_outputs = build_state.sapling_output_meta.into_iter().enumerate().map( + |(i, (recipient, value, memo))| { + let output_index = build_result + .sapling_meta() + .output_index(i) + .expect("An output should exist in the transaction for each Sapling payment."); + + let recipient = recipient + .map_internal_account_note(|pool| { + assert!(pool == PoolType::SAPLING); + build_result + .transaction() + .sapling_bundle() + .and_then(|bundle| { + try_sapling_note_decryption( + &sapling_internal_ivk, + &bundle.shielded_outputs()[output_index], + zip212_enforcement(params, min_target_height), + ) + .map(|(note, _, _)| Note::Sapling(note)) + }) + }) + .internal_account_note_transpose_option() + .expect("Wallet-internal outputs must be decryptable with the wallet's IVK"); - SentTransactionOutput::from_parts(output_index, recipient, value, memo) - }); + SentTransactionOutput::from_parts(output_index, recipient, value, memo) + }, + ); let txid: [u8; 32] = build_result.transaction().txid().into(); assert_eq!( - transparent_output_meta.len(), + build_state.transparent_output_meta.len(), build_result .transaction() .transparent_bundle() @@ -1078,8 +1158,11 @@ where ); #[allow(unused_variables)] - let transparent_outputs = transparent_output_meta.into_iter().enumerate().map( - |(n, (recipient, address, value, step_output_index))| { + let transparent_outputs = build_state + .transparent_output_meta + .into_iter() + .enumerate() + .map(|(n, (recipient, address, value, step_output_index))| { // This assumes that transparent outputs are pushed onto `transparent_output_meta` // with the same indices they have in the transaction's transparent outputs. // We do not reorder transparent outputs; there is no reason to do so because it @@ -1089,12 +1172,11 @@ where let recipient = recipient.map_ephemeral_transparent_outpoint(|()| outpoint.clone()); #[cfg(feature = "transparent-inputs")] unused_transparent_outputs.insert( - StepOutput::new(step_index, step_output_index), + StepOutput::new(build_state.step_index, step_output_index), (address, outpoint), ); SentTransactionOutput::from_parts(n, recipient, value, None) - }, - ); + }); let mut outputs: Vec> = vec![]; #[cfg(feature = "orchard")] @@ -1107,7 +1189,7 @@ where outputs, fee_amount: proposal_step.balance().fee_required(), #[cfg(feature = "transparent-inputs")] - utxos_spent, + utxos_spent: build_state.utxos_spent, }) } diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 855f4c233..9d70a7538 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -660,6 +660,7 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< /// /// Upon success, returns a tuple containing the final transaction, and the /// [`SaplingMetadata`] generated during the build process. + #[allow(clippy::too_many_arguments)] pub fn build( self, rng: R, @@ -692,6 +693,7 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< self.build_internal(rng, spend_prover, output_prover, fee) } + #[allow(clippy::too_many_arguments)] fn build_internal( self, mut rng: R, From 9a02e45bbadd48447c7b3e373d6f5a15d1f96a84 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 4 Dec 2024 23:56:10 +0000 Subject: [PATCH 02/12] zcash_primitives: Provide Orchard saks at build instead of spend add --- pczt/tests/end_to_end.rs | 2 +- .../src/data_api/testing/pool.rs | 1 + zcash_client_backend/src/data_api/wallet.rs | 20 ++++++++-- .../init/migrations/receiving_key_scopes.rs | 1 + zcash_primitives/CHANGELOG.md | 7 ++++ zcash_primitives/src/transaction/builder.rs | 37 +++++++++---------- 6 files changed, 43 insertions(+), 25 deletions(-) diff --git a/pczt/tests/end_to_end.rs b/pczt/tests/end_to_end.rs index e2e59b30e..525e89bfc 100644 --- a/pczt/tests/end_to_end.rs +++ b/pczt/tests/end_to_end.rs @@ -350,7 +350,7 @@ fn orchard_to_orchard() { }, ); builder - .add_orchard_spend::(&orchard_sk, note, merkle_path) + .add_orchard_spend::(orchard_fvk.clone(), note, merkle_path) .unwrap(); builder .add_orchard_output::( diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index ba1ae5772..96ae41d5a 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -742,6 +742,7 @@ pub fn send_multi_step_proposed_transfer( let test_prover = LocalTxProver::bundled(); let build_result = builder .build( + &[], OsRng, &test_prover, &test_prover, diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index acffa9ec9..747ed7a87 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -458,6 +458,8 @@ struct BuildState<'a, P, AccountId> { step_index: usize, builder: Builder<'a, P, ()>, #[cfg(feature = "orchard")] + orchard_saks: Vec, + #[cfg(feature = "orchard")] orchard_output_meta: Vec<( Recipient, NonNegativeAmount, @@ -644,6 +646,8 @@ where orchard_anchor, }, ); + #[cfg(feature = "orchard")] + let mut orchard_saks = vec![]; #[cfg(all(feature = "transparent-inputs", not(feature = "orchard")))] let has_shielded_inputs = !sapling_inputs.is_empty(); @@ -656,7 +660,8 @@ where #[cfg(feature = "orchard")] for (orchard_note, merkle_path) in orchard_inputs.into_iter() { - builder.add_orchard_spend(usk.orchard(), *orchard_note, merkle_path.into())?; + builder.add_orchard_spend(usk.orchard().into(), *orchard_note, merkle_path.into())?; + orchard_saks.push(usk.orchard().into()); } #[cfg(feature = "transparent-inputs")] @@ -1029,6 +1034,8 @@ where step_index, builder, #[cfg(feature = "orchard")] + orchard_saks, + #[cfg(feature = "orchard")] orchard_output_meta, sapling_output_meta, transparent_output_meta, @@ -1081,9 +1088,14 @@ where )?; // Build the transaction with the specified fee rule - let build_result = build_state - .builder - .build(OsRng, spend_prover, output_prover, fee_rule)?; + #[cfg(feature = "orchard")] + let orchard_saks = &build_state.orchard_saks; + #[cfg(not(feature = "orchard"))] + let orchard_saks = &[]; + let build_result = + build_state + .builder + .build(orchard_saks, OsRng, spend_prover, output_prover, fee_rule)?; #[cfg(feature = "orchard")] let orchard_fvk: orchard::keys::FullViewingKey = usk.orchard().into(); diff --git a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs index 778ea22c2..d6a134e3e 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs @@ -402,6 +402,7 @@ mod tests { let prover = LocalTxProver::bundled(); let res = builder .build( + &[], OsRng, &prover, &prover, diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 4d4e2df42..ff6a5ed9c 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -18,6 +18,13 @@ and this library adheres to Rust's notion of - `impl MapAuth for ()` - `sighash::SighashType` +### Changed +- `zcash_primitives::transaction::builder`: + - `Builder::add_orchard_spend` now takes `orchard::keys::FullViewingKey` + instead of `&orchard::keys::SpendingKey`. + - `Builder::build` now takes an `&[orchard::keys::SpendAuthorizingKey]` + argument. + ## [0.20.0] - 2024-11-14 ### Added diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 9d70a7538..1394ee248 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -311,7 +311,6 @@ pub struct Builder<'a, P, U: sapling::builder::ProverProgress> { // transaction, and then the caller will be responsible for using the spending keys or their // derivatives for proving and signing to complete transaction creation. sapling_asks: Vec, - orchard_saks: Vec, #[cfg(zcash_unstable = "zfuture")] tze_builder: TzeBuilder<'a, TransactionData>, #[cfg(not(zcash_unstable = "zfuture"))] @@ -396,7 +395,6 @@ impl<'a, P: consensus::Parameters> Builder<'a, P, ()> { sapling_builder, orchard_builder, sapling_asks: vec![], - orchard_saks: Vec::new(), #[cfg(zcash_unstable = "zfuture")] tze_builder: TzeBuilder::empty(), #[cfg(not(zcash_unstable = "zfuture"))] @@ -424,7 +422,6 @@ impl<'a, P: consensus::Parameters> Builder<'a, P, ()> { sapling_builder: self.sapling_builder, orchard_builder: self.orchard_builder, sapling_asks: self.sapling_asks, - orchard_saks: self.orchard_saks, tze_builder: self.tze_builder, progress_notifier, } @@ -438,16 +435,12 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< /// the given note. pub fn add_orchard_spend( &mut self, - sk: &orchard::keys::SpendingKey, + fvk: orchard::keys::FullViewingKey, note: orchard::Note, merkle_path: orchard::tree::MerklePath, ) -> Result<(), Error> { if let Some(builder) = self.orchard_builder.as_mut() { - builder.add_spend(orchard::keys::FullViewingKey::from(sk), note, merkle_path)?; - - self.orchard_saks - .push(orchard::keys::SpendAuthorizingKey::from(sk)); - + builder.add_spend(fvk, note, merkle_path)?; Ok(()) } else { Err(Error::OrchardBuilderNotAvailable) @@ -663,13 +656,14 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< #[allow(clippy::too_many_arguments)] pub fn build( self, + orchard_saks: &[orchard::keys::SpendAuthorizingKey], rng: R, spend_prover: &SP, output_prover: &OP, fee_rule: &FR, ) -> Result> { let fee = self.get_fee(fee_rule).map_err(Error::Fee)?; - self.build_internal(rng, spend_prover, output_prover, fee) + self.build_internal(orchard_saks, rng, spend_prover, output_prover, fee) } /// Builds a transaction from the configured spends and outputs. @@ -684,18 +678,20 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< FR: FutureFeeRule, >( self, + orchard_saks: &[orchard::keys::SpendAuthorizingKey], rng: R, spend_prover: &SP, output_prover: &OP, fee_rule: &FR, ) -> Result> { let fee = self.get_fee_zfuture(fee_rule).map_err(Error::Fee)?; - self.build_internal(rng, spend_prover, output_prover, fee) + self.build_internal(orchard_saks, rng, spend_prover, output_prover, fee) } #[allow(clippy::too_many_arguments)] fn build_internal( self, + orchard_saks: &[orchard::keys::SpendAuthorizingKey], mut rng: R, spend_prover: &SP, output_prover: &OP, @@ -834,7 +830,7 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< b.apply_signatures( &mut rng, *shielded_sig_commitment.as_ref(), - &self.orchard_saks, + orchard_saks, ) }) }) @@ -997,6 +993,7 @@ mod testing { /// DO NOT USE EXCEPT FOR UNIT TESTING. pub fn mock_build( self, + orchard_saks: &[orchard::keys::SpendAuthorizingKey], rng: R, ) -> Result> { struct FakeCryptoRng(R); @@ -1020,6 +1017,7 @@ mod testing { } self.build( + orchard_saks, FakeCryptoRng(rng), &MockSpendProver, &MockOutputProver, @@ -1094,7 +1092,6 @@ mod tests { progress_notifier: (), orchard_builder: None, sapling_asks: vec![], - orchard_saks: Vec::new(), }; let tsk = AccountPrivKey::from_seed(&TEST_NETWORK, &[0u8; 32], AccountId::ZERO).unwrap(); @@ -1125,7 +1122,7 @@ mod tests { ) .unwrap(); - let res = builder.mock_build(OsRng).unwrap(); + let res = builder.mock_build(&[], OsRng).unwrap(); // No binding signature, because only t input and outputs assert!(res.transaction().sapling_bundle.is_none()); } @@ -1170,7 +1167,7 @@ mod tests { .unwrap(); // A binding signature (and bundle) is present because there is a Sapling spend. - let res = builder.mock_build(OsRng).unwrap(); + let res = builder.mock_build(&[], OsRng).unwrap(); assert!(res.transaction().sapling_bundle().is_some()); } @@ -1195,7 +1192,7 @@ mod tests { }; let builder = Builder::new(TEST_NETWORK, tx_height, build_config); assert_matches!( - builder.mock_build(OsRng), + builder.mock_build(&[], OsRng), Err(Error::InsufficientFunds(expected)) if expected == MINIMUM_FEE.into() ); } @@ -1221,7 +1218,7 @@ mod tests { ) .unwrap(); assert_matches!( - builder.mock_build(OsRng), + builder.mock_build(&[], OsRng), Err(Error::InsufficientFunds(expected)) if expected == (NonNegativeAmount::const_from_u64(50000) + MINIMUM_FEE).unwrap().into() ); @@ -1242,7 +1239,7 @@ mod tests { ) .unwrap(); assert_matches!( - builder.mock_build(OsRng), + builder.mock_build(&[], OsRng), Err(Error::InsufficientFunds(expected)) if expected == (NonNegativeAmount::const_from_u64(50000) + MINIMUM_FEE).unwrap().into() ); @@ -1283,7 +1280,7 @@ mod tests { ) .unwrap(); assert_matches!( - builder.mock_build(OsRng), + builder.mock_build(&[], OsRng), Err(Error::InsufficientFunds(expected)) if expected == Amount::const_from_i64(1) ); } @@ -1325,7 +1322,7 @@ mod tests { NonNegativeAmount::const_from_u64(15000), ) .unwrap(); - let res = builder.mock_build(OsRng).unwrap(); + let res = builder.mock_build(&[], OsRng).unwrap(); assert_eq!( res.transaction() .fee_paid(|_| Err(BalanceError::Overflow)) From b209e4bc4db381f75ac99d9a7d3f9ababf0b5814 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 5 Dec 2024 02:04:40 +0000 Subject: [PATCH 03/12] zcash_primitives: Move transparent keys into a separate signing set --- pczt/tests/end_to_end.rs | 4 +- .../src/data_api/testing/pool.rs | 7 +- zcash_client_backend/src/data_api/wallet.rs | 27 +++-- .../init/migrations/receiving_key_scopes.rs | 15 ++- zcash_primitives/CHANGELOG.md | 14 ++- zcash_primitives/src/transaction/builder.rs | 90 +++++++++++----- .../components/transparent/builder.rs | 100 ++++++++++++------ 7 files changed, 186 insertions(+), 71 deletions(-) diff --git a/pczt/tests/end_to_end.rs b/pczt/tests/end_to_end.rs index 525e89bfc..77e87f160 100644 --- a/pczt/tests/end_to_end.rs +++ b/pczt/tests/end_to_end.rs @@ -49,6 +49,8 @@ fn transparent_to_orchard() { let transparent_sk = transparent_account_sk .derive_external_secret_key(address_index) .unwrap(); + let secp = secp256k1::Secp256k1::signing_only(); + let transparent_pubkey = transparent_sk.public_key(&secp); // Create an Orchard account to receive funds. let orchard_sk = orchard::keys::SpendingKey::from_bytes([0; 32]).unwrap(); @@ -73,7 +75,7 @@ fn transparent_to_orchard() { }, ); builder - .add_transparent_input(transparent_sk, utxo, coin) + .add_transparent_input(transparent_pubkey, utxo, coin) .unwrap(); builder .add_orchard_output::( diff --git a/zcash_client_backend/src/data_api/testing/pool.rs b/zcash_client_backend/src/data_api/testing/pool.rs index 96ae41d5a..143959be2 100644 --- a/zcash_client_backend/src/data_api/testing/pool.rs +++ b/zcash_client_backend/src/data_api/testing/pool.rs @@ -494,6 +494,8 @@ pub fn send_multi_step_proposed_transfer( DSF: DataStoreFactory, ::AccountId: std::fmt::Debug, { + use zcash_primitives::transaction::components::transparent::builder::TransparentSigningSet; + use crate::data_api::{OutputOfSentTx, GAP_LIMIT}; let mut st = TestBuilder::new() @@ -715,6 +717,7 @@ pub fn send_multi_step_proposed_transfer( orchard_anchor: None, }, ); + let mut transparent_signing_set = TransparentSigningSet::new(); let (colliding_addr, _) = &known_addrs[10]; let utxo_value = (value - zip317::MINIMUM_FEE).unwrap(); assert_matches!( @@ -726,6 +729,7 @@ pub fn send_multi_step_proposed_transfer( .transparent() .derive_secret_key(Scope::External.into(), default_index) .unwrap(); + let pubkey = transparent_signing_set.add_key(sk); let outpoint = OutPoint::fake(); let txout = TxOut { script_pubkey: default_addr.script(), @@ -738,10 +742,11 @@ pub fn send_multi_step_proposed_transfer( ) .unwrap(); - assert_matches!(builder.add_transparent_input(sk, outpoint, txout), Ok(_)); + assert_matches!(builder.add_transparent_input(pubkey, outpoint, txout), Ok(_)); let test_prover = LocalTxProver::bundled(); let build_result = builder .build( + &transparent_signing_set, &[], OsRng, &test_prover, diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 747ed7a87..c2def9810 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -63,7 +63,10 @@ use zcash_primitives::{ legacy::TransparentAddress, transaction::{ builder::{BuildConfig, BuildResult, Builder}, - components::{amount::NonNegativeAmount, sapling::zip212_enforcement, OutPoint}, + components::{ + amount::NonNegativeAmount, sapling::zip212_enforcement, + transparent::builder::TransparentSigningSet, OutPoint, + }, fees::FeeRule, Transaction, TxId, }, @@ -457,6 +460,7 @@ struct BuildState<'a, P, AccountId> { #[cfg(feature = "transparent-inputs")] step_index: usize, builder: Builder<'a, P, ()>, + transparent_signing_set: TransparentSigningSet, #[cfg(feature = "orchard")] orchard_saks: Vec, #[cfg(feature = "orchard")] @@ -646,6 +650,8 @@ where orchard_anchor, }, ); + #[cfg_attr(not(feature = "transparent-inputs"), allow(unused_mut))] + let mut transparent_signing_set = TransparentSigningSet::new(); #[cfg(feature = "orchard")] let mut orchard_saks = vec![]; @@ -698,6 +704,7 @@ where let mut utxos_spent: Vec = vec![]; let add_transparent_input = |builder: &mut Builder<_, _>, + transparent_signing_set: &mut TransparentSigningSet, utxos_spent: &mut Vec<_>, address_metadata: &TransparentAddressMetadata, outpoint: OutPoint, @@ -707,9 +714,10 @@ where .transparent() .derive_secret_key(address_metadata.scope(), address_metadata.address_index()) .expect("spending key derivation should not fail"); + let pubkey = transparent_signing_set.add_key(secret_key); utxos_spent.push(outpoint.clone()); - builder.add_transparent_input(secret_key, outpoint, txout)?; + builder.add_transparent_input(pubkey, outpoint, txout)?; Ok(()) }; @@ -717,6 +725,7 @@ where for utxo in proposal_step.transparent_inputs() { add_transparent_input( &mut builder, + &mut transparent_signing_set, &mut utxos_spent, &metadata_from_address(*utxo.recipient_address())?, utxo.outpoint().clone(), @@ -742,6 +751,7 @@ where add_transparent_input( &mut builder, + &mut transparent_signing_set, &mut utxos_spent, &address_metadata, outpoint, @@ -1033,6 +1043,7 @@ where #[cfg(feature = "transparent-inputs")] step_index, builder, + transparent_signing_set, #[cfg(feature = "orchard")] orchard_saks, #[cfg(feature = "orchard")] @@ -1092,10 +1103,14 @@ where let orchard_saks = &build_state.orchard_saks; #[cfg(not(feature = "orchard"))] let orchard_saks = &[]; - let build_result = - build_state - .builder - .build(orchard_saks, OsRng, spend_prover, output_prover, fee_rule)?; + let build_result = build_state.builder.build( + &build_state.transparent_signing_set, + orchard_saks, + OsRng, + spend_prover, + output_prover, + fee_rule, + )?; #[cfg(feature = "orchard")] let orchard_fvk: orchard::keys::FullViewingKey = usk.orchard().into(); diff --git a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs index d6a134e3e..7dbd78cfa 100644 --- a/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs +++ b/zcash_client_sqlite/src/wallet/init/migrations/receiving_key_scopes.rs @@ -306,7 +306,10 @@ mod tests { memo::MemoBytes, transaction::{ builder::{BuildConfig, BuildResult, Builder}, - components::{amount::NonNegativeAmount, transparent}, + components::{ + amount::NonNegativeAmount, + transparent::{self, builder::TransparentSigningSet}, + }, fees::fixed, Transaction, }, @@ -364,11 +367,14 @@ mod tests { orchard_anchor: None, }, ); + let mut transparent_signing_set = TransparentSigningSet::new(); builder .add_transparent_input( - usk0.transparent() - .derive_external_secret_key(NonHardenedChildIndex::ZERO) - .unwrap(), + transparent_signing_set.add_key( + usk0.transparent() + .derive_external_secret_key(NonHardenedChildIndex::ZERO) + .unwrap(), + ), transparent::OutPoint::fake(), transparent::TxOut { value: NonNegativeAmount::const_from_u64(EXTERNAL_VALUE + INTERNAL_VALUE), @@ -402,6 +408,7 @@ mod tests { let prover = LocalTxProver::bundled(); let res = builder .build( + &transparent_signing_set, &[], OsRng, &prover, diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index ff6a5ed9c..d73a978b4 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -16,14 +16,24 @@ and this library adheres to Rust's notion of - `pczt` module. - `EffectsOnly` - `impl MapAuth for ()` + - `builder::TransparentSigningSet` - `sighash::SighashType` ### Changed +- `zcash_primitives::transaction::components::transparent`: + - `builder::TransparentBuilder::add_input` now takes `secp256k1::PublicKey` + instead of `secp256k1::SecretKey`. + - `Bundle::apply_signatures` now takes an additional argument + `&TransparentSigningSet`. + - `builder::Error` has a new variant `MissingSigningKey`. - `zcash_primitives::transaction::builder`: - `Builder::add_orchard_spend` now takes `orchard::keys::FullViewingKey` instead of `&orchard::keys::SpendingKey`. - - `Builder::build` now takes an `&[orchard::keys::SpendAuthorizingKey]` - argument. + - `Builder::add_transparent_input` now takes `secp256k1::PublicKey` instead of + `secp256k1::SecretKey`. + - `Builder::build` now takes several additional arguments: + - `&TransparentSigningSet` + - `&[orchard::keys::SpendAuthorizingKey]` ## [0.20.0] - 2024-11-14 diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 1394ee248..e6f46408d 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -53,6 +53,7 @@ use crate::{ use super::components::amount::NonNegativeAmount; use super::components::sapling::zip212_enforcement; +use super::components::transparent::builder::TransparentSigningSet; /// Since Blossom activation, the default transaction expiry delta should be 40 blocks. /// @@ -511,11 +512,11 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< #[cfg(feature = "transparent-inputs")] pub fn add_transparent_input( &mut self, - sk: secp256k1::SecretKey, + pubkey: secp256k1::PublicKey, utxo: transparent::OutPoint, coin: TxOut, ) -> Result<(), transparent::builder::Error> { - self.transparent_builder.add_input(sk, utxo, coin) + self.transparent_builder.add_input(pubkey, utxo, coin) } /// Adds a transparent address to send funds to. @@ -656,6 +657,7 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< #[allow(clippy::too_many_arguments)] pub fn build( self, + transparent_signing_set: &TransparentSigningSet, orchard_saks: &[orchard::keys::SpendAuthorizingKey], rng: R, spend_prover: &SP, @@ -663,7 +665,14 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< fee_rule: &FR, ) -> Result> { let fee = self.get_fee(fee_rule).map_err(Error::Fee)?; - self.build_internal(orchard_saks, rng, spend_prover, output_prover, fee) + self.build_internal( + transparent_signing_set, + orchard_saks, + rng, + spend_prover, + output_prover, + fee, + ) } /// Builds a transaction from the configured spends and outputs. @@ -678,6 +687,7 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< FR: FutureFeeRule, >( self, + transparent_signing_set: &TransparentSigningSet, orchard_saks: &[orchard::keys::SpendAuthorizingKey], rng: R, spend_prover: &SP, @@ -685,12 +695,20 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< fee_rule: &FR, ) -> Result> { let fee = self.get_fee_zfuture(fee_rule).map_err(Error::Fee)?; - self.build_internal(orchard_saks, rng, spend_prover, output_prover, fee) + self.build_internal( + transparent_signing_set, + orchard_saks, + rng, + spend_prover, + output_prover, + fee, + ) } #[allow(clippy::too_many_arguments)] fn build_internal( self, + transparent_signing_set: &TransparentSigningSet, orchard_saks: &[orchard::keys::SpendAuthorizingKey], mut rng: R, spend_prover: &SP, @@ -787,14 +805,20 @@ impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder< // let txid_parts = unauthed_tx.digest(TxIdDigester); - let transparent_bundle = unauthed_tx.transparent_bundle.clone().map(|b| { - b.apply_signatures( - #[cfg(feature = "transparent-inputs")] - &unauthed_tx, - #[cfg(feature = "transparent-inputs")] - &txid_parts, - ) - }); + let transparent_bundle = unauthed_tx + .transparent_bundle + .clone() + .map(|b| { + b.apply_signatures( + #[cfg(feature = "transparent-inputs")] + &unauthed_tx, + #[cfg(feature = "transparent-inputs")] + &txid_parts, + transparent_signing_set, + ) + }) + .transpose() + .map_err(Error::TransparentBuild)?; #[cfg(zcash_unstable = "zfuture")] let tze_bundle = unauthed_tx @@ -985,7 +1009,7 @@ mod testing { self, prover::mock::{MockOutputProver, MockSpendProver}, }, - transaction::fees::zip317, + transaction::{components::transparent::builder::TransparentSigningSet, fees::zip317}, }; impl<'a, P: consensus::Parameters, U: sapling::builder::ProverProgress> Builder<'a, P, U> { @@ -993,6 +1017,7 @@ mod testing { /// DO NOT USE EXCEPT FOR UNIT TESTING. pub fn mock_build( self, + transparent_signing_set: &TransparentSigningSet, orchard_saks: &[orchard::keys::SpendAuthorizingKey], rng: R, ) -> Result> { @@ -1017,6 +1042,7 @@ mod testing { } self.build( + transparent_signing_set, orchard_saks, FakeCryptoRng(rng), &MockSpendProver, @@ -1044,7 +1070,10 @@ mod tests { sapling::{self, zip32::ExtendedSpendingKey, Node, Rseed}, transaction::{ builder::BuildConfig, - components::amount::{Amount, BalanceError, NonNegativeAmount}, + components::{ + amount::{Amount, BalanceError, NonNegativeAmount}, + transparent::builder::TransparentSigningSet, + }, }, }; @@ -1069,6 +1098,7 @@ mod tests { use crate::consensus::NetworkUpgrade; use crate::legacy::keys::NonHardenedChildIndex; use crate::transaction::builder::{self, TransparentBuilder}; + use crate::transaction::components::transparent::builder::TransparentSigningSet; let sapling_activation_height = TEST_NETWORK .activation_height(NetworkUpgrade::Sapling) @@ -1094,7 +1124,12 @@ mod tests { sapling_asks: vec![], }; + let mut transparent_signing_set = TransparentSigningSet::new(); let tsk = AccountPrivKey::from_seed(&TEST_NETWORK, &[0u8; 32], AccountId::ZERO).unwrap(); + let sk = tsk + .derive_external_secret_key(NonHardenedChildIndex::ZERO) + .unwrap(); + let pubkey = transparent_signing_set.add_key(sk); let prev_coin = TxOut { value: NonNegativeAmount::const_from_u64(50000), script_pubkey: tsk @@ -1106,12 +1141,7 @@ mod tests { .script(), }; builder - .add_transparent_input( - tsk.derive_external_secret_key(NonHardenedChildIndex::ZERO) - .unwrap(), - OutPoint::fake(), - prev_coin, - ) + .add_transparent_input(pubkey, OutPoint::fake(), prev_coin) .unwrap(); // Create a tx with only t output. No binding_sig should be present @@ -1122,7 +1152,9 @@ mod tests { ) .unwrap(); - let res = builder.mock_build(&[], OsRng).unwrap(); + let res = builder + .mock_build(&transparent_signing_set, &[], OsRng) + .unwrap(); // No binding signature, because only t input and outputs assert!(res.transaction().sapling_bundle.is_none()); } @@ -1167,7 +1199,9 @@ mod tests { .unwrap(); // A binding signature (and bundle) is present because there is a Sapling spend. - let res = builder.mock_build(&[], OsRng).unwrap(); + let res = builder + .mock_build(&TransparentSigningSet::new(), &[], OsRng) + .unwrap(); assert!(res.transaction().sapling_bundle().is_some()); } @@ -1192,7 +1226,7 @@ mod tests { }; let builder = Builder::new(TEST_NETWORK, tx_height, build_config); assert_matches!( - builder.mock_build(&[], OsRng), + builder.mock_build(&TransparentSigningSet::new(), &[], OsRng), Err(Error::InsufficientFunds(expected)) if expected == MINIMUM_FEE.into() ); } @@ -1218,7 +1252,7 @@ mod tests { ) .unwrap(); assert_matches!( - builder.mock_build(&[], OsRng), + builder.mock_build(&TransparentSigningSet::new(), &[], OsRng), Err(Error::InsufficientFunds(expected)) if expected == (NonNegativeAmount::const_from_u64(50000) + MINIMUM_FEE).unwrap().into() ); @@ -1239,7 +1273,7 @@ mod tests { ) .unwrap(); assert_matches!( - builder.mock_build(&[], OsRng), + builder.mock_build(&TransparentSigningSet::new(), &[], OsRng), Err(Error::InsufficientFunds(expected)) if expected == (NonNegativeAmount::const_from_u64(50000) + MINIMUM_FEE).unwrap().into() ); @@ -1280,7 +1314,7 @@ mod tests { ) .unwrap(); assert_matches!( - builder.mock_build(&[], OsRng), + builder.mock_build(&TransparentSigningSet::new(), &[], OsRng), Err(Error::InsufficientFunds(expected)) if expected == Amount::const_from_i64(1) ); } @@ -1322,7 +1356,9 @@ mod tests { NonNegativeAmount::const_from_u64(15000), ) .unwrap(); - let res = builder.mock_build(&[], OsRng).unwrap(); + let res = builder + .mock_build(&TransparentSigningSet::new(), &[], OsRng) + .unwrap(); assert_eq!( res.transaction() .fee_paid(|_| Err(BalanceError::Overflow)) diff --git a/zcash_primitives/src/transaction/components/transparent/builder.rs b/zcash_primitives/src/transaction/components/transparent/builder.rs index f0841e5e0..b076627e5 100644 --- a/zcash_primitives/src/transaction/components/transparent/builder.rs +++ b/zcash_primitives/src/transaction/components/transparent/builder.rs @@ -30,6 +30,8 @@ use { pub enum Error { InvalidAddress, InvalidAmount, + /// A bundle could not be built because a required signing keys was missing. + MissingSigningKey, } impl fmt::Display for Error { @@ -37,15 +39,56 @@ impl fmt::Display for Error { match self { Error::InvalidAddress => write!(f, "Invalid address"), Error::InvalidAmount => write!(f, "Invalid amount"), + Error::MissingSigningKey => write!(f, "Missing signing key"), } } } +/// A set of transparent signing keys. +/// +/// When the `transparent-inputs` feature flag is enabled, transparent signing keys can be +/// stored in this set and used to authorize transactions with transparent inputs. +pub struct TransparentSigningSet { + #[cfg(feature = "transparent-inputs")] + secp: secp256k1::Secp256k1, + #[cfg(feature = "transparent-inputs")] + keys: Vec<(secp256k1::SecretKey, secp256k1::PublicKey)>, +} + +impl Default for TransparentSigningSet { + fn default() -> Self { + Self::new() + } +} + +impl TransparentSigningSet { + /// Constructs an empty set of signing keys. + pub fn new() -> Self { + Self { + #[cfg(feature = "transparent-inputs")] + secp: secp256k1::Secp256k1::gen_new(), + #[cfg(feature = "transparent-inputs")] + keys: vec![], + } + } + + /// Adds a signing key to the set. + /// + /// Returns the corresponding pubkey. + #[cfg(feature = "transparent-inputs")] + pub fn add_key(&mut self, sk: secp256k1::SecretKey) -> secp256k1::PublicKey { + let pubkey = secp256k1::PublicKey::from_secret_key(&self.secp, &sk); + // Cache the pubkey for ease of matching later. + self.keys.push((sk, pubkey)); + pubkey + } +} + +// TODO: This feature gate can be removed. #[cfg(feature = "transparent-inputs")] #[derive(Debug, Clone)] pub struct TransparentInputInfo { - sk: secp256k1::SecretKey, - pubkey: [u8; secp256k1::constants::PUBLIC_KEY_SIZE], + pubkey: secp256k1::PublicKey, utxo: OutPoint, coin: TxOut, } @@ -62,8 +105,6 @@ impl TransparentInputInfo { } pub struct TransparentBuilder { - #[cfg(feature = "transparent-inputs")] - secp: secp256k1::Secp256k1, #[cfg(feature = "transparent-inputs")] inputs: Vec, vout: Vec, @@ -71,8 +112,6 @@ pub struct TransparentBuilder { #[derive(Debug, Clone)] pub struct Unauthorized { - #[cfg(feature = "transparent-inputs")] - secp: secp256k1::Secp256k1, #[cfg(feature = "transparent-inputs")] inputs: Vec, } @@ -85,8 +124,6 @@ impl TransparentBuilder { /// Constructs a new TransparentBuilder pub fn empty() -> Self { TransparentBuilder { - #[cfg(feature = "transparent-inputs")] - secp: secp256k1::Secp256k1::gen_new(), #[cfg(feature = "transparent-inputs")] inputs: vec![], vout: vec![], @@ -109,32 +146,27 @@ impl TransparentBuilder { #[cfg(feature = "transparent-inputs")] pub fn add_input( &mut self, - sk: secp256k1::SecretKey, + pubkey: secp256k1::PublicKey, utxo: OutPoint, coin: TxOut, ) -> Result<(), Error> { // Ensure that the RIPEMD-160 digest of the public key associated with the // provided secret key matches that of the address to which the provided // output may be spent. - let pubkey = secp256k1::PublicKey::from_secret_key(&self.secp, &sk).serialize(); match coin.script_pubkey.address() { Some(TransparentAddress::PublicKeyHash(hash)) => { use ripemd::Ripemd160; use sha2::Sha256; - if hash[..] != Ripemd160::digest(Sha256::digest(pubkey))[..] { + if hash[..] != Ripemd160::digest(Sha256::digest(pubkey.serialize()))[..] { return Err(Error::InvalidAddress); } } _ => return Err(Error::InvalidAddress), } - self.inputs.push(TransparentInputInfo { - sk, - pubkey, - utxo, - coin, - }); + self.inputs + .push(TransparentInputInfo { pubkey, utxo, coin }); Ok(()) } @@ -192,8 +224,6 @@ impl TransparentBuilder { vin, vout: self.vout, authorization: Unauthorized { - #[cfg(feature = "transparent-inputs")] - secp: self.secp, #[cfg(feature = "transparent-inputs")] inputs: self.inputs, }, @@ -296,7 +326,8 @@ impl Bundle { self, #[cfg(feature = "transparent-inputs")] mtx: &TransactionData, #[cfg(feature = "transparent-inputs")] txid_parts_cache: &TxDigests, - ) -> Bundle { + signing_set: &TransparentSigningSet, + ) -> Result, Error> { #[cfg(feature = "transparent-inputs")] let script_sigs = self .authorization @@ -304,6 +335,13 @@ impl Bundle { .iter() .enumerate() .map(|(index, info)| { + // Find the matching signing key. + let (sk, _) = signing_set + .keys + .iter() + .find(|(_, pubkey)| pubkey == &info.pubkey) + .ok_or(Error::MissingSigningKey)?; + let sighash = signature_hash( mtx, &SignableInput::Transparent { @@ -317,32 +355,34 @@ impl Bundle { ); let msg = secp256k1::Message::from_slice(sighash.as_ref()).expect("32 bytes"); - let sig = self.authorization.secp.sign_ecdsa(&msg, &info.sk); + let sig = signing_set.secp.sign_ecdsa(&msg, sk); // Signature has to have "SIGHASH_ALL" appended to it let mut sig_bytes: Vec = sig.serialize_der()[..].to_vec(); sig_bytes.extend([SIGHASH_ALL]); // P2PKH scriptSig - Script::default() << &sig_bytes[..] << &info.pubkey[..] + Ok(Script::default() << &sig_bytes[..] << &info.pubkey.serialize()[..]) }); #[cfg(not(feature = "transparent-inputs"))] - let script_sigs = std::iter::empty::