From 06cb76168e8f6dd6c9f9cbaf824389b5e2baf476 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 12 Dec 2023 11:12:10 -0700 Subject: [PATCH 1/4] Rename `Builder::add_recipient` to `add_output`. The term `recipient` is commonly used to refer to the address to which a note is sent; however, a bundle may include multiple outputs to the same recipient. This change is intended to clarify this usage. Co-authored-by: Daira Emma Hopwood --- CHANGELOG.md | 9 +++-- benches/circuit.rs | 2 +- benches/note_decryption.rs | 4 +-- src/builder.rs | 69 +++++++++++++++++++------------------- src/value.rs | 4 +-- tests/builder.rs | 4 +-- 6 files changed, 48 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0bc6b6f..507ce07c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to Rust's notion of ## [Unreleased] +### Changed +- `orchard::builder::Builder::add_recipient` has been renamed to `add_output` + in order to clarify than more than one output of a given transaction may be + sent to the same recipient. + ## [0.6.0] - 2023-09-08 ### Changed - MSRV is now 1.65.0. @@ -22,8 +27,8 @@ and this project adheres to Rust's notion of - `orchard::builder`: - `{SpendInfo::new, InputView, OutputView}` - `Builder::{spends, outputs}` - - `SpendError` - - `OutputError` + - `SpendError` + - `OutputError` ### Changed - MSRV is now 1.60.0. diff --git a/benches/circuit.rs b/benches/circuit.rs index 3140a90a..276a4731 100644 --- a/benches/circuit.rs +++ b/benches/circuit.rs @@ -32,7 +32,7 @@ fn criterion_benchmark(c: &mut Criterion) { ); for _ in 0..num_recipients { builder - .add_recipient(None, recipient, NoteValue::from_raw(10), None) + .add_output(None, recipient, NoteValue::from_raw(10), None) .unwrap(); } let bundle: Bundle<_, i64> = builder.build(rng).unwrap(); diff --git a/benches/note_decryption.rs b/benches/note_decryption.rs index 3ebbc6ed..057c0491 100644 --- a/benches/note_decryption.rs +++ b/benches/note_decryption.rs @@ -52,10 +52,10 @@ fn bench_note_decryption(c: &mut Criterion) { // The builder pads to two actions, and shuffles their order. Add two recipients // so the first action is always decryptable. builder - .add_recipient(None, recipient, NoteValue::from_raw(10), None) + .add_output(None, recipient, NoteValue::from_raw(10), None) .unwrap(); builder - .add_recipient(None, recipient, NoteValue::from_raw(10), None) + .add_output(None, recipient, NoteValue::from_raw(10), None) .unwrap(); let bundle: Bundle<_, i64> = builder.build(rng).unwrap(); bundle diff --git a/src/builder.rs b/src/builder.rs index b2d747b7..496b2080 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -157,16 +157,16 @@ impl SpendInfo { } } -/// Information about a specific recipient to receive funds in an [`Action`]. +/// Information about a specific output to receive funds in an [`Action`]. #[derive(Debug)] -struct RecipientInfo { +struct OutputInfo { ovk: Option, recipient: Address, value: NoteValue, memo: Option<[u8; 512]>, } -impl RecipientInfo { +impl OutputInfo { /// Defined in [Zcash Protocol Spec § 4.8.3: Dummy Notes (Orchard)][orcharddummynotes]. /// /// [orcharddummynotes]: https://zips.z.cash/protocol/nu5.pdf#orcharddummynotes @@ -174,7 +174,7 @@ impl RecipientInfo { let fvk: FullViewingKey = (&SpendingKey::random(rng)).into(); let recipient = fvk.address_at(0u32, Scope::External); - RecipientInfo { + OutputInfo { ovk: None, recipient, value: NoteValue::zero(), @@ -187,12 +187,12 @@ impl RecipientInfo { #[derive(Debug)] struct ActionInfo { spend: SpendInfo, - output: RecipientInfo, + output: OutputInfo, rcv: ValueCommitTrapdoor, } impl ActionInfo { - fn new(spend: SpendInfo, output: RecipientInfo, rng: impl RngCore) -> Self { + fn new(spend: SpendInfo, output: OutputInfo, rng: impl RngCore) -> Self { ActionInfo { spend, output, @@ -256,12 +256,12 @@ impl ActionInfo { } } -/// A builder that constructs a [`Bundle`] from a set of notes to be spent, and recipients +/// A builder that constructs a [`Bundle`] from a set of notes to be spent, and outputs /// to receive funds. #[derive(Debug)] pub struct Builder { spends: Vec, - recipients: Vec, + outputs: Vec, flags: Flags, anchor: Anchor, } @@ -271,7 +271,7 @@ impl Builder { pub fn new(flags: Flags, anchor: Anchor) -> Self { Builder { spends: vec![], - recipients: vec![], + outputs: vec![], flags, anchor, } @@ -323,7 +323,7 @@ impl Builder { } /// Adds an address which will receive funds in this transaction. - pub fn add_recipient( + pub fn add_output( &mut self, ovk: Option, recipient: Address, @@ -334,7 +334,7 @@ impl Builder { return Err(OutputError); } - self.recipients.push(RecipientInfo { + self.outputs.push(OutputInfo { ovk, recipient, value, @@ -353,7 +353,7 @@ impl Builder { /// Returns the action output components that will be produced by the /// transaction being constructed pub fn outputs(&self) -> &Vec { - &self.recipients + &self.outputs } /// The net value of the bundle to be built. The value of all spends, @@ -372,16 +372,16 @@ impl Builder { .iter() .map(|spend| spend.note.value() - NoteValue::zero()) .chain( - self.recipients + self.outputs .iter() - .map(|recipient| NoteValue::zero() - recipient.value), + .map(|output| NoteValue::zero() - output.value), ) .fold(Some(ValueSum::zero()), |acc, note_value| acc? + note_value) .ok_or(OverflowError)?; i64::try_from(value_balance).and_then(|i| V::try_from(i).map_err(|_| value::OverflowError)) } - /// Builds a bundle containing the given spent notes and recipients. + /// Builds a bundle containing the given spent notes and outputs. /// /// The returned bundle will have no proof or signatures; these can be applied with /// [`Bundle::create_proof`] and [`Bundle::apply_signatures`] respectively. @@ -389,11 +389,11 @@ impl Builder { mut self, mut rng: impl RngCore, ) -> Result, V>, BuildError> { - // Pair up the spends and recipients, extending with dummy values as necessary. + // Pair up the spends and outputs, extending with dummy values as necessary. let pre_actions: Vec<_> = { let num_spends = self.spends.len(); - let num_recipients = self.recipients.len(); - let num_actions = [num_spends, num_recipients, MIN_ACTIONS] + let num_outputs = self.outputs.len(); + let num_actions = [num_spends, num_outputs, MIN_ACTIONS] .iter() .max() .cloned() @@ -402,21 +402,20 @@ impl Builder { self.spends.extend( iter::repeat_with(|| SpendInfo::dummy(&mut rng)).take(num_actions - num_spends), ); - self.recipients.extend( - iter::repeat_with(|| RecipientInfo::dummy(&mut rng)) - .take(num_actions - num_recipients), + self.outputs.extend( + iter::repeat_with(|| OutputInfo::dummy(&mut rng)).take(num_actions - num_outputs), ); - // Shuffle the spends and recipients, so that learning the position of a + // Shuffle the spends and outputs, so that learning the position of a // specific spent note or output note doesn't reveal anything on its own about // the meaning of that note in the transaction context. self.spends.shuffle(&mut rng); - self.recipients.shuffle(&mut rng); + self.outputs.shuffle(&mut rng); self.spends .into_iter() - .zip(self.recipients.into_iter()) - .map(|(spend, recipient)| ActionInfo::new(spend, recipient, &mut rng)) + .zip(self.outputs.into_iter()) + .map(|(spend, output)| ActionInfo::new(spend, output, &mut rng)) .collect() }; @@ -749,7 +748,7 @@ pub trait OutputView { fn value>(&self) -> V; } -impl OutputView for RecipientInfo { +impl OutputView for OutputInfo { fn value>(&self) -> V { V::from(self.value.inner()) } @@ -793,7 +792,7 @@ pub mod testing { sk: SpendingKey, anchor: Anchor, notes: Vec<(Note, MerklePath)>, - recipient_amounts: Vec<(Address, NoteValue)>, + output_amounts: Vec<(Address, NoteValue)>, } impl ArbitraryBundleInputs { @@ -807,12 +806,12 @@ pub mod testing { builder.add_spend(fvk.clone(), note, path).unwrap(); } - for (addr, value) in self.recipient_amounts.into_iter() { + for (addr, value) in self.output_amounts.into_iter() { let scope = fvk.scope_for_address(&addr).unwrap(); let ovk = fvk.to_ovk(scope); builder - .add_recipient(Some(ovk.clone()), addr, value, None) + .add_output(Some(ovk.clone()), addr, value, None) .unwrap(); } @@ -834,7 +833,7 @@ pub mod testing { fn arb_bundle_inputs(sk: SpendingKey) ( n_notes in 1usize..30, - n_recipients in 1..30, + n_outputs in 1..30, ) ( @@ -843,12 +842,12 @@ pub mod testing { arb_positive_note_value(MAX_NOTE_VALUE / n_notes as u64).prop_flat_map(arb_note), n_notes ), - recipient_amounts in vec( + output_amounts in vec( arb_address().prop_flat_map(move |a| { - arb_positive_note_value(MAX_NOTE_VALUE / n_recipients as u64) + arb_positive_note_value(MAX_NOTE_VALUE / n_outputs as u64) .prop_map(move |v| (a, v)) }), - n_recipients as usize + n_outputs as usize ), rng_seed in prop::array::uniform32(prop::num::u8::ANY) ) -> ArbitraryBundleInputs { @@ -873,7 +872,7 @@ pub mod testing { sk, anchor: frontier.root().into(), notes: notes_and_auth_paths, - recipient_amounts + output_amounts } } } @@ -922,7 +921,7 @@ mod tests { ); builder - .add_recipient(None, recipient, NoteValue::from_raw(5000), None) + .add_output(None, recipient, NoteValue::from_raw(5000), None) .unwrap(); let balance: i64 = builder.value_balance().unwrap(); assert_eq!(balance, -5000); diff --git a/src/value.rs b/src/value.rs index 2f86c723..302c0b3f 100644 --- a/src/value.rs +++ b/src/value.rs @@ -16,7 +16,7 @@ //! - Define your `valueBalanceOrchard` type to enforce your valid value range. This can //! be checked in its `TryFrom` implementation. //! - Define your own "amount" type for note values, and convert it to `NoteValue` prior -//! to calling [`Builder::add_recipient`]. +//! to calling [`Builder::add_output`]. //! //! Inside the circuit, note values are constrained to be unsigned 64-bit integers. //! @@ -34,7 +34,7 @@ //! [`Bundle`]: crate::bundle::Bundle //! [`Bundle::value_balance`]: crate::bundle::Bundle::value_balance //! [`Builder::value_balance`]: crate::builder::Builder::value_balance -//! [`Builder::add_recipient`]: crate::builder::Builder::add_recipient +//! [`Builder::add_output`]: crate::builder::Builder::add_output //! [Rust documentation]: https://doc.rust-lang.org/stable/std/primitive.i64.html use core::fmt::{self, Debug}; diff --git a/tests/builder.rs b/tests/builder.rs index 8cda6e70..f62f82a0 100644 --- a/tests/builder.rs +++ b/tests/builder.rs @@ -44,7 +44,7 @@ fn bundle_chain() { let mut builder = Builder::new(Flags::from_parts(false, true), anchor); assert_eq!( - builder.add_recipient(None, recipient, NoteValue::from_raw(5000), None), + builder.add_output(None, recipient, NoteValue::from_raw(5000), None), Ok(()) ); let unauthorized = builder.build(&mut rng).unwrap(); @@ -86,7 +86,7 @@ fn bundle_chain() { let mut builder = Builder::new(Flags::from_parts(true, true), anchor); assert_eq!(builder.add_spend(fvk, note, merkle_path), Ok(())); assert_eq!( - builder.add_recipient(None, recipient, NoteValue::from_raw(5000), None), + builder.add_output(None, recipient, NoteValue::from_raw(5000), None), Ok(()) ); let unauthorized = builder.build(&mut rng).unwrap(); From 0a257d6f68c641edc1d8c3a07349b590f3b1cfd5 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 8 Dec 2023 13:38:52 -0700 Subject: [PATCH 2/4] Add explicit control of padding to the Builder API. --- CHANGELOG.md | 4 +++ benches/circuit.rs | 4 +-- benches/note_decryption.rs | 4 +-- src/builder.rs | 69 +++++++++++++++++++++++++++++++------- tests/builder.rs | 6 ++-- 5 files changed, 67 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 507ce07c..b7568b82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,15 @@ and this project adheres to Rust's notion of [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added +- `orchard::builder::BundleType` ### Changed - `orchard::builder::Builder::add_recipient` has been renamed to `add_output` in order to clarify than more than one output of a given transaction may be sent to the same recipient. +- `orchard::builder::Builder::build` now takes an additional `BundleType` argument + that specifies how actions should be padded, instead of using hardcoded padding. ## [0.6.0] - 2023-09-08 ### Changed diff --git a/benches/circuit.rs b/benches/circuit.rs index 276a4731..3bc6d3a3 100644 --- a/benches/circuit.rs +++ b/benches/circuit.rs @@ -7,7 +7,7 @@ use criterion::{BenchmarkId, Criterion}; use pprof::criterion::{Output, PProfProfiler}; use orchard::{ - builder::Builder, + builder::{Builder, BundleType}, bundle::Flags, circuit::{ProvingKey, VerifyingKey}, keys::{FullViewingKey, Scope, SpendingKey}, @@ -35,7 +35,7 @@ fn criterion_benchmark(c: &mut Criterion) { .add_output(None, recipient, NoteValue::from_raw(10), None) .unwrap(); } - let bundle: Bundle<_, i64> = builder.build(rng).unwrap(); + let bundle: Bundle<_, i64> = builder.build(rng, &BundleType::Transactional).unwrap(); let instances: Vec<_> = bundle .actions() diff --git a/benches/note_decryption.rs b/benches/note_decryption.rs index 057c0491..6c560f6d 100644 --- a/benches/note_decryption.rs +++ b/benches/note_decryption.rs @@ -1,6 +1,6 @@ use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; use orchard::{ - builder::Builder, + builder::{Builder, BundleType}, bundle::Flags, circuit::ProvingKey, keys::{FullViewingKey, PreparedIncomingViewingKey, Scope, SpendingKey}, @@ -57,7 +57,7 @@ fn bench_note_decryption(c: &mut Criterion) { builder .add_output(None, recipient, NoteValue::from_raw(10), None) .unwrap(); - let bundle: Bundle<_, i64> = builder.build(rng).unwrap(); + let bundle: Bundle<_, i64> = builder.build(rng, &BundleType::Transactional).unwrap(); bundle .create_proof(&pk, rng) .unwrap() diff --git a/src/builder.rs b/src/builder.rs index 496b2080..c9361d0e 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -27,6 +27,42 @@ use crate::{ const MIN_ACTIONS: usize = 2; +/// An enumeration of rules Orchard bundle construction. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum BundleType { + /// A transactional bundle will be padded if necessary to contain at least 2 actions, + /// irrespective of whether any genuine actions are required. + Transactional, + /// A coinbase bundle is required to have no non-dummy spends. No padding is performed. + Coinbase, +} + +impl BundleType { + /// Returns the number of logical actions that builder will produce in constructing a bundle + /// of this type, given the specified numbers of spends and outputs. + /// + /// Returns an error if the specified number of spends and outputs is incompatible with + /// this bundle type. + pub fn num_actions( + &self, + num_spends: usize, + num_outputs: usize, + ) -> Result { + let num_real_actions = core::cmp::max(num_spends, num_outputs); + + match self { + BundleType::Transactional => Ok(core::cmp::max(num_real_actions, MIN_ACTIONS)), + BundleType::Coinbase => { + if num_spends == 0 { + Ok(num_real_actions) + } else { + Err("Spends not allowed in coinbase bundles") + } + } + } + } +} + /// An error type for the kinds of errors that can occur during bundle construction. #[derive(Debug)] pub enum BuildError { @@ -42,6 +78,8 @@ pub enum BuildError { /// A signature is valid for more than one input. This should never happen if `alpha` /// is sampled correctly, and indicates a critical failure in randomness generation. DuplicateSignature, + /// The bundle being constructed violated the construction rules for the requested bundle type. + BundleTypeNotSatisfiable, } impl Display for BuildError { @@ -53,6 +91,9 @@ impl Display for BuildError { ValueSum(_) => f.write_str("Overflow occurred during value construction"), InvalidExternalSignature => f.write_str("External signature was invalid"), DuplicateSignature => f.write_str("Signature valid for more than one input"), + BundleTypeNotSatisfiable => { + f.write_str("Bundle structure did not conform to requested bundle type.") + } } } } @@ -388,22 +429,23 @@ impl Builder { pub fn build>( mut self, mut rng: impl RngCore, + bundle_type: &BundleType, ) -> Result, V>, BuildError> { + let num_real_spends = self.spends.len(); + let num_real_outputs = self.outputs.len(); + let num_actions = bundle_type + .num_actions(num_real_spends, num_real_outputs) + .map_err(|_| BuildError::BundleTypeNotSatisfiable)?; + // Pair up the spends and outputs, extending with dummy values as necessary. let pre_actions: Vec<_> = { - let num_spends = self.spends.len(); - let num_outputs = self.outputs.len(); - let num_actions = [num_spends, num_outputs, MIN_ACTIONS] - .iter() - .max() - .cloned() - .unwrap(); - self.spends.extend( - iter::repeat_with(|| SpendInfo::dummy(&mut rng)).take(num_actions - num_spends), + iter::repeat_with(|| SpendInfo::dummy(&mut rng)) + .take(num_actions - num_real_spends), ); self.outputs.extend( - iter::repeat_with(|| OutputInfo::dummy(&mut rng)).take(num_actions - num_outputs), + iter::repeat_with(|| OutputInfo::dummy(&mut rng)) + .take(num_actions - num_real_outputs), ); // Shuffle the spends and outputs, so that learning the position of a @@ -776,7 +818,7 @@ pub mod testing { Address, Note, }; - use super::Builder; + use super::{Builder, BundleType}; /// An intermediate type used for construction of arbitrary /// bundle values. This type is required because of a limitation @@ -817,7 +859,7 @@ pub mod testing { let pk = ProvingKey::build(); builder - .build(&mut self.rng) + .build(&mut self.rng, &BundleType::Transactional) .unwrap() .create_proof(&pk, &mut self.rng) .unwrap() @@ -898,6 +940,7 @@ mod tests { use super::Builder; use crate::{ + builder::BundleType, bundle::{Authorized, Bundle, Flags}, circuit::ProvingKey, constants::MERKLE_DEPTH_ORCHARD, @@ -927,7 +970,7 @@ mod tests { assert_eq!(balance, -5000); let bundle: Bundle = builder - .build(&mut rng) + .build(&mut rng, &BundleType::Transactional) .unwrap() .create_proof(&pk, &mut rng) .unwrap() diff --git a/tests/builder.rs b/tests/builder.rs index f62f82a0..40194cd6 100644 --- a/tests/builder.rs +++ b/tests/builder.rs @@ -1,7 +1,7 @@ use bridgetree::BridgeTree; use incrementalmerkletree::Hashable; use orchard::{ - builder::Builder, + builder::{Builder, BundleType}, bundle::{Authorized, Flags}, circuit::{ProvingKey, VerifyingKey}, keys::{FullViewingKey, PreparedIncomingViewingKey, Scope, SpendAuthorizingKey, SpendingKey}, @@ -47,7 +47,7 @@ fn bundle_chain() { builder.add_output(None, recipient, NoteValue::from_raw(5000), None), Ok(()) ); - let unauthorized = builder.build(&mut rng).unwrap(); + let unauthorized = builder.build(&mut rng, &BundleType::Transactional).unwrap(); let sighash = unauthorized.commitment().into(); let proven = unauthorized.create_proof(&pk, &mut rng).unwrap(); proven.apply_signatures(rng, sighash, &[]).unwrap() @@ -89,7 +89,7 @@ fn bundle_chain() { builder.add_output(None, recipient, NoteValue::from_raw(5000), None), Ok(()) ); - let unauthorized = builder.build(&mut rng).unwrap(); + let unauthorized = builder.build(&mut rng, &BundleType::Transactional).unwrap(); let sighash = unauthorized.commitment().into(); let proven = unauthorized.create_proof(&pk, &mut rng).unwrap(); proven From 2e2c161d52763b136c1d36dae7addeaeab22687d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 15 Dec 2023 16:19:24 -0700 Subject: [PATCH 3/4] Add a public bundle construction function & use it in the builder. --- CHANGELOG.md | 16 ++ benches/circuit.rs | 8 +- benches/note_decryption.rs | 8 +- src/builder.rs | 303 +++++++++++++++++++++---------------- src/bundle.rs | 30 +++- src/tree.rs | 6 +- src/value.rs | 2 +- tests/builder.rs | 8 +- 8 files changed, 231 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7568b82..ffd6aad4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,14 +7,30 @@ and this project adheres to Rust's notion of ## [Unreleased] ### Added +- `orchard::builder::bundle` - `orchard::builder::BundleType` +- `orchard::builder::OutputInfo` +- `orchard::bundle::Flags::{ENABLED, SPENDS_DISABLED, OUTPUTS_DISABLED}` ### Changed +- `orchard::builder::Builder::new` now takes the bundle type to be used + in bundle construction, instead of taking the flags and anchor separately. - `orchard::builder::Builder::add_recipient` has been renamed to `add_output` in order to clarify than more than one output of a given transaction may be sent to the same recipient. - `orchard::builder::Builder::build` now takes an additional `BundleType` argument that specifies how actions should be padded, instead of using hardcoded padding. + It also now returns a `Result>, ...>` instead of a + `Result, ...>`. +- `orchard::builder::BuildError` has additional variants: + - `SpendsDisabled` + - `OutputsDisabled` + - `AnchorMismatch` +- `orchard::builder::SpendInfo::new` now returns a `Result` + instead of an `Option`. + +### Removed +- `orchard::bundle::Flags::from_parts` ## [0.6.0] - 2023-09-08 ### Changed diff --git a/benches/circuit.rs b/benches/circuit.rs index 3bc6d3a3..6dd8e9de 100644 --- a/benches/circuit.rs +++ b/benches/circuit.rs @@ -26,16 +26,16 @@ fn criterion_benchmark(c: &mut Criterion) { let pk = ProvingKey::build(); let create_bundle = |num_recipients| { - let mut builder = Builder::new( - Flags::from_parts(true, true), + let mut builder = Builder::new(BundleType::Transactional( + Flags::ENABLED, Anchor::from_bytes([0; 32]).unwrap(), - ); + )); for _ in 0..num_recipients { builder .add_output(None, recipient, NoteValue::from_raw(10), None) .unwrap(); } - let bundle: Bundle<_, i64> = builder.build(rng, &BundleType::Transactional).unwrap(); + let bundle: Bundle<_, i64> = builder.build(rng).unwrap().unwrap(); let instances: Vec<_> = bundle .actions() diff --git a/benches/note_decryption.rs b/benches/note_decryption.rs index 6c560f6d..a4939b12 100644 --- a/benches/note_decryption.rs +++ b/benches/note_decryption.rs @@ -45,10 +45,10 @@ fn bench_note_decryption(c: &mut Criterion) { .collect(); let bundle = { - let mut builder = Builder::new( - Flags::from_parts(true, true), + let mut builder = Builder::new(BundleType::Transactional( + Flags::ENABLED, Anchor::from_bytes([0; 32]).unwrap(), - ); + )); // The builder pads to two actions, and shuffles their order. Add two recipients // so the first action is always decryptable. builder @@ -57,7 +57,7 @@ fn bench_note_decryption(c: &mut Criterion) { builder .add_output(None, recipient, NoteValue::from_raw(10), None) .unwrap(); - let bundle: Bundle<_, i64> = builder.build(rng, &BundleType::Transactional).unwrap(); + let bundle: Bundle<_, i64> = builder.build(rng).unwrap().unwrap(); bundle .create_proof(&pk, rng) .unwrap() diff --git a/src/builder.rs b/src/builder.rs index c9361d0e..3eea0ae7 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -32,7 +32,7 @@ const MIN_ACTIONS: usize = 2; pub enum BundleType { /// A transactional bundle will be padded if necessary to contain at least 2 actions, /// irrespective of whether any genuine actions are required. - Transactional, + Transactional(Flags, Anchor), /// A coinbase bundle is required to have no non-dummy spends. No padding is performed. Coinbase, } @@ -48,17 +48,27 @@ impl BundleType { num_spends: usize, num_outputs: usize, ) -> Result { - let num_real_actions = core::cmp::max(num_spends, num_outputs); + let num_requested_actions = core::cmp::max(num_spends, num_outputs); match self { - BundleType::Transactional => Ok(core::cmp::max(num_real_actions, MIN_ACTIONS)), - BundleType::Coinbase => { - if num_spends == 0 { - Ok(num_real_actions) + BundleType::Transactional(flags, _) => { + if !flags.spends_enabled() && num_spends > 0 { + Err("Spends are disabled, so num_spends must be zero") + } else if !flags.outputs_enabled() && num_outputs > 0 { + Err("Outputs are disabled, so num_outputs must be zero") } else { - Err("Spends not allowed in coinbase bundles") + Ok(core::cmp::max(num_requested_actions, MIN_ACTIONS)) } } + BundleType::Coinbase => Ok(num_outputs), + } + } + + /// Returns the set of flags and the anchor that will be used for bundle construction. + pub fn bundle_config(&self) -> (Flags, Anchor) { + match self { + BundleType::Transactional(flags, anchor) => (*flags, *anchor), + BundleType::Coinbase => (Flags::SPENDS_DISABLED, Anchor::empty_tree()), } } } @@ -66,6 +76,12 @@ impl BundleType { /// An error type for the kinds of errors that can occur during bundle construction. #[derive(Debug)] pub enum BuildError { + /// Spends are disabled for the provided bundle type. + SpendsDisabled, + /// Spends are disabled for the provided bundle type. + OutputsDisabled, + /// The anchor provided to this builder doesn't match the merkle path used to add a spend. + AnchorMismatch, /// A bundle could not be built because required signatures were missing. MissingSignatures, /// An error occurred in the process of producing a proof for a bundle. @@ -94,12 +110,29 @@ impl Display for BuildError { BundleTypeNotSatisfiable => { f.write_str("Bundle structure did not conform to requested bundle type.") } + SpendsDisabled => f.write_str("Spends are not enabled for the requested bundle type."), + OutputsDisabled => f.write_str("Spends are not enabled for the requested bundle type."), + AnchorMismatch => { + f.write_str("All spends must share the anchor requested for the transaction.") + } } } } impl std::error::Error for BuildError {} +impl From for BuildError { + fn from(e: halo2_proofs::plonk::Error) -> Self { + BuildError::Proof(e) + } +} + +impl From for BuildError { + fn from(e: value::OverflowError) -> Self { + BuildError::ValueSum(e) + } +} + /// An error type for adding a spend to the builder. #[derive(Debug, PartialEq, Eq)] pub enum SpendError { @@ -136,18 +169,6 @@ impl Display for OutputError { impl std::error::Error for OutputError {} -impl From for BuildError { - fn from(e: halo2_proofs::plonk::Error) -> Self { - BuildError::Proof(e) - } -} - -impl From for BuildError { - fn from(e: value::OverflowError) -> Self { - BuildError::ValueSum(e) - } -} - /// Information about a specific note to be spent in an [`Action`]. #[derive(Debug)] pub struct SpendInfo { @@ -200,27 +221,41 @@ impl SpendInfo { /// Information about a specific output to receive funds in an [`Action`]. #[derive(Debug)] -struct OutputInfo { +pub struct OutputInfo { ovk: Option, recipient: Address, value: NoteValue, - memo: Option<[u8; 512]>, + memo: [u8; 512], } impl OutputInfo { + /// Constructs a new OutputInfo from its constituent parts. + pub fn new( + ovk: Option, + recipient: Address, + value: NoteValue, + memo: Option<[u8; 512]>, + ) -> Self { + Self { + ovk, + recipient, + value, + memo: memo.unwrap_or_else(|| { + let mut memo = [0; 512]; + memo[0] = 0xf6; + memo + }), + } + } + /// Defined in [Zcash Protocol Spec § 4.8.3: Dummy Notes (Orchard)][orcharddummynotes]. /// /// [orcharddummynotes]: https://zips.z.cash/protocol/nu5.pdf#orcharddummynotes - fn dummy(rng: &mut impl RngCore) -> Self { + pub fn dummy(rng: &mut impl RngCore) -> Self { let fvk: FullViewingKey = (&SpendingKey::random(rng)).into(); let recipient = fvk.address_at(0u32, Scope::External); - OutputInfo { - ovk: None, - recipient, - value: NoteValue::zero(), - memo: None, - } + Self::new(None, recipient, NoteValue::zero(), None) } } @@ -264,15 +299,7 @@ impl ActionInfo { let cm_new = note.commitment(); let cmx = cm_new.into(); - let encryptor = OrchardNoteEncryption::new( - self.output.ovk, - note, - self.output.memo.unwrap_or_else(|| { - let mut memo = [0; 512]; - memo[0] = 0xf6; - memo - }), - ); + let encryptor = OrchardNoteEncryption::new(self.output.ovk, note, self.output.memo); let encrypted_note = TransmittedNoteCiphertext { epk_bytes: encryptor.epk().to_bytes().0, @@ -303,18 +330,16 @@ impl ActionInfo { pub struct Builder { spends: Vec, outputs: Vec, - flags: Flags, - anchor: Anchor, + bundle_type: BundleType, } impl Builder { /// Constructs a new empty builder for an Orchard bundle. - pub fn new(flags: Flags, anchor: Anchor) -> Self { + pub fn new(bundle_type: BundleType) -> Self { Builder { spends: vec![], outputs: vec![], - flags, - anchor, + bundle_type, } } @@ -336,29 +361,20 @@ impl Builder { note: Note, merkle_path: MerklePath, ) -> Result<(), SpendError> { - if !self.flags.spends_enabled() { + let (flags, anchor) = self.bundle_type.bundle_config(); + if !flags.spends_enabled() { return Err(SpendError::SpendsDisabled); } // Consistency check: all anchors must be equal. let cm = note.commitment(); let path_root = merkle_path.root(cm.into()); - if path_root != self.anchor { + if path_root != anchor { return Err(SpendError::AnchorMismatch); } - // Check if note is internal or external. - let scope = fvk - .scope_for_address(¬e.recipient()) - .ok_or(SpendError::FvkMismatch)?; - - self.spends.push(SpendInfo { - dummy_sk: None, - fvk, - scope, - note, - merkle_path, - }); + self.spends + .push(SpendInfo::new(fvk, note, merkle_path).ok_or(SpendError::FvkMismatch)?); Ok(()) } @@ -371,16 +387,13 @@ impl Builder { value: NoteValue, memo: Option<[u8; 512]>, ) -> Result<(), OutputError> { - if !self.flags.outputs_enabled() { + let (flags, _) = self.bundle_type.bundle_config(); + if !flags.outputs_enabled() { return Err(OutputError); } - self.outputs.push(OutputInfo { - ovk, - recipient, - value, - memo, - }); + self.outputs + .push(OutputInfo::new(ovk, recipient, value, memo)); Ok(()) } @@ -427,75 +440,105 @@ impl Builder { /// The returned bundle will have no proof or signatures; these can be applied with /// [`Bundle::create_proof`] and [`Bundle::apply_signatures`] respectively. pub fn build>( - mut self, - mut rng: impl RngCore, - bundle_type: &BundleType, - ) -> Result, V>, BuildError> { - let num_real_spends = self.spends.len(); - let num_real_outputs = self.outputs.len(); - let num_actions = bundle_type - .num_actions(num_real_spends, num_real_outputs) - .map_err(|_| BuildError::BundleTypeNotSatisfiable)?; + self, + rng: impl RngCore, + ) -> Result, V>>, BuildError> { + bundle(rng, self.spends, self.outputs, self.bundle_type) + } +} - // Pair up the spends and outputs, extending with dummy values as necessary. - let pre_actions: Vec<_> = { - self.spends.extend( - iter::repeat_with(|| SpendInfo::dummy(&mut rng)) - .take(num_actions - num_real_spends), - ); - self.outputs.extend( - iter::repeat_with(|| OutputInfo::dummy(&mut rng)) - .take(num_actions - num_real_outputs), - ); +/// Builds a bundle containing the given spent notes and outputs. +/// +/// The returned bundle will have no proof or signatures; these can be applied with +/// [`Bundle::create_proof`] and [`Bundle::apply_signatures`] respectively. +pub fn bundle>( + mut rng: impl RngCore, + mut spends: Vec, + mut outputs: Vec, + bundle_type: BundleType, +) -> Result, V>>, BuildError> { + let (flags, anchor) = bundle_type.bundle_config(); - // Shuffle the spends and outputs, so that learning the position of a - // specific spent note or output note doesn't reveal anything on its own about - // the meaning of that note in the transaction context. - self.spends.shuffle(&mut rng); - self.outputs.shuffle(&mut rng); + let num_requested_spends = spends.len(); + if !flags.spends_enabled() && num_requested_spends > 0 { + return Err(BuildError::SpendsDisabled); + } - self.spends - .into_iter() - .zip(self.outputs.into_iter()) - .map(|(spend, output)| ActionInfo::new(spend, output, &mut rng)) - .collect() - }; + for spend in &spends { + if spend.note.value() != NoteValue::zero() { + let cm = spend.note.commitment(); + let path_root = spend.merkle_path.root(cm.into()); + if path_root != anchor { + return Err(BuildError::AnchorMismatch); + } + } + } - // Move some things out of self that we will need. - let flags = self.flags; - let anchor = self.anchor; + let num_requested_outputs = outputs.len(); + if !flags.outputs_enabled() && num_requested_outputs > 0 { + return Err(BuildError::OutputsDisabled); + } - // Determine the value balance for this bundle, ensuring it is valid. - let value_balance = pre_actions - .iter() - .fold(Some(ValueSum::zero()), |acc, action| { - acc? + action.value_sum() - }) - .ok_or(OverflowError)?; + let num_actions = bundle_type + .num_actions(num_requested_spends, num_requested_outputs) + .map_err(|_| BuildError::BundleTypeNotSatisfiable)?; - let result_value_balance: V = i64::try_from(value_balance) - .map_err(BuildError::ValueSum) - .and_then(|i| V::try_from(i).map_err(|_| BuildError::ValueSum(value::OverflowError)))?; + // Pair up the spends and outputs, extending with dummy values as necessary. + let pre_actions: Vec<_> = { + spends.extend( + iter::repeat_with(|| SpendInfo::dummy(&mut rng)) + .take(num_actions - num_requested_spends), + ); + outputs.extend( + iter::repeat_with(|| OutputInfo::dummy(&mut rng)) + .take(num_actions - num_requested_outputs), + ); - // Compute the transaction binding signing key. - let bsk = pre_actions - .iter() - .map(|a| &a.rcv) - .sum::() - .into_bsk(); + // Shuffle the spends and outputs, so that learning the position of a + // specific spent note or output note doesn't reveal anything on its own about + // the meaning of that note in the transaction context. + spends.shuffle(&mut rng); + outputs.shuffle(&mut rng); - // Create the actions. - let (actions, circuits): (Vec<_>, Vec<_>) = - pre_actions.into_iter().map(|a| a.build(&mut rng)).unzip(); + spends + .into_iter() + .zip(outputs.into_iter()) + .map(|(spend, output)| ActionInfo::new(spend, output, &mut rng)) + .collect() + }; - // Verify that bsk and bvk are consistent. - let bvk = (actions.iter().map(|a| a.cv_net()).sum::() - - ValueCommitment::derive(value_balance, ValueCommitTrapdoor::zero())) - .into_bvk(); - assert_eq!(redpallas::VerificationKey::from(&bsk), bvk); + // Determine the value balance for this bundle, ensuring it is valid. + let value_balance = pre_actions + .iter() + .fold(Some(ValueSum::zero()), |acc, action| { + acc? + action.value_sum() + }) + .ok_or(OverflowError)?; - Ok(Bundle::from_parts( - NonEmpty::from_vec(actions).unwrap(), + let result_value_balance: V = i64::try_from(value_balance) + .map_err(BuildError::ValueSum) + .and_then(|i| V::try_from(i).map_err(|_| BuildError::ValueSum(value::OverflowError)))?; + + // Compute the transaction binding signing key. + let bsk = pre_actions + .iter() + .map(|a| &a.rcv) + .sum::() + .into_bsk(); + + // Create the actions. + let (actions, circuits): (Vec<_>, Vec<_>) = + pre_actions.into_iter().map(|a| a.build(&mut rng)).unzip(); + + // Verify that bsk and bvk are consistent. + let bvk = (actions.iter().map(|a| a.cv_net()).sum::() + - ValueCommitment::derive(value_balance, ValueCommitTrapdoor::zero())) + .into_bvk(); + assert_eq!(redpallas::VerificationKey::from(&bsk), bvk); + + Ok(NonEmpty::from_vec(actions).map(|actions| { + Bundle::from_parts( + actions, flags, result_value_balance, anchor, @@ -503,8 +546,8 @@ impl Builder { proof: Unproven { circuits }, sigs: Unauthorized { bsk }, }, - )) - } + ) + })) } /// Marker trait representing bundle signatures in the process of being created. @@ -842,7 +885,7 @@ pub mod testing { fn into_bundle>(mut self) -> Bundle { let fvk = FullViewingKey::from(&self.sk); let flags = Flags::from_parts(true, true); - let mut builder = Builder::new(flags, self.anchor); + let mut builder = Builder::new(BundleType::Transactional(flags, self.anchor)); for (note, path) in self.notes.into_iter() { builder.add_spend(fvk.clone(), note, path).unwrap(); @@ -859,7 +902,8 @@ pub mod testing { let pk = ProvingKey::build(); builder - .build(&mut self.rng, &BundleType::Transactional) + .build(&mut self.rng) + .unwrap() .unwrap() .create_proof(&pk, &mut self.rng) .unwrap() @@ -958,10 +1002,10 @@ mod tests { let fvk = FullViewingKey::from(&sk); let recipient = fvk.address_at(0u32, Scope::External); - let mut builder = Builder::new( + let mut builder = Builder::new(BundleType::Transactional( Flags::from_parts(true, true), EMPTY_ROOTS[MERKLE_DEPTH_ORCHARD].into(), - ); + )); builder .add_output(None, recipient, NoteValue::from_raw(5000), None) @@ -970,7 +1014,8 @@ mod tests { assert_eq!(balance, -5000); let bundle: Bundle = builder - .build(&mut rng, &BundleType::Transactional) + .build(&mut rng) + .unwrap() .unwrap() .create_proof(&pk, &mut rng) .unwrap() diff --git a/src/bundle.rs b/src/bundle.rs index f84c3f6f..f5dbb55d 100644 --- a/src/bundle.rs +++ b/src/bundle.rs @@ -42,7 +42,7 @@ impl Action { } /// Orchard-specific flags. -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct Flags { /// Flag denoting whether Orchard spends are enabled in the transaction. /// @@ -64,13 +64,31 @@ const FLAGS_EXPECTED_UNSET: u8 = !(FLAG_SPENDS_ENABLED | FLAG_OUTPUTS_ENABLED); impl Flags { /// Construct a set of flags from its constituent parts - pub fn from_parts(spends_enabled: bool, outputs_enabled: bool) -> Self { + pub(crate) fn from_parts(spends_enabled: bool, outputs_enabled: bool) -> Self { Flags { spends_enabled, outputs_enabled, } } + /// The flag set with both spends and outputs enabled. + pub const ENABLED: Flags = Flags { + spends_enabled: true, + outputs_enabled: true, + }; + + /// The flag set with spends disabled. + pub const SPENDS_DISABLED: Flags = Flags { + spends_enabled: false, + outputs_enabled: true, + }; + + /// The flag set with outputs disabled. + pub const OUTPUTS_DISABLED: Flags = Flags { + spends_enabled: true, + outputs_enabled: false, + }; + /// Flag denoting whether Orchard spends are enabled in the transaction. /// /// If `false`, spent notes within [`Action`]s in the transaction's [`Bundle`] are @@ -113,10 +131,10 @@ impl Flags { pub fn from_byte(value: u8) -> Option { // https://p.z.cash/TCR:bad-txns-v5-reserved-bits-nonzero if value & FLAGS_EXPECTED_UNSET == 0 { - Some(Self::from_parts( - value & FLAG_SPENDS_ENABLED != 0, - value & FLAG_OUTPUTS_ENABLED != 0, - )) + Some(Self { + spends_enabled: value & FLAG_SPENDS_ENABLED != 0, + outputs_enabled: value & FLAG_OUTPUTS_ENABLED != 0, + }) } else { None } diff --git a/src/tree.rs b/src/tree.rs index ba68265a..c93ad695 100644 --- a/src/tree.rs +++ b/src/tree.rs @@ -58,12 +58,14 @@ impl From for Anchor { } impl Anchor { + pub(crate) fn empty_tree() -> Anchor { + Anchor(MerkleHashOrchard::empty_root(Level::from(MERKLE_DEPTH_ORCHARD as u8)).0) + } + pub(crate) fn inner(&self) -> pallas::Base { self.0 } -} -impl Anchor { /// Parses an Orchard anchor from a byte encoding. pub fn from_bytes(bytes: [u8; 32]) -> CtOption { pallas::Base::from_repr(bytes).map(Anchor) diff --git a/src/value.rs b/src/value.rs index 302c0b3f..9ac440ee 100644 --- a/src/value.rs +++ b/src/value.rs @@ -83,7 +83,7 @@ impl fmt::Display for OverflowError { impl std::error::Error for OverflowError {} /// The non-negative value of an individual Orchard note. -#[derive(Clone, Copy, Debug, Default)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub struct NoteValue(u64); impl NoteValue { diff --git a/tests/builder.rs b/tests/builder.rs index 40194cd6..db5c2ae5 100644 --- a/tests/builder.rs +++ b/tests/builder.rs @@ -42,12 +42,12 @@ fn bundle_chain() { // Use the empty tree. let anchor = MerkleHashOrchard::empty_root(32.into()).into(); - let mut builder = Builder::new(Flags::from_parts(false, true), anchor); + let mut builder = Builder::new(BundleType::Transactional(Flags::SPENDS_DISABLED, anchor)); assert_eq!( builder.add_output(None, recipient, NoteValue::from_raw(5000), None), Ok(()) ); - let unauthorized = builder.build(&mut rng, &BundleType::Transactional).unwrap(); + let unauthorized = builder.build(&mut rng).unwrap().unwrap(); let sighash = unauthorized.commitment().into(); let proven = unauthorized.create_proof(&pk, &mut rng).unwrap(); proven.apply_signatures(rng, sighash, &[]).unwrap() @@ -83,13 +83,13 @@ fn bundle_chain() { let anchor = root.into(); assert_eq!(anchor, merkle_path.root(cmx)); - let mut builder = Builder::new(Flags::from_parts(true, true), anchor); + let mut builder = Builder::new(BundleType::Transactional(Flags::ENABLED, anchor)); assert_eq!(builder.add_spend(fvk, note, merkle_path), Ok(())); assert_eq!( builder.add_output(None, recipient, NoteValue::from_raw(5000), None), Ok(()) ); - let unauthorized = builder.build(&mut rng, &BundleType::Transactional).unwrap(); + let unauthorized = builder.build(&mut rng).unwrap().unwrap(); let sighash = unauthorized.commitment().into(); let proven = unauthorized.create_proof(&pk, &mut rng).unwrap(); proven From 938b203de5f132053fecaf73a7c4f2c3978cc0bf Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 19 Dec 2023 12:18:31 -0700 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: str4d --- src/builder.rs | 48 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 3eea0ae7..a7b1f8fe 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -27,7 +27,7 @@ use crate::{ const MIN_ACTIONS: usize = 2; -/// An enumeration of rules Orchard bundle construction. +/// An enumeration of rules for Orchard bundle construction. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum BundleType { /// A transactional bundle will be padded if necessary to contain at least 2 actions, @@ -60,7 +60,13 @@ impl BundleType { Ok(core::cmp::max(num_requested_actions, MIN_ACTIONS)) } } - BundleType::Coinbase => Ok(num_outputs), + BundleType::Coinbase => { + if num_spends > 0 { + Err("Coinbase bundles have spends disabled, so num_spends must be zero") + } else { + Ok(num_outputs) + } + } } } @@ -80,7 +86,7 @@ pub enum BuildError { SpendsDisabled, /// Spends are disabled for the provided bundle type. OutputsDisabled, - /// The anchor provided to this builder doesn't match the merkle path used to add a spend. + /// The anchor provided to this builder doesn't match the Merkle path used to add a spend. AnchorMismatch, /// A bundle could not be built because required signatures were missing. MissingSignatures, @@ -217,6 +223,16 @@ impl SpendInfo { merkle_path, } } + + fn has_matching_anchor(&self, anchor: Anchor) -> bool { + if self.note.value() == NoteValue::zero() { + true + } else { + let cm = self.note.commitment(); + let path_root = self.merkle_path.root(cm.into()); + path_root == anchor + } + } } /// Information about a specific output to receive funds in an [`Action`]. @@ -324,6 +340,11 @@ impl ActionInfo { } } +/// Type alias for an in-progress bundle that has no proofs or signatures. +/// +/// This is returned by [`Builder::build`]. +pub type UnauthorizedBundle = Bundle, V>; + /// A builder that constructs a [`Bundle`] from a set of notes to be spent, and outputs /// to receive funds. #[derive(Debug)] @@ -366,15 +387,14 @@ impl Builder { return Err(SpendError::SpendsDisabled); } + let spend = SpendInfo::new(fvk, note, merkle_path).ok_or(SpendError::FvkMismatch)?; + // Consistency check: all anchors must be equal. - let cm = note.commitment(); - let path_root = merkle_path.root(cm.into()); - if path_root != anchor { + if !spend.has_matching_anchor(anchor) { return Err(SpendError::AnchorMismatch); } - self.spends - .push(SpendInfo::new(fvk, note, merkle_path).ok_or(SpendError::FvkMismatch)?); + self.spends.push(spend); Ok(()) } @@ -442,7 +462,7 @@ impl Builder { pub fn build>( self, rng: impl RngCore, - ) -> Result, V>>, BuildError> { + ) -> Result>, BuildError> { bundle(rng, self.spends, self.outputs, self.bundle_type) } } @@ -456,7 +476,7 @@ pub fn bundle>( mut spends: Vec, mut outputs: Vec, bundle_type: BundleType, -) -> Result, V>>, BuildError> { +) -> Result>, BuildError> { let (flags, anchor) = bundle_type.bundle_config(); let num_requested_spends = spends.len(); @@ -465,12 +485,8 @@ pub fn bundle>( } for spend in &spends { - if spend.note.value() != NoteValue::zero() { - let cm = spend.note.commitment(); - let path_root = spend.merkle_path.root(cm.into()); - if path_root != anchor { - return Err(BuildError::AnchorMismatch); - } + if !spend.has_matching_anchor(anchor) { + return Err(BuildError::AnchorMismatch); } }