Add amount checks to integration tests #351
Conversation
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
Outdated
| 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); |
There was a problem hiding this comment.
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
);There was a problem hiding this comment.
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.
|
Nightly changed from 1.77 to 1.78 so cargo fmt changed. grrr |
|
I'd rather we use rust-bitcoin's fee calculation instead of lopp's estimations, but I ran into some trouble when I tried. If you're confident this calculation is just a bit of repetition and the edge cases of variable signature size are palatable rather than tech debt I'm comfortable merging this request. If you see the issues I raised as problems, we should address them before this gets in. |
|
Ok let's hold off on merging, I'll take a stab at using rust-bitcoin fee calculations today. |
|
I believe the reason we want these tests is to make sure transaction construction doesn't regress. Is that correct? Has this caught bugs and enabled development in #332? |
|
Regression testing was the original reason for adding these checks, but I also like that it helps self-document what exactly is going on in each scenario. These changes were originally part of #332 but I pulled them out into a separate PR for independent review and to make #332 lighter. I've found them very helpful in testing cut-through scenarios, e.g. receiver forwards payment. The fee checks are especially useful when the sender and the receiver are both on the hook for fees. I ran these tests on a loop for ~an hour (187 iterations) and didn't get any errors, so I'm reasonably confident that they're not flaky. |
| @@ -879,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()], | |||
There was a problem hiding this comment.
I find it mildly irritating that we have to assume P2WPKH script since the tx primitive doesn't have prevout data, but I'm OK living with it. PSBTv2 fixes this.
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.
Originally added these changes to the tx-cut-through branch but pulled them out into a separate PR for easier review.