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

Complete receive payjoin feature groundwork #18

Merged
merged 5 commits into from
Jan 13, 2023
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
2 changes: 1 addition & 1 deletion payjoin-client/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ fn main() {
let psbt = client.wallet_process_psbt(&psbt, None, None, None).unwrap().psbt;
let psbt = load_psbt_from_base64(psbt.as_bytes()).unwrap();
println!("Original psbt: {:#?}", psbt);
let pj_params = payjoin::sender::Params::with_fee_contribution(
let pj_params = payjoin::sender::Configuration::with_fee_contribution(
payjoin::bitcoin::Amount::from_sat(10000),
None,
);
Expand Down
2 changes: 1 addition & 1 deletion payjoin/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ base64 = "0.13.0"
rand = { version = "0.8.4", optional = true }
bip21 = "0.2.0"
url = "2.2.2"
log = { version = "0.4.14"}

[dev-dependencies]
bitcoind = { version = "0.27.1", features = ["0_21_1"] }
env_logger = "0.9.0"
log = "0.4.14"

[package.metadata.docs.rs]
features = ["sender"]
1 change: 1 addition & 0 deletions payjoin/src/receiver/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub(crate) enum InternalRequestError {
InvalidContentType(String),
InvalidContentLength(std::num::ParseIntError),
ContentLengthTooLarge(u64),
SenderParams(super::optional_parameters::Error),
}

impl From<InternalRequestError> for RequestError {
Expand Down
40 changes: 28 additions & 12 deletions payjoin/src/receiver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,34 @@ use bitcoin::util::psbt::PartiallySignedTransaction as Psbt;
use bitcoin::{AddressType, Script, TxOut};

mod error;
mod optional_parameters;

use error::InternalRequestError;
pub use error::RequestError;
use optional_parameters::Params;

pub trait Headers {
fn get_header(&self, key: &str) -> Option<&str>;
}

pub struct UncheckedProposal {
psbt: Psbt,
params: Params,
}

pub struct MaybeInputsOwned {
psbt: Psbt,
params: Params,
}

pub struct MaybeMixedInputScripts {
psbt: Psbt,
params: Params,
}

pub struct MaybeInputsSeen {
psbt: Psbt,
params: Params,
}

impl UncheckedProposal {
Expand Down Expand Up @@ -55,7 +61,12 @@ impl UncheckedProposal {
let mut reader = base64::read::DecoderReader::new(&mut limited, base64::STANDARD);
let psbt = Psbt::consensus_decode(&mut reader).map_err(InternalRequestError::Decode)?;

Ok(UncheckedProposal { psbt })
let pairs = url::form_urlencoded::parse(query.as_bytes());
let params = Params::from_query_pairs(pairs).map_err(InternalRequestError::SenderParams)?;

// TODO check that params are valid for the request's Original PSBT

Ok(UncheckedProposal { psbt, params })
}

/// The Sender's Original PSBT
Expand All @@ -76,7 +87,7 @@ impl UncheckedProposal {
///
/// Call this after checking downstream.
pub fn assume_tested_and_scheduled_broadcast(self) -> MaybeInputsOwned {
MaybeInputsOwned { psbt: self.psbt }
MaybeInputsOwned { psbt: self.psbt, params: self.params }
}

/// Call this method if the only way to initiate a PayJoin with this receiver
Expand All @@ -85,7 +96,7 @@ impl UncheckedProposal {
/// So-called "non-interactive" receivers, like payment processors, that allow arbitrary requests are otherwise vulnerable to probing attacks.
/// Those receivers call `get_transaction_to_check_broadcast()` and `attest_tested_and_scheduled_broadcast()` after making those checks downstream.
pub fn assume_interactive_receive_endpoint(self) -> MaybeInputsOwned {
MaybeInputsOwned { psbt: self.psbt }
MaybeInputsOwned { psbt: self.psbt, params: self.params }
}
}

Expand All @@ -103,7 +114,7 @@ impl MaybeInputsOwned {
/// An attacker could try to spend receiver's own inputs. This check prevents that.
/// Call this after checking downstream.
pub fn assume_inputs_not_owned(self) -> MaybeMixedInputScripts {
MaybeMixedInputScripts { psbt: self.psbt }
MaybeMixedInputScripts { psbt: self.psbt, params: self.params }
}
}

Expand All @@ -122,7 +133,7 @@ impl MaybeMixedInputScripts {
/// Note: mixed spends do not necessarily indicate distinct wallet fingerprints.
/// This check is intended to prevent some types of wallet fingerprinting.
pub fn assume_no_mixed_input_scripts(self) -> MaybeInputsSeen {
MaybeInputsSeen { psbt: self.psbt }
MaybeInputsSeen { psbt: self.psbt, params: self.params }
}
}

Expand All @@ -140,30 +151,31 @@ impl MaybeInputsSeen {
///
/// Call this after checking downstream.
pub fn assume_no_inputs_seen_before(self) -> UnlockedProposal {
UnlockedProposal { psbt: self.psbt }
UnlockedProposal { psbt: self.psbt, params: self.params }
}
}

pub struct UnlockedProposal {
psbt: Psbt,
params: Params,
}

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

pub fn assume_locked(self) -> Proposal { Proposal { psbt: self.psbt } }
pub fn psbt(self) -> Psbt { self.psbt }

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

/// Transaction that must be broadcasted.
#[must_use = "The transaction must be broadcasted to prevent abuse"]
pub struct MustBroadcast(pub bitcoin::Transaction);

pub struct Proposal {
psbt: Psbt,
}

/*
impl Proposal {
pub fn replace_output_script(&mut self, new_output_script: Script, options: NewOutputOptions) -> Result<Self, OutputError> {
Expand Down Expand Up @@ -226,7 +238,11 @@ mod test {

let body = original_psbt.as_bytes();
let headers = MockHeaders::new(body.len() as u64);
UncheckedProposal::from_request(body, "", headers)
UncheckedProposal::from_request(
body,
"?maxadditionalfeecontribution=0.00000182?additionalfeeoutputindex=0",
headers,
)
}

#[test]
Expand Down
118 changes: 118 additions & 0 deletions payjoin/src/receiver/optional_parameters.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
use std::borrow::Borrow;
use std::fmt;

use log::warn;

use crate::fee_rate::FeeRate;

#[derive(Debug)]
pub(crate) struct Params {
// version
// v: usize,
// disableoutputsubstitution
pub disable_output_substitution: bool,
// maxadditionalfeecontribution, additionalfeeoutputindex
pub additional_fee_contribution: Option<(bitcoin::Amount, usize)>,
// minfeerate
pub min_feerate: FeeRate,
}

impl Default for Params {
fn default() -> Self {
Params {
disable_output_substitution: false,
additional_fee_contribution: None,
min_feerate: FeeRate::ZERO,
DanGould marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

impl Params {
#[cfg(feature = "receiver")]
pub fn from_query_pairs<K, V, I>(pairs: I) -> Result<Self, Error>
where
I: Iterator<Item = (K, V)>,
K: Borrow<str> + Into<String>,
V: Borrow<str> + Into<String>,
{
let mut params = Params::default();

let mut additional_fee_output_index = None;
let mut max_additional_fee_contribution = None;

for (k, v) in pairs {
match (k.borrow(), v.borrow()) {
("v", v) =>
if v != "1" {
return Err(Error::UnknownVersion);
},
("additionalfeeoutputindex", index) =>
additional_fee_output_index = match index.parse::<usize>() {
Ok(index) => Some(index),
Err(_error) => {
warn!(
"bad `additionalfeeoutputindex` query value '{}': {}",
index, _error
);
None
}
},
("maxadditionalfeecontribution", fee) =>
max_additional_fee_contribution =
match bitcoin::Amount::from_str_in(&fee, bitcoin::Denomination::Bitcoin) {
Ok(contribution) => Some(contribution),
Err(_error) => {
warn!(
"bad `maxadditionalfeecontribution` query value '{}': {}",
fee, _error
);
None
}
},
("minfeerate", feerate) =>
params.min_feerate = match feerate.parse::<u64>() {
Ok(rate) => FeeRate::from_sat_per_vb(rate),
Err(e) => return Err(Error::FeeRate(e)),
},
("disableoutputsubstitution", v) =>
params.disable_output_substitution = v == "true",
_ => (),
}
}

match (max_additional_fee_contribution, additional_fee_output_index) {
(Some(amount), Some(index)) =>
params.additional_fee_contribution = Some((amount, index)),
(Some(_), None) | (None, Some(_)) => {
warn!("only one additional-fee parameter specified: {:?}", params);
}
_ => (),
}

Ok(params)
}
}

#[derive(Debug)]
pub(crate) enum Error {
UnknownVersion,
FeeRate(std::num::ParseIntError),
}

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Error::UnknownVersion => write!(f, "unknown version"),
Error::FeeRate(_) => write!(f, "could not parse feerate"),
}
}
}

impl std::error::Error for Error {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
match self {
Error::FeeRate(error) => Some(error),
_ => None,
}
}
}
12 changes: 6 additions & 6 deletions payjoin/src/sender/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ type InternalResult<T> = Result<T, InternalValidationError>;
/// Builder for sender-side payjoin parameters
///
/// These parameters define how client wants to handle PayJoin.
pub struct Params {
pub struct Configuration {
disable_output_substitution: bool,
fee_contribution: Option<(bitcoin::Amount, Option<usize>)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any value putting params here as a field?

Copy link
Contributor Author

@DanGould DanGould Jan 12, 2023

Choose a reason for hiding this comment

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

If this configuration is going to hold this data then yes, I think it makes sense to be part of this. The idea of a builder to expose this interface in an accessible way rather than expose optional_parameters::Params as pub makes sense to me.

I'm starting to think of this Configuration struct more as a sender::RequestBuilder that can turn into a (Request, Context) when you call from_psbt_and_uri. Maybe the signature for that method should be

impl RequestBuilder {
    fn create_pj_request(psbt: Psbt, pj_url: Uri<PayJoin>) -> Result<(Request, Context)>;
}

instead of on the manually-validated Uri<PayJoinParams> how it is now: pj_url.create_pj_request(psbt, pj_params: Configuration) -> Result<(Request, Context)>.

Then create_pj_request could validate the endpoint check_pj_supported() during that call instead of requiring it be done by hand before the call, too.

Copy link
Contributor Author

@DanGould DanGould Jan 12, 2023

Choose a reason for hiding this comment

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

The builder could have .psbt(), pj_uri(), .build() methods in addition to the existing ones like .non_incentivizing() to more closely resemble a builder pattern, too

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, builder pattern kinda makes sense. We might need more than one to implement type state, since both URI and PSBT are mandatory. Alternatively we could just accept both in a single method.

clamp_fee_contribution: bool,
min_fee_rate: FeeRate,
}

impl Params {
impl Configuration {
/// Offer the receiver contribution to pay for his input.
///
/// These parameters will allow the receiver to take `max_fee_contribution` from given change
Expand All @@ -57,7 +57,7 @@ impl Params {
max_fee_contribution: bitcoin::Amount,
change_index: Option<usize>,
) -> Self {
Params {
Configuration {
disable_output_substitution: false,
fee_contribution: Some((max_fee_contribution, change_index)),
clamp_fee_contribution: false,
Expand All @@ -70,7 +70,7 @@ impl Params {
/// While it's generally better to offer some contribution some users may wish not to.
/// This function disables contribution.
pub fn non_incentivizing() -> Self {
Params {
Configuration {
disable_output_substitution: false,
fee_contribution: None,
clamp_fee_contribution: false,
Expand Down Expand Up @@ -539,7 +539,7 @@ fn check_change_index(
fn determine_fee_contribution(
psbt: &Psbt,
payee: &Script,
params: &Params,
params: &Configuration,
) -> Result<Option<(bitcoin::Amount, usize)>, InternalCreateRequestError> {
Ok(match params.fee_contribution {
Some((fee, None)) => find_change_index(psbt, payee, fee, params.clamp_fee_contribution)?,
Expand Down Expand Up @@ -583,7 +583,7 @@ fn serialize_psbt(psbt: &Psbt) -> Vec<u8> {
pub(crate) fn from_psbt_and_uri(
mut psbt: Psbt,
uri: crate::uri::PjUri<'_>,
params: Params,
params: Configuration,
) -> Result<(Request, Context), CreateRequestError> {
psbt.validate_input_utxos(true).map_err(InternalCreateRequestError::InvalidOriginalInput)?;
let disable_output_substitution =
Expand Down
4 changes: 2 additions & 2 deletions payjoin/src/uri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub trait PjUriExt: sealed::UriExt {
fn create_pj_request(
self,
psbt: bitcoin::util::psbt::PartiallySignedTransaction,
params: sender::Params,
params: sender::Configuration,
) -> Result<(sender::Request, sender::Context), sender::CreateRequestError>;
}

Expand All @@ -55,7 +55,7 @@ impl<'a> PjUriExt for PjUri<'a> {
fn create_pj_request(
self,
psbt: bitcoin::util::psbt::PartiallySignedTransaction,
params: sender::Params,
params: sender::Configuration,
) -> Result<(sender::Request, sender::Context), sender::CreateRequestError> {
sender::from_psbt_and_uri(
psbt.try_into()
Expand Down
11 changes: 7 additions & 4 deletions payjoin/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,17 +73,20 @@ mod integration {
let psbt = sender.wallet_process_psbt(&psbt, None, None, None).unwrap().psbt;
let psbt = load_psbt_from_base64(psbt.as_bytes()).unwrap();
debug!("Original psbt: {:#?}", psbt);
let pj_params = payjoin::sender::Params::with_fee_contribution(
let pj_params = payjoin::sender::Configuration::with_fee_contribution(
payjoin::bitcoin::Amount::from_sat(10000),
None,
);
let (req, ctx) = pj_uri.create_pj_request(psbt, pj_params).unwrap();
let headers = HeaderMock::from_vec(&req.body);

// Receiver receive payjoin proposal, IRL it will be an HTTP request (over ssl or onion)
let _proposal =
payjoin::receiver::UncheckedProposal::from_request(req.body.as_slice(), "", headers)
.unwrap();
let proposal = payjoin::receiver::UncheckedProposal::from_request(
req.body.as_slice(),
req.url.query().unwrap_or(""),
headers,
)
.unwrap();

// TODO
}
Expand Down