Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for transparent-source-only (TEX) addresses #1257

Merged
merged 56 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
2fae4bb
ZIP 320 implementation.
daira Jun 22, 2024
0f3de63
Apply documentation suggestions from code review.
daira Jun 22, 2024
549fe0b
In `reserve_next_n_ephemeral_addresses`, exclude addresses observed in
daira Jun 22, 2024
eb88461
Address review comment: `EphemeralIvk` should not implement `Incoming…
daira Jun 22, 2024
c6520cf
Change the protobuf schema to explicitly specify whether a `ChangeValue`
daira Jun 22, 2024
2f521d7
If a change memo is supplied, it should not be used in the second step
daira Jun 23, 2024
0f49dae
`mark_ephemeral_address_as_mined` now prefers setting `mined_in_tx` to
daira Jun 23, 2024
637ae92
Add a migration test for the `ephemeral_addresses` migration.
daira Jun 23, 2024
994f6ff
Change type of `n` in `reserve_next_n_ephemeral_addresses`.
daira Jun 24, 2024
25f07da
Add a constraint on the range of `ephemeral_addresses(address_index)`.
daira Jun 24, 2024
e164b59
Move most ephemeral address index handling into helper functions in
daira Jun 24, 2024
914acb5
Move most remaining code for wallet support of ephemeral addresses into
daira Jun 24, 2024
745054b
`find_account_for_transparent_output` now searches unreserved ephemeral
daira Jun 24, 2024
6b465b7
Document the mapping functions on `zcash_client_backend::wallet::Reci…
daira Jun 24, 2024
4f43a01
Refactor transparent address metadata lookups. This is correct as-is but
daira Jun 25, 2024
5a90fff
Factor out the conversion of the `diversifier_index_be` field in the
daira Jun 25, 2024
7fb3557
Implement `WalletRead::get_transparent_address_metadata` for
daira Jun 25, 2024
7d8a96f
Don't cache metadata between steps; it's not an important optimization.
daira Jun 25, 2024
bd6c9f3
Apply documentation suggestions from code review.
daira Jun 26, 2024
0735390
Rename `amount` to `transfer_amount` in `send_multi_step_proposed_tra…
daira Jun 26, 2024
c926e7c
Filter ephemeral transparent `ChangeValue`s by `is_ephemeral()` as we…
daira Jun 26, 2024
ffb2ddf
`zcash_client_backend::fees::ChangeValue` is now an enum, allowing only
daira Jun 26, 2024
b778139
Refactor ephemeral output-related parameters to balance calculation.
daira Jun 26, 2024
7a05b44
Make mutable inputs to closures in `create_proposed_transaction` expl…
daira Jun 26, 2024
6471d4c
Don't assume that prior step outputs are ephemeral iff they are
daira Jun 27, 2024
ec4a6d0
Documentation improvements.
daira Jun 27, 2024
81a2846
Simpler way to calculate `has_shielded_inputs`.
daira Jun 27, 2024
f0e5aab
Improve discrimination of proposal errors.
daira Jun 27, 2024
feabe6d
Remove complicated code to calculate the number of dust spends.
daira Jun 26, 2024
baccb43
Restore the logic to determine whether we are spending inputs that are
daira Jun 28, 2024
9c082dc
`zcash_primitives::transaction::fees::zip317::FeeRule::non_standard` has
daira Jun 28, 2024
3829663
Change note selection query to select notes > 5000 zats, not >= 5000 …
daira Jun 28, 2024
25006ab
Documentation improvement from code review.
daira Jun 28, 2024
3922d71
Change the type of `n` in `reserve_next_n_ephemeral_addresses` back t…
daira Jun 28, 2024
d32b7db
Remove `ChangeValue::new`. Also document `ChangeValue::is_ephemeral` as
daira Jun 28, 2024
286439a
Define a constant `EphemeralParameters::NONE` instead of deriving `De…
daira Jun 29, 2024
bc38f2a
Document the `possible_change` parameter to `check_for_uneconomic_inp…
daira Jun 29, 2024
7838c04
Make `ephemeral_parameters` and `EphemeralParameters::NONE` available
daira Jun 29, 2024
14bdcde
We cannot spend prior outputs at all when "transparent-inputs" is not
daira Jun 29, 2024
8636daa
Tiny simplification.
daira Jun 29, 2024
01ff201
Minor changes responding to review comments.
daira Jul 3, 2024
b63ff5b
Rename `get_reserved_ephemeral_addresses` to `get_known_ephemeral_add…
daira Jul 3, 2024
e97da43
Refactoring to address review comments.
daira Jul 3, 2024
a01588b
Ensure that `mark_ephemeral_address_as_mined` correctly handles indices
daira Jul 4, 2024
86428c4
Refactor `find_account_for_transparent_output` (now called
daira Jul 4, 2024
b48f627
Minor simplification in a test.
daira Jul 4, 2024
6bc22f4
Documentation fixes and improvements.
daira Jul 4, 2024
27ca6e4
Remove unneeded `impl SealedChangeLevelKey for EphemeralIvk`.
daira Jul 4, 2024
bda6451
Change `unwrap`s to `expect`s when constructing `NonHardenedChildIndex`.
daira Jul 4, 2024
22b8cff
The `TxId` argument to `EphemeralAddressReuse` does not need to be op…
daira Jul 4, 2024
9856a70
Simpler handling of a potential overflow.
daira Jul 4, 2024
56aa348
Extend the `send_multi_step_proposed_transfer` test to check the beha…
daira Jul 4, 2024
aa43123
Use `EphemeralBalance` instead of `EphemeralParameters`
nuttycom Jul 16, 2024
dbb5eeb
Fix a potential crash related to varying behavior between change stra…
nuttycom Jul 16, 2024
24b6d50
Apply suggestions from code review
nuttycom Jul 16, 2024
f8bedd8
Make ephemeral_addresses.address unique
nuttycom Jul 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions zcash_client_backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail
- `chain::BlockCache` trait, behind the `sync` feature flag.
- `WalletRead::get_spendable_transparent_outputs`.
- `zcash_client_backend::fees`:
- `EphemeralBalance`
- `ChangeValue::shielded, is_ephemeral`
- `ChangeValue::ephemeral_transparent` (when "transparent-inputs" is enabled)
- `sapling::EmptyBundleView`
Expand Down Expand Up @@ -67,10 +68,10 @@ funds to those addresses. See [ZIP 320](https://zips.z.cash/zip-0320) for detail
return type of `ChangeValue::output_pool` has (unconditionally) changed
from `ShieldedProtocol` to `zcash_protocol::PoolType`.
- `ChangeStrategy::compute_balance`: this trait method has an additional
`&EphemeralParameters` parameter. If the "transparent-inputs" feature is
`Option<&EphemeralBalance>` parameter. If the "transparent-inputs" feature is
enabled, this can be used to specify whether the change memo should be
ignored, and the amounts of additional transparent P2PKH inputs and
outputs. Passing `&EphemeralParameters::NONE` will retain the previous
outputs. Passing `None` will retain the previous
behaviour (and is necessary when the "transparent-inputs" feature is
not enabled).
- `zcash_client_backend::input_selection::GreedyInputSelectorError` has a
Expand Down
27 changes: 13 additions & 14 deletions zcash_client_backend/src/data_api/wallet/input_selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
use crate::{
address::{Address, UnifiedAddress},
data_api::{InputSource, SimpleNoteRetention, SpendableNotes},
fees::{sapling, ChangeError, ChangeStrategy, DustOutputPolicy, EphemeralParameters},
fees::{sapling, ChangeError, ChangeStrategy, DustOutputPolicy},
proposal::{Proposal, ProposalError, ShieldedInputs},
wallet::WalletTransparentOutput,
zip321::TransactionRequest,
Expand All @@ -33,7 +33,7 @@
#[cfg(feature = "transparent-inputs")]
use {
crate::{
fees::ChangeValue,
fees::{ChangeValue, EphemeralBalance},
proposal::{Step, StepOutput, StepOutputIndex},
zip321::Payment,
},
Expand Down Expand Up @@ -460,12 +460,12 @@
}

#[cfg(not(feature = "transparent-inputs"))]
let ephemeral_parameters = EphemeralParameters::NONE;
let ephemeral_balance = None;

Check warning on line 463 in zcash_client_backend/src/data_api/wallet/input_selection.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api/wallet/input_selection.rs#L463

Added line #L463 was not covered by tests

#[cfg(feature = "transparent-inputs")]
let (ephemeral_parameters, tr1_balance_opt) = {
let (ephemeral_balance, tr1_balance_opt) = {
if tr1_transparent_outputs.is_empty() {
(EphemeralParameters::NONE, None)
(None, None)
} else {
// The ephemeral input going into transaction 1 must be able to pay that
// transaction's fee, as well as the TEX address payments.
Expand All @@ -484,11 +484,11 @@
#[cfg(feature = "orchard")]
&orchard_fees::EmptyBundleView,
&self.dust_output_policy,
&EphemeralParameters::new(true, Some(NonNegativeAmount::ZERO), None),
Some(&EphemeralBalance::Input(NonNegativeAmount::ZERO)),
) {
Err(ChangeError::InsufficientFunds { required, .. }) => required,
Ok(_) => NonNegativeAmount::ZERO, // shouldn't happen
Err(other) => return Err(other.into()),

Check warning on line 491 in zcash_client_backend/src/data_api/wallet/input_selection.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api/wallet/input_selection.rs#L490-L491

Added lines #L490 - L491 were not covered by tests
};

// Now recompute to obtain the `TransactionBalance` and verify that it
Expand All @@ -502,12 +502,12 @@
#[cfg(feature = "orchard")]
&orchard_fees::EmptyBundleView,
&self.dust_output_policy,
&EphemeralParameters::new(true, Some(tr1_required_input_value), None),
Some(&EphemeralBalance::Input(tr1_required_input_value)),
)?;
assert_eq!(tr1_balance.total(), tr1_balance.fee_required());

(
EphemeralParameters::new(false, None, Some(tr1_required_input_value)),
Some(EphemeralBalance::Output(tr1_required_input_value)),
Some(tr1_balance),
)
}
Expand Down Expand Up @@ -582,7 +582,7 @@
&orchard_outputs[..],
),
&self.dust_output_policy,
&ephemeral_parameters,
ephemeral_balance.as_ref(),
);

match balance {
Expand All @@ -598,7 +598,7 @@
}))
.map(|notes| ShieldedInputs::from_parts(anchor_height, notes));

