From fc0f55d82b0da82ae5b9ce22add8735cffed1e91 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 23 Apr 2021 12:47:22 -0600 Subject: [PATCH] Make ValueSum correctly respect the proper specified range. --- src/builder.rs | 30 +++++++++++++++++------------- src/bundle.rs | 22 +++++++++++----------- src/value.rs | 45 +++++++++++++++++++++++++-------------------- 3 files changed, 53 insertions(+), 44 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 9e5372b2..74dc67d2 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -1,6 +1,8 @@ //! Logic for building Orchard components of transactions. +use std::convert::TryFrom; use std::iter; +use std::marker::PhantomData; use ff::Field; use nonempty::NonEmpty; @@ -110,7 +112,7 @@ impl ActionInfo { } /// Returns the value sum for this action. - fn value_sum(&self) -> Result { + fn value_sum(&self) -> Option { self.spend.note.value() - self.output.value } @@ -119,7 +121,7 @@ impl ActionInfo { /// Defined in [Zcash Protocol Spec ยง 4.7.3: Sending Notes (Orchard)][orchardsend]. /// /// [orchardsend]: https://zips.z.cash/protocol/nu5.pdf#orchardsend - fn build(self, mut rng: impl RngCore) -> (Action, Circuit) { + fn build(self, mut rng: impl RngCore) -> (Action, Circuit) { let v_net = self.value_sum().expect("already checked this"); let cv_net = ValueCommitment::derive(v_net, self.rcv); @@ -237,11 +239,11 @@ impl Builder { /// /// This API assumes that none of the notes being spent are controlled by (threshold) /// multisignatures, and immediately constructs the bundle proof. - fn build( + fn build>( mut self, mut rng: impl RngCore, pk: &ProvingKey, - ) -> Result, Error> { + ) -> Result, Error> { // Pair up the spends and recipients, extending with dummy values as necessary. // // TODO: Do we want to shuffle the order like we do for Sapling? And if we do, do @@ -276,11 +278,11 @@ impl Builder { let anchor = self.anchor; // Determine the value balance for this bundle, ensuring it is valid. - let value_balance: ValueSum = pre_actions + let value_balance = pre_actions .iter() - .fold(Ok(ValueSum::zero()), |acc, action| { + .fold(Some(ValueSum::zero()), |acc, action| { acc? + action.value_sum()? - })?; + }).ok_or(Error::ValueSum(value::OverflowError))?; // Compute the transaction binding signing key. let bsk = pre_actions @@ -291,7 +293,7 @@ impl Builder { // Create the actions. let (actions, circuits): (Vec<_>, Vec<_>) = - pre_actions.into_iter().map(|a| a.build(&mut rng)).unzip(); + pre_actions.into_iter().map(|a| a.build(&mut rng, PhantomData::)).unzip(); // Verify that bsk and bvk are consistent. let bvk = (actions.iter().map(|a| a.cv_net()).sum::() @@ -306,6 +308,8 @@ impl Builder { .collect(); let proof = Proof::create(pk, &circuits, &instances)?; + let value_balance: V = i64::try_from(value_balance).map_err(Error::ValueSum).and_then(|i| V::try_from(i).map_err(|_| Error::ValueSum(value::OverflowError)))?; + Ok(Bundle::from_parts( NonEmpty::from_vec(actions).unwrap(), flags, @@ -353,7 +357,7 @@ impl Authorization for PartiallyAuthorized { type SpendAuth = (Option>, SpendValidatingKey); } -impl Bundle { +impl Bundle { /// Loads the sighash into this bundle, preparing it for signing. /// /// This API ensures that all signatures are created over the same sighash. @@ -361,7 +365,7 @@ impl Bundle { self, mut rng: R, sighash: [u8; 32], - ) -> Bundle { + ) -> Bundle { self.authorize( &mut rng, |rng, _, SigningMetadata { dummy_ask, ak }| { @@ -385,7 +389,7 @@ impl Bundle { mut rng: R, sighash: [u8; 32], signing_keys: &[SpendAuthorizingKey], - ) -> Result, Error> { + ) -> Result, Error> { signing_keys .iter() .fold(self.prepare(&mut rng, sighash), |partial, ask| { @@ -395,7 +399,7 @@ impl Bundle { } } -impl Bundle { +impl Bundle { /// Signs this bundle with the given [`SpendAuthorizingKey`]. /// /// This will apply signatures for all notes controlled by this spending key. @@ -426,7 +430,7 @@ impl Bundle { /// Finalizes this bundle, enabling it to be included in a transaction. /// /// Returns an error if any signatures are missing. - pub fn finalize(self) -> Result, Error> { + pub fn finalize(self) -> Result, Error> { self.try_authorize( &mut (), |_, _, (sig, _)| match sig { diff --git a/src/bundle.rs b/src/bundle.rs index 9632e9d1..9489d578 100644 --- a/src/bundle.rs +++ b/src/bundle.rs @@ -7,7 +7,7 @@ use crate::{ note::{ExtractedNoteCommitment, Nullifier, TransmittedNoteCiphertext}, primitives::redpallas::{self, Binding, SpendAuth}, tree::Anchor, - value::{ValueCommitment, ValueSum}, + value::{ValueCommitment}, }; /// An action applied to the global ledger. @@ -213,7 +213,7 @@ impl Authorization for Authorized { /// A bundle of actions to be applied to the ledger. #[derive(Debug)] -pub struct Bundle { +pub struct Bundle { /// The list of actions that make up this bundle. actions: NonEmpty>, /// Orchard-specific transaction-level flags for this bundle. @@ -221,19 +221,19 @@ pub struct Bundle { /// The net value moved out of the Orchard shielded pool. /// /// This is the sum of Orchard spends minus the sum of Orchard outputs. - value_balance: ValueSum, + value_balance: V, /// The root of the Orchard commitment tree that this bundle commits to. anchor: Anchor, /// The authorization for this bundle. authorization: T, } -impl Bundle { +impl Bundle { /// Constructs a `Bundle` from its constituent parts. pub fn from_parts( actions: NonEmpty>, flags: Flags, - value_balance: ValueSum, + value_balance: V, anchor: Anchor, authorization: T, ) -> Self { @@ -259,7 +259,7 @@ impl Bundle { /// Returns the net value moved into or out of the Orchard shielded pool. /// /// This is the sum of Orchard spends minus the sum Orchard outputs. - pub fn value_balance(&self) -> &ValueSum { + pub fn value_balance(&self) -> &V { &self.value_balance } @@ -287,7 +287,7 @@ impl Bundle { context: &mut R, mut spend_auth: impl FnMut(&mut R, &T, T::SpendAuth) -> U::SpendAuth, step: impl FnOnce(&mut R, T) -> U, - ) -> Bundle { + ) -> Bundle { let authorization = self.authorization; Bundle { actions: self @@ -306,7 +306,7 @@ impl Bundle { context: &mut R, mut spend_auth: impl FnMut(&mut R, &T, T::SpendAuth) -> Result, step: impl FnOnce(&mut R, T) -> Result, - ) -> Result, E> { + ) -> Result, E> { let authorization = self.authorization; let new_actions = self .actions @@ -342,13 +342,13 @@ pub enum BundleAuthError { AuthLengthMismatch(usize, usize), } -impl Bundle { +impl Bundle { /// Compute the authorizing data for a bundle and apply it to the bundle, returning the /// authorized result. pub fn with_auth Result>( self, f: F, - ) -> Result, BundleAuthError> { + ) -> Result, BundleAuthError> { let auth = f(&self).map_err(BundleAuthError::Wrapped)?; let actions_len = self.actions.len(); @@ -388,7 +388,7 @@ impl Authorized { } } -impl Bundle { +impl Bundle { /// Computes a commitment to the authorizing data within for this bundle. /// /// This together with `Bundle::commitment` bind the entire bundle. diff --git a/src/value.rs b/src/value.rs index 1e867d6a..08c35fe2 100644 --- a/src/value.rs +++ b/src/value.rs @@ -14,8 +14,8 @@ //! [`Action`]: crate::bundle::Action //! [`Bundle`]: crate::bundle::Bundle -use std::convert::TryInto; -use std::fmt; +use std::convert::{TryInto, TryFrom}; +use std::fmt::{self, Debug}; use std::iter::Sum; use std::ops::{Add, Sub}; @@ -65,19 +65,20 @@ impl NoteValue { } } + impl Sub for NoteValue { - type Output = Result; + type Output = Option; fn sub(self, rhs: Self) -> Self::Output { - let a: i64 = self.0.try_into().map_err(|_| OverflowError)?; - let b: i64 = rhs.0.try_into().map_err(|_| OverflowError)?; - Ok(ValueSum(a - b)) + let a = self.0 as i128; + let b = rhs.0 as i128; + a.checked_sub(b).filter(|v| v > &(-(std::u64::MAX as i128))).map(ValueSum) } } -/// A sum of Orchard note values. +/// A sum of Orchard note values #[derive(Clone, Copy, Debug, Default, PartialEq)] -pub struct ValueSum(i64); +pub struct ValueSum(i128); impl ValueSum { pub(crate) fn zero() -> Self { @@ -90,21 +91,29 @@ impl ValueSum { /// This only enforces that the value is a signed 63-bit integer. Callers should /// enforce any additional constraints on the value's valid range themselves. pub fn from_raw(value: i64) -> Self { - ValueSum(value) + ValueSum(value as i128) } } impl Add for ValueSum { - type Output = Result; + type Output = Option; fn add(self, rhs: Self) -> Self::Output { - self.0.checked_add(rhs.0).map(ValueSum).ok_or(OverflowError) + self.0.checked_add(rhs.0).filter(|v| v < &(std::u64::MAX as i128)).map(ValueSum) } } impl<'a> Sum<&'a ValueSum> for Result { fn sum>(iter: I) -> Self { - iter.fold(Ok(ValueSum(0)), |acc, cv| acc? + *cv) + iter.fold(Ok(ValueSum(0)), |acc, cv| (acc? + *cv).ok_or(OverflowError)) + } +} + +impl TryFrom for i64 { + type Error = OverflowError; + + fn try_from(v: ValueSum) -> Result { + i64::try_from(v.0).map_err(|_| OverflowError) } } @@ -190,11 +199,12 @@ impl ValueCommitment { let hasher = pallas::Point::hash_to_curve("z.cash:Orchard-cv"); let V = hasher(b"v"); let R = hasher(b"r"); + let value = i64::try_from(value.0).expect("value must be in valid range"); - let value = if value.0.is_negative() { - -pallas::Scalar::from_u64((-value.0) as u64) + let value = if value.is_negative() { + -pallas::Scalar::from_u64((-value) as u64) } else { - pallas::Scalar::from_u64(value.0 as u64) + pallas::Scalar::from_u64(value as u64) }; ValueCommitment(V * value + R * rcv.0) @@ -266,9 +276,4 @@ mod tests { assert_eq!(redpallas::VerificationKey::from(&bsk), bvk); } } - - /// Serialize the value commitment to its canonical byte representation. - pub fn to_bytes(&self) -> [u8; 32] { - self.0.to_bytes() - } }