From b202452c638a41a68b10f12e166d667e158a5a33 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 11 Dec 2023 12:36:16 -0700 Subject: [PATCH 1/2] Add `builder::BundleType` for explicit control over output padding. --- CHANGELOG.md | 9 +++++++ src/builder.rs | 65 ++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 62 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e4d611..5741c0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ The entries below are relative to the `zcash_primitives::sapling` module as of - `SpendDescriptionInfo::value` - `SaplingOutputInfo` - `ProverProgress` + - `BundleType` - `sapling_crypto::bundle` module: - The following types moved from `zcash_primitives::transaction::components::sapling`: @@ -102,10 +103,18 @@ The entries below are relative to the `zcash_primitives::sapling` module as of generic parameters and returns `(UnauthorizedBundle, SaplingMetadata)`. The caller can then use `Bundle::>::create_proofs` to create spend and output proofs for the bundle. + - `SaplingBuilder::build` now takes a `BundleType` argument that instructs + it how to pad the bundle with dummy outputs. + - `SaplingBuilder::bundle_output_count` has been changed to use a padding + rule to compute its result. It also now returns a `Result` + instead of a bare `usize` in order to be able to indicate that current + state of the builder will produce a bundle that is incompatible with + the specified bundle type. - `Error` has new error variants: - `Error::DuplicateSignature` - `Error::InvalidExternalSignature` - `Error::MissingSignatures` + - `Error::BundleTypeNotSatisfiable` - `sapling_crypto::bundle`: - `Bundle` now has a second generic parameter `V`. - `Bundle::value_balance` now returns `&V` instead of diff --git a/src/builder.rs b/src/builder.rs index be084af..b6d44a6 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -29,6 +29,41 @@ use crate::{ /// with dummy outputs if necessary. See . const MIN_SHIELDED_OUTPUTS: usize = 2; +/// An enumeration of rules for construction of dummy actions that may be applied to Sapling 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_outputs( + &self, + num_spends: usize, + num_outputs: usize, + ) -> Result { + match self { + BundleType::Transactional => Ok(core::cmp::max(num_outputs, MIN_SHIELDED_OUTPUTS)), + BundleType::Coinbase => { + if num_spends == 0 { + Ok(num_outputs) + } else { + Err("Spends not allowed in coinbase bundles") + } + } + } + } +} + #[derive(Debug, PartialEq, Eq)] pub enum Error { AnchorMismatch, @@ -43,6 +78,8 @@ pub enum Error { /// A bundle could not be built because required signatures were missing. MissingSignatures, SpendProof, + /// The bundle being constructed violated the construction rules for the requested bundle type. + BundleTypeNotSatisfiable, } impl fmt::Display for Error { @@ -58,6 +95,9 @@ impl fmt::Display for Error { Error::InvalidExternalSignature => write!(f, "External signature was invalid"), Error::MissingSignatures => write!(f, "Required signatures were missing during build"), Error::SpendProof => write!(f, "Failed to create Sapling spend proof"), + Error::BundleTypeNotSatisfiable => { + f.write_str("Bundle structure did not conform to requested bundle type.") + } } } } @@ -318,12 +358,10 @@ impl SaplingBuilder { /// /// This may be larger than the number of outputs that have been added to the builder, /// depending on whether padding is going to be applied. - pub fn bundle_output_count(&self) -> usize { - // This matches the padding behaviour in `Self::build`. - match self.spends.len() { - 0 => self.outputs.len(), - _ => std::cmp::max(MIN_SHIELDED_OUTPUTS, self.outputs.len()), - } + pub fn bundle_output_count(&self, padding_rule: &BundleType) -> Result { + padding_rule + .num_outputs(self.spends.len(), self.outputs.len()) + .map_err(|_| Error::BundleTypeNotSatisfiable) } /// Returns the net value represented by the spends and outputs added to this builder, @@ -405,8 +443,10 @@ impl SaplingBuilder { pub fn build>( self, mut rng: R, + padding_rule: &BundleType, ) -> Result, SaplingMetadata)>, Error> { let value_balance = self.try_value_balance()?; + let bundle_output_count = self.bundle_output_count(padding_rule)?; // Record initial positions of spends and outputs let mut indexed_spends: Vec<_> = self.spends.into_iter().enumerate().collect(); @@ -424,10 +464,8 @@ impl SaplingBuilder { tx_metadata.output_indices.resize(indexed_outputs.len(), 0); // Pad Sapling outputs - if !indexed_spends.is_empty() { - while indexed_outputs.len() < MIN_SHIELDED_OUTPUTS { - indexed_outputs.push(None); - } + while indexed_outputs.len() < bundle_output_count { + indexed_outputs.push(None); } // Randomize order of inputs and outputs @@ -876,7 +914,7 @@ pub mod testing { frontier::testing::arb_commitment_tree, witness::IncrementalWitness, }; - use super::SaplingBuilder; + use super::{BundleType, SaplingBuilder}; #[allow(dead_code)] fn arb_bundle>( @@ -913,7 +951,10 @@ pub mod testing { } let (bundle, _) = builder - .build::(&mut rng) + .build::( + &mut rng, + &BundleType::Transactional, + ) .unwrap() .unwrap(); From 2abe3ea8e295a2bbc383e758455e56a38ebd24ff Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 15 Dec 2023 10:01:35 -0700 Subject: [PATCH 2/2] Apply comments from code review. Co-authored-by: str4d --- CHANGELOG.md | 6 +----- src/builder.rs | 28 +++++++++------------------- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5741c0c..68f2784 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,11 +105,6 @@ The entries below are relative to the `zcash_primitives::sapling` module as of create spend and output proofs for the bundle. - `SaplingBuilder::build` now takes a `BundleType` argument that instructs it how to pad the bundle with dummy outputs. - - `SaplingBuilder::bundle_output_count` has been changed to use a padding - rule to compute its result. It also now returns a `Result` - instead of a bare `usize` in order to be able to indicate that current - state of the builder will produce a bundle that is incompatible with - the specified bundle type. - `Error` has new error variants: - `Error::DuplicateSignature` - `Error::InvalidExternalSignature` @@ -178,6 +173,7 @@ The entries below are relative to the `zcash_primitives::sapling` module as of - `sapling_crypto::redjubjub` module (use the `redjubjub` crate instead). - `sapling_crypto::spend_sig` (use `redjubjub::SigningKey::{randomize, sign}` instead). +- `sapling_crypto::builder::SaplingBuilder::bundle_output_count` ## [0.0.1] - 2017-12-06 Initial release to reserve crate name. diff --git a/src/builder.rs b/src/builder.rs index b6d44a6..d3fae7f 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -29,19 +29,18 @@ use crate::{ /// with dummy outputs if necessary. See . const MIN_SHIELDED_OUTPUTS: usize = 2; -/// An enumeration of rules for construction of dummy actions that may be applied to Sapling bundle -/// construction. +/// An enumeration of rules for Sapling 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. + /// A transactional bundle will be padded if necessary to contain at least 2 outputs, + /// irrespective of whether any genuine outputs are required. Transactional, - /// A coinbase bundle is required to have no non-dummy spends. No padding is performed. + /// A coinbase bundle is required to have no spends. No output padding is performed. Coinbase, } impl BundleType { - /// Returns the number of logical actions that builder will produce in constructing a bundle + /// Returns the number of logical outputs that a 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 @@ -353,17 +352,6 @@ impl SaplingBuilder { &self.outputs } - /// Returns the number of outputs that will be present in the Sapling bundle built by - /// this builder. - /// - /// This may be larger than the number of outputs that have been added to the builder, - /// depending on whether padding is going to be applied. - pub fn bundle_output_count(&self, padding_rule: &BundleType) -> Result { - padding_rule - .num_outputs(self.spends.len(), self.outputs.len()) - .map_err(|_| Error::BundleTypeNotSatisfiable) - } - /// Returns the net value represented by the spends and outputs added to this builder, /// or an error if the values added to this builder overflow the range of a Zcash /// monetary amount. @@ -443,10 +431,12 @@ impl SaplingBuilder { pub fn build>( self, mut rng: R, - padding_rule: &BundleType, + bundle_type: &BundleType, ) -> Result, SaplingMetadata)>, Error> { let value_balance = self.try_value_balance()?; - let bundle_output_count = self.bundle_output_count(padding_rule)?; + let bundle_output_count = bundle_type + .num_outputs(self.spends.len(), self.outputs.len()) + .map_err(|_| Error::BundleTypeNotSatisfiable)?; // Record initial positions of spends and outputs let mut indexed_spends: Vec<_> = self.spends.into_iter().enumerate().collect();