#[cfg(feature = "transparent-inputs")]

Check warning on line 601 in zcash_client_backend/src/data_api/wallet/input_selection.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api/wallet/input_selection.rs#L601

Added line #L601 was not covered by tests
if let Some(tr1_balance) = tr1_balance_opt {
// Construct two new `TransactionRequest`s:
// * `tr0` excludes the TEX outputs, and in their place includes
Expand All @@ -609,8 +609,8 @@
assert_eq!(
*balance.proposed_change().last().expect("nonempty"),
ChangeValue::ephemeral_transparent(
ephemeral_parameters
.ephemeral_output_amount()
ephemeral_balance
.and_then(|b| b.ephemeral_output_amount())
.expect("ephemeral output is present")
)
);
Expand Down Expand Up @@ -773,7 +773,7 @@
#[cfg(feature = "orchard")]
&orchard_fees::EmptyBundleView,
&self.dust_output_policy,
&EphemeralParameters::NONE,
None,
);

let balance = match trial_balance {
Expand All @@ -786,13 +786,12 @@
params,
target_height,
&transparent_inputs,
&[] as &[TxOut],

Check warning on line 789 in zcash_client_backend/src/data_api/wallet/input_selection.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api/wallet/input_selection.rs#L789

Added line #L789 was not covered by tests
&sapling::EmptyBundleView,
#[cfg(feature = "orchard")]
&orchard_fees::EmptyBundleView,
&self.dust_output_policy,
#[cfg(feature = "transparent-inputs")]
&EphemeralParameters::NONE,
None,

Check warning on line 794 in zcash_client_backend/src/data_api/wallet/input_selection.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/data_api/wallet/input_selection.rs#L794

Added line #L794 was not covered by tests
)?
}
Err(other) => {
Expand Down
69 changes: 22 additions & 47 deletions zcash_client_backend/src/fees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,61 +330,36 @@
}
}

