payjoin/src/send/mod.rs:288:24: replace match guard with true
creates a mutation on this code block
|
fn check_outputs(&self, proposal: &Psbt) -> InternalResult<Amount> { |
|
let mut original_outputs = |
|
self.original_psbt.unsigned_tx.output.iter().enumerate().peekable(); |
|
let mut contributed_fee = Amount::ZERO; |
|
|
|
for (proposed_txout, proposed_psbtout) in |
|
proposal.unsigned_tx.output.iter().zip(&proposal.outputs) |
|
{ |
|
ensure( |
|
proposed_psbtout.bip32_derivation.is_empty(), |
|
InternalProposalError::TxOutContainsKeyPaths, |
|
)?; |
|
match (original_outputs.peek(), self.fee_contribution) { |
|
// fee output |
|
( |
|
Some((original_output_index, original_output)), |
|
Some(AdditionalFeeContribution { |
|
max_amount: max_fee_contrib, |
|
vout: fee_contrib_idx, |
|
}), |
|
) if proposed_txout.script_pubkey == original_output.script_pubkey |
|
&& *original_output_index == fee_contrib_idx => |
|
{ |
|
if proposed_txout.value < original_output.value { |
|
contributed_fee = original_output.value - proposed_txout.value; |
|
ensure( |
|
contributed_fee <= max_fee_contrib, |
|
InternalProposalError::FeeContributionExceedsMaximum, |
|
)?; |
|
// The remaining fee checks are done in later in `check_fees` |
|
} |
|
original_outputs.next(); |
|
} |
|
// payee output |
|
(Some((_original_output_index, original_output)), _) |
|
if original_output.script_pubkey == self.payee => |
|
{ |
|
ensure( |
|
self.output_substitution == OutputSubstitution::Enabled |
|
|| (proposed_txout.script_pubkey == original_output.script_pubkey |
|
&& proposed_txout.value >= original_output.value), |
|
InternalProposalError::DisallowedOutputSubstitution, |
|
)?; |
|
original_outputs.next(); |
|
} |
|
// our output |
|
(Some((_original_output_index, original_output)), _) |
|
if proposed_txout.script_pubkey == original_output.script_pubkey => |
|
{ |
|
ensure( |
|
proposed_txout.value >= original_output.value, |
|
InternalProposalError::OutputValueDecreased, |
|
)?; |
|
original_outputs.next(); |
|
} |
|
// additional output |
|
_ => (), |
|
} |
|
} |
|
|
|
ensure(original_outputs.peek().is_none(), InternalProposalError::MissingOrShuffledOutputs)?; |
|
Ok(contributed_fee) |
|
} |
Namely creating this mutation.
(Some((_original_output_index, original_output)), _)
if true =>
{
ensure(
proposed_txout.value >= original_output.value,
InternalProposalError::OutputValueDecreased,
)?;
original_outputs.next();
}
The issue arises that this mutation is only caught sometimes by the test integration::batching::receiver_forwards_payment
https://github.com/payjoin/rust-payjoin/blob/master/payjoin/tests/integration.rs#L1208
It is able to catch the mutation when the receiver output "Our output" is early in the iteration but if it occurs last of the 3 outputs in this test it is seemingly skipped and the test passes with this mutation incorrectly.
N.B. This is my understanding of how this match block works, It is possible I am misunderstanding the logic as the non-deterministic nature is causing some confusion.
I have 2 questions about the approach trying to solve this mutation.
- Can we force some deterministic result to prevent this mutation being inconsistent, possibly in some new unit-test?
- Is there some simple assert I am missing in the integration test to catch this change.