Skip to content

Commit

Permalink
Fix a potential crash related to varying behavior between change stra…
Browse files Browse the repository at this point in the history
…tegies.

It was possible for `GreedyInputSelector` to crash if the change
strategy being used for input selection were to place a ZIP 320
ephemeral output anywhere but as the last element in the returned change
values. This has been replaced by an error; the error will also be
returned if the change strategy returns more than one ephemeral output
in the change values.
  • Loading branch information
nuttycom committed Jul 16, 2024
1 parent aa43123 commit dbb5eeb
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 14 deletions.
43 changes: 29 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 @@ -33,7 +33,7 @@ use crate::{
#[cfg(feature = "transparent-inputs")]
use {
crate::{
fees::{ChangeValue, EphemeralBalance},
fees::EphemeralBalance,
proposal::{Step, StepOutput, StepOutputIndex},
zip321::Payment,
},
Expand Down Expand Up @@ -328,6 +328,11 @@ pub struct GreedyInputSelector<DbT, ChangeT> {
impl<DbT, ChangeT: ChangeStrategy> GreedyInputSelector<DbT, ChangeT> {
/// Constructs a new greedy input selector that uses the provided change strategy to determine
/// change values and fee amounts.
///
/// The [`ChangeStrategy`] provided must produce exactly one ephemeral change value when
/// computing a transaction balance if an [`EphemeralBalance::Output`] value is provided for
/// its ephemeral balance, or the resulting [`GreedyInputSelector`] will return an error when
/// attempting to construct a transaction proposal that requires such an output.
pub fn new(change_strategy: ChangeT, dust_output_policy: DustOutputPolicy) -> Self {
GreedyInputSelector {
change_strategy,
Expand Down Expand Up @@ -605,19 +610,29 @@ where
// a single additional ephemeral output to the transparent pool.
// * `tr1` spends from that ephemeral output to each TEX output.

// The ephemeral output should always be at the last change index.
assert_eq!(
*balance.proposed_change().last().expect("nonempty"),
ChangeValue::ephemeral_transparent(
ephemeral_balance
.and_then(|b| b.ephemeral_output_amount())
.expect("ephemeral output is present")
)
);
let ephemeral_stepoutput = StepOutput::new(
0,
StepOutputIndex::Change(balance.proposed_change().len() - 1),
);
// Find exactly one ephemeral change output.
let ephemeral_outputs = balance
.proposed_change()
.iter()
.enumerate()
.filter(|(_, c)| c.is_ephemeral())
.collect::<Vec<_>>();

let ephemeral_value = ephemeral_balance
.and_then(|b| b.ephemeral_output_amount())
.expect("ephemeral output balance exists (constructed above)");

let ephemeral_output_index = match &ephemeral_outputs[..] {
[(i, change_value)] if change_value.value() == ephemeral_value => {
Ok(*i)
}
_ => Err(InputSelectorError::Proposal(
ProposalError::EphemeralOutputsInvalid,

Check warning on line 630 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#L629-L630

Added lines #L629 - L630 were not covered by tests
)),
}?;

let ephemeral_stepoutput =
StepOutput::new(0, StepOutputIndex::Change(ephemeral_output_index));

let tr0 = TransactionRequest::from_indexed(
transaction_request
Expand Down
9 changes: 9 additions & 0 deletions zcash_client_backend/src/proposal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ pub enum ProposalError {
/// The proposal included a payment to a TEX address and a spend from a shielded input in the same step.
#[cfg(feature = "transparent-inputs")]
PaysTexFromShielded,
/// The change strategy provided to input selection failed to correctly generate an ephemeral
/// change output when needed for sending to a TEX address.
#[cfg(feature = "transparent-inputs")]
EphemeralOutputsInvalid,
}

impl Display for ProposalError {
Expand Down Expand Up @@ -114,6 +118,11 @@ impl Display for ProposalError {
f,

Check warning on line 118 in zcash_client_backend/src/proposal.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/proposal.rs#L117-L118

Added lines #L117 - L118 were not covered by tests
"The proposal included a payment to a TEX address and a spend from a shielded input in the same step.",
),
#[cfg(feature = "transparent-inputs")]
ProposalError::EphemeralOutputsInvalid => write!(
f,

Check warning on line 123 in zcash_client_backend/src/proposal.rs

View check run for this annotation

Codecov / codecov/patch

zcash_client_backend/src/proposal.rs#L122-L123

Added lines #L122 - L123 were not covered by tests
"The change strategy provided to input selection failed to correctly generate an ephemeral change output when needed for sending to a TEX address."
),
}
}
}
Expand Down

0 comments on commit dbb5eeb

Please sign in to comment.