/// `EphemeralParameters` can be used to specify variations on how balance
/// and fees are computed that are relevant to transactions using ephemeral
/// transparent outputs. These are only relevant when the "transparent-inputs"
/// feature is enabled, but to reduce feature-dependent boilerplate, the type
/// and the `EphemeralParameters::NONE` constant are present unconditionally.
/// `EphemeralBalance` describes the ephemeral input or output value for
/// a transaction. It is use in the computation of fees are relevant to transactions using
nuttycom marked this conversation as resolved.
Show resolved Hide resolved
/// ephemeral transparent outputs.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct EphemeralParameters {
ignore_change_memo: bool,
ephemeral_input_amount: Option<NonNegativeAmount>,
ephemeral_output_amount: Option<NonNegativeAmount>,
pub enum EphemeralBalance {
Input(NonNegativeAmount),
Output(NonNegativeAmount),
}

impl EphemeralParameters {
/// An `EphemeralParameters` indicating no use of ephemeral inputs
/// or outputs. It has:
///
/// * `ignore_change_memo: false`,
/// * `ephemeral_input_amount: None`,
/// * `ephemeral_output_amount: None`.
pub const NONE: Self = Self {
ignore_change_memo: false,
ephemeral_input_amount: None,
ephemeral_output_amount: None,
};

/// Returns an `EphemeralParameters` with the following parameters:
///
/// * `ignore_change_memo`: `true` if the change memo should be
/// ignored for the purpose of deciding whether there will be a
/// change output.
/// * `ephemeral_input_amount`: specifies that there will be an
/// additional P2PKH input of the given amount.
/// * `ephemeral_output_amount`: specifies that there will be an
/// additional P2PKH output of the given amount.
#[cfg(feature = "transparent-inputs")]
pub const fn new(
ignore_change_memo: bool,
ephemeral_input_amount: Option<NonNegativeAmount>,
ephemeral_output_amount: Option<NonNegativeAmount>,
) -> Self {
Self {
ignore_change_memo,
ephemeral_input_amount,
ephemeral_output_amount,
}
impl EphemeralBalance {
pub fn is_input(&self) -> bool {
matches!(self, EphemeralBalance::Input(_))
}

pub fn ignore_change_memo(&self) -> bool {
self.ignore_change_memo
pub fn is_output(&self) -> bool {
matches!(self, EphemeralBalance::Output(_))

Check warning on line 348 in zcash_client_backend/src/fees.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees.rs#L347-L348

Added lines #L347 - L348 were not covered by tests
}

pub fn ephemeral_input_amount(&self) -> Option<NonNegativeAmount> {
self.ephemeral_input_amount
match self {
EphemeralBalance::Input(v) => Some(*v),
EphemeralBalance::Output(_) => None,
}
}

pub fn ephemeral_output_amount(&self) -> Option<NonNegativeAmount> {
self.ephemeral_output_amount
match self {
EphemeralBalance::Input(_) => None,
EphemeralBalance::Output(v) => Some(*v),
}
}
}

Expand Down Expand Up @@ -433,7 +408,7 @@
sapling: &impl sapling::BundleView<NoteRefT>,
#[cfg(feature = "orchard")] orchard: &impl orchard::BundleView<NoteRefT>,
dust_output_policy: &DustOutputPolicy,
ephemeral_parameters: &EphemeralParameters,
ephemeral_balance: Option<&EphemeralBalance>,
) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>>;
}

Expand Down
41 changes: 20 additions & 21 deletions zcash_client_backend/src/fees/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

use super::{
sapling as sapling_fees, ChangeError, ChangeValue, DustAction, DustOutputPolicy,
EphemeralParameters, TransactionBalance,
EphemeralBalance, TransactionBalance,
};

#[cfg(feature = "orchard")]
Expand Down Expand Up @@ -49,8 +49,7 @@
transparent_outputs: &[impl transparent::OutputView],
sapling: &impl sapling_fees::BundleView<NoteRefT>,
#[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
ephemeral_input_amount: Option<NonNegativeAmount>,
ephemeral_output_amount: Option<NonNegativeAmount>,
ephemeral_balance: Option<&EphemeralBalance>,
) -> Result<NetFlows, ChangeError<E, NoteRefT>>
where
E: From<F::Error> + From<BalanceError>,
Expand All @@ -60,13 +59,13 @@
let t_in = transparent_inputs
.iter()
.map(|t_in| t_in.coin().value)
.chain(ephemeral_input_amount)
.chain(ephemeral_balance.and_then(|b| b.ephemeral_input_amount()))
.sum::<Option<_>>()
.ok_or_else(overflow)?;
let t_out = transparent_outputs
.iter()
.map(|t_out| t_out.value())
.chain(ephemeral_output_amount)
.chain(ephemeral_balance.and_then(|b| b.ephemeral_output_amount()))
.sum::<Option<_>>()
.ok_or_else(overflow)?;
let sapling_in = sapling
Expand Down Expand Up @@ -162,12 +161,15 @@
fallback_change_pool: ShieldedProtocol,
marginal_fee: NonNegativeAmount,
grace_actions: usize,
ephemeral_parameters: &EphemeralParameters,
ephemeral_balance: Option<&EphemeralBalance>,
) -> Result<TransactionBalance, ChangeError<E, NoteRefT>>
where
E: From<F::Error> + From<BalanceError>,
{
let change_memo = change_memo.filter(|_| !ephemeral_parameters.ignore_change_memo());
// The change memo, if any, must be attached to the change in the intermediate step that
// produces the ephemeral output, and so it should be discarded in the ultimate step; this is
// distinguished by identifying that this transaction has ephemeral inputs.
let change_memo = change_memo.filter(|_| ephemeral_balance.map_or(true, |b| !b.is_input()));

let overflow = || ChangeError::StrategyError(E::from(BalanceError::Overflow));
let underflow = || ChangeError::StrategyError(E::from(BalanceError::Underflow));
Expand All @@ -178,8 +180,7 @@
sapling,
#[cfg(feature = "orchard")]
orchard,
ephemeral_parameters.ephemeral_input_amount(),
ephemeral_parameters.ephemeral_output_amount(),
ephemeral_balance,
)?;

