Make Amount opaque, and use it more

This helps to ensure type-safety of values that are required to satisfy
zatoshi range bounds.

librustzcash/src/rustzcash.rs

@@ -63,6 +63,7 @@ use zcash_primitives::{
     merkle_tree::CommitmentTreeWitness,
     note_encryption::sapling_ka_agree,
     sapling::{merkle_hash, spend_sig},
+    transaction::components::Amount,
     zip32, JUBJUB,
 };
 use zcash_proofs::{
@@ -704,6 +705,11 @@ pub extern "system" fn librustzcash_sapling_final_check(
     binding_sig: *const [c_uchar; 64],
     sighash_value: *const [c_uchar; 32],
 ) -> bool {
+    let value_balance = match Amount::from_i64(value_balance, true) {
+        Ok(vb) => vb,
+        Err(()) => return false,
+    };
+
     // Deserialize the signature
     let binding_sig = match Signature::read(&(unsafe { &*binding_sig })[..]) {
         Ok(sig) => sig,
@@ -1022,6 +1028,11 @@ pub extern "system" fn librustzcash_sapling_binding_sig(
     sighash: *const [c_uchar; 32],
     result: *mut [c_uchar; 64],
 ) -> bool {
+    let value_balance = match Amount::from_i64(value_balance, true) {
+        Ok(vb) => vb,
+        Err(()) => return false,
+    };
+
     // Sign
     let sig = match unsafe { &*ctx }.binding_sig(value_balance, unsafe { &*sighash }, &JUBJUB) {
         Ok(s) => s,

zcash_primitives/src/prover.rs

@@ -8,7 +8,9 @@ use sapling_crypto::{
 };
 
 use crate::{
-    merkle_tree::CommitmentTreeWitness, sapling::Node, transaction::components::GROTH_PROOF_SIZE,
+    merkle_tree::CommitmentTreeWitness,
+    sapling::Node,
+    transaction::components::{Amount, GROTH_PROOF_SIZE},
 };
 
 /// Interface for creating zero-knowledge proofs for shielded transactions.
@@ -63,7 +65,7 @@ pub trait TxProver {
     fn binding_sig(
         &self,
         ctx: &mut Self::SaplingProvingContext,
-        value_balance: i64,
+        value_balance: Amount,
         sighash: &[u8; 32],
     ) -> Result<Signature, ()>;
 }
@@ -80,8 +82,10 @@ pub(crate) mod mock {
     };
 
     use crate::{
-        merkle_tree::CommitmentTreeWitness, sapling::Node,
-        transaction::components::GROTH_PROOF_SIZE, JUBJUB,
+        merkle_tree::CommitmentTreeWitness,
+        sapling::Node,
+        transaction::components::{Amount, GROTH_PROOF_SIZE},
+        JUBJUB,
     };
 
     use super::TxProver;
@@ -153,7 +157,7 @@ pub(crate) mod mock {
         fn binding_sig(
             &self,
             _ctx: &mut Self::SaplingProvingContext,
-            _value_balance: i64,
+            _value_balance: Amount,
             _sighash: &[u8; 32],
         ) -> Result<Signature, ()> {
             Err(())

zcash_primitives/src/transaction/builder.rs

@@ -18,13 +18,12 @@ use crate::{
     prover::TxProver,
     sapling::{spend_sig, Node},
     transaction::{
-        components::{Amount, OutputDescription, SpendDescription, TxOut},
+        components::{amount::DEFAULT_FEE, Amount, OutputDescription, SpendDescription, TxOut},
         signature_hash_data, Transaction, TransactionData, SIGHASH_ALL,
     },
     JUBJUB,
 };
 
-const DEFAULT_FEE: Amount = Amount(10000);
 const DEFAULT_TX_EXPIRY_DELTA: u32 = 20;
 
 /// If there are any shielded inputs, always have at least two shielded outputs, padding
@@ -88,7 +87,7 @@ impl SaplingOutput {
         let note = Note {
             g_d,
             pk_d: to.pk_d.clone(),
-            value: value.0 as u64,
+            value: value.into(),
             r: rcm,
         };
 
@@ -252,7 +251,8 @@ impl<R: RngCore + CryptoRng> Builder<R> {
 
         let alpha = Fs::random(&mut self.rng);
 
-        self.mtx.value_balance += Amount(note.value as i64);
+        self.mtx.value_balance +=
+            Amount::from_u64(note.value).map_err(|_| Error(ErrorKind::InvalidAmount))?;
 
         self.spends.push(SpendDescriptionInfo {
             extsk,
@@ -528,7 +528,7 @@ impl<R: RngCore + CryptoRng> Builder<R> {
         }
         self.mtx.binding_sig = Some(
             prover
-                .binding_sig(&mut ctx, self.mtx.value_balance.0, &sighash)
+                .binding_sig(&mut ctx, self.mtx.value_balance, &sighash)
                 .map_err(|()| Error(ErrorKind::BindingSig))?,
         );
 
@@ -564,7 +564,7 @@ mod tests {
         let to = extfvk.default_address().unwrap().1;
 
         let mut builder = Builder::new(0);
-        match builder.add_sapling_output(ovk, to, Amount(-1), None) {
+        match builder.add_sapling_output(ovk, to, Amount::from_i64(-1, true).unwrap(), None) {
             Err(e) => assert_eq!(e.kind(), &ErrorKind::InvalidAmount),
             Ok(_) => panic!("Should have failed"),
         }
@@ -573,7 +573,10 @@ mod tests {
     #[test]
     fn fails_on_negative_transparent_output() {
         let mut builder = Builder::new(0);
-        match builder.add_transparent_output(&TransparentAddress::PublicKey([0; 20]), Amount(-1)) {
+        match builder.add_transparent_output(
+            &TransparentAddress::PublicKey([0; 20]),
+            Amount::from_i64(-1, true).unwrap(),
+        ) {
             Err(e) => assert_eq!(e.kind(), &ErrorKind::InvalidAmount),
             Ok(_) => panic!("Should have failed"),
         }
@@ -591,7 +594,10 @@ mod tests {
         {
             let builder = Builder::new(0);
             match builder.build(1, MockTxProver) {
-                Err(e) => assert_eq!(e.kind(), &ErrorKind::ChangeIsNegative(Amount(-10000))),
+                Err(e) => assert_eq!(
+                    e.kind(),
+                    &ErrorKind::ChangeIsNegative(Amount::from_i64(-10000, true).unwrap())
+                ),
                 Ok(_) => panic!("Should have failed"),
             }
         }
@@ -605,10 +611,18 @@ mod tests {
         {
             let mut builder = Builder::new(0);
             builder
-                .add_sapling_output(ovk.clone(), to.clone(), Amount(50000), None)
+                .add_sapling_output(
+                    ovk.clone(),
+                    to.clone(),
+                    Amount::from_u64(50000).unwrap(),
+                    None,
+                )
                 .unwrap();
             match builder.build(1, MockTxProver) {
-                Err(e) => assert_eq!(e.kind(), &ErrorKind::ChangeIsNegative(Amount(-60000))),
+                Err(e) => assert_eq!(
+                    e.kind(),
+                    &ErrorKind::ChangeIsNegative(Amount::from_i64(-60000, true).unwrap())
+                ),
                 Ok(_) => panic!("Should have failed"),
             }
         }
@@ -618,10 +632,16 @@ mod tests {
         {
             let mut builder = Builder::new(0);
             builder
-                .add_transparent_output(&TransparentAddress::PublicKey([0; 20]), Amount(50000))
+                .add_transparent_output(
+                    &TransparentAddress::PublicKey([0; 20]),
+                    Amount::from_u64(50000).unwrap(),
+                )
                 .unwrap();
             match builder.build(1, MockTxProver) {
-                Err(e) => assert_eq!(e.kind(), &ErrorKind::ChangeIsNegative(Amount(-60000))),
+                Err(e) => assert_eq!(
+                    e.kind(),
+                    &ErrorKind::ChangeIsNegative(Amount::from_i64(-60000, true).unwrap())
+                ),
                 Ok(_) => panic!("Should have failed"),
             }
         }
@@ -647,13 +667,24 @@ mod tests {
                 )
                 .unwrap();
             builder
-                .add_sapling_output(ovk.clone(), to.clone(), Amount(30000), None)
+                .add_sapling_output(
+                    ovk.clone(),
+                    to.clone(),
+                    Amount::from_u64(30000).unwrap(),
+                    None,
+                )
                 .unwrap();
             builder
-                .add_transparent_output(&TransparentAddress::PublicKey([0; 20]), Amount(20000))
+                .add_transparent_output(
+                    &TransparentAddress::PublicKey([0; 20]),
+                    Amount::from_u64(20000).unwrap(),
+                )
                 .unwrap();
             match builder.build(1, MockTxProver) {
-                Err(e) => assert_eq!(e.kind(), &ErrorKind::ChangeIsNegative(Amount(-1))),
+                Err(e) => assert_eq!(
+                    e.kind(),
+                    &ErrorKind::ChangeIsNegative(Amount::from_i64(-1, true).unwrap())
+                ),
                 Ok(_) => panic!("Should have failed"),
             }
         }
@@ -678,10 +709,13 @@ mod tests {
                 .add_sapling_spend(extsk, to.diversifier, note2, witness2)
                 .unwrap();
             builder
-                .add_sapling_output(ovk, to, Amount(30000), None)
+                .add_sapling_output(ovk, to, Amount::from_u64(30000).unwrap(), None)
                 .unwrap();
             builder
-                .add_transparent_output(&TransparentAddress::PublicKey([0; 20]), Amount(20000))
+                .add_transparent_output(
+                    &TransparentAddress::PublicKey([0; 20]),
+                    Amount::from_u64(20000).unwrap(),
+                )
                 .unwrap();
             match builder.build(1, MockTxProver) {
                 Err(e) => assert_eq!(e.kind(), &ErrorKind::BindingSig),

zcash_primitives/src/transaction/components.rs

@@ -10,7 +10,7 @@ use std::io::{self, Read, Write};
 use legacy::Script;
 use JUBJUB;
 
-mod amount;
+pub mod amount;
 pub use self::amount::Amount;
 
 // π_A + π_B + π_C
@@ -76,7 +76,12 @@ pub struct TxOut {
 
 impl TxOut {
     pub fn read<R: Read>(mut reader: &mut R) -> io::Result<Self> {
-        let value = Amount::read_i64(&mut reader, false)?;
+        let value = {
+            let mut tmp = [0; 8];
+            reader.read_exact(&mut tmp)?;
+            Amount::from_i64_le_bytes(tmp, false)
+        }
+        .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "value out of range"))?;
         let script_pubkey = Script::read(&mut reader)?;
 
         Ok(TxOut {
@@ -86,7 +91,7 @@ impl TxOut {
     }
 
     pub fn write<W: Write>(&self, mut writer: W) -> io::Result<()> {
-        writer.write_i64::<LittleEndian>(self.value.0)?;
+        writer.write_all(&self.value.to_i64_le_bytes())?;
         self.script_pubkey.write(&mut writer)
     }
 }
@@ -298,10 +303,20 @@ impl std::fmt::Debug for JSDescription {
 impl JSDescription {
     pub fn read<R: Read>(mut reader: R, use_groth: bool) -> io::Result<Self> {
         // Consensus rule (§4.3): Canonical encoding is enforced here
-        let vpub_old = Amount::read_u64(&mut reader)?;
+        let vpub_old = {
+            let mut tmp = [0; 8];
+            reader.read_exact(&mut tmp)?;
+            Amount::from_u64_le_bytes(tmp)
+        }
+        .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "vpub_old out of range"))?;
 
         // Consensus rule (§4.3): Canonical encoding is enforced here
-        let vpub_new = Amount::read_u64(&mut reader)?;
+        let vpub_new = {
+            let mut tmp = [0; 8];
+            reader.read_exact(&mut tmp)?;
+            Amount::from_u64_le_bytes(tmp)
+        }
+        .map_err(|_| io::Error::new(io::ErrorKind::InvalidData, "vpub_new out of range"))?;
 
         // Consensus rule (§4.3): One of vpub_old and vpub_new being zero is
         // enforced by CheckTransactionWithoutProofVerification() in zcashd.
@@ -374,8 +389,8 @@ impl JSDescription {
     }
 
     pub fn write<W: Write>(&self, mut writer: W) -> io::Result<()> {
-        writer.write_i64::<LittleEndian>(self.vpub_old.0)?;
-        writer.write_i64::<LittleEndian>(self.vpub_new.0)?;
+        writer.write_all(&self.vpub_old.to_i64_le_bytes())?;
+        writer.write_all(&self.vpub_new.to_i64_le_bytes())?;
         writer.write_all(&self.anchor)?;
         writer.write_all(&self.nullifiers[0])?;
         writer.write_all(&self.nullifiers[1])?;