Receiver<Monitor> integration tests for different script patterns and sender behaviors by mehmetefeumit · Pull Request #1211 · payjoin/rust-payjoin

mehmetefeumit · 2025-12-02T06:29:03Z

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

Closes #1137.

Overview

This PR adds:

Documentation for Receiver<Monitor> and the check_payment function.
Integration tests for the following cases:
- v2 P2WPKH and Taproot addresses. It checks if the correct confirmation function is used for each case, and that the final receiver persister state is what is expected.
- v2 sender fallback test. It checks whether the receiver can detect when the sender broadcasts the fallback transaction and that the final receiver persister state is what is expected.

Note that this PR does not implement tests for other wallet types or double-spend attack due to #1214. When the non-segwit monitoring is finalized, there should be more integration test scenarios to cover them.

coveralls · 2025-12-02T06:32:41Z

Pull Request Test Coverage Report for Build 19999836616

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 83.489%

Totals
Change from base Build 19938128275:	0.03%
Covered Lines:	9001
Relevant Lines:	10781

💛 - Coveralls

arminsabouri

I appreciate you keeping these commits clean! Did a first pass, approach looks good. Hand a handful of pointers for reducing the code changes in this PR and a few nits.

payjoin/src/core/receive/v2/mod.rs

payjoin/tests/integration.rs

arminsabouri · 2025-12-02T21:19:40Z

payjoin/tests/integration.rs

-                    |_| {
-                        panic!("We should never check outpoints since the payjoin tx has been broadcasted")
+                    |outpoint| {
+                        match address_type {


Do we need a match on address type?

It is an additional validation for the order of operations in the check_payment function. Since it should return Success if the TXID is found, this closure should never be run. This validates that that is the case.

I understand now, thanks. We could use a comment to explain this. Same thing for getting the tx via txid. e.g "non-segwit inputs will have a different txid after sender signs"

payjoin/tests/integration.rs

arminsabouri · 2025-12-04T17:42:42Z

payjoin/tests/integration.rs

-                    |_| {
-                        panic!("We should never check outpoints since the payjoin tx has been broadcasted")
+                    |outpoint| {
+                        match address_type {


I understand now, thanks. We could use a comment to explain this. Same thing for getting the tx via txid. e.g "non-segwit inputs will have a different txid after sender signs"

arminsabouri · 2025-12-04T17:51:17Z

payjoin/tests/integration.rs

+                // broadcasting the fallback transaction (both of which the receiver is aware of)
+                // instead they broadcast another transaction which spends the senders outputs in a
+                // transaction the receiver has not seen before.
+                let double_spend_tx = extract_pj_tx(&sender, double_spend_psbt)?;


Unrelated to this PR but extract_pj_tx should probably be renamed to sign_and_finalize_psbt

arminsabouri · 2025-12-04T18:01:51Z

payjoin/tests/integration.rs

+                // transaction the receiver has not seen before, but using the outpoint which the
+                // sender included in the original proposal.
+                let mut double_spend_tx = psbt.clone().unsigned_tx;
+                double_spend_tx.output[0].value -= Amount::from_sat(1);


I am not sure this is actually emulating a double spend. This is the fallback tx with the output value slightly adjusted -- kinda emulating a RBF on a fallback tx. I was expecting a tx spending the same inputs to entirely different outputs. However, I still think there is some merit behind this test. If the sender does RBF the fallback, the receiver can still pick it up. In that case we may want to rename this test to something like: "RBF'd fallback" or "Output adjusted fallback".

In the current Monitor implementation, if the sender does RBF on a non-segwit fallback transaction, the receiver will pick it up as a double-spend (which I thought what double-spend in the context of a Payjoin session was while writing the PR 😅)

With that in mind, I do not think the source code is ready for this test at the moment. Or we can put a big TODO next to it to and keep it active so that it tests the eventual fix or re-design under #1214.

arminsabouri

cACK c14b2c1
Great work reducing the boilerplate and code de-duplication.

arminsabouri · 2025-12-09T14:14:18Z

payjoin/tests/integration.rs

+                    panic!("psbt should exist");
+                };
+
+            let broadcasted_transaction = match sender_final_action {


Nit:

Suggested change

let broadcasted_transaction = match sender_final_action {

let tx_to_broadcast = match sender_final_action {

spacebear21

ACK c14b2c1, nice work on the SenderFinalAction refactor!

arminsabouri self-requested a review December 2, 2025 19:00

arminsabouri requested changes Dec 2, 2025

View reviewed changes

mehmetefeumit force-pushed the receiver-monitor-tests branch from 38a4a1e to 2b0d2db Compare December 3, 2025 16:13

mehmetefeumit requested a review from arminsabouri December 4, 2025 03:26

spacebear21 reviewed Dec 4, 2025

View reviewed changes

payjoin/tests/integration.rs Outdated Show resolved Hide resolved

payjoin/tests/integration.rs Outdated Show resolved Hide resolved

arminsabouri reviewed Dec 4, 2025

View reviewed changes

Add documentation for the Receiver monitor function

41926c5

mehmetefeumit force-pushed the receiver-monitor-tests branch from 2b0d2db to 2d9d5d5 Compare December 7, 2025 05:41

mehmetefeumit requested a review from arminsabouri December 7, 2025 05:44

mehmetefeumit added 2 commits December 6, 2025 21:47

Add v2 tests for p2wpkh and taproot

05961de

Add v2 test for fallback transaction broadcast

c14b2c1

mehmetefeumit force-pushed the receiver-monitor-tests branch from 2d9d5d5 to c14b2c1 Compare December 7, 2025 05:50

mehmetefeumit requested a review from spacebear21 December 7, 2025 15:57

mehmetefeumit mentioned this pull request Dec 9, 2025

Skip check_payment in Receiver<Monitor> if the sender is using non-segwit address #1218

Merged

2 tasks

arminsabouri approved these changes Dec 9, 2025

View reviewed changes

spacebear21 approved these changes Dec 9, 2025

View reviewed changes

spacebear21 merged commit 84b9e7c into payjoin:master Dec 9, 2025
10 checks passed

	let broadcasted_transaction = match sender_final_action {
	let tx_to_broadcast = match sender_final_action {

Conversation

mehmetefeumit commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Uh oh!

coveralls commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19999836616

Details

💛 - Coveralls

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminsabouri Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

mehmetefeumit Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

arminsabouri Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminsabouri Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

arminsabouri Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

arminsabouri Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

mehmetefeumit Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

arminsabouri Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mehmetefeumit commented Dec 2, 2025 •

edited

Loading

coveralls commented Dec 2, 2025 •

edited

Loading

mehmetefeumit Dec 5, 2025 •

edited

Loading