Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add amount checks to integration tests #351

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Add amount checks to integration tests #351

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7b5c17
Add amount checks to integration tests
spacebear21 Aug 13, 2024
7391636
Explain why MIN_RELAY_TX_FEE is used
DanGould Aug 21, 2024
f1401ce
Use the rust-bitcoin fee calculator instead of hardcoded estimates
spacebear21 Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 92 additions & 17 deletions payjoin/tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ mod integration {
use std::str::FromStr;

use bitcoin::psbt::Psbt;
use bitcoin::{Amount, FeeRate, OutPoint};
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};
use log::{log_enabled, Level};
Expand All @@ -21,6 +22,14 @@ mod integration {
static EXAMPLE_URL: Lazy<Url> =
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;
Expand All @@ -39,6 +48,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()
Expand All @@ -55,16 +66,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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it could occasionally fail if a signature size happened to be below a calculated estimation. Even lopp's calculator suggests that the results are an upper bound, which means we should be assert!(balance >= amount - fees) instead of assert_eq. Could input amount be subtracted from output amount instead? Or is this test meant to check that the actual network fee matches an expected FeeRate?

I'm a bit confused also why manual calculation is done rather than using Transaction::weight()

However, it seems when weight() is used as follows the assertion is off by 1 or 2 sats.

let network_fees = BROADCAST_MIN * tx.weight();
assert_eq!(
    receiver.get_balances()?.mine.untrusted_pending,
    Amount::from_btc(100.0)? - network_fees
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a bit confused also why manual calculation is done rather than using Transaction::weight()

Simple answer is that I didn't think of that 🤦 That does seem like a better approach though if we can get it to pass.

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(())
}
}
Expand Down Expand Up @@ -264,7 +287,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
Expand All @@ -290,7 +313,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?;
Expand All @@ -300,7 +323,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())?;
Expand All @@ -311,6 +333,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(())
}
}
Expand All @@ -335,24 +375,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(())
}

Expand Down Expand Up @@ -427,6 +479,8 @@ mod integration {
// **********************
// Inside the Receiver:
let agent_clone: Arc<Client> = agent.clone();
let receiver: Arc<bitcoincore_rpc::Client> = 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 {
Expand All @@ -446,7 +500,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
Expand Down Expand Up @@ -476,6 +530,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(())
}
}
Expand Down Expand Up @@ -535,7 +606,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
Expand Down Expand Up @@ -618,7 +689,7 @@ mod integration {
})
.unwrap())
},
Some(bitcoin::FeeRate::MIN),
Some(FeeRate::BROADCAST_MIN),
)
.unwrap();
payjoin_proposal
Expand Down Expand Up @@ -671,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()
};
Expand Down Expand Up @@ -740,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(2000)),
// 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
Expand All @@ -761,7 +836,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(
Expand All @@ -779,7 +854,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();
Expand Down Expand Up @@ -861,7 +936,7 @@ mod integration {
})
.unwrap())
},
Some(bitcoin::FeeRate::MIN),
Some(FeeRate::BROADCAST_MIN),
)
.unwrap();
payjoin_proposal
Expand Down
Loading