From 7f3d057a1b99e65a8e5f4578fc1960404c30cab7 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 28 Nov 2023 10:31:46 -0700 Subject: [PATCH] zcash_primitives: Avoid passing duplicate diversifier information to Sapling builder. The note provided to `add_sapling_spend` contains the recipient address, and we can extract the diversifier from this address, so we should not pass it separately. --- zcash_client_backend/src/data_api/wallet.rs | 2 +- zcash_extensions/src/transparent/demo.rs | 2 +- zcash_primitives/CHANGELOG.md | 6 ++++- zcash_primitives/src/sapling/builder.rs | 30 ++++----------------- zcash_primitives/src/transaction/builder.rs | 23 +++++----------- 5 files changed, 18 insertions(+), 45 deletions(-) diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 093f52452..6f40486f6 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -605,7 +605,7 @@ where }) })?; - builder.add_sapling_spend(key, selected.diversifier(), note, merkle_path)?; + builder.add_sapling_spend(key, note, merkle_path)?; } Ok(()) })?; diff --git a/zcash_extensions/src/transparent/demo.rs b/zcash_extensions/src/transparent/demo.rs index 9171dd132..640f7e617 100644 --- a/zcash_extensions/src/transparent/demo.rs +++ b/zcash_extensions/src/transparent/demo.rs @@ -823,7 +823,7 @@ mod tests { let mut builder_a = demo_builder(tx_height); builder_a - .add_sapling_spend(extsk, *to.diversifier(), note1, witness1.path().unwrap()) + .add_sapling_spend(extsk, note1, witness1.path().unwrap()) .unwrap(); let value = NonNegativeAmount::const_from_u64(100000); diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index e9842d675..1e7b91a33 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -111,7 +111,9 @@ and this library adheres to Rust's notion of parameter. - `builder::SaplingBuilder::new` now takes a `Zip212Enforcement` argument instead of a `P: consensus::Parameters` argument and a target height. - - `builder::SaplingBuilder::add_spend` now takes `extsk` by reference. + - `builder::SaplingBuilder::add_spend` now takes `extsk` by reference. Also, + it no longer takes a `diversifier` argument as the diversifier may be obtained + from the note. - `builder::SaplingBuilder::add_output` now takes an `Option<[u8; 512]>` memo instead of a `MemoBytes`. - `builder::SaplingBuilder::build` no longer takes a prover, proving context, @@ -151,6 +153,8 @@ and this library adheres to Rust's notion of - `zcash_primitives::transaction`: - `builder::Builder::{build, build_zfuture}` now take `&impl SpendProver, &impl OutputProver` instead of `&impl TxProver`. + - `builder::Builder::add_sapling_spend` no longer takes a `diversifier` + argument as the diversifier may be obtained from the note. - `components::transparent::TxOut.value` now has type `NonNegativeAmount` instead of `Amount`. - `Unauthorized::SaplingAuth` now has type `InProgress`. diff --git a/zcash_primitives/src/sapling/builder.rs b/zcash_primitives/src/sapling/builder.rs index e0a4f3e23..656894057 100644 --- a/zcash_primitives/src/sapling/builder.rs +++ b/zcash_primitives/src/sapling/builder.rs @@ -74,7 +74,6 @@ impl fmt::Display for Error { #[derive(Debug, Clone)] pub struct SpendDescriptionInfo { proof_generation_key: ProofGenerationKey, - diversifier: Diversifier, note: Note, alpha: jubjub::Fr, merkle_path: MerklePath, @@ -97,13 +96,11 @@ impl SpendDescriptionInfo { fn new_internal( mut rng: &mut R, extsk: &ExtendedSpendingKey, - diversifier: Diversifier, note: Note, merkle_path: MerklePath, ) -> Self { SpendDescriptionInfo { proof_generation_key: extsk.expsk.proof_generation_key(), - diversifier, note, alpha: jubjub::Fr::random(&mut rng), merkle_path, @@ -133,7 +130,7 @@ impl SpendDescriptionInfo { let zkproof = Pr::prepare_circuit( self.proof_generation_key, - self.diversifier, + *self.note.recipient().diversifier(), *self.note.rseed(), self.note.value(), self.alpha, @@ -370,7 +367,6 @@ impl SaplingBuilder { &mut self, mut rng: R, extsk: &ExtendedSpendingKey, - diversifier: Diversifier, note: Note, merkle_path: MerklePath, ) -> Result<(), Error> { @@ -388,8 +384,7 @@ impl SaplingBuilder { self.value_balance = (self.value_balance + note.value()).ok_or(Error::InvalidAmount)?; self.try_value_balance::()?; - let spend = - SpendDescriptionInfo::new_internal(&mut rng, extsk, diversifier, note, merkle_path); + let spend = SpendDescriptionInfo::new_internal(&mut rng, extsk, note, merkle_path); self.spends.push(spend); @@ -881,7 +876,6 @@ pub mod testing { testing::{arb_node, arb_note}, value::testing::arb_positive_note_value, zip32::testing::arb_extended_spending_key, - Diversifier, }; use incrementalmerkletree::{ frontier::testing::arb_commitment_tree, witness::IncrementalWitness, @@ -907,34 +901,20 @@ pub mod testing { .prop_map(|t| IncrementalWitness::from_tree(t).path().unwrap()), n_notes, ), - vec( - prop::array::uniform11(any::()).prop_map(Diversifier), - n_notes, - ), prop::array::uniform32(any::()), prop::array::uniform32(any::()), ) }) .prop_map( - move |( - extsk, - spendable_notes, - commitment_trees, - diversifiers, - rng_seed, - fake_sighash_bytes, - )| { + move |(extsk, spendable_notes, commitment_trees, rng_seed, fake_sighash_bytes)| { let mut builder = SaplingBuilder::new(zip212_enforcement); let mut rng = StdRng::from_seed(rng_seed); - for ((note, path), diversifier) in spendable_notes + for (note, path) in spendable_notes .into_iter() .zip(commitment_trees.into_iter()) - .zip(diversifiers.into_iter()) { - builder - .add_spend(&mut rng, &extsk, diversifier, note, path) - .unwrap(); + builder.add_spend(&mut rng, &extsk, note, path).unwrap(); } let (bundle, _) = builder diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 3ff1cec40..9ef6cd00c 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -15,7 +15,7 @@ use crate::{ self, builder::{self as sapling_builder, SaplingBuilder, SaplingMetadata}, prover::{OutputProver, SpendProver}, - redjubjub, Diversifier, Note, PaymentAddress, + redjubjub, Note, PaymentAddress, }, transaction::{ components::{ @@ -328,12 +328,11 @@ impl<'a, P: consensus::Parameters, R: RngCore + CryptoRng> Builder<'a, P, R> { pub fn add_sapling_spend( &mut self, extsk: sapling::zip32::ExtendedSpendingKey, - diversifier: Diversifier, note: Note, merkle_path: sapling::MerklePath, ) -> Result<(), sapling_builder::Error> { self.sapling_builder - .add_spend(&mut self.rng, &extsk, diversifier, note, merkle_path)?; + .add_spend(&mut self.rng, &extsk, note, merkle_path)?; self.sapling_asks .push(redjubjub::PrivateKey(extsk.expsk.ask)); @@ -842,7 +841,7 @@ mod tests { // Create a tx with a sapling spend. binding_sig should be present builder - .add_sapling_spend(extsk, *to.diversifier(), note1, witness1.path().unwrap()) + .add_sapling_spend(extsk, note1, witness1.path().unwrap()) .unwrap(); builder @@ -933,12 +932,7 @@ mod tests { { let mut builder = Builder::new(TEST_NETWORK, tx_height, None); builder - .add_sapling_spend( - extsk.clone(), - *to.diversifier(), - note1.clone(), - witness1.path().unwrap(), - ) + .add_sapling_spend(extsk.clone(), note1.clone(), witness1.path().unwrap()) .unwrap(); builder .add_sapling_output( @@ -974,15 +968,10 @@ mod tests { { let mut builder = Builder::new(TEST_NETWORK, tx_height, None); builder - .add_sapling_spend( - extsk.clone(), - *to.diversifier(), - note1, - witness1.path().unwrap(), - ) + .add_sapling_spend(extsk.clone(), note1, witness1.path().unwrap()) .unwrap(); builder - .add_sapling_spend(extsk, *to.diversifier(), note2, witness2.path().unwrap()) + .add_sapling_spend(extsk, note2, witness2.path().unwrap()) .unwrap(); builder .add_sapling_output(