From 94d72d517df2badc714f7e8647b495b9532881ef Mon Sep 17 00:00:00 2001 From: spacebear Date: Tue, 15 Oct 2024 22:46:57 -0400 Subject: [PATCH 1/9] Use PSBT input pairs for input contributions Previously an "input" consisted of a (OutPoint, TxOut) tuple, which limits the caller to only contributing witness inputs. Instead, redefine an input as a (psbt::Input, TxIn) tuple, which is more flexible as it allows for any necessary information to be included in the PSBT input. Two PSBT input fields we are particularly interested in are `non_witness_utxo` and `redeem_script`, which enable legacy inputs and nested segwit (p2wpkh-in-p2sh) respectively. `contribute_witness_inputs` is renamed to `contribute_inputs`. --- payjoin-cli/src/app/mod.rs | 23 +++++++++++++ payjoin-cli/src/app/v1.rs | 9 ++--- payjoin-cli/src/app/v2.rs | 9 ++--- payjoin/src/receive/error.rs | 4 +++ payjoin/src/receive/mod.rs | 61 ++++++++++++--------------------- payjoin/src/receive/v2/mod.rs | 10 +++--- payjoin/tests/integration.rs | 63 ++++++++++++++++++----------------- 7 files changed, 91 insertions(+), 88 deletions(-) diff --git a/payjoin-cli/src/app/mod.rs b/payjoin-cli/src/app/mod.rs index 73cb0dd2..5d8c000b 100644 --- a/payjoin-cli/src/app/mod.rs +++ b/payjoin-cli/src/app/mod.rs @@ -2,6 +2,8 @@ use std::collections::HashMap; use std::str::FromStr; use anyhow::{anyhow, Context, Result}; +use bitcoin::psbt::Input as PsbtInput; +use bitcoin::TxIn; use bitcoincore_rpc::bitcoin::Amount; use bitcoincore_rpc::RpcApi; use payjoin::bitcoin::psbt::Psbt; @@ -120,3 +122,24 @@ fn read_local_cert() -> Result> { local_cert_path.push(LOCAL_CERT_FILE); Ok(std::fs::read(local_cert_path)?) } + +pub fn input_pair_from_list_unspent( + utxo: &bitcoincore_rpc::bitcoincore_rpc_json::ListUnspentResultEntry, +) -> (PsbtInput, TxIn) { + let psbtin = PsbtInput { + // NOTE: non_witness_utxo is not necessary because bitcoin-cli always supplies + // witness_utxo, even for non-witness inputs + witness_utxo: Some(bitcoin::TxOut { + value: utxo.amount, + script_pubkey: utxo.script_pub_key.clone(), + }), + redeem_script: utxo.redeem_script.clone(), + witness_script: utxo.witness_script.clone(), + ..Default::default() + }; + let txin = TxIn { + previous_output: bitcoin::OutPoint { txid: utxo.txid, vout: utxo.vout }, + ..Default::default() + }; + (psbtin, txin) +} diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index ca8093b7..8ae62f43 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -21,7 +21,7 @@ use tokio::net::TcpListener; use super::config::AppConfig; use super::App as AppTrait; -use crate::app::http_agent; +use crate::app::{http_agent, input_pair_from_list_unspent}; use crate::db::Database; #[cfg(feature = "danger-local-https")] pub const LOCAL_CERT_FILE: &str = "localhost.der"; @@ -396,13 +396,10 @@ fn try_contributing_inputs( .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout) .context("This shouldn't happen. Failed to retrieve the privacy preserving utxo from those we provided to the seclector.")?; log::debug!("selected utxo: {:#?}", selected_utxo); - let txo_to_contribute = bitcoin::TxOut { - value: selected_utxo.amount, - script_pubkey: selected_utxo.script_pub_key.clone(), - }; + let input_pair = input_pair_from_list_unspent(selected_utxo); Ok(payjoin - .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .contribute_inputs(vec![input_pair]) .expect("This shouldn't happen. Failed to contribute inputs.") .commit_inputs()) } diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index 0969769b..8dd1f0da 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -15,7 +15,7 @@ use tokio::sync::watch; use super::config::AppConfig; use super::App as AppTrait; -use crate::app::http_agent; +use crate::app::{http_agent, input_pair_from_list_unspent}; use crate::db::Database; #[derive(Clone)] @@ -375,13 +375,10 @@ fn try_contributing_inputs( .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout) .context("This shouldn't happen. Failed to retrieve the privacy preserving utxo from those we provided to the seclector.")?; log::debug!("selected utxo: {:#?}", selected_utxo); - let txo_to_contribute = bitcoin::TxOut { - value: selected_utxo.amount, - script_pubkey: selected_utxo.script_pub_key.clone(), - }; + let input_pair = input_pair_from_list_unspent(selected_utxo); Ok(payjoin - .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .contribute_inputs(vec![input_pair]) .expect("This shouldn't happen. Failed to contribute inputs.") .commit_inputs()) } diff --git a/payjoin/src/receive/error.rs b/payjoin/src/receive/error.rs index f1ba34d4..1cfc01f5 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -301,6 +301,8 @@ pub struct InputContributionError(InternalInputContributionError); #[derive(Debug)] pub(crate) enum InternalInputContributionError { + /// Missing previous txout information + PrevTxOut(crate::psbt::PrevTxOutError), /// Total input value is not enough to cover additional output value ValueTooLow, } @@ -308,6 +310,8 @@ pub(crate) enum InternalInputContributionError { impl fmt::Display for InputContributionError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match &self.0 { + InternalInputContributionError::PrevTxOut(e) => + write!(f, "Missing previous txout information: {}", e), InternalInputContributionError::ValueTooLow => write!(f, "Total input value is not enough to cover additional output value"), } diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 8fe524bc..b4ee2050 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -28,8 +28,8 @@ use std::cmp::{max, min}; use bitcoin::base64::prelude::BASE64_STANDARD; use bitcoin::base64::Engine; -use bitcoin::psbt::Psbt; -use bitcoin::{Amount, FeeRate, OutPoint, Script, TxOut, Weight}; +use bitcoin::psbt::{Input as PsbtInput, Psbt}; +use bitcoin::{Amount, FeeRate, OutPoint, Script, TxIn, TxOut, Weight}; mod error; mod optional_parameters; @@ -45,7 +45,7 @@ use error::{ }; use optional_parameters::Params; -use crate::psbt::PsbtExt; +use crate::psbt::{InputPair, PsbtExt}; pub trait Headers { fn get_header(&self, key: &str) -> Option<&str>; @@ -575,14 +575,14 @@ impl WantsInputs { /// Add the provided list of inputs to the transaction. /// Any excess input amount is added to the change_vout output indicated previously. - pub fn contribute_witness_inputs( + pub fn contribute_inputs( self, - inputs: impl IntoIterator, + inputs: impl IntoIterator, ) -> Result { let mut payjoin_psbt = self.payjoin_psbt.clone(); // The payjoin proposal must not introduce mixed input sequence numbers let original_sequence = self - .payjoin_psbt + .original_psbt .unsigned_tx .input .first() @@ -592,21 +592,17 @@ impl WantsInputs { // Insert contributions at random indices for privacy let mut rng = rand::thread_rng(); let mut receiver_input_amount = Amount::ZERO; - for (outpoint, txo) in inputs.into_iter() { - receiver_input_amount += txo.value; + for (psbtin, txin) in inputs.into_iter() { + receiver_input_amount += InputPair { txin: &txin, psbtin: &psbtin } + .previous_txout() + .map_err(InternalInputContributionError::PrevTxOut)? + .value; let index = rng.gen_range(0..=self.payjoin_psbt.unsigned_tx.input.len()); - payjoin_psbt.inputs.insert( - index, - bitcoin::psbt::Input { witness_utxo: Some(txo), ..Default::default() }, - ); - payjoin_psbt.unsigned_tx.input.insert( - index, - bitcoin::TxIn { - previous_output: outpoint, - sequence: original_sequence, - ..Default::default() - }, - ); + payjoin_psbt.inputs.insert(index, psbtin); + payjoin_psbt + .unsigned_tx + .input + .insert(index, TxIn { sequence: original_sequence, ..txin }); } // Add the receiver change amount to the receiver change output, if applicable @@ -874,8 +870,7 @@ impl PayjoinProposal { mod test { use std::str::FromStr; - use bitcoin::hashes::Hash; - use bitcoin::{Address, Network, ScriptBuf}; + use bitcoin::{Address, Network}; use rand::rngs::StdRng; use rand::SeedableRng; @@ -958,23 +953,9 @@ mod test { // Specify excessive fee rate in sender params proposal.params.min_feerate = FeeRate::from_sat_per_vb_unchecked(1000); // Input contribution for the receiver, from the BIP78 test vector - let input: (OutPoint, TxOut) = ( - OutPoint { - txid: "833b085de288cda6ff614c6e8655f61e7ae4f84604a2751998dc25a0d1ba278f" - .parse() - .unwrap(), - vout: 1, - }, - TxOut { - value: Amount::from_sat(2000000), - // HACK: The script pubkey in the original test vector is a nested p2sh witness - // script, which is not correctly supported in our current weight calculations. - // To get around this limitation, this test uses a native segwit script instead. - script_pubkey: ScriptBuf::new_p2wpkh(&bitcoin::WPubkeyHash::hash( - "00145f806655e5924c9204c2d51be5394f4bf9eda210".as_bytes(), - )), - }, - ); + let proposal_psbt = Psbt::from_str("cHNidP8BAJwCAAAAAo8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////jye60aAl3JgZdaIERvjkeh72VYZuTGH/ps2I4l0IO4MBAAAAAP7///8CJpW4BQAAAAAXqRQd6EnwadJ0FQ46/q6NcutaawlEMIcACT0AAAAAABepFHdAltvPSGdDwi9DR+m0af6+i2d6h9MAAAAAAAEBIICEHgAAAAAAF6kUyPLL+cphRyyI5GTUazV0hF2R2NWHAQcXFgAUX4BmVeWSTJIEwtUb5TlPS/ntohABCGsCRzBEAiBnu3tA3yWlT0WBClsXXS9j69Bt+waCs9JcjWtNjtv7VgIge2VYAaBeLPDB6HGFlpqOENXMldsJezF9Gs5amvDQRDQBIQJl1jz1tBt8hNx2owTm+4Du4isx0pmdKNMNIjjaMHFfrQAAAA==").unwrap(); + let input: (PsbtInput, TxIn) = + (proposal_psbt.inputs[1].clone(), proposal_psbt.unsigned_tx.input[1].clone()); let mut payjoin = proposal .assume_interactive_receiver() .check_inputs_not_owned(|_| Ok(false)) @@ -993,7 +974,7 @@ mod test { }) .expect("Receiver output should be identified") .commit_outputs() - .contribute_witness_inputs(vec![input]) + .contribute_inputs(vec![input]) .expect("Failed to contribute inputs") .commit_inputs(); let mut payjoin_clone = payjoin.clone(); diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index e717bdc2..5d3bf41b 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -3,8 +3,8 @@ use std::time::{Duration, SystemTime}; use bitcoin::base64::prelude::BASE64_URL_SAFE_NO_PAD; use bitcoin::base64::Engine; -use bitcoin::psbt::Psbt; -use bitcoin::{Address, Amount, FeeRate, OutPoint, Script, TxOut}; +use bitcoin::psbt::{Input as PsbtInput, Psbt}; +use bitcoin::{Address, Amount, FeeRate, OutPoint, Script, TxIn, TxOut}; use serde::de::Deserializer; use serde::{Deserialize, Serialize}; use url::Url; @@ -466,11 +466,11 @@ impl WantsInputs { /// Add the provided list of inputs to the transaction. /// Any excess input amount is added to the change_vout output indicated previously. - pub fn contribute_witness_inputs( + pub fn contribute_inputs( self, - inputs: impl IntoIterator, + inputs: impl IntoIterator, ) -> Result { - let inner = self.inner.contribute_witness_inputs(inputs)?; + let inner = self.inner.contribute_inputs(inputs)?; Ok(WantsInputs { inner, context: self.context }) } diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index dcc53ec5..5d31bb74 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -5,8 +5,9 @@ mod integration { use std::str::FromStr; use bitcoin::policy::DEFAULT_MIN_RELAY_TX_FEE; - use bitcoin::psbt::Psbt; - use bitcoin::{Amount, FeeRate, OutPoint, TxOut, Weight}; + use bitcoin::psbt::{Input as PsbtInput, Psbt}; + use bitcoin::transaction::InputWeightPrediction; + use bitcoin::{Amount, FeeRate, OutPoint, TxIn, TxOut, Weight}; use bitcoind::bitcoincore_rpc::json::{AddressType, WalletProcessPsbtResult}; use bitcoind::bitcoincore_rpc::{self, RpcApi}; use log::{log_enabled, Level}; @@ -639,15 +640,8 @@ mod integration { .iter() .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout) .unwrap(); - let txo_to_contribute = bitcoin::TxOut { - value: selected_utxo.amount, - script_pubkey: selected_utxo.script_pub_key.clone(), - }; - - let payjoin = payjoin - .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) - .unwrap() - .commit_inputs(); + let input_pair = input_pair_from_list_unspent(selected_utxo); + let payjoin = payjoin.contribute_inputs(vec![input_pair]).unwrap().commit_inputs(); // Sign and finalize the proposal PSBT let payjoin_proposal = payjoin @@ -796,17 +790,7 @@ mod integration { .script_pubkey(), }]; let drain_script = outputs[0].script_pubkey.clone(); - let inputs = receiver_utxos - .iter() - .map(|utxo| { - let outpoint = OutPoint { txid: utxo.txid, vout: utxo.vout }; - let txo = bitcoin::TxOut { - value: utxo.amount, - script_pubkey: utxo.script_pub_key.clone(), - }; - (outpoint, txo) - }) - .collect(); + let inputs = receiver_utxos.iter().map(input_pair_from_list_unspent).collect(); let response = handle_v1_pj_request( req, headers, @@ -1008,7 +992,7 @@ mod integration { receiver: &bitcoincore_rpc::Client, custom_outputs: Option>, drain_script: Option<&bitcoin::Script>, - custom_inputs: Option>, + custom_inputs: Option>, ) -> String { // Receiver receive payjoin proposal, IRL it will be an HTTP request (over ssl or onion) let proposal = payjoin::receive::UncheckedProposal::from_request( @@ -1030,7 +1014,7 @@ mod integration { receiver: &bitcoincore_rpc::Client, custom_outputs: Option>, drain_script: Option<&bitcoin::Script>, - custom_inputs: Option>, + custom_inputs: Option>, ) -> payjoin::receive::PayjoinProposal { // in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx let _to_broadcast_in_failure_case = proposal.extract_tx_to_schedule_broadcast(); @@ -1100,15 +1084,11 @@ mod integration { .iter() .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout) .unwrap(); - let txo_to_contribute = bitcoin::TxOut { - value: selected_utxo.amount, - script_pubkey: selected_utxo.script_pub_key.clone(), - }; - vec![(selected_outpoint, txo_to_contribute)] + let input_pair = input_pair_from_list_unspent(selected_utxo); + vec![input_pair] } }; - - let payjoin = payjoin.contribute_witness_inputs(inputs).unwrap().commit_inputs(); + let payjoin = payjoin.contribute_inputs(inputs).unwrap().commit_inputs(); let payjoin_proposal = payjoin .finalize_proposal( @@ -1152,6 +1132,27 @@ mod integration { ) } + fn input_pair_from_list_unspent( + utxo: &bitcoind::bitcoincore_rpc::bitcoincore_rpc_json::ListUnspentResultEntry, + ) -> (PsbtInput, TxIn) { + let psbtin = PsbtInput { + // NOTE: non_witness_utxo is not necessary because bitcoin-cli always supplies + // witness_utxo, even for non-witness inputs + witness_utxo: Some(TxOut { + value: utxo.amount, + script_pubkey: utxo.script_pub_key.clone(), + }), + redeem_script: utxo.redeem_script.clone(), + witness_script: utxo.witness_script.clone(), + ..Default::default() + }; + let txin = TxIn { + previous_output: OutPoint { txid: utxo.txid, vout: utxo.vout }, + ..Default::default() + }; + (psbtin, txin) + } + struct HeaderMock(HashMap); impl payjoin::receive::Headers for HeaderMock { From 800658977d1bb4a29b9553aecafdf45cca7a51a7 Mon Sep 17 00:00:00 2001 From: spacebear Date: Tue, 15 Oct 2024 22:57:45 -0400 Subject: [PATCH 2/9] Fix weight estimations for nested segwit inputs To determine whether an input script uses nested segwit (p2wpkh-in-p2sh), we need the redeem script that unlocks it. This can be obtained from either the input signature (if the input is signed, e.g. the sender inputs ), or the redeem_script field on the PSBT input which can be populated by the wallet that owns it (e.g. the receiver inputs). When making weight predictions for a P2SH input, we now check the final_script_sig first and fallback to the redeem_script if there is no signature. Additionally, the order of operations in `finalize_proposal` is updated such that we don't clear the sender's signatures until after the weight predictions/fee estimations are completed. --- payjoin/src/psbt.rs | 23 ++++++++++++++-------- payjoin/src/receive/mod.rs | 39 ++++++++++++++++++++++---------------- 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/payjoin/src/psbt.rs b/payjoin/src/psbt.rs index 2bc04cca..25a028e4 100644 --- a/payjoin/src/psbt.rs +++ b/payjoin/src/psbt.rs @@ -195,17 +195,24 @@ impl<'a> InputPair<'a> { // Get the input weight prediction corresponding to spending an output of this address type let iwp = match self.address_type()? { P2pkh => Ok(InputWeightPrediction::P2PKH_COMPRESSED_MAX), - P2sh => - match self.psbtin.final_script_sig.as_ref().and_then(|s| redeem_script(s.as_ref())) - { + P2sh => { + // redeemScript can be extracted from scriptSig for signed P2SH inputs + let redeem_script = if let Some(ref script_sig) = self.psbtin.final_script_sig { + redeem_script(script_sig) + // try the PSBT redeem_script field for unsigned inputs. + } else { + self.psbtin.redeem_script.as_ref().map(|script| script.as_ref()) + }; + match redeem_script { // Nested segwit p2wpkh. Some(script) if script.is_witness_program() && script.is_p2wpkh() => Ok(NESTED_P2WPKH_MAX), // Other script or witness program. Some(_) => Err(InputWeightError::NotSupported), // No redeem script provided. Cannot determine the script type. - None => Err(InputWeightError::NotFinalized), - }, + None => Err(InputWeightError::NoRedeemScript), + } + } P2wpkh => Ok(InputWeightPrediction::P2WPKH_MAX), P2wsh => Err(InputWeightError::NotSupported), P2tr => Ok(InputWeightPrediction::P2TR_KEY_DEFAULT_SIGHASH), @@ -323,7 +330,7 @@ impl From for AddressTypeError { #[derive(Debug)] pub(crate) enum InputWeightError { AddressType(AddressTypeError), - NotFinalized, + NoRedeemScript, NotSupported, } @@ -331,7 +338,7 @@ impl fmt::Display for InputWeightError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match self { Self::AddressType(_) => write!(f, "invalid address type"), - Self::NotFinalized => write!(f, "input not finalized"), + Self::NoRedeemScript => write!(f, "p2sh input missing a redeem script"), Self::NotSupported => write!(f, "weight prediction not supported"), } } @@ -341,7 +348,7 @@ impl std::error::Error for InputWeightError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { Self::AddressType(error) => Some(error), - Self::NotFinalized => None, + Self::NoRedeemScript => None, Self::NotSupported => None, } } diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index b4ee2050..3437de1c 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -776,12 +776,7 @@ impl ProvisionalProposal { output_contribution_weight } - /// Return a Payjoin Proposal PSBT that the sender will find acceptable. - /// - /// This attempts to calculate any network fee owed by the receiver, subtract it from their output, - /// and return a PSBT that can produce a consensus-valid transaction that the sender will accept. - /// - /// wallet_process_psbt should sign and finalize receiver inputs + /// Prepare the PSBT by clearing the fields that the sender expects to be empty fn prepare_psbt(mut self, processed_psbt: Psbt) -> Result { self.payjoin_psbt = processed_psbt; log::trace!("Preparing PSBT {:#?}", self.payjoin_psbt); @@ -808,6 +803,7 @@ impl ProvisionalProposal { Ok(PayjoinProposal { payjoin_psbt: self.payjoin_psbt, params: self.params }) } + /// Return the indexes of the sender inputs fn sender_input_indexes(&self) -> Vec { // iterate proposal as mutable WITH the outpoint (previous_output) available too let mut original_inputs = self.original_psbt.input_pairs().peekable(); @@ -828,20 +824,27 @@ impl ProvisionalProposal { sender_input_indexes } + /// Return a Payjoin Proposal PSBT that the sender will find acceptable. + /// + /// This attempts to calculate any network fee owed by the receiver, subtract it from their output, + /// and return a PSBT that can produce a consensus-valid transaction that the sender will accept. + /// + /// wallet_process_psbt should sign and finalize receiver inputs pub fn finalize_proposal( mut self, wallet_process_psbt: impl Fn(&Psbt) -> Result, min_feerate_sat_per_vb: Option, max_feerate_sat_per_vb: FeeRate, ) -> Result { + let mut psbt = self.apply_fee(min_feerate_sat_per_vb, max_feerate_sat_per_vb)?.clone(); + // Remove now-invalid sender signatures before applying the receiver signatures for i in self.sender_input_indexes() { - log::trace!("Clearing sender script signatures for input {}", i); - self.payjoin_psbt.inputs[i].final_script_sig = None; - self.payjoin_psbt.inputs[i].final_script_witness = None; - self.payjoin_psbt.inputs[i].tap_key_sig = None; + log::trace!("Clearing sender input {}", i); + psbt.inputs[i].final_script_sig = None; + psbt.inputs[i].final_script_witness = None; + psbt.inputs[i].tap_key_sig = None; } - let psbt = self.apply_fee(min_feerate_sat_per_vb, max_feerate_sat_per_vb)?; - let psbt = wallet_process_psbt(psbt)?; + let psbt = wallet_process_psbt(&psbt)?; let payjoin_proposal = self.prepare_psbt(psbt)?; Ok(payjoin_proposal) } @@ -1002,13 +1005,17 @@ mod test { // Input weight for a single nested P2WPKH (nested segwit) receiver input let nested_p2wpkh_proposal = ProvisionalProposal { - original_psbt: Psbt::from_str("cHNidP8BAHECAAAAAX57euL5j6xOst5JB/e/gp58RihmmpxXpsc2hEKKcVFkAAAAAAD9////AhAnAAAAAAAAFgAUtjrU62JOASAnPQ4e30wBM/Exk7ZM0QKVAAAAABYAFL6xh6gjSHmznJnPMbolG7wbGuwtAAAAAAABAIYCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wQCqgAA/////wIA+QKVAAAAABepFOyefe4gjXozL4pzi5vcPrjMeCJwhwAAAAAAAAAAJmokqiGp7eL2HD9x0d79P6mZ36NpU3VcaQaJeZlitIvr2DaXToz5AAAAAAEBIAD5ApUAAAAAF6kU7J597iCNejMvinOLm9w+uMx4InCHAQcXFgAUd6fhKfAd+JIJGpIGkMfMpjd/26sBCGsCRzBEAiBaCDgIrTw5bB1VZrB8RPycgKGNPw/YS6P+psUyxOUwgwIgbJkcbHlMoZxG7vBOVWnQQWayDTSvub6L20dDo1R5SS8BIQK2GCTydo2dJXC6C5wcSKzQ2pCsSygXa0+cMlJrRRnKtwAAIgIC0VgJvaoW2/lbq5atJhxfcgVzs6/gnpafsJHbz+ei484YDOqFk1QAAIABAACAAAAAgAEAAAACAAAAAA==").unwrap(), - payjoin_psbt: Psbt::from_str("cHNidP8BAJoCAAAAAn57euL5j6xOst5JB/e/gp58RihmmpxXpsc2hEKKcVFkAAAAAAD9////VinByqmVDo3wPNB9LnNELJoJ0g+hOdWiTSXzWEUVtiAAAAAAAP3///8CEBkGKgEAAAAWABSZUDn7eqenP01ziWRBnTCrpwwD6vHQApUAAAAAFgAUvrGHqCNIebOcmc8xuiUbvBsa7C0AAAAAAAEAhgIAAAABAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////BAKqAAD/////AgD5ApUAAAAAF6kU7J597iCNejMvinOLm9w+uMx4InCHAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEgAPkClQAAAAAXqRTsnn3uII16My+Kc4ub3D64zHgicIcAAQCEAgAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP////8CYAD/////AgDyBSoBAAAAF6kUx/+ZHBBBZ+6E/US1N2Oe7IDItXiHAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEgAPIFKgEAAAAXqRTH/5kcEEFn7oT9RLU3Y57sgMi1eIcBBxcWABRDVkPBhZHK7tVQqp2uWqQC/GGTCgEIawJHMEQCIEv8/8VpUz0dK4MCcVzS7zoyt+hPRvWwLskZBuaurnFiAiBIuyt1IRaHqFSspDbjDNM607nrDQz4lmDnekNqMNn07AEhAp1Ol7vKvG2Oi8RSrsb7uSPTET83/YXuknx63PhfCG/zAAAA").unwrap(), + original_psbt: Psbt::from_str("cHNidP8BAHECAAAAAeOsT9cRWRz3te+bgmtweG1vDLkdSH4057NuoodDNPFWAAAAAAD9////AhAnAAAAAAAAFgAUtp3bPFM/YWThyxD5Cc9OR4mb8tdMygUqAQAAABYAFODlplDoE6EGlZvmqoUngBgsu8qCAAAAAAABAIUCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wMBZwD/////AgDyBSoBAAAAF6kU2JnIn4Mmcb5kuF3EYeFei8IB43qHAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEgAPIFKgEAAAAXqRTYmcifgyZxvmS4XcRh4V6LwgHjeocBBxcWABSPGoPK1yl60X4Z9OfA7IQPUWCgVwEIawJHMEQCICZG3s2cbulPnLTvK4TwlKhsC+cem8tD2GjZZ3eMJD7FAiADh/xwv0ib8ksOrj1M27DYLiw7WFptxkMkE2YgiNMRVgEhAlDMm5DA8kU+QGiPxEWUyV1S8+XGzUOepUOck257ZOhkAAAiAgP+oMbeca66mt+UtXgHm6v/RIFEpxrwG7IvPDim5KWHpBgfVHrXVAAAgAEAAIAAAACAAQAAAAAAAAAA").unwrap(), + payjoin_psbt: Psbt::from_str("cHNidP8BAJoCAAAAAuXYOTUaVRiB8cPPhEXzcJ72/SgZOPEpPx5pkG0fNeGCAAAAAAD9////46xP1xFZHPe175uCa3B4bW8MuR1IfjTns26ih0M08VYAAAAAAP3///8CEBkGKgEAAAAWABQHuuu4H4fbQWV51IunoJLUtmMTfEzKBSoBAAAAFgAU4OWmUOgToQaVm+aqhSeAGCy7yoIAAAAAAAEBIADyBSoBAAAAF6kUQ4BssmVBS3r0s95c6dl1DQCHCR+HAQQWABQbDc333XiiOeEXroP523OoYNb1aAABAIUCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wMBZwD/////AgDyBSoBAAAAF6kU2JnIn4Mmcb5kuF3EYeFei8IB43qHAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEgAPIFKgEAAAAXqRTYmcifgyZxvmS4XcRh4V6LwgHjeocBBxcWABSPGoPK1yl60X4Z9OfA7IQPUWCgVwEIawJHMEQCICZG3s2cbulPnLTvK4TwlKhsC+cem8tD2GjZZ3eMJD7FAiADh/xwv0ib8ksOrj1M27DYLiw7WFptxkMkE2YgiNMRVgEhAlDMm5DA8kU+QGiPxEWUyV1S8+XGzUOepUOck257ZOhkAAAA").unwrap(), params: Params::default(), change_vout: 0 }; - // Currently nested segwit is not supported, see https://github.com/payjoin/rust-payjoin/issues/358 - assert!(nested_p2wpkh_proposal.additional_input_weight().is_err()); + assert_eq!( + nested_p2wpkh_proposal + .additional_input_weight() + .expect("should calculate input weight"), + Weight::from_wu(364) + ); // Input weight for a single P2WPKH (native segwit) receiver input let p2wpkh_proposal = ProvisionalProposal { From 47400ac8a31daf0e26b88af9c7f0c4070cdc5088 Mon Sep 17 00:00:00 2001 From: spacebear Date: Mon, 14 Oct 2024 18:21:12 -0400 Subject: [PATCH 3/9] Test each input script type Add an integration test for each supported input script type. --- payjoin/tests/integration.rs | 101 +++++++++++++++++++++++++++++------ 1 file changed, 85 insertions(+), 16 deletions(-) diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 5d31bb74..23e85c2e 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -31,10 +31,51 @@ mod integration { use super::*; #[test] - fn v1_to_v1() -> Result<(), BoxError> { + fn v1_to_v1_p2pkh() -> Result<(), BoxError> { init_tracing(); - let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver( + Some(AddressType::Legacy), + Some(AddressType::Legacy), + )?; + do_v1_to_v1(sender, receiver, true) + } + + #[test] + fn v1_to_v1_nested_p2wpkh() -> Result<(), BoxError> { + init_tracing(); + let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver( + Some(AddressType::P2shSegwit), + Some(AddressType::P2shSegwit), + )?; + do_v1_to_v1(sender, receiver, false) + } + + #[test] + fn v1_to_v1_p2wpkh() -> Result<(), BoxError> { + init_tracing(); + let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver( + Some(AddressType::Bech32), + Some(AddressType::Bech32), + )?; + do_v1_to_v1(sender, receiver, false) + } + // TODO: Not supported by bitcoind 0_21_2. Later versions fail for unknown reasons + //#[test] + //fn v1_to_v1_taproot() -> Result<(), BoxError> { + // init_tracing(); + // let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver( + // Some(AddressType::Bech32m), + // Some(AddressType::Bech32m), + // )?; + // do_v1_to_v1(sender, receiver, false) + //} + + fn do_v1_to_v1( + sender: bitcoincore_rpc::Client, + receiver: bitcoincore_rpc::Client, + is_p2pkh: bool, + ) -> Result<(), BoxError> { // Receiver creates the payjoin URI let pj_receiver_address = receiver.get_new_address(None, None)?.assume_checked(); let pj_uri = PjUriBuilder::new(pj_receiver_address, EXAMPLE_URL.to_owned()) @@ -70,7 +111,18 @@ mod integration { sender.send_raw_transaction(&payjoin_tx)?; // Check resulting transaction and balances - let network_fees = predicted_tx_weight(&payjoin_tx) * FeeRate::BROADCAST_MIN; + let mut predicted_tx_weight = predicted_tx_weight(&payjoin_tx); + if is_p2pkh { + // HACK: + // bitcoin-cli always grinds signatures to save 1 byte (4WU) and simplify fee + // estimates. This results in the original PSBT having a fee of 219 sats + // instead of the "worst case" 220 sats assuming a maximum-size signature. + // Note that this also affects weight predictions for segwit inputs, but the + // resulting signatures are only 1WU smaller (.25 bytes) and therefore don't + // affect our weight predictions for the original sender inputs. + predicted_tx_weight -= Weight::from_non_witness_data_size(1); + } + let network_fees = predicted_tx_weight * FeeRate::BROADCAST_MIN; assert_eq!(payjoin_tx.input.len(), 2); assert_eq!(payjoin_tx.output.len(), 2); assert_eq!(receiver.get_balances()?.mine.untrusted_pending, Amount::from_btc(51.0)?); @@ -166,7 +218,7 @@ mod integration { directory: Url, cert_der: Vec, ) -> Result<(), BoxError> { - let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver(None, None)?; let agent = Arc::new(http_agent(cert_der.clone())?); wait_for_service_ready(ohttp_relay.clone(), agent.clone()).await.unwrap(); wait_for_service_ready(directory.clone(), agent.clone()).await.unwrap(); @@ -236,7 +288,7 @@ mod integration { directory: Url, cert_der: Vec, ) -> Result<(), BoxError> { - let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver(None, None)?; let agent = Arc::new(http_agent(cert_der.clone())?); wait_for_service_ready(ohttp_relay.clone(), agent.clone()).await.unwrap(); wait_for_service_ready(directory.clone(), agent.clone()).await.unwrap(); @@ -341,7 +393,7 @@ mod integration { #[test] fn v2_to_v1() -> Result<(), BoxError> { init_tracing(); - let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver(None, None)?; // Receiver creates the payjoin URI let pj_receiver_address = receiver.get_new_address(None, None)?.assume_checked(); let pj_uri = PjUriBuilder::new(pj_receiver_address, EXAMPLE_URL.to_owned(), None, None) @@ -409,7 +461,7 @@ mod integration { directory: Url, cert_der: Vec, ) -> Result<(), BoxError> { - let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver(None, None)?; let agent: Arc = Arc::new(http_agent(cert_der.clone())?); wait_for_service_ready(ohttp_relay.clone(), agent.clone()).await?; wait_for_service_ready(directory.clone(), agent.clone()).await?; @@ -744,7 +796,7 @@ mod integration { #[test] fn receiver_consolidates_utxos() -> Result<(), BoxError> { init_tracing(); - let (bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + let (bitcoind, sender, receiver) = init_bitcoind_sender_receiver(None, None)?; // Generate more UTXOs for the receiver let receiver_address = receiver.get_new_address(None, Some(AddressType::Bech32))?.assume_checked(); @@ -832,7 +884,7 @@ mod integration { #[test] fn receiver_forwards_payment() -> Result<(), BoxError> { init_tracing(); - let (bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + let (bitcoind, sender, receiver) = init_bitcoind_sender_receiver(None, None)?; let third_party = bitcoind.create_wallet("third-party")?; // Receiver creates the payjoin URI @@ -928,6 +980,8 @@ mod integration { } fn init_bitcoind_sender_receiver( + sender_address_type: Option, + receiver_address_type: Option, ) -> Result<(bitcoind::BitcoinD, bitcoincore_rpc::Client, bitcoincore_rpc::Client), BoxError> { let bitcoind_exe = @@ -937,10 +991,9 @@ mod integration { let bitcoind = bitcoind::BitcoinD::with_conf(bitcoind_exe, &conf)?; let receiver = bitcoind.create_wallet("receiver")?; let receiver_address = - receiver.get_new_address(None, Some(AddressType::Bech32))?.assume_checked(); + receiver.get_new_address(None, receiver_address_type)?.assume_checked(); let sender = bitcoind.create_wallet("sender")?; - let sender_address = - sender.get_new_address(None, Some(AddressType::Bech32))?.assume_checked(); + let sender_address = sender.get_new_address(None, sender_address_type)?.assume_checked(); bitcoind.client.generate_to_address(1, &receiver_address)?; bitcoind.client.generate_to_address(101, &sender_address)?; @@ -1125,11 +1178,27 @@ mod integration { Ok(payjoin_psbt.extract_tx()?) } + /// Simplified input weight predictions for a fully-signed transaction fn predicted_tx_weight(tx: &bitcoin::Transaction) -> Weight { - bitcoin::transaction::predict_weight( - vec![bitcoin::transaction::InputWeightPrediction::P2WPKH_MAX; tx.input.len()], - tx.script_pubkey_lens(), - ) + let input_weight_predictions = tx.input.iter().map(|txin| { + // See https://bitcoin.stackexchange.com/a/107873 + match (txin.script_sig.is_empty(), txin.witness.is_empty()) { + // witness is empty: legacy input + (false, true) => InputWeightPrediction::P2PKH_COMPRESSED_MAX, + // script sig is empty: native segwit input + (true, false) => match txin.witness.len() { + // + 1 => InputWeightPrediction::P2TR_KEY_DEFAULT_SIGHASH, + // + 2 => InputWeightPrediction::P2WPKH_MAX, + _ => panic!("unsupported witness"), + }, + // neither are empty: nested segwit (p2wpkh-in-p2sh) input + (false, false) => InputWeightPrediction::from_slice(23, &[72, 33]), + _ => panic!("one of script_sig or witness should be non-empty"), + } + }); + bitcoin::transaction::predict_weight(input_weight_predictions, tx.script_pubkey_lens()) } fn input_pair_from_list_unspent( From 18c37cdaa5f0bb2dfe7b7b1beae0e0dc0f1fbff4 Mon Sep 17 00:00:00 2001 From: spacebear Date: Tue, 15 Oct 2024 21:57:12 -0400 Subject: [PATCH 4/9] Test mixed input scripts in v1 The payjoin v1 spec disallows mixed input script types. Add a test that validates this behavior. --- payjoin/tests/integration.rs | 42 ++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 23e85c2e..32794803 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -132,6 +132,48 @@ mod integration { ); Ok(()) } + + #[test] + fn disallow_mixed_input_scripts() -> Result<(), BoxError> { + init_tracing(); + let (_bitcoind, sender, receiver) = init_bitcoind_sender_receiver( + Some(AddressType::Bech32), + Some(AddressType::P2shSegwit), + )?; + + // Receiver creates the payjoin URI + let pj_receiver_address = receiver.get_new_address(None, None)?.assume_checked(); + let pj_uri = PjUriBuilder::new(pj_receiver_address, EXAMPLE_URL.to_owned()) + .amount(Amount::ONE_BTC) + .build(); + + // ********************** + // Inside the Sender: + // Sender create a funded PSBT (not broadcasted) to address with amount given in the pj_uri + let uri = Uri::from_str(&pj_uri.to_string()) + .unwrap() + .assume_checked() + .check_pj_supported() + .unwrap(); + let psbt = build_original_psbt(&sender, &uri)?; + debug!("Original psbt: {:#?}", psbt); + let (req, ctx) = RequestBuilder::from_psbt_and_uri(psbt, uri)? + .build_with_additional_fee(Amount::from_sat(10000), None, FeeRate::ZERO, false)? + .extract_v1()?; + let headers = HeaderMock::new(&req.body, req.content_type); + + // ********************** + // 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()); + Ok(()) + } } #[cfg(feature = "danger-local-https")] From ffa629749931881003a1fe2bf1996285190f3403 Mon Sep 17 00:00:00 2001 From: spacebear Date: Fri, 11 Oct 2024 14:58:32 -0400 Subject: [PATCH 5/9] Allow mixed input scripts in payjoin v2 Add support for mixed input script types, which are allowed in the payjoin v2 spec and enable more batching use cases. --- payjoin/src/receive/mod.rs | 36 ++++--- payjoin/src/send/error.rs | 6 ++ payjoin/src/send/mod.rs | 53 ++++++--- payjoin/tests/integration.rs | 202 +++++++++++++++++++++++++++++++---- 4 files changed, 251 insertions(+), 46 deletions(-) diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 3437de1c..a692fb97 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -223,6 +223,11 @@ 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 check_no_mixed_input_scripts(self) -> Result { + // 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 @@ -740,21 +745,22 @@ impl ProvisionalProposal { /// Calculate the additional input weight contributed by the receiver fn additional_input_weight(&self) -> Result { - // This error should never happen. We check for at least one input in the constructor - let input_pair = self - .payjoin_psbt - .input_pairs() - .next() - .ok_or(InternalRequestError::OriginalPsbtNotBroadcastable)?; - // Calculate the additional weight contribution - let input_count = self.payjoin_psbt.inputs.len() - self.original_psbt.inputs.len(); - log::trace!("input_count : {}", input_count); - let weight_per_input = - input_pair.expected_input_weight().map_err(InternalRequestError::InputWeight)?; - log::trace!("weight_per_input : {}", weight_per_input); - let contribution_weight = weight_per_input * input_count as u64; - log::trace!("contribution_weight: {}", contribution_weight); - Ok(contribution_weight) + fn inputs_weight(psbt: &Psbt) -> Result { + psbt.input_pairs().try_fold( + Weight::ZERO, + |acc, input_pair| -> Result { + let input_weight = input_pair + .expected_input_weight() + .map_err(InternalRequestError::InputWeight)?; + Ok(acc + input_weight) + }, + ) + } + let payjoin_inputs_weight = inputs_weight(&self.payjoin_psbt)?; + let original_inputs_weight = inputs_weight(&self.original_psbt)?; + let input_contribution_weight = payjoin_inputs_weight - original_inputs_weight; + log::trace!("input_contribution_weight : {}", input_contribution_weight); + Ok(input_contribution_weight) } /// Calculate the additional output weight contributed by the receiver diff --git a/payjoin/src/send/error.rs b/payjoin/src/send/error.rs index 114ff962..f453d866 100644 --- a/payjoin/src/send/error.rs +++ b/payjoin/src/send/error.rs @@ -19,6 +19,8 @@ pub(crate) enum InternalValidationError { Io(std::io::Error), InvalidAddressType(crate::psbt::AddressTypeError), NoInputs, + PrevTxOut(crate::psbt::PrevTxOutError), + InputWeight(crate::psbt::InputWeightError), VersionsDontMatch { proposed: Version, original: Version, @@ -82,6 +84,8 @@ impl fmt::Display for ValidationError { Io(e) => write!(f, "couldn't read PSBT: {}", e), InvalidAddressType(e) => write!(f, "invalid input address type: {}", e), NoInputs => write!(f, "PSBT doesn't have any inputs"), + PrevTxOut(e) => write!(f, "missing previous txout information: {}", e), + InputWeight(e) => write!(f, "can not determine expected input weight: {}", e), VersionsDontMatch { proposed, original, } => write!(f, "proposed transaction version {} doesn't match the original {}", proposed, original), LockTimesDontMatch { proposed, original, } => write!(f, "proposed transaction lock time {} doesn't match the original {}", proposed, original), SenderTxinSequenceChanged { proposed, original, } => write!(f, "proposed transaction sequence number {} doesn't match the original {}", proposed, original), @@ -125,6 +129,8 @@ impl std::error::Error for ValidationError { Io(error) => Some(error), InvalidAddressType(error) => Some(error), NoInputs => None, + PrevTxOut(error) => Some(error), + InputWeight(error) => Some(error), VersionsDontMatch { proposed: _, original: _ } => None, LockTimesDontMatch { proposed: _, original: _ } => None, SenderTxinSequenceChanged { proposed: _, original: _ } => None, diff --git a/payjoin/src/send/mod.rs b/payjoin/src/send/mod.rs index 99f10584..1147333f 100644 --- a/payjoin/src/send/mod.rs +++ b/payjoin/src/send/mod.rs @@ -248,7 +248,7 @@ pub struct RequestContext { } impl RequestContext { - /// Extract serialized V1 Request and Context froma Payjoin Proposal + /// Extract serialized V1 Request and Context from a Payjoin Proposal pub fn extract_v1(&self) -> Result<(Request, ContextV1), CreateRequestError> { let url = serialize_url( self.endpoint.clone(), @@ -267,6 +267,7 @@ impl RequestContext { fee_contribution: self.fee_contribution, payee: self.payee.clone(), min_fee_rate: self.min_fee_rate, + allow_mixed_input_scripts: false, }, )) } @@ -335,6 +336,7 @@ impl RequestContext { fee_contribution: self.fee_contribution, payee: self.payee.clone(), min_fee_rate: self.min_fee_rate, + allow_mixed_input_scripts: true, }, rs: Some(self.extract_rs_pubkey()?), e: Some(self.e.clone()), @@ -377,6 +379,7 @@ pub struct ContextV1 { fee_contribution: Option<(bitcoin::Amount, usize)>, min_fee_rate: FeeRate, payee: ScriptBuf, + allow_mixed_input_scripts: bool, } #[cfg(feature = "v2")] @@ -473,18 +476,35 @@ impl ContextV1 { ensure!(contributed_fee <= proposed_fee - original_fee, PayeeTookContributedFee); let original_weight = self.original_psbt.clone().extract_tx_unchecked_fee_rate().weight(); let original_fee_rate = original_fee / original_weight; - // TODO: This should support mixed input types - ensure!( - contributed_fee - <= original_fee_rate - * self - .original_psbt - .input_pairs() - .next() - .expect("This shouldn't happen. Failed to get an original input.") + let original_spks = self + .original_psbt + .input_pairs() + .map(|input_pair| { + input_pair + .previous_txout() + .map_err(InternalValidationError::PrevTxOut) + .map(|txout| txout.script_pubkey.clone()) + }) + .collect::>>()?; + let additional_input_weight = proposal.input_pairs().try_fold( + Weight::ZERO, + |acc, input_pair| -> InternalResult { + let spk = &input_pair + .previous_txout() + .map_err(InternalValidationError::PrevTxOut)? + .script_pubkey; + if original_spks.contains(spk) { + Ok(acc) + } else { + let weight = input_pair .expected_input_weight() - .expect("This shouldn't happen. Weight should have been calculated successfully before.") - * (proposal.inputs.len() - self.original_psbt.inputs.len()) as u64, + .map_err(InternalValidationError::InputWeight)?; + Ok(acc + weight) + } + }, + )?; + ensure!( + contributed_fee <= original_fee_rate * additional_input_weight, FeeContributionPaysOutputSizeIncrease ); if self.min_fee_rate > FeeRate::ZERO { @@ -560,7 +580,13 @@ impl ContextV1 { ReceiverTxinMissingUtxoInfo ); ensure!(proposed.txin.sequence == original.txin.sequence, MixedSequence); - check_eq!(proposed.address_type()?, original.address_type()?, MixedInputTypes); + if !self.allow_mixed_input_scripts { + check_eq!( + proposed.address_type()?, + original.address_type()?, + MixedInputTypes + ); + } } } } @@ -836,6 +862,7 @@ mod test { fee_contribution: Some((bitcoin::Amount::from_sat(182), 0)), min_fee_rate: FeeRate::ZERO, payee, + allow_mixed_input_scripts: false, }; ctx } diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 32794803..37f0bdac 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -397,7 +397,7 @@ mod integration { // POST payjoin let proposal = session.process_res(response.bytes().await?.to_vec().as_slice(), ctx)?.unwrap(); - let mut payjoin_proposal = handle_directory_proposal(&receiver, proposal); + let mut payjoin_proposal = handle_directory_proposal(&receiver, proposal, None); assert!(!payjoin_proposal.is_output_substitution_disabled()); let (req, ctx) = payjoin_proposal.extract_v2_req()?; let response = agent.post(req.url).body(req.body).send().await?; @@ -432,6 +432,161 @@ mod integration { } } + #[tokio::test] + async fn v2_to_v2_mixed_input_script_types() { + init_tracing(); + let (cert, key) = local_cert_key(); + let ohttp_relay_port = find_free_port(); + let ohttp_relay = + Url::parse(&format!("http://localhost:{}", ohttp_relay_port)).unwrap(); + let directory_port = find_free_port(); + let directory = Url::parse(&format!("https://localhost:{}", directory_port)).unwrap(); + let gateway_origin = http::Uri::from_str(directory.as_str()).unwrap(); + tokio::select!( + _ = ohttp_relay::listen_tcp(ohttp_relay_port, gateway_origin) => assert!(false, "Ohttp relay is long running"), + _ = init_directory(directory_port, (cert.clone(), key)) => assert!(false, "Directory server is long running"), + res = do_v2_send_receive(ohttp_relay, directory, cert) => assert!(res.is_ok(), "v2 send receive failed: {:#?}", res) + ); + + async fn do_v2_send_receive( + ohttp_relay: Url, + directory: Url, + cert_der: Vec, + ) -> Result<(), BoxError> { + let (bitcoind, sender, receiver) = init_bitcoind_sender_receiver(None, None)?; + let agent = Arc::new(http_agent(cert_der.clone())?); + wait_for_service_ready(ohttp_relay.clone(), agent.clone()).await.unwrap(); + wait_for_service_ready(directory.clone(), agent.clone()).await.unwrap(); + let ohttp_keys = + payjoin::io::fetch_ohttp_keys(ohttp_relay, directory.clone(), cert_der.clone()) + .await?; + // ********************** + // Inside the Receiver: + // make utxos with different script types + + let legacy_address = + receiver.get_new_address(None, Some(AddressType::Legacy))?.assume_checked(); + let nested_segwit_address = + receiver.get_new_address(None, Some(AddressType::P2shSegwit))?.assume_checked(); + let segwit_address = + receiver.get_new_address(None, Some(AddressType::Bech32))?.assume_checked(); + // TODO: + //let taproot_address = + // receiver.get_new_address(None, Some(AddressType::Bech32m))?.assume_checked(); + bitcoind.client.generate_to_address(1, &legacy_address)?; + bitcoind.client.generate_to_address(1, &nested_segwit_address)?; + bitcoind.client.generate_to_address(101, &segwit_address)?; + let receiver_utxos = receiver + .list_unspent( + None, + None, + Some(&[&legacy_address, &nested_segwit_address, &segwit_address]), + None, + None, + ) + .unwrap(); + assert_eq!(3, receiver_utxos.len(), "receiver doesn't have enough UTXOs"); + assert_eq!( + Amount::from_btc(150.0)?, + receiver_utxos.iter().fold(Amount::ZERO, |acc, txo| acc + txo.amount), + "receiver doesn't have enough bitcoin" + ); + + let address = receiver.get_new_address(None, None)?.assume_checked(); + + // test session with expiry in the future + let mut session = initialize_session( + address.clone(), + directory.clone(), + ohttp_keys.clone(), + cert_der.clone(), + None, + ) + .await?; + println!("session: {:#?}", &session); + let pj_uri_string = session.pj_uri_builder().build().to_string(); + // Poll receive request + let (req, ctx) = session.extract_req()?; + let response = agent.post(req.url).body(req.body).send().await?; + assert!(response.status().is_success()); + let response_body = + session.process_res(response.bytes().await?.to_vec().as_slice(), ctx).unwrap(); + // No proposal yet since sender has not responded + assert!(response_body.is_none()); + + // ********************** + // Inside the Sender: + // Create a funded PSBT (not broadcasted) to address with amount given in the pj_uri + let pj_uri = Uri::from_str(&pj_uri_string) + .unwrap() + .assume_checked() + .check_pj_supported() + .unwrap(); + let psbt = build_sweep_psbt(&sender, &pj_uri)?; + let mut req_ctx = RequestBuilder::from_psbt_and_uri(psbt.clone(), pj_uri.clone())? + .build_recommended(FeeRate::BROADCAST_MIN)?; + let (Request { url, body, content_type, .. }, send_ctx) = + req_ctx.extract_v2(directory.to_owned())?; + let response = agent + .post(url.clone()) + .header("Content-Type", content_type) + .body(body.clone()) + .send() + .await + .unwrap(); + log::info!("Response: {:#?}", &response); + assert!(response.status().is_success()); + let response_body = + send_ctx.process_response(&mut response.bytes().await?.to_vec().as_slice())?; + // No response body yet since we are async and pushed fallback_psbt to the buffer + assert!(response_body.is_none()); + + // ********************** + // Inside the Receiver: + + // GET fallback psbt + let (req, ctx) = session.extract_req()?; + let response = agent.post(req.url).body(req.body).send().await?; + // POST payjoin + let proposal = + session.process_res(response.bytes().await?.to_vec().as_slice(), ctx)?.unwrap(); + let inputs = receiver_utxos.iter().map(input_pair_from_list_unspent).collect(); + let mut payjoin_proposal = + handle_directory_proposal(&receiver, proposal, Some(inputs)); + assert!(!payjoin_proposal.is_output_substitution_disabled()); + let (req, ctx) = payjoin_proposal.extract_v2_req()?; + let response = agent.post(req.url).body(req.body).send().await?; + let res = response.bytes().await?.to_vec(); + payjoin_proposal.process_res(res, ctx)?; + + // ********************** + // Inside the Sender: + // Sender checks, signs, finalizes, extracts, and broadcasts + // Replay post fallback to get the response + let (Request { url, body, .. }, send_ctx) = + req_ctx.extract_v2(directory.to_owned())?; + let response = agent.post(url).body(body).send().await?; + let checked_payjoin_proposal_psbt = send_ctx + .process_response(&mut response.bytes().await?.to_vec().as_slice())? + .unwrap(); + let payjoin_tx = extract_pj_tx(&sender, checked_payjoin_proposal_psbt)?; + sender.send_raw_transaction(&payjoin_tx)?; + log::info!("sent"); + + // Check resulting transaction and balances + let network_fees = predicted_tx_weight(&payjoin_tx) * FeeRate::BROADCAST_MIN; + // Sender sent the entire value of their utxo to receiver (minus fees) + assert_eq!(payjoin_tx.input.len(), 4); + assert_eq!(payjoin_tx.output.len(), 1); + assert_eq!( + receiver.get_balances()?.mine.untrusted_pending, + Amount::from_btc(200.0)? - network_fees + ); + assert_eq!(sender.get_balances()?.mine.untrusted_pending, Amount::from_btc(0.0)?); + Ok(()) + } + } + #[test] fn v2_to_v1() -> Result<(), BoxError> { init_tracing(); @@ -574,7 +729,8 @@ mod integration { } }; let proposal = session.process_res(response.as_slice(), ctx).unwrap().unwrap(); - let mut payjoin_proposal = handle_directory_proposal(&receiver_clone, proposal); + let mut payjoin_proposal = + handle_directory_proposal(&receiver_clone, proposal, None); assert!(payjoin_proposal.is_output_substitution_disabled()); // Respond with payjoin psbt within the time window the sender is willing to wait // this response would be returned as http response to the sender @@ -678,6 +834,7 @@ mod integration { fn handle_directory_proposal( receiver: &bitcoincore_rpc::Client, proposal: UncheckedProposal, + custom_inputs: Option>, ) -> PayjoinProposal { // in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx let _to_broadcast_in_failure_case = proposal.extract_tx_to_schedule_broadcast(); @@ -720,22 +877,31 @@ mod integration { let payjoin = payjoin.commit_outputs(); - // Select receiver payjoin inputs. TODO Lock them. - let available_inputs = receiver.list_unspent(None, None, None, None, None).unwrap(); - let candidate_inputs: HashMap = available_inputs - .iter() - .map(|i| (i.amount, OutPoint { txid: i.txid, vout: i.vout })) - .collect(); - - let selected_outpoint = payjoin - .try_preserving_privacy(candidate_inputs) - .expect("Failed to make privacy preserving selection"); - let selected_utxo = available_inputs - .iter() - .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout) - .unwrap(); - let input_pair = input_pair_from_list_unspent(selected_utxo); - let payjoin = payjoin.contribute_inputs(vec![input_pair]).unwrap().commit_inputs(); + let inputs = match custom_inputs { + Some(inputs) => inputs, + None => { + // Select receiver payjoin inputs. TODO Lock them. + let available_inputs = + receiver.list_unspent(None, None, None, None, None).unwrap(); + let candidate_inputs: HashMap = available_inputs + .iter() + .map(|i| (i.amount, OutPoint { txid: i.txid, vout: i.vout })) + .collect(); + + let selected_outpoint = payjoin + .try_preserving_privacy(candidate_inputs) + .expect("Failed to make privacy preserving selection"); + let selected_utxo = available_inputs + .iter() + .find(|i| { + i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout + }) + .unwrap(); + let input_pair = input_pair_from_list_unspent(selected_utxo); + vec![input_pair] + } + }; + let payjoin = payjoin.contribute_inputs(inputs).unwrap().commit_inputs(); // Sign and finalize the proposal PSBT let payjoin_proposal = payjoin From 38eab07d91aa909b252f86131ba826d5f777e500 Mon Sep 17 00:00:00 2001 From: spacebear Date: Wed, 16 Oct 2024 12:38:33 -0400 Subject: [PATCH 6/9] Cleanup comments and fix doc links --- payjoin-cli/src/app/v1.rs | 2 +- payjoin-cli/src/app/v2.rs | 2 +- payjoin/src/receive/mod.rs | 18 +++++++++++++----- payjoin/src/send/mod.rs | 14 ++++++-------- payjoin/tests/integration.rs | 6 ++---- 5 files changed, 23 insertions(+), 19 deletions(-) diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index 8ae62f43..f40da24f 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -319,7 +319,7 @@ impl App { } })?; log::trace!("check2"); - // Receive Check 3: receiver can't sign for proposal inputs + // Receive Check 3: no mixed input scripts let proposal = proposal.check_no_mixed_input_scripts()?; log::trace!("check3"); diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index 8dd1f0da..f088729d 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -306,7 +306,7 @@ impl App { } })?; log::trace!("check2"); - // Receive Check 3: receiver can't sign for proposal inputs + // Receive Check 3: no mixed input scripts let proposal = proposal.check_no_mixed_input_scripts()?; log::trace!("check3"); diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index a692fb97..31fee244 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -165,7 +165,7 @@ impl UncheckedProposal { /// Typestate to validate that the Original PSBT has no receiver-owned inputs. /// -/// Call [`check_no_receiver_owned_inputs()`](struct.UncheckedProposal.html#method.check_no_receiver_owned_inputs) to proceed. +/// Call [`Self::check_inputs_not_owned`] to proceed. #[derive(Clone)] pub struct MaybeInputsOwned { psbt: Psbt, @@ -208,8 +208,9 @@ impl MaybeInputsOwned { } /// Typestate to validate that the Original PSBT has no mixed input types. +/// This check is skipped in payjoin v2. /// -/// Call [`check_no_mixed_input_types`](struct.UncheckedProposal.html#method.check_no_mixed_input_scripts) to proceed. +/// Call [`Self::check_no_mixed_input_scripts`] to proceed. #[derive(Clone)] pub struct MaybeMixedInputScripts { psbt: Psbt, @@ -261,7 +262,7 @@ impl MaybeMixedInputScripts { /// Typestate to validate that the Original PSBT has no inputs that have been seen before. /// -/// Call [`check_no_inputs_seen`](struct.MaybeInputsSeen.html#method.check_no_inputs_seen_before) to proceed. +/// Call [`Self::check_no_inputs_seen_before`] to proceed. #[derive(Clone)] pub struct MaybeInputsSeen { psbt: Psbt, @@ -295,7 +296,7 @@ impl MaybeInputsSeen { /// The receiver has not yet identified which outputs belong to the receiver. /// /// Only accept PSBTs that send us money. -/// Identify those outputs with `identify_receiver_outputs()` to proceed +/// Identify those outputs with [`Self::identify_receiver_outputs`] to proceed. #[derive(Clone)] pub struct OutputsUnknown { psbt: Psbt, @@ -345,6 +346,8 @@ impl OutputsUnknown { } /// A checked proposal that the receiver may substitute or add outputs to +/// +/// Call [`Self::commit_outputs`] to proceed. #[derive(Debug, Clone)] pub struct WantsOutputs { original_psbt: Psbt, @@ -486,6 +489,8 @@ fn interleave_shuffle( } /// A checked proposal that the receiver may contribute inputs to to make a payjoin +/// +/// Call [`Self::commit_inputs`] to proceed. #[derive(Debug, Clone)] pub struct WantsInputs { original_psbt: Psbt, @@ -658,6 +663,8 @@ impl WantsInputs { /// A checked proposal that the receiver may sign and finalize to make a proposal PSBT that the /// sender will accept. +/// +/// Call [`Self::finalize_proposal`] to return a finalized [`PayjoinProposal`]. #[derive(Debug, Clone)] pub struct ProvisionalProposal { original_psbt: Psbt, @@ -856,7 +863,8 @@ impl ProvisionalProposal { } } -/// A mutable checked proposal that the receiver may contribute inputs to to make a payjoin. +/// A finalized payjoin proposal, complete with fees and receiver signatures, that the sender +/// should find acceptable. #[derive(Clone)] pub struct PayjoinProposal { payjoin_psbt: Psbt, diff --git a/payjoin/src/send/mod.rs b/payjoin/src/send/mod.rs index 1147333f..774104df 100644 --- a/payjoin/src/send/mod.rs +++ b/payjoin/src/send/mod.rs @@ -9,11 +9,9 @@ //! 2. Construct URI request parameters, a finalized “Original PSBT” paying .amount to .address //! 3. (optional) Spawn a thread or async task that will broadcast the original PSBT fallback after //! delay (e.g. 1 minute) unless canceled -//! 4. Construct the request using [`RequestBuilder`](crate::send::RequestBuilder) with the PSBT -//! and payjoin uri +//! 4. Construct the request using [`RequestBuilder`] with the PSBT and payjoin uri //! 5. Send the request and receive response -//! 6. Process the response with -//! [`Context::process_response()`](crate::send::Context::process_response()) +//! 6. Process the response with [`ContextV1::process_response`] //! 7. Sign and finalize the Payjoin Proposal PSBT //! 8. Broadcast the Payjoin Transaction (and cancel the optional fallback broadcast) //! @@ -34,7 +32,7 @@ pub(crate) use error::{InternalCreateRequestError, InternalValidationError}; use serde::{Deserialize, Serialize}; use url::Url; -use crate::psbt::{InputPair, PsbtExt}; +use crate::psbt::PsbtExt; use crate::request::Request; #[cfg(feature = "v2")] use crate::v2::{HpkePublicKey, HpkeSecretKey}; @@ -123,7 +121,7 @@ impl<'a> RequestBuilder<'a> { .find(|(_, txo)| payout_scripts.all(|script| script != txo.script_pubkey)) .map(|(i, txo)| (i, txo.value)) { - let mut input_pairs = self.psbt.input_pairs().collect::>().into_iter(); + let mut input_pairs = self.psbt.input_pairs(); let first_input_pair = input_pairs.next().ok_or(InternalCreateRequestError::NoInputs)?; let mut input_weight = first_input_pair @@ -370,8 +368,8 @@ impl RequestContext { /// Data required for validation of response. /// -/// This type is used to process the response. Get it from [`RequestBuilder`](crate::send::RequestBuilder)'s build methods. -/// Then you only need to call [`.process_response()`](crate::send::Context::process_response()) on it to continue BIP78 flow. +/// This type is used to process the response. Get it from [`RequestBuilder`]'s build methods. +/// Then you only need to call [`Self::process_response`] on it to continue BIP78 flow. #[derive(Debug, Clone)] pub struct ContextV1 { original_psbt: Psbt, diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 37f0bdac..1137202f 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -860,7 +860,7 @@ mod integration { }) .expect("Receiver should not own any of the inputs"); - // Receive Check 3: receiver can't sign for proposal 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. @@ -880,7 +880,6 @@ mod integration { let inputs = match custom_inputs { Some(inputs) => inputs, None => { - // Select receiver payjoin inputs. TODO Lock them. let available_inputs = receiver.list_unspent(None, None, None, None, None).unwrap(); let candidate_inputs: HashMap = available_inputs @@ -1301,7 +1300,7 @@ mod integration { }) .expect("Receiver should not own any of the inputs"); - // Receive Check 3: receiver can't sign for proposal 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. @@ -1331,7 +1330,6 @@ mod integration { let inputs = match custom_inputs { Some(inputs) => inputs, None => { - // Select receiver payjoin inputs. TODO Lock them. let available_inputs = receiver.list_unspent(None, None, None, None, None).unwrap(); let candidate_inputs: HashMap = available_inputs .iter() From 0af90e61d6af42f0bb0f080bcdabdaef2e783d2a Mon Sep 17 00:00:00 2001 From: spacebear Date: Fri, 18 Oct 2024 16:46:49 -0400 Subject: [PATCH 7/9] Remove MaybeMixedInputScripts typestate 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. --- payjoin-cli/src/app/v1.rs | 7 +-- payjoin-cli/src/app/v2.rs | 7 +-- payjoin/src/receive/error.rs | 27 +++++----- payjoin/src/receive/mod.rs | 99 +++++++++++++---------------------- payjoin/src/receive/v2/mod.rs | 23 +------- payjoin/tests/integration.rs | 23 +++----- 6 files changed, 64 insertions(+), 122 deletions(-) diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index f40da24f..a2d98cc2 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -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) { diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index f088729d..3a8c8a18 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -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| { diff --git a/payjoin/src/receive/error.rs b/payjoin/src/receive/error.rs index 1cfc01f5..82bcbb77 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -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 @@ -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(_) => @@ -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), @@ -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, } @@ -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"), } diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 31fee244..135c4a38 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -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; @@ -180,7 +182,7 @@ impl MaybeInputsOwned { pub fn check_inputs_not_owned( self, is_owned: impl Fn(&Script) -> Result, - ) -> Result { + ) -> Result { let mut err = Ok(()); if let Some(e) = self .psbt @@ -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 { - // 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::>(); - 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 }) } } @@ -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; @@ -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 @@ -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| { @@ -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| { diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index 5d3bf41b..2c250bfa 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -325,29 +325,8 @@ impl MaybeInputsOwned { pub fn check_inputs_not_owned( self, is_owned: impl Fn(&Script) -> Result, - ) -> Result { + ) -> Result { 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 { - let inner = self.inner.check_no_mixed_input_scripts()?; Ok(MaybeInputsSeen { inner, context: self.context }) } } diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 1137202f..e830fdf8 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -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(()) } } @@ -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() @@ -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() From b86b6392684137867a39ac5cecea8df3457ea28c Mon Sep 17 00:00:00 2001 From: DanGould Date: Sat, 19 Oct 2024 12:22:23 -0400 Subject: [PATCH 8/9] Check uniform_sender_input_type as an Option Returning a (bool, value) tuple is a code smell vs returning an Option. CLean up the check with a helper function for clarity. --- payjoin/src/receive/mod.rs | 44 +++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 135c4a38..b8a7b288 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -547,7 +547,7 @@ impl WantsInputs { .first() .map(|input| input.sequence) .unwrap_or_default(); - let (sender_has_mixed_inputs, sender_input_type) = self.sender_mixed_inputs()?; + let uniform_sender_input_type = self.uniform_sender_input_type()?; // Insert contributions at random indices for privacy let mut rng = rand::thread_rng(); @@ -556,14 +556,12 @@ impl WantsInputs { 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()); + + if self.params.v == 1 { + // v1 payjoin proposals must not introduce mixed input script types + self.check_mixed_input_types(input_type, uniform_sender_input_type)?; } + receiver_input_amount += input_pair .previous_txout() .map_err(InternalInputContributionError::PrevTxOut)? @@ -593,22 +591,44 @@ impl WantsInputs { }) } - fn sender_mixed_inputs(&self) -> Result<(bool, bitcoin::AddressType), InputContributionError> { + /// Check for mixed input types and throw an error if conditions are met + fn check_mixed_input_types( + &self, + receiver_input_type: bitcoin::AddressType, + uniform_sender_input_type: Option, + ) -> Result<(), InputContributionError> { + if let Some(uniform_sender_input_type) = uniform_sender_input_type { + if receiver_input_type != uniform_sender_input_type { + return Err(InternalInputContributionError::MixedInputScripts( + receiver_input_type, + uniform_sender_input_type, + ) + .into()); + } + } + Ok(()) + } + + /// Check if the sender's inputs are all of the same type + /// + /// Returns `None` if the sender inputs are not all of the same type + fn uniform_sender_input_type( + &self, + ) -> Result, 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; + return Ok(None); } } - Ok((sender_has_mixed_inputs, first_input_type)) + Ok(Some(first_input_type)) } // Compute the minimum amount that the receiver must contribute to the transaction as input From 065e92bdfd9761e60394f404280b91b3f2aa4726 Mon Sep 17 00:00:00 2001 From: DanGould Date: Sat, 19 Oct 2024 12:29:28 -0400 Subject: [PATCH 9/9] Bubble up many integration.rs errors Avoid unwrap() by returning a Result<_, BoxError> --- payjoin/tests/integration.rs | 122 ++++++++++++++++------------------- 1 file changed, 54 insertions(+), 68 deletions(-) diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index e830fdf8..1b19feff 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -100,7 +100,7 @@ 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); + let response = handle_v1_pj_request(req, headers, &receiver, None, None, None)?; // this response would be returned as http response to the sender // ********************** @@ -157,7 +157,7 @@ mod integration { .unwrap(); let psbt = build_original_psbt(&sender, &uri)?; debug!("Original psbt: {:#?}", psbt); - let (req, ctx) = RequestBuilder::from_psbt_and_uri(psbt, uri)? + let (req, _ctx) = RequestBuilder::from_psbt_and_uri(psbt, uri)? .build_with_additional_fee(Amount::from_sat(10000), None, FeeRate::ZERO, false)? .extract_v1()?; let headers = HeaderMock::new(&req.body, req.content_type); @@ -165,10 +165,7 @@ mod integration { // ********************** // Inside the Receiver: // 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()); + assert!(handle_v1_pj_request(req, headers, &receiver, None, None, None).is_err()); Ok(()) } } @@ -611,7 +608,7 @@ 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); + let response = handle_v1_pj_request(req, headers, &receiver, None, None, None)?; // this response would be returned as http response to the sender // ********************** @@ -1051,7 +1048,7 @@ mod integration { Some(outputs), Some(&drain_script), Some(inputs), - ); + )?; // this response would be returned as http response to the sender // ********************** @@ -1137,7 +1134,7 @@ mod integration { Some(outputs), Some(&drain_script), Some(inputs), - ); + )?; // this response would be returned as http response to the sender // ********************** @@ -1247,20 +1244,19 @@ mod integration { custom_outputs: Option>, drain_script: Option<&bitcoin::Script>, custom_inputs: Option>, - ) -> String { + ) -> Result { // Receiver receive payjoin proposal, IRL it will be an HTTP request (over ssl or onion) let proposal = payjoin::receive::UncheckedProposal::from_request( req.body.as_slice(), req.url.query().unwrap_or(""), headers, - ) - .unwrap(); + )?; let proposal = - handle_proposal(proposal, receiver, custom_outputs, drain_script, custom_inputs); + handle_proposal(proposal, receiver, custom_outputs, drain_script, custom_inputs)?; assert!(!proposal.is_output_substitution_disabled()); let psbt = proposal.psbt(); tracing::debug!("Receiver's Payjoin proposal PSBT: {:#?}", &psbt); - psbt.to_string() + Ok(psbt.to_string()) } fn handle_proposal( @@ -1269,59 +1265,48 @@ mod integration { custom_outputs: Option>, drain_script: Option<&bitcoin::Script>, custom_inputs: Option>, - ) -> payjoin::receive::PayjoinProposal { + ) -> Result { // in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx let _to_broadcast_in_failure_case = proposal.extract_tx_to_schedule_broadcast(); // Receive Check 1: Can Broadcast - let proposal = proposal - .check_broadcast_suitability(None, |tx| { - Ok(receiver - .test_mempool_accept(&[bitcoin::consensus::encode::serialize_hex(&tx)]) - .unwrap() - .first() - .unwrap() - .allowed) - }) - .expect("Payjoin proposal should be broadcastable"); + let proposal = proposal.check_broadcast_suitability(None, |tx| { + Ok(receiver + .test_mempool_accept(&[bitcoin::consensus::encode::serialize_hex(&tx)]) + .unwrap() + .first() + .unwrap() + .allowed) + })?; // Receive Check 2: receiver can't sign for proposal inputs - let proposal = proposal - .check_inputs_not_owned(|input| { - let address = - bitcoin::Address::from_script(&input, bitcoin::Network::Regtest).unwrap(); - Ok(receiver.get_address_info(&address).unwrap().is_mine.unwrap()) - }) - .expect("Receiver should not own any of the inputs"); + let proposal = proposal.check_inputs_not_owned(|input| { + let address = bitcoin::Address::from_script(&input, bitcoin::Network::Regtest).unwrap(); + Ok(receiver.get_address_info(&address).unwrap().is_mine.unwrap()) + })?; // 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() + .check_no_inputs_seen_before(|_| Ok(false))? .identify_receiver_outputs(|output_script| { let address = bitcoin::Address::from_script(&output_script, bitcoin::Network::Regtest) .unwrap(); Ok(receiver.get_address_info(&address).unwrap().is_mine.unwrap()) - }) - .expect("Receiver should have at least one output"); + })?; let payjoin = match custom_outputs { - Some(txos) => payjoin - .replace_receiver_outputs(txos, &drain_script.unwrap()) - .expect("Could not substitute outputs"), - None => payjoin - .substitute_receiver_script( - &receiver.get_new_address(None, None).unwrap().assume_checked().script_pubkey(), - ) - .expect("Could not substitute outputs"), + Some(txos) => payjoin.replace_receiver_outputs(txos, &drain_script.unwrap())?, + None => payjoin.substitute_receiver_script( + &receiver.get_new_address(None, None)?.assume_checked().script_pubkey(), + )?, } .commit_outputs(); let inputs = match custom_inputs { Some(inputs) => inputs, None => { - let available_inputs = receiver.list_unspent(None, None, None, None, None).unwrap(); + let available_inputs = receiver.list_unspent(None, None, None, None, None)?; let candidate_inputs: HashMap = available_inputs .iter() .map(|i| (i.amount, OutPoint { txid: i.txid, vout: i.vout })) @@ -1329,7 +1314,7 @@ mod integration { let selected_outpoint = payjoin .try_preserving_privacy(candidate_inputs) - .expect("Failed to make privacy preserving selection"); + .map_err(|e| format!("Failed to make privacy preserving selection: {:?}", e))?; let selected_utxo = available_inputs .iter() .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout) @@ -1338,29 +1323,30 @@ mod integration { vec![input_pair] } }; - let payjoin = payjoin.contribute_inputs(inputs).unwrap().commit_inputs(); + let payjoin = payjoin + .contribute_inputs(inputs) + .map_err(|e| format!("Failed to contribute inputs: {:?}", e))? + .commit_inputs(); - let payjoin_proposal = payjoin - .finalize_proposal( - |psbt: &Psbt| { - Ok(receiver - .wallet_process_psbt( - &psbt.to_string(), - None, - None, - Some(true), // check that the receiver properly clears keypaths - ) - .map(|res: WalletProcessPsbtResult| { - let psbt = Psbt::from_str(&res.psbt).unwrap(); - return psbt; - }) - .unwrap()) - }, - Some(FeeRate::BROADCAST_MIN), - FeeRate::from_sat_per_vb_unchecked(2), - ) - .unwrap(); - payjoin_proposal + let payjoin_proposal = payjoin.finalize_proposal( + |psbt: &Psbt| { + Ok(receiver + .wallet_process_psbt( + &psbt.to_string(), + None, + None, + Some(true), // check that the receiver properly clears keypaths + ) + .map(|res: WalletProcessPsbtResult| { + let psbt = Psbt::from_str(&res.psbt).unwrap(); + return psbt; + }) + .unwrap()) + }, + Some(FeeRate::BROADCAST_MIN), + FeeRate::from_sat_per_vb_unchecked(2), + )?; + Ok(payjoin_proposal) } fn extract_pj_tx(