From 4efb21d1c7639a309fb43dc52dab921a685d3a94 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 May 2021 09:57:28 -0600 Subject: [PATCH 1/6] Make amount addition and subtraction traits use checked operations. --- zcash_client_backend/Cargo.toml | 2 +- zcash_client_backend/src/data_api/error.rs | 7 ++ zcash_client_backend/src/data_api/wallet.rs | 8 ++- zcash_client_sqlite/src/chain.rs | 25 +++++-- zcash_client_sqlite/src/lib.rs | 2 +- zcash_client_sqlite/src/wallet/transact.rs | 5 +- zcash_extensions/src/transparent/demo.rs | 4 +- zcash_primitives/Cargo.toml | 11 ++- zcash_primitives/src/transaction/builder.rs | 36 +++++++--- .../src/transaction/components/amount.rs | 69 ++++++++++++++----- 10 files changed, 128 insertions(+), 41 deletions(-) diff --git a/zcash_client_backend/Cargo.toml b/zcash_client_backend/Cargo.toml index ef1e450a1f..1c5e21f823 100644 --- a/zcash_client_backend/Cargo.toml +++ b/zcash_client_backend/Cargo.toml @@ -23,7 +23,7 @@ hex = "0.4" jubjub = "0.6" nom = "6.1" percent-encoding = "2.1.0" -proptest = { version = "0.10.1", optional = true } +proptest = { version = "1.0.0", optional = true } protobuf = "2.20" rand_core = "0.6" subtle = "2.2.3" diff --git a/zcash_client_backend/src/data_api/error.rs b/zcash_client_backend/src/data_api/error.rs index 1968af8f94..bd4241d91e 100644 --- a/zcash_client_backend/src/data_api/error.rs +++ b/zcash_client_backend/src/data_api/error.rs @@ -24,6 +24,9 @@ pub enum ChainInvalid { #[derive(Debug)] pub enum Error { + /// The amount specified exceeds the allowed range. + InvalidAmount, + /// Unable to create a new spend because the wallet balance is not sufficient. InsufficientBalance(Amount, Amount), @@ -72,6 +75,10 @@ impl ChainInvalid { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self { + Error::InvalidAmount => write!( + f, + "The value lies outside the valid range of Zcash amounts." + ), Error::InsufficientBalance(have, need) => write!( f, "Insufficient balance (have {}, need {} including fee)", diff --git a/zcash_client_backend/src/data_api/wallet.rs b/zcash_client_backend/src/data_api/wallet.rs index 82a9a9a9b5..c8b5e5acbd 100644 --- a/zcash_client_backend/src/data_api/wallet.rs +++ b/zcash_client_backend/src/data_api/wallet.rs @@ -183,11 +183,15 @@ where .get_target_and_anchor_heights() .and_then(|x| x.ok_or_else(|| Error::ScanRequired.into()))?; - let target_value = value + DEFAULT_FEE; + let target_value = (value + DEFAULT_FEE).ok_or_else(|| E::from(Error::InvalidAmount))?; let spendable_notes = wallet_db.select_spendable_notes(account, target_value, anchor_height)?; // Confirm we were able to select sufficient value - let selected_value = spendable_notes.iter().map(|n| n.note_value).sum(); + let selected_value = spendable_notes + .iter() + .map(|n| n.note_value) + .sum::>() + .ok_or_else(|| E::from(Error::InvalidAmount))?; if selected_value < target_value { return Err(E::from(Error::InsufficientBalance( selected_value, diff --git a/zcash_client_sqlite/src/chain.rs b/zcash_client_sqlite/src/chain.rs index b416ebe889..1b3692239c 100644 --- a/zcash_client_sqlite/src/chain.rs +++ b/zcash_client_sqlite/src/chain.rs @@ -358,13 +358,19 @@ mod tests { scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap(); // Account balance should reflect both received notes - assert_eq!(get_balance(&db_data, AccountId(0)).unwrap(), value + value2); + assert_eq!( + get_balance(&db_data, AccountId(0)).unwrap(), + (value + value2).unwrap() + ); // "Rewind" to height of last scanned block rewind_to_height(&db_data, sapling_activation_height() + 1).unwrap(); // Account balance should be unaltered - assert_eq!(get_balance(&db_data, AccountId(0)).unwrap(), value + value2); + assert_eq!( + get_balance(&db_data, AccountId(0)).unwrap(), + (value + value2).unwrap() + ); // Rewind so that one block is dropped rewind_to_height(&db_data, sapling_activation_height()).unwrap(); @@ -376,7 +382,10 @@ mod tests { scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap(); // Account balance should again reflect both received notes - assert_eq!(get_balance(&db_data, AccountId(0)).unwrap(), value + value2); + assert_eq!( + get_balance(&db_data, AccountId(0)).unwrap(), + (value + value2).unwrap() + ); } #[test] @@ -485,7 +494,10 @@ mod tests { scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap(); // Account balance should reflect both received notes - assert_eq!(get_balance(&db_data, AccountId(0)).unwrap(), value + value2); + assert_eq!( + get_balance(&db_data, AccountId(0)).unwrap(), + (value + value2).unwrap() + ); } #[test] @@ -543,6 +555,9 @@ mod tests { scan_cached_blocks(&tests::network(), &db_cache, &mut db_write, None).unwrap(); // Account balance should equal the change - assert_eq!(get_balance(&db_data, AccountId(0)).unwrap(), value - value2); + assert_eq!( + get_balance(&db_data, AccountId(0)).unwrap(), + (value - value2).unwrap() + ); } } diff --git a/zcash_client_sqlite/src/lib.rs b/zcash_client_sqlite/src/lib.rs index 0d9047574b..b9629e1cfc 100644 --- a/zcash_client_sqlite/src/lib.rs +++ b/zcash_client_sqlite/src/lib.rs @@ -701,7 +701,7 @@ mod tests { let note = Note { g_d: change_addr.diversifier().g_d().unwrap(), pk_d: *change_addr.pk_d(), - value: (in_value - value).into(), + value: (in_value - value).unwrap().into(), rseed, }; let encryptor = sapling_note_encryption::<_, Network>( diff --git a/zcash_client_sqlite/src/wallet/transact.rs b/zcash_client_sqlite/src/wallet/transact.rs index 5fccb430f3..129ff1fea7 100644 --- a/zcash_client_sqlite/src/wallet/transact.rs +++ b/zcash_client_sqlite/src/wallet/transact.rs @@ -356,7 +356,10 @@ mod tests { // Verified balance does not include the second note let (_, anchor_height2) = (&db_data).get_target_and_anchor_heights().unwrap().unwrap(); - assert_eq!(get_balance(&db_data, AccountId(0)).unwrap(), value + value); + assert_eq!( + get_balance(&db_data, AccountId(0)).unwrap(), + (value + value).unwrap() + ); assert_eq!( get_balance_at(&db_data, AccountId(0), anchor_height2).unwrap(), value diff --git a/zcash_extensions/src/transparent/demo.rs b/zcash_extensions/src/transparent/demo.rs index 6109ec8585..741f79cd97 100644 --- a/zcash_extensions/src/transparent/demo.rs +++ b/zcash_extensions/src/transparent/demo.rs @@ -740,7 +740,7 @@ mod tests { TzeOutPoint::new(tx_a.txid().0, 0), tx_a.tze_outputs[0].clone(), ); - let value_xfr = value - DEFAULT_FEE; + let value_xfr = (value - DEFAULT_FEE).unwrap(); db_b.demo_transfer_to_close(prevout_a, value_xfr, preimage_1, h2) .map_err(|e| format!("transfer failure: {:?}", e)) .unwrap(); @@ -769,7 +769,7 @@ mod tests { builder_c .add_transparent_output( &TransparentAddress::PublicKey([0; 20]), - value_xfr - DEFAULT_FEE, + (value_xfr - DEFAULT_FEE).unwrap(), ) .unwrap(); diff --git a/zcash_primitives/Cargo.toml b/zcash_primitives/Cargo.toml index e62ec0cbcd..d5b910bbed 100644 --- a/zcash_primitives/Cargo.toml +++ b/zcash_primitives/Cargo.toml @@ -31,7 +31,9 @@ hex = "0.4" jubjub = "0.6" lazy_static = "1" log = "0.4" -proptest = { version = "0.10.1", optional = true } +nonempty = "0.6" +orchard = { git = "https://github.com/zcash/orchard", branch = "main" } +proptest = { version = "1.0.0", optional = true } rand = "0.8" rand_core = "0.6" ripemd160 = { version = "0.9", optional = true } @@ -43,11 +45,16 @@ zcash_note_encryption = { version = "0.0", path = "../components/zcash_note_encr # Temporary workaround for https://github.com/myrrlyn/funty/issues/3 funty = "=1.1.0" +[dependencies.pasta_curves] +git = "https://github.com/zcash/pasta_curves.git" +rev = "b55a6960dfafd7f767e2820ddf1adaa499322f98" + [dev-dependencies] criterion = "0.3" hex-literal = "0.3" -proptest = "0.10.1" +proptest = "1.0.0" rand_xorshift = "0.3" +orchard = { git = "https://github.com/zcash/orchard", branch = "main", features = ["test-dependencies"] } [features] transparent-inputs = ["ripemd160", "secp256k1"] diff --git a/zcash_primitives/src/transaction/builder.rs b/zcash_primitives/src/transaction/builder.rs index 280dffbd57..cc6b260d8e 100644 --- a/zcash_primitives/src/transaction/builder.rs +++ b/zcash_primitives/src/transaction/builder.rs @@ -251,18 +251,18 @@ impl TransparentInputs { Ok(()) } - fn value_sum(&self) -> Amount { + fn value_sum(&self) -> Option { #[cfg(feature = "transparent-inputs")] { self.inputs .iter() .map(|input| input.coin.value) - .sum::() + .sum::>() } #[cfg(not(feature = "transparent-inputs"))] { - Amount::zero() + Some(Amount::zero()) } } @@ -643,8 +643,18 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> { // // Valid change - let change = self.mtx.value_balance - self.fee + self.transparent_inputs.value_sum() - - self.mtx.vout.iter().map(|vo| vo.value).sum::(); + let change = self.mtx.value_balance - self.fee + + self + .transparent_inputs + .value_sum() + .ok_or(Error::InvalidAmount)? + - self + .mtx + .vout + .iter() + .map(|vo| vo.value) + .sum::>() + .ok_or(Error::InvalidAmount)?; #[cfg(feature = "zfuture")] let change = change @@ -653,13 +663,17 @@ impl<'a, P: consensus::Parameters, R: RngCore> Builder<'a, P, R> { .builders .iter() .map(|ein| ein.prevout.value) - .sum::() + .sum::>() + .ok_or(Error::InvalidAmount)? - self .mtx .tze_outputs .iter() .map(|tzo| tzo.value) - .sum::(); + .sum::>() + .ok_or(Error::InvalidAmount)?; + + let change = change.ok_or(Error::InvalidAmount)?; if change.is_negative() { return Err(Error::ChangeIsNegative(change)); @@ -1150,7 +1164,9 @@ mod tests { let builder = Builder::new(TEST_NETWORK, H0); assert_eq!( builder.build(consensus::BranchId::Sapling, &MockTxProver), - Err(Error::ChangeIsNegative(Amount::zero() - DEFAULT_FEE)) + Err(Error::ChangeIsNegative( + (Amount::zero() - DEFAULT_FEE).unwrap() + )) ); } @@ -1168,7 +1184,7 @@ mod tests { assert_eq!( builder.build(consensus::BranchId::Sapling, &MockTxProver), Err(Error::ChangeIsNegative( - Amount::from_i64(-50000).unwrap() - DEFAULT_FEE + (Amount::from_i64(-50000).unwrap() - DEFAULT_FEE).unwrap() )) ); } @@ -1186,7 +1202,7 @@ mod tests { assert_eq!( builder.build(consensus::BranchId::Sapling, &MockTxProver), Err(Error::ChangeIsNegative( - Amount::from_i64(-50000).unwrap() - DEFAULT_FEE + (Amount::from_i64(-50000).unwrap() - DEFAULT_FEE).unwrap() )) ); } diff --git a/zcash_primitives/src/transaction/components/amount.rs b/zcash_primitives/src/transaction/components/amount.rs index f47b485f74..1fa376f622 100644 --- a/zcash_primitives/src/transaction/components/amount.rs +++ b/zcash_primitives/src/transaction/components/amount.rs @@ -1,3 +1,4 @@ +use std::convert::TryFrom; use std::iter::Sum; use std::ops::{Add, AddAssign, Sub, SubAssign}; @@ -101,6 +102,14 @@ impl Amount { } } +impl TryFrom for Amount { + type Error = (); + + fn try_from(value: i64) -> Result { + Amount::from_i64(value) + } +} + impl From for i64 { fn from(amount: Amount) -> i64 { amount.0 @@ -114,36 +123,52 @@ impl From for u64 { } impl Add for Amount { - type Output = Amount; + type Output = Option; + + fn add(self, rhs: Amount) -> Option { + Amount::from_i64(self.0 + rhs.0).ok() + } +} + +impl Add for Option { + type Output = Self; - fn add(self, rhs: Amount) -> Amount { - Amount::from_i64(self.0 + rhs.0).expect("addition should remain in range") + fn add(self, rhs: Amount) -> Option { + self.and_then(|lhs| lhs + rhs) } } impl AddAssign for Amount { fn add_assign(&mut self, rhs: Amount) { - *self = *self + rhs + *self = (*self + rhs).expect("Addition must produce a valid amount value.") } } impl Sub for Amount { - type Output = Amount; + type Output = Option; + + fn sub(self, rhs: Amount) -> Option { + Amount::from_i64(self.0 - rhs.0).ok() + } +} + +impl Sub for Option { + type Output = Self; - fn sub(self, rhs: Amount) -> Amount { - Amount::from_i64(self.0 - rhs.0).expect("subtraction should remain in range") + fn sub(self, rhs: Amount) -> Option { + self.and_then(|lhs| lhs - rhs) } } impl SubAssign for Amount { fn sub_assign(&mut self, rhs: Amount) { - *self = *self - rhs + *self = (*self - rhs).expect("Subtraction must produce a valid amount value.") } } -impl Sum for Amount { - fn sum>(iter: I) -> Amount { - iter.fold(Amount::zero(), Add::add) +impl Sum for Option { + fn sum>(iter: I) -> Self { + iter.fold(Some(Amount::zero()), |acc, a| acc? + a) } } @@ -153,11 +178,23 @@ pub mod testing { use super::{Amount, MAX_MONEY}; + prop_compose! { + pub fn arb_amount()(amt in -MAX_MONEY..MAX_MONEY) -> Amount { + Amount::from_i64(amt).unwrap() + } + } + prop_compose! { pub fn arb_nonnegative_amount()(amt in 0i64..MAX_MONEY) -> Amount { Amount::from_i64(amt).unwrap() } } + + prop_compose! { + pub fn arb_positive_amount()(amt in 1i64..MAX_MONEY) -> Amount { + Amount::from_i64(amt).unwrap() + } + } } #[cfg(test)] @@ -213,10 +250,9 @@ mod tests { } #[test] - #[should_panic] - fn add_panics_on_overflow() { + fn add_overflow() { let v = Amount(MAX_MONEY); - let _sum = v + Amount(1); + assert_eq!(v + Amount(1), None) } #[test] @@ -227,10 +263,9 @@ mod tests { } #[test] - #[should_panic] - fn sub_panics_on_underflow() { + fn sub_underflow() { let v = Amount(-MAX_MONEY); - let _diff = v - Amount(1); + assert_eq!(v - Amount(1), None) } #[test] From 3dc05a69eb691eaaf401813c7a35032b1e4317e9 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 May 2021 10:10:21 -0600 Subject: [PATCH 2/6] Add Nu5 NetworkUpgrade variant. --- components/zcash_note_encryption/src/lib.rs | 1 - zcash_primitives/src/consensus.rs | 107 +++++++++++++++++++- 2 files changed, 105 insertions(+), 3 deletions(-) diff --git a/components/zcash_note_encryption/src/lib.rs b/components/zcash_note_encryption/src/lib.rs index 481ee1e81e..3bece34f22 100644 --- a/components/zcash_note_encryption/src/lib.rs +++ b/components/zcash_note_encryption/src/lib.rs @@ -445,7 +445,6 @@ pub fn try_compact_note_decryption>( /// /// Implements part of section 4.19.3 of the /// [Zcash Protocol Specification](https://zips.z.cash/protocol/nu5.pdf#decryptovk) -/// For decryption using a Full Viewing Key see [`try_sapling_output_recovery`]. pub fn try_output_recovery_with_ock>( domain: &D, ock: &OutgoingCipherKey, diff --git a/zcash_primitives/src/consensus.rs b/zcash_primitives/src/consensus.rs index 4e5c49dd4b..02e3913f73 100644 --- a/zcash_primitives/src/consensus.rs +++ b/zcash_primitives/src/consensus.rs @@ -3,7 +3,7 @@ use std::cmp::{Ord, Ordering}; use std::convert::TryFrom; use std::fmt; -use std::ops::{Add, Sub}; +use std::ops::{Add, Bound, RangeBounds, Sub}; use crate::constants; @@ -195,6 +195,7 @@ impl Parameters for MainNetwork { NetworkUpgrade::Blossom => Some(BlockHeight(653_600)), NetworkUpgrade::Heartwood => Some(BlockHeight(903_000)), NetworkUpgrade::Canopy => Some(BlockHeight(1_046_400)), + NetworkUpgrade::Nu5 => None, #[cfg(feature = "zfuture")] NetworkUpgrade::ZFuture => None, } @@ -239,6 +240,7 @@ impl Parameters for TestNetwork { NetworkUpgrade::Blossom => Some(BlockHeight(584_000)), NetworkUpgrade::Heartwood => Some(BlockHeight(903_800)), NetworkUpgrade::Canopy => Some(BlockHeight(1_028_500)), + NetworkUpgrade::Nu5 => None, #[cfg(feature = "zfuture")] NetworkUpgrade::ZFuture => None, } @@ -352,6 +354,10 @@ pub enum NetworkUpgrade { /// /// [Canopy]: https://z.cash/upgrade/canopy/ Canopy, + /// The [Nu5] network upgrade. + /// + /// [Nu5]: https://z.cash/upgrade/nu5/ + Nu5, /// The ZFUTURE network upgrade. /// /// This upgrade is expected never to activate on mainnet; @@ -369,6 +375,7 @@ impl fmt::Display for NetworkUpgrade { NetworkUpgrade::Blossom => write!(f, "Blossom"), NetworkUpgrade::Heartwood => write!(f, "Heartwood"), NetworkUpgrade::Canopy => write!(f, "Canopy"), + NetworkUpgrade::Nu5 => write!(f, "Nu5"), #[cfg(feature = "zfuture")] NetworkUpgrade::ZFuture => write!(f, "ZFUTURE"), } @@ -383,6 +390,7 @@ impl NetworkUpgrade { NetworkUpgrade::Blossom => BranchId::Blossom, NetworkUpgrade::Heartwood => BranchId::Heartwood, NetworkUpgrade::Canopy => BranchId::Canopy, + NetworkUpgrade::Nu5 => BranchId::Nu5, #[cfg(feature = "zfuture")] NetworkUpgrade::ZFuture => BranchId::ZFuture, } @@ -399,6 +407,7 @@ const UPGRADES_IN_ORDER: &[NetworkUpgrade] = &[ NetworkUpgrade::Blossom, NetworkUpgrade::Heartwood, NetworkUpgrade::Canopy, + NetworkUpgrade::Nu5, ]; pub const ZIP212_GRACE_PERIOD: u32 = 32256; @@ -430,6 +439,8 @@ pub enum BranchId { Heartwood, /// The consensus rules deployed by [`NetworkUpgrade::Canopy`]. Canopy, + /// The consensus rules deployed by [`NetworkUpgrade::Nu5`]. + Nu5, /// Candidates for future consensus rules; this branch will never /// activate on mainnet. #[cfg(feature = "zfuture")] @@ -447,6 +458,7 @@ impl TryFrom for BranchId { 0x2bb4_0e60 => Ok(BranchId::Blossom), 0xf5b9_230b => Ok(BranchId::Heartwood), 0xe9ff_75a6 => Ok(BranchId::Canopy), + 0xf919_a198 => Ok(BranchId::Nu5), #[cfg(feature = "zfuture")] 0xffff_ffff => Ok(BranchId::ZFuture), _ => Err("Unknown consensus branch ID"), @@ -463,6 +475,7 @@ impl From for u32 { BranchId::Blossom => 0x2bb4_0e60, BranchId::Heartwood => 0xf5b9_230b, BranchId::Canopy => 0xe9ff_75a6, + BranchId::Nu5 => 0xf919_a198, #[cfg(feature = "zfuture")] BranchId::ZFuture => 0xffff_ffff, } @@ -484,6 +497,94 @@ impl BranchId { // Sprout rules apply before any network upgrade BranchId::Sprout } + + /// Returns the range of heights for the consensus epoch associated with this branch id. + /// + /// The resulting tuple implements the [`RangeBounds`] trait. + pub fn height_range(&self, params: &P) -> Option> { + self.height_bounds(params).map(|(lower, upper)| { + ( + Bound::Included(lower), + upper.map_or(Bound::Unbounded, Bound::Excluded), + ) + }) + } + + /// Returns the range of heights for the consensus epoch associated with this branch id. + /// + /// The return type of this value is slightly more precise than [`Self::height_range`]. + pub fn height_bounds( + &self, + params: &P, + ) -> Option<(BlockHeight, Option)> { + match self { + BranchId::Sprout => params + .activation_height(NetworkUpgrade::Overwinter) + .map(|upper| (BlockHeight(0), Some(upper))), + BranchId::Overwinter => params + .activation_height(NetworkUpgrade::Overwinter) + .map(|lower| (lower, params.activation_height(NetworkUpgrade::Sapling))), + BranchId::Sapling => params + .activation_height(NetworkUpgrade::Sapling) + .map(|lower| (lower, params.activation_height(NetworkUpgrade::Blossom))), + BranchId::Blossom => params + .activation_height(NetworkUpgrade::Blossom) + .map(|lower| (lower, params.activation_height(NetworkUpgrade::Heartwood))), + BranchId::Heartwood => params + .activation_height(NetworkUpgrade::Heartwood) + .map(|lower| (lower, params.activation_height(NetworkUpgrade::Canopy))), + BranchId::Canopy => params + .activation_height(NetworkUpgrade::Canopy) + .map(|lower| (lower, params.activation_height(NetworkUpgrade::Nu5))), + BranchId::Nu5 => params.activation_height(NetworkUpgrade::Nu5).map(|lower| { + #[cfg(feature = "zfuture")] + let upper = params.activation_height(NetworkUpgrade::ZFuture); + #[cfg(not(feature = "zfuture"))] + let upper = None; + (lower, upper) + }), + #[cfg(feature = "zfuture")] + BranchId::ZFuture => params + .activation_height(NetworkUpgrade::ZFuture) + .map(|lower| (lower, None)), + } + } +} + +#[cfg(any(test, feature = "test-dependencies"))] +pub mod testing { + use proptest::sample::select; + use proptest::strategy::{Just, Strategy}; + + use super::{BlockHeight, BranchId, Parameters}; + + pub fn arb_branch_id() -> impl Strategy { + select(vec![ + BranchId::Sprout, + BranchId::Overwinter, + BranchId::Sapling, + BranchId::Blossom, + BranchId::Heartwood, + BranchId::Canopy, + BranchId::Nu5, + #[cfg(feature = "zfuture")] + BranchId::ZFuture, + ]) + } + + pub fn arb_height( + branch_id: BranchId, + params: &P, + ) -> impl Strategy> { + branch_id + .height_bounds(params) + .map_or(Strategy::boxed(Just(None)), |(lower, upper)| { + Strategy::boxed( + (lower.0..upper.map_or(std::u32::MAX, |u| u.0)) + .prop_map(|h| Some(BlockHeight(h))), + ) + }) + } } #[cfg(test)] @@ -503,7 +604,9 @@ mod tests { MAIN_NETWORK.activation_height(nu_a), MAIN_NETWORK.activation_height(nu_b), ) { - (a, b) if a < b => (), + (Some(a), Some(b)) if a < b => (), + (Some(_), None) => (), + (None, None) => (), _ => panic!( "{} should not be before {} in UPGRADES_IN_ORDER", nu_a, nu_b From 936b552de2b78ea2512b4e50587d6c7ddf2fce45 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 May 2021 10:16:09 -0600 Subject: [PATCH 3/6] Add NoteValue newtype, Nullifier::as_ref and proptest generation. --- zcash_primitives/src/sapling.rs | 84 ++++++++++++++++++++++++++++++++- zcash_primitives/src/zip32.rs | 13 +++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/zcash_primitives/src/sapling.rs b/zcash_primitives/src/sapling.rs index f99f8efe14..12c36bde94 100644 --- a/zcash_primitives/src/sapling.rs +++ b/zcash_primitives/src/sapling.rs @@ -16,13 +16,14 @@ use group::{Curve, Group, GroupEncoding}; use lazy_static::lazy_static; use rand_core::{CryptoRng, RngCore}; use std::array::TryFromSliceError; -use std::convert::TryInto; +use std::convert::{TryFrom, TryInto}; use std::io::{self, Read, Write}; use subtle::{Choice, ConstantTimeEq}; use crate::{ constants::{self, SPENDING_KEY_GENERATOR}, merkle_tree::Hashable, + transaction::components::amount::MAX_MONEY, }; use self::{ @@ -360,6 +361,11 @@ impl Nullifier { self.0.to_vec() } } +impl AsRef<[u8]> for Nullifier { + fn as_ref(&self) -> &[u8] { + &self.0 + } +} impl ConstantTimeEq for Nullifier { fn ct_eq(&self, other: &Self) -> Choice { @@ -367,6 +373,27 @@ impl ConstantTimeEq for Nullifier { } } +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct NoteValue(u64); + +impl TryFrom for NoteValue { + type Error = (); + + fn try_from(value: u64) -> Result { + if value <= MAX_MONEY as u64 { + Ok(NoteValue(value)) + } else { + Err(()) + } + } +} + +impl From for u64 { + fn from(value: NoteValue) -> u64 { + value.0 + } +} + #[derive(Clone, Debug)] pub struct Note { /// The value of the note @@ -485,3 +512,58 @@ impl Note { } } } + +#[cfg(any(test, feature = "test-dependencies"))] +pub mod testing { + use proptest::prelude::*; + use std::cmp::min; + use std::convert::TryFrom; + + use crate::{ + transaction::components::amount::MAX_MONEY, zip32::testing::arb_extended_spending_key, + }; + + use super::{Node, Note, NoteValue, PaymentAddress, Rseed}; + + prop_compose! { + pub fn arb_note_value()(value in 0u64..MAX_MONEY as u64) -> NoteValue { + NoteValue::try_from(value).unwrap() + } + } + + prop_compose! { + /// The + pub fn arb_positive_note_value(bound: u64)( + value in 1u64..(min(bound, MAX_MONEY as u64)) + ) -> NoteValue { + NoteValue::try_from(value).unwrap() + } + } + + pub fn arb_payment_address() -> impl Strategy { + arb_extended_spending_key() + .prop_map(|sk| sk.default_address().map(|(_, a)| a)) + .prop_filter("A valid payment address is required.", |r| r.is_ok()) + .prop_map(|r| r.unwrap()) + } + + prop_compose! { + pub fn arb_node()(value in prop::array::uniform32(prop::num::u8::ANY)) -> Node { + Node::new(value) + } + } + + prop_compose! { + pub fn arb_note(value: NoteValue)( + addr in arb_payment_address(), + rseed in prop::array::uniform32(prop::num::u8::ANY).prop_map(Rseed::AfterZip212) + ) -> Note { + Note { + value: value.into(), + g_d: addr.g_d().unwrap(), // this unwrap is safe because arb_payment_address always generates an address witha valid g_d + pk_d: *addr.pk_d(), + rseed + } + } + } +} diff --git a/zcash_primitives/src/zip32.rs b/zcash_primitives/src/zip32.rs index 8fb1ee4028..a39936d5b3 100644 --- a/zcash_primitives/src/zip32.rs +++ b/zcash_primitives/src/zip32.rs @@ -1071,3 +1071,16 @@ mod tests { } } } + +#[cfg(any(test, feature = "test-dependencies"))] +pub mod testing { + use proptest::prelude::*; + + use super::ExtendedSpendingKey; + + prop_compose! { + pub fn arb_extended_spending_key()(seed in prop::array::uniform32(prop::num::u8::ANY)) -> ExtendedSpendingKey { + ExtendedSpendingKey::master(&seed) + } + } +} From 76999eb5c7b83c095e95e731ae77e47f24bbe05d Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Tue, 11 May 2021 10:54:39 -0600 Subject: [PATCH 4/6] Make txid contents private & use txid for TzeOutPoint --- zcash_client_backend/src/data_api.rs | 4 +-- zcash_client_backend/src/proto.rs | 14 +++++++- zcash_client_backend/src/welding_rig.rs | 16 +++++---- zcash_client_sqlite/src/wallet.rs | 8 ++--- zcash_extensions/src/transparent/demo.rs | 8 ++--- .../src/transaction/components/tze.rs | 18 +++++----- zcash_primitives/src/transaction/mod.rs | 35 ++++++++++++++++--- 7 files changed, 72 insertions(+), 31 deletions(-) diff --git a/zcash_client_backend/src/data_api.rs b/zcash_client_backend/src/data_api.rs index 166fd80cba..3d823eaacf 100644 --- a/zcash_client_backend/src/data_api.rs +++ b/zcash_client_backend/src/data_api.rs @@ -412,14 +412,14 @@ pub mod testing { &mut self, _received_tx: &ReceivedTransaction, ) -> Result { - Ok(TxId([0u8; 32])) + Ok(TxId::from_bytes([0u8; 32])) } fn store_sent_tx( &mut self, _sent_tx: &SentTransaction, ) -> Result { - Ok(TxId([0u8; 32])) + Ok(TxId::from_bytes([0u8; 32])) } fn rewind_to_height(&mut self, _block_height: BlockHeight) -> Result<(), Self::Error> { diff --git a/zcash_client_backend/src/proto.rs b/zcash_client_backend/src/proto.rs index 9493c4c269..49d6ebf13e 100644 --- a/zcash_client_backend/src/proto.rs +++ b/zcash_client_backend/src/proto.rs @@ -8,7 +8,10 @@ use zcash_primitives::{ block::{BlockHash, BlockHeader}, consensus::BlockHeight, sapling::Nullifier, - transaction::components::sapling::{CompactOutputDescription, OutputDescription}, + transaction::{ + components::sapling::{CompactOutputDescription, OutputDescription}, + TxId, + }, }; use zcash_note_encryption::COMPACT_NOTE_SIZE; @@ -74,6 +77,15 @@ impl compact_formats::CompactBlock { } } +impl compact_formats::CompactTx { + /// Returns the transaction Id + pub fn txid(&self) -> TxId { + let mut hash = [0u8; 32]; + hash.copy_from_slice(&self.hash); + TxId::from_bytes(hash) + } +} + impl compact_formats::CompactOutput { /// Returns the note commitment for this output. /// diff --git a/zcash_client_backend/src/welding_rig.rs b/zcash_client_backend/src/welding_rig.rs index b48a4eec9d..55d354bd6e 100644 --- a/zcash_client_backend/src/welding_rig.rs +++ b/zcash_client_backend/src/welding_rig.rs @@ -12,7 +12,7 @@ use zcash_primitives::{ note_encryption::{try_sapling_compact_note_decryption, SaplingDomain}, Node, Note, Nullifier, PaymentAddress, SaplingIvk, }, - transaction::{components::sapling::CompactOutputDescription, TxId}, + transaction::components::sapling::CompactOutputDescription, zip32::ExtendedFullViewingKey, }; @@ -32,7 +32,8 @@ use crate::wallet::{AccountId, WalletShieldedOutput, WalletShieldedSpend, Wallet fn scan_output( params: &P, height: BlockHeight, - (index, output): (usize, CompactOutput), + index: usize, + output: CompactOutput, vks: &[(&AccountId, &K)], spent_from_accounts: &HashSet, tree: &mut CommitmentTree, @@ -198,6 +199,8 @@ pub fn scan_block( let block_height = block.height(); for tx in block.vtx.into_iter() { + let txid = tx.txid(); + let index = tx.index as usize; let num_spends = tx.spends.len(); let num_outputs = tx.outputs.len(); @@ -249,7 +252,7 @@ pub fn scan_block( }) .collect(); - for to_scan in tx.outputs.into_iter().enumerate() { + for (idx, c_out) in tx.outputs.into_iter().enumerate() { // Grab mutable references to new witnesses from previous outputs // in this transaction so that we can update them. Scoped so we // don't hold mutable references to shielded_outputs for too long. @@ -261,7 +264,8 @@ pub fn scan_block( if let Some(output) = scan_output( params, block_height, - to_scan, + idx, + c_out, vks, &spent_from_accounts, tree, @@ -275,11 +279,9 @@ pub fn scan_block( } if !(shielded_spends.is_empty() && shielded_outputs.is_empty()) { - let mut txid = TxId([0u8; 32]); - txid.0.copy_from_slice(&tx.hash); wtxs.push(WalletTx { txid, - index: tx.index as usize, + index, num_spends, num_outputs, shielded_spends, diff --git a/zcash_client_sqlite/src/wallet.rs b/zcash_client_sqlite/src/wallet.rs index 9fbc41185f..73e7ffd236 100644 --- a/zcash_client_sqlite/src/wallet.rs +++ b/zcash_client_sqlite/src/wallet.rs @@ -389,7 +389,7 @@ pub fn block_height_extrema

( /// /// let data_file = NamedTempFile::new().unwrap(); /// let db = WalletDb::for_path(data_file, Network::TestNetwork).unwrap(); -/// let height = get_tx_height(&db, TxId([0u8; 32])); +/// let height = get_tx_height(&db, TxId::from_bytes([0u8; 32])); /// ``` pub fn get_tx_height

( wdb: &WalletDb

, @@ -398,7 +398,7 @@ pub fn get_tx_height

( wdb.conn .query_row( "SELECT block FROM transactions WHERE txid = ?", - &[txid.0.to_vec()], + &[txid.as_ref().to_vec()], |row| row.get(0).map(u32::into), ) .optional() @@ -616,7 +616,7 @@ pub fn put_tx_meta<'a, P, N>( tx: &WalletTx, height: BlockHeight, ) -> Result { - let txid = tx.txid.0.to_vec(); + let txid = tx.txid.as_ref().to_vec(); if stmts .stmt_update_tx_meta .execute(params![u32::from(height), (tx.index as i64), txid])? @@ -643,7 +643,7 @@ pub fn put_tx_data<'a, P>( tx: &Transaction, created_at: Option, ) -> Result { - let txid = tx.txid().0.to_vec(); + let txid = tx.txid().as_ref().to_vec(); let mut raw_tx = vec![]; tx.write(&mut raw_tx)?; diff --git a/zcash_extensions/src/transparent/demo.rs b/zcash_extensions/src/transparent/demo.rs index 741f79cd97..520d4a96bb 100644 --- a/zcash_extensions/src/transparent/demo.rs +++ b/zcash_extensions/src/transparent/demo.rs @@ -625,7 +625,7 @@ mod tests { // let in_b = TzeIn { - prevout: TzeOutPoint::new(tx_a.txid().0, 0), + prevout: TzeOutPoint::new(tx_a.txid(), 0), witness: tze::Witness::from(0, &Witness::open(preimage_1)), }; let out_b = TzeOut { @@ -642,7 +642,7 @@ mod tests { // let in_c = TzeIn { - prevout: TzeOutPoint::new(tx_b.txid().0, 0), + prevout: TzeOutPoint::new(tx_b.txid(), 0), witness: tze::Witness::from(0, &Witness::close(preimage_2)), }; @@ -737,7 +737,7 @@ mod tests { extension_id: 0, }; let prevout_a = ( - TzeOutPoint::new(tx_a.txid().0, 0), + TzeOutPoint::new(tx_a.txid(), 0), tx_a.tze_outputs[0].clone(), ); let value_xfr = (value - DEFAULT_FEE).unwrap(); @@ -759,7 +759,7 @@ mod tests { extension_id: 0, }; let prevout_b = ( - TzeOutPoint::new(tx_a.txid().0, 0), + TzeOutPoint::new(tx_a.txid(), 0), tx_b.tze_outputs[0].clone(), ); db_c.demo_close(prevout_b, preimage_2) diff --git a/zcash_primitives/src/transaction/components/tze.rs b/zcash_primitives/src/transaction/components/tze.rs index d26485825d..1744b43c5d 100644 --- a/zcash_primitives/src/transaction/components/tze.rs +++ b/zcash_primitives/src/transaction/components/tze.rs @@ -10,6 +10,7 @@ use std::convert::TryFrom; use crate::{ extensions::transparent as tze, serialize::{CompactSize, Vector}, + transaction::TxId, }; use super::amount::Amount; @@ -20,24 +21,23 @@ fn to_io_error(_: std::num::TryFromIntError) -> io::Error { #[derive(Clone, Debug, PartialEq)] pub struct TzeOutPoint { - hash: [u8; 32], + txid: TxId, n: u32, } impl TzeOutPoint { - pub fn new(hash: [u8; 32], n: u32) -> Self { - TzeOutPoint { hash, n } + pub fn new(txid: TxId, n: u32) -> Self { + TzeOutPoint { txid, n } } pub fn read(mut reader: R) -> io::Result { - let mut hash = [0u8; 32]; - reader.read_exact(&mut hash)?; + let txid = TxId::read(&mut reader)?; let n = reader.read_u32::()?; - Ok(TzeOutPoint { hash, n }) + Ok(TzeOutPoint { txid, n }) } pub fn write(&self, mut writer: W) -> io::Result<()> { - writer.write_all(&self.hash)?; + self.txid.write(&mut writer)?; writer.write_u32::(self.n) } @@ -45,8 +45,8 @@ impl TzeOutPoint { self.n } - pub fn hash(&self) -> &[u8; 32] { - &self.hash + pub fn txid(&self) -> &TxId { + &self.txid } } diff --git a/zcash_primitives/src/transaction/mod.rs b/zcash_primitives/src/transaction/mod.rs index 78bdf9546b..3d359dd3af 100644 --- a/zcash_primitives/src/transaction/mod.rs +++ b/zcash_primitives/src/transaction/mod.rs @@ -41,7 +41,7 @@ const ZFUTURE_VERSION_GROUP_ID: u32 = 0xFFFFFFFF; const ZFUTURE_TX_VERSION: u32 = 0x0000FFFF; #[derive(Clone, Copy, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] -pub struct TxId(pub [u8; 32]); +pub struct TxId([u8; 32]); impl fmt::Display for TxId { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -51,6 +51,29 @@ impl fmt::Display for TxId { } } +impl AsRef<[u8; 32]> for TxId { + fn as_ref(&self) -> &[u8; 32] { + &self.0 + } +} + +impl TxId { + pub fn from_bytes(bytes: [u8; 32]) -> Self { + TxId(bytes) + } + + pub fn read(mut reader: R) -> io::Result { + let mut hash = [0u8; 32]; + reader.read_exact(&mut hash)?; + Ok(TxId::from_bytes(hash)) + } + + pub fn write(&self, mut writer: W) -> io::Result<()> { + writer.write_all(&self.0)?; + Ok(()) + } +} + /// The set of defined transaction format versions. /// /// This is serialized in the first four or eight bytes of the transaction format, and @@ -502,7 +525,7 @@ pub mod testing { use super::{ components::{amount::MAX_MONEY, Amount, OutPoint, TxIn, TxOut}, - Transaction, TransactionData, TxVersion, + Transaction, TransactionData, TxId, TxVersion, }; #[cfg(feature = "zfuture")] @@ -519,6 +542,10 @@ pub mod testing { 0x6a, // OP_RETURN, ]; + pub fn arb_txid() -> impl Strategy { + prop::array::uniform32(any::()).prop_map(TxId::from_bytes) + } + prop_compose! { pub fn arb_outpoint()(hash in prop::array::uniform32(1u8..), n in 1..100u32) -> OutPoint { OutPoint::new(hash, n) @@ -551,8 +578,8 @@ pub mod testing { #[cfg(feature = "zfuture")] prop_compose! { - pub fn arb_tzeoutpoint()(hash in prop::array::uniform32(1u8..), n in 1..100u32) -> TzeOutPoint { - TzeOutPoint::new(hash, n) + pub fn arb_tzeoutpoint()(txid in arb_txid(), n in 1..100u32) -> TzeOutPoint { + TzeOutPoint::new(txid, n) } } From 62bd06f14e5af8343df94ce3ad61809233faacc4 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Thu, 27 May 2021 16:33:46 -0600 Subject: [PATCH 5/6] Apply suggestions from code review Co-authored-by: str4d --- zcash_primitives/src/consensus.rs | 7 ++++++- zcash_primitives/src/sapling.rs | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/zcash_primitives/src/consensus.rs b/zcash_primitives/src/consensus.rs index 02e3913f73..07f12f4b36 100644 --- a/zcash_primitives/src/consensus.rs +++ b/zcash_primitives/src/consensus.rs @@ -512,7 +512,12 @@ impl BranchId { /// Returns the range of heights for the consensus epoch associated with this branch id. /// - /// The return type of this value is slightly more precise than [`Self::height_range`]. + /// The return type of this value is slightly more precise than [`Self::height_range`]: + /// - `Some((x, Some(y)))` means that the consensus rules corresponding to this branch id + /// are in effect for the range `x..y` + /// - `Some((x, None))` means that the consensus rules corresponding to this branch id are + /// in effect for the range `x..` + /// - `None` means that the consensus rules corresponding to this branch id are never in effect. pub fn height_bounds( &self, params: &P, diff --git a/zcash_primitives/src/sapling.rs b/zcash_primitives/src/sapling.rs index 12c36bde94..8b0207ec5b 100644 --- a/zcash_primitives/src/sapling.rs +++ b/zcash_primitives/src/sapling.rs @@ -526,7 +526,7 @@ pub mod testing { use super::{Node, Note, NoteValue, PaymentAddress, Rseed}; prop_compose! { - pub fn arb_note_value()(value in 0u64..MAX_MONEY as u64) -> NoteValue { + pub fn arb_note_value()(value in 0u64..=MAX_MONEY as u64) -> NoteValue { NoteValue::try_from(value).unwrap() } } @@ -534,7 +534,7 @@ pub mod testing { prop_compose! { /// The pub fn arb_positive_note_value(bound: u64)( - value in 1u64..(min(bound, MAX_MONEY as u64)) + value in 1u64..=(min(bound, MAX_MONEY as u64)) ) -> NoteValue { NoteValue::try_from(value).unwrap() } From 168314cec654b9928c9ce6ec127c4e59287e5adb Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Mon, 31 May 2021 11:08:45 -0600 Subject: [PATCH 6/6] Update changelog. --- zcash_primitives/CHANGELOG.md | 14 +++++++++++++- zcash_primitives/src/consensus.rs | 4 ++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/zcash_primitives/CHANGELOG.md b/zcash_primitives/CHANGELOG.md index 3576f25ae9..ad5eb67ad4 100644 --- a/zcash_primitives/CHANGELOG.md +++ b/zcash_primitives/CHANGELOG.md @@ -10,7 +10,12 @@ and this library adheres to Rust's notion of - `zcash_primitives::transaction::Builder::with_progress_notifier`, for setting a notification channel on which transaction build progress updates will be sent. - +- `zcash_primitives::transaction::Txid::{read, write, from_bytes}` +- `zcash_primitives::sapling::NoteValue` a typesafe wrapper for Sapling note values. +- `zcash_primitives::consensus::BranchId::{height_range, height_bounds}` functions + to provide range values for branch active heights. +- `zcash_primitives::consensus::NetworkUpgrade::Nu5` value representing the Nu5 upgrade. +- `zcash_primitives::consensus::BranchId::Nu5` value representing the Nu5 consensus branch. ### Changed - MSRV is now 1.51.0. - The following modules and helpers have been moved into @@ -24,6 +29,13 @@ and this library adheres to Rust's notion of - `zcash_primitives::util::{hash_to_scalar, generate_random_rseed}` - Renamed `zcash_primitives::transaction::components::JSDescription` to `JsDescription` (matching Rust naming conventions). +- `zcash_primitives::transaction::TxId` contents is now private. +- Renamed `zcash_primitives::transaction::components::tze::hash` to + `zcash_primitives::transaction::components::tze::txid` +- `zcash_primitives::transaction::components::tze::TzeOutPoint` constructor + now taxes a TxId rather than a raw byte array. +- `zcash_primitives::transaction::components::Amount` addition, subtraction, + and summation now return `Option` rather than panicing on overflow. ## [0.5.0] - 2021-03-26 ### Added diff --git a/zcash_primitives/src/consensus.rs b/zcash_primitives/src/consensus.rs index 07f12f4b36..437677b34f 100644 --- a/zcash_primitives/src/consensus.rs +++ b/zcash_primitives/src/consensus.rs @@ -513,9 +513,9 @@ impl BranchId { /// Returns the range of heights for the consensus epoch associated with this branch id. /// /// The return type of this value is slightly more precise than [`Self::height_range`]: - /// - `Some((x, Some(y)))` means that the consensus rules corresponding to this branch id + /// - `Some((x, Some(y)))` means that the consensus rules corresponding to this branch id /// are in effect for the range `x..y` - /// - `Some((x, None))` means that the consensus rules corresponding to this branch id are + /// - `Some((x, None))` means that the consensus rules corresponding to this branch id are /// in effect for the range `x..` /// - `None` means that the consensus rules corresponding to this branch id are never in effect. pub fn height_bounds(