Skip to content

Commit

Permalink
Remove MaybeMixedInputScripts typestate
Browse files Browse the repository at this point in the history 
The BIP78 spec says:
"If the sender's inputs are all from the same scriptPubKey type, the
receiver must match the same type. If the receiver can't match the type,
they must return error unavailable."

Instead of rejecting an original PSBT that contains mixed input scripts
types, we should be checking if all input script types are the same and
only error if the receiver input contributions would *introduce* mixed
script types.

This check is now done in `contribute_inputs` (for v1 receivers only)
and the previous mixed input scripts check is removed.
  • Loading branch information
@spacebear21
spacebear21 committed Oct 18, 2024
1 parent 38eab07 commit 0af90e6
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 122 deletions.
7 changes: 2 additions & 5 deletions payjoin-cli/src/app/v1.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,15 +319,12 @@ impl App {
}
})?;
log::trace!("check2");
// Receive Check 3: no mixed input scripts
let proposal = proposal.check_no_mixed_input_scripts()?;
log::trace!("check3");

// Receive Check 4: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
// Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
let payjoin = proposal.check_no_inputs_seen_before(|input| {
self.db.insert_input_seen_before(*input).map_err(|e| Error::Server(e.into()))
})?;
log::trace!("check4");
log::trace!("check3");