#[allow(unused_variables)]
Expand Down Expand Up @@ -213,7 +214,7 @@
marginal_fee,
grace_actions,
&possible_change[..],
ephemeral_parameters,
ephemeral_balance,
)?;
}

Expand Down Expand Up @@ -287,16 +288,16 @@
.iter()
.map(|i| i.serialized_size())
.chain(
ephemeral_parameters
.ephemeral_input_amount()
ephemeral_balance
.and_then(|b| b.ephemeral_input_amount())
.map(|_| transparent::InputSize::STANDARD_P2PKH),
);
let transparent_output_sizes = transparent_outputs
.iter()
.map(|i| i.serialized_size())
.chain(
ephemeral_parameters
.ephemeral_output_amount()
ephemeral_balance
.and_then(|b| b.ephemeral_output_amount())
.map(|_| P2PKH_STANDARD_OUTPUT_SIZE),
);

Expand Down Expand Up @@ -388,7 +389,7 @@
});
}
}
DustAction::AllowDustChange => simple_case(),

Check warning on line 392 in zcash_client_backend/src/fees/common.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/common.rs#L392

Added line #L392 was not covered by tests
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.
Expand All @@ -404,13 +405,13 @@
if fee_with_dust > reasonable_fee {
// Defend against losing money by using AddDustToFee with a too-high
// dust threshold.
simple_case()

Check warning on line 408 in zcash_client_backend/src/fees/common.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/common.rs#L408

Added line #L408 was not covered by tests
} else if change_memo.is_some() {
(
vec![ChangeValue::shielded(
change_pool,
NonNegativeAmount::ZERO,
change_memo.cloned(),

Check warning on line 414 in zcash_client_backend/src/fees/common.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/common.rs#L414

Added line #L414 was not covered by tests
)],
fee_with_dust,
)
Expand All @@ -426,8 +427,8 @@
};
#[cfg(feature = "transparent-inputs")]
change.extend(
ephemeral_parameters
.ephemeral_output_amount()
ephemeral_balance
.and_then(|b| b.ephemeral_output_amount())
.map(ChangeValue::ephemeral_transparent),
);

