From b5dae4eae77e353844becde038b63b81ec494265 Mon Sep 17 00:00:00 2001 From: spacebear Date: Tue, 30 Jul 2024 15:54:21 -0400 Subject: [PATCH 1/9] Add new typestates for output and input contributions This effectively splits the `ProvisionalProposal` state into three states that must be completed in order: - `WantsOutputs` allows the receiver to substitute or add output(s) and produces a WantsInputs - `WantsInputs` allows the receiver to contribute input(s) as needed to fund their outputs and produces a ProvisionalProposal - `ProvisionalProposal` is only responsible for finalizing the payjoin proposal and producing a PSBT that will be acceptable to the sender --- payjoin-cli/src/app/mod.rs | 32 -------- payjoin-cli/src/app/v1.rs | 55 ++++++++++---- payjoin-cli/src/app/v2.rs | 40 ++++++++-- payjoin/src/receive/mod.rs | 135 ++++++++++++++++++++++++++-------- payjoin/src/receive/v2/mod.rs | 67 ++++++++++++----- payjoin/tests/integration.rs | 23 +++--- 6 files changed, 243 insertions(+), 109 deletions(-) diff --git a/payjoin-cli/src/app/mod.rs b/payjoin-cli/src/app/mod.rs index 811f3b58..73cb0dd2 100644 --- a/payjoin-cli/src/app/mod.rs +++ b/payjoin-cli/src/app/mod.rs @@ -94,38 +94,6 @@ pub trait App { } } -fn try_contributing_inputs( - payjoin: &mut payjoin::receive::ProvisionalProposal, - bitcoind: &bitcoincore_rpc::Client, -) -> Result<()> { - use bitcoin::OutPoint; - - let available_inputs = bitcoind - .list_unspent(None, None, None, None, None) - .context("Failed to list unspent from bitcoind")?; - 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("gg"); - let selected_utxo = available_inputs - .iter() - .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); - - // calculate receiver payjoin outputs given receiver payjoin inputs and original_psbt, - let txo_to_contribute = bitcoin::TxOut { - value: selected_utxo.amount, - script_pubkey: selected_utxo.script_pub_key.clone(), - }; - let outpoint_to_contribute = - bitcoin::OutPoint { txid: selected_utxo.txid, vout: selected_utxo.vout }; - payjoin.contribute_witness_input(txo_to_contribute, outpoint_to_contribute); - Ok(()) -} - #[cfg(feature = "danger-local-https")] fn http_agent() -> Result { Ok(http_agent_builder()?.build()?) } diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index 53c9d539..b03adac8 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, try_contributing_inputs}; +use crate::app::http_agent; use crate::db::Database; #[cfg(feature = "danger-local-https")] pub const LOCAL_CERT_FILE: &str = "localhost.der"; @@ -329,7 +329,7 @@ impl App { })?; log::trace!("check4"); - let mut provisional_payjoin = payjoin.identify_receiver_outputs(|output_script| { + let provisional_payjoin = payjoin.identify_receiver_outputs(|output_script| { if let Ok(address) = bitcoin::Address::from_script(output_script, network) { bitcoind .get_address_info(&address) @@ -340,19 +340,17 @@ impl App { } })?; - _ = try_contributing_inputs(&mut provisional_payjoin, &bitcoind) - .map_err(|e| log::warn!("Failed to contribute inputs: {}", e)); + let provisional_payjoin = provisional_payjoin.try_substitute_receiver_output(|| { + Ok(bitcoind + .get_new_address(None, None) + .map_err(|e| Error::Server(e.into()))? + .require_network(network) + .map_err(|e| Error::Server(e.into()))? + .script_pubkey()) + })?; - _ = provisional_payjoin - .try_substitute_receiver_output(|| { - Ok(bitcoind - .get_new_address(None, None) - .map_err(|e| Error::Server(e.into()))? - .require_network(network) - .map_err(|e| Error::Server(e.into()))? - .script_pubkey()) - }) - .map_err(|e| log::warn!("Failed to substitute output: {}", e)); + let provisional_payjoin = try_contributing_inputs(provisional_payjoin, &bitcoind) + .map_err(|e| Error::Server(e.into()))?; let payjoin_proposal = provisional_payjoin.finalize_proposal( |psbt: &Psbt| { @@ -367,6 +365,35 @@ impl App { } } +fn try_contributing_inputs( + payjoin: payjoin::receive::WantsInputs, + bitcoind: &bitcoincore_rpc::Client, +) -> Result { + use bitcoin::OutPoint; + + let available_inputs = bitcoind + .list_unspent(None, None, None, None, None) + .context("Failed to list unspent from bitcoind")?; + 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("gg"); + let selected_utxo = available_inputs + .iter() + .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); + + // calculate receiver payjoin outputs given receiver payjoin inputs and original_psbt, + let txo_to_contribute = bitcoin::TxOut { + value: selected_utxo.amount, + script_pubkey: selected_utxo.script_pub_key.clone(), + }; + Ok(payjoin.contribute_witness_input(txo_to_contribute, selected_outpoint)) +} + fn full>(chunk: T) -> BoxBody { Full::new(chunk.into()).map_err(|never| match never {}).boxed() } diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index 2aba3f8f..8ba7ba00 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::str::FromStr; use std::sync::Arc; @@ -272,8 +273,6 @@ impl App { &self, proposal: payjoin::receive::v2::UncheckedProposal, ) -> Result { - use crate::app::try_contributing_inputs; - let bitcoind = self.bitcoind().map_err(|e| Error::Server(e.into()))?; // in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx @@ -317,7 +316,7 @@ impl App { })?; log::trace!("check4"); - let mut provisional_payjoin = payjoin.identify_receiver_outputs(|output_script| { + let provisional_payjoin = payjoin.identify_receiver_outputs(|output_script| { if let Ok(address) = bitcoin::Address::from_script(output_script, network) { bitcoind .get_address_info(&address) @@ -328,8 +327,10 @@ impl App { } })?; - _ = try_contributing_inputs(&mut provisional_payjoin.inner, &bitcoind) - .map_err(|e| log::warn!("Failed to contribute inputs: {}", e)); + let provisional_payjoin = provisional_payjoin.try_substitute_receiver_outputs(None)?; + + let provisional_payjoin = try_contributing_inputs(provisional_payjoin, &bitcoind) + .map_err(|e| Error::Server(e.into()))?; let payjoin_proposal = provisional_payjoin.finalize_proposal( |psbt: &Psbt| { @@ -346,6 +347,35 @@ impl App { } } +fn try_contributing_inputs( + payjoin: payjoin::receive::v2::WantsInputs, + bitcoind: &bitcoincore_rpc::Client, +) -> Result { + use bitcoin::OutPoint; + + let available_inputs = bitcoind + .list_unspent(None, None, None, None, None) + .context("Failed to list unspent from bitcoind")?; + 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("gg"); + let selected_utxo = available_inputs + .iter() + .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); + + // calculate receiver payjoin outputs given receiver payjoin inputs and original_psbt, + let txo_to_contribute = bitcoin::TxOut { + value: selected_utxo.amount, + script_pubkey: selected_utxo.script_pub_key.clone(), + }; + Ok(payjoin.contribute_witness_input(txo_to_contribute, selected_outpoint)) +} + async fn unwrap_ohttp_keys_or_else_fetch(config: &AppConfig) -> Result { if let Some(keys) = config.ohttp_keys.clone() { println!("Using OHTTP Keys from config"); diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 78bafaf3..2e1dd9e9 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -307,7 +307,7 @@ impl OutputsUnknown { pub fn identify_receiver_outputs( self, is_receiver_output: impl Fn(&Script) -> Result, - ) -> Result { + ) -> Result { let owned_vouts: Vec = self .psbt .unsigned_tx @@ -325,25 +325,86 @@ impl OutputsUnknown { return Err(Error::BadRequest(InternalRequestError::MissingPayment.into())); } - Ok(ProvisionalProposal { - original_psbt: self.psbt.clone(), - payjoin_psbt: self.psbt, + Ok(WantsOutputs { psbt: self.psbt, params: self.params, owned_vouts }) + } +} + +/// A checked proposal that the receiver may substitute or add outputs to +#[derive(Debug, Clone)] +pub struct WantsOutputs { + psbt: Psbt, + params: Params, + owned_vouts: Vec, +} + +impl WantsOutputs { + pub fn is_output_substitution_disabled(&self) -> bool { + self.params.disable_output_substitution + } + + /// If output substitution is enabled, replace the receiver's output script with a new one. + pub fn try_substitute_receiver_output( + self, + generate_script: impl Fn() -> Result, + ) -> Result { + let output_value = self.psbt.unsigned_tx.output[self.owned_vouts[0]].value; + let outputs = vec![TxOut { value: output_value, script_pubkey: generate_script()? }]; + self.try_substitute_receiver_outputs(Some(outputs)) + } + + pub fn try_substitute_receiver_outputs( + self, + outputs: Option>, + ) -> Result { + let mut payjoin_psbt = self.psbt.clone(); + match outputs { + Some(o) => { + if self.params.disable_output_substitution { + // TODO: only fail if the original output's amount decreased or its script pubkey is not in `outputs` + return Err(Error::Server("Output substitution is disabled.".into())); + } + let mut replacement_outputs = o.into_iter(); + let mut outputs = vec![]; + for (i, output) in self.psbt.unsigned_tx.output.iter().enumerate() { + if self.owned_vouts.contains(&i) { + // Receiver output: substitute with a provided output + // TODO: pick from outputs in random order? + outputs.push( + replacement_outputs + .next() + .ok_or(Error::Server("Not enough outputs".into()))?, + ); + } else { + // Sender output: leave it as is + outputs.push(output.clone()); + } + } + // Append all remaining outputs + outputs.extend(replacement_outputs); + payjoin_psbt.unsigned_tx.output = outputs; + // TODO: update self.owned_vouts? + } + None => log::info!("No outputs provided: skipping output substitution."), + } + Ok(WantsInputs { + original_psbt: self.psbt, + payjoin_psbt, params: self.params, - owned_vouts, + owned_vouts: self.owned_vouts, }) } } -/// A mutable checked proposal that the receiver may contribute inputs to to make a payjoin. +/// A checked proposal that the receiver may contribute inputs to to make a payjoin #[derive(Debug, Clone)] -pub struct ProvisionalProposal { +pub struct WantsInputs { original_psbt: Psbt, payjoin_psbt: Psbt, params: Params, owned_vouts: Vec, } -impl ProvisionalProposal { +impl WantsInputs { /// Select receiver input such that the payjoin avoids surveillance. /// Return the input chosen that has been applied to the Proposal. /// @@ -427,7 +488,8 @@ impl ProvisionalProposal { .ok_or_else(|| SelectionError::from(InternalSelectionError::NotFound)) } - pub fn contribute_witness_input(&mut self, txo: TxOut, outpoint: OutPoint) { + pub fn contribute_witness_input(self, txo: TxOut, outpoint: OutPoint) -> ProvisionalProposal { + let mut payjoin_psbt = self.payjoin_psbt.clone(); // The payjoin proposal must not introduce mixed input sequence numbers let original_sequence = self .payjoin_psbt @@ -441,15 +503,15 @@ impl ProvisionalProposal { let txo_value = txo.value; let vout_to_augment = self.owned_vouts.choose(&mut rand::thread_rng()).expect("owned_vouts is empty"); - self.payjoin_psbt.unsigned_tx.output[*vout_to_augment].value += txo_value; + payjoin_psbt.unsigned_tx.output[*vout_to_augment].value += txo_value; // Insert contribution at random index for privacy let mut rng = rand::thread_rng(); let index = rng.gen_range(0..=self.payjoin_psbt.unsigned_tx.input.len()); - self.payjoin_psbt + payjoin_psbt .inputs .insert(index, bitcoin::psbt::Input { witness_utxo: Some(txo), ..Default::default() }); - self.payjoin_psbt.unsigned_tx.input.insert( + payjoin_psbt.unsigned_tx.input.insert( index, bitcoin::TxIn { previous_output: outpoint, @@ -457,29 +519,38 @@ impl ProvisionalProposal { ..Default::default() }, ); + ProvisionalProposal { + original_psbt: self.original_psbt, + payjoin_psbt, + params: self.params, + owned_vouts: self.owned_vouts, + } } - pub fn is_output_substitution_disabled(&self) -> bool { - self.params.disable_output_substitution - } - - /// If output substitution is enabled, replace the receiver's output script with a new one. - pub fn try_substitute_receiver_output( - &mut self, - generate_script: impl Fn() -> Result, - ) -> Result<(), Error> { - if self.params.disable_output_substitution { - return Err(Error::Server("Output substitution is disabled.".into())); + // TODO: temporary workaround + fn skip_contribute_inputs(self) -> ProvisionalProposal { + ProvisionalProposal { + original_psbt: self.original_psbt, + payjoin_psbt: self.payjoin_psbt, + params: self.params, + owned_vouts: self.owned_vouts, } - let substitute_script = generate_script()?; - self.payjoin_psbt.unsigned_tx.output[self.owned_vouts[0]].script_pubkey = substitute_script; - Ok(()) } +} +/// A checked proposal that the receiver may sign and finalize to make a proposal PSBT that the +/// sender will accept. +#[derive(Debug, Clone)] +pub struct ProvisionalProposal { + original_psbt: Psbt, + payjoin_psbt: Psbt, + params: Params, + owned_vouts: Vec, +} + +impl ProvisionalProposal { /// Apply additional fee contribution now that the receiver has contributed input /// this is kind of a "build_proposal" step before we sign and finalize and extract - /// - /// WARNING: DO NOT ALTER INPUTS OR OUTPUTS AFTER THIS STEP fn apply_fee(&mut self, min_feerate: Option) -> Result<&Psbt, RequestError> { let min_feerate = min_feerate.unwrap_or(FeeRate::MIN); log::trace!("min_feerate: {:?}", min_feerate); @@ -506,7 +577,7 @@ impl ProvisionalProposal { additional_fee = max_additional_fee_contribution; } log::trace!("additional_fee: {}", additional_fee); - if additional_fee > bitcoin::Amount::ZERO { + if additional_fee > Amount::ZERO { log::trace!( "self.params.additional_fee_contribution: {:?}", self.params.additional_fee_contribution @@ -688,7 +759,11 @@ mod test { .require_network(network) .unwrap()) }) - .expect("Receiver output should be identified"); + .expect("Receiver output should be identified") + .try_substitute_receiver_outputs(None) + .expect("Substitute outputs should do nothing") + .skip_contribute_inputs(); // TODO: temporary workaround + let payjoin = payjoin.apply_fee(None); assert!(payjoin.is_ok(), "Payjoin should be a valid PSBT"); diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index ec9cd819..d7c83a1e 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -383,20 +383,50 @@ impl OutputsUnknown { pub fn identify_receiver_outputs( self, is_receiver_output: impl Fn(&Script) -> Result, - ) -> Result { + ) -> Result { let inner = self.inner.identify_receiver_outputs(is_receiver_output)?; - Ok(ProvisionalProposal { inner, context: self.context }) + Ok(WantsOutputs { inner, context: self.context }) } } -/// A mutable checked proposal that the receiver may contribute inputs to to make a payjoin. +/// A checked proposal that the receiver may substitute or add outputs to #[derive(Debug, Clone)] -pub struct ProvisionalProposal { - pub inner: super::ProvisionalProposal, +pub struct WantsOutputs { + inner: super::WantsOutputs, context: SessionContext, } -impl ProvisionalProposal { +impl WantsOutputs { + pub fn is_output_substitution_disabled(&self) -> bool { + self.inner.is_output_substitution_disabled() + } + + /// If output substitution is enabled, replace the receiver's output script with a new one. + pub fn try_substitute_receiver_output( + self, + generate_script: impl Fn() -> Result, + ) -> Result { + let inner = self.inner.try_substitute_receiver_output(generate_script)?; + Ok(WantsInputs { inner, context: self.context }) + } + + pub fn try_substitute_receiver_outputs( + self, + generate_outputs: Option>, + ) -> Result { + let inner = self.inner.try_substitute_receiver_outputs(generate_outputs)?; + Ok(WantsInputs { inner, context: self.context }) + } +} + +/// A checked proposal that the receiver may contribute inputs to to make a payjoin +#[derive(Debug, Clone)] +pub struct WantsInputs { + inner: super::WantsInputs, + context: SessionContext, +} + +impl WantsInputs { /// Select receiver input such that the payjoin avoids surveillance. /// Return the input chosen that has been applied to the Proposal. /// @@ -415,22 +445,21 @@ impl ProvisionalProposal { self.inner.try_preserving_privacy(candidate_inputs) } - pub fn contribute_witness_input(&mut self, txo: TxOut, outpoint: OutPoint) { - self.inner.contribute_witness_input(txo, outpoint) - } - - pub fn is_output_substitution_disabled(&self) -> bool { - self.inner.is_output_substitution_disabled() + pub fn contribute_witness_input(self, txo: TxOut, outpoint: OutPoint) -> ProvisionalProposal { + let inner = self.inner.contribute_witness_input(txo, outpoint); + ProvisionalProposal { inner, context: self.context } } +} - /// If output substitution is enabled, replace the receiver's output script with a new one. - pub fn try_substitute_receiver_output( - &mut self, - generate_script: impl Fn() -> Result, - ) -> Result<(), Error> { - self.inner.try_substitute_receiver_output(generate_script) - } +/// A checked proposal that the receiver may sign and finalize to make a proposal PSBT that the +/// sender will accept. +#[derive(Debug, Clone)] +pub struct ProvisionalProposal { + inner: super::ProvisionalProposal, + context: SessionContext, +} +impl ProvisionalProposal { pub fn finalize_proposal( self, wallet_process_psbt: impl Fn(&Psbt) -> Result, diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index f61f8757..a8cb6bd7 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -615,7 +615,7 @@ mod integration { 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. - let mut payjoin = proposal + let payjoin = proposal .check_no_inputs_seen_before(|_| Ok(false)) .unwrap() .identify_receiver_outputs(|output_script| { @@ -626,6 +626,10 @@ mod integration { }) .expect("Receiver should have at least one output"); + let payjoin = payjoin + .try_substitute_receiver_outputs(None) + .expect("Could not substitute 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 @@ -646,11 +650,9 @@ mod integration { }; let outpoint_to_contribute = bitcoin::OutPoint { txid: selected_utxo.txid, vout: selected_utxo.vout }; - payjoin.contribute_witness_input(txo_to_contribute, outpoint_to_contribute); + let payjoin = + payjoin.contribute_witness_input(txo_to_contribute, outpoint_to_contribute); - _ = payjoin.try_substitute_receiver_output(|| { - Ok(receiver.get_new_address(None, None).unwrap().assume_checked().script_pubkey()) - }); let payjoin_proposal = payjoin .finalize_proposal( |psbt: &Psbt| { @@ -873,6 +875,12 @@ mod integration { }) .expect("Receiver should have at least one output"); + let payjoin = payjoin + .try_substitute_receiver_output(|| { + Ok(receiver.get_new_address(None, None).unwrap().assume_checked().script_pubkey()) + }) + .expect("Could not substitute 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 @@ -893,11 +901,8 @@ mod integration { }; let outpoint_to_contribute = bitcoin::OutPoint { txid: selected_utxo.txid, vout: selected_utxo.vout }; - payjoin.contribute_witness_input(txo_to_contribute, outpoint_to_contribute); + let payjoin = payjoin.contribute_witness_input(txo_to_contribute, outpoint_to_contribute); - _ = payjoin.try_substitute_receiver_output(|| { - Ok(receiver.get_new_address(None, None).unwrap().assume_checked().script_pubkey()) - }); let payjoin_proposal = payjoin .finalize_proposal( |psbt: &Psbt| { From 4325e932597f6530d7ed6c6257785380a9ccc1a4 Mon Sep 17 00:00:00 2001 From: spacebear Date: Wed, 31 Jul 2024 22:58:31 -0400 Subject: [PATCH 2/9] Allow contributing multiple inputs `contribute_witness_input` is now `contribute_witness_inputs`. It takes an arbitrary number of inputs as a HashMap and returns a `ProvisionalProposal`. --- payjoin-cli/src/app/v1.rs | 5 +-- payjoin-cli/src/app/v2.rs | 5 +-- payjoin/src/receive/mod.rs | 83 ++++++++++++++++++++++------------- payjoin/src/receive/v2/mod.rs | 7 ++- payjoin/tests/integration.rs | 18 +++----- 5 files changed, 69 insertions(+), 49 deletions(-) diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index b03adac8..e61721e9 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -385,13 +385,12 @@ 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); - - // calculate receiver payjoin outputs given receiver payjoin inputs and original_psbt, let txo_to_contribute = bitcoin::TxOut { value: selected_utxo.amount, script_pubkey: selected_utxo.script_pub_key.clone(), }; - Ok(payjoin.contribute_witness_input(txo_to_contribute, selected_outpoint)) + + Ok(payjoin.contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)])) } fn full>(chunk: T) -> BoxBody { diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index 8ba7ba00..707ad604 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -367,13 +367,12 @@ 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); - - // calculate receiver payjoin outputs given receiver payjoin inputs and original_psbt, let txo_to_contribute = bitcoin::TxOut { value: selected_utxo.amount, script_pubkey: selected_utxo.script_pub_key.clone(), }; - Ok(payjoin.contribute_witness_input(txo_to_contribute, selected_outpoint)) + + Ok(payjoin.contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)])) } async fn unwrap_ohttp_keys_or_else_fetch(config: &AppConfig) -> Result { diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 2e1dd9e9..8a67c34d 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -485,10 +485,13 @@ impl WantsInputs { .values() .next() .cloned() - .ok_or_else(|| SelectionError::from(InternalSelectionError::NotFound)) + .ok_or(SelectionError::from(InternalSelectionError::NotFound)) } - pub fn contribute_witness_input(self, txo: TxOut, outpoint: OutPoint) -> ProvisionalProposal { + pub fn contribute_witness_inputs( + self, + inputs: impl IntoIterator, + ) -> ProvisionalProposal { let mut payjoin_psbt = self.payjoin_psbt.clone(); // The payjoin proposal must not introduce mixed input sequence numbers let original_sequence = self @@ -499,26 +502,39 @@ impl WantsInputs { .map(|input| input.sequence) .unwrap_or_default(); - // Add the value of new receiver input to receiver output - let txo_value = txo.value; - let vout_to_augment = - self.owned_vouts.choose(&mut rand::thread_rng()).expect("owned_vouts is empty"); - payjoin_psbt.unsigned_tx.output[*vout_to_augment].value += txo_value; - - // Insert contribution at random index for privacy + // Insert contributions at random indices for privacy let mut rng = rand::thread_rng(); - 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() - }, - ); + let mut receiver_input_amount = Amount::ZERO; + for (outpoint, txo) in inputs.into_iter() { + receiver_input_amount += txo.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() + }, + ); + } + + // Add the receiver change amount to the receiver change output, if applicable + let receiver_min_input_amount = self.receiver_min_input_amount(); + if receiver_input_amount >= receiver_min_input_amount { + let change_amount = receiver_input_amount - receiver_min_input_amount; + // TODO: ensure that owned_vouts only refers to outpoints actually owned by the + // receiver (e.g. not a forwarded payment) + let vout_to_augment = + self.owned_vouts.choose(&mut rand::thread_rng()).expect("owned_vouts is empty"); + payjoin_psbt.unsigned_tx.output[*vout_to_augment].value += change_amount; + } else { + todo!("Return an error?"); + } + ProvisionalProposal { original_psbt: self.original_psbt, payjoin_psbt, @@ -527,14 +543,21 @@ impl WantsInputs { } } - // TODO: temporary workaround - fn skip_contribute_inputs(self) -> ProvisionalProposal { - ProvisionalProposal { - original_psbt: self.original_psbt, - payjoin_psbt: self.payjoin_psbt, - params: self.params, - owned_vouts: self.owned_vouts, - } + // 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 + .payjoin_psbt + .unsigned_tx + .output + .iter() + .fold(Amount::ZERO, |acc, output| acc + output.value); + let original_output_amount = self + .original_psbt + .unsigned_tx + .output + .iter() + .fold(Amount::ZERO, |acc, output| acc + output.value); + max(Amount::ZERO, output_amount - original_output_amount) } } @@ -762,7 +785,7 @@ mod test { .expect("Receiver output should be identified") .try_substitute_receiver_outputs(None) .expect("Substitute outputs should do nothing") - .skip_contribute_inputs(); // TODO: temporary workaround + .contribute_witness_inputs(vec![]); let payjoin = payjoin.apply_fee(None); diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index d7c83a1e..32404648 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -445,8 +445,11 @@ impl WantsInputs { self.inner.try_preserving_privacy(candidate_inputs) } - pub fn contribute_witness_input(self, txo: TxOut, outpoint: OutPoint) -> ProvisionalProposal { - let inner = self.inner.contribute_witness_input(txo, outpoint); + pub fn contribute_witness_inputs( + self, + inputs: impl IntoIterator, + ) -> ProvisionalProposal { + let inner = self.inner.contribute_witness_inputs(inputs); ProvisionalProposal { inner, context: self.context } } } diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index a8cb6bd7..62763a69 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -642,17 +642,15 @@ mod integration { .iter() .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout) .unwrap(); - - // calculate receiver payjoin outputs given receiver payjoin inputs and original_psbt, let txo_to_contribute = bitcoin::TxOut { value: selected_utxo.amount, script_pubkey: selected_utxo.script_pub_key.clone(), }; - let outpoint_to_contribute = - bitcoin::OutPoint { txid: selected_utxo.txid, vout: selected_utxo.vout }; + let payjoin = - payjoin.contribute_witness_input(txo_to_contribute, outpoint_to_contribute); + payjoin.contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]); + // Sign and finalize the proposal PSBT let payjoin_proposal = payjoin .finalize_proposal( |psbt: &Psbt| { @@ -864,7 +862,7 @@ mod integration { 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. - let mut payjoin = proposal + let payjoin = proposal .check_no_inputs_seen_before(|_| Ok(false)) .unwrap() .identify_receiver_outputs(|output_script| { @@ -893,15 +891,13 @@ mod integration { .iter() .find(|i| i.txid == selected_outpoint.txid && i.vout == selected_outpoint.vout) .unwrap(); - - // calculate receiver payjoin outputs given receiver payjoin inputs and original_psbt, let txo_to_contribute = bitcoin::TxOut { value: selected_utxo.amount, script_pubkey: selected_utxo.script_pub_key.clone(), }; - let outpoint_to_contribute = - bitcoin::OutPoint { txid: selected_utxo.txid, vout: selected_utxo.vout }; - let payjoin = payjoin.contribute_witness_input(txo_to_contribute, outpoint_to_contribute); + + let payjoin = + payjoin.contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]); let payjoin_proposal = payjoin .finalize_proposal( From 4062b567fe011c3f915d7b7f560c8eb43d939fd0 Mon Sep 17 00:00:00 2001 From: spacebear Date: Wed, 14 Aug 2024 12:01:56 -0400 Subject: [PATCH 3/9] Implement disable_output_substitution check correctly When disable_output_substitition is true, the receiver may not change the script pubkey or decrease the value of their original output, but is allowed to increase the amount or add new outputs. --- payjoin/src/receive/mod.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 8a67c34d..f139bb82 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -360,8 +360,22 @@ impl WantsOutputs { match outputs { Some(o) => { if self.params.disable_output_substitution { - // TODO: only fail if the original output's amount decreased or its script pubkey is not in `outputs` - return Err(Error::Server("Output substitution is disabled.".into())); + // Receiver may not change the script pubkey or decrease the value of the + // their output, but can still increase the amount or add new outputs. + // https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#payment-output-substitution + let original_output = &self.psbt.unsigned_tx.output[self.owned_vouts[0]]; + match o.iter().find(|txo| txo.script_pubkey == original_output.script_pubkey) { + Some(txo) if txo.value < original_output.value => { + return Err(Error::Server( + "Decreasing the receiver output value is not allowed".into(), + )); + } + None => + return Err(Error::Server( + "Changing the receiver output script pubkey is not allowed".into(), + )), + _ => log::info!("Receiver is augmenting outputs"), + } } let mut replacement_outputs = o.into_iter(); let mut outputs = vec![]; From 5f9ca539f2fdc50f6961eccb97de4e63a7778f28 Mon Sep 17 00:00:00 2001 From: spacebear Date: Wed, 14 Aug 2024 11:40:40 -0400 Subject: [PATCH 4/9] Use a builder pattern in WantsOutputs and WantsInputs - Output substitution functions return a modified WantsOutputs instead of a WantsInputs. `freeze_outputs` is used to proceed to the WantsInputs step. - Similarly, contribute_witness_inputs returns a modified WantsInputs and `freeze_inputs` returns the ProvisionalProposal - One change output (aka drain script) must be identified if outputs are replaced. The change output receives all excess input amount from the receiver, and will be responsible for paying receiver fees if applicable. --- payjoin-cli/src/app/v1.rs | 33 +++-- payjoin-cli/src/app/v2.rs | 35 +++--- payjoin/src/receive/mod.rs | 228 +++++++++++++++++++++------------- payjoin/src/receive/v2/mod.rs | 51 +++++--- payjoin/tests/integration.rs | 23 ++-- 5 files changed, 227 insertions(+), 143 deletions(-) diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index e61721e9..55a69864 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -329,7 +329,7 @@ impl App { })?; log::trace!("check4"); - let provisional_payjoin = payjoin.identify_receiver_outputs(|output_script| { + let payjoin = payjoin.identify_receiver_outputs(|output_script| { if let Ok(address) = bitcoin::Address::from_script(output_script, network) { bitcoind .get_address_info(&address) @@ -340,17 +340,22 @@ impl App { } })?; - let provisional_payjoin = provisional_payjoin.try_substitute_receiver_output(|| { - Ok(bitcoind - .get_new_address(None, None) - .map_err(|e| Error::Server(e.into()))? - .require_network(network) - .map_err(|e| Error::Server(e.into()))? - .script_pubkey()) - })?; - - let provisional_payjoin = try_contributing_inputs(provisional_payjoin, &bitcoind) - .map_err(|e| Error::Server(e.into()))?; + let payjoin = payjoin + .substitute_receiver_script( + &bitcoind + .get_new_address(None, None) + .map_err(|e| Error::Server(e.into()))? + .require_network(network) + .map_err(|e| Error::Server(e.into()))? + .script_pubkey(), + )? + .commit_outputs(); + + let provisional_payjoin = try_contributing_inputs(payjoin.clone(), &bitcoind) + .unwrap_or_else(|e| { + log::warn!("Failed to contribute inputs: {}", e); + payjoin.commit_inputs() + }); let payjoin_proposal = provisional_payjoin.finalize_proposal( |psbt: &Psbt| { @@ -390,7 +395,9 @@ fn try_contributing_inputs( script_pubkey: selected_utxo.script_pub_key.clone(), }; - Ok(payjoin.contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)])) + Ok(payjoin + .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .commit_inputs()) } fn full>(chunk: T) -> BoxBody { diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index 707ad604..b4732677 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -316,21 +316,24 @@ impl App { })?; log::trace!("check4"); - let provisional_payjoin = payjoin.identify_receiver_outputs(|output_script| { - if let Ok(address) = bitcoin::Address::from_script(output_script, network) { - bitcoind - .get_address_info(&address) - .map(|info| info.is_mine.unwrap_or(false)) - .map_err(|e| Error::Server(e.into())) - } else { - Ok(false) - } - })?; - - let provisional_payjoin = provisional_payjoin.try_substitute_receiver_outputs(None)?; + let payjoin = payjoin + .identify_receiver_outputs(|output_script| { + if let Ok(address) = bitcoin::Address::from_script(output_script, network) { + bitcoind + .get_address_info(&address) + .map(|info| info.is_mine.unwrap_or(false)) + .map_err(|e| Error::Server(e.into())) + } else { + Ok(false) + } + })? + .commit_outputs(); - let provisional_payjoin = try_contributing_inputs(provisional_payjoin, &bitcoind) - .map_err(|e| Error::Server(e.into()))?; + let provisional_payjoin = try_contributing_inputs(payjoin.clone(), &bitcoind) + .unwrap_or_else(|e| { + log::warn!("Failed to contribute inputs: {}", e); + payjoin.commit_inputs() + }); let payjoin_proposal = provisional_payjoin.finalize_proposal( |psbt: &Psbt| { @@ -372,7 +375,9 @@ fn try_contributing_inputs( script_pubkey: selected_utxo.script_pub_key.clone(), }; - Ok(payjoin.contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)])) + Ok(payjoin + .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .commit_inputs()) } async fn unwrap_ohttp_keys_or_else_fetch(config: &AppConfig) -> Result { diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index f139bb82..0f8338d5 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -37,7 +37,6 @@ mod optional_parameters; #[cfg(feature = "v2")] pub mod v2; -use bitcoin::secp256k1::rand::seq::SliceRandom; use bitcoin::secp256k1::rand::{self, Rng}; pub use error::{Error, RequestError, SelectionError}; use error::{InternalRequestError, InternalSelectionError}; @@ -325,15 +324,32 @@ impl OutputsUnknown { return Err(Error::BadRequest(InternalRequestError::MissingPayment.into())); } - Ok(WantsOutputs { psbt: self.psbt, params: self.params, owned_vouts }) + let mut params = self.params.clone(); + if let Some((_, additional_fee_output_index)) = params.additional_fee_contribution { + // If the additional fee output index specified by the sender is pointing to a receiver output, + // the receiver should ignore the parameter. + if owned_vouts.contains(&additional_fee_output_index) { + params.additional_fee_contribution = None; + } + } + + Ok(WantsOutputs { + original_psbt: self.psbt.clone(), + payjoin_psbt: self.psbt, + params, + change_vout: owned_vouts[0], + owned_vouts, + }) } } /// A checked proposal that the receiver may substitute or add outputs to #[derive(Debug, Clone)] pub struct WantsOutputs { - psbt: Psbt, + original_psbt: Psbt, + payjoin_psbt: Psbt, params: Params, + change_vout: usize, owned_vouts: Vec, } @@ -342,71 +358,98 @@ impl WantsOutputs { self.params.disable_output_substitution } - /// If output substitution is enabled, replace the receiver's output script with a new one. - pub fn try_substitute_receiver_output( - self, - generate_script: impl Fn() -> Result, - ) -> Result { - let output_value = self.psbt.unsigned_tx.output[self.owned_vouts[0]].value; - let outputs = vec![TxOut { value: output_value, script_pubkey: generate_script()? }]; - self.try_substitute_receiver_outputs(Some(outputs)) + /// Substitute the receiver output script with the provided script. + pub fn substitute_receiver_script(self, output_script: &Script) -> Result { + let output_value = self.original_psbt.unsigned_tx.output[self.change_vout].value; + let outputs = vec![TxOut { value: output_value, script_pubkey: output_script.into() }]; + self.replace_receiver_outputs(outputs, output_script) } - pub fn try_substitute_receiver_outputs( + /// Replace **all** receiver outputs with one or more provided outputs. + /// The drain script specifies which address to *drain* coins to. An output corresponding to + /// that address must be included in `replacement_outputs`. The value of that output may be + /// increased or decreased depending on the receiver's input contributions and whether the + /// receiver needs to pay for additional miner fees (e.g. in the case of adding many outputs). + pub fn replace_receiver_outputs( self, - outputs: Option>, - ) -> Result { - let mut payjoin_psbt = self.psbt.clone(); - match outputs { - Some(o) => { - if self.params.disable_output_substitution { - // Receiver may not change the script pubkey or decrease the value of the - // their output, but can still increase the amount or add new outputs. - // https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#payment-output-substitution - let original_output = &self.psbt.unsigned_tx.output[self.owned_vouts[0]]; - match o.iter().find(|txo| txo.script_pubkey == original_output.script_pubkey) { - Some(txo) if txo.value < original_output.value => { - return Err(Error::Server( - "Decreasing the receiver output value is not allowed".into(), - )); - } - None => - return Err(Error::Server( - "Changing the receiver output script pubkey is not allowed".into(), - )), - _ => log::info!("Receiver is augmenting outputs"), - } + replacement_outputs: Vec, + drain_script: &Script, + ) -> Result { + let mut payjoin_psbt = self.original_psbt.clone(); + let mut change_vout: Option = None; + if self.params.disable_output_substitution { + // Receiver may not change the script pubkey or decrease the value of the + // their output, but can still increase the amount or add new outputs. + // https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#payment-output-substitution + let original_output = &self.original_psbt.unsigned_tx.output[self.owned_vouts[0]]; + match replacement_outputs + .iter() + .find(|txo| txo.script_pubkey == original_output.script_pubkey) + { + Some(txo) if txo.value < original_output.value => { + return Err(Error::Server( + "Decreasing the receiver output value is not allowed".into(), + )); } - let mut replacement_outputs = o.into_iter(); - let mut outputs = vec![]; - for (i, output) in self.psbt.unsigned_tx.output.iter().enumerate() { - if self.owned_vouts.contains(&i) { - // Receiver output: substitute with a provided output - // TODO: pick from outputs in random order? - outputs.push( - replacement_outputs - .next() - .ok_or(Error::Server("Not enough outputs".into()))?, - ); - } else { - // Sender output: leave it as is - outputs.push(output.clone()); - } + None => + return Err(Error::Server( + "Changing the receiver output script pubkey is not allowed".into(), + )), + _ => log::info!("Receiver is augmenting outputs"), + } + } + let mut outputs = vec![]; + let mut replacement_outputs = replacement_outputs.clone(); + let mut rng = rand::thread_rng(); + // Replace the existing receiver outputs + for (i, output) in self.original_psbt.unsigned_tx.output.iter().enumerate() { + if self.owned_vouts.contains(&i) { + // Receiver output: substitute with a provided output picked randomly + if replacement_outputs.is_empty() { + return Err(Error::Server("Not enough outputs".into())); + } + let index = rng.gen_range(0..replacement_outputs.len()); + let txo = replacement_outputs.swap_remove(index); + if *drain_script == txo.script_pubkey { + change_vout = Some(i); } - // Append all remaining outputs - outputs.extend(replacement_outputs); - payjoin_psbt.unsigned_tx.output = outputs; - // TODO: update self.owned_vouts? + outputs.push(txo); + } else { + // Sender output: leave it as is + outputs.push(output.clone()); } - None => log::info!("No outputs provided: skipping output substitution."), } - Ok(WantsInputs { - original_psbt: self.psbt, + // Append all remaining outputs + while !replacement_outputs.is_empty() { + let index = rng.gen_range(0..replacement_outputs.len()); + let txo = replacement_outputs.swap_remove(index); + if *drain_script == txo.script_pubkey { + change_vout = Some(outputs.len()); + } + outputs.push(txo); + // Additional outputs must also be added to the PSBT outputs data structure + payjoin_psbt.outputs.push(Default::default()); + } + payjoin_psbt.unsigned_tx.output = outputs; + Ok(WantsOutputs { + original_psbt: self.original_psbt, payjoin_psbt, params: self.params, + change_vout: change_vout.ok_or(Error::Server("Invalid drain script".into()))?, owned_vouts: self.owned_vouts, }) } + + /// Proceed to the input contribution step. + /// Outputs cannot be modified after this function is called. + pub fn commit_outputs(self) -> WantsInputs { + WantsInputs { + original_psbt: self.original_psbt, + payjoin_psbt: self.payjoin_psbt, + params: self.params, + change_vout: self.change_vout, + } + } } /// A checked proposal that the receiver may contribute inputs to to make a payjoin @@ -415,7 +458,7 @@ pub struct WantsInputs { original_psbt: Psbt, payjoin_psbt: Psbt, params: Params, - owned_vouts: Vec, + change_vout: usize, } impl WantsInputs { @@ -451,8 +494,8 @@ impl WantsInputs { /// UIH "Unnecessary input heuristic" is one class of heuristics to avoid. We define /// UIH1 and UIH2 according to the BlockSci practice /// BlockSci UIH1 and UIH2: - // if min(in) > min(out) then UIH1 else UIH2 - // https://eprint.iacr.org/2022/589.pdf + /// if min(in) > min(out) then UIH1 else UIH2 + /// https://eprint.iacr.org/2022/589.pdf fn avoid_uih( &self, candidate_inputs: HashMap, @@ -464,16 +507,16 @@ impl WantsInputs { .iter() .map(|output| output.value) .min() - .unwrap_or_else(|| Amount::MAX_MONEY); + .unwrap_or(Amount::MAX_MONEY); let min_original_in_sats = self .payjoin_psbt .input_pairs() .filter_map(|input| input.previous_txout().ok().map(|txo| txo.value)) .min() - .unwrap_or_else(|| Amount::MAX_MONEY); + .unwrap_or(Amount::MAX_MONEY); - let prior_payment_sats = self.payjoin_psbt.unsigned_tx.output[self.owned_vouts[0]].value; + let prior_payment_sats = self.payjoin_psbt.unsigned_tx.output[self.change_vout].value; for candidate in candidate_inputs { let candidate_sats = candidate.0; @@ -502,10 +545,12 @@ impl WantsInputs { .ok_or(SelectionError::from(InternalSelectionError::NotFound)) } + /// 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( self, inputs: impl IntoIterator, - ) -> ProvisionalProposal { + ) -> WantsInputs { let mut payjoin_psbt = self.payjoin_psbt.clone(); // The payjoin proposal must not introduce mixed input sequence numbers let original_sequence = self @@ -540,20 +585,16 @@ impl WantsInputs { let receiver_min_input_amount = self.receiver_min_input_amount(); if receiver_input_amount >= receiver_min_input_amount { let change_amount = receiver_input_amount - receiver_min_input_amount; - // TODO: ensure that owned_vouts only refers to outpoints actually owned by the - // receiver (e.g. not a forwarded payment) - let vout_to_augment = - self.owned_vouts.choose(&mut rand::thread_rng()).expect("owned_vouts is empty"); - payjoin_psbt.unsigned_tx.output[*vout_to_augment].value += change_amount; + payjoin_psbt.unsigned_tx.output[self.change_vout].value += change_amount; } else { todo!("Return an error?"); } - ProvisionalProposal { + WantsInputs { original_psbt: self.original_psbt, payjoin_psbt, params: self.params, - owned_vouts: self.owned_vouts, + change_vout: self.change_vout, } } @@ -573,6 +614,17 @@ impl WantsInputs { .fold(Amount::ZERO, |acc, output| acc + output.value); max(Amount::ZERO, output_amount - original_output_amount) } + + /// Proceed to the proposal finalization step. + /// Inputs cannot be modified after this function is called. + pub fn commit_inputs(self) -> ProvisionalProposal { + ProvisionalProposal { + original_psbt: self.original_psbt, + payjoin_psbt: self.payjoin_psbt, + params: self.params, + change_vout: self.change_vout, + } + } } /// A checked proposal that the receiver may sign and finalize to make a proposal PSBT that the @@ -582,7 +634,7 @@ pub struct ProvisionalProposal { original_psbt: Psbt, payjoin_psbt: Psbt, params: Params, - owned_vouts: Vec, + change_vout: usize, } impl ProvisionalProposal { @@ -621,11 +673,21 @@ impl ProvisionalProposal { ); if let Some((_, additional_fee_output_index)) = self.params.additional_fee_contribution { - if !self.owned_vouts.contains(&additional_fee_output_index) { - // remove additional miner fee from the sender's specified output - self.payjoin_psbt.unsigned_tx.output[additional_fee_output_index].value -= - additional_fee; - } + // Find the sender's specified output in the original psbt. + // This step is necessary because the sender output may have shifted if new + // receiver outputs were added to the payjoin psbt. + let sender_fee_output = + &self.original_psbt.unsigned_tx.output[additional_fee_output_index]; + // Find the index of that output in the payjoin psbt + let sender_fee_vout = self + .payjoin_psbt + .unsigned_tx + .output + .iter() + .position(|txo| txo.script_pubkey == sender_fee_output.script_pubkey) + .expect("Sender output is missing from payjoin PSBT"); + // Remove additional miner fee from the sender's specified output + self.payjoin_psbt.unsigned_tx.output[sender_fee_vout].value -= additional_fee; } } Ok(&self.payjoin_psbt) @@ -660,11 +722,7 @@ impl ProvisionalProposal { self.payjoin_psbt.inputs[i].tap_key_sig = None; } - Ok(PayjoinProposal { - payjoin_psbt: self.payjoin_psbt, - owned_vouts: self.owned_vouts, - params: self.params, - }) + Ok(PayjoinProposal { payjoin_psbt: self.payjoin_psbt, params: self.params }) } fn sender_input_indexes(&self) -> Vec { @@ -710,7 +768,6 @@ impl ProvisionalProposal { pub struct PayjoinProposal { payjoin_psbt: Psbt, params: Params, - owned_vouts: Vec, } impl PayjoinProposal { @@ -722,8 +779,6 @@ impl PayjoinProposal { self.params.disable_output_substitution } - pub fn owned_vouts(&self) -> &Vec { &self.owned_vouts } - pub fn psbt(&self) -> &Psbt { &self.payjoin_psbt } } @@ -797,9 +852,8 @@ mod test { .unwrap()) }) .expect("Receiver output should be identified") - .try_substitute_receiver_outputs(None) - .expect("Substitute outputs should do nothing") - .contribute_witness_inputs(vec![]); + .commit_outputs() + .commit_inputs(); let payjoin = payjoin.apply_fee(None); diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index 32404648..445ca497 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -401,21 +401,31 @@ impl WantsOutputs { self.inner.is_output_substitution_disabled() } - /// If output substitution is enabled, replace the receiver's output script with a new one. - pub fn try_substitute_receiver_output( - self, - generate_script: impl Fn() -> Result, - ) -> Result { - let inner = self.inner.try_substitute_receiver_output(generate_script)?; - Ok(WantsInputs { inner, context: self.context }) + /// Substitute the receiver output script with the provided script. + pub fn substitute_receiver_script(self, output_script: &Script) -> Result { + let inner = self.inner.substitute_receiver_script(output_script)?; + Ok(WantsOutputs { inner, context: self.context }) } - pub fn try_substitute_receiver_outputs( + /// Replace **all** receiver outputs with one or more provided outputs. + /// The drain script specifies which address to *drain* coins to. An output corresponding to + /// that address must be included in `replacement_outputs`. The value of that output may be + /// increased or decreased depending on the receiver's input contributions and whether the + /// receiver needs to pay for additional miner fees (e.g. in the case of adding many outputs). + pub fn replace_receiver_outputs( self, - generate_outputs: Option>, - ) -> Result { - let inner = self.inner.try_substitute_receiver_outputs(generate_outputs)?; - Ok(WantsInputs { inner, context: self.context }) + replacement_outputs: Vec, + drain_script: &Script, + ) -> Result { + let inner = self.inner.replace_receiver_outputs(replacement_outputs, drain_script)?; + Ok(WantsOutputs { inner, context: self.context }) + } + + /// Proceed to the input contribution step. + /// Outputs cannot be modified after this function is called. + pub fn commit_outputs(self) -> WantsInputs { + let inner = self.inner.commit_outputs(); + WantsInputs { inner, context: self.context } } } @@ -436,8 +446,8 @@ impl WantsInputs { /// UIH "Unnecessary input heuristic" is one class of them to avoid. We define /// UIH1 and UIH2 according to the BlockSci practice /// BlockSci UIH1 and UIH2: - // if min(in) > min(out) then UIH1 else UIH2 - // https://eprint.iacr.org/2022/589.pdf + /// if min(in) > min(out) then UIH1 else UIH2 + /// https://eprint.iacr.org/2022/589.pdf pub fn try_preserving_privacy( &self, candidate_inputs: HashMap, @@ -445,11 +455,20 @@ impl WantsInputs { self.inner.try_preserving_privacy(candidate_inputs) } + /// 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( self, inputs: impl IntoIterator, - ) -> ProvisionalProposal { + ) -> WantsInputs { let inner = self.inner.contribute_witness_inputs(inputs); + WantsInputs { inner, context: self.context } + } + + /// Proceed to the proposal finalization step. + /// Inputs cannot be modified after this function is called. + pub fn commit_inputs(self) -> ProvisionalProposal { + let inner = self.inner.commit_inputs(); ProvisionalProposal { inner, context: self.context } } } @@ -489,8 +508,6 @@ impl PayjoinProposal { self.inner.is_output_substitution_disabled() } - pub fn owned_vouts(&self) -> &Vec { self.inner.owned_vouts() } - pub fn psbt(&self) -> &Psbt { self.inner.psbt() } pub fn extract_v1_req(&self) -> String { self.inner.payjoin_psbt.to_string() } diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 62763a69..66c73374 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -626,9 +626,7 @@ mod integration { }) .expect("Receiver should have at least one output"); - let payjoin = payjoin - .try_substitute_receiver_outputs(None) - .expect("Could not substitute outputs"); + let payjoin = payjoin.commit_outputs(); // Select receiver payjoin inputs. TODO Lock them. let available_inputs = receiver.list_unspent(None, None, None, None, None).unwrap(); @@ -647,8 +645,9 @@ mod integration { script_pubkey: selected_utxo.script_pub_key.clone(), }; - let payjoin = - payjoin.contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]); + let payjoin = payjoin + .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .commit_inputs(); // Sign and finalize the proposal PSBT let payjoin_proposal = payjoin @@ -874,10 +873,11 @@ mod integration { .expect("Receiver should have at least one output"); let payjoin = payjoin - .try_substitute_receiver_output(|| { - Ok(receiver.get_new_address(None, None).unwrap().assume_checked().script_pubkey()) - }) - .expect("Could not substitute outputs"); + .substitute_receiver_script( + &receiver.get_new_address(None, None).unwrap().assume_checked().script_pubkey(), + ) + .expect("Could not substitute outputs") + .commit_outputs(); // Select receiver payjoin inputs. TODO Lock them. let available_inputs = receiver.list_unspent(None, None, None, None, None).unwrap(); @@ -896,8 +896,9 @@ mod integration { script_pubkey: selected_utxo.script_pub_key.clone(), }; - let payjoin = - payjoin.contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]); + let payjoin = payjoin + .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .commit_inputs(); let payjoin_proposal = payjoin .finalize_proposal( From d6295983ea218569940b9a8fffcf3c456c4ebdf1 Mon Sep 17 00:00:00 2001 From: spacebear Date: Mon, 19 Aug 2024 22:06:25 -0400 Subject: [PATCH 5/9] Implement receiver fee contributions Modify `apply_fee` to enable fee contributions from the receiver for fees not covered by the sender. This includes - any fees that pay for additional inputs, in excess of the sender's max_additional_fee_contribution. - all fees that pay for additional *outputs*. - in the case where the sender doesn't have a fee output (e.g. a sweep transaction), all tx fees are deducted from the receiver output. --- payjoin/src/receive/mod.rs | 96 +++++++++++++++++++++++++++--------- payjoin/tests/integration.rs | 11 ++--- 2 files changed, 78 insertions(+), 29 deletions(-) diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 0f8338d5..e8697899 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -30,7 +30,7 @@ use std::collections::HashMap; use bitcoin::base64::prelude::BASE64_STANDARD; use bitcoin::base64::Engine; use bitcoin::psbt::Psbt; -use bitcoin::{Amount, FeeRate, OutPoint, Script, TxOut}; +use bitcoin::{Amount, FeeRate, OutPoint, Script, TxOut, Weight}; mod error; mod optional_parameters; @@ -647,31 +647,18 @@ impl ProvisionalProposal { let min_feerate = max(min_feerate, self.params.min_feerate); log::debug!("min_feerate: {:?}", min_feerate); - // 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)?; - let txo = input_pair.previous_txout().map_err(InternalRequestError::PrevTxOut)?; - let input_type = InputType::from_spent_input(txo, &self.payjoin_psbt.inputs[0]) - .map_err(InternalRequestError::InputType)?; - let contribution_weight = input_type.expected_input_weight(); - log::trace!("contribution_weight: {}", contribution_weight); - let mut additional_fee = contribution_weight * min_feerate; - let max_additional_fee_contribution = - self.params.additional_fee_contribution.unwrap_or_default().0; - if additional_fee >= max_additional_fee_contribution { - // Cap fee at the sender's contribution to simplify this method - additional_fee = max_additional_fee_contribution; - } - log::trace!("additional_fee: {}", additional_fee); + // If the sender specified a fee contribution, the receiver is allowed to decrease the + // sender's fee output to pay for additional input fees. Any fees in excess of + // `max_additional_fee_contribution` must be covered by the receiver. + let additional_fee = self.additional_input_fee(min_feerate)?; if additional_fee > Amount::ZERO { log::trace!( "self.params.additional_fee_contribution: {:?}", self.params.additional_fee_contribution ); - if let Some((_, additional_fee_output_index)) = self.params.additional_fee_contribution + let mut receiver_additional_fee = additional_fee; + if let Some((max_additional_fee_contribution, additional_fee_output_index)) = + self.params.additional_fee_contribution { // Find the sender's specified output in the original psbt. // This step is necessary because the sender output may have shifted if new @@ -686,13 +673,78 @@ impl ProvisionalProposal { .iter() .position(|txo| txo.script_pubkey == sender_fee_output.script_pubkey) .expect("Sender output is missing from payjoin PSBT"); + // Determine the additional amount that the sender will pay in fees + let sender_additional_fee = min(max_additional_fee_contribution, additional_fee); + log::trace!("sender_additional_fee: {}", sender_additional_fee); // Remove additional miner fee from the sender's specified output - self.payjoin_psbt.unsigned_tx.output[sender_fee_vout].value -= additional_fee; + self.payjoin_psbt.unsigned_tx.output[sender_fee_vout].value -= + sender_additional_fee; + receiver_additional_fee -= sender_additional_fee; } + + // The receiver covers any additional fees if applicable + if receiver_additional_fee > Amount::ZERO { + log::trace!("receiver_additional_fee: {}", receiver_additional_fee); + // Remove additional miner fee from the receiver's specified output + self.payjoin_psbt.unsigned_tx.output[self.change_vout].value -= + receiver_additional_fee; + } + } + + // The sender's fee contribution can only be used to pay for additional input weight, so + // any additional outputs must be paid for by the receiver. + let additional_receiver_fee = self.additional_output_fee(min_feerate); + if additional_receiver_fee > Amount::ZERO { + // Remove additional miner fee from the receiver's specified output + self.payjoin_psbt.unsigned_tx.output[self.change_vout].value -= additional_receiver_fee; } Ok(&self.payjoin_psbt) } + /// Calculate the additional fee required to pay for any additional inputs in the payjoin tx + fn additional_input_fee(&self, min_feerate: FeeRate) -> 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 fee contribution + let txo = input_pair.previous_txout().map_err(InternalRequestError::PrevTxOut)?; + let input_type = InputType::from_spent_input(txo, &self.payjoin_psbt.inputs[0]) + .map_err(InternalRequestError::InputType)?; + let input_count = self.payjoin_psbt.inputs.len() - self.original_psbt.inputs.len(); + log::trace!("input_count : {}", input_count); + let weight_per_input = input_type.expected_input_weight(); + log::trace!("weight_per_input : {}", weight_per_input); + let contribution_weight = weight_per_input * input_count as u64; + log::trace!("contribution_weight: {}", contribution_weight); + let additional_fee = contribution_weight * min_feerate; + log::trace!("additional_fee: {}", additional_fee); + Ok(additional_fee) + } + + /// Calculate the additional fee required to pay for any additional outputs in the payjoin tx + fn additional_output_fee(&self, min_feerate: FeeRate) -> Amount { + let payjoin_outputs_weight = self + .payjoin_psbt + .unsigned_tx + .output + .iter() + .fold(Weight::ZERO, |acc, txo| acc + txo.weight()); + let original_outputs_weight = self + .original_psbt + .unsigned_tx + .output + .iter() + .fold(Weight::ZERO, |acc, txo| acc + txo.weight()); + let output_contribution_weight = payjoin_outputs_weight - original_outputs_weight; + log::trace!("output_contribution_weight : {}", output_contribution_weight); + let additional_fee = output_contribution_weight * min_feerate; + log::trace!("additional_fee: {}", additional_fee); + additional_fee + } + /// 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, diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 66c73374..d3a63357 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -324,10 +324,7 @@ mod integration { log::info!("sent"); // Check resulting transaction and balances - // NOTE: No one is contributing fees for the receiver input because the sender has - // no change output and the receiver doesn't contribute fees. Temporary workaround - // is to ensure the sender overpays in the original psbt for the receiver's input. - let network_fees = psbt.fee()?; + 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(), 2); assert_eq!(payjoin_tx.output.len(), 1); @@ -719,9 +716,9 @@ mod integration { outputs.insert(pj_uri.address.to_string(), Amount::from_btc(50.0)?); let options = bitcoincore_rpc::json::WalletCreateFundedPsbtOptions { lock_unspent: Some(true), - // The current API doesn't let the receiver pay for additional fees, - // so we double the minimum relay fee to ensure that the sender pays for the receiver's inputs - fee_rate: Some(Amount::from_sat((DEFAULT_MIN_RELAY_TX_FEE * 2).into())), + // The minimum relay feerate ensures that tests fail if the receiver would add inputs/outputs + // that cannot be covered by the sender's additional fee contributions. + fee_rate: Some(Amount::from_sat(DEFAULT_MIN_RELAY_TX_FEE.into())), subtract_fee_from_outputs: vec![0], ..Default::default() }; From b73750011121a7eb7dc5ed99ce441d8f1d1a9d25 Mon Sep 17 00:00:00 2001 From: spacebear Date: Thu, 15 Aug 2024 18:35:22 -0400 Subject: [PATCH 6/9] Add batching tests Adds two integration tests for transaction cut-through: - Receiver UTXO consolidation - Receiver forwarding payment to a third-party Also fixes some issues in outputs and inputs contribution logic that became apparent while testing. --- payjoin/src/receive/mod.rs | 4 +- payjoin/tests/integration.rs | 266 +++++++++++++++++++++++++++++++---- 2 files changed, 239 insertions(+), 31 deletions(-) diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index e8697899..61b3b79c 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -587,7 +587,7 @@ impl WantsInputs { let change_amount = receiver_input_amount - receiver_min_input_amount; payjoin_psbt.unsigned_tx.output[self.change_vout].value += change_amount; } else { - todo!("Return an error?"); + todo!("Input amount is not enough to cover additional output value"); } WantsInputs { @@ -612,7 +612,7 @@ impl WantsInputs { .output .iter() .fold(Amount::ZERO, |acc, output| acc + output.value); - max(Amount::ZERO, output_amount - original_output_amount) + output_amount.checked_sub(original_output_amount).unwrap_or(Amount::ZERO) } /// Proceed to the proposal finalization step. diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index d3a63357..8dfc1ccf 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -6,7 +6,7 @@ mod integration { use bitcoin::policy::DEFAULT_MIN_RELAY_TX_FEE; use bitcoin::psbt::Psbt; - use bitcoin::{Amount, FeeRate, OutPoint, Weight}; + use bitcoin::{Amount, FeeRate, OutPoint, TxOut, Weight}; use bitcoind::bitcoincore_rpc::json::{AddressType, WalletProcessPsbtResult}; use bitcoind::bitcoincore_rpc::{self, RpcApi}; use log::{log_enabled, Level}; @@ -58,7 +58,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); + let response = handle_v1_pj_request(req, headers, &receiver, None, None, None); // this response would be returned as http response to the sender // ********************** @@ -364,7 +364,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); + let response = handle_v1_pj_request(req, headers, &receiver, None, None, None); // this response would be returned as http response to the sender // ********************** @@ -736,6 +736,197 @@ mod integration { } } + #[cfg(not(feature = "v2"))] + mod batching { + use payjoin::UriExt; + + use super::*; + + // In this test the receiver consolidates a bunch of UTXOs into the destination output + #[test] + fn receiver_consolidates_utxos() -> Result<(), BoxError> { + init_tracing(); + let (bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + // Generate more UTXOs for the receiver + let receiver_address = + receiver.get_new_address(None, Some(AddressType::Bech32))?.assume_checked(); + bitcoind.client.generate_to_address(199, &receiver_address)?; + let receiver_utxos = receiver.list_unspent(None, None, None, None, None).unwrap(); + assert_eq!(100, receiver_utxos.len(), "receiver doesn't have enough UTXOs"); + assert_eq!( + Amount::from_btc(3700.0)?, // 48*50.0 + 52*25.0 (halving occurs every 150 blocks) + receiver.get_balances()?.mine.trusted, + "receiver doesn't have enough bitcoin" + ); + + // 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)?; + log::debug!("Original psbt: {:#?}", psbt); + let max_additional_fee = Amount::from_sat(1000); + let (req, ctx) = RequestBuilder::from_psbt_and_uri(psbt.clone(), uri)? + .build_with_additional_fee(max_additional_fee, 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 outputs = vec![TxOut { + value: Amount::from_btc(3700.0)?, + script_pubkey: receiver + .get_new_address(None, None)? + .assume_checked() + .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 response = handle_v1_pj_request( + req, + headers, + &receiver, + Some(outputs), + Some(&drain_script), + Some(inputs), + ); + // this response would be returned as http response to the sender + + // ********************** + // Inside the Sender: + // Sender checks, signs, finalizes, extracts, and broadcasts + let checked_payjoin_proposal_psbt = ctx.process_response(&mut response.as_bytes())?; + let payjoin_tx = extract_pj_tx(&sender, checked_payjoin_proposal_psbt)?; + sender.send_raw_transaction(&payjoin_tx)?; + + // Check resulting transaction and balances + let network_fees = predicted_tx_weight(&payjoin_tx) * FeeRate::BROADCAST_MIN; + // The sender pays (original tx fee + max additional fee) + let original_tx_fee = psbt.fee()?; + let sender_fee = original_tx_fee + max_additional_fee; + // The receiver pays the difference + let receiver_fee = network_fees - sender_fee; + assert_eq!(payjoin_tx.input.len(), 101); + assert_eq!(payjoin_tx.output.len(), 2); + assert_eq!( + receiver.get_balances()?.mine.untrusted_pending, + Amount::from_btc(3701.0)? - receiver_fee + ); + assert_eq!( + sender.get_balances()?.mine.untrusted_pending, + Amount::from_btc(49.0)? - sender_fee + ); + Ok(()) + } + + // In this test the receiver forwards part of the sender payment to another payee + #[test] + fn receiver_forwards_payment() -> Result<(), BoxError> { + init_tracing(); + let (bitcoind, sender, receiver) = init_bitcoind_sender_receiver()?; + let third_party = bitcoind.create_wallet("third-party")?; + + // 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)?; + log::debug!("Original psbt: {:#?}", psbt); + let (req, ctx) = RequestBuilder::from_psbt_and_uri(psbt.clone(), 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 outputs = vec![ + TxOut { + value: Amount::from_sat(10000000), + script_pubkey: third_party + .get_new_address(None, None)? + .assume_checked() + .script_pubkey(), + }, + TxOut { + value: Amount::from_sat(90000000), + script_pubkey: receiver + .get_new_address(None, None)? + .assume_checked() + .script_pubkey(), + }, + ]; + let drain_script = outputs[1].script_pubkey.clone(); + let inputs = vec![]; + let response = handle_v1_pj_request( + req, + headers, + &receiver, + Some(outputs), + Some(&drain_script), + Some(inputs), + ); + // this response would be returned as http response to the sender + + // ********************** + // Inside the Sender: + // Sender checks, signs, finalizes, extracts, and broadcasts + let checked_payjoin_proposal_psbt = ctx.process_response(&mut response.as_bytes())?; + let payjoin_tx = extract_pj_tx(&sender, checked_payjoin_proposal_psbt)?; + sender.send_raw_transaction(&payjoin_tx)?; + + // Check resulting transaction and balances + let network_fees = predicted_tx_weight(&payjoin_tx) * FeeRate::BROADCAST_MIN; + // The sender pays original tx fee + let original_tx_fee = psbt.fee()?; + let sender_fee = original_tx_fee; + // The receiver pays the difference + let receiver_fee = network_fees - sender_fee; + assert_eq!(payjoin_tx.input.len(), 1); + assert_eq!(payjoin_tx.output.len(), 3); + assert_eq!( + receiver.get_balances()?.mine.untrusted_pending, + Amount::from_btc(0.9)? - receiver_fee + ); + assert_eq!(third_party.get_balances()?.mine.untrusted_pending, Amount::from_btc(0.1)?); + // sender balance is considered "trusted" because all inputs in the transaction were + // created by their wallet + assert_eq!(sender.get_balances()?.mine.trusted, Amount::from_btc(49.0)? - sender_fee); + Ok(()) + } + } + fn init_tracing() { INIT_TRACING.get_or_init(|| { let subscriber = FmtSubscriber::builder() @@ -811,6 +1002,9 @@ mod integration { req: Request, headers: impl payjoin::receive::Headers, receiver: &bitcoincore_rpc::Client, + custom_outputs: Option>, + drain_script: Option<&bitcoin::Script>, + 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( @@ -819,7 +1013,8 @@ mod integration { headers, ) .unwrap(); - let proposal = handle_proposal(proposal, receiver); + let proposal = + 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); @@ -829,6 +1024,9 @@ mod integration { fn handle_proposal( proposal: payjoin::receive::UncheckedProposal, receiver: &bitcoincore_rpc::Client, + custom_outputs: Option>, + drain_script: Option<&bitcoin::Script>, + 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(); @@ -869,33 +1067,43 @@ mod integration { }) .expect("Receiver should have at least one output"); - let payjoin = payjoin - .substitute_receiver_script( - &receiver.get_new_address(None, None).unwrap().assume_checked().script_pubkey(), - ) - .expect("Could not substitute outputs") - .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("gg"); - let selected_utxo = available_inputs - .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 = 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"), + } + .commit_outputs(); + + 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("gg"); + let selected_utxo = available_inputs + .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 payjoin = payjoin - .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) - .commit_inputs(); + let payjoin = payjoin.contribute_witness_inputs(inputs).commit_inputs(); let payjoin_proposal = payjoin .finalize_proposal( From f4f2f37bd58226524d8137b172f20e400bfb22a8 Mon Sep 17 00:00:00 2001 From: spacebear Date: Thu, 22 Aug 2024 18:09:56 -0400 Subject: [PATCH 7/9] Add custom error types for output and input contributions Partially addresses https://github.com/payjoin/rust-payjoin/issues/312 --- payjoin-cli/src/app/v1.rs | 4 +- payjoin-cli/src/app/v2.rs | 1 + payjoin/src/receive/error.rs | 71 +++++++++++++++++++++++++++++++++++ payjoin/src/receive/mod.rs | 52 +++++++++++++------------ payjoin/src/receive/v2/mod.rs | 18 ++++++--- payjoin/tests/integration.rs | 3 +- 6 files changed, 117 insertions(+), 32 deletions(-) diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index 55a69864..a5205717 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -348,7 +348,8 @@ impl App { .require_network(network) .map_err(|e| Error::Server(e.into()))? .script_pubkey(), - )? + ) + .map_err(|e| Error::Server(e.into()))? .commit_outputs(); let provisional_payjoin = try_contributing_inputs(payjoin.clone(), &bitcoind) @@ -397,6 +398,7 @@ fn try_contributing_inputs( Ok(payjoin .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .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 b4732677..00be2cd7 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -377,6 +377,7 @@ fn try_contributing_inputs( Ok(payjoin .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .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 6c7d21ea..07540f88 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -196,6 +196,51 @@ impl std::error::Error for RequestError { } } +/// Error that may occur when output substitution fails. +/// +/// This is currently opaque type because we aren't sure which variants will stay. +/// You can only display it. +#[derive(Debug)] +pub struct OutputSubstitutionError(InternalOutputSubstitutionError); + +#[derive(Debug)] +pub(crate) enum InternalOutputSubstitutionError { + /// Output substitution is disabled + OutputSubstitutionDisabled(&'static str), + /// Current output substitution implementation doesn't support reducing the number of outputs + NotEnoughOutputs, + /// The provided drain script could not be identified in the provided replacement outputs + InvalidDrainScript, +} + +impl fmt::Display for OutputSubstitutionError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match &self.0 { + InternalOutputSubstitutionError::OutputSubstitutionDisabled(reason) => write!(f, "{}", &format!("Output substitution is disabled: {}", reason)), + InternalOutputSubstitutionError::NotEnoughOutputs => write!( + f, + "Current output substitution implementation doesn't support reducing the number of outputs" + ), + InternalOutputSubstitutionError::InvalidDrainScript => + write!(f, "The provided drain script could not be identified in the provided replacement outputs"), + } + } +} + +impl From for OutputSubstitutionError { + fn from(value: InternalOutputSubstitutionError) -> Self { OutputSubstitutionError(value) } +} + +impl std::error::Error for OutputSubstitutionError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match &self.0 { + InternalOutputSubstitutionError::OutputSubstitutionDisabled(_) => None, + InternalOutputSubstitutionError::NotEnoughOutputs => None, + InternalOutputSubstitutionError::InvalidDrainScript => None, + } + } +} + /// Error that may occur when coin selection fails. /// /// This is currently opaque type because we aren't sure which variants will stay. @@ -230,3 +275,29 @@ impl fmt::Display for SelectionError { impl From for SelectionError { fn from(value: InternalSelectionError) -> Self { SelectionError(value) } } + +/// Error that may occur when input contribution fails. +/// +/// This is currently opaque type because we aren't sure which variants will stay. +/// You can only display it. +#[derive(Debug)] +pub struct InputContributionError(InternalInputContributionError); + +#[derive(Debug)] +pub(crate) enum InternalInputContributionError { + /// Total input value is not enough to cover additional output value + ValueTooLow, +} + +impl fmt::Display for InputContributionError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match &self.0 { + InternalInputContributionError::ValueTooLow => + write!(f, "Total input value is not enough to cover additional output value"), + } + } +} + +impl From for InputContributionError { + fn from(value: InternalInputContributionError) -> Self { InputContributionError(value) } +} diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 61b3b79c..aa9d47b0 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -38,8 +38,11 @@ mod optional_parameters; pub mod v2; use bitcoin::secp256k1::rand::{self, Rng}; -pub use error::{Error, RequestError, SelectionError}; -use error::{InternalRequestError, InternalSelectionError}; +pub use error::{Error, OutputSubstitutionError, RequestError, SelectionError}; +use error::{ + InputContributionError, InternalInputContributionError, InternalOutputSubstitutionError, + InternalRequestError, InternalSelectionError, +}; use optional_parameters::Params; use crate::input_type::InputType; @@ -359,7 +362,10 @@ impl WantsOutputs { } /// Substitute the receiver output script with the provided script. - pub fn substitute_receiver_script(self, output_script: &Script) -> Result { + pub fn substitute_receiver_script( + self, + output_script: &Script, + ) -> Result { let output_value = self.original_psbt.unsigned_tx.output[self.change_vout].value; let outputs = vec![TxOut { value: output_value, script_pubkey: output_script.into() }]; self.replace_receiver_outputs(outputs, output_script) @@ -374,7 +380,7 @@ impl WantsOutputs { self, replacement_outputs: Vec, drain_script: &Script, - ) -> Result { + ) -> Result { let mut payjoin_psbt = self.original_psbt.clone(); let mut change_vout: Option = None; if self.params.disable_output_substitution { @@ -387,14 +393,16 @@ impl WantsOutputs { .find(|txo| txo.script_pubkey == original_output.script_pubkey) { Some(txo) if txo.value < original_output.value => { - return Err(Error::Server( - "Decreasing the receiver output value is not allowed".into(), - )); + return Err(InternalOutputSubstitutionError::OutputSubstitutionDisabled( + "Decreasing the receiver output value is not allowed", + ) + .into()); } None => - return Err(Error::Server( - "Changing the receiver output script pubkey is not allowed".into(), - )), + return Err(InternalOutputSubstitutionError::OutputSubstitutionDisabled( + "Changing the receiver output script pubkey is not allowed", + ) + .into()), _ => log::info!("Receiver is augmenting outputs"), } } @@ -406,7 +414,7 @@ impl WantsOutputs { if self.owned_vouts.contains(&i) { // Receiver output: substitute with a provided output picked randomly if replacement_outputs.is_empty() { - return Err(Error::Server("Not enough outputs".into())); + return Err(InternalOutputSubstitutionError::NotEnoughOutputs.into()); } let index = rng.gen_range(0..replacement_outputs.len()); let txo = replacement_outputs.swap_remove(index); @@ -435,7 +443,7 @@ impl WantsOutputs { original_psbt: self.original_psbt, payjoin_psbt, params: self.params, - change_vout: change_vout.ok_or(Error::Server("Invalid drain script".into()))?, + change_vout: change_vout.ok_or(InternalOutputSubstitutionError::InvalidDrainScript)?, owned_vouts: self.owned_vouts, }) } @@ -475,13 +483,13 @@ impl WantsInputs { candidate_inputs: HashMap, ) -> Result { if candidate_inputs.is_empty() { - return Err(SelectionError::from(InternalSelectionError::Empty)); + return Err(InternalSelectionError::Empty.into()); } if self.payjoin_psbt.outputs.len() > 2 { // This UIH avoidance function supports only // many-input, n-output transactions such that n <= 2 for now - return Err(SelectionError::from(InternalSelectionError::TooManyOutputs)); + return Err(InternalSelectionError::TooManyOutputs.into()); } if self.payjoin_psbt.outputs.len() == 2 { @@ -531,18 +539,14 @@ impl WantsInputs { } // No suitable privacy preserving selection found - Err(SelectionError::from(InternalSelectionError::NotFound)) + Err(InternalSelectionError::NotFound.into()) } fn select_first_candidate( &self, candidate_inputs: HashMap, ) -> Result { - candidate_inputs - .values() - .next() - .cloned() - .ok_or(SelectionError::from(InternalSelectionError::NotFound)) + candidate_inputs.values().next().cloned().ok_or(InternalSelectionError::NotFound.into()) } /// Add the provided list of inputs to the transaction. @@ -550,7 +554,7 @@ impl WantsInputs { pub fn contribute_witness_inputs( self, inputs: impl IntoIterator, - ) -> WantsInputs { + ) -> Result { let mut payjoin_psbt = self.payjoin_psbt.clone(); // The payjoin proposal must not introduce mixed input sequence numbers let original_sequence = self @@ -587,15 +591,15 @@ impl WantsInputs { let change_amount = receiver_input_amount - receiver_min_input_amount; payjoin_psbt.unsigned_tx.output[self.change_vout].value += change_amount; } else { - todo!("Input amount is not enough to cover additional output value"); + return Err(InternalInputContributionError::ValueTooLow.into()); } - WantsInputs { + Ok(WantsInputs { original_psbt: self.original_psbt, payjoin_psbt, params: self.params, change_vout: self.change_vout, - } + }) } // Compute the minimum amount that the receiver must contribute to the transaction as input diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index 445ca497..a7482450 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -15,7 +15,10 @@ use serde::{Deserialize, Serialize, Serializer}; use url::Url; use super::v2::error::{InternalSessionError, SessionError}; -use super::{Error, InternalRequestError, RequestError, SelectionError}; +use super::{ + Error, InputContributionError, InternalRequestError, OutputSubstitutionError, RequestError, + SelectionError, +}; use crate::psbt::PsbtExt; use crate::receive::optional_parameters::Params; use crate::v2::OhttpEncapsulationError; @@ -402,7 +405,10 @@ impl WantsOutputs { } /// Substitute the receiver output script with the provided script. - pub fn substitute_receiver_script(self, output_script: &Script) -> Result { + pub fn substitute_receiver_script( + self, + output_script: &Script, + ) -> Result { let inner = self.inner.substitute_receiver_script(output_script)?; Ok(WantsOutputs { inner, context: self.context }) } @@ -416,7 +422,7 @@ impl WantsOutputs { self, replacement_outputs: Vec, drain_script: &Script, - ) -> Result { + ) -> Result { let inner = self.inner.replace_receiver_outputs(replacement_outputs, drain_script)?; Ok(WantsOutputs { inner, context: self.context }) } @@ -460,9 +466,9 @@ impl WantsInputs { pub fn contribute_witness_inputs( self, inputs: impl IntoIterator, - ) -> WantsInputs { - let inner = self.inner.contribute_witness_inputs(inputs); - WantsInputs { inner, context: self.context } + ) -> Result { + let inner = self.inner.contribute_witness_inputs(inputs)?; + Ok(WantsInputs { inner, context: self.context }) } /// Proceed to the proposal finalization step. diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 8dfc1ccf..eaab38ea 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -644,6 +644,7 @@ mod integration { let payjoin = payjoin .contribute_witness_inputs(vec![(selected_outpoint, txo_to_contribute)]) + .unwrap() .commit_inputs(); // Sign and finalize the proposal PSBT @@ -1103,7 +1104,7 @@ mod integration { } }; - let payjoin = payjoin.contribute_witness_inputs(inputs).commit_inputs(); + let payjoin = payjoin.contribute_witness_inputs(inputs).unwrap().commit_inputs(); let payjoin_proposal = payjoin .finalize_proposal( From ab9b8b8f6dc0c868d6e3afa1632be7e8f0e8f65f Mon Sep 17 00:00:00 2001 From: spacebear Date: Wed, 28 Aug 2024 12:59:42 -0400 Subject: [PATCH 8/9] Insert additional outputs in random indices Additional outputs should be inserted at random indices for privacy, while preserving the relative ordering of receiver/sender outputs from the original PSBT. This commit also ensures that `disable_output_substitution` checks are made for every receiver output, in the unlikely case that there are multiple receiver outputs in the original PSBT. --- payjoin/src/receive/mod.rs | 141 +++++++++++++++++++++++++------------ payjoin/src/send/mod.rs | 2 +- 2 files changed, 98 insertions(+), 45 deletions(-) diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index aa9d47b0..23039684 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -37,6 +37,7 @@ mod optional_parameters; #[cfg(feature = "v2")] pub mod v2; +use bitcoin::secp256k1::rand::seq::SliceRandom; use bitcoin::secp256k1::rand::{self, Rng}; pub use error::{Error, OutputSubstitutionError, RequestError, SelectionError}; use error::{ @@ -382,62 +383,61 @@ impl WantsOutputs { drain_script: &Script, ) -> Result { let mut payjoin_psbt = self.original_psbt.clone(); - let mut change_vout: Option = None; - if self.params.disable_output_substitution { - // Receiver may not change the script pubkey or decrease the value of the - // their output, but can still increase the amount or add new outputs. - // https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#payment-output-substitution - let original_output = &self.original_psbt.unsigned_tx.output[self.owned_vouts[0]]; - match replacement_outputs - .iter() - .find(|txo| txo.script_pubkey == original_output.script_pubkey) - { - Some(txo) if txo.value < original_output.value => { - return Err(InternalOutputSubstitutionError::OutputSubstitutionDisabled( - "Decreasing the receiver output value is not allowed", - ) - .into()); - } - None => - return Err(InternalOutputSubstitutionError::OutputSubstitutionDisabled( - "Changing the receiver output script pubkey is not allowed", - ) - .into()), - _ => log::info!("Receiver is augmenting outputs"), - } - } let mut outputs = vec![]; let mut replacement_outputs = replacement_outputs.clone(); let mut rng = rand::thread_rng(); - // Replace the existing receiver outputs - for (i, output) in self.original_psbt.unsigned_tx.output.iter().enumerate() { + // Substitute the existing receiver outputs, keeping the sender/receiver output ordering + for (i, original_output) in self.original_psbt.unsigned_tx.output.iter().enumerate() { if self.owned_vouts.contains(&i) { - // Receiver output: substitute with a provided output picked randomly + // Receiver output: substitute in-place a provided replacement output if replacement_outputs.is_empty() { return Err(InternalOutputSubstitutionError::NotEnoughOutputs.into()); } - let index = rng.gen_range(0..replacement_outputs.len()); - let txo = replacement_outputs.swap_remove(index); - if *drain_script == txo.script_pubkey { - change_vout = Some(i); + match replacement_outputs + .iter() + .position(|txo| txo.script_pubkey == original_output.script_pubkey) + { + // Select an output with the same address if one was provided + Some(pos) => { + let txo = replacement_outputs.swap_remove(pos); + if self.params.disable_output_substitution + && txo.value < original_output.value + { + return Err( + InternalOutputSubstitutionError::OutputSubstitutionDisabled( + "Decreasing the receiver output value is not allowed", + ) + .into(), + ); + } + outputs.push(txo); + } + // Otherwise randomly select one of the replacement outputs + None => { + if self.params.disable_output_substitution { + return Err( + InternalOutputSubstitutionError::OutputSubstitutionDisabled( + "Changing the receiver output script pubkey is not allowed", + ) + .into(), + ); + } + let index = rng.gen_range(0..replacement_outputs.len()); + let txo = replacement_outputs.swap_remove(index); + outputs.push(txo); + } } - outputs.push(txo); } else { // Sender output: leave it as is - outputs.push(output.clone()); - } - } - // Append all remaining outputs - while !replacement_outputs.is_empty() { - let index = rng.gen_range(0..replacement_outputs.len()); - let txo = replacement_outputs.swap_remove(index); - if *drain_script == txo.script_pubkey { - change_vout = Some(outputs.len()); + outputs.push(original_output.clone()); } - outputs.push(txo); - // Additional outputs must also be added to the PSBT outputs data structure - payjoin_psbt.outputs.push(Default::default()); } + // Insert all remaining outputs at random indices for privacy + interleave_shuffle(&mut outputs, &mut replacement_outputs, &mut rng); + // Identify the receiver output that will be used for change and fees + let change_vout = outputs.iter().position(|txo| txo.script_pubkey == *drain_script); + // Update the payjoin PSBT outputs + payjoin_psbt.outputs = vec![Default::default(); outputs.len()]; payjoin_psbt.unsigned_tx.output = outputs; Ok(WantsOutputs { original_psbt: self.original_psbt, @@ -460,6 +460,34 @@ impl WantsOutputs { } } +/// Shuffles `new` vector, then interleaves its elements with those from `original`, +/// maintaining the relative order in `original` but randomly inserting elements from `new`. +/// The combined result replaces the contents of `original`. +fn interleave_shuffle( + original: &mut Vec, + new: &mut Vec, + rng: &mut R, +) { + // Shuffle the substitute_outputs + new.shuffle(rng); + // Create a new vector to store the combined result + let mut combined = Vec::with_capacity(original.len() + new.len()); + // Initialize indices + let mut original_index = 0; + let mut new_index = 0; + // Interleave elements + while original_index < original.len() || new_index < new.len() { + if original_index < original.len() && (new_index >= new.len() || rng.gen_bool(0.5)) { + combined.push(original[original_index].clone()); + original_index += 1; + } else { + combined.push(new[new_index].clone()); + new_index += 1; + } + } + *original = combined; +} + /// A checked proposal that the receiver may contribute inputs to to make a payjoin #[derive(Debug, Clone)] pub struct WantsInputs { @@ -840,6 +868,9 @@ impl PayjoinProposal { #[cfg(test)] mod test { + use rand::rngs::StdRng; + use rand::SeedableRng; + use super::*; struct MockHeaders { @@ -915,4 +946,26 @@ mod test { assert!(payjoin.is_ok(), "Payjoin should be a valid PSBT"); } + + #[test] + fn test_interleave_shuffle() { + let mut original1 = vec![1, 2, 3]; + let mut original2 = original1.clone(); + let mut original3 = original1.clone(); + let mut new1 = vec![4, 5, 6]; + let mut new2 = new1.clone(); + let mut new3 = new1.clone(); + let mut rng1 = StdRng::seed_from_u64(123); + let mut rng2 = StdRng::seed_from_u64(234); + let mut rng3 = StdRng::seed_from_u64(345); + // Operate on the same data multiple times with different RNG seeds. + interleave_shuffle(&mut original1, &mut new1, &mut rng1); + interleave_shuffle(&mut original2, &mut new2, &mut rng2); + interleave_shuffle(&mut original3, &mut new3, &mut rng3); + // The result should be different for each seed + // and the relative ordering from `original` always preserved/ + assert_eq!(original1, vec![1, 6, 2, 5, 4, 3]); + assert_eq!(original2, vec![1, 5, 4, 2, 6, 3]); + assert_eq!(original3, vec![4, 5, 1, 2, 6, 3]); + } } diff --git a/payjoin/src/send/mod.rs b/payjoin/src/send/mod.rs index 5a19f6f7..69275f6b 100644 --- a/payjoin/src/send/mod.rs +++ b/payjoin/src/send/mod.rs @@ -846,7 +846,7 @@ impl ContextV1 { ensure!(proposed_txout.value >= original_output.value, OutputValueDecreased); original_outputs.next(); } - // all original outputs processed, only additional outputs remain + // additional output _ => (), } } From 59c3d271e278dafb0a41bcacbde8b3b0d4ec2738 Mon Sep 17 00:00:00 2001 From: spacebear Date: Thu, 5 Sep 2024 21:36:55 -0400 Subject: [PATCH 9/9] Allow receiver to specify a max feerate This ensures that the receiver does not pay more in fees than they would by building a separate transaction at max_feerate instead. That prevents a scenario where the sender specifies a `minfeerate` param that would require the receiver to pay more than they are willing to spend in fees. Privacy-conscious receivers should err on the side of indulgence with their preferred max feerate in order to satisfy their preference for privacy. This commit also adds a `max-feerate` receive option to payjoin-cli. --- payjoin-cli/src/app/config.rs | 11 ++- payjoin-cli/src/app/v1.rs | 5 +- payjoin-cli/src/app/v2.rs | 5 +- payjoin-cli/src/main.rs | 6 ++ payjoin/src/receive/error.rs | 10 +++ payjoin/src/receive/mod.rs | 122 +++++++++++++++++++++++++--------- payjoin/src/receive/v2/mod.rs | 7 +- payjoin/tests/integration.rs | 2 + 8 files changed, 134 insertions(+), 34 deletions(-) diff --git a/payjoin-cli/src/app/config.rs b/payjoin-cli/src/app/config.rs index d89ddf6c..5c275e1a 100644 --- a/payjoin-cli/src/app/config.rs +++ b/payjoin-cli/src/app/config.rs @@ -15,6 +15,8 @@ pub struct AppConfig { pub bitcoind_rpcuser: String, pub bitcoind_rpcpassword: String, pub db_path: PathBuf, + // receive-only + pub max_fee_rate: Option, // v2 only #[cfg(feature = "v2")] @@ -113,7 +115,14 @@ impl AppConfig { )? }; - builder + let max_fee_rate = matches + .get_one::("max_fee_rate") + .map(|max_fee_rate| max_fee_rate.parse::()) + .transpose() + .map_err(|_| { + ConfigError::Message("\"max_fee_rate\" must be a valid amount".to_string()) + })?; + builder.set_override_option("max_fee_rate", max_fee_rate)? } #[cfg(feature = "v2")] Some(("resume", _)) => builder, diff --git a/payjoin-cli/src/app/v1.rs b/payjoin-cli/src/app/v1.rs index a5205717..c7a20ca1 100644 --- a/payjoin-cli/src/app/v1.rs +++ b/payjoin-cli/src/app/v1.rs @@ -14,7 +14,7 @@ use hyper::service::service_fn; use hyper::{Method, Request, Response, StatusCode}; use hyper_util::rt::TokioIo; use payjoin::bitcoin::psbt::Psbt; -use payjoin::bitcoin::{self}; +use payjoin::bitcoin::{self, FeeRate}; use payjoin::receive::{PayjoinProposal, UncheckedProposal}; use payjoin::{Error, PjUriBuilder, Uri, UriExt}; use tokio::net::TcpListener; @@ -366,6 +366,9 @@ impl App { .map_err(|e| Error::Server(e.into()))? }, Some(bitcoin::FeeRate::MIN), + self.config.max_fee_rate.map_or(Ok(FeeRate::ZERO), |fee_rate| { + FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Server("Invalid fee rate".into())) + })?, )?; Ok(payjoin_proposal) } diff --git a/payjoin-cli/src/app/v2.rs b/payjoin-cli/src/app/v2.rs index 00be2cd7..c2a20be9 100644 --- a/payjoin-cli/src/app/v2.rs +++ b/payjoin-cli/src/app/v2.rs @@ -6,7 +6,7 @@ use anyhow::{anyhow, Context, Result}; use bitcoincore_rpc::RpcApi; use payjoin::bitcoin::consensus::encode::serialize_hex; use payjoin::bitcoin::psbt::Psbt; -use payjoin::bitcoin::Amount; +use payjoin::bitcoin::{Amount, FeeRate}; use payjoin::receive::v2::ActiveSession; use payjoin::send::RequestContext; use payjoin::{bitcoin, Error, Uri}; @@ -343,6 +343,9 @@ impl App { .map_err(|e| Error::Server(e.into()))? }, Some(bitcoin::FeeRate::MIN), + self.config.max_fee_rate.map_or(Ok(FeeRate::ZERO), |fee_rate| { + FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Server("Invalid fee rate".into())) + })?, )?; let payjoin_proposal_psbt = payjoin_proposal.psbt(); log::debug!("Receiver's Payjoin proposal PSBT Rsponse: {:#?}", payjoin_proposal_psbt); diff --git a/payjoin-cli/src/main.rs b/payjoin-cli/src/main.rs index deb50d7a..f14c9256 100644 --- a/payjoin-cli/src/main.rs +++ b/payjoin-cli/src/main.rs @@ -111,6 +111,12 @@ fn cli() -> ArgMatches { let mut cmd = cmd.subcommand(Command::new("resume")); // Conditional arguments based on features for the receive subcommand + receive_cmd = receive_cmd.arg( + Arg::new("max_fee_rate") + .long("max-fee-rate") + .num_args(1) + .help("The maximum effective fee rate the receiver is willing to pay (in sat/vB)"), + ); #[cfg(not(feature = "v2"))] { receive_cmd = receive_cmd.arg( diff --git a/payjoin/src/receive/error.rs b/payjoin/src/receive/error.rs index 07540f88..2056bcf8 100644 --- a/payjoin/src/receive/error.rs +++ b/payjoin/src/receive/error.rs @@ -90,6 +90,8 @@ pub(crate) enum InternalRequestError { /// /// Second argument is the minimum fee rate optionaly set by the receiver. PsbtBelowFeeRate(bitcoin::FeeRate, bitcoin::FeeRate), + /// Effective receiver feerate exceeds maximum allowed feerate + FeeTooHigh(bitcoin::FeeRate, bitcoin::FeeRate), } impl From for RequestError { @@ -172,6 +174,14 @@ impl fmt::Display for RequestError { original_psbt_fee_rate, receiver_min_fee_rate ), ), + InternalRequestError::FeeTooHigh(proposed_feerate, max_feerate) => write_error( + f, + "original-psbt-rejected", + &format!( + "Effective receiver feerate exceeds maximum allowed feerate: {} > {}", + proposed_feerate, max_feerate + ), + ), } } } diff --git a/payjoin/src/receive/mod.rs b/payjoin/src/receive/mod.rs index 23039684..44ce7a31 100644 --- a/payjoin/src/receive/mod.rs +++ b/payjoin/src/receive/mod.rs @@ -672,7 +672,15 @@ pub struct ProvisionalProposal { impl ProvisionalProposal { /// Apply additional fee contribution now that the receiver has contributed input /// this is kind of a "build_proposal" step before we sign and finalize and extract - fn apply_fee(&mut self, min_feerate: Option) -> Result<&Psbt, RequestError> { + /// + /// max_feerate is the maximum effective feerate that the receiver is willing to pay for their + /// own input/output contributions. A max_feerate of zero indicates that the receiver is not + /// willing to pay any additional fees. + fn apply_fee( + &mut self, + min_feerate: Option, + max_feerate: FeeRate, + ) -> Result<&Psbt, RequestError> { let min_feerate = min_feerate.unwrap_or(FeeRate::MIN); log::trace!("min_feerate: {:?}", min_feerate); log::trace!("params.min_feerate: {:?}", self.params.min_feerate); @@ -682,13 +690,15 @@ impl ProvisionalProposal { // If the sender specified a fee contribution, the receiver is allowed to decrease the // sender's fee output to pay for additional input fees. Any fees in excess of // `max_additional_fee_contribution` must be covered by the receiver. - let additional_fee = self.additional_input_fee(min_feerate)?; + let input_contribution_weight = self.additional_input_weight()?; + let additional_fee = input_contribution_weight * min_feerate; + log::trace!("additional_fee: {}", additional_fee); + let mut receiver_additional_fee = additional_fee; if additional_fee > Amount::ZERO { log::trace!( "self.params.additional_fee_contribution: {:?}", self.params.additional_fee_contribution ); - let mut receiver_additional_fee = additional_fee; if let Some((max_additional_fee_contribution, additional_fee_output_index)) = self.params.additional_fee_contribution { @@ -713,28 +723,31 @@ impl ProvisionalProposal { sender_additional_fee; receiver_additional_fee -= sender_additional_fee; } - - // The receiver covers any additional fees if applicable - if receiver_additional_fee > Amount::ZERO { - log::trace!("receiver_additional_fee: {}", receiver_additional_fee); - // Remove additional miner fee from the receiver's specified output - self.payjoin_psbt.unsigned_tx.output[self.change_vout].value -= - receiver_additional_fee; - } } // The sender's fee contribution can only be used to pay for additional input weight, so // any additional outputs must be paid for by the receiver. - let additional_receiver_fee = self.additional_output_fee(min_feerate); - if additional_receiver_fee > Amount::ZERO { + let output_contribution_weight = self.additional_output_weight(); + receiver_additional_fee += output_contribution_weight * min_feerate; + log::trace!("receiver_additional_fee: {}", receiver_additional_fee); + // Ensure that the receiver does not pay more in fees + // than they would by building a separate transaction at max_feerate instead. + let max_fee = (input_contribution_weight + output_contribution_weight) * max_feerate; + log::trace!("max_fee: {}", max_fee); + if receiver_additional_fee > max_fee { + let proposed_feerate = + receiver_additional_fee / (input_contribution_weight + output_contribution_weight); + return Err(InternalRequestError::FeeTooHigh(proposed_feerate, max_feerate).into()); + } + if receiver_additional_fee > Amount::ZERO { // Remove additional miner fee from the receiver's specified output - self.payjoin_psbt.unsigned_tx.output[self.change_vout].value -= additional_receiver_fee; + self.payjoin_psbt.unsigned_tx.output[self.change_vout].value -= receiver_additional_fee; } Ok(&self.payjoin_psbt) } - /// Calculate the additional fee required to pay for any additional inputs in the payjoin tx - fn additional_input_fee(&self, min_feerate: FeeRate) -> Result { + /// 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 @@ -751,13 +764,11 @@ impl ProvisionalProposal { log::trace!("weight_per_input : {}", weight_per_input); let contribution_weight = weight_per_input * input_count as u64; log::trace!("contribution_weight: {}", contribution_weight); - let additional_fee = contribution_weight * min_feerate; - log::trace!("additional_fee: {}", additional_fee); - Ok(additional_fee) + Ok(contribution_weight) } - /// Calculate the additional fee required to pay for any additional outputs in the payjoin tx - fn additional_output_fee(&self, min_feerate: FeeRate) -> Amount { + /// Calculate the additional output weight contributed by the receiver + fn additional_output_weight(&self) -> Weight { let payjoin_outputs_weight = self .payjoin_psbt .unsigned_tx @@ -772,9 +783,7 @@ impl ProvisionalProposal { .fold(Weight::ZERO, |acc, txo| acc + txo.weight()); let output_contribution_weight = payjoin_outputs_weight - original_outputs_weight; log::trace!("output_contribution_weight : {}", output_contribution_weight); - let additional_fee = output_contribution_weight * min_feerate; - log::trace!("additional_fee: {}", additional_fee); - additional_fee + output_contribution_weight } /// Return a Payjoin Proposal PSBT that the sender will find acceptable. @@ -833,6 +842,7 @@ impl ProvisionalProposal { mut self, wallet_process_psbt: impl Fn(&Psbt) -> Result, min_feerate_sat_per_vb: Option, + max_feerate_sat_per_vb: FeeRate, ) -> Result { for i in self.sender_input_indexes() { log::trace!("Clearing sender script signatures for input {}", i); @@ -840,7 +850,7 @@ impl ProvisionalProposal { self.payjoin_psbt.inputs[i].final_script_witness = None; self.payjoin_psbt.inputs[i].tap_key_sig = None; } - let psbt = self.apply_fee(min_feerate_sat_per_vb)?; + let psbt = self.apply_fee(min_feerate_sat_per_vb, max_feerate_sat_per_vb)?; let psbt = wallet_process_psbt(psbt)?; let payjoin_proposal = self.prepare_psbt(psbt)?; Ok(payjoin_proposal) @@ -868,6 +878,10 @@ impl PayjoinProposal { #[cfg(test)] mod test { + use std::str::FromStr; + + use bitcoin::hashes::Hash; + use bitcoin::{Address, Network, ScriptBuf}; use rand::rngs::StdRng; use rand::SeedableRng; @@ -916,10 +930,6 @@ mod test { #[test] fn unchecked_proposal_unlocks_after_checks() { - use std::str::FromStr; - - use bitcoin::{Address, Network}; - let proposal = proposal_from_test_vector().unwrap(); assert_eq!(proposal.psbt_fee_rate().unwrap().to_sat_per_vb_floor(), 2); let mut payjoin = proposal @@ -942,11 +952,63 @@ mod test { .commit_outputs() .commit_inputs(); - let payjoin = payjoin.apply_fee(None); + let payjoin = payjoin.apply_fee(None, FeeRate::ZERO); assert!(payjoin.is_ok(), "Payjoin should be a valid PSBT"); } + #[test] + fn sender_specifies_excessive_feerate() { + let mut proposal = proposal_from_test_vector().unwrap(); + assert_eq!(proposal.psbt_fee_rate().unwrap().to_sat_per_vb_floor(), 2); + // 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 mut payjoin = proposal + .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| { + let network = Network::Bitcoin; + Ok(Address::from_script(script, network).unwrap() + == Address::from_str(&"3CZZi7aWFugaCdUCS15dgrUUViupmB8bVM") + .unwrap() + .require_network(network) + .unwrap()) + }) + .expect("Receiver output should be identified") + .commit_outputs() + .contribute_witness_inputs(vec![input]) + .expect("Failed to contribute inputs") + .commit_inputs(); + let mut payjoin_clone = payjoin.clone(); + let psbt = payjoin.apply_fee(None, FeeRate::from_sat_per_vb_unchecked(1000)); + assert!(psbt.is_ok(), "Payjoin should be a valid PSBT"); + let psbt = payjoin_clone.apply_fee(None, FeeRate::from_sat_per_vb_unchecked(995)); + assert!(psbt.is_err(), "Payjoin exceeds receiver fee preference and should error"); + } + #[test] fn test_interleave_shuffle() { let mut original1 = vec![1, 2, 3]; diff --git a/payjoin/src/receive/v2/mod.rs b/payjoin/src/receive/v2/mod.rs index a7482450..4ffbbb6f 100644 --- a/payjoin/src/receive/v2/mod.rs +++ b/payjoin/src/receive/v2/mod.rs @@ -492,8 +492,13 @@ impl ProvisionalProposal { self, wallet_process_psbt: impl Fn(&Psbt) -> Result, min_feerate_sat_per_vb: Option, + max_feerate_sat_per_vb: FeeRate, ) -> Result { - let inner = self.inner.finalize_proposal(wallet_process_psbt, min_feerate_sat_per_vb)?; + let inner = self.inner.finalize_proposal( + wallet_process_psbt, + min_feerate_sat_per_vb, + max_feerate_sat_per_vb, + )?; Ok(PayjoinProposal { inner, context: self.context }) } } diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index eaab38ea..6ed9292e 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -665,6 +665,7 @@ mod integration { .unwrap()) }, Some(FeeRate::BROADCAST_MIN), + FeeRate::from_sat_per_vb_unchecked(2), ) .unwrap(); payjoin_proposal @@ -1123,6 +1124,7 @@ mod integration { .unwrap()) }, Some(FeeRate::BROADCAST_MIN), + FeeRate::from_sat_per_vb_unchecked(2), ) .unwrap(); payjoin_proposal