Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 5 additions & 2 deletions payjoin-cli/src/app/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,11 @@ impl App {
let pj_part = payjoin::Url::parse(pj_part)
.map_err(|e| anyhow!("Failed to parse pj_endpoint: {}", e))?;

let mut pj_uri =
payjoin::receive::v1::build_v1_pj_uri(&pj_receiver_address, &pj_part, false)?;
let mut pj_uri = payjoin::receive::v1::build_v1_pj_uri(
&pj_receiver_address,
&pj_part,
payjoin::OutputSubstitution::Enabled,
)?;
pj_uri.amount = Some(amount);

Ok(pj_uri.to_string())
Expand Down
5 changes: 4 additions & 1 deletion payjoin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,11 @@ mod request;
#[cfg(feature = "_core")]
pub use request::*;
#[cfg(feature = "_core")]
pub(crate) mod output_substitution;
#[cfg(feature = "v1")]
pub use output_substitution::OutputSubstitution;
#[cfg(feature = "_core")]
mod uri;

#[cfg(feature = "_core")]
pub use into_url::{Error as IntoUrlError, IntoUrl};
#[cfg(feature = "_core")]
Expand Down
20 changes: 20 additions & 0 deletions payjoin/src/output_substitution.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/// Whether the receiver is allowed to substitute original outputs or not.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[cfg_attr(feature = "v2", derive(serde::Serialize, serde::Deserialize))]
pub enum OutputSubstitution {
Enabled,
Disabled,
}

impl OutputSubstitution {
/// Combine two output substitution flags.
///
/// If both are enabled, the result is enabled.
/// If one is disabled, the result is disabled.
pub(crate) fn combine(self, other: Self) -> Self {
match (self, other) {
(Self::Enabled, Self::Enabled) => Self::Enabled,
_ => Self::Disabled,
}
}
}
12 changes: 9 additions & 3 deletions payjoin/src/receive/optional_parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ use std::fmt;
use bitcoin::FeeRate;
use log::warn;

use crate::output_substitution::OutputSubstitution;

#[derive(Debug, Clone)]
pub(crate) struct Params {
// version
pub v: usize,
// disableoutputsubstitution
pub disable_output_substitution: bool,
pub output_substitution: OutputSubstitution,
// maxadditionalfeecontribution, additionalfeeoutputindex
pub additional_fee_contribution: Option<(bitcoin::Amount, usize)>,
// minfeerate
Expand All @@ -23,7 +25,7 @@ impl Default for Params {
fn default() -> Self {
Params {
v: 1,
disable_output_substitution: false,
output_substitution: OutputSubstitution::Enabled,
additional_fee_contribution: None,
min_fee_rate: FeeRate::BROADCAST_MIN,
#[cfg(feature = "_multiparty")]
Expand Down Expand Up @@ -88,7 +90,11 @@ impl Params {
Err(_) => return Err(Error::FeeRate),
},
("disableoutputsubstitution", v) =>
params.disable_output_substitution = v == "true",
params.output_substitution = if v == "true" {
OutputSubstitution::Disabled
} else {
OutputSubstitution::Enabled
},
#[cfg(feature = "_multiparty")]
("optimisticmerge", v) => params.optimistic_merge = v == "true",
_ => (),
Expand Down
5 changes: 2 additions & 3 deletions payjoin/src/receive/v1/exclusive/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@ pub trait Headers {
pub fn build_v1_pj_uri<'a>(
address: &bitcoin::Address,
endpoint: impl IntoUrl,
disable_output_substitution: bool,
output_substitution: OutputSubstitution,
) -> Result<crate::uri::PjUri<'a>, crate::into_url::Error> {
let extras =
crate::uri::PayjoinExtras { endpoint: endpoint.into_url()?, disable_output_substitution };
let extras = crate::uri::PayjoinExtras { endpoint: endpoint.into_url()?, output_substitution };
Ok(bitcoin_uri::Uri::with_extras(address.clone(), extras))
}

Expand Down
20 changes: 7 additions & 13 deletions payjoin/src/receive/v1/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ use super::optional_parameters::Params;
use super::{
ImplementationError, InputPair, OutputSubstitutionError, ReplyableError, SelectionError,
};
use crate::output_substitution::OutputSubstitution;
use crate::psbt::PsbtExt;
use crate::receive::InternalPayloadError;

Expand Down Expand Up @@ -264,9 +265,8 @@ pub struct WantsOutputs {
}

impl WantsOutputs {
pub fn is_output_substitution_disabled(&self) -> bool {
self.params.disable_output_substitution
}
/// Whether the receiver is allowed to substitute original outputs or not.
pub fn output_substitution(&self) -> OutputSubstitution { self.params.output_substitution }

/// Substitute the receiver output script with the provided script.
pub fn substitute_receiver_script(
Expand Down Expand Up @@ -306,7 +306,7 @@ impl WantsOutputs {
// Select an output with the same address if one was provided
Some(pos) => {
let txo = replacement_outputs.swap_remove(pos);
if self.params.disable_output_substitution
if self.output_substitution() == OutputSubstitution::Disabled
&& txo.value < original_output.value
{
return Err(
Expand All @@ -317,7 +317,7 @@ impl WantsOutputs {
}
// Otherwise randomly select one of the replacement outputs
None => {
if self.params.disable_output_substitution {
if self.output_substitution() == OutputSubstitution::Disabled {
return Err(
InternalOutputSubstitutionError::ScriptPubKeyChangedWhenDisabled
.into(),
Expand Down Expand Up @@ -708,7 +708,7 @@ impl ProvisionalProposal {
self.payjoin_psbt.inputs[i].tap_key_sig = None;
}

PayjoinProposal { payjoin_psbt: self.payjoin_psbt, params: self.params }
PayjoinProposal { payjoin_psbt: self.payjoin_psbt }
}

/// Return the indexes of the sender inputs
Expand Down Expand Up @@ -763,18 +763,13 @@ impl ProvisionalProposal {
#[derive(Debug, Clone)]
pub struct PayjoinProposal {
payjoin_psbt: Psbt,
params: Params,
}

impl PayjoinProposal {
pub fn utxos_to_be_locked(&self) -> impl '_ + Iterator<Item = &bitcoin::OutPoint> {
self.payjoin_psbt.unsigned_tx.input.iter().map(|input| &input.previous_output)
}

pub fn is_output_substitution_disabled(&self) -> bool {
self.params.disable_output_substitution
}

pub fn psbt(&self) -> &Psbt { &self.payjoin_psbt }
}

Expand Down Expand Up @@ -949,8 +944,7 @@ pub(crate) mod test {
#[test]
fn test_pjos_disabled() {
let mut proposal = proposal_from_test_vector().unwrap();
// Specify outputsubstitution is disabled
proposal.params.disable_output_substitution = true;
proposal.params.output_substitution = OutputSubstitution::Disabled;
let wants_outputs = wants_outputs_from_test_vector(proposal).unwrap();

let output_value =
Expand Down
17 changes: 7 additions & 10 deletions payjoin/src/receive/v2/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use super::{
};
use crate::hpke::{decrypt_message_a, encrypt_message_b, HpkeKeyPair, HpkePublicKey};
use crate::ohttp::{ohttp_decapsulate, ohttp_encapsulate, OhttpEncapsulationError, OhttpKeys};
use crate::output_substitution::OutputSubstitution;
use crate::receive::{parse_payload, InputPair};
use crate::uri::ShortId;
use crate::{IntoUrl, IntoUrlError, Request};
Expand Down Expand Up @@ -192,7 +193,7 @@ impl Receiver {
//
// see: https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#unsecured-payjoin-server
if params.v == 1 {
params.disable_output_substitution = true;
params.output_substitution = OutputSubstitution::Disabled;

// Additionally V1 sessions never have an optimistic merge opportunity
#[cfg(feature = "_multiparty")]
Expand All @@ -212,7 +213,8 @@ impl Receiver {
pj.set_receiver_pubkey(self.context.s.public_key().clone());
pj.set_ohttp(self.context.ohttp_keys.clone());
pj.set_exp(self.context.expiry);
let extras = PayjoinExtras { endpoint: pj, disable_output_substitution: false };
let extras =
PayjoinExtras { endpoint: pj, output_substitution: OutputSubstitution::Enabled };
bitcoin_uri::Uri::with_extras(self.context.address.clone(), extras)
}

Expand Down Expand Up @@ -385,9 +387,8 @@ pub struct WantsOutputs {
}

impl WantsOutputs {
pub fn is_output_substitution_disabled(&self) -> bool {
self.v1.is_output_substitution_disabled()
}
/// Whether the receiver is allowed to substitute original outputs or not.
pub fn output_substitution(&self) -> OutputSubstitution { self.v1.output_substitution() }
Comment on lines +390 to +391
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't remember why this is public on WantsOutputs and PayjoinProposal but maybe it doesn't need to be? It's checked in some tests but I don't know why a user would need it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

WantsOutputs is should be better to handle an OutputSubstitutionError::OutputSubstitutionDisabled variant, assuming that variant, agreed. I'm not sure why it would be on PayjoinProposal.

Similarly, I'm confused why there are multiple OutputSubstitutionDisabled(&str) variants instead of distinct variants. This part is going to become a new issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, though if they're removed, the OutputSubstitution Enum again only makes sense to expose publicly within the context of receive::v1::exclusive

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A v1 receiver on an unsecured server MUST disallow output substitution, but that doesn't preclude a v2 receiver disabling it for other reasons?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A v2 receiver is always going to serialize pjos=0 to prevent A v1 sender from trying output substitution over the directory in plaintext messages, but a v2 sender may use disableoutputsubstitution Params to prevent the receiver from doing so. A receiver is really responding to preferences of the sender in v2 because they're always using an authenticated protocol.


/// Substitute the receiver output script with the provided script.
pub fn substitute_receiver_script(
Expand Down Expand Up @@ -503,10 +504,6 @@ impl PayjoinProposal {
self.v1.utxos_to_be_locked()
}

pub fn is_output_substitution_disabled(&self) -> bool {
self.v1.is_output_substitution_disabled()
}

pub fn psbt(&self) -> &Psbt { self.v1.psbt() }

pub fn extract_v2_req(
Expand Down Expand Up @@ -652,6 +649,6 @@ mod test {
fn test_v2_pj_uri() {
let uri = Receiver { context: SHARED_CONTEXT.clone() }.pj_uri();
assert_ne!(uri.extras.endpoint, EXAMPLE_URL.clone());
assert!(!uri.extras.disable_output_substitution);
assert_eq!(uri.extras.output_substitution, OutputSubstitution::Enabled);
}
}
28 changes: 21 additions & 7 deletions payjoin/src/send/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub use error::{BuildSenderError, ResponseError, ValidationError, WellKnownError
pub(crate) use error::{InternalBuildSenderError, InternalProposalError, InternalValidationError};
use url::Url;

use crate::output_substitution::OutputSubstitution;
use crate::psbt::PsbtExt;

// See usize casts
Expand Down Expand Up @@ -51,7 +52,7 @@ pub(crate) struct AdditionalFeeContribution {
#[derive(Debug, Clone)]
pub struct PsbtContext {
original_psbt: Psbt,
disable_output_substitution: bool,
output_substitution: OutputSubstitution,
fee_contribution: Option<AdditionalFeeContribution>,
min_fee_rate: FeeRate,
payee: ScriptBuf,
Expand Down Expand Up @@ -253,7 +254,7 @@ impl PsbtContext {
if original_output.script_pubkey == self.payee =>
{
ensure!(
!self.disable_output_substitution
self.output_substitution == OutputSubstitution::Enabled
|| (proposed_txout.script_pubkey == original_output.script_pubkey
&& proposed_txout.value >= original_output.value),
DisallowedOutputSubstitution
Expand Down Expand Up @@ -414,14 +415,14 @@ fn determine_fee_contribution(

fn serialize_url(
endpoint: Url,
disable_output_substitution: bool,
output_substitution: OutputSubstitution,
fee_contribution: Option<AdditionalFeeContribution>,
min_fee_rate: FeeRate,
version: &str,
) -> Url {
let mut url = endpoint;
url.query_pairs_mut().append_pair("v", version);
if disable_output_substitution {
if output_substitution == OutputSubstitution::Disabled {
url.query_pairs_mut().append_pair("disableoutputsubstitution", "true");
}
if let Some(AdditionalFeeContribution { max_amount, vout }) = fee_contribution {
Expand Down Expand Up @@ -449,14 +450,15 @@ mod test {
use super::{
check_single_payee, clear_unneeded_fields, determine_fee_contribution, serialize_url,
};
use crate::output_substitution::OutputSubstitution;
use crate::psbt::PsbtExt;
use crate::send::{AdditionalFeeContribution, InternalBuildSenderError, InternalProposalError};

pub(crate) fn create_psbt_context() -> Result<super::PsbtContext, BoxError> {
let payee = PARSED_ORIGINAL_PSBT.unsigned_tx.output[1].script_pubkey.clone();
Ok(super::PsbtContext {
original_psbt: PARSED_ORIGINAL_PSBT.clone(),
disable_output_substitution: false,
output_substitution: OutputSubstitution::Enabled,
fee_contribution: Some(AdditionalFeeContribution {
max_amount: bitcoin::Amount::from_sat(182),
vout: 0,
Expand Down Expand Up @@ -641,10 +643,22 @@ mod test {

#[test]
fn test_disable_output_substitution_query_param() -> Result<(), BoxError> {
let url = serialize_url(Url::parse("http://localhost")?, true, None, FeeRate::ZERO, "2");
let url = serialize_url(
Url::parse("http://localhost")?,
OutputSubstitution::Disabled,
None,
FeeRate::ZERO,
"2",
);
assert_eq!(url, Url::parse("http://localhost?v=2&disableoutputsubstitution=true")?);

let url = serialize_url(Url::parse("http://localhost")?, false, None, FeeRate::ZERO, "2");
let url = serialize_url(
Url::parse("http://localhost")?,
OutputSubstitution::Enabled,
None,
FeeRate::ZERO,
"2",
);
assert_eq!(url, Url::parse("http://localhost?v=2")?);
Ok(())
}
Expand Down
Loading