Expand Down Expand Up @@ -456,7 +457,7 @@
marginal_fee: NonNegativeAmount,
grace_actions: usize,
possible_change: &[(usize, usize, usize)],
daira marked this conversation as resolved.
Show resolved Hide resolved
ephemeral_parameters: &EphemeralParameters,
ephemeral_balance: Option<&EphemeralBalance>,
) -> Result<(), ChangeError<E, NoteRefT>> {
let mut t_dust: Vec<_> = transparent_inputs
.iter()
Expand All @@ -464,7 +465,7 @@
// For now, we're just assuming P2PKH inputs, so we don't check the
// size of the input script.
if i.coin().value <= marginal_fee {
Some(i.outpoint().clone())

Check warning on line 468 in zcash_client_backend/src/fees/common.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/common.rs#L468

Added line #L468 was not covered by tests
} else {
None
}
Expand Down Expand Up @@ -504,10 +505,8 @@
}

let (t_inputs_len, t_outputs_len) = (
transparent_inputs.len()
+ usize::from(ephemeral_parameters.ephemeral_input_amount().is_some()),
transparent_outputs.len()
+ usize::from(ephemeral_parameters.ephemeral_output_amount().is_some()),
transparent_inputs.len() + usize::from(ephemeral_balance.map_or(false, |b| b.is_input())),
transparent_outputs.len() + usize::from(ephemeral_balance.map_or(false, |b| b.is_output())),
);
let (s_inputs_len, s_outputs_len) = (sapling.inputs().len(), sapling.outputs().len());
#[cfg(feature = "orchard")]
Expand Down Expand Up @@ -598,14 +597,14 @@

let (t_extra, s_extra, o_extra) = if baseline >= grace_actions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be refactored into something that doesn't involve such code complexity, or at least extracted into a function that can be unit tested?

Copy link
Contributor Author

@daira daira Jul 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really possible to make it much less complex, I think. If we want to unit-test allowed_dust and/or hypothetical_actions, we can easily pull them out into standalone functions then (note that they close over a few variables, which means you end up with some boilerplate argument passing by doing that). None of this is public API surface, so we don't need to do that in the absence of an actual test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately what we probably want to do here is, instead of checking for dust inputs that exceed the allowed limit and returning an error, treat dust inputs separately and simply prune to what we can consume in unused input slots given the available logical actions. We know the minimum number of logical actions going in to input selection, so we can just take the largest dust values and spend them.

(0, 0, 0)
} else if t_dust.len() > t_allowed && hypothetical_actions(1, 0, 0)? <= baseline {
(1, 0, 0)
} else if s_dust.len() > s_allowed && hypothetical_actions(0, 1, 0)? <= baseline {
(0, 1, 0)
} else if o_dust.len() > o_allowed && hypothetical_actions(0, 0, 1)? <= baseline {
(0, 0, 1)

Check warning on line 605 in zcash_client_backend/src/fees/common.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/common.rs#L600-L605

Added lines #L600 - L605 were not covered by tests
} else {
(0, 0, 0)

Check warning on line 607 in zcash_client_backend/src/fees/common.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/fees/common.rs#L607

Added line #L607 was not covered by tests
};
Ok((
t_allowed + t_extra,
Expand Down
12 changes: 6 additions & 6 deletions zcash_client_backend/src/fees/fixed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::ShieldedProtocol;

use super::{
common::single_change_output_balance, sapling as sapling_fees, ChangeError, ChangeStrategy,
DustOutputPolicy, EphemeralParameters, TransactionBalance,
DustOutputPolicy, EphemeralBalance, TransactionBalance,
};

#[cfg(feature = "orchard")]
Expand Down Expand Up @@ -63,7 +63,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
sapling: &impl sapling_fees::BundleView<NoteRefT>,
#[cfg(feature = "orchard")] orchard: &impl orchard_fees::BundleView<NoteRefT>,
dust_output_policy: &DustOutputPolicy,
ephemeral_parameters: &EphemeralParameters,
ephemeral_balance: Option<&EphemeralBalance>,
) -> Result<TransactionBalance, ChangeError<Self::Error, NoteRefT>> {
single_change_output_balance(
params,
Expand All @@ -80,7 +80,7 @@ impl ChangeStrategy for SingleOutputChangeStrategy {
self.fallback_change_pool,
NonNegativeAmount::ZERO,
0,
ephemeral_parameters,
ephemeral_balance,
)
}
}
Expand All @@ -100,7 +100,7 @@ mod tests {
data_api::wallet::input_selection::SaplingPayment,
fees::{
tests::{TestSaplingInput, TestTransparentInput},
ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy, EphemeralParameters,
ChangeError, ChangeStrategy, ChangeValue, DustOutputPolicy,
},
ShieldedProtocol,
};
Expand Down Expand Up @@ -136,7 +136,7 @@ mod tests {
#[cfg(feature = "orchard")]
&orchard_fees::EmptyBundleView,
&DustOutputPolicy::default(),
&EphemeralParameters::NONE,
None,
);

assert_matches!(
Expand Down Expand Up @@ -182,7 +182,7 @@ mod tests {
#[cfg(feature = "orchard")]
&orchard_fees::EmptyBundleView,
&DustOutputPolicy::default(),
&EphemeralParameters::NONE,
None,
);

assert_matches!(
Expand Down
Loading
Loading