Skip to content

Commit

Permalink
Allow receiver to specify a max feerate
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
spacebear21 committed Sep 11, 2024
1 parent ab9b8b8 commit 59c3d27
Show file tree
Hide file tree
Showing 8 changed files with 134 additions and 34 deletions.
11 changes: 10 additions & 1 deletion payjoin-cli/src/app/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<u64>,

// v2 only
#[cfg(feature = "v2")]
Expand Down Expand Up @@ -113,7 +115,14 @@ impl AppConfig {
)?
};

builder
let max_fee_rate = matches
.get_one::<String>("max_fee_rate")
.map(|max_fee_rate| max_fee_rate.parse::<u64>())
.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,
Expand Down
5 changes: 4 additions & 1 deletion payjoin-cli/src/app/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
}
Expand Down
5 changes: 4 additions & 1 deletion payjoin-cli/src/app/v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 6 additions & 0 deletions payjoin-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
10 changes: 10 additions & 0 deletions payjoin/src/receive/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<InternalRequestError> for RequestError {
Expand Down Expand Up @@ -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
),
),
}
}
}
Expand Down
122 changes: 92 additions & 30 deletions payjoin/src/receive/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<FeeRate>) -> 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<FeeRate>,
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);
Expand All @@ -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
{
Expand All @@ -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<Amount, RequestError> {
/// Calculate the additional input weight contributed by the receiver
fn additional_input_weight(&self) -> Result<Weight, RequestError> {
// This error should never happen. We check for at least one input in the constructor
let input_pair = self
.payjoin_psbt
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -833,14 +842,15 @@ impl ProvisionalProposal {
mut self,
wallet_process_psbt: impl Fn(&Psbt) -> Result<Psbt, Error>,
min_feerate_sat_per_vb: Option<FeeRate>,
max_feerate_sat_per_vb: FeeRate,
) -> Result<PayjoinProposal, Error> {
for i in self.sender_input_indexes() {
log::trace!("Clearing sender script signatures for input {}", i);
self.payjoin_psbt.inputs[i].final_script_sig = None;
self.payjoin_psbt.inputs[i].final_script_witness = None;
self.payjoin_psbt.inputs[i].tap_key_sig = None;
}
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)
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -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];
Expand Down
7 changes: 6 additions & 1 deletion payjoin/src/receive/v2/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -492,8 +492,13 @@ impl ProvisionalProposal {
self,
wallet_process_psbt: impl Fn(&Psbt) -> Result<Psbt, Error>,
min_feerate_sat_per_vb: Option<FeeRate>,
max_feerate_sat_per_vb: FeeRate,
) -> Result<PayjoinProposal, Error> {
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 })
}
}
Expand Down
2 changes: 2 additions & 0 deletions payjoin/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,7 @@ mod integration {
.unwrap())
},
Some(FeeRate::BROADCAST_MIN),
FeeRate::from_sat_per_vb_unchecked(2),
)
.unwrap();
payjoin_proposal
Expand Down Expand Up @@ -1123,6 +1124,7 @@ mod integration {
.unwrap())
},
Some(FeeRate::BROADCAST_MIN),
FeeRate::from_sat_per_vb_unchecked(2),
)
.unwrap();
payjoin_proposal
Expand Down

0 comments on commit 59c3d27

Please sign in to comment.