Fix `pjos` handling in v2 receiver and sender #847

spacebear21 · 2025-07-02T18:14:49Z

This fixes #843 and ensures the desired behavior is tested explicitly.

Notably, assert that the receiver's output substitution preference is respected.

coveralls · 2025-07-02T18:17:52Z

Pull Request Test Coverage Report for Build 16036827797

Details

82 of 85 (96.47%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 85.075%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/receive/v2/mod.rs	0	1	0.0%
payjoin/src/core/send/v2/mod.rs	48	50	96.0%

Totals
Change from base Build 16036284870:	0.2%
Covered Lines:	7302
Relevant Lines:	8583

💛 - Coveralls

spacebear21 · 2025-07-02T18:19:20Z

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

+        // V2 senders may always ignore the receiver's `pjos` output substitution preference,
+        // because all communications with the receiver are end-to-end authenticated.
+        if self.0.output_substitution == OutputSubstitution::Enabled {
+            v1.output_substitution = OutputSubstitution::Enabled;
+        }


This is a bit of a hack but I couldn't think of another way to enforce this override without many breaking changes in the v1 SenderBuilder.

Additionally, this doesn't account for V2 senders that downgrade to V1 for a V1 receiver but as Dan said elsewhere "I don't really like that that's not just a v1 sender in the first place" - so maybe that should be handled in a follow-up?

Yeah IMO best to separate as much as possible, this feels like the kind of confusion that often arises with OO due to inheritance, where code reuse and subtyping relationships are at odds...

IMO it makes more sense to have separate logic for v2 than to have complicated interactions in the name of code reuse, so the right thing to me seems to first check the URI and determine which version of payjoin it is as an enum, where each branch corresponds to a separate SenderBuilder. This is more burdensome for app developers but i think it's an important distinction to make.

V2 receivers should always signal `pjos=0` to preserve backwards compatibility with V1 senders, since V1 communication is unauthenticated and goes through an untrusted payjoin directory. V2 senders may safely ignore this flag because all communications are end-to-end authenticated and allowing output substitution enables more interesting use cases for receivers. A V2 sender may still signal their own output substitution preference via the `disableoutputsubstitution` request param.

Per [BIP77](https://github.com/bitcoin/bips/blob/master/bip-0077.md#payjoin-uri): > Since BIP 78 payloads are neither encrypted nor authenticated, a directory used for backwards-compatible payloads is known as an "unsecured payjoin server" in BIP 78 parlance. Backwards-compatible receivers MUST disable output substitution by setting pjos=0 to prevent modification by a malicious directory. Note that pjos=0 is ignored by V2 senders per the previous commit.

nothingmuch

(this is from yesterday, i just noticed i neglected to post it, so it's out of date... i will return to this shortly sorry)

utACK, assuming the hackyness is going to addressed as followup. I'm concerned that without doing that something similar might crop up, as arguably the bug is the result of tech debt in the first place

nothingmuch · 2025-07-02T18:48:32Z

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

            Ok(inner) => inner,
            Err(e) => return MaybeBadInitInputsTransition::bad_init_inputs(e),
        };
+        // V2 senders may always ignore the receiver's `pjos` output substitution preference,


Suggested change

// V2 senders may always ignore the receiver's `pjos` output substitution preference,

// V2 senders interacting with V2 receivers may ignore the receiver's `pjos` output substitution preference,

"always" seems a bit too broad, and could be interpreted to mean v2 senders interacting with v1 receivers that explicitly disabled it unrelated to BIP 77's requirement

nothingmuch · 2025-07-02T18:50:08Z

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

+        // V2 senders may always ignore the receiver's `pjos` output substitution preference,
+        // because all communications with the receiver are end-to-end authenticated.
+        if self.0.output_substitution == OutputSubstitution::Enabled {
+            v1.output_substitution = OutputSubstitution::Enabled;
+        }


Yeah IMO best to separate as much as possible, this feels like the kind of confusion that often arises with OO due to inheritance, where code reuse and subtyping relationships are at odds...

IMO it makes more sense to have separate logic for v2 than to have complicated interactions in the name of code reuse, so the right thing to me seems to first check the URI and determine which version of payjoin it is as an enum, where each branch corresponds to a separate SenderBuilder. This is more burdensome for app developers but i think it's an important distinction to make.

nothingmuch · 2025-07-02T19:00:30Z

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

+            .build_recommended(FeeRate::BROADCAST_MIN)
+            .save(&NoopSessionPersister::default())
+            .expect("sender should succeed");
+        // v2 senders may always override the receiver's `pjos` parameter to enable output


the "always" phrasing invites even more ambiguity in this testing context, see above comment & suggestion

DanGould · 2025-07-03T15:16:05Z

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

+        // V2 senders may always ignore the receiver's `pjos` output substitution preference,
+        // because all communications with the receiver are end-to-end authenticated.
+        if self.0.output_substitution == OutputSubstitution::Enabled {
+            v1.output_substitution = OutputSubstitution::Enabled;
+        }


I have w similar concern as @nothingmuch regarding this override. yes it works to override v1 fields with v2 preferences but it might be more legible if the hierarchy of v1/v2 preferences were evaluated at the specific version's code and passed into a general function on sender::[v1::?]PsbtContext logic that took into account the net preference alone.

I had toyed around with tearing out PsbtContext from v1::Sender to fix #809 last week but couldn't justify the additional code. This change might change that balance in favor of the abstraction.

DanGould

I think we're taking on some tech debt with this one but it does seem to fix and test for problem it purports to address

ACK dee6890

DanGould · 2025-07-08T21:28:59Z

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

        min_fee_rate: FeeRate,
    ) -> MaybeBadInitInputsTransition<SessionEvent, Sender<WithReplyKey>, BuildSenderError> {
-        let v1 = match self.0.build_non_incentivizing(min_fee_rate) {
+        self.v2_sender_from_v1(self.0.clone().build_non_incentivizing(min_fee_rate))


These self.0.clone() smell like we're taking on tech debt but I couldn't find an easy solution in my 15 minutes of play with it.

DanGould · 2025-07-13T19:31:02Z

I'm gonna merge this and address the issues in a follow up re: #809 and doing something like #728 for v1

Once the v1 / v2 have separate PsbtContexts I think this'll be cleaned up adequately.

benalleng · 2025-07-14T15:52:13Z

payjoin/src/core/send/v1.rs

-        )
-        .build_recommended(FeeRate::MIN);
-        assert!(sender.is_ok(), "{:#?}", sender.err());
-        assert_eq!(NON_WITNESS_INPUT_WEIGHT, bitcoin::Weight::from_wu(160));


I missed the removal of this assert, we need it to catch mutants on the logic for this const.

This just prevents any mutants from being formed on the operands adding the index and sequence

I see, sorry for removing it but it seemed unrelated to that specific test. #875 presents a good opportunity to add this back in an appropriate unit test.

`{v1,v2,multiparty}::Sender{,Builder}` types have been built off of a heirarchy where multiparty:: has a v2:: and v2:: has a v1::, which was done for quick convenience and not because that relationship actually makes functional sense. This PR is the first in a few which I believe are necessary to rectify this distinction. It does so by drawing the separation between the sender's `PsbtContext` checks / response processing and the version-specific serialization for networked messaging. However, I don't think it goes far enough. For one, it only really rectifies this issue between v1 and v2. Multiparty is left abstract over v2. Second, There are still distinct SenderBuilders that can't tell whether or not they're handling a v1 or v2 URIs. Since the information necessary to distinguish between a v1/v2 URI is in the URI itself, it seems that ought to be the first order of business for the sender to do even before calling `SenderBuilder::new`. The lack of this distinction leads to a [problem](bitcoindevkit/bdk-cli#200 (comment)) with a hacky [solution](bitcoindevkit/bdk-cli#200 (comment)) where downstream users need to wait all the way until they attempt to create a v2 request and handle an error there in order to figure out the version. The `SenderBuilder` also ought to behave differently for each version, and I'm not sure our current fix of #847 does this completely (Does a v2 SenderBuilder sending to v1 URI honor pjos? it should). In order to do so I reckon we could create an actual `PjUri` type, rather than an alias, that enumerates over the versions when `check_pj_supported` or its replacement is called. In order to do *that* effectively by making sure the correct parameters are there and we're not just switching on the presence of a fragment, I think `UrlExt` also needs to check for the parameter presence and validity. The other issue with v1::Sender flow is that it doesn't use the generic typestate machine pattern to match v2, which would be nice as well but out of the scope of this PR. re: #809

Closes #872, closes #836, closes #771, closes #890 This addressed #778 for now but I still would like to create a test instead of excluding it I also added the basic test that should address the mutation from #847 (comment) The exclusions are catogerized as follows. - timeouts - mutations that allow code to simply skip or follow a code path that does not actually cause or prevent an error - expiry time checks I found not worth creating a test for. - TODO exclusions

Closes #872, closes #836, closes #771, closes #890 This addressed #778 for now but I still would like to create a test instead of excluding it I also added the basic test that should address the mutation from payjoin/rust-payjoin#847 (comment) The exclusions are catogerized as follows. - timeouts - mutations that allow code to simply skip or follow a code path that does not actually cause or prevent an error - expiry time checks I found not worth creating a test for. - TODO exclusions

Add more assertions to v1 SenderBuilder tests

b9dfd24

Notably, assert that the receiver's output substitution preference is respected.

spacebear21 commented Jul 2, 2025

View reviewed changes

spacebear21 requested review from DanGould and nothingmuch July 2, 2025 18:19

nothingmuch mentioned this pull request Jul 2, 2025

Fix PR review issues from PR #812 #823

Closed

spacebear21 added 2 commits July 2, 2025 18:09

spacebear21 force-pushed the fix-pjos branch from ccde8c9 to dee6890 Compare July 2, 2025 22:09

nothingmuch reviewed Jul 3, 2025

View reviewed changes

DanGould reviewed Jul 3, 2025

View reviewed changes

DanGould approved these changes Jul 8, 2025

View reviewed changes

DanGould mentioned this pull request Jul 9, 2025

Fix send::PsbtContext / send::multiparty::PsbtContext discrepancy #863

Open

DanGould merged commit 145d1cf into payjoin:master Jul 13, 2025
8 checks passed

benalleng reviewed Jul 14, 2025

View reviewed changes

benalleng mentioned this pull request Jul 14, 2025

De-duplicate non-witness data weight const #875

Closed

DanGould mentioned this pull request Jul 16, 2025

Abstract common sender functionality, not heirarchy #884

Merged

This was referenced Jul 20, 2025

New Mutants Found #890

Closed

Cargo mutants version update #882

Merged

	// V2 senders may always ignore the receiver's `pjos` output substitution preference,
	// V2 senders interacting with V2 receivers may ignore the receiver's `pjos` output substitution preference,

Fix pjos handling in v2 receiver and sender #847

Fix pjos handling in v2 receiver and sender #847

Uh oh!

Conversation

spacebear21 commented Jul 2, 2025 • edited by DanGould Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 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 16036827797

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould commented Jul 13, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix `pjos` handling in v2 receiver and sender #847

Fix `pjos` handling in v2 receiver and sender #847

spacebear21 commented Jul 2, 2025 •

edited by DanGould

Loading

coveralls commented Jul 2, 2025 •

edited

Loading