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.

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

Conversation

spacebear21
Copy link
Collaborator

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.

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.
Comment on lines 79 to 84
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.

@DanGould
Copy link
Contributor

Nightly changed from 1.77 to 1.78 so cargo fmt changed. grrr

@DanGould
Copy link
Contributor

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.

@spacebear21
Copy link
Collaborator Author

Ok let's hold off on merging, I'll take a stab at using rust-bitcoin fee calculations today.

@DanGould
Copy link
Contributor

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?

@spacebear21
Copy link
Collaborator Author

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.

Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

tACK f1401ce

Much prettier to use rust-bitcoin imo 👑

@@ -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()],
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@DanGould DanGould merged commit b14f179 into payjoin:master Aug 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants