From 9119d6e7a1cff3d954d3f9b7040eac695d82f588 Mon Sep 17 00:00:00 2001 From: Murisi Tarusenga Date: Tue, 7 May 2024 16:41:01 +0200 Subject: [PATCH] Guarantee success in ValueSum::from_pair. --- masp_primitives/src/convert.rs | 15 ++--- masp_primitives/src/transaction/builder.rs | 18 ++--- .../src/transaction/components/amount.rs | 66 +++++++++---------- .../transaction/components/sapling/builder.rs | 6 +- .../src/transaction/components/transparent.rs | 10 ++- .../components/transparent/builder.rs | 12 ++-- masp_proofs/benches/convert.rs | 5 +- masp_proofs/src/circuit/convert.rs | 5 +- 8 files changed, 59 insertions(+), 78 deletions(-) diff --git a/masp_primitives/src/convert.rs b/masp_primitives/src/convert.rs index 2821dbc1..04c71014 100644 --- a/masp_primitives/src/convert.rs +++ b/masp_primitives/src/convert.rs @@ -223,12 +223,11 @@ mod tests { #[test] fn test_homomorphism() { // Left operand - let a = ValueSum::from_pair(zec(), 5i128).unwrap() - + ValueSum::from_pair(btc(), 6i128).unwrap() - + ValueSum::from_pair(xan(), 7i128).unwrap(); + let a = ValueSum::from_pair(zec(), 5i128) + + ValueSum::from_pair(btc(), 6i128) + + ValueSum::from_pair(xan(), 7i128); // Right operand - let b = ValueSum::from_pair(zec(), 2i128).unwrap() - + ValueSum::from_pair(xan(), 10i128).unwrap(); + let b = ValueSum::from_pair(zec(), 2i128) + ValueSum::from_pair(xan(), 10i128); // Test homomorphism assert_eq!( AllowedConversion::from(a.clone() + b.clone()), @@ -238,9 +237,9 @@ mod tests { #[test] fn test_serialization() { // Make conversion - let a: AllowedConversion = (ValueSum::from_pair(zec(), 5i128).unwrap() - + ValueSum::from_pair(btc(), 6i128).unwrap() - + ValueSum::from_pair(xan(), 7i128).unwrap()) + let a: AllowedConversion = (ValueSum::from_pair(zec(), 5i128) + + ValueSum::from_pair(btc(), 6i128) + + ValueSum::from_pair(xan(), 7i128)) .into(); // Serialize conversion let mut data = Vec::new(); diff --git a/masp_primitives/src/transaction/builder.rs b/masp_primitives/src/transaction/builder.rs index c14e7897..fd742064 100644 --- a/masp_primitives/src/transaction/builder.rs +++ b/masp_primitives/src/transaction/builder.rs @@ -292,13 +292,13 @@ impl Builder { } /// Returns the sum of the transparent, Sapling, and TZE value balances. - pub fn value_balance(&self) -> Result { + pub fn value_balance(&self) -> I128Sum { let value_balances = [ - self.transparent_builder.value_balance()?, + self.transparent_builder.value_balance(), self.sapling_builder.value_balance(), ]; - Ok(value_balances.into_iter().sum::()) + value_balances.into_iter().sum::() } /// Builds a transaction from the configured spends and outputs. @@ -337,7 +337,7 @@ impl Builder { // // After fees are accounted for, the value balance of the transaction must be zero. - let balance_after_fees = self.value_balance()? - I128Sum::from_sum(fee); + let balance_after_fees = self.value_balance() - I128Sum::from_sum(fee); if balance_after_fees != ValueSum::zero() { return Err(Error::InsufficientFunds(-balance_after_fees)); @@ -600,8 +600,7 @@ mod tests { assert_eq!( builder.mock_build(), Err(Error::InsufficientFunds( - I128Sum::from_pair(zec(), 50000).unwrap() - + &I128Sum::from_sum(DEFAULT_FEE.clone()) + I128Sum::from_pair(zec(), 50000) + &I128Sum::from_sum(DEFAULT_FEE.clone()) )) ); } @@ -616,8 +615,7 @@ mod tests { assert_eq!( builder.mock_build(), Err(Error::InsufficientFunds( - I128Sum::from_pair(zec(), 50000).unwrap() - + &I128Sum::from_sum(DEFAULT_FEE.clone()) + I128Sum::from_pair(zec(), 50000) + &I128Sum::from_sum(DEFAULT_FEE.clone()) )) ); } @@ -649,9 +647,7 @@ mod tests { .unwrap(); assert_eq!( builder.mock_build(), - Err(Error::InsufficientFunds( - ValueSum::from_pair(zec(), 1).unwrap() - )) + Err(Error::InsufficientFunds(ValueSum::from_pair(zec(), 1))) ); } diff --git a/masp_primitives/src/transaction/components/amount.rs b/masp_primitives/src/transaction/components/amount.rs index 2530894f..9a91bbec 100644 --- a/masp_primitives/src/transaction/components/amount.rs +++ b/masp_primitives/src/transaction/components/amount.rs @@ -17,7 +17,7 @@ use zcash_encoding::Vector; pub const MAX_MONEY: u64 = u64::MAX; lazy_static::lazy_static! { -pub static ref DEFAULT_FEE: U64Sum = ValueSum::from_pair(zec(), 1000).unwrap(); +pub static ref DEFAULT_FEE: U64Sum = ValueSum::from_pair(zec(), 1000); } /// A type-safe representation of some quantity of Zcash. /// @@ -99,20 +99,20 @@ where Value: BorshSerialize + BorshDeserialize + PartialEq + Eq + Copy + Default, { /// Creates an ValueSum from a Value. - pub fn from_pair(atype: Unit, amount: Value) -> Result { + pub fn from_pair(atype: Unit, amount: Value) -> Self { if amount == Value::default() { - Ok(Self::zero()) + Self::zero() } else { let mut ret = BTreeMap::new(); ret.insert(atype, amount); - Ok(ValueSum(ret)) + ValueSum(ret) } } /// Filters out everything but the given AssetType from this ValueSum pub fn project(&self, index: Unit) -> Self { let val = self.0.get(&index).copied().unwrap_or_default(); - Self::from_pair(index, val).unwrap() + Self::from_pair(index, val) } /// Get the given AssetType within this ValueSum @@ -252,9 +252,7 @@ impl ValueSum { })?; let mut ret = Self::zero(); for (atype, amt) in vec { - ret += Self::from_pair(atype, amt).map_err(|_| { - std::io::Error::new(std::io::ErrorKind::InvalidData, "amount out of range") - })?; + ret += Self::from_pair(atype, amt); } Ok(ret) } @@ -283,9 +281,7 @@ impl ValueSum { })?; let mut ret = Self::zero(); for (atype, amt) in vec { - ret += Self::from_pair(atype, amt).map_err(|_| { - std::io::Error::new(std::io::ErrorKind::InvalidData, "amount out of range") - })?; + ret += Self::from_pair(atype, amt); } Ok(ret) } @@ -314,9 +310,7 @@ impl ValueSum { })?; let mut ret = Self::zero(); for (atype, amt) in vec { - ret += Self::from_pair(atype, amt).map_err(|_| { - std::io::Error::new(std::io::ErrorKind::InvalidData, "amount out of range") - })?; + ret += Self::from_pair(atype, amt); } Ok(ret) } @@ -729,7 +723,7 @@ pub fn zec() -> AssetType { } pub fn default_fee() -> ValueSum { - ValueSum::from_pair(zec(), 10000).unwrap() + ValueSum::from_pair(zec(), 10000) } #[cfg(any(test, feature = "test-dependencies"))] @@ -741,25 +735,25 @@ pub mod testing { prop_compose! { pub fn arb_i64_sum()(asset_type in arb_asset_type(), amt in i64::MIN..i64::MAX) -> I64Sum { - ValueSum::from_pair(asset_type, amt).unwrap() + ValueSum::from_pair(asset_type, amt) } } prop_compose! { pub fn arb_i128_sum()(asset_type in arb_asset_type(), amt in i128::MIN..i128::MAX) -> I128Sum { - ValueSum::from_pair(asset_type, amt).unwrap() + ValueSum::from_pair(asset_type, amt) } } prop_compose! { pub fn arb_nonnegative_amount()(asset_type in arb_asset_type(), amt in 0u64..MAX_MONEY) -> U64Sum { - ValueSum::from_pair(asset_type, amt).unwrap() + ValueSum::from_pair(asset_type, amt) } } prop_compose! { pub fn arb_positive_amount()(asset_type in arb_asset_type(), amt in 1u64..MAX_MONEY) -> U64Sum { - ValueSum::from_pair(asset_type, amt).unwrap() + ValueSum::from_pair(asset_type, amt) } } } @@ -790,7 +784,7 @@ mod tests { } macro_rules! value_amount { ($t:ty, $val:expr) => {{ - let mut amount = <$t>::from_pair(zec(), 1).unwrap(); + let mut amount = <$t>::from_pair(zec(), 1); *amount.0.get_mut(&zec()).unwrap() = $val; amount }}; @@ -798,23 +792,23 @@ mod tests { let test_amounts_i32 = [ value_amount!(I32Sum, 0), // zec() asset with value 0 - I32Sum::from_pair(zec(), -1).unwrap(), - I32Sum::from_pair(zec(), i32::MAX).unwrap(), - I32Sum::from_pair(zec(), -i32::MAX).unwrap(), + I32Sum::from_pair(zec(), -1), + I32Sum::from_pair(zec(), i32::MAX), + I32Sum::from_pair(zec(), -i32::MAX), ]; let test_amounts_i64 = [ value_amount!(I64Sum, 0), // zec() asset with value 0 - I64Sum::from_pair(zec(), -1).unwrap(), - I64Sum::from_pair(zec(), i64::MAX).unwrap(), - I64Sum::from_pair(zec(), -i64::MAX).unwrap(), + I64Sum::from_pair(zec(), -1), + I64Sum::from_pair(zec(), i64::MAX), + I64Sum::from_pair(zec(), -i64::MAX), ]; let test_amounts_i128 = [ value_amount!(I128Sum, 0), // zec() asset with value 0 - I128Sum::from_pair(zec(), MAX_MONEY as i128).unwrap(), + I128Sum::from_pair(zec(), MAX_MONEY as i128), value_amount!(I128Sum, MAX_MONEY as i128 + 1), - I128Sum::from_pair(zec(), -(MAX_MONEY as i128)).unwrap(), + I128Sum::from_pair(zec(), -(MAX_MONEY as i128)), value_amount!(I128Sum, -(MAX_MONEY as i128 - 1)), ]; @@ -896,28 +890,28 @@ mod tests { #[test] #[should_panic] fn add_panics_on_overflow() { - let v = ValueSum::from_pair(zec(), MAX_MONEY).unwrap(); - let _sum = v + ValueSum::from_pair(zec(), 1).unwrap(); + let v = ValueSum::from_pair(zec(), MAX_MONEY); + let _sum = v + ValueSum::from_pair(zec(), 1); } #[test] #[should_panic] fn add_assign_panics_on_overflow() { - let mut a = ValueSum::from_pair(zec(), MAX_MONEY).unwrap(); - a += ValueSum::from_pair(zec(), 1).unwrap(); + let mut a = ValueSum::from_pair(zec(), MAX_MONEY); + a += ValueSum::from_pair(zec(), 1); } #[test] #[should_panic] fn sub_panics_on_underflow() { - let v = ValueSum::from_pair(zec(), 0u64).unwrap(); - let _diff = v - ValueSum::from_pair(zec(), 1).unwrap(); + let v = ValueSum::from_pair(zec(), 0u64); + let _diff = v - ValueSum::from_pair(zec(), 1); } #[test] #[should_panic] fn sub_assign_panics_on_underflow() { - let mut a = ValueSum::from_pair(zec(), 0u64).unwrap(); - a -= ValueSum::from_pair(zec(), 1).unwrap(); + let mut a = ValueSum::from_pair(zec(), 0u64); + a -= ValueSum::from_pair(zec(), 1); } } diff --git a/masp_primitives/src/transaction/components/sapling/builder.rs b/masp_primitives/src/transaction/components/sapling/builder.rs index 5ac1d1ae..f35030b1 100644 --- a/masp_primitives/src/transaction/components/sapling/builder.rs +++ b/masp_primitives/src/transaction/components/sapling/builder.rs @@ -470,8 +470,7 @@ impl SaplingBuilder

{ let alpha = jubjub::Fr::random(&mut rng); - self.value_balance += ValueSum::from_pair(note.asset_type, note.value.into()) - .map_err(|_| Error::InvalidAmount)?; + self.value_balance += ValueSum::from_pair(note.asset_type, note.value.into()); self.spends.push(SpendDescriptionInfo { extsk, @@ -540,8 +539,7 @@ impl SaplingBuilder

{ memo, )?; - self.value_balance -= - ValueSum::from_pair(asset_type, value.into()).map_err(|_| Error::InvalidAmount)?; + self.value_balance -= ValueSum::from_pair(asset_type, value.into()); self.outputs.push(output); diff --git a/masp_primitives/src/transaction/components/transparent.rs b/masp_primitives/src/transaction/components/transparent.rs index 18a41b3a..ef4758fd 100644 --- a/masp_primitives/src/transaction/components/transparent.rs +++ b/masp_primitives/src/transaction/components/transparent.rs @@ -63,7 +63,7 @@ impl Bundle { /// transferred out of the transparent pool into shielded pools or to fees; a negative value /// means that the containing transaction has funds being transferred into the transparent pool /// from the shielded pools. - pub fn value_balance(&self) -> Result + pub fn value_balance(&self) -> I128Sum where E: From, { @@ -71,18 +71,16 @@ impl Bundle { .vin .iter() .map(|p| ValueSum::from_pair(p.asset_type, p.value as i128)) - .sum::>() - .map_err(|_| BalanceError::Overflow)?; + .sum::(); let output_sum = self .vout .iter() .map(|p| ValueSum::from_pair(p.asset_type, p.value as i128)) - .sum::>() - .map_err(|_| BalanceError::Overflow)?; + .sum::(); // Cannot panic when subtracting two positive i64 - Ok(input_sum - output_sum) + input_sum - output_sum } } diff --git a/masp_primitives/src/transaction/components/transparent/builder.rs b/masp_primitives/src/transaction/components/transparent/builder.rs index 31fbd4c4..5cb1fa35 100644 --- a/masp_primitives/src/transaction/components/transparent/builder.rs +++ b/masp_primitives/src/transaction/components/transparent/builder.rs @@ -6,7 +6,7 @@ use crate::{ asset_type::AssetType, transaction::{ components::{ - amount::{BalanceError, I128Sum, ValueSum, MAX_MONEY}, + amount::{I128Sum, ValueSum, MAX_MONEY}, transparent::{self, fees, Authorization, Authorized, Bundle, TxIn, TxOut}, }, sighash::TransparentAuthorizingContext, @@ -130,14 +130,13 @@ impl TransparentBuilder { Ok(()) } - pub fn value_balance(&self) -> Result { + pub fn value_balance(&self) -> I128Sum { #[cfg(feature = "transparent-inputs")] let input_sum = self .inputs .iter() .map(|input| ValueSum::from_pair(input.coin.asset_type, input.coin.value as i128)) - .sum::>() - .map_err(|_| BalanceError::Overflow)?; + .sum::(); #[cfg(not(feature = "transparent-inputs"))] let input_sum = ValueSum::zero(); @@ -146,11 +145,10 @@ impl TransparentBuilder { .vout .iter() .map(|vo| ValueSum::from_pair(vo.asset_type, vo.value as i128)) - .sum::>() - .map_err(|_| BalanceError::Overflow)?; + .sum::(); // Cannot panic when subtracting two positive i64 - Ok(input_sum - output_sum) + input_sum - output_sum } pub fn build(self) -> Option> { diff --git a/masp_proofs/benches/convert.rs b/masp_proofs/benches/convert.rs index 0ac849fe..2a2ed062 100644 --- a/masp_proofs/benches/convert.rs +++ b/masp_proofs/benches/convert.rs @@ -39,9 +39,8 @@ fn criterion_benchmark(c: &mut Criterion) { let mint_value = i as i128 + 1; let allowed_conversion: AllowedConversion = (ValueSum::from_pair(spend_asset, spend_value) - .unwrap() - + ValueSum::from_pair(output_asset, output_value).unwrap() - + ValueSum::from_pair(mint_asset, mint_value).unwrap()) + + ValueSum::from_pair(output_asset, output_value) + + ValueSum::from_pair(mint_asset, mint_value)) .into(); let value = rng.next_u64(); diff --git a/masp_proofs/src/circuit/convert.rs b/masp_proofs/src/circuit/convert.rs index 7fbcc88b..ec28c6e6 100644 --- a/masp_proofs/src/circuit/convert.rs +++ b/masp_proofs/src/circuit/convert.rs @@ -155,9 +155,8 @@ fn test_convert_circuit_with_bls12_381() { let mint_value = i as i128 + 1; let allowed_conversion: AllowedConversion = (ValueSum::from_pair(spend_asset, spend_value) - .unwrap() - + ValueSum::from_pair(output_asset, output_value).unwrap() - + ValueSum::from_pair(mint_asset, mint_value).unwrap()) + + ValueSum::from_pair(output_asset, output_value) + + ValueSum::from_pair(mint_asset, mint_value)) .into(); let value = rng.next_u64();