let payjoin = payjoin.identify_receiver_outputs(|output_script| {
if let Ok(address) = bitcoin::Address::from_script(output_script, network) {
Expand Down
7 changes: 2 additions & 5 deletions payjoin-cli/src/app/v2.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,15 +306,12 @@ impl App {
}
})?;
log::trace!("check2");
// Receive Check 3: no mixed input scripts
let proposal = proposal.check_no_mixed_input_scripts()?;
log::trace!("check3");

// Receive Check 4: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
// Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
let payjoin = proposal.check_no_inputs_seen_before(|input| {
self.db.insert_input_seen_before(*input).map_err(|e| Error::Server(e.into()))
})?;
log::trace!("check4");
log::trace!("check3");

let payjoin = payjoin
.identify_receiver_outputs(|output_script| {
Expand Down
27 changes: 15 additions & 12 deletions payjoin/src/receive/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,6 @@ pub(crate) enum InternalRequestError {
OriginalPsbtNotBroadcastable,
/// The sender is trying to spend the receiver input
InputOwned(bitcoin::ScriptBuf),
/// The original psbt has mixed input address types that could harm privacy
MixedInputScripts(bitcoin::AddressType, bitcoin::AddressType),
/// The address type could not be determined
AddressType(crate::psbt::AddressTypeError),
/// The expected input weight cannot be determined
InputWeight(crate::psbt::InputWeightError),
/// Original PSBT input has been seen before. Only automatic receivers, aka "interactive" in the spec
Expand Down Expand Up @@ -152,13 +148,6 @@ impl fmt::Display for RequestError {
),
InternalRequestError::InputOwned(_) =>
write_error(f, "original-psbt-rejected", "The receiver rejected the original PSBT."),
InternalRequestError::MixedInputScripts(type_a, type_b) => write_error(
f,
"original-psbt-rejected",
&format!("Mixed input scripts: {}; {}.", type_a, type_b),
),
InternalRequestError::AddressType(e) =>
write_error(f, "original-psbt-rejected", &format!("AddressType Error: {}", e)),
InternalRequestError::InputWeight(e) =>
write_error(f, "original-psbt-rejected", &format!("InputWeight Error: {}", e)),
InternalRequestError::InputSeen(_) =>
Expand Down Expand Up @@ -200,7 +189,6 @@ impl std::error::Error for RequestError {
InternalRequestError::SenderParams(e) => Some(e),
InternalRequestError::InconsistentPsbt(e) => Some(e),
InternalRequestError::PrevTxOut(e) => Some(e),
InternalRequestError::AddressType(e) => Some(e),
InternalRequestError::InputWeight(e) => Some(e),
#[cfg(feature = "v2")]
InternalRequestError::ParsePsbt(e) => Some(e),
Expand Down Expand Up @@ -303,6 +291,12 @@ pub struct InputContributionError(InternalInputContributionError);
pub(crate) enum InternalInputContributionError {
/// Missing previous txout information
PrevTxOut(crate::psbt::PrevTxOutError),
/// The address type could not be determined
AddressType(crate::psbt::AddressTypeError),
/// The original PSBT has no inputs
NoSenderInputs,
/// The proposed receiver inputs would introduce mixed input script types
MixedInputScripts(bitcoin::AddressType, bitcoin::AddressType),
/// Total input value is not enough to cover additional output value
ValueTooLow,
}
Expand All @@ -312,6 +306,15 @@ impl fmt::Display for InputContributionError {
match &self.0 {
InternalInputContributionError::PrevTxOut(e) =>
write!(f, "Missing previous txout information: {}", e),
InternalInputContributionError::AddressType(e) =>
write!(f, "The address type could not be determined: {}", e),
InternalInputContributionError::NoSenderInputs =>
write!(f, "The original PSBT has no inputs"),
InternalInputContributionError::MixedInputScripts(type_a, type_b) => write!(
f,
"The proposed receiver inputs would introduce mixed input script types: {}; {}.",
type_a, type_b
),
InternalInputContributionError::ValueTooLow =>
write!(f, "Total input value is not enough to cover additional output value"),
}
Expand Down
99 changes: 37 additions & 62 deletions payjoin/src/receive/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,12 @@ pub mod v2;

use bitcoin::secp256k1::rand::seq::SliceRandom;
use bitcoin::secp256k1::rand::{self, Rng};
pub use error::{Error, OutputSubstitutionError, RequestError, SelectionError};
pub use error::{
Error, InputContributionError, OutputSubstitutionError, RequestError, SelectionError,
};
use error::{
InputContributionError, InternalInputContributionError, InternalOutputSubstitutionError,
InternalRequestError, InternalSelectionError,
InternalInputContributionError, InternalOutputSubstitutionError, InternalRequestError,
InternalSelectionError,
};
use optional_parameters::Params;

Expand Down Expand Up @@ -180,7 +182,7 @@ impl MaybeInputsOwned {
pub fn check_inputs_not_owned(
self,
is_owned: impl Fn(&Script) -> Result<bool, Error>,
) -> Result<MaybeMixedInputScripts, Error> {
) -> Result<MaybeInputsSeen, Error> {
let mut err = Ok(());
if let Some(e) = self
.psbt
Expand All @@ -203,59 +205,6 @@ impl MaybeInputsOwned {
}
err?;

Ok(MaybeMixedInputScripts { psbt: self.psbt, params: self.params })
}
}

/// Typestate to validate that the Original PSBT has no mixed input types.
/// This check is skipped in payjoin v2.
///
/// Call [`Self::check_no_mixed_input_scripts`] to proceed.
#[derive(Clone)]
pub struct MaybeMixedInputScripts {
psbt: Psbt,
params: Params,
}

impl MaybeMixedInputScripts {
/// Verify the original transaction did not have mixed input types
/// Call this after checking downstream.
///
/// Note: mixed spends do not necessarily indicate distinct wallet fingerprints.
/// This check is intended to prevent some types of wallet fingerprinting.
pub fn check_no_mixed_input_scripts(self) -> Result<MaybeInputsSeen, RequestError> {
// Allow mixed input scripts in payjoin v2
if self.params.v == 2 {
return Ok(MaybeInputsSeen { psbt: self.psbt, params: self.params });
}

let mut err = Ok(());
let input_scripts = self
.psbt
.input_pairs()
.scan(&mut err, |err, input| match input.address_type() {
Ok(address_type) => Some(address_type),
Err(e) => {
**err = Err(RequestError::from(InternalRequestError::AddressType(e)));
None
}
})
.collect::<Vec<_>>();
err?;

if let Some(first) = input_scripts.first() {
input_scripts.iter().try_for_each(|input_type| {
if input_type != first {
Err(RequestError::from(InternalRequestError::MixedInputScripts(
*first,
*input_type,
)))
} else {
Ok(())
}
})?;
}

Ok(MaybeInputsSeen { psbt: self.psbt, params: self.params })
}
}
Expand Down Expand Up @@ -598,12 +547,24 @@ impl WantsInputs {
.first()
.map(|input| input.sequence)
.unwrap_or_default();
let (sender_has_mixed_inputs, sender_input_type) = self.sender_mixed_inputs()?;

// Insert contributions at random indices for privacy
let mut rng = rand::thread_rng();
let mut receiver_input_amount = Amount::ZERO;
for (psbtin, txin) in inputs.into_iter() {
receiver_input_amount += InputPair { txin: &txin, psbtin: &psbtin }
let input_pair = InputPair { txin: &txin, psbtin: &psbtin };
let input_type =
input_pair.address_type().map_err(InternalInputContributionError::AddressType)?;
// The payjoin proposal must not introduce mixed input script types (in v1 only)
if self.params.v == 1 && !sender_has_mixed_inputs && input_type != sender_input_type {
return Err(InternalInputContributionError::MixedInputScripts(
input_type,
sender_input_type,
)
.into());
}
receiver_input_amount += input_pair
.previous_txout()
.map_err(InternalInputContributionError::PrevTxOut)?
.value;
Expand Down Expand Up @@ -632,6 +593,24 @@ impl WantsInputs {
})
}

fn sender_mixed_inputs(&self) -> Result<(bool, bitcoin::AddressType), InputContributionError> {
let mut sender_inputs = self.original_psbt.input_pairs();
let first_input_type = sender_inputs
.next()
.ok_or(InternalInputContributionError::NoSenderInputs)?
.address_type()
.map_err(InternalInputContributionError::AddressType)?;
let mut sender_has_mixed_inputs = false;
for input in sender_inputs {
if input.address_type().map_err(InternalInputContributionError::AddressType)?
!= first_input_type
{
sender_has_mixed_inputs = true;
}
}
Ok((sender_has_mixed_inputs, first_input_type))
}

// Compute the minimum amount that the receiver must contribute to the transaction as input
fn receiver_min_input_amount(&self) -> Amount {
let output_amount = self
Expand Down Expand Up @@ -942,8 +921,6 @@ mod test {
.assume_interactive_receiver()
.check_inputs_not_owned(|_| Ok(false))
.expect("No inputs should be owned")
.check_no_mixed_input_scripts()
.expect("No mixed input scripts")
.check_no_inputs_seen_before(|_| Ok(false))
.expect("No inputs should be seen before")
.identify_receiver_outputs(|script| {
Expand Down Expand Up @@ -977,8 +954,6 @@ mod test {
.assume_interactive_receiver()
.check_inputs_not_owned(|_| Ok(false))
.expect("No inputs should be owned")
.check_no_mixed_input_scripts()
.expect("No mixed input scripts")
.check_no_inputs_seen_before(|_| Ok(false))
.expect("No inputs should be seen before")
.identify_receiver_outputs(|script| {
Expand Down
23 changes: 1 addition & 22 deletions payjoin/src/receive/v2/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -325,29 +325,8 @@ impl MaybeInputsOwned {
pub fn check_inputs_not_owned(
self,
is_owned: impl Fn(&Script) -> Result<bool, Error>,
) -> Result<MaybeMixedInputScripts, Error> {
) -> Result<MaybeInputsSeen, Error> {
let inner = self.inner.check_inputs_not_owned(is_owned)?;
Ok(MaybeMixedInputScripts { inner, context: self.context })
}
}

/// Typestate to validate that the Original PSBT has no mixed input types.
///
/// Call [`check_no_mixed_input_types`](struct.UncheckedProposal.html#method.check_no_mixed_input_scripts) to proceed.
#[derive(Clone)]
pub struct MaybeMixedInputScripts {
inner: super::MaybeMixedInputScripts,
context: SessionContext,
}

impl MaybeMixedInputScripts {
/// Verify the original transaction did not have mixed input types
/// Call this after checking downstream.
///
/// Note: mixed spends do not necessarily indicate distinct wallet fingerprints.
/// This check is intended to prevent some types of wallet fingerprinting.
pub fn check_no_mixed_input_scripts(self) -> Result<MaybeInputsSeen, RequestError> {
let inner = self.inner.check_no_mixed_input_scripts()?;
Ok(MaybeInputsSeen { inner, context: self.context })
}
}
Expand Down
23 changes: 7 additions & 16 deletions payjoin/tests/integration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,14 +164,11 @@ mod integration {

// **********************
// Inside the Receiver:
// this data would transit from one party to another over the network in production
let response = handle_v1_pj_request(req, headers, &receiver, None, None, None);
// this response would be returned as http response to the sender

// **********************
// Inside the Sender:
// Sender checks error due to mixed input scripts
assert!(ctx.process_response(&mut response.as_bytes()).is_err());
// This should error because the receiver is attempting to introduce mixed input script types
let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
handle_v1_pj_request(req, headers, &receiver, None, None, None)
}));
assert!(result.is_err());
Ok(())
}
}
Expand Down Expand Up @@ -860,10 +857,7 @@ mod integration {
})
.expect("Receiver should not own any of the inputs");

// Receive Check 3: no mixed input scripts
let proposal = proposal.check_no_mixed_input_scripts().unwrap();

// Receive Check 4: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
// Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
let payjoin = proposal
.check_no_inputs_seen_before(|_| Ok(false))
.unwrap()
Expand Down Expand Up @@ -1300,10 +1294,7 @@ mod integration {
})
.expect("Receiver should not own any of the inputs");

// Receive Check 3: no mixed input scripts
let proposal = proposal.check_no_mixed_input_scripts().unwrap();

// Receive Check 4: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
// Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
let payjoin = proposal
.check_no_inputs_seen_before(|_| Ok(false))
.unwrap()
Expand Down

0 comments on commit 0af90e6

Please sign in to comment.