Make ValueSum correctly respect the proper specified range.

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<ValueSum, value::OverflowError> {
+    fn value_sum(&self) -> Option<ValueSum> {
         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<SigningMetadata>, Circuit) {
+    fn build<V: TryFrom<i64>(self, mut rng: impl RngCore) -> (Action<SigningMetadata>, 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<V: TryFrom<i64>>(
         mut self,
         mut rng: impl RngCore,
         pk: &ProvingKey,
-    ) -> Result<Bundle<Unauthorized>, Error> {
+    ) -> Result<Bundle<Unauthorized, V>, 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::<V>)).unzip();
 
         // Verify that bsk and bvk are consistent.
         let bvk = (actions.iter().map(|a| a.cv_net()).sum::<ValueCommitment>()
@@ -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<redpallas::Signature<SpendAuth>>, SpendValidatingKey);
 }
 
-impl Bundle<Unauthorized> {
+impl<V> Bundle<Unauthorized, V> {
     /// 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<Unauthorized> {
         self,
         mut rng: R,
         sighash: [u8; 32],
-    ) -> Bundle<PartiallyAuthorized> {
+    ) -> Bundle<PartiallyAuthorized, V> {
         self.authorize(
             &mut rng,
             |rng, _, SigningMetadata { dummy_ask, ak }| {
@@ -385,7 +389,7 @@ impl Bundle<Unauthorized> {
         mut rng: R,
         sighash: [u8; 32],
         signing_keys: &[SpendAuthorizingKey],
-    ) -> Result<Bundle<Authorized>, Error> {
+    ) -> Result<Bundle<Authorized, V>, Error> {
         signing_keys
             .iter()
             .fold(self.prepare(&mut rng, sighash), |partial, ask| {
@@ -395,7 +399,7 @@ impl Bundle<Unauthorized> {
     }
 }
 
-impl Bundle<PartiallyAuthorized> {
+impl<V> Bundle<PartiallyAuthorized, V> {
     /// 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<PartiallyAuthorized> {
     /// Finalizes this bundle, enabling it to be included in a transaction.
     ///
     /// Returns an error if any signatures are missing.
-    pub fn finalize(self) -> Result<Bundle<Authorized>, Error> {
+    pub fn finalize(self) -> Result<Bundle<Authorized, V>, Error> {
         self.try_authorize(
             &mut (),
             |_, _, (sig, _)| match sig {

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<T: Authorization> {
+pub struct Bundle<T: Authorization, V> {
     /// The list of actions that make up this bundle.
     actions: NonEmpty<Action<T::SpendAuth>>,
     /// Orchard-specific transaction-level flags for this bundle.
@@ -221,19 +221,19 @@ pub struct Bundle<T: Authorization> {
     /// 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<T: Authorization> Bundle<T> {
+impl<T: Authorization, V> Bundle<T, V> {
     /// Constructs a `Bundle` from its constituent parts.
     pub fn from_parts(
         actions: NonEmpty<Action<T::SpendAuth>>,
         flags: Flags,
-        value_balance: ValueSum,
+        value_balance: V,
         anchor: Anchor,
         authorization: T,
     ) -> Self {
@@ -259,7 +259,7 @@ impl<T: Authorization> Bundle<T> {
     /// 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<T: Authorization> Bundle<T> {
         context: &mut R,
         mut spend_auth: impl FnMut(&mut R, &T, T::SpendAuth) -> U::SpendAuth,
         step: impl FnOnce(&mut R, T) -> U,
-    ) -> Bundle<U> {
+    ) -> Bundle<U, V> {
         let authorization = self.authorization;
         Bundle {
             actions: self
@@ -306,7 +306,7 @@ impl<T: Authorization> Bundle<T> {
         context: &mut R,
         mut spend_auth: impl FnMut(&mut R, &T, T::SpendAuth) -> Result<U::SpendAuth, E>,
         step: impl FnOnce(&mut R, T) -> Result<U, E>,
-    ) -> Result<Bundle<U>, E> {
+    ) -> Result<Bundle<U, V>, E> {
         let authorization = self.authorization;
         let new_actions = self
             .actions
@@ -342,13 +342,13 @@ pub enum BundleAuthError<E> {
     AuthLengthMismatch(usize, usize),
 }
 
-impl Bundle<Unauthorized> {
+impl<V> Bundle<Unauthorized, V> {
     /// Compute the authorizing data for a bundle and apply it to the bundle, returning the
     /// authorized result.
     pub fn with_auth<E, F: FnOnce(&Self) -> Result<BundleAuth, E>>(
         self,
         f: F,
-    ) -> Result<Bundle<Authorized>, BundleAuthError<E>> {
+    ) -> Result<Bundle<Authorized, V>, BundleAuthError<E>> {
         let auth = f(&self).map_err(BundleAuthError::Wrapped)?;
         let actions_len = self.actions.len();
 
@@ -388,7 +388,7 @@ impl Authorized {
     }
 }
 
-impl Bundle<Authorized> {
+impl<V> Bundle<Authorized, V> {
     /// Computes a commitment to the authorizing data within for this bundle.
     ///
     /// This together with `Bundle::commitment` bind the entire bundle.

src/value.rs

@@ -14,8 +14,8 @@
 //! [`Action`]: crate::bundle::Action
 //! [`Bundle`]: crate::bundle::Bundle
 
-use std::convert::TryInto;
+use std::convert::{TryInto, TryFrom};
-use std::fmt;
+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<ValueSum, OverflowError>;
+    type Output = Option<ValueSum>;
 
     fn sub(self, rhs: Self) -> Self::Output {
-        let a: i64 = self.0.try_into().map_err(|_| OverflowError)?;
+        let a = self.0 as i128;
-        let b: i64 = rhs.0.try_into().map_err(|_| OverflowError)?;
+        let b = rhs.0 as i128;
-        Ok(ValueSum(a - b))
+	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<ValueSum, OverflowError>;
+    type Output = Option<ValueSum>;
 
     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<ValueSum, OverflowError> {
     fn sum<I: Iterator<Item = &'a ValueSum>>(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<ValueSum> for i64 {
+    type Error = OverflowError;
+
+    fn try_from(v: ValueSum) -> Result<i64, Self::Error> {
+        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() {
+        let value = if value.is_negative() {
-            -pallas::Scalar::from_u64((-value.0) as u64)
+            -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()
-    }
 }