From c7b5c171a9af84ff52d2e4512391537e37eb490f Mon Sep 17 00:00:00 2001 From: spacebear Date: Tue, 13 Aug 2024 14:18:12 -0400 Subject: [PATCH 1/3] Add amount checks to integration tests At the end of each integration test, check the resulting transaction inputs/outputs, and the resulting balances for the sender and receiver. Note that the sender sweep test case requires the original PSBT fee rate to be high enough to cover the receiver input, because neither the sender nor the receiver can contribute additional fees in the payjoin PSBT due to current API limitations. --- payjoin/tests/integration.rs | 102 +++++++++++++++++++++++++++++------ 1 file changed, 86 insertions(+), 16 deletions(-) diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index df50b898..3dfd0795 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -5,7 +5,7 @@ mod integration { use std::str::FromStr; use bitcoin::psbt::Psbt; - use bitcoin::{Amount, FeeRate, OutPoint}; + use bitcoin::{Amount, FeeRate, OutPoint, Weight}; use bitcoind::bitcoincore_rpc::json::{AddressType, WalletProcessPsbtResult}; use bitcoind::bitcoincore_rpc::{self, RpcApi}; use log::{log_enabled, Level}; @@ -21,6 +21,14 @@ mod integration { static EXAMPLE_URL: Lazy = Lazy::new(|| Url::parse("https://example.com").expect("Invalid Url")); + // See https://jlopp.github.io/bitcoin-transaction-size-calculator/ + // Base weight of an empty transaction (no inputs, no outputs) + static BASE_WEIGHT: Weight = Weight::from_vb_unchecked(11); + // Per-input weight for P2WPKH inputs + static INPUT_WEIGHT: Weight = Weight::from_vb_unchecked(68); + // Per-output weight for P2WPKH inputs + static OUTPUT_WEIGHT: Weight = Weight::from_vb_unchecked(31); + #[cfg(not(feature = "v2"))] mod v1 { use log::debug; @@ -39,6 +47,8 @@ mod integration { .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() @@ -55,16 +65,28 @@ 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); // 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 input_count = payjoin_tx.input.len() as u64; + let output_count = payjoin_tx.output.len() as u64; + let tx_weight = BASE_WEIGHT + input_count * INPUT_WEIGHT + output_count * OUTPUT_WEIGHT; + let network_fees = tx_weight * FeeRate::BROADCAST_MIN; + assert_eq!(input_count, 2); + assert_eq!(output_count, 2); + assert_eq!(receiver.get_balances()?.mine.untrusted_pending, Amount::from_btc(51.0)?); + assert_eq!( + sender.get_balances()?.mine.untrusted_pending, + Amount::from_btc(49.0)? - network_fees + ); Ok(()) } } @@ -264,7 +286,7 @@ mod integration { .unwrap(); let psbt = build_sweep_psbt(&sender, &pj_uri)?; let mut req_ctx = RequestBuilder::from_psbt_and_uri(psbt.clone(), pj_uri.clone())? - .build_recommended(payjoin::bitcoin::FeeRate::BROADCAST_MIN)?; + .build_recommended(FeeRate::BROADCAST_MIN)?; let (Request { url, body, content_type, .. }, send_ctx) = req_ctx.extract_v2(directory.to_owned())?; let response = agent @@ -290,7 +312,7 @@ mod integration { // POST payjoin let proposal = session.process_res(response.bytes().await?.to_vec().as_slice(), ctx)?.unwrap(); - let mut payjoin_proposal = handle_directory_proposal(receiver, proposal); + let mut payjoin_proposal = handle_directory_proposal(&receiver, proposal); assert!(!payjoin_proposal.is_output_substitution_disabled()); let (req, ctx) = payjoin_proposal.extract_v2_req()?; let response = agent.post(req.url).body(req.body).send().await?; @@ -300,7 +322,6 @@ mod integration { // ********************** // Inside the Sender: // Sender checks, signs, finalizes, extracts, and broadcasts - // Replay post fallback to get the response let (Request { url, body, .. }, send_ctx) = req_ctx.extract_v2(directory.to_owned())?; @@ -311,6 +332,24 @@ mod integration { let payjoin_tx = extract_pj_tx(&sender, checked_payjoin_proposal_psbt)?; sender.send_raw_transaction(&payjoin_tx)?; log::info!("sent"); + + // Check resulting transaction and balances + let input_count = payjoin_tx.input.len() as u64; + let output_count = payjoin_tx.output.len() as u64; + let tx_weight = + BASE_WEIGHT + input_count * INPUT_WEIGHT + output_count * OUTPUT_WEIGHT; + // NOTE: No one is contributing fees for the receiver input because the sender has + // no change output and the receiver doesn't contribute fees + let network_fees = + (tx_weight - 1 * INPUT_WEIGHT) * FeeRate::from_sat_per_vb_unchecked(2); + // Sender sent the entire value of their utxo to receiver (minus fees) + assert_eq!(input_count, 2); + assert_eq!(output_count, 1); + assert_eq!( + receiver.get_balances()?.mine.untrusted_pending, + Amount::from_btc(100.0)? - network_fees + ); + assert_eq!(sender.get_balances()?.mine.untrusted_pending, Amount::from_btc(0.0)?); Ok(()) } } @@ -335,24 +374,36 @@ mod integration { .unwrap(); let psbt = build_original_psbt(&sender, &pj_uri)?; let mut req_ctx = RequestBuilder::from_psbt_and_uri(psbt.clone(), pj_uri.clone())? - .build_recommended(payjoin::bitcoin::FeeRate::BROADCAST_MIN)?; + .build_recommended(FeeRate::BROADCAST_MIN)?; let (req, ctx) = req_ctx.extract_v2(EXAMPLE_URL.to_owned())?; let headers = HeaderMock::new(&req.body, req.content_type); // ********************** // Inside the Receiver: // this data would transit from one party to another over the network in production - let response = handle_v1_pj_request(req, headers, receiver); + let response = handle_v1_pj_request(req, headers, &receiver); // 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())?.unwrap(); let payjoin_tx = extract_pj_tx(&sender, checked_payjoin_proposal_psbt)?; sender.send_raw_transaction(&payjoin_tx)?; + + // Check resulting transaction and balances + let input_count = payjoin_tx.input.len() as u64; + let output_count = payjoin_tx.output.len() as u64; + let tx_weight = BASE_WEIGHT + input_count * INPUT_WEIGHT + output_count * OUTPUT_WEIGHT; + let network_fees = tx_weight * FeeRate::BROADCAST_MIN; + assert_eq!(input_count, 2); + assert_eq!(output_count, 2); + assert_eq!(receiver.get_balances()?.mine.untrusted_pending, Amount::from_btc(51.0)?); + assert_eq!( + sender.get_balances()?.mine.untrusted_pending, + Amount::from_btc(49.0)? - network_fees + ); Ok(()) } @@ -427,6 +478,8 @@ mod integration { // ********************** // Inside the Receiver: let agent_clone: Arc = agent.clone(); + let receiver: Arc = Arc::new(receiver); + let receiver_clone = receiver.clone(); let receiver_loop = tokio::task::spawn(async move { let agent_clone = agent_clone.clone(); let (response, ctx) = loop { @@ -446,7 +499,7 @@ mod integration { } }; let proposal = session.process_res(response.as_slice(), ctx).unwrap().unwrap(); - let mut payjoin_proposal = handle_directory_proposal(receiver, proposal); + let mut payjoin_proposal = handle_directory_proposal(&receiver_clone, proposal); assert!(payjoin_proposal.is_output_substitution_disabled()); // Respond with payjoin psbt within the time window the sender is willing to wait // this response would be returned as http response to the sender @@ -476,6 +529,23 @@ mod integration { receiver_loop.await.is_ok(), "The spawned task panicked or returned an error" ); + + // Check resulting transaction and balances + let input_count = payjoin_tx.input.len() as u64; + let output_count = payjoin_tx.output.len() as u64; + let tx_weight = + BASE_WEIGHT + input_count * INPUT_WEIGHT + output_count * OUTPUT_WEIGHT; + let network_fees = tx_weight * FeeRate::BROADCAST_MIN; + assert_eq!(input_count, 2); + assert_eq!(output_count, 2); + assert_eq!( + receiver.get_balances()?.mine.untrusted_pending, + Amount::from_btc(51.0)? + ); + assert_eq!( + sender.get_balances()?.mine.untrusted_pending, + Amount::from_btc(49.0)? - network_fees + ); Ok(()) } } @@ -535,7 +605,7 @@ mod integration { } fn handle_directory_proposal( - receiver: bitcoincore_rpc::Client, + receiver: &bitcoincore_rpc::Client, proposal: UncheckedProposal, ) -> PayjoinProposal { // in a payment processor where the sender could go offline, this is where you schedule to broadcast the original_tx @@ -618,7 +688,7 @@ mod integration { }) .unwrap()) }, - Some(bitcoin::FeeRate::MIN), + Some(FeeRate::BROADCAST_MIN), ) .unwrap(); payjoin_proposal @@ -740,7 +810,7 @@ mod integration { outputs.insert(pj_uri.address.to_string(), pj_uri.amount.unwrap_or(Amount::ONE_BTC)); let options = bitcoincore_rpc::json::WalletCreateFundedPsbtOptions { lock_unspent: Some(true), - fee_rate: Some(Amount::from_sat(2000)), + fee_rate: Some(Amount::from_sat(1000)), ..Default::default() }; let psbt = sender @@ -761,7 +831,7 @@ mod integration { fn handle_v1_pj_request( req: Request, headers: impl payjoin::receive::Headers, - receiver: bitcoincore_rpc::Client, + receiver: &bitcoincore_rpc::Client, ) -> String { // Receiver receive payjoin proposal, IRL it will be an HTTP request (over ssl or onion) let proposal = payjoin::receive::UncheckedProposal::from_request( @@ -779,7 +849,7 @@ mod integration { fn handle_proposal( proposal: payjoin::receive::UncheckedProposal, - receiver: bitcoincore_rpc::Client, + receiver: &bitcoincore_rpc::Client, ) -> 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(); @@ -861,7 +931,7 @@ mod integration { }) .unwrap()) }, - Some(bitcoin::FeeRate::MIN), + Some(FeeRate::BROADCAST_MIN), ) .unwrap(); payjoin_proposal From 7391636536cec40e0b5f0b303695da9915e44dfa Mon Sep 17 00:00:00 2001 From: DanGould Date: Wed, 21 Aug 2024 12:51:06 -0400 Subject: [PATCH 2/3] Explain why MIN_RELAY_TX_FEE is used --- payjoin/tests/integration.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 3dfd0795..57be5fb9 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -5,6 +5,7 @@ mod integration { use std::str::FromStr; use bitcoin::psbt::Psbt; + use bitcoin::policy::DEFAULT_MIN_RELAY_TX_FEE; use bitcoin::{Amount, FeeRate, OutPoint, Weight}; use bitcoind::bitcoincore_rpc::json::{AddressType, WalletProcessPsbtResult}; use bitcoind::bitcoincore_rpc::{self, RpcApi}; @@ -741,7 +742,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), - fee_rate: Some(Amount::from_sat(2000)), + // 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())), subtract_fee_from_outputs: vec![0], ..Default::default() }; @@ -810,7 +813,9 @@ mod integration { outputs.insert(pj_uri.address.to_string(), pj_uri.amount.unwrap_or(Amount::ONE_BTC)); let options = bitcoincore_rpc::json::WalletCreateFundedPsbtOptions { lock_unspent: Some(true), - fee_rate: Some(Amount::from_sat(1000)), + // 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())), ..Default::default() }; let psbt = sender From f1401cec9b1bd0f99a0d71a9b7271d3bb41ddcc2 Mon Sep 17 00:00:00 2001 From: spacebear Date: Wed, 21 Aug 2024 23:51:29 -0400 Subject: [PATCH 3/3] Use the rust-bitcoin fee calculator instead of hardcoded estimates --- payjoin/tests/integration.rs | 59 ++++++++++++++---------------------- 1 file changed, 22 insertions(+), 37 deletions(-) diff --git a/payjoin/tests/integration.rs b/payjoin/tests/integration.rs index 57be5fb9..a46c9032 100644 --- a/payjoin/tests/integration.rs +++ b/payjoin/tests/integration.rs @@ -4,8 +4,8 @@ mod integration { use std::env; use std::str::FromStr; - use bitcoin::psbt::Psbt; use bitcoin::policy::DEFAULT_MIN_RELAY_TX_FEE; + use bitcoin::psbt::Psbt; use bitcoin::{Amount, FeeRate, OutPoint, Weight}; use bitcoind::bitcoincore_rpc::json::{AddressType, WalletProcessPsbtResult}; use bitcoind::bitcoincore_rpc::{self, RpcApi}; @@ -22,14 +22,6 @@ mod integration { static EXAMPLE_URL: Lazy = Lazy::new(|| Url::parse("https://example.com").expect("Invalid Url")); - // See https://jlopp.github.io/bitcoin-transaction-size-calculator/ - // Base weight of an empty transaction (no inputs, no outputs) - static BASE_WEIGHT: Weight = Weight::from_vb_unchecked(11); - // Per-input weight for P2WPKH inputs - static INPUT_WEIGHT: Weight = Weight::from_vb_unchecked(68); - // Per-output weight for P2WPKH inputs - static OUTPUT_WEIGHT: Weight = Weight::from_vb_unchecked(31); - #[cfg(not(feature = "v2"))] mod v1 { use log::debug; @@ -77,12 +69,9 @@ mod integration { sender.send_raw_transaction(&payjoin_tx)?; // Check resulting transaction and balances - let input_count = payjoin_tx.input.len() as u64; - let output_count = payjoin_tx.output.len() as u64; - let tx_weight = BASE_WEIGHT + input_count * INPUT_WEIGHT + output_count * OUTPUT_WEIGHT; - let network_fees = tx_weight * FeeRate::BROADCAST_MIN; - assert_eq!(input_count, 2); - assert_eq!(output_count, 2); + let network_fees = predicted_tx_weight(&payjoin_tx) * FeeRate::BROADCAST_MIN; + assert_eq!(payjoin_tx.input.len(), 2); + assert_eq!(payjoin_tx.output.len(), 2); assert_eq!(receiver.get_balances()?.mine.untrusted_pending, Amount::from_btc(51.0)?); assert_eq!( sender.get_balances()?.mine.untrusted_pending, @@ -335,17 +324,13 @@ mod integration { log::info!("sent"); // Check resulting transaction and balances - let input_count = payjoin_tx.input.len() as u64; - let output_count = payjoin_tx.output.len() as u64; - let tx_weight = - BASE_WEIGHT + input_count * INPUT_WEIGHT + output_count * OUTPUT_WEIGHT; // NOTE: No one is contributing fees for the receiver input because the sender has - // no change output and the receiver doesn't contribute fees - let network_fees = - (tx_weight - 1 * INPUT_WEIGHT) * FeeRate::from_sat_per_vb_unchecked(2); + // 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()?; // Sender sent the entire value of their utxo to receiver (minus fees) - assert_eq!(input_count, 2); - assert_eq!(output_count, 1); + assert_eq!(payjoin_tx.input.len(), 2); + assert_eq!(payjoin_tx.output.len(), 1); assert_eq!( receiver.get_balances()?.mine.untrusted_pending, Amount::from_btc(100.0)? - network_fees @@ -394,12 +379,9 @@ mod integration { sender.send_raw_transaction(&payjoin_tx)?; // Check resulting transaction and balances - let input_count = payjoin_tx.input.len() as u64; - let output_count = payjoin_tx.output.len() as u64; - let tx_weight = BASE_WEIGHT + input_count * INPUT_WEIGHT + output_count * OUTPUT_WEIGHT; - let network_fees = tx_weight * FeeRate::BROADCAST_MIN; - assert_eq!(input_count, 2); - assert_eq!(output_count, 2); + let network_fees = predicted_tx_weight(&payjoin_tx) * FeeRate::BROADCAST_MIN; + assert_eq!(payjoin_tx.input.len(), 2); + assert_eq!(payjoin_tx.output.len(), 2); assert_eq!(receiver.get_balances()?.mine.untrusted_pending, Amount::from_btc(51.0)?); assert_eq!( sender.get_balances()?.mine.untrusted_pending, @@ -532,13 +514,9 @@ mod integration { ); // Check resulting transaction and balances - let input_count = payjoin_tx.input.len() as u64; - let output_count = payjoin_tx.output.len() as u64; - let tx_weight = - BASE_WEIGHT + input_count * INPUT_WEIGHT + output_count * OUTPUT_WEIGHT; - let network_fees = tx_weight * FeeRate::BROADCAST_MIN; - assert_eq!(input_count, 2); - assert_eq!(output_count, 2); + let network_fees = predicted_tx_weight(&payjoin_tx) * FeeRate::BROADCAST_MIN; + assert_eq!(payjoin_tx.input.len(), 2); + assert_eq!(payjoin_tx.output.len(), 2); assert_eq!( receiver.get_balances()?.mine.untrusted_pending, Amount::from_btc(51.0)? @@ -954,6 +932,13 @@ mod integration { Ok(payjoin_psbt.extract_tx()?) } + fn predicted_tx_weight(tx: &bitcoin::Transaction) -> Weight { + bitcoin::transaction::predict_weight( + vec![bitcoin::transaction::InputWeightPrediction::P2WPKH_MAX; tx.input.len()], + tx.script_pubkey_lens(), + ) + } + struct HeaderMock(HashMap); impl payjoin::receive::Headers for HeaderMock {