Skip to content

Commit

Permalink
`zcash_client_backend::{fixed,standard,zip317}::SingleOutputChangeStr…
Browse files Browse the repository at this point in the history 
…ategy`

now implement a different strategy for choosing whether there will be any
change, and its value. The aims are:

* Ensure that it is possible to create fully transparent transactions with
  no change (this will be needed for ZIP 320). The `InsufficientFunds`
  error in this case should have a `required` field that reflects the
  additional amount needed, according to the fee calculated without an
  extra change output.
* Avoid leaking information about note amounts in some cases: an adversary
  that knew the number of external recipients and the sum of their outputs
  was able to learn the sum of the inputs if no change output was present.
* Defend against losing money by using `DustAction::AddDustToFee` with a
  too-high dust threshold.
* Ensure that if a "change memo" is requested, there will always be a
  shielded change output in which to put it. Previously, this would not
  be the case when using `DustAction::AddDustToFee`.

Co-authored-by: Jack Grigg <jack@electriccoin.co>
Co-authored-by: Kris Nuttycombe <kris@nutty.land>
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
  • Loading branch information
@daira @str4d @nuttycom
3 people committed Jun 19, 2024
1 parent 06c0895 commit 21d5731
Show file tree
Hide file tree
Showing 4 changed files with 389 additions and 79 deletions.
7 changes: 7 additions & 0 deletions zcash_client_backend/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ and this library adheres to Rust's notion of

### Changed
- MSRV is now 1.70.0.
- `zcash_client_backend::{fixed,standard,zip317}::SingleOutputChangeStrategy`
now implement a different strategy for choosing whether there will be any
change, and its value. This can avoid leaking information about note amounts
in some cases. It also ensures that there will be a change output whenever a
`change_memo` is given, and defends against losing money by using
`DustAction::AddDustToFee` with a too-high dust threshold.
See [#1430](https://github.com/zcash/librustzcash/pull/1430) for details.
- `zcash_client_backend::zip321` has been extracted to, and is now a reexport
of the root module of the `zip321` crate. Several of the APIs of this module
have changed as a consequence of this extraction; please see the `zip321`
Expand Down
232 changes: 176 additions & 56 deletions zcash_client_backend/src/fees/common.rs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use core::cmp::max;

use zcash_primitives::{
consensus::{self, BlockHeight},
memo::MemoBytes,
transaction::{
components::amount::{BalanceError, NonNegativeAmount},
fees::{transparent, FeeRule},
fees::{transparent, zip317::MINIMUM_FEE, FeeRule},
},
};
use zcash_protocol::ShieldedProtocol;
Expand All @@ -25,6 +27,22 @@ pub(crate) struct NetFlows {
orchard_out: NonNegativeAmount,
}

impl NetFlows {
fn total_in(&self) -> Result<NonNegativeAmount, BalanceError> {
(self.t_in + self.sapling_in + self.orchard_in).ok_or(BalanceError::Overflow)
}
fn total_out(&self) -> Result<NonNegativeAmount, BalanceError> {
(self.t_out + self.sapling_out + self.orchard_out).ok_or(BalanceError::Overflow)
}
/// Returns true iff the flows excluding change are fully transparent.
fn is_transparent(&self) -> bool {
!(self.sapling_in.is_positive()
|| self.sapling_out.is_positive()
|| self.orchard_in.is_positive()
|| self.orchard_out.is_positive())
}
}

#[allow(clippy::too_many_arguments)]
pub(crate) fn calculate_net_flows<NoteRefT: Clone, F: FeeRule, E>(
transparent_inputs: &[impl transparent::InputView],
Expand Down Expand Up @@ -137,7 +155,7 @@ pub(crate) fn single_change_output_balance<
dust_output_policy: &DustOutputPolicy,
default_dust_threshold: NonNegativeAmount,
change_memo: Option<MemoBytes>,
_fallback_change_pool: ShieldedProtocol,
fallback_change_pool: ShieldedProtocol,
) -> Result<TransactionBalance, ChangeError<E, NoteRefT>>
where
E: From<F::Error> + From<BalanceError>,
Expand All @@ -152,14 +170,26 @@ where
#[cfg(feature = "orchard")]
orchard,
)?;
let (change_pool, sapling_change, _orchard_change) =
single_change_output_policy(&net_flows, _fallback_change_pool);
let total_in = net_flows
.total_in()
.map_err(|e| ChangeError::StrategyError(E::from(e)))?;
let total_out = net_flows
.total_out()
.map_err(|e| ChangeError::StrategyError(E::from(e)))?;

#[allow(unused_variables)]
let (change_pool, sapling_change, orchard_change) =
single_change_output_policy(&net_flows, fallback_change_pool);

let sapling_input_count = sapling
.bundle_type()
.num_spends(sapling.inputs().len())
.map_err(ChangeError::BundleError)?;
let sapling_output_count = sapling
.bundle_type()
.num_outputs(sapling.inputs().len(), sapling.outputs().len())
.map_err(ChangeError::BundleError)?;
let sapling_output_count_with_change = sapling
.bundle_type()
.num_outputs(
sapling.inputs().len(),
Expand All @@ -169,16 +199,48 @@ where

#[cfg(feature = "orchard")]
let orchard_action_count = orchard
.bundle_type()
.num_actions(orchard.inputs().len(), orchard.outputs().len())
.map_err(ChangeError::BundleError)?;
#[cfg(feature = "orchard")]
let orchard_action_count_with_change = orchard
.bundle_type()
.num_actions(
orchard.inputs().len(),
orchard.outputs().len() + _orchard_change,
orchard.outputs().len() + orchard_change,
)
.map_err(ChangeError::BundleError)?;
#[cfg(not(feature = "orchard"))]
let orchard_action_count = 0;
#[cfg(not(feature = "orchard"))]
let orchard_action_count_with_change = 0;

// Once we calculate the balance with and without change, there are five cases:
//
// 1. Insufficient funds even without change.
// 2. The fee amount without change exactly cancels out the net flow balance.
// 3. The fee amount without change is smaller than the change.
// 3a. Insufficient funds once the change output is added.
// 3b. The fee amount with change exactly cancels out the net flow balance.
// 3c. The fee amount with change leaves a non-zero change value.
//
// Case 2 happens for the second transaction of a ZIP 320 pair. In that case
// the transaction will be fully transparent, and there must be no change.
//
// If cases 2 or 3b happen for a transaction with any shielded flows, we
// want there to be a zero-value shielded change output anyway (i.e. treat
// case 2 as case 3, and case 3b as case 3c), because:
// * being able to distinguish these cases potentially leaks too much
// information (an adversary that knows the number of external recipients
// and the sum of their outputs learns the sum of the inputs if no change
// output is present); and
// * we will then always have an shielded output in which to put change_memo,
// if one is given.
//
// Note that using the `DustAction::AddDustToFee` policy inherently leaks
// more information.

let fee_amount = fee_rule
let fee_without_change = fee_rule
.fee_required(
params,
target_height,
Expand All @@ -190,58 +252,116 @@ where
)
.map_err(|fee_error| ChangeError::StrategyError(E::from(fee_error)))?;

let total_in =
(net_flows.t_in + net_flows.sapling_in + net_flows.orchard_in).ok_or_else(overflow)?;
let total_out = (net_flows.t_out + net_flows.sapling_out + net_flows.orchard_out + fee_amount)
.ok_or_else(overflow)?;
let fee_with_change = max(
fee_without_change,
fee_rule
.fee_required(
params,
target_height,
transparent_inputs.iter().map(|i| i.serialized_size()),
transparent_outputs.iter().map(|i| i.serialized_size()),
sapling_input_count,
sapling_output_count_with_change,
orchard_action_count_with_change,
)
.map_err(|fee_error| ChangeError::StrategyError(E::from(fee_error)))?,
);

let proposed_change = (total_in - total_out).ok_or(ChangeError::InsufficientFunds {
available: total_in,
required: total_out,
})?;

if proposed_change.is_zero() {
TransactionBalance::new(vec![], fee_amount).map_err(|_| overflow())
} else {
let dust_threshold = dust_output_policy
.dust_threshold()
.unwrap_or(default_dust_threshold);

if proposed_change < dust_threshold {
match dust_output_policy.action() {
DustAction::Reject => {
let shortfall = (dust_threshold - proposed_change).ok_or_else(underflow)?;

Err(ChangeError::InsufficientFunds {
available: total_in,
required: (total_in + shortfall).ok_or_else(overflow)?,
})
}
DustAction::AllowDustChange => TransactionBalance::new(
vec![ChangeValue::shielded(
change_pool,
proposed_change,
change_memo,
)],
fee_amount,
)
.map_err(|_| overflow()),
DustAction::AddDustToFee => TransactionBalance::new(
vec![],
(fee_amount + proposed_change).ok_or_else(overflow)?,
// We don't create a fully-transparent transaction if a change memo is requested.
let transparent = net_flows.is_transparent() && change_memo.is_none();

let total_out_plus_fee_without_change =
(total_out + fee_without_change).ok_or_else(overflow)?;
let total_out_plus_fee_with_change = (total_out + fee_with_change).ok_or_else(overflow)?;

let (change, fee) = {
if transparent && total_in < total_out_plus_fee_without_change {
// Case 1 for a tx with all transparent flows.
return Err(ChangeError::InsufficientFunds {
available: total_in,
required: total_out_plus_fee_without_change,
});
} else if transparent && total_in == total_out_plus_fee_without_change {
// Case 2 for a tx with all transparent flows.
(vec![], fee_without_change)
} else if total_in < total_out_plus_fee_with_change {
// Case 3a, or case 1 or 2 with non-transparent flows.
return Err(ChangeError::InsufficientFunds {
available: total_in,
required: total_out_plus_fee_with_change,
});
} else {
// Case 3b or 3c.
let proposed_change =
(total_in - total_out_plus_fee_with_change).expect("checked above");
let simple_case = |memo| {
(
vec![ChangeValue::shielded(change_pool, proposed_change, memo)],
fee_with_change,
)
.map_err(|_| overflow()),
};

let dust_threshold = dust_output_policy
.dust_threshold()
.unwrap_or(default_dust_threshold);

if proposed_change < dust_threshold {
match dust_output_policy.action() {
DustAction::Reject => {
// Always allow zero-valued change even for the `Reject` policy:
// * it should be allowed in order to record change memos and to improve
// indistinguishability;
// * this case occurs in practice when sending all funds from an account;
// * zero-valued notes do not require witness tracking;
// * the effect on trial decryption overhead is small.
if proposed_change.is_zero() {
simple_case(change_memo)
} else {
let shortfall =
(dust_threshold - proposed_change).ok_or_else(underflow)?;

return Err(ChangeError::InsufficientFunds {
available: total_in,
required: (total_in + shortfall).ok_or_else(overflow)?,
});
}
}
DustAction::AllowDustChange => simple_case(change_memo),
DustAction::AddDustToFee => {
// Zero-valued change is also always allowed for this policy, but when
// no change memo is given, we might omit the change output instead.

let fee_with_dust = (total_in - total_out)
.expect("we already checked for sufficient funds");
// We can add a change output if necessary.
assert!(fee_with_change <= fee_with_dust);

let reasonable_fee =
(fee_with_change + (MINIMUM_FEE * 10).unwrap()).ok_or_else(overflow)?;

if fee_with_dust > reasonable_fee {
// Defend against losing money by using AddDustToFee with a too-high
// dust threshold.
simple_case(change_memo)
} else if change_memo.is_some() {
(
vec![ChangeValue::shielded(
change_pool,
NonNegativeAmount::ZERO,
change_memo,
)],
fee_with_dust,
)
} else {
(vec![], fee_with_dust)
}
}
}
} else {
simple_case(change_memo)
}
} else {
TransactionBalance::new(
vec![ChangeValue::shielded(
change_pool,
proposed_change,
change_memo,
)],
fee_amount,
)
.map_err(|_| overflow())
}
}
};

TransactionBalance::new(change, fee).map_err(|_| overflow())
}

0 comments on commit 21d5731

Please sign in